vc-modify-change-comment for "modern" backend fix

vc-modify-change-comment for "modern" backend fix

[Alfred M. Szmidt]

I'm trying to add vc-modify-change-comment functionality to vc-fossil,
but running into a small issue in log-view.el, namley the following
mentioned in vc.el:

;; - Second, `log-view-modify-change-comment' doesn't seem to support
;;   modern backends at all because `log-view-extract-comment'
;;   unconditionally calls `log-view-current-file'.  This should be easy to
;;   fix.

There are two ways to fix it, one is to just check if
log-view-per-file-logs is set before calling log-view-extract-comment,
or simply make sure that log-view-current-file behaves.

Attached two patches, the first modifies all calleers and
log-view-current-file, the second just log-view-extract-comment.  I
think the first one is preferable...

Thoughts?

diff --git a/lisp/vc/log-view.el b/lisp/vc/log-view.el
index 415b1564ed..791ef66c92 100644
--- a/lisp/vc/log-view.el
+++ b/lisp/vc/log-view.el
@@ -282,22 +282,24 @@ log-view-dir-re
 
 (defun log-view-current-file ()
   "Return the current file."
-  (save-excursion
-    (forward-line 1)
-    (or (re-search-backward log-view-file-re nil t)
-	(re-search-forward log-view-file-re nil t)
-	(error "Unable to determine the current file"))
-    (let* ((file (match-string 1))
-	   (cvsdir (and (re-search-backward log-view-dir-re nil t)
-			(match-string 1)))
-	   (pcldir (and (boundp 'cvs-pcl-cvs-dirchange-re)
-			(re-search-backward cvs-pcl-cvs-dirchange-re nil t)
-			(match-string 1)))
-	   (dir ""))
-      (let ((default-directory ""))
-	(when pcldir (setq dir (expand-file-name pcldir dir)))
-	(when cvsdir (setq dir (expand-file-name cvsdir dir))))
-      (expand-file-name file dir))))
+  (if log-view-per-file-logs
+      (save-excursion
+        (forward-line 1)
+        (or (re-search-backward log-view-file-re nil t)
+	    (re-search-forward log-view-file-re nil t)
+	    (error "Unable to determine the current file"))
+        (let* ((file (match-string 1))
+	       (cvsdir (and (re-search-backward log-view-dir-re nil t)
+			    (match-string 1)))
+	       (pcldir (and (boundp 'cvs-pcl-cvs-dirchange-re)
+			    (re-search-backward cvs-pcl-cvs-dirchange-re nil t)
+			    (match-string 1)))
+	       (dir ""))
+          (let ((default-directory ""))
+	    (when pcldir (setq dir (expand-file-name pcldir dir)))
+	    (when cvsdir (setq dir (expand-file-name cvsdir dir))))
+          (expand-file-name file dir)))
+    (car log-view-vc-fileset)))
 
 (defun log-view-current-entry (&optional pos move)
   "Return the position and revision tag of the Log View entry at POS.
@@ -512,9 +514,7 @@ log-view-find-revision
       (user-error "Multiple files shown in this buffer, cannot use this command here")))
   (save-excursion
     (goto-char pos)
-    (switch-to-buffer (vc-find-revision (if log-view-per-file-logs
-					    (log-view-current-file)
-					  (car log-view-vc-fileset))
+    (switch-to-buffer (vc-find-revision (log-view-current-file)
 					(log-view-current-tag)))))
 
 
@@ -541,9 +541,7 @@ log-view-extract-comment
 (defun log-view-modify-change-comment ()
   "Edit the change comment displayed at point."
   (interactive)
-  (vc-modify-change-comment (list (if log-view-per-file-logs
-				      (log-view-current-file)
-				    (car log-view-vc-fileset)))
+  (vc-modify-change-comment (list (log-view-current-file))
 			    (log-view-current-tag)
 			    (log-view-extract-comment)))
 
@@ -558,9 +556,7 @@ log-view-annotate-version
       (user-error "Multiple files shown in this buffer, cannot use this command here")))
   (save-excursion
     (goto-char pos)
-    (vc-annotate (if log-view-per-file-logs
-		     (log-view-current-file)
-		   (car log-view-vc-fileset))
+    (vc-annotate (log-view-current-file)
 		 (log-view-current-tag))))
 
 ;;
@@ -620,9 +616,7 @@ log-view-diff-common
              ;; diff for all the files in the changeset, pass NIL for
              ;; the file list.
              (unless whole-changeset
-               (if log-view-per-file-logs
-                   (list (log-view-current-file))
-                 log-view-vc-fileset)))
+                   (list (log-view-current-file))))
      fr to)))
 
 (provide 'log-view)

diff --git a/lisp/vc/log-view.el b/lisp/vc/log-view.el
index 415b1564ed..83557402fc 100644
--- a/lisp/vc/log-view.el
+++ b/lisp/vc/log-view.el
@@ -521,7 +521,10 @@ log-view-find-revision
 (defun log-view-extract-comment ()
   "Parse comment from around the current point in the log."
   (save-excursion
-    (let (st en (backend (vc-backend (log-view-current-file))))
+    (let (st en (backend (vc-backend
+                          (if log-view-per-file-logs
+                              (log-view-current-file)
+                            (car log-view-vc-fileset)))))
       (log-view-end-of-defun)
       (cond ((eq backend 'SVN)
 	     (forward-line -1)))

Re: vc-modify-change-comment for "modern" backend fix

[Alfred M. Szmidt]

On a similar thread, log-view-extract-comment is not really extensible
for other VCS's.

Ideally, this should get the actual commit message directly from the
backend for a file / revision; but there is no good function for doing
that -- vc-print-log is the closest, but the output of that depends on
the backend.

What would be the best way to go about getting just the commit message
in a semi-backend-agnostic manner?

Re: vc-modify-change-comment for "modern" backend fix

[Eli Zaretskii]

> From: "Alfred M. Szmidt" <ams@gnu.org>
> Cc: emacs-devel@gnu.org
> Date: Tue, 17 May 2022 04:36:56 -0400
> 
> On a similar thread, log-view-extract-comment is not really extensible
> for other VCS's.
> 
> Ideally, this should get the actual commit message directly from the
> backend for a file / revision; but there is no good function for doing
> that -- vc-print-log is the closest, but the output of that depends on
> the backend.
> 
> What would be the best way to go about getting just the commit message
> in a semi-backend-agnostic manner?

A new command, supported by backends in a way that leaves only the
text without the "decorations"?

Re: vc-modify-change-comment for "modern" backend fix

[Stefan Monnier]

> Ideally, this should get the actual commit message directly from the
> backend for a file / revision; but there is no good function for doing
> that -- vc-print-log is the closest, but the output of that depends on
> the backend.
> What would be the best way to go about getting just the commit message
> in a semi-backend-agnostic manner?

Add a new backend operation?

Note that maybe you don't want "just the commit message text" because
you may want to preserve extra info such as the `Author:`.


        Stefan

Re: vc-modify-change-comment for "modern" backend fix

[Alfred M. Szmidt]

Thanks Eli, and Stefan.

   Note that maybe you don't want "just the commit message text" because
   you may want to preserve extra info such as the `Author:`.

There is I think a very deep hole -- creation time, author, committer,
branch colour, etc, etc, etc.  As it is now, modify-change-comment is
quite specific in what it wants to do.

I'll whip something up ...

Re: vc-modify-change-comment for "modern" backend fix

[Dmitry Gutov]

On 17.05.2022 22:10, Alfred M. Szmidt wrote:
> Thanks Eli, and Stefan.
> 
>     Note that maybe you don't want "just the commit message text" because
>     you may want to preserve extra info such as the `Author:`.
> 
> There is I think a very deep hole -- creation time, author, committer,
> branch colour, etc, etc, etc.  As it is now, modify-change-comment is
> quite specific in what it wants to do.

The command could return the contents of the log-edit buffer, including 
the recognized headers, which currently include, for Git,

  Author, Date, No-Verify and Sign-Off.

There's also "Amend", and you may or may not reuse it to implement this 
functionality. Depending on whether you intend to support editing of 
older commits (not just the tip one).

Re: vc-modify-change-comment for "modern" backend fix

[Alfred M. Szmidt]

What about something like this, I've tested it briefly with a
vc-fossil-get-log-entry, and with RCS and it seems to behave as it
does previously -- though slightly confused by what it does for RCS
(it inserts the header, but not the commit?).

diff --git a/lisp/vc/log-view.el b/lisp/vc/log-view.el
index 415b1564ed..93e38bf3d2 100644
--- a/lisp/vc/log-view.el
+++ b/lisp/vc/log-view.el
@@ -521,17 +520,6 @@
 (defun log-view-extract-comment ()
-  "Parse comment from around the current point in the log."
-  (save-excursion
-    (let (st en (backend (vc-backend (log-view-current-file))))
-      (log-view-end-of-defun)
-      (cond ((eq backend 'SVN)
-	     (forward-line -1)))
-      (setq en (point))
-      (or (log-view-current-entry nil t)
-          (throw 'beginning-of-buffer nil))
-      (cond ((memq backend '(SCCS RCS CVS SVN))
-	     (forward-line 2))
-	    ((eq backend 'Hg)
-	     (forward-line 4)
-	     (re-search-forward "summary: *" nil t)))
-      (setq st (point))
-      (buffer-substring st en))))
+  "Extract the commit message for the current file."
+  (vc-call-backend log-view-vc-backend
+                   'get-log-entry
+                   (log-view-current-file)
+                   (log-view-current-tag)))
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 3508f684c4..01ff7db356 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -359,6 +359,11 @@
 ;;   and make sure it is displayed in the buffer's window.  The default
 ;;   implementation of this function works for RCS-style logs.
 ;;
+;; - get-log-entry (file revision)
+;;
+;;   Return a string containing the log entry for FILE at revision
+;;   REVISION.
+
 ;; - comment-history (file)
 ;;
 ;;   Return a string containing all log entries that were made for FILE.
@@ -671,12 +676,7 @@
 ;; - The git backend supports amending, but in a different
 ;;   way (press `C-c C-e' in log-edit buffer, when making a new commit).
 ;;
