bug#36526: 26.2; Open diff at current location (feature request with suggested code)

bug#36526: 26.2; Open diff at current location (feature request with suggested code)

[Campbell Barton]

This is a feature suggestion which I've written example code for

`vc-root-diff` (or a new function), could jump to the closest line.

This is useful since you move to the context of the current line.

I've written code that does this,
it would be useful if it was built into emacs.

See: https://emacs.stackexchange.com/a/51439/2418

In GNU Emacs 26.2 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.8)

bug#36526: 26.2; Open diff at current location (feature request with suggested code)

[Stefan Kangas]

Campbell Barton <ideasman42@gmail.com> writes:

> This is a feature suggestion which I've written example code for
>
> `vc-root-diff` (or a new function), could jump to the closest line.
>
> This is useful since you move to the context of the current line.
>
> I've written code that does this,
> it would be useful if it was built into emacs.
>
> See: https://emacs.stackexchange.com/a/51439/2418

Thanks for the feature suggestion.

I haven't looked at your code, but if we decided to use it, it seems
like a non-trivial change and would require you to sign copyright
papers.  You can find more information about copyright assignment
here:

https://www.gnu.org/software/emacs/manual/html_node/emacs/Copyright-Assignment.html

Would you be willing to do that?

If the answer to that is yes, could you please also submit your code
as a patch?  You can find more information on how to do that in the
CONTRIBUTE file in the emacs repository.  That would make it easier to
review and test your suggested changes.

Best regards,
Stefan Kangas

bug#36526: 26.2; Open diff at current location (feature request with suggested code)

[Cambell Barton]

Hi Stefan, I don't mind doing the copyright assignment.

Would someone be able to check the code and see if it's going in the 
right direction?

I'm still not that experienced with elisp, so I'd rather not spend time 
to make a patch if the general approach isn't right.

On 2019-09-13 21:59, Stefan Kangas wrote:
> Campbell Barton <ideasman42@gmail.com> writes:
> 
>> This is a feature suggestion which I've written example code for
>>
>> `vc-root-diff` (or a new function), could jump to the closest line.
>>
>> This is useful since you move to the context of the current line.
>>
>> I've written code that does this,
>> it would be useful if it was built into emacs.
>>
>> See: https://emacs.stackexchange.com/a/51439/2418
> 
> Thanks for the feature suggestion.
> 
> I haven't looked at your code, but if we decided to use it, it seems
> like a non-trivial change and would require you to sign copyright
> papers.  You can find more information about copyright assignment
> here:
> 
> https://www.gnu.org/software/emacs/manual/html_node/emacs/Copyright-Assignment.html
> 
> Would you be willing to do that?
> 
> If the answer to that is yes, could you please also submit your code
> as a patch?  You can find more information on how to do that in the
> CONTRIBUTE file in the emacs repository.  That would make it easier to
> review and test your suggested changes.
> 
> Best regards,
> Stefan Kangas
> 

bug#36526: 26.2; Open diff at current location (feature request with suggested code)

[Stefan Kangas]

Cambell Barton <ideasman42@gmail.com> writes:

> Hi Stefan, I don't mind doing the copyright assignment.

Excellent.

> Would someone be able to check the code and see if it's going in the
> right direction?

OK, but see below.

> >> This is a feature suggestion which I've written example code for
> >>
> >> `vc-root-diff` (or a new function), could jump to the closest line.
> >>
> >> This is useful since you move to the context of the current line.

I'm not sure I understand what this code is supposed to do or how to
use it. Could you please give a recipe for how to test it?

Best regards,
Stefan Kangas

bug#36526: 26.2; Open diff at current location (feature request with suggested code)

[Stefan Kangas]

Stefan Kangas <stefan@marxist.se> writes:

>> Would someone be able to check the code and see if it's going in the
>> right direction?
>
> OK, but see below.
>
>> >> This is a feature suggestion which I've written example code for
>> >>
>> >> `vc-root-diff` (or a new function), could jump to the closest line.
>> >>
>> >> This is useful since you move to the context of the current line.
>
> I'm not sure I understand what this code is supposed to do or how to
> use it. Could you please give a recipe for how to test it?

