bug#51215: Add a navigation feature to vc and diff mode: diff-goto-line

bug#51215: Add a navigation feature to vc and diff mode: diff-goto-line

[Uwe Brauer]

[-- Attachment #1: Type: text/plain, Size: 2682 bytes --]

Tags: patch

<#secure method=smime mode=sign>
Hi all


Attached you find a patch, in a git compatible format, (to be installed
with git apply --patch-format=hg) provided by Davis Herring with Davis's
approval of course. Davis has already signed the FSF papers.

I would like to also discuss the whole idea connected to that patch.
Please tell me whether that is appropriate, and if it is not, we might
continue the discussion on the developer list. I will send just in case
a similar message to the developer mailing list. 

A bit of history to put that into context: 5 years ago Davis Herring
send a patch (with respect to vc.el and diff-mode.el) to the devel
list, see the link for further details.

[[https://lists.gnu.org/archive/html/emacs-devel/2016-02/msg01066.html][First short vc-diff patch]]

The idea of that patch can be described best by referring to is the
diff-hg package. The function `diff-hl-diff-goto-hunk' shows the
un-committed changes in a file, and allows to jump to these changes.
Strangely enough vc-diff does not provide a similar  feature. Davis' Harris
patch, however, does introduce it.

I find it especially helpful in collaboration where push and pulling
to/from a repository, using either git or mercurial. I presume most of
the users in this mailing list are acquainted with such a workflow, but
if necessary I could provide more details.

That patch was considered as interesting but certain enhancements were
required. So Davis send a much larger patch 6 month later. That patch
however was rejected as being to large. 
[[https://lists.gnu.org/archive/html/emacs-devel/2016-07/msg00727.html][Second longer patch, cannot be merged]]

I became very interested in that feature and was not able to install
Davis's second patch (most likely to many changes in the last 5 years)

His first patch, however, I could still apply and I tested it (using
mercurial) almost for a year and it worked flawlessly for me. 

That is why I strongly recommend to apply the patch, then 

    1. Either errors are found and these will be corrected or the patch removed
    2. Or even better the feature will enhanced

But I think to reject the patch, again, would not be a bit of a shame.

regards

Uwe Brauer 




In GNU Emacs 28.0.50 (build 1, x86_64-pc-linux-gnu, X toolkit, cairo version 1.14.6, Xaw3d scroll bars)
 of 2021-07-31 built on Utnapischtim
Repository revision: 83a915d3dfafd5f3d737afe1e13b75e4dd3aef96
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.11804000
System Description: Ubuntu 16.04.7 LTS

Configured using:
 'configure --prefix=/opt/emacs28 --with-x-toolkit=athena --without-pop
 --with-mailutils'


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: diff-goto-line.patch --]
[-- Type: text/patch, Size: 10872 bytes --]

# HG changeset patch
# User Davis Herring <herring@lanl.gov>
# Date 1634243435 -7200
#      Thu Oct 14 22:30:35 2021 +0200
# Node ID 18f4bbf01c0247e973b636163991fe1590459f1b
# Parent  0745c5efa8df0977a457656484c15ca4d53392c6
Add a navigation feature to vc and diff mode: diff-goto-line

* lisp/vc/vc.el (diff-goto-line): new decalare function to include the
  new diff-goto-line-functionality.

* lisp/vc/vc.el (vc-diff-finish): Modify the funcion, to suppor the
  new diff-goto-line-functionality.

* lisp/vc/diff-mode.el (diff-goto-line): New function that allows to
  jump from a specific line in the source buffer to the corresponding
  line in the buffer.

* lisp/vc/diff-mode.el (diff-hunk-header-re): modify the const, so
  that it can be used with the new diff-goto-line-functionality.

* lisp/vc/diff-mode.el (diff-hunk-header-re-context): modify the
  const, so that it can be used with the new
  diff-goto-line-functionality.

* lisp/vc/diff-mode.el (diff-hunk-header-re): Modify the const, to be
  used with new diff-goto-line-functionality.

* lisp/vc/diff-mode.el: (diff-hunk-header-re-context): New variable
  that allows to jump from a specific line in the source buffer to the
  corresponding line in the diff buffer.

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -442,8 +442,10 @@
 
 (defconst diff-hunk-header-re-unified
   "^@@ -\\([0-9]+\\)\\(?:,\\([0-9]+\\)\\)? \\+\\([0-9]+\\)\\(?:,\\([0-9]+\\)\\)? @@")
+(defconst diff-hunk-header-re-context
+  "\\*\\{15\\}\\(?: .*\\)?\n\\*\\*\\* \\(\\([0-9]+\\)\\(?:,\\(-?[0-9]+\\)\\)?\\) \\*\\*\\*\\*")
 (defconst diff-context-mid-hunk-header-re
-  "--- \\([0-9]+\\)\\(?:,\\([0-9]+\\)\\)? ----$")
+  "^--- \\([0-9]+\\)\\(?:,\\([0-9]+\\)\\)? ----$")
 
 (defvar diff-use-changed-face (and (face-differs-from-default-p 'diff-changed)
 				   (not (face-equal 'diff-changed 'diff-added))
@@ -526,7 +528,9 @@
 See https://lists.gnu.org/r/emacs-devel/2007-11/msg01990.html")
 
 (defconst diff-hunk-header-re
-  (concat "^\\(?:" diff-hunk-header-re-unified ".*\\|\\*\\{15\\}.*\n\\*\\*\\* .+ \\*\\*\\*\\*\\|[0-9]+\\(,[0-9]+\\)?[acd][0-9]+\\(,[0-9]+\\)?\\)$"))
+  (concat "^\\(?:" diff-hunk-header-re-unified ".*\\|"
+          diff-hunk-header-re-context
+          "\\|[0-9]+\\(,[0-9]+\\)?[acd][0-9]+\\(,[0-9]+\\)?\\)$"))
 (defconst diff-file-header-re (concat "^\\(--- .+\n\\+\\+\\+ \\|\\*\\*\\* .+\n--- \\|[^-+!<>0-9@* \n]\\).+\n" (substring diff-hunk-header-re 1)))
 
 (defconst diff-separator-re "^--+ ?$")
@@ -716,6 +720,46 @@
 (easy-mmode-define-navigation
  diff-file diff-file-header-re "file" diff-end-of-file)
 
+(defun diff-goto-line (file line column)
+  "Go to the place in this diff producing LINE and COLUMN in FILE.
+If LINE (in the new version of FILE) is included (in the diff
+buffer as +/! line or a context line), move to it and then COLUMN
+characters forward.  If it is absent, go to the first hunk
+starting after LINE, or to the end if none does.  If FILE isn't
+mentioned, go to the beginning of the buffer."
+  (goto-char (point-min))
+  (while (and (re-search-forward diff-file-header-re nil 'move)
+              (not (string-equal (diff-find-file-name) file))))
+  (if (eobp) (goto-char (point-min))
+    (forward-line -1)
+    (while
+        (progn
+          (condition-case nil (diff-hunk-next)
+            (error (goto-char (point-max))))
+          (cond ((eobp) nil)            ; end of the line
+                ((looking-at diff-hunk-header-re-unified)
+                 (let ((start (string-to-number (match-string 3)))
+                       ;; FIXME: assuming that we have the length
+                       (len (string-to-number (match-string 4))))
+                   (cond ((< line start) nil)    ; nothing found
+                         ((< line (+ start len)) ; this is our stop
+                          (dotimes (i (- line start -1))
+                            (while (progn (forward-line 1)
+                                          (eq (char-after) ?-))))
+                          (forward-char (1+ column)))
+                         (t))))         ; keep looking
+                ((looking-at diff-hunk-header-re-context)
+                 (re-search-forward diff-context-mid-hunk-header-re)
+                 (let ((start (string-to-number (match-string 2)))
+                       ;; FIXME: assuming that we have the length
+                       (end (string-to-number (match-string 3))))
+                   (cond ((< line start) nil) ; nothing found
+                         ((<= line end)       ; this is our stop
+                          (forward-line (- line start -1))
+                          (forward-char (+ column 2)))
+                         (t))))         ; keep looking
+                (t (error "Unified or context diffs only")))))))
+
 (defun diff-bounds-of-hunk ()
   "Return the bounds of the diff hunk at point.
 The return value is a list (BEG END), which are the hunk's start
@@ -1186,7 +1230,11 @@
           (inhibit-read-only t))
       (save-excursion
         (goto-char start)
-        (while (and (re-search-forward "^\\(\\(\\*\\*\\*\\) .+\n\\(---\\) .+\\|\\*\\{15\\}.*\n\\*\\*\\* \\([0-9]+\\),\\(-?[0-9]+\\) \\*\\*\\*\\*\\)\\(?: \\(.*\\)\\|$\\)" nil t)
+        (while (and (re-search-forward
+                     (concat "^\\(\\(\\*\\*\\*\\) .+\n\\(---\\) .+\\|"
+                             diff-hunk-header-re-context
+                             "\\)\\(?: \\(.*\\)\\|$\\)")
+                     nil t)
                     (< (point) end))
           (combine-after-change-calls
             (if (match-beginning 2)
@@ -1196,15 +1244,15 @@
                   (replace-match "+++" t t nil 3)
                   (replace-match "---" t t nil 2))
               ;; we matched a hunk header
-              (let ((line1s (match-string 4))
-                    (line1e (match-string 5))
+              (let ((line1s (match-string 5))
+                    (line1e (match-string 6))
                     (pt1 (match-beginning 0))
                     ;; Variables to use the special undo function.
                     (old-undo buffer-undo-list)
                     (old-end (marker-position end))
                     ;; We currently throw away the comment that can follow
                     ;; the hunk header.  FIXME: Preserve it instead!
-                    (reversible (not (match-end 6))))
+                    (reversible (not (match-end 7))))
                 (replace-match "")
                 (unless (re-search-forward
                          diff-context-mid-hunk-header-re nil t)
@@ -1282,7 +1330,11 @@
 	(inhibit-read-only t))
     (save-excursion
       (goto-char start)
-      (while (and (re-search-forward "^\\(\\([-*][-*][-*] \\)\\(.+\\)\n\\([-+][-+][-+] \\)\\(.+\\)\\|\\*\\{15\\}.*\n\\*\\*\\* \\(.+\\) \\*\\*\\*\\*\\|@@ -\\([0-9,]+\\) \\+\\([0-9,]+\\) @@.*\\)$" nil t)
+      (while (and (re-search-forward
+                   (concat "^\\(\\([-*][-*][-*] \\)\\(.+\\)\n\\([-+][-+][-+] \\)\\(.+\\)\\|"
+                           diff-hunk-header-re-context
+                           "\\|@@ -\\([0-9,]+\\) \\+\\([0-9,]+\\) @@.*\\)$")
+                   nil t)
 		  (< (point) end))
 	(combine-after-change-calls
 	  (cond
@@ -1646,13 +1698,13 @@
 
        ;; A context diff.
        ((eq (char-after) ?*)
-        (if (not (looking-at "\\*\\{15\\}\\(?: .*\\)?\n\\*\\*\\* \\([0-9]+\\)\\(?:,\\([0-9]+\\)\\)? \\*\\*\\*\\*"))
+        (if (not (looking-at diff-hunk-header-re-context))
             (error "Unrecognized context diff first hunk header format")
           (forward-line 2)
           (diff-sanity-check-context-hunk-half
-	   (if (match-end 2)
-	       (1+ (- (string-to-number (match-string 2))
-		      (string-to-number (match-string 1))))
+           (if (match-end 3)
+               (1+ (- (string-to-number (match-string 3))
+                      (string-to-number (match-string 2))))
 	     1))
           (if (not (looking-at diff-context-mid-hunk-header-re))
               (error "Unrecognized context diff second hunk header format")
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -739,6 +739,7 @@
 (require 'cl-lib)
 
 (declare-function diff-setup-whitespace "diff-mode" ())
+(declare-function diff-goto-line "diff-mode")
 
 (eval-when-compile
   (require 'dired))
@@ -1716,7 +1717,7 @@
       ;; any switches in diff-switches.
       (when (listp switches) switches))))
 
-(defun vc-diff-finish (buffer messages)
+(defun vc-diff-finish (buffer messages loc)
   ;; The empty sync output case has already been handled, so the only
   ;; possibility of an empty output is for an async process.
   (when (buffer-live-p buffer)
@@ -1730,7 +1731,8 @@
 	(diff-setup-whitespace)
 	(goto-char (point-min))
 	(when window
-	  (shrink-window-if-larger-than-buffer window)))
+	  (shrink-window-if-larger-than-buffer window))
+        (when loc (apply #'diff-goto-line loc)))
       (when (and messages (not emptyp))
 	(message "%sdone" (car messages))))))
 
@@ -1754,7 +1756,8 @@
 	 ;; but the only way to set it for each file included would
 	 ;; be to call the back end separately for each file.
 	 (coding-system-for-read
-	  (if files (vc-coding-system-for-diff (car files)) 'undecided)))
+	  (if files (vc-coding-system-for-diff (car files)) 'undecided))
+	 loc)
     ;; On MS-Windows and MS-DOS, Diff is likely to produce DOS-style
     ;; EOLs, which will look ugly if (car files) happens to have Unix
     ;; EOLs.
@@ -1791,6 +1794,19 @@
                      (if async 'async 1) "diff" file
                      (append (vc-switches nil 'diff) `(,(null-device)))))))
         (setq files (nreverse filtered))))