-;; - Second, `log-view-modify-change-comment' doesn't seem to support
-;;   modern backends at all because `log-view-extract-comment'
-;;   unconditionally calls `log-view-current-file'.  This should be easy to
-;;   fix.
-;;
-;; - Third, doing message editing in log-view might be a natural way to go
+;; - Second, doing message editing in log-view might be a natural way to go
 ;;   about it, but editing any but the last commit (and even it, if it's
 ;;   been pushed) is a dangerous operation in Git, which we shouldn't make
 ;;   too easy for users to perform.
@@ -3158,6 +3158,23 @@ vc-default-show-log-entry
   (with-no-warnings
    (log-view-goto-rev rev)))
 
+(defun vc-default-get-log-entry (_backend _file _revision)
+  (save-excursion
+    (let (st en (backend (vc-backend (log-view-current-file))))
+      (log-view-end-of-defun)
+      (cond ((eq backend 'SVN)
+             (forward-line -1)))
+      (setq en (point))
+      (or (log-view-current-entry nil t)
+          (throw 'beginning-of-buffer nil))
+      (cond ((memq backend '(SCCS RCS CVS SVN))
+             (forward-line 2))
+            ((eq backend 'Hg)
+             (forward-line 4)
+             (re-search-forward "summary: *" nil t)))
+      (setq st (point))
+      (buffer-substring st en))))
+
 (defun vc-default-comment-history (backend file)
   "Return a string with all log entries stored in BACKEND for FILE."
   (when (vc-find-backend-function backend 'print-log)

Re: vc-modify-change-comment for "modern" backend fix

[Eli Zaretskii]

> From: "Alfred M. Szmidt" <ams@gnu.org>
> Cc: emacs-devel@gnu.org
> Date: Wed, 18 May 2022 02:34:48 -0400
> 
> What about something like this, I've tested it briefly with a
> vc-fossil-get-log-entry, and with RCS and it seems to behave as it
> does previously -- though slightly confused by what it does for RCS
> (it inserts the header, but not the commit?).

Maybe it's me, but it looks a bit inelegant: log-view calls the VC
backend, which then turns around and calls back into log-view? a VC
method that isn't implemented in any backend, but instead does TRT for
each backend "by hand"?  Can't we come up with something cleaner, even
if that requires to add a new function?

Thanks.

Re: vc-modify-change-comment for "modern" backend fix

[Alfred M. Szmidt]

   > What about something like this, I've tested it briefly with a
   > vc-fossil-get-log-entry, and with RCS and it seems to behave as it
   > does previously -- though slightly confused by what it does for RCS
   > (it inserts the header, but not the commit?).

   Maybe it's me, but it looks a bit inelegant: log-view calls the VC
   backend, which then turns around and calls back into log-view? a VC
   method that isn't implemented in any backend, but instead does TRT for
   each backend "by hand"?  Can't we come up with something cleaner, even
   if that requires to add a new function?

It is a bit of a kludge, yes.  If someone can come up with an idea how
this could look like, I can try implementing that.  But I won't be
able to test this for SCSS, CVS, SVN or Hg which seem to support this
-- the current patch should still keep those working as previously
though which is probobly the only thing it has going for it...

I think the modify-change-comment functionality hasn't seen much use,
and still quite confused how it is intended to work.  E.g., in RCS, I
have this as the latest commit (and in log view mode):

  ----------------------------
  revision 1.3
  date: 2022/05/18 06:32:36;  author: ams;  state: Exp;  lines: +2 -1
  Summary: this is a test message
  ----------------------------

At the beginning of the buffer, log-view-modify-change-comment, then
you get a new buffer with the following:

  revision 1.3
  date: 2022/05/18 06:32:36;  author: ams;  state: Exp;  lines: +2 -1

Note the lack of the actual log message.  And when you C-c C-c that
you get basically something that isn't very useful...

  ----------------------------
  revision 1.3
  date: 2022/05/18 06:32:36;  author: ams;  state: Exp;  lines: +2 -1
  revision 1.3
  date: 2022/05/18 06:32:36;  author: ams;  state: Exp;  lines: +2 -1
  ----------------------------

Re: vc-modify-change-comment for "modern" backend fix

[Stefan Monnier]

> I think the modify-change-comment functionality hasn't seen much use,
> and still quite confused how it is intended to work.  E.g., in RCS, I
> have this as the latest commit (and in log view mode):
>
>   ----------------------------
>   revision 1.3
>   date: 2022/05/18 06:32:36;  author: ams;  state: Exp;  lines: +2 -1
>   Summary: this is a test message
>   ----------------------------
>
> At the beginning of the buffer, log-view-modify-change-comment, then
> you get a new buffer with the following:
>
>   revision 1.3
>   date: 2022/05/18 06:32:36;  author: ams;  state: Exp;  lines: +2 -1

That's a bug.  It should contain "Summary: this is a test message".

FWIW I implemented this functionality originally for CVS and I can't
remember ever testing it on another VCS, so I'm not surprised it doesn't
work right for other VCS.  I also remember having trouble using it last
time I needed it in CVS (which was quite some years ago), so it might
also be broken for CVS.  :-(


        Stefan

Re: vc-modify-change-comment for "modern" backend fix

[Dmitry Gutov]

On 18.05.2022 09:34, Alfred M. Szmidt wrote:
> +  "Extract the commit message for the current file."
> +  (vc-call-backend log-view-vc-backend
> +                   'get-log-entry
> +                   (log-view-current-file)
> +                   (log-view-current-tag)))

Why not customize the extraction function only for backends that need it?

The backend command could be called 'extract-comment'. Which is somewhat 
different from "log entry" because the latter seems to include the 
headers, in VC parlance.

And you haven't explained thus far your plan for implementing 
'modify-change-comment' for e.g. Git.

Re: vc-modify-change-comment for "modern" backend fix

[Alfred M. Szmidt]

   On 18.05.2022 09:34, Alfred M. Szmidt wrote:
   > +  "Extract the commit message for the current file."
   > +  (vc-call-backend log-view-vc-backend
   > +                   'get-log-entry
   > +                   (log-view-current-file)
   > +                   (log-view-current-tag)))

   Why not customize the extraction function only for backends that need it?

This does that, no?  If you don't have get-log-entry, then things work
as previously.

   The backend command could be called 'extract-comment'. Which is somewhat 
   different from "log entry" because the latter seems to include the 
   headers, in VC parlance.


   And you haven't explained thus far your plan for implementing 
   'modify-change-comment' for e.g. Git.

I have no plans on implementing such functionality for git.

Re: vc-modify-change-comment for "modern" backend fix

[Dmitry Gutov]

On 19.05.2022 09:11, Alfred M. Szmidt wrote:
>     On 18.05.2022 09:34, Alfred M. Szmidt wrote:
>     > +  "Extract the commit message for the current file."
>     > +  (vc-call-backend log-view-vc-backend
>     > +                   'get-log-entry
>     > +                   (log-view-current-file)
>     > +                   (log-view-current-tag)))
> 
>     Why not customize the extraction function only for backends that need it?
> 
> This does that, no?  If you don't have get-log-entry, then things work
> as previously.

Yeah, ok. But see the other recommendations.

>     The backend command could be called 'extract-comment'. Which is somewhat
>     different from "log entry" because the latter seems to include the
>     headers, in VC parlance.
> 
> 
>     And you haven't explained thus far your plan for implementing
>     'modify-change-comment' for e.g. Git.
> 
> I have no plans on implementing such functionality for git.

Okay, but how will this work in fossil? Similar to editing a single 
commit during a rebase?

Re: vc-modify-change-comment for "modern" backend fix

[Alfred M. Szmidt]

   >     The backend command could be called 'extract-comment'. Which is somewhat
   >     different from "log entry" because the latter seems to include the
   >     headers, in VC parlance.
   > 
   > 
   >     And you haven't explained thus far your plan for implementing
   >     'modify-change-comment' for e.g. Git.
   > 
   > I have no plans on implementing such functionality for git.

   Okay, but how will this work in fossil? Similar to editing a single 
   commit during a rebase?

Fossil has no concept of rebase.  Amending makes a new commit, and the
last commit message is shown when viewing the timeline/log.

~/tt $ fossil timeline -R foo.fossil
=== 2022-05-17 ===
09:42:27 [3b410767c6] Edit [73bf888ab97bd2c3|73bf888ab9]: Edit check-in comment. (user: ams)
07:50:06 [30aa7e8b06] Edit [73bf888ab97bd2c3|73bf888ab9]: Edit check-in comment. (user: ams)
07:33:59 [1c13590b96] Edit [73bf888ab97bd2c3|73bf888ab9]: Edit check-in comment. (user: ams)
07:30:34 [73bf888ab9] zork zork zborkrkrk (user: ams tags: trunk)
07:30:02 [40289e9363] initial empty check-in (user: ams tags: trunk)
+++ no more data (5) +++
~/tt $ fossil amend -m "this is a new message" 40289e9363 -R foo.fossil 
hash:         40289e9363e0e3e1fe5e2df9c9827fe65e51f547 2022-05-17 07:30:02 UTC
tags:         trunk
comment:      this is a new message (user: ams)
~/tt $ fossil timeline -R foo.fossil
=== 2022-05-19 ===
09:56:34 [5a950dc2f6] Edit [40289e9363e0e3e1|40289e9363]: Edit check-in comment. (user: ams)
=== 2022-05-17 ===
09:42:27 [3b410767c6] Edit [73bf888ab97bd2c3|73bf888ab9]: Edit check-in comment. (user: ams)
07:50:06 [30aa7e8b06] Edit [73bf888ab97bd2c3|73bf888ab9]: Edit check-in comment. (user: ams)
07:33:59 [1c13590b96] Edit [73bf888ab97bd2c3|73bf888ab9]: Edit check-in comment. (user: ams)
07:30:34 [73bf888ab9] zork zork zborkrkrk (user: ams tags: trunk)
07:30:02 [40289e9363] this is a new message (user: ams tags: trunk)

Re: vc-modify-change-comment for "modern" backend fix

[Dmitry Gutov]

On 19.05.2022 12:58, Alfred M. Szmidt wrote:
>     Okay, but how will this work in fossil? Similar to editing a single
>     commit during a rebase?
> 
> Fossil has no concept of rebase.  Amending makes a new commit, and the
> last commit message is shown when viewing the timeline/log.
> 
> ~/tt $ fossil timeline -R foo.fossil
> === 2022-05-17 ===
> 09:42:27 [3b410767c6] Edit [73bf888ab97bd2c3|73bf888ab9]: Edit check-in comment. (user: ams)
> 07:50:06 [30aa7e8b06] Edit [73bf888ab97bd2c3|73bf888ab9]: Edit check-in comment. (user: ams)
> 07:33:59 [1c13590b96] Edit [73bf888ab97bd2c3|73bf888ab9]: Edit check-in comment. (user: ams)
> 07:30:34 [73bf888ab9] zork zork zborkrkrk (user: ams tags: trunk)
> 07:30:02 [40289e9363] initial empty check-in (user: ams tags: trunk)
> +++ no more data (5) +++
> ~/tt $ fossil amend -m "this is a new message" 40289e9363 -R foo.fossil
> hash:         40289e9363e0e3e1fe5e2df9c9827fe65e51f547 2022-05-17 07:30:02 UTC
> tags:         trunk
> comment:      this is a new message (user: ams)
> ~/tt $ fossil timeline -R foo.fossil
> === 2022-05-19 ===
> 09:56:34 [5a950dc2f6] Edit [40289e9363e0e3e1|40289e9363]: Edit check-in comment. (user: ams)
> === 2022-05-17 ===
> 09:42:27 [3b410767c6] Edit [73bf888ab97bd2c3|73bf888ab9]: Edit check-in comment. (user: ams)
> 07:50:06 [30aa7e8b06] Edit [73bf888ab97bd2c3|73bf888ab9]: Edit check-in comment. (user: ams)
> 07:33:59 [1c13590b96] Edit [73bf888ab97bd2c3|73bf888ab9]: Edit check-in comment. (user: ams)
> 07:30:34 [73bf888ab9] zork zork zborkrkrk (user: ams tags: trunk)
> 07:30:02 [40289e9363] this is a new message (user: ams tags: trunk)

So it's like the commit log is part of the content tree, edited by the 
last commit. Cool.