Hi Campbell,

Just to follow up on this, could you please provide instructions for
how to test your patch?

Thanks in advance.

Best regards,
Stefan Kangas

bug#36526: 26.2; Open diff at current location (feature request with suggested code)

[Campbell Barton]

On Sat, 2019-11-02 at 02:02 +0100, Stefan Kangas wrote:
> Stefan Kangas <stefan@marxist.se> writes:
> 
> >> Would someone be able to check the code and see if it's going in
> the
> >> right direction?
> >
> > OK, but see below.
> >
> >> >> This is a feature suggestion which I've written example code
> for
> >> >>
> >> >> `vc-root-diff` (or a new function), could jump to the closest
> line.
> >> >>
> >> >> This is useful since you move to the context of the current
> line.
> >
> > I'm not sure I understand what this code is supposed to do or how
> to
> > use it. Could you please give a recipe for how to test it?
> 
> Hi Campbell,
> 
> Just to follow up on this, could you please provide instructions for
> how to test your patch?
> 
> Thanks in advance.
> 
> Best regards,
> Stefan Kangas


Hi Stefan,

I've updated the Q&A to include the code and a utility function to use
it:

https://emacs.stackexchange.com/a/51439/2418

Try binding "vc-root-diff-fullscreen-and-jump-to-point" from a key in a
git repository with local edits, this will open the diff at (or close
to) the location being edited.

Included example video, showing how this is useful:

https://youtu.be/hbD8WrtWfEg

bug#36526: 26.2; Open diff at current location (feature request with suggested code)

[Campbell Barton]

On Sat, 2019-11-02 at 02:02 +0100, Stefan Kangas wrote:
> Stefan Kangas <stefan@marxist.se> writes:
> 
> > > Would someone be able to check the code and see if it's going in
> > > the
> > > right direction?
> > 
> > OK, but see below.
> > 
> > > > > This is a feature suggestion which I've written example code
> > > > > for
> > > > > 
> > > > > `vc-root-diff` (or a new function), could jump to the closest
> > > > > line.
> > > > > 
> > > > > This is useful since you move to the context of the current
> > > > > line.
> > 
> > I'm not sure I understand what this code is supposed to do or how
> > to
> > use it. Could you please give a recipe for how to test it?
> 
> Hi Campbell,
> 
> Just to follow up on this, could you please provide instructions for
> how to test your patch?
> 
> Thanks in advance.
> 
> Best regards,
> Stefan Kangas

Hi Stefan, my last post didn't get a response so I assumed there wasn't
much interest here.

I've since made this a package, however a reply on reddit suggests I
try to get this into emacs source.

If there is interest, I'm happy to create a patch and other steps
needed to have in added into emacs.  

https://www.reddit.com/r/emacs/comments/efu71g/announce_diffatpoint_now_available_in_melpa/fc4s4im?utm_source=share&utm_medium=web2x

bug#36526: 26.2; Open diff at current location (feature request with suggested code)

[Stefan Kangas]

Campbell Barton <ideasman42@gmail.com> writes:

> Hi Stefan,
>
> I've updated the Q&A to include the code and a utility function to use
> it:
>
> https://emacs.stackexchange.com/a/51439/2418
>
> Try binding "vc-root-diff-fullscreen-and-jump-to-point" from a key in a
> git repository with local edits, this will open the diff at (or close
> to) the location being edited.
>
> Included example video, showing how this is useful:
>
> https://youtu.be/hbD8WrtWfEg

Campbell Barton <ideasman42@gmail.com> writes:

> Hi Stefan, my last post didn't get a response so I assumed there wasn't
> much interest here.
>
> I've since made this a package, however a reply on reddit suggests I
> try to get this into emacs source.
>
> If there is interest, I'm happy to create a patch and other steps
> needed to have in added into emacs.  
>
> https://www.reddit.com/r/emacs/comments/efu71g/announce_diffatpoint_now_available_in_melpa/fc4s4im?utm_source=share&utm_medium=web2x

