* [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation @ 2016-11-29 12:07 Tino Calancha 2016-11-30 0:03 ` Noam Postavsky ` (2 more replies) 0 siblings, 3 replies; 21+ messages in thread From: Tino Calancha @ 2016-11-29 12:07 UTC (permalink / raw) To: Dima Kogan; +Cc: tino.calancha, emacs-devel @@ -1959,8 +2064,8 @@ diff-refine-hunk (interactive) (require 'smerge-mode) (save-excursion - (diff-beginning-of-hunk t) - (let* ((start (point)) + (let* ((hunk-bounds (diff-bounds-of-hunk)) + (start (goto-char (car hunk-bounds))) (style (diff-hunk-style)) ;Skips the hunk header as well. (beg (point)) (props-c '((diff-mode . fine) (face diff-refine-changed))) @@ -1968,7 +2073,7 @@ diff-refine-hunk (props-a '((diff-mode . fine) (face diff-refine-added))) ;; Be careful to go back to `start' so diff-end-of-hunk gets ;; to read the hunk header's line info. - (end (progn (goto-char start) (diff-end-of-hunk) (point)))) + (end (goto-char (cadr hunk-bounds)))) Hi Dima, after the patch variable start is not used. You might want to rename this var as _start, to avoid: diff-mode.el:2086:48:Warning: Unused lexical variable ‘start’ ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation 2016-11-29 12:07 [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation Tino Calancha @ 2016-11-30 0:03 ` Noam Postavsky 2016-11-30 14:22 ` Dmitry Gutov 2016-12-06 2:03 ` Mark Oteiza 2 siblings, 0 replies; 21+ messages in thread From: Noam Postavsky @ 2016-11-30 0:03 UTC (permalink / raw) To: Tino Calancha; +Cc: Emacs developers, Dima Kogan On Tue, Nov 29, 2016 at 7:07 AM, Tino Calancha <tino.calancha@gmail.com> wrote: > > > @@ -1959,8 +2064,8 @@ diff-refine-hunk > (interactive) > (require 'smerge-mode) > (save-excursion > - (diff-beginning-of-hunk t) > - (let* ((start (point)) > + (let* ((hunk-bounds (diff-bounds-of-hunk)) > + (start (goto-char (car hunk-bounds))) > (style (diff-hunk-style)) ;Skips the hunk header as well. > (beg (point)) > (props-c '((diff-mode . fine) (face diff-refine-changed))) > @@ -1968,7 +2073,7 @@ diff-refine-hunk > (props-a '((diff-mode . fine) (face diff-refine-added))) > ;; Be careful to go back to `start' so diff-end-of-hunk gets > ;; to read the hunk header's line info. > - (end (progn (goto-char start) (diff-end-of-hunk) (point)))) > + (end (goto-char (cadr hunk-bounds)))) > > Hi Dima, > > after the patch variable start is not used. You might want to rename > this var as _start, to avoid: > diff-mode.el:2086:48:Warning: Unused lexical variable ‘start’ > Ah, I probably should have caught that in review. I even had the warning in my *compilation* buffer, I just didn't look. Fixed now. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation 2016-11-29 12:07 [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation Tino Calancha 2016-11-30 0:03 ` Noam Postavsky @ 2016-11-30 14:22 ` Dmitry Gutov 2016-11-30 14:31 ` Stefan Monnier 2016-12-10 1:27 ` Dima Kogan 2016-12-06 2:03 ` Mark Oteiza 2 siblings, 2 replies; 21+ messages in thread From: Dmitry Gutov @ 2016-11-30 14:22 UTC (permalink / raw) To: Tino Calancha, Dima Kogan; +Cc: emacs-devel Hi all, I appreciate the navigation fixes, but some of these changes made diff-auto-refine-mode work worse. In particular: - If there's just one hunk, I routinely pressed `n' to take advantage of the auto-refine behavior. And point moved to the end of the hunk. Now, pressing `n' gives me a ding at the top of the window, then some time passes (if the hunk is of a significant size), and auto-refine is applied. The point doesn't move. - Another example: there are several hunks. I press `n', and the first one doesn't get refined. It only gets refined if I press `p' to jump back over it. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation 2016-11-30 14:22 ` Dmitry Gutov @ 2016-11-30 14:31 ` Stefan Monnier 2016-11-30 14:34 ` Dmitry Gutov 2016-12-10 1:27 ` Dima Kogan 1 sibling, 1 reply; 21+ messages in thread From: Stefan Monnier @ 2016-11-30 14:31 UTC (permalink / raw) To: emacs-devel > I appreciate the navigation fixes, but some of these changes made > diff-auto-refine-mode work worse. In particular: BTW, diff-mode would really benefit from having diff-refine hooked into font-lock since the hunks are just always refined without having to do anything at all. Stefan ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation 2016-11-30 14:31 ` Stefan Monnier @ 2016-11-30 14:34 ` Dmitry Gutov 2016-11-30 15:07 ` Stefan Monnier 0 siblings, 1 reply; 21+ messages in thread From: Dmitry Gutov @ 2016-11-30 14:34 UTC (permalink / raw) To: Stefan Monnier, emacs-devel On 30.11.2016 16:31, Stefan Monnier wrote: > BTW, diff-mode would really benefit from having diff-refine hooked into > font-lock since the hunks are just always refined without having to do > anything at all. Couldn't a slow font-lock rule make normal navigation worse? ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation 2016-11-30 14:34 ` Dmitry Gutov @ 2016-11-30 15:07 ` Stefan Monnier 0 siblings, 0 replies; 21+ messages in thread From: Stefan Monnier @ 2016-11-30 15:07 UTC (permalink / raw) To: Dmitry Gutov; +Cc: emacs-devel >> BTW, diff-mode would really benefit from having diff-refine hooked into >> font-lock since the hunks are just always refined without having to do >> anything at all. > Couldn't a slow font-lock rule make normal navigation worse? Oh yes, which is why it shouldn't be slow: it should be asynchronous, instead (which is also why I haven't written it yet: doing it synchronous and slow would be easy). Stefan ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation 2016-11-30 14:22 ` Dmitry Gutov 2016-11-30 14:31 ` Stefan Monnier @ 2016-12-10 1:27 ` Dima Kogan 2016-12-10 10:14 ` Dmitry Gutov 1 sibling, 1 reply; 21+ messages in thread From: Dima Kogan @ 2016-12-10 1:27 UTC (permalink / raw) To: Dmitry Gutov; +Cc: emacs-devel, Tino Calancha Hi. Dmitry Gutov <dgutov@yandex.ru> writes: > I appreciate the navigation fixes, but some of these changes made > diff-auto-refine-mode work worse. In particular: > > - If there's just one hunk, I routinely pressed `n' to take advantage of > the auto-refine behavior. And point moved to the end of the hunk. > > Now, pressing `n' gives me a ding at the top of the window, then some > time passes (if the hunk is of a significant size), and auto-refine is > applied. The point doesn't move. I'm unclear about what the complaint is. Both the old and new behaviors made the auto-refinement do its thing. Is the complaint that the point no longer moves to the end (and if so, why is that "right"?) or is the "ding" the "problem? > - Another example: there are several hunks. I press `n', and the first > one doesn't get refined. It only gets refined if I press `p' to jump > back over it. OK. The logic in place is to auto-refine the hunk at point after a motion, which is why you're seeing this behavior. Are you seeing this issue only with the first hunk in a buffer and only when you first load such a buffer? If so, an auto-refinement call from a diff-mode-hook would solve this. Sounds reasonable, or are such things frowned-upon? ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation 2016-12-10 1:27 ` Dima Kogan @ 2016-12-10 10:14 ` Dmitry Gutov 2016-12-10 17:27 ` Dima Kogan 0 siblings, 1 reply; 21+ messages in thread From: Dmitry Gutov @ 2016-12-10 10:14 UTC (permalink / raw) To: Dima Kogan; +Cc: emacs-devel, Tino Calancha On 10.12.2016 03:27, Dima Kogan wrote: > I'm unclear about what the complaint is. Both the old and new behaviors > made the auto-refinement do its thing. Is the complaint that the point > no longer moves to the end (and if so, why is that "right"?) or is the > "ding" the "problem? Sorry, let me clarify. The first complaint is that, yes, point doesn't move to the end anymore. Foremost, I'm simply used to the old behavior. Second, I'm not sure how to implement on-demand refining of the first hunk in a sane fashion without it, see below. Third, it's simply handy if the first hunk is taller than the height of the window, I would navigate to its end to examine it. Fourth, I'd see the end of the buffer myself before the next pressing of `n' invokes a "ding". The second complaint is that the command does a "ding" (informing me that I did something wrong), and then proceeds to do something useful: refining the hunk. > OK. The logic in place is to auto-refine the hunk at point after a > motion, which is why you're seeing this behavior. Are you seeing this > issue only with the first hunk in a buffer and only when you first load > such a buffer? Yes. > If so, an auto-refinement call from a diff-mode-hook > would solve this. Sounds reasonable, or are such things frowned-upon? What if the first hunk is big and refining takes a lot of time? As it is now, I can make the choice to refine or not myself. If `diff-mode-hook' does that, I won't even see the diff before refining is done. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation 2016-12-10 10:14 ` Dmitry Gutov @ 2016-12-10 17:27 ` Dima Kogan 2016-12-11 11:07 ` Dmitry Gutov 0 siblings, 1 reply; 21+ messages in thread From: Dima Kogan @ 2016-12-10 17:27 UTC (permalink / raw) To: Dmitry Gutov; +Cc: emacs-devel, Tino Calancha On December 10, 2016 2:14:41 AM PST, Dmitry Gutov <dgutov@yandex.ru> wrote: >On 10.12.2016 03:27, Dima Kogan wrote: > >> I'm unclear about what the complaint is. Both the old and new >behaviors >> made the auto-refinement do its thing. Is the complaint that the >point >> no longer moves to the end (and if so, why is that "right"?) or is >the >> "ding" the "problem? > >Sorry, let me clarify. > >The first complaint is that, yes, point doesn't move to the end >anymore. >Foremost, I'm simply used to the old behavior. Second, I'm not sure how > >to implement on-demand refining of the first hunk in a sane fashion >without it, see below. > >Third, it's simply handy if the first hunk is taller than the height of > >the window, I would navigate to its end to examine it. Fourth, I'd see >the end of the buffer myself before the next pressing of `n' invokes a >"ding". > >The second complaint is that the command does a "ding" (informing me >that I did something wrong), and then proceeds to do something useful: >refining the hunk. > >> OK. The logic in place is to auto-refine the hunk at point after a >> motion, which is why you're seeing this behavior. Are you seeing this >> issue only with the first hunk in a buffer and only when you first >load >> such a buffer? > >Yes. > >> If so, an auto-refinement call from a diff-mode-hook >> would solve this. Sounds reasonable, or are such things frowned-upon? > >What if the first hunk is big and refining takes a lot of time? > >As it is now, I can make the choice to refine or not myself. If >`diff-mode-hook' does that, I won't even see the diff before refining >is >done. It sounds like "auto" refinement isn't what you really want. You want to ask emacs to refine the hunk you are on by invoking diff-next even though diff-refine-hunk makes more sense. For the other concerns, I can special-case the last hunk, and move to eob for diff-next, and to ding only if we're already at eob to begin with. That will give you the legacy behavior if there's only a single hunk, I think. Seems reasonable? ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation 2016-12-10 17:27 ` Dima Kogan @ 2016-12-11 11:07 ` Dmitry Gutov 2016-12-12 7:28 ` Dima Kogan 0 siblings, 1 reply; 21+ messages in thread From: Dmitry Gutov @ 2016-12-11 11:07 UTC (permalink / raw) To: Dima Kogan; +Cc: Tino Calancha, emacs-devel On 10.12.2016 19:27, Dima Kogan wrote: > It sounds like "auto" refinement isn't what you really want. You want to ask emacs to refine the hunk you are on by invoking diff-next even though diff-refine-hunk makes more sense. Why bother with two different bindings when one can do? Up until now, it's worked nicely. > For the other concerns, I can special-case the last hunk, and move to eob for diff-next, and to ding only if we're already at eob to begin with. Sounds good. > That will give you the legacy behavior if there's only a single hunk, I think. Seems reasonable? Yes, thank you. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation 2016-12-11 11:07 ` Dmitry Gutov @ 2016-12-12 7:28 ` Dima Kogan 2016-12-16 1:05 ` Dmitry Gutov 2016-12-20 2:22 ` Tino Calancha 0 siblings, 2 replies; 21+ messages in thread From: Dima Kogan @ 2016-12-12 7:28 UTC (permalink / raw) To: Dmitry Gutov; +Cc: Tino Calancha, emacs-devel Dmitry Gutov <dgutov@yandex.ru> writes: > On 10.12.2016 19:27, Dima Kogan wrote: > >> For the other concerns, I can special-case the last hunk, and move to >> eob for diff-next, and to ding only if we're already at eob to begin >> with. Hi. I played around with this, and I now really don't like this idea because it would mean that diff-hunk-next no longer always moves to the next hunk. At the last hunk, diff-hunk-next would stay on the SAME hunk if this was implemented. It would really make more sense if - diff-hunk-next moves to next hunk - if this isn't possible, the point stays where it is, and an error is signalled This is what the behavior in git intends to do. Furthermore, I think invoking auto-refinement in diff-mode-hook makes sense. The auto-refinement logic is written (not by me) to auto-refine the hunk at point when it is visited. And this is triggered not just with functions such as diff-hunk-next but also with actions such as turning on auto-refine-mode: diff-auto-refine-mode refines the hunk at point. Given this, I think it makes sense for us to do what diff-auto-refine-mode does if we enter diff-mode and auto-refinement is already on. And I think that this should happen even if the first hunk is large. This would indeed be slow, but this is AUTO refinement. If you want to decide when refinement should or should not happen, auto-refinement is not what you want. We can add a variable to block auto-refinement for hunks larger than some size, which probably would be a good idea anyway. I fixed the incorrect behavior where auto-refinement was kicking in even when the navigation failed and we signalled an error. Not attaching any patches yet. Let's agree on an approach, and then talk implementation. dima ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation 2016-12-12 7:28 ` Dima Kogan @ 2016-12-16 1:05 ` Dmitry Gutov 2016-12-20 2:22 ` Tino Calancha 1 sibling, 0 replies; 21+ messages in thread From: Dmitry Gutov @ 2016-12-16 1:05 UTC (permalink / raw) To: Dima Kogan; +Cc: emacs-devel, Tino Calancha On 12.12.2016 09:28, Dima Kogan wrote: > Hi. I played around with this, and I now really don't like this idea > because it would mean that diff-hunk-next no longer always moves to the > next hunk. We have other commands that behave this way. beginning-of-defun and end-of-defun, for instance. You could say they're named differently, but I think the uniformity of behavior is more important. > At the last hunk, diff-hunk-next would stay on the SAME hunk > if this was implemented. It would go beyond the last hunk, wouldn't it? > Furthermore, I think invoking auto-refinement in diff-mode-hook makes > sense. From a strict and logical point of view, maybe. But not from the user convenience point of view. I don't think we should exchange the latter for the former. > And I think that this should happen even if the first hunk is large. > This would indeed be slow, but this is AUTO refinement. Again, words are important, but it's up to us as developers to choose how to interpret them for maximum user benefit. > If you want to > decide when refinement should or should not happen, auto-refinement is > not what you want. It's worked quite well before. > We can add a variable to block auto-refinement for > hunks larger than some size, which probably would be a good idea anyway. That sounds finicky. It would be hard to find the value that's good for all kinds of diffs, users and differently performing computers. > I fixed the incorrect behavior where auto-refinement was kicking in even > when the navigation failed and we signalled an error. That's something, I guess. > Not attaching any patches yet. Let's agree on an approach, and then talk > implementation. I'm sorry for not proposing a particular course of action. If nobody has a good suggestion aside from a revert (which we obviously want to avoid), I'll try to dig in sometime later (January at the earliest). ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation 2016-12-12 7:28 ` Dima Kogan 2016-12-16 1:05 ` Dmitry Gutov @ 2016-12-20 2:22 ` Tino Calancha 2016-12-20 7:31 ` Dima Kogan 2016-12-25 6:52 ` Dima Kogan 1 sibling, 2 replies; 21+ messages in thread From: Tino Calancha @ 2016-12-20 2:22 UTC (permalink / raw) To: Dima Kogan; +Cc: emacs-devel, Tino Calancha, Dmitry Gutov Dima Kogan <dima@secretsauce.net> writes: > Dmitry Gutov <dgutov@yandex.ru> writes: > >> On 10.12.2016 19:27, Dima Kogan wrote: >> >>> For the other concerns, I can special-case the last hunk, and move to >>> eob for diff-next, and to ding only if we're already at eob to begin >>> with. > > Hi. I played around with this, and I now really don't like this idea > because it would mean that diff-hunk-next no longer always moves to the > next hunk. At the last hunk, diff-hunk-next would stay on the SAME hunk > if this was implemented. It would really make more sense if > > - diff-hunk-next moves to next hunk > - if this isn't possible, the point stays where it is, and an error is > signalled > > This is what the behavior in git intends to do. Hi Dima, after commit 2c8a7e5 the following receipt doesn't work: M-! git show HEAD RET C-x o C-x C-q M-x diff-mode RET n ;; Signal error: diff-beginning-of-file-and-junk: Can’t find the beginning of the file I use above code very often: before commit 2c8a7e5, it skips the commit message and jumps to the first hunk. Is it possible to make above snippet working as before? Regards, Tino ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation 2016-12-20 2:22 ` Tino Calancha @ 2016-12-20 7:31 ` Dima Kogan 2016-12-25 6:52 ` Dima Kogan 1 sibling, 0 replies; 21+ messages in thread From: Dima Kogan @ 2016-12-20 7:31 UTC (permalink / raw) To: Tino Calancha; +Cc: emacs-devel, Dmitry Gutov Tino Calancha <tino.calancha@gmail.com> writes: > Hi Dima, > after commit 2c8a7e5 the following receipt doesn't work: > > M-! git show HEAD RET > C-x o > C-x C-q > M-x diff-mode RET > n > ;; Signal error: > diff-beginning-of-file-and-junk: Can t find the beginning of the file > > I use above code very often: before commit 2c8a7e5, it skips the commit > message and jumps to the first hunk. > Is it possible to make above snippet working as before? Hi. This was brought up earlier, and I proposed a patch here: https://lists.gnu.org/archive/html/emacs-devel/2016-12/msg00239.html I need to test this and a few other small tweaks. Hopefully this week I'll have an update. dima ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation 2016-12-20 2:22 ` Tino Calancha 2016-12-20 7:31 ` Dima Kogan @ 2016-12-25 6:52 ` Dima Kogan 2016-12-25 9:48 ` Tino Calancha 1 sibling, 1 reply; 21+ messages in thread From: Dima Kogan @ 2016-12-25 6:52 UTC (permalink / raw) To: Tino Calancha; +Cc: emacs-devel, Dmitry Gutov Tino Calancha <tino.calancha@gmail.com> writes: > Hi Dima, > after commit 2c8a7e5 the following receipt doesn't work: > > M-! git show HEAD RET > C-x o > C-x C-q > M-x diff-mode RET > n > ;; Signal error: > diff-beginning-of-file-and-junk: Can t find the beginning of the file > > I use above code very often: before commit 2c8a7e5, it skips the commit > message and jumps to the first hunk. > Is it possible to make above snippet working as before? Hi. I pushed to master the fix to this and the logic to invoke the auto-refinement only if the preceding motion was successful. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation 2016-12-25 6:52 ` Dima Kogan @ 2016-12-25 9:48 ` Tino Calancha 2016-12-25 9:58 ` Tino Calancha 0 siblings, 1 reply; 21+ messages in thread From: Tino Calancha @ 2016-12-25 9:48 UTC (permalink / raw) To: Dima Kogan; +Cc: Dmitry Gutov, Tino Calancha, emacs-devel Dima Kogan <lists@dima.secretsauce.net> writes: > Tino Calancha <tino.calancha@gmail.com> writes: > >> Hi Dima, >> after commit 2c8a7e5 the following receipt doesn't work: >> >> M-! git show HEAD RET >> C-x o >> C-x C-q >> M-x diff-mode RET >> n >> ;; Signal error: >> diff-beginning-of-file-and-junk: Can t find the beginning of the file >> >> I use above code very often: before commit 2c8a7e5, it skips the commit >> message and jumps to the first hunk. >> Is it possible to make above snippet working as before? > > Hi. I pushed to master the fix to this and the logic to invoke the > auto-refinement only if the preceding motion was successful. Thank you. I think is not working as before. Consider following snippet code: M-! git show SHA1 RET C-x b *Shell Command Output* RET C-x C-q M-x diff-mode n When i use as SHA1 e5ef59b87da5c2ddfa22f7342efe29b3eea6ed97 previous commands successfully move point to next hunk. But if i use commit 6b6abe0dba6a9a2e5f78aac3814421886e7a184f then they don't work; i got the following error: user-error: No next hunk I am wondering why the second commit behaves differently than the first one. Regards, Tino ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation 2016-12-25 9:48 ` Tino Calancha @ 2016-12-25 9:58 ` Tino Calancha 2016-12-25 14:10 ` Dmitry Gutov 0 siblings, 1 reply; 21+ messages in thread From: Tino Calancha @ 2016-12-25 9:58 UTC (permalink / raw) To: Dima Kogan; +Cc: Tino Calancha, emacs-devel, Dmitry Gutov Tino Calancha <tino.calancha@gmail.com> writes: >> Hi. I pushed to master the fix to this and the logic to invoke the >> auto-refinement only if the preceding motion was successful. > > Thank you. > I think is not working as before. > Consider following snippet code: > > M-! git show SHA1 RET > C-x b *Shell Command Output* RET > C-x C-q > M-x diff-mode > n > > When i use as SHA1 > e5ef59b87da5c2ddfa22f7342efe29b3eea6ed97 > previous commands successfully move point to next hunk. > > But if i use commit > 6b6abe0dba6a9a2e5f78aac3814421886e7a184f > then they don't work; i got the following error: > user-error: No next hunk > > I am wondering why the second commit behaves differently than > the first one. In Emacs-25 the commit message is not considered part of the first hunk: So in the previous examples, the point is set in front of @@ -551,23 +551,7 @@ diff--auto-refine-data (for e5ef59b87da5c2ddfa22f7342efe29b3eea6ed97) @@ -768,7 +768,7 @@ diff-beginning-of-file-and-junk (for 6b6abe0dba6a9a2e5f78aac3814421886e7a184f). After your changes the commit message is somehow part of the first hunk; then, from the beginning of buffer `n' will jump to the second hunk. This is unconvenient specially when the commit messages are long. I prefer the way this is handled in Emacs-25. Regards, Tino ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation 2016-12-25 9:58 ` Tino Calancha @ 2016-12-25 14:10 ` Dmitry Gutov 0 siblings, 0 replies; 21+ messages in thread From: Dmitry Gutov @ 2016-12-25 14:10 UTC (permalink / raw) To: Tino Calancha, Dima Kogan; +Cc: emacs-devel On 25.12.2016 11:58, Tino Calancha wrote: > In Emacs-25 the commit message is not considered part of the > first hunk: Can we make the file header not part of the hunk either? Then the first `n' would move to the first hunk, and this should solve the remaining auto-refine problem. To make things work like they do now, some (all?) commands that act on hunks would first see if point is inside a file header, and if so, jump forward to the next hunk. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation 2016-11-29 12:07 [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation Tino Calancha 2016-11-30 0:03 ` Noam Postavsky 2016-11-30 14:22 ` Dmitry Gutov @ 2016-12-06 2:03 ` Mark Oteiza 2016-12-07 7:29 ` Dima Kogan 2 siblings, 1 reply; 21+ messages in thread From: Mark Oteiza @ 2016-12-06 2:03 UTC (permalink / raw) To: Tino Calancha; +Cc: emacs-devel, Dima Kogan Fixing C-c C-a to DTRT is great, thanks, but I don't think the off-by-one navigation changes to "n" and "p" (diff-hunk-next, diff-hunk-prev) make sense. While it may have made fixing the issues mentioned in the commit message easier, the changes to what "n" and "p" do at the beginning and end of a diff are not documented, and I didn't see any discussion of it in the associated bug. I contend that the new behavior is inconsistent with the behavior of other outline/thing-with-headers type things in Emacs. outline-mode, org-mode, and rst-mode are the first ones that come to mind. It's also not clear how the introduced oddity with auto-refine is going to be resolved, unless a way is found to autorefine the first hunk without there being any user interaction. Then opening a diff has inconsistent auto-refining from the start. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation 2016-12-06 2:03 ` Mark Oteiza @ 2016-12-07 7:29 ` Dima Kogan 2017-01-06 2:58 ` Mark Oteiza 0 siblings, 1 reply; 21+ messages in thread From: Dima Kogan @ 2016-12-07 7:29 UTC (permalink / raw) To: Mark Oteiza; +Cc: 25105, emacs-devel, Tino Calancha [-- Attachment #1: Type: text/plain, Size: 1722 bytes --] Mark Oteiza <mvoteiza@udel.edu> writes: > Fixing C-c C-a to DTRT is great, thanks, but I don't think the > off-by-one navigation changes to "n" and "p" (diff-hunk-next, > diff-hunk-prev) make sense. While it may have made fixing the issues > mentioned in the commit message easier, the changes to what "n" and "p" > do at the beginning and end of a diff are not documented, and I didn't > see any discussion of it in the associated bug. > > I contend that the new behavior is inconsistent with the behavior of > other outline/thing-with-headers type things in Emacs. outline-mode, > org-mode, and rst-mode are the first ones that come to mind. Can you give a specific example of interaction in any of these modes that is analogous to the off-by-one behavior you're referring to? The fundamental question is what hunk diff-mode should think the point is looking at, when it is in some non-diff message above the first hunk. The answer I chose for the new navigation logic is "first hunk". You could also choose "invalid hunk, not a hunk at all", which would imply that C-c C-a and M-ret and such shouldn't work there. Better suggestions welcome. > It's also not clear how the introduced oddity with auto-refine is going > to be resolved, unless a way is found to autorefine the first hunk > without there being any user interaction. Then opening a diff has > inconsistent auto-refining from the start. I don't use auto-refinement, so didn't notice the breakage. Will look at it in a bit. In the meantime, I'm attaching a patch to address the 2nd point in the bug report (25105). This serves to treat the junk before the first hunk (i.e. commit message from 'git format-patch') as a file header. Looks reasonable? [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: diff-mode-handle-initial-junk.patch --] [-- Type: text/x-diff, Size: 544 bytes --] diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el index 5b48c8d..41476bd 100644 --- a/lisp/vc/diff-mode.el +++ b/lisp/vc/diff-mode.el @@ -768,7 +768,7 @@ diff-beginning-of-file-and-junk (setq prevfile nextfile)) (if (and previndex (numberp prevfile) (< previndex prevfile)) (setq prevfile previndex)) - (if (and (numberp prevfile) (<= prevfile start)) + (if (numberp prevfile) (progn (goto-char prevfile) ;; Now skip backward over the leading junk we may have before the ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation 2016-12-07 7:29 ` Dima Kogan @ 2017-01-06 2:58 ` Mark Oteiza 0 siblings, 0 replies; 21+ messages in thread From: Mark Oteiza @ 2017-01-06 2:58 UTC (permalink / raw) To: Dima Kogan; +Cc: 25105, emacs-devel, Tino Calancha On 06/12/16 at 11:29pm, Dima Kogan wrote: > Mark Oteiza <mvoteiza@udel.edu> writes: > > > Fixing C-c C-a to DTRT is great, thanks, but I don't think the > > off-by-one navigation changes to "n" and "p" (diff-hunk-next, > > diff-hunk-prev) make sense. While it may have made fixing the issues > > mentioned in the commit message easier, the changes to what "n" and "p" > > do at the beginning and end of a diff are not documented, and I didn't > > see any discussion of it in the associated bug. > > > > I contend that the new behavior is inconsistent with the behavior of > > other outline/thing-with-headers type things in Emacs. outline-mode, > > org-mode, and rst-mode are the first ones that come to mind. > > Can you give a specific example of interaction in any of these modes > that is analogous to the off-by-one behavior you're referring to? I wrote about how your changes are make diff-mode _not_ analogous. > The > fundamental question is what hunk diff-mode should think the point is > looking at, when it is in some non-diff message above the first hunk. > The answer I chose for the new navigation logic is "first hunk". You > could also choose "invalid hunk, not a hunk at all", which would imply > that C-c C-a and M-ret and such shouldn't work there. Better suggestions > welcome. One might argue that C-c C-a and friends in a file header should apply all hunks in a file, or perhaps that there should actually be diff-apply-file commands, etc. The way n and p worked was not a bug, yet you gratuitously changed them, and broke auto-refinement. Why do I have to now hit two keys to refine the first hunk, and one key to refine the second? > > It's also not clear how the introduced oddity with auto-refine is going > > to be resolved, unless a way is found to autorefine the first hunk > > without there being any user interaction. Then opening a diff has > > inconsistent auto-refining from the start. > > I don't use auto-refinement, so didn't notice the breakage. Will look at > it in a bit. It's on by default, so this statement perplexes me. ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2017-01-06 2:58 UTC | newest] Thread overview: 21+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-11-29 12:07 [Emacs-diffs] master 2c8a7e5: Improve diff-mode navigation/manipulation Tino Calancha 2016-11-30 0:03 ` Noam Postavsky 2016-11-30 14:22 ` Dmitry Gutov 2016-11-30 14:31 ` Stefan Monnier 2016-11-30 14:34 ` Dmitry Gutov 2016-11-30 15:07 ` Stefan Monnier 2016-12-10 1:27 ` Dima Kogan 2016-12-10 10:14 ` Dmitry Gutov 2016-12-10 17:27 ` Dima Kogan 2016-12-11 11:07 ` Dmitry Gutov 2016-12-12 7:28 ` Dima Kogan 2016-12-16 1:05 ` Dmitry Gutov 2016-12-20 2:22 ` Tino Calancha 2016-12-20 7:31 ` Dima Kogan 2016-12-25 6:52 ` Dima Kogan 2016-12-25 9:48 ` Tino Calancha 2016-12-25 9:58 ` Tino Calancha 2016-12-25 14:10 ` Dmitry Gutov 2016-12-06 2:03 ` Mark Oteiza 2016-12-07 7:29 ` Dima Kogan 2017-01-06 2:58 ` Mark Oteiza
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).