when do revert for current hunk, only show the (will be reverted) diff of current hunk. (or, even disable show diff, revert immediately)

[zw963]

As above screenshot, i only want revert the current hunk, that is, the font settings line.
but, when i press r key to try to revert this line code, right side, it give me many diff not only current hunk, which not so useful for me, and more bad, it give me a misconception, i will revert all diff show in right side.

i don't know emacs VC if exists function for diff of current hunk, but, i consider if not, we can add a defcustom, which can let user disable show this diff buffer, and revert it directly instead.

because, anyway, we can see the major changes directly on left side as following, diff buffer not so useful.

[yugaego]

it give me many diff not only current hunk, which not so useful for me, and more bad, it give me a misconception, i will revert all diff show in right side.

I agree, this was extremely confusing for me too.
If there's no better solution, might be set diff-hl-ask-before-revert-hunk to nil by default,
since it'd probably be easy to cancel a wrong revert with the Emacs' undo command even for a newcomer.

[dgutov]

The method for emphasizing which hunk will be reverted is customizable through diff-hl-highlight-revert-hunk-function.

By default it highlights the first column of that hunk with inverse-video. Which might be harder or easier to notice depending on the current color theme. Example:

You can use a different method through choosing a different function. For instance, this will hide everything except for the current hunk:

(defun diff-hl-revert-narrow-to-hunk (end)
  (narrow-to-region (point) end))

(setq diff-hl-highlight-revert-hunk-function #'diff-hl-revert-narrow-to-hunk)

(including the filename header, unfortunately)

But as for the revert command invoked from the "show hunk" interface, perhaps it indeed should not do the extra prompt, since the hunk is visible already:

diff --git a/diff-hl-show-hunk.el b/diff-hl-show-hunk.el
index c1b2013..82b04a5 100644
--- a/diff-hl-show-hunk.el
+++ b/diff-hl-show-hunk.el
@@ -291,7 +291,8 @@ BUFFER is a buffer with the hunk."
   "Dismiss the popup and prompt to revert the current diff hunk."
   (interactive)
   (diff-hl-show-hunk-hide)
-  (diff-hl-revert-hunk))
+  (let (diff-hl-ask-before-revert-hunk)
+    (diff-hl-revert-hunk)))
 
 ;;;###autoload
 (defun diff-hl-show-hunk-previous ()

[yugaego]

The method for emphasizing which hunk will be reverted is customizable through diff-hl-highlight-revert-hunk-function.

If to consider this feature as being specifically confusing for newbies, then the current default - that inverses a color of the first column - wouldn't be very helpful, because of being hardly noticeable (especially to a person new to this interface).

Maybe the suggested narrow-to-region solution (perhaps showing a bit wider area) could replace the current default?

perhaps it indeed should not do the extra prompt, since the hunk is visible already

Seems it already abides to diff-hl-ask-before-revert-hunk, in the then called diff-hl-revert-hunk-1.

[dgutov]

If to consider this feature as being specifically confusing for newbies, then the current default - that inverses a color of the first column - wouldn't be very helpful, because of being hardly noticeable (especially to a person new to this interface).

We've lived with this for years, so it must be not entirely terrible, but I see your point, and people have indeed asked this question a couple of times before.

Maybe the suggested narrow-to-region solution (perhaps showing a bit wider area) could replace the current default?

That can work, though I was kind of worried to show only one hunk (which often might contain only a few lines) without any context. The rest of the window being empty, it seems like a waste of screen space.

The "perhaps showing a bit wider area" is a little harder to do only using the current diff-hl-highlight-revert-hunk-function. That will require some consideration.

Seems it already abides to diff-hl-ask-before-revert-hunk, in the then called diff-hl-revert-hunk-1.

But it also affects diff-hl-revert-hunk's behavior. Which can act not only on hunk at point, but some hunk near point as well. Showing the exact hunk before reverting by default seems prudent.

[zw963]

That can work, though I was kind of worried to show only one hunk (which often might contain only a few lines) without any context. The rest of the window being empty, it seems like a waste of screen space.

We can use fancy-narrow-to-region instead.

https://github.com/Malabarba/fancy-narrow, we can pull this as a dependency.

[dgutov]

Not something we can use by default, or have a hard dependency on: it's not in GNU ELPA.

But it's an interesting option.

[zw963]

Not something we can use by default, or have a hard dependency on: it's not in GNU ELPA.

But it's an interesting option.

fancy-narrow only 400 LOC, include comments and black. : )

[yugaego]

Looks like we could say the half of the problem is solved by the last commit, not prompting at all when reverting from a shown hunk.

Could probably un-fontify approach suggested earlier to improve the UI for the diff-hl-revert-hunk call?

Or, alternatively, with the "simplified fancy-narrow" (maybe using font-lock instead of overlays):