Hi Campbell,

Sorry for the late reply, and thank you for your continued efforts in
pushing for this feature.

I've watched your video, and I think it looks interesting.  I would
think it useful enough to include, and it certainly helps that you are
willing to do the work to get it done.

Unfortunately, I'm not a heavy vc user, so I don't feel fully
qualified to comment on this use case.

Could someone else please take a look at this proposal and give
Campbell a more informed reply?

Best regards,
Stefan Kangas

bug#36526: 26.2; Open diff at current location (feature request with suggested code)

[Dmitry Gutov]

On 15.01.2020 5:20, Stefan Kangas wrote:
> I've watched your video, and I think it looks interesting.  I would
> think it useful enough to include, and it certainly helps that you are
> willing to do the work to get it done.
> 
> Unfortunately, I'm not a heavy vc user, so I don't feel fully
> qualified to comment on this use case.
> 
> Could someone else please take a look at this proposal and give
> Campbell a more informed reply?

Speaking of the "open diff at point" feature, it's very reminiscent of 
the feature in a package of mine: 
https://github.com/dgutov/diff-hl/blob/fb9eb1cd3c4c6ed24b93de1a7cfb369d2983be74/diff-hl.el#L373-L404

And the implementation is much shorter there. So far, I don't understand 
why.

I would be happy to facilitate moving this feature to the core, but for 
obvious reasons (chiefly ease of review) I'd prefer to to be based off 
my own code. Or to see a good explanation why it's not a good choice.

If the implementation in the package above doesn't cut it for some 
reasons, similarly I'd prefer to see a patch against that code rather 
than a full reimplementation. If feasible, of course.

bug#36526: 26.2; Open diff at current location (feature request with suggested code)

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 15.01.2020 5:20, Stefan Kangas wrote:

>> I've watched your video, and I think it looks interesting.  I would
>> think it useful enough to include, and it certainly helps that you
>> are willing to do the work to get it done.  Unfortunately, I'm not a
>> heavy vc user, so I don't feel fully qualified to comment on this use
>> case.  Could someone else please take a look at this proposal and
>> give Campbell a more informed reply?
>
> Speaking of the "open diff at point" feature, it's very reminiscent of
> the feature in a package of mine: 
> https://github.com/dgutov/diff-hl/blob/fb9eb1cd3c4c6ed24b93de1a7cfb369d2983be74/diff-hl.el#L373-L404
>
> And the implementation is much shorter there. So far, I don't
> understand why.
>
> I would be happy to facilitate moving this feature to the core, but
> for obvious reasons (chiefly ease of review) I'd prefer to to be based
> off my own code. Or to see a good explanation why it's not a good
> choice.
>
> If the implementation in the package above doesn't cut it for some
> reasons, similarly I'd prefer to see a patch against that code rather 
> than a full reimplementation. If feasible, of course.

I had a look at the copyright assignment list, and I couldn't find
Campbell there.  Campbell, didn't anybody send you copyright assignment
papers?

And Dmitry, did you look at moving this feature to the core?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#36526: 26.2; Open diff at current location (feature request with suggested code)

[Dmitry Gutov]

On 09.08.2020 22:37, Lars Ingebrigtsen wrote:

> And Dmitry, did you look at moving this feature to the core?

How would I look at it? I didn't even get a confirmation that it 
satisfies the requested functionality.

But adding it to the core should be easy enough (basically, just copy 
the functions, 40 lines total; with a rename, of course).

If you tried it, and you like it, please feel free to.

bug#36526: 26.2; Open diff at current location (feature request with suggested code)

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 09.08.2020 22:37, Lars Ingebrigtsen wrote:
>
>> And Dmitry, did you look at moving this feature to the core?
>
> How would I look at it? I didn't even get a confirmation that it
> satisfies the requested functionality.
>
> But adding it to the core should be easy enough (basically, just copy
> the functions, 40 lines total; with a rename, of course).
>
> If you tried it, and you like it, please feel free to.