+    (unless rev2    ; remember the position in the current buffer
+      (let ((f files))
+        (while f
+          (let ((buf (find-buffer-visiting (car f))))
+            (when buf
+              (setq loc
+                    (with-current-buffer buf
+                      (save-restriction
+                        (widen)
+                        (list (file-relative-name (car f))
+                              (line-number-at-pos)
+                              (- (point) (line-beginning-position)))))))
+            (setq f (unless (eq buf (current-buffer)) (cdr f)))))))
     (vc-call-backend (car vc-fileset) 'diff files rev1 rev2 buffer async)
     (set-buffer buffer)
     (diff-mode)
@@ -1815,7 +1831,7 @@
       ;; after `pop-to-buffer'; the former assumes the diff buffer is
       ;; shown in some window.
       (let ((buf (current-buffer)))
-        (vc-run-delayed (vc-diff-finish buf (when verbose messages))))
+        (vc-run-delayed (vc-diff-finish buf (when verbose messages) loc)))
       ;; In the async case, we return t even if there are no differences
       ;; because we don't know that yet.
       t)))

bug#51215: Add a navigation feature to vc and diff mode: diff-goto-line

[Dmitry Gutov]

Hi Uwe,

On 14.10.2021 23:42, Uwe Brauer wrote:
> I became very interested in that feature and was not able to install
> Davis's second patch (most likely to many changes in the last 5 years)
> 
> His first patch, however, I could still apply and I tested it (using
> mercurial) almost for a year and it worked flawlessly for me.
> 
> That is why I strongly recommend to apply the patch, then
> 
>      1. Either errors are found and these will be corrected or the patch removed
>      2. Or even better the feature will enhanced
> 
> But I think to reject the patch, again, would not be a bit of a shame.

The most recent discussion regarding it can be found in bug#36526.

To sum up: the patch(es) have merit, and they're waiting for someone to 
champion their inclusion: test them for bugs, compare the different 
available implementations of the "skip to" functionality, and generally 
examine how the new functionality affects the existing workflows (e.g. 
whether it should be enabled on root diffs).

bug#51215: Add a navigation feature to vc and diff mode: diff-goto-line

[Uwe Brauer]

[-- Attachment #1: Type: text/plain, Size: 2294 bytes --]

>>> "DG" == Dmitry Gutov <dgutov@yandex.ru> writes:
Hi Dmitry


Thanks for answering and clarify some points
> Hi Uwe,
> On 14.10.2021 23:42, Uwe Brauer wrote:
>> I became very interested in that feature and was not able to install
>> Davis's second patch (most likely to many changes in the last 5 years)
>> His first patch, however, I could still apply and I tested it (using
>> mercurial) almost for a year and it worked flawlessly for me.
>> That is why I strongly recommend to apply the patch, then
>> 1. Either errors are found and these will be corrected or the
>> patch removed
>> 2. Or even better the feature will enhanced
>> But I think to reject the patch, again, would not be a bit of a
>> shame.

> The most recent discussion regarding it can be found in bug#36526.

I am not that used to that interface I jumped to 
https://lists.gnu.org/archive/html/emacs-bug-tracker/2020-08/msg00633.html

But that seems to be an automatic generated message. How can I see its
history and see that it is really Davis' original patch? The first or
the second. As I said I only tested the first one, for a year now.

> To sum up: the patch(es) have merit, and they're waiting for someone
> to champion their inclusion: test them for bugs, compare the different
> available implementations of the "skip to" functionality, and
> generally examine how the new functionality affects the existing
> workflows (e.g. whether it should be enabled on root diffs).

Forgive me my ignorance (maybe because I am  a mercurial user 😇)
the documentation vc-root-diff talks about tree revision

So if I understand that correctly this is for the case I want really
consider the diff of the repository? 

That is the same effect as if:  I change 3 files in the repository and would run 
git diff
or 
hg diff 
from the command line?

If that is the case I would say, disable the feature for that sort of
diffs  for the moment. The patch is more useful for
one-file-basis.
At least that is how  I used it for a year.

So what is the best strategy? How can I test the patch concerning 
bug#36526?

The message talks about emacs 26, I am running a 2 month old
git master emacs (still 28).
So can that patch be tested in version I use?

Regards

Uwe 



[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5673 bytes --]

bug#51215: Add a navigation feature to vc and diff mode: diff-goto-line

[Uwe Brauer]

[-- Attachment #1: Type: text/plain, Size: 534 bytes --]


> Hi Dmitry


> Thanks for answering and clarify some points


> I am not that used to that interface I jumped to 
> https://lists.gnu.org/archive/html/emacs-bug-tracker/2020-08/msg00633.html

To answer my own question:

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=%2336526

Is a different patch than the one I committed. Mine (=Davis) does not
touch the vc-root-diff command it is on a strict one-file base and
therefore simpler.

Why not apply «mine» and if there is sufficient interest switch to
2336526? 

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5673 bytes --]

bug#51215: Add a navigation feature to vc and diff mode: diff-goto-line

[Dmitry Gutov]

On 18.10.2021 10:24, Uwe Brauer wrote:
>> Hi Dmitry
> 
>> Thanks for answering and clarify some points
> 
>> I am not that used to that interface I jumped to
>> https://lists.gnu.org/archive/html/emacs-bug-tracker/2020-08/msg00633.html
> To answer my own question:
> 
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=%2336526
> 
> Is a different patch than the one I committed. Mine (=Davis) does not
> touch the vc-root-diff command it is on a strict one-file base and
> therefore simpler.
> 
> Why not apply «mine» and if there is sufficient interest switch to
> 2336526?

Read further down, we've discussed (unfortunately briefly) Davis's patch 
closer to the end: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=36526#53

I don't object to applying it, but like I said, it needs someone to 
champion it, do a proper review. I don't have the time or interest at 
the moment.

bug#51215: Add a navigation feature to vc and diff mode: diff-goto-line

[Dmitry Gutov]

Hi Uwe,

On 18.10.2021 10:16, Uwe Brauer wrote:

 >> The most recent discussion regarding it can be found in bug#36526.
 >
 > I am not that used to that interface I jumped to
 > 
https://lists.gnu.org/archive/html/emacs-bug-tracker/2020-08/msg00633.html

You can read the discussion starting from 
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=36526#53.

 >> To sum up: the patch(es) have merit, and they're waiting for someone
 >> to champion their inclusion: test them for bugs, compare the different
 >> available implementations of the "skip to" functionality, and
 >> generally examine how the new functionality affects the existing
 >> workflows (e.g. whether it should be enabled on root diffs).
 >
 > Forgive me my ignorance (maybe because I am  a mercurial user 😇)
 > the documentation vc-root-diff talks about tree revision
 >
 > So if I understand that correctly this is for the case I want really
 > consider the diff of the repository?
 >
 > That is the same effect as if:  I change 3 files in the repository 
and would run
 > git diff
 > or
 > hg diff
 > from the command line?

vc-root-diff, yes. 'C-x v D'.

 > If that is the case I would say, disable the feature for that sort of
 > diffs  for the moment. The patch is more useful for
 > one-file-basis.
 > At least that is how  I used it for a year.

OK, thanks for the feedback. But we should consider both changes in the 
proposal, ideally. Not just the one you happened to have been using for 
a period of time.

Would somebody else like to chime in?

 > So what is the best strategy? How can I test the patch concerning
 > bug#36526?
 >
 > The message talks about emacs 26, I am running a 2 month old
 > git master emacs (still 28).
 > So can that patch be tested in version I use?
That's a part of championing the change: try to see whether the patch 
still applies. If not, maybe try updating it yourself.

bug#51215: Add a navigation feature to vc and diff mode: diff-goto-line

[Juri Linkov]

>> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=36526
>> Is a different patch than the one I committed. Mine (=Davis) does not
>> touch the vc-root-diff command it is on a strict one-file base and
>> therefore simpler.
>> Why not apply «mine» and if there is sufficient interest switch to 36526?
>
> Read further down, we've discussed (unfortunately briefly) Davis's patch
> closer to the end: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=36526#53
>
> I don't object to applying it, but like I said, it needs someone to
> champion it, do a proper review. I don't have the time or interest at the
> moment.

I have been using Davis's patch for a long time
and collected a long list of its problems below.
Later I could try to help fixing all these issues:

1. diff-goto-line doesn't work with 'C-x v D'

2. 'C-c C-d' (log-edit-show-diff) leaves point at end, should leave at beginning

3. It should be customizable

4. This feature is unusable with 'C-c C-d' (log-edit-show-diff):

5. Often when committing a change 'C-c C-d' (log-edit-show-diff)
   displays only the end of the diff buffer, so some parts of the diff
   might be committed unnoticed.  This is because 'diff-goto-line'
   moves point to some random place in the diff output.  It's difficult
   to understand its logic, why it decides to move to the middle of the
   multi-file diff.
   Sometimes displaying the end of the diff buffer makes an impression
   that it's a complete diff buffer, thus a danger to miss the
   beginning of the partially hidden diff buffer.

6. 'C-c C-d' (log-edit-show-diff)
   read-file-name("Use file /dev/null: " "/dev/" "/dev/null" t "null")
   diff-find-file-name()
   diff-goto-line("Makefile" 33 0)

7. on the last hunk it signals the error: "user-error: No next hunk"

8. compilation warnings:
   vc/diff-mode.el:706:1: Warning: Unused lexical variable `i'
   vc/diff-mode.el:733:30: Warning: reference to free variable

9. signals an error:

Debugger entered--Lisp error: (wrong-type-argument stringp nil)
  string-to-number(nil)
  (let ((start (string-to-number (match-string 3))) (len (string-to-number (match-string 4))))
  (cond ((eobp) nil) ((looking-at diff-hunk-header-re-unified
  (progn (condition-case nil (diff-hunk-next)
  (while (progn (condition-case nil (diff-hunk-next)
  (if (eobp) (goto-char (point-min)) (forward-line -1) (while
  diff-goto-line("manifest.js" 1 0)
  apply(diff-goto-line ("manifest.js" 1 0))
  vc-diff-finish

bug#51215: Add a navigation feature to vc and diff mode: diff-goto-line

[Dmitry Gutov]

On 19.10.2021 09:53, Juri Linkov wrote:
>>> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=36526
>>> Is a different patch than the one I committed. Mine (=Davis) does not
>>> touch the vc-root-diff command it is on a strict one-file base and
>>> therefore simpler.
>>> Why not apply «mine» and if there is sufficient interest switch to 36526?
>>
>> Read further down, we've discussed (unfortunately briefly) Davis's patch
>> closer to the end: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=36526#53
>>
>> I don't object to applying it, but like I said, it needs someone to
>> champion it, do a proper review. I don't have the time or interest at the
>> moment.
> 
> I have been using Davis's patch for a long time
> and collected a long list of its problems below.
> Later I could try to help fixing all these issues:

Thank you for testing and cataloguing the problems. So it needs more 
work. I'll Cc Davis, perhaps he would like to continue the effort 
himself, too. Or to at least be aware of the progress.

> 1. diff-goto-line doesn't work with 'C-x v D'
> 
> 2. 'C-c C-d' (log-edit-show-diff) leaves point at end, should leave at beginning
> 
> 3. It should be customizable
> 
> 4. This feature is unusable with 'C-c C-d' (log-edit-show-diff):
> 
> 5. Often when committing a change 'C-c C-d' (log-edit-show-diff)
>     displays only the end of the diff buffer, so some parts of the diff
>     might be committed unnoticed.  This is because 'diff-goto-line'
>     moves point to some random place in the diff output.  It's difficult
>     to understand its logic, why it decides to move to the middle of the
>     multi-file diff.
>     Sometimes displaying the end of the diff buffer makes an impression
>     that it's a complete diff buffer, thus a danger to miss the
>     beginning of the partially hidden diff buffer.
> 
> 6. 'C-c C-d' (log-edit-show-diff)
>     read-file-name("Use file /dev/null: " "/dev/" "/dev/null" t "null")
>     diff-find-file-name()
>     diff-goto-line("Makefile" 33 0)
> 
> 7. on the last hunk it signals the error: "user-error: No next hunk"
> 
> 8. compilation warnings:
>     vc/diff-mode.el:706:1: Warning: Unused lexical variable `i'
>     vc/diff-mode.el:733:30: Warning: reference to free variable
> 
> 9. signals an error:
> 
> Debugger entered--Lisp error: (wrong-type-argument stringp nil)
>    string-to-number(nil)
>    (let ((start (string-to-number (match-string 3))) (len (string-to-number (match-string 4))))
>    (cond ((eobp) nil) ((looking-at diff-hunk-header-re-unified
>    (progn (condition-case nil (diff-hunk-next)
>    (while (progn (condition-case nil (diff-hunk-next)
>    (if (eobp) (goto-char (point-min)) (forward-line -1) (while
>    diff-goto-line("manifest.js" 1 0)
>    apply(diff-goto-line ("manifest.js" 1 0))
>    vc-diff-finish
> 

bug#51215: Add a navigation feature to vc and diff mode: diff-goto-line

[Uwe Brauer]

[-- Attachment #1: Type: text/plain, Size: 3976 bytes --]

>>> "JL" == Juri Linkov <juri@linkov.net> writes:

Hi Juri

I did not see your message, thanks very much for this detailed
information


>>> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=36526
>>> Is a different patch than the one I committed. Mine (=Davis) does not
>>> touch the vc-root-diff command it is on a strict one-file base and
>>> therefore simpler.
>>> Why not apply «mine» and if there is sufficient interest switch to 36526?
>> 
>> Read further down, we've discussed (unfortunately briefly) Davis's patch
>> closer to the end: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=36526#53
>> 
>> I don't object to applying it, but like I said, it needs someone to
>> champion it, do a proper review. I don't have the time or interest at the
>> moment.

> I have been using Davis's patch for a long time
> and collected a long list of its problems below.
> Later I could try to help fixing all these issues:


Great.
> 1. diff-goto-line doesn't work with 'C-x v D'

Right, that is the vc-root-diff command. As I said before in my
understanding Davis patch is just suited for a one-file-analysis. So
that could later be added

> 2. 'C-c C-d' (log-edit-show-diff) leaves point at end, should leave at beginning

Oh, I admit it is the first time I come across that command (or I have used it in the past and forgot it).

When I try it out I obtain 
,----
| Debugger entered--Lisp error: (error "Diff functionality has not been setup")
|   signal(error ("Diff functionality has not been setup"))
|   error("Diff functionality has not been setup")
|   #f(compiled-function () #<bytecode 0x9abeb64133bec99>)()
|   log-edit-show-diff()
|   funcall-interactively(log-edit-show-diff)
|   apply(funcall-interactively log-edit-show-diff nil)
|   repeat-complex-command(1)
|   funcall-interactively(repeat-complex-command 1)
|   call-interactively(repeat-complex-command nil nil)
|   command-execute(repeat-complex-command)
`----

The documentation of that function is sparse to say the least.
Could that be a git only thing?
That is getting a bit off topic but any advice would be appreciated.


> 3. It should be customizable

> 4. This feature is unusable with 'C-c C-d' (log-edit-show-diff):

> 5. Often when committing a change 'C-c C-d' (log-edit-show-diff)
>    displays only the end of the diff buffer, so some parts of the diff
>    might be committed unnoticed.  This is because 'diff-goto-line'
>    moves point to some random place in the diff output.  It's difficult
>    to understand its logic, why it decides to move to the middle of the
>    multi-file diff.
>    Sometimes displaying the end of the diff buffer makes an impression
>    that it's a complete diff buffer, thus a danger to miss the
>    beginning of the partially hidden diff buffer.

> 6. 'C-c C-d' (log-edit-show-diff)
>    read-file-name("Use file /dev/null: " "/dev/" "/dev/null" t "null")
>    diff-find-file-name()
>    diff-goto-line("Makefile" 33 0)

I need to set this up to reproduce it

> 7. on the last hunk it signals the error: "user-error: No next hunk"

> 8. compilation warnings:
>    vc/diff-mode.el:706:1: Warning: Unused lexical variable `i'
>    vc/diff-mode.el:733:30: Warning: reference to free variable

Well that was written in 2016 without lexical binding I suppose

> 9. signals an error:

> Debugger entered--Lisp error: (wrong-type-argument stringp nil)
>   string-to-number(nil)
>   (let ((start (string-to-number (match-string 3))) (len (string-to-number (match-string 4))))
>   (cond ((eobp) nil) ((looking-at diff-hunk-header-re-unified
>   (progn (condition-case nil (diff-hunk-next)
>   (while (progn (condition-case nil (diff-hunk-next)
>   (if (eobp) (goto-char (point-min)) (forward-line -1) (while
>   diff-goto-line("manifest.js" 1 0)
>   apply(diff-goto-line ("manifest.js" 1 0))
>   vc-diff-finish

I can reproduce this. Can you give me a recipe


Uwe 

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5673 bytes --]

bug#51215: Add a navigation feature to vc and diff mode: diff-goto-line

[Uwe Brauer]

[-- Attachment #1: Type: text/plain, Size: 1905 bytes --]

>>> "JL" == Juri Linkov <juri@linkov.net> writes:

>>> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=36526
>>> Is a different patch than the one I committed. Mine (=Davis) does not
>>> touch the vc-root-diff command it is on a strict one-file base and
>>> therefore simpler.
>>> Why not apply «mine» and if there is sufficient interest switch to 36526?
>> 
>> Read further down, we've discussed (unfortunately briefly) Davis's patch
>> closer to the end: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=36526#53
>> 
>> I don't object to applying it, but like I said, it needs someone to
>> champion it, do a proper review. I don't have the time or interest at the
>> moment.

> I have been using Davis's patch for a long time
> and collected a long list of its problems below.
> Later I could try to help fixing all these issues:

> 1. diff-goto-line doesn't work with 'C-x v D'

> 2. 'C-c C-d' (log-edit-show-diff) leaves point at end, should leave at beginning


Ok I figured that function out, now. It is in the vc-log buffer. I find 
(diff-hl-diff-goto-hunk much more useful but that is a question of
taste).

I cannot reproduce the behavior you describe in my workflow.

The pointer is neither at the end nor at the beginning but on some hunk (I
did not figured out, why to this particular hunk). Are you saying the
default behavior is that is should jump to the beginning of the diff
buffer? Hm seems odd to me.

There is another issue: Dmitry's very nice diff-hl pkg seems to have
implemented lately a very similar feature that the one provided by Davis patch.

However, I am struggling currently with this new functionality in some
circumstances and I am in contact with him to resolve it.

In case diff-hg finally works as expected, the question is then: should
Davis patch or an improved version of it still be  included in GNU emacs vanilla?

Uwe  

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5673 bytes --]

bug#51215: Add a navigation feature to vc and diff mode: diff-goto-line

[Juri Linkov]

Hi Uwe,

> I did not see your message, thanks very much for this detailed information

My messages sent to the maillist get lost, so I will try to send again, sorry.

bug#51215: Add a navigation feature to vc and diff mode: diff-goto-line

[Lars Ingebrigtsen]

Uwe Brauer <oub@mat.ucm.es> writes:

> There is another issue: Dmitry's very nice diff-hl pkg seems to have
> implemented lately a very similar feature that the one provided by Davis patch.
>
> However, I am struggling currently with this new functionality in some
> circumstances and I am in contact with him to resolve it.
>
> In case diff-hg finally works as expected, the question is then: should
> Davis patch or an improved version of it still be included in GNU
> emacs vanilla?

Reading this bug report, it seems like the proposed patch is
problematic, so it can't be applied as is, at least.

Was any further work done here?  Or is this no longer relevant now that
the diff-hl implements something similar?

bug#51215: Add a navigation feature to vc and diff mode: diff-goto-line

[Lars Ingebrigtsen]

Uwe Brauer <oub@mat.ucm.es> writes:

> There is another issue: Dmitry's very nice diff-hl pkg seems to have
> implemented lately a very similar feature that the one provided by Davis patch.
>
> However, I am struggling currently with this new functionality in some
> circumstances and I am in contact with him to resolve it.
>
> In case diff-hg finally works as expected, the question is then: should
> Davis patch or an improved version of it still be included in GNU
> emacs vanilla?

Reading this bug report, it seems like the proposed patch is
problematic, so it can't be applied as is, at least.

Was any further work done here?  Or is this no longer relevant now that
the diff-hl implements something similar?

bug#51215: Add a navigation feature to vc and diff mode: diff-goto-line

[Uwe Brauer]

[-- Attachment #1: Type: text/plain, Size: 635 bytes --]


> Uwe Brauer <oub@mat.ucm.es> writes:

> Reading this bug report, it seems like the proposed patch is
> problematic, so it can't be applied as is, at least.

> Was any further work done here?  Or is this no longer relevant now that
> the diff-hl implements something similar?

I think Dmitry is more qualified to answer this. In my experience the
implementation in diff-hl covers most of (my) use cases.



-- 
I strongly condemn Putin's war of aggression against the Ukraine.
I support to deliver weapons to Ukraine's military. 
I support the ban of Russia from SWIFT.
I support the EU membership of the Ukraine. 

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5673 bytes --]

bug#51215: Add a navigation feature to vc and diff mode: diff-goto-line

[Dmitry Gutov]

On 10.09.2022 10:15, Uwe Brauer wrote:
>> Uwe Brauer<oub@mat.ucm.es>  writes:
>> Reading this bug report, it seems like the proposed patch is
>> problematic, so it can't be applied as is, at least.
>> Was any further work done here?  Or is this no longer relevant now that
>> the diff-hl implements something similar?
> I think Dmitry is more qualified to answer this. In my experience the
> implementation in diff-hl covers most of (my) use cases.

I can't really answer that.

IMHO diff-hl (with its full feature set) might as well be in the default 
Emacs configuration, but I've seen few requests on emacs-devel for that 
to happen.

When the ELPA bundling feature arrives, though, it should be easy enough 
to do, if the consensus is in favor.

Until then, I'd say this feature can be worthwhile to have separately, 
but someone(tm) should really work on the patch and iron out the kinks.