diff --git a/diff-hl.el b/diff-hl.el
index 5481caa..94a739d 100644
--- a/diff-hl.el
+++ b/diff-hl.el
@@ -141,13 +141,18 @@
            (when on (global-diff-hl-mode 1)))))
 
 (defcustom diff-hl-highlight-revert-hunk-function
-  #'diff-hl-revert-highlight-first-column
+  #'diff-hl-revert-emphasize-hunk
   "Function to highlight the current hunk in `diff-hl-revert-hunk'.
 The function is called at the beginning of the hunk and passed
 the end position as its only argument."
   :type '(choice (const :tag "Do nothing" ignore)
+                 (const :tag "Emphasize a hunk to revert"
+                        diff-hl-revert-emphasize-hunk)
                  (const :tag "Highlight the first column"
-                        diff-hl-revert-highlight-first-column)))
+                        diff-hl-revert-highlight-first-column))
+  :package-version '(diff-hl . "1.8.9"))
+
+
 
 (defcustom diff-hl-global-modes '(not image-mode)
   "Modes for which `diff-hl-mode' is automagically turned on.
@@ -521,6 +526,23 @@ in the source file, or the last line of the hunk above it."
                 (unless (looking-at "^-")
                   (cl-decf to-go))))))))))
 
+(defface diff-hl-not-reverted-hunks
+  '((((background light)) (:foreground "gray70"))
+    (((background dark)) (:foreground "gray40")))
+  "Face used to de-emphasize the hunks that will not be reverted."
+  :package-version '(diff-hl . "1.8.9"))
+
+(defun diff-hl-revert-emphasize-hunk (end)
+  (let ((buffer-beg (point-min))
+        (buffer-end (point-max))
+        (hunk-beg (point))
+        (hunk-end end))
+    (overlay-put (make-overlay buffer-beg hunk-beg)
+                 'face 'diff-hl-not-reverted-hunks)
+    (overlay-put (make-overlay hunk-beg hunk-end) 'face 'bold)
+    (overlay-put (make-overlay hunk-end buffer-end)
+                 'face 'diff-hl-not-reverted-hunks)))
+
 (defface diff-hl-reverted-hunk-highlight
   '((default :inverse-video t))
   "Face used to highlight the first column of the hunk to be reverted.")

[dgutov]

fancy-narrow only 400 LOC, include comments and black. : )

There are other reasons not to inline it: the users might have already customized its "blocked" face (or themes did). We'd miss out on some bugfixes too, I guess.

[dgutov]

@yugaego Thanks for the diff, see below how both options look in my session.

fancy-narrow:

emphazie-hunk:

Fancy-narrow probably does a better job hiding the other hunk (which just added two empty lines), but both seem a little... untidy? I don't know. Happy to have either as an option, though.

Let's consider some more options. First, a simple narrowing to the hunk but with context added (we know how to do that now). It would look like this:

It still looks a little "barren" with smaller hunks, though:

[dgutov]

And going back to the current default behavior, IMHO it might be primarily problematic with dark-background themes, where the first column doesn't stand out as obviously. Because this looks pretty obvious to me:

Obviously I'm biased. But perhaps someone would like to propose a different definition for the diff-hl-reverted-hunk-highlight face, which would make it stand out on dark-bg themes more?

I've also considered decorating the hunk boundaries, but the :box attribute is not very flexible, and it won't work when the hunk is the last in the buffer. It doesn't look particularly good:

Or instead of the first column we could use the fringe (or the margin) to indicate the lines which will be used. Though I'm not sure which graphic/color to use (or chars, for the margin).

[yugaego]

Hi Dmitry,

a simple narrowing to the hunk but with context added (we know how to do that now)

I suspect that's what many (maybe most) of the users would expect to see.

A similar approach was discussed before:

Maybe the suggested narrow-to-region solution (perhaps showing a bit wider area) could replace the current default?

That can work, though I was kind of worried to show only one hunk (which often might contain only a few lines) without any context. The rest of the window being empty, it seems like a waste of screen space.

If that's still a worry, my preference would go to the "fancy-narrow" like output.
("emphasize-hunk" indeed was bad at un-fontifying the other hunks.)
If this approach is chosen, I could probably extract the basic parts from the fancy-narrow, so that avoid non-maintained third-party package dependency. I assume it could result in something similar to "emphasize-hunk" in the code structure , but with one more definition added (close to fancy-narrow-properties) and diff-hl-revert-emphasize-hunk improved (extracted from fancy-narrow-to-region + fancy-narrow--propertize-region).

going back to the current default behavior, IMHO it might be primarily problematic with dark-background themes, where the first column doesn't stand out as obviously.

Yeah, might be. Your light-bg screenshot show better visibility than on my dark-bg setup.

But perhaps someone would like to propose a different definition for the diff-hl-reverted-hunk-highlight face, which would make it stand out on dark-bg themes more?