I haven't tried it -- you seemed to object to adding the patch in
question saying you had a simpler solution.  Then adding that simpler
solution to Emacs would be a better idea, but you didn't?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#36526: 26.2; Open diff at current location (feature request with suggested code)

[Dmitry Gutov]

On 10.08.2020 13:43, Lars Ingebrigtsen wrote:
> I haven't tried it -- you seemed to object to adding the patch in
> question saying you had a simpler solution.  Then adding that simpler
> solution to Emacs would be a better idea, but you didn't?

There was no patch.

There was a code dump, which I countered with an alternative, tidier 
code dump. Since no discussion continued from that, I see no point in 
doing the work myself.

bug#36526: 26.2; Open diff at current location (feature request with suggested code)

[Lars Ingebrigtsen]

Campbell Barton <ideasman42@gmail.com> writes:

> If there is interest, I'm happy to create a patch and other steps
> needed to have in added into emacs.  

Campbell, have you had a chance to look at Dmitry's suggestion?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#36526: Previous work on this subject

[Herring, Davis via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[Please CC me; I'm subscribed to devel, not bugs.  Also, hopefully DMARC doesn't mess anyone up here.]

I wrote a version of this feature before; at the time, the list wasn't convinced of the motivation, but there seems to be some more interest since.  The patch still applies and works; I'd be glad to update it further for inclusion.

https://lists.gnu.org/archive/html/emacs-devel/2016-02/msg01066.html

Davis

bug#36526: Previous work on this subject

[Juri Linkov]

> [Please CC me; I'm subscribed to devel, not bugs.
> Also, hopefully DMARC doesn't mess anyone up here.]
>
> I wrote a version of this feature before; at the time, the list wasn't
> convinced of the motivation, but there seems to be some more interest
> since.  The patch still applies and works; I'd be glad to update it
> further for inclusion.
>
> https://lists.gnu.org/archive/html/emacs-devel/2016-02/msg01066.html

Thanks, this is the most advanced implementation among several others.
I tried it, and it does everything needed, and the code is quite compact.
So I recommend to install this useful feature from the patches above.

PS: I noticed some minor issues that could be improved afterwards.

bug#36526: Previous work on this subject

[Dmitry Gutov]

On 16.11.2020 23:12, Juri Linkov wrote:
> Thanks, this is the most advanced implementation among several others.
> I tried it, and it does everything needed, and the code is quite compact.
> So I recommend to install this useful feature from the patches above.

Could someone outline briefly how it compares to the implementation in 
'diff-hl-diff-skip-to'?

If nothing else, that one seems shorter.

bug#36526: Previous work on this subject

[Juri Linkov]

>> Thanks, this is the most advanced implementation among several others.
>> I tried it, and it does everything needed, and the code is quite compact.
>> So I recommend to install this useful feature from the patches above.
>
> Could someone outline briefly how it compares to the implementation in
> 'diff-hl-diff-skip-to'?

Improvements:
1. support context diffs;
2. support multiple files in the same patch (e.g. by C-x v D);

bug#36526: Previous work on this subject

[Dmitry Gutov]

On 17.11.2020 00:04, Juri Linkov wrote:
>>> Thanks, this is the most advanced implementation among several others.
>>> I tried it, and it does everything needed, and the code is quite compact.
>>> So I recommend to install this useful feature from the patches above.
>>
>> Could someone outline briefly how it compares to the implementation in
>> 'diff-hl-diff-skip-to'?
> 
> Improvements:
> 1. support context diffs;

IIUC that part is concentrated in the patch #1. So it should be possible 
to swap out patch #2 for the impl in diff-hl-diff-skip-to. *If* that is 
a good idea.

> 2. support multiple files in the same patch (e.g. by C-x v D);

Interesting. Personally, it's a context where I wouldn't expect this new 
behavior (when invoking the repo-wide diff, I expect to see a full 
overview and be able to page through it without calling 
beginning-of-buffer first). But to be fair, I haven't tried it in 
practice yet.

So if the implementation is smart enough and you like both aspects of 
the new behavior, don't mind me. Looking forward to seeing it in action.