I had considered a couple of options but wasn't fascinated with the results.

but the :box attribute is not very flexible, and it won't work when the hunk is the last in the buffer. It doesn't look particularly good

Yeah. It doesn't look like a good tool to signify a hunk.

instead of the first column we could use the fringe (or the margin) to indicate the lines which will be used.

I'm afraid fringes would suffer again from the insufficient visibility.

[dgutov]

Hi Yuzhana,

A similar approach was discussed before:

Yup. It might be better with the added context. Let's try it.

If this approach is chosen, I could probably extract the basic parts from the fancy-narrow, so that avoid non-maintained third-party package dependency. I assume it could result in something similar to "emphasize-hunk" in the code structure , but with one more definition added (close to fancy-narrow-properties) and diff-hl-revert-emphasize-hunk improved (extracted from fancy-narrow-to-region + fancy-narrow--propertize-region).

I just noticed that it's annotated as "unsupported". If you or someone else prefer this approach, I'm happy to review such PR, but as for the default behavior, one of the other options seem better so far (either narrow-to-region or the current default 🤷).

It could be decided by a vote, though, sometime later. Someone could open a poll on Reddit.

[yugaego]

Yup. It might be better with the added context. Let's try it.

Looks great to me. 👍🏻

[zw963]

When i try to do revert use C-x v n, or pressing r from show hunk UI, always get failing message:

diff-beginning-of-hunk: Can’t find the beginning of the hunk.

so, if you meet same issue as me?

[dgutov]

@zw963 I can repro.

Will need to add explicit support for diff-hl-show-staged-changes=nil to diff-hl-revert-hunk.

[dgutov]

That happens when you have some staged changes, right?

[zw963]

Will need to add explicit support for diff-hl-show-staged-changes=nil to diff-hl-revert-hunk.

Yes, i done it.

That happens when you have some staged changes, right?

No, it can reproduce use my (many) config on some cases (no stage)

but, after use only diff-hl.el test it again, i can't reproduce it now, so, i consider another package broken this.

after check it, this issue caused by following code in my own config, when fancy-narrow is required, revert failed.

(defun diff-hl-revert-narrow-to-hunk (end)
  (if (fboundp 'fancy-narrow-to-region)
      (fancy-narrow-to-region (point) end)
    (narrow-to-region (point) end)))

(setq diff-hl-highlight-revert-hunk-function #'diff-hl-revert-narrow-to-hunk)

After move above code, revert start to working now.

thank you.

[dgutov]

Oh, that's a different bug. :-(

[dgutov]

I'm not sure what exactly, but something in fancy-narrow breaks diff-find-file-name, stopping it from reading the needed header. Maybe it's the intangible property or whatever, but it's not supposed to work anymore (inhibit-point-motion-hooks defaults to t these days). Debugging didn't help much, and the Commentary in fancy-narrow doesn't recommend to use it in Lisp programs anyway, so... anybody who really wants to use it, is welcome to help figuring this out.

[dgutov]

Will need to add explicit support for diff-hl-show-staged-changes=nil to diff-hl-revert-hunk.

This is now done, BTW. With this setting, diff-hl-revert-hunk now works with changes against the staging area.

[zw963]

Will need to add explicit support for diff-hl-show-staged-changes=nil to diff-hl-revert-hunk.

In fact, i am not ensure what this support means,

anyway, i guess your means is, when set diff-hl-show-staged-changes=nil(yes, i set it), and i test on newest master(6fa3af0), i can revert code sccessful now with C-x v n Or press r from show hunk UI.

[zw963]

When i try to do revert use C-x v n, or pressing r from show hunk UI, always get failing message:

diff-beginning-of-hunk: Can’t find the beginning of the hunk.

so, if you meet same issue as me?

Just for report.
And, for this issue. (when use with fancy-narrow), i get following error message instead original.

ad-Advice-buffer-substring: Args out of range: #, 935, 937

And it seem like introduce a new issue on master, i will report on #117

[dgutov]

In fact, i am not ensure what this support means,

It means that the "revert" command works against the staged area as well. I.e. when some version of the current file is staged, the changes you can revert will be computed against this staged version.

anyway, i guess your means is, when set diff-hl-show-staged-changes=nil(yes, i set it), and i test on newest master(6fa3af0), i can revert code sccessful now with C-x v n Or press r from show hunk UI.

The previous mismatch in the face of staged changes could indeed bring errors and surprising behavior.

And, for this issue. (when use with fancy-narrow), i get following error message instead original.

I had to give up on investigating the fancy-narrow integration. But patches welcome.

And it seem like introduce a new issue on master

Were you going to file a new issue?

[zw963]

the changes you can revert will be computed against this staged version.

So, what you means revert here for staged version, is git reset on command line, right?

Were you going to file a new issue?

Sorry, i mean #177 , you saw it.