bug#64055: [WIP Patch] Enable editing commit messages - vc-git-modify-change-comment

bug#64055: [WIP Patch] Enable editing commit messages - vc-git-modify-change-comment

[Morgan Smith]

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

Hello!


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-enable-editing-log-comments.patch --]
[-- Type: text/x-patch, Size: 4832 bytes --]

From dfff1a074dbcc244a1d6b40694b559c12c331a8f Mon Sep 17 00:00:00 2001
From: Morgan Smith <Morgan.J.Smith@outlook.com>
Date: Tue, 13 Jun 2023 18:33:56 -0400
Subject: [PATCH] enable editing log comments

---
 lisp/vc/log-view.el      | 11 ++++++++---
 lisp/vc/vc-dispatcher.el |  4 +++-
 lisp/vc/vc-git.el        | 26 +++++++++++++++++++++++++-
 lisp/vc/vc.el            | 10 ----------
 4 files changed, 36 insertions(+), 15 deletions(-)

diff --git a/lisp/vc/log-view.el b/lisp/vc/log-view.el
index e6eb6a5b973..86911eeb02c 100644
--- a/lisp/vc/log-view.el
+++ b/lisp/vc/log-view.el
@@ -522,9 +522,11 @@ 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 log-view-vc-backend))
+      (unless (get-text-property (car (log-view-current-entry)) 'log-view-entry-expanded)
+        (log-view-toggle-entry-display))
       (log-view-end-of-defun)
-      (cond ((eq backend 'SVN)
+      (cond ((memq backend '(SVN Git))
 	     (forward-line -1)))
       (setq en (point))
       (or (log-view-current-entry nil t)
@@ -533,7 +535,10 @@ log-view-extract-comment
 	     (forward-line 2))
 	    ((eq backend 'Hg)
 	     (forward-line 4)
-	     (re-search-forward "summary: *" nil t)))
+	     (re-search-forward "summary: *" nil t))
+            ((eq backend 'Git)
+             (re-search-forward "^$" nil t)
+             (forward-line 1)))
       (setq st (point))
       (buffer-substring st en))))
 
diff --git a/lisp/vc/vc-dispatcher.el b/lisp/vc/vc-dispatcher.el
index fd5f655a0f6..4351a71977e 100644
--- a/lisp/vc/vc-dispatcher.el
+++ b/lisp/vc/vc-dispatcher.el
@@ -827,7 +827,9 @@ vc-dispatcher-browsing
   "Are we in a directory browser buffer?"
   (or (derived-mode-p 'vc-dir-mode)
       (derived-mode-p 'dired-mode)
-      (derived-mode-p 'diff-mode)))
+      (derived-mode-p 'diff-mode)
+      (derived-mode-p 'log-view-mode)
+      ))
 
 ;; These are unused.
 ;; (defun vc-dispatcher-in-fileset-p (fileset)
diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index a3469b71386..18d6f1f47dc 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -1384,6 +1384,24 @@ vc-git-clone
     (vc-git--out-ok "clone" remote directory))
   directory)
 
+(defun vc-git-modify-change-comment (_files rev comment)
+  "Modify the change comments on REV to COMMENT."
+  ;; This is very similar to using to "git commit --fixup=amend"
+  ;; command but it is more precise as it does the rebase matching
+  ;; with the hash instead of the subject line.  Also we can't use
+  ;; --fixup non-interactively (it doesn't support -m or -F) so this
+  ;; is much easier.
+  (vc-git-command nil 0 nil "commit"
+                  "--allow-empty"
+                  "-m" (concat "amend! " rev "\n\n" comment))
+  ;; We should really be able to do this "non-interactively" but we
+  ;; can't so we set GIT_SEQUENCE_EDITOR
+  (let ((process-environment
+         (cons
+          "GIT_SEQUENCE_EDITOR=:"
+          process-environment)))
+    (vc-git-command nil 0 nil "rebase" "--autosquash" "-i" (concat rev "~1"))))
+
 ;;; HISTORY FUNCTIONS
 
 (autoload 'vc-setup-buffer "vc-dispatcher")
@@ -1576,7 +1594,13 @@ vc-git-expanded-log-entry
     (apply #'vc-git-command t nil nil
            `("log"
              ,revision
-             "-1"  "--no-color" ,@(ensure-list vc-git-log-switches)
+             "-1"  "--no-color"
+             ;; The same as the default "medium" format but it doesn't
+             ;; put spaces at the beginning of the body.  This is so
+             ;; we can grab this as the initial value when calling
+             ;; log-view-modify-change-comment
+             "--pretty=format:commit %H%nAuthor: %an %ae%nDate:   %ad%n%n%B"
+             ,@(ensure-list vc-git-log-switches)
              "--"))
     (goto-char (point-min))
     (unless (eobp)
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index a93d85caedb..92a0ef74d56 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -699,16 +699,6 @@
 ;; - 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
-;;   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.
-;;
 ;;   There should be a check that the given comment is not reachable
 ;;   from any of the "remote" refs?
 ;;
-- 
2.40.1


[-- Attachment #3: Type: text/plain, Size: 1152 bytes --]


Please look at this patch.  Ignore the bad commit message for now, I'm
trying to figure out how to edit that :P.

I've spent far too long looking at git documentation and source code.  I
think this is the best way to enable this feature.  In fact I really
don't think there is another way to implement this feature that doesn't
rely on the "autosquash" git feature.

I found the hack located in vc-git-modify-change-comment' of setting
"GIT_SEQUENCE_EDITOR=:" on the internet and I don't understand how it
works.

Also I'm not really sure what the 'vc-dispatcher-browsing' function is
supposed to do but it's getting in my way.  Like are 'diff-mode' buffers
a "directory browser buffer"?  I don't understand the intent here.

Also you'll noticed I deleted some comments in vc.el.  That's mainly to
highlight them so we can talk about them now.  Is there a good way to
stop users from editing remote commits?


I feel like I should probably just keep working on this and get it more
polished and answer some of my own questions before posting here but I'm
getting kind of frustrated with this.  I am many hours deep into this
problem.

Thanks,

Morgan

bug#64055: [WIP Patch] Enable editing commit messages - vc-git-modify-change-comment

[Robert Pluim]

>>>>> On Tue, 13 Jun 2023 18:59:24 -0400, Morgan Smith <Morgan.J.Smith@outlook.com> said:

    Morgan> Please look at this patch.  Ignore the bad commit message for now, I'm
    Morgan> trying to figure out how to edit that :P.

    Morgan> I've spent far too long looking at git documentation and source code.  I
    Morgan> think this is the best way to enable this feature.  In fact I really
    Morgan> don't think there is another way to implement this feature that doesn't
    Morgan> rely on the "autosquash" git feature.

Does 'git commit --amend -m' not work?

    Morgan> I found the hack located in vc-git-modify-change-comment' of setting
    Morgan> "GIT_SEQUENCE_EDITOR=:" on the internet and I don't understand how it
    Morgan> works.

    Morgan> Also I'm not really sure what the 'vc-dispatcher-browsing' function is
    Morgan> supposed to do but it's getting in my way.  Like are 'diff-mode' buffers
    Morgan> a "directory browser buffer"?  I don't understand the intent here.

    Morgan> Also you'll noticed I deleted some comments in vc.el.  That's mainly to
    Morgan> highlight them so we can talk about them now.  Is there a good way to
    Morgan> stop users from editing remote commits?

You canʼt stop anyone from doing anything on their local system, but I
guess you could check if 'git merge-base' is upstream.

    Morgan> I feel like I should probably just keep working on this and get it more
    Morgan> polished and answer some of my own questions before posting here but I'm
    Morgan> getting kind of frustrated with this.  I am many hours deep into this
    Morgan> problem.

We can be your rubber duck :-)

Robert
-- 

bug#64055: [WIP Patch] Enable editing commit messages - vc-git-modify-change-comment

[Morgan Smith]

Robert Pluim <rpluim@gmail.com> writes:

> Does 'git commit --amend -m' not work?

This only works if you want to amend the latest commit.  If you want to
edit commit messages of older commits generally one would use the reword
feature during an interactive rebase.  To do everything from the
commandline I think we need to use the autosquash feature.

'git commit --amend' is actually already built into the Emacs vc system
and works great.  I don't think you can use it without a diff though.

> Robert

Morgan

bug#64055: [WIP Patch] Enable editing commit messages - vc-git-modify-change-comment

[Robert Pluim]

>>>>> On Wed, 14 Jun 2023 07:41:18 -0400, Morgan Smith <Morgan.J.Smith@outlook.com> said:

    Morgan> Robert Pluim <rpluim@gmail.com> writes:
    >> Does 'git commit --amend -m' not work?

    Morgan> This only works if you want to amend the latest commit.  If you want to
    Morgan> edit commit messages of older commits generally one would use the reword
    Morgan> feature during an interactive rebase.  To do everything from the
    Morgan> commandline I think we need to use the autosquash feature.

Yes, eg magitʼs interactive rebase is great for that kind of stuff. I donʼt
know offhand if it uses autosquash.

    Morgan> 'git commit --amend' is actually already built into the Emacs vc system
    Morgan> and works great.  I don't think you can use it without a diff though.

Fixing that would fit 99.99% of the cases where I want to reword a
commit message, and I suspect Iʼm not alone.

Robert
-- 

bug#64055: [WIP Patch] Enable editing commit messages - vc-git-modify-change-comment

[Morgan Smith]

Robert Pluim <rpluim@gmail.com> writes:

> Yes, eg magitʼs interactive rebase is great for that kind of stuff. I donʼt
> know offhand if it uses autosquash.

I look briefly at their code and they use a perl wrapper around the
interactive rebase.  I suppose that's another way of doing it.  I don't
think it's any better or worse then my approach though.

>
>     Morgan> 'git commit --amend' is actually already built into the Emacs vc system
>     Morgan> and works great.  I don't think you can use it without a diff though.
>
> Fixing that would fit 99.99% of the cases where I want to reword a
> commit message, and I suspect Iʼm not alone.

Strange.  I'm usually working on at least a couple commits at a time.
For example, if I'm working on a change and I notice a typo in a
docstring unrelated to my change then I'll stick that into a commit.
Then I'll fix up all my commit messages at the end of my session.

Regardless, the current amend logic is great if you need to amend code
changes.  For the case without a diff, you could simply use the feature
I'm adding here.

Morgan

bug#64055: [WIP Patch] Enable editing commit messages - vc-git-modify-change-comment

[Robert Pluim]

>>>>> On Wed, 14 Jun 2023 09:54:20 -0400, Morgan Smith <Morgan.J.Smith@outlook.com> said:

    Morgan> Robert Pluim <rpluim@gmail.com> writes:
    >> Yes, eg magitʼs interactive rebase is great for that kind of stuff. I donʼt
    >> know offhand if it uses autosquash.

    Morgan> I look briefly at their code and they use a perl wrapper around the
    Morgan> interactive rebase.  I suppose that's another way of doing it.  I don't
    Morgan> think it's any better or worse then my approach though.

OK

    >> 
    Morgan> 'git commit --amend' is actually already built into the Emacs vc system
    Morgan> and works great.  I don't think you can use it without a diff though.
    >> 
    >> Fixing that would fit 99.99% of the cases where I want to reword a
    >> commit message, and I suspect Iʼm not alone.

    Morgan> Strange.  I'm usually working on at least a couple commits at a time.
    Morgan> For example, if I'm working on a change and I notice a typo in a
    Morgan> docstring unrelated to my change then I'll stick that into a commit.
    Morgan> Then I'll fix up all my commit messages at the end of my session.

That kind of thing I just leave in, and if it gets in the way Iʼll
stash it, and then commit it afterwards.  But then again, for Emacs I
try not to have too many commits in flight at the same time. To each
their own.

    Morgan> Regardless, the current amend logic is great if you need to amend code
    Morgan> changes.  For the case without a diff, you could simply use the feature
    Morgan> I'm adding here.

Right

Robert
-- 

bug#64055: [WIP Patch] Enable editing commit messages - vc-git-modify-change-comment

[Dmitry Gutov]

Hi!

This is a welcome addition, thanks for your efforts.

On 14/06/2023 01:59, Morgan Smith wrote:
> -             "-1"  "--no-color" ,@(ensure-list vc-git-log-switches)
> +             "-1"  "--no-color"
> +             ;; The same as the default "medium" format but it doesn't
> +             ;; put spaces at the beginning of the body.  This is so
> +             ;; we can grab this as the initial value when calling
> +             ;; log-view-modify-change-comment
> +             "--pretty=format:commit %H%nAuthor: %an %ae%nDate:   %ad%n%n%B"
> +             ,@(ensure-list vc-git-log-switches)

Does this improve the overall look of the log? Otherwise, we could 
remove the extra spaces when grabbing the log message (as long as we 
know how many were added), just like we do in log-edit-insert-changelog.

But an even better solution, perhaps, would be to delegate to 
(vc-call-backend backend 'expanded-log-entry revision).

Here's how it looked in my old, unfinished version of this feature:

(defun log-view-modify-change-comment ()
   "Edit the change comment displayed at point."
   (interactive)
   (let* ((file (and log-view-per-file-logs
                     (log-view-current-file)))
          (revision (log-view-current-tag))
          (backend (vc-responsible-backend (or file (car 
log-view-vc-fileset)))))
     (vc-modify-change-comment (list file)
                               revision
                               (vc-call-backend backend 
'expanded-log-entry revision))))

It will need to fall back to log-view-extract-comment for backends that 
don't have this method defined, though. Such as CVS/RCS/SVN/SCCS.

> -;; - Third, 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.

...

> I found the hack located in vc-git-modify-change-comment' of setting
> "GIT_SEQUENCE_EDITOR=:" on the internet and I don't understand how it
> works.

Apparently ":' is a shell built-in which "does nothing and then 
succeeds" (https://stackoverflow.com/a/29094904). "true" should also work.

> Also I'm not really sure what the 'vc-dispatcher-browsing' function is
> supposed to do but it's getting in my way.  Like are 'diff-mode' buffers
> a "directory browser buffer"?

The docstring is 15 years old and never updated. If we can find a better 
wording, that would be great.

 >  I don't understand the intent here.

Check out the two places where it is called.

> Also you'll noticed I deleted some comments in vc.el.  That's mainly to
> highlight them so we can talk about them now.  Is there a good way to
> stop users from editing remote commits?

We could do something like this: take the remote branch associated with 
the current branch, and its top commit. And then, somehow, see if the 
commit to be edited is reachable by traversing up history.

There likely are some known Git snippets out there on SO or general 
Internet that do these steps, but I haven't looked yet.

Anyway, we could do this check first thing inside 
vc-git-modify-change-comment and, when the operation is dangerous, 
doubly prompt the user whether they want to proceed.

Or, a more involved approach: add a new backend function which would do 
the check. Then log-view-modify-change-comment could abort earlier.

> I feel like I should probably just keep working on this and get it more
> polished and answer some of my own questions before posting here but I'm
> getting kind of frustrated with this.  I am many hours deep into this
> problem.

Please feel free to ask questions.

bug#64055: [WIP Patch] Enable editing commit messages - vc-git-modify-change-comment

[Sean Whitton]

Hello,

On Sat 17 Jun 2023 at 05:40am +03, Dmitry Gutov wrote:

> We could do something like this: take the remote branch associated with the
> current branch, and its top commit. And then, somehow, see if the commit to be
> edited is reachable by traversing up history.
>
> There likely are some known Git snippets out there on SO or general Internet
> that do these steps, but I haven't looked yet.
>
> Anyway, we could do this check first thing inside vc-git-modify-change-comment
> and, when the operation is dangerous, doubly prompt the user whether they want
> to proceed.

I think that we refuse to proceed, or at least ask for confirmation,
in the case that the commit the user wants to edit does not appear in
*vc-outgoing* (C-x v O).

The difficulty of prompting is that the prompt has to be short to fit in
the echo area.  But something like

  This commit is published; are you sure you want to rewrite history?

is only explanatory to someone who already has a lot of git-specific
knowledge.  So, by default, we I think we should refuse to rewrite any
commit that does not appear in *vc-outgoing*.

Then there could be a defcustom to enable a yes/no prompt (or to enable
unconditionally rewriting).  In the docstring for the defcustom we could
provide a reference to the relevant git documentation, so the user can
find out what this notion of "rewriting history" is all about.

-- 
Sean Whitton

bug#64055: [WIP Patch] Enable editing commit messages - vc-git-modify-change-comment

[Sean Whitton]

Hello,

On Wed 14 Jun 2023 at 03:13pm +02, Robert Pluim wrote:

>     Morgan> 'git commit --amend' is actually already built into the Emacs vc system
>     Morgan> and works great.  I don't think you can use it without a diff though.
>
> Fixing that would fit 99.99% of the cases where I want to reword a
> commit message, and I suspect Iʼm not alone.

Like Morgan, I regularly rebase solely to edit commit messages other
than that of the most recent commit.

Morgan, Dmitry, you have both posted WIP on this.  Have you made any
progress?  Are you interested in looking at it again?

-- 
Sean Whitton

bug#64055: [WIP Patch] Enable editing commit messages - vc-git-modify-change-comment

[Dmitry Gutov]

On 01/10/2024 05:37, Sean Whitton wrote:
> The difficulty of prompting is that the prompt has to be short to fit in
> the echo area.  But something like
> 
>    This commit is published; are you sure you want to rewrite history?
> 
> is only explanatory to someone who already has a lot of git-specific
> knowledge.  So, by default, we I think we should refuse to rewrite any
> commit that does not appear in*vc-outgoing*.

Yep! That sounds good to me.

bug#64055: [WIP Patch] Enable editing commit messages - vc-git-modify-change-comment

[Dmitry Gutov]

On 01/10/2024 05:38, Sean Whitton wrote:
> Morgan, Dmitry, you have both posted WIP on this.  Have you made any
> progress?  Are you interested in looking at it again?

I haven't worked on it, sorry.

If my old WIP looks useful, you're quite welcome to use it as a starting 
point. No need for attribution or anything like that.

bug#64055: [WIP Patch] Enable editing commit messages - vc-git-modify-change-comment

[Sean Whitton]

Hello,

On Tue 01 Oct 2024 at 10:32pm +03, Dmitry Gutov wrote:

> On 01/10/2024 05:38, Sean Whitton wrote:
>> Morgan, Dmitry, you have both posted WIP on this.  Have you made any
>> progress?  Are you interested in looking at it again?
>
> I haven't worked on it, sorry.
>
> If my old WIP looks useful, you're quite welcome to use it as a starting
> point. No need for attribution or anything like that.

Could you point me at that WIP, please?  It's not posted to this bug and
I couldn't find it in my mail archives.

-- 
Sean Whitton

bug#64055: [WIP Patch] Enable editing commit messages - vc-git-modify-change-comment

[Dmitry Gutov]

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

On 02/10/2024 03:01, Sean Whitton wrote:
> On Tue 01 Oct 2024 at 10:32pm +03, Dmitry Gutov wrote:
> 
>> On 01/10/2024 05:38, Sean Whitton wrote:
>>> Morgan, Dmitry, you have both posted WIP on this.  Have you made any
>>> progress?  Are you interested in looking at it again?
>> I haven't worked on it, sorry.
>>
>> If my old WIP looks useful, you're quite welcome to use it as a starting
>> point. No need for attribution or anything like that.
> Could you point me at that WIP, please?  It's not posted to this bug and
> I couldn't find it in my mail archives.

Yeah, sorry about that.

Here's the diff I have lying around from back then. Not sure how much it 
will help - the changes in log-view.el seems like an improvement (more 
generic approach), whereas the vc-git-modify-change-comment definition 
might be better in Morgan's patch (it /would/ be nice to be able to edit 
older commits, not just the most recent one).

[-- Attachment #2: log-view-modify-change-comment.diff --]
[-- Type: text/x-patch, Size: 4344 bytes --]

diff --git a/lisp/vc/log-view.el b/lisp/vc/log-view.el
index 054c2b9134..8794ce5d31 100644
--- a/lisp/vc/log-view.el
+++ b/lisp/vc/log-view.el
@@ -529,34 +529,18 @@ log-view-find-revision
 					(log-view-current-tag)))))
 
 
-(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))))
-
 (declare-function vc-modify-change-comment "vc" (files rev oldcomment))
 
 (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)))
-			    (log-view-current-tag)
-			    (log-view-extract-comment)))
+  (let* ((file (and log-view-per-file-logs
+                    (log-view-current-file)))
+         (revision (log-view-current-tag))
+         (backend (vc-responsible-backend (or file (car log-view-vc-fileset)))))
+    (vc-modify-change-comment (list file)
+                              revision
+                              (vc-call-backend backend 'expanded-log-entry))))
 
 (defun log-view-annotate-version (pos)
   "Annotate the version at POS.
diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index ca4c66a06d..d792eeb429 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -950,6 +950,49 @@ vc-git-checkin
 		    (if only (list "--only" "--") '("-a")))))
     (if (and msg-file (file-exists-p msg-file)) (delete-file msg-file))))
 
+(defun vc-git-modify-change-comment (files rev comment)
+  (let* ((file1 (or (car files) default-directory))
+         (root (vc-git-root file1))
+         (default-directory (expand-file-name root))
+         (pcsw coding-system-for-write)
+         (coding-system-for-write
+          ;; On MS-Windows, we must encode command-line arguments in
+          ;; the system codepage.
+          (if (eq system-type 'windows-nt)
+              locale-coding-system
+            (or coding-system-for-write vc-git-commits-coding-system)))
+         (msg-file
+          ;; On MS-Windows, pass the commit log message through a
+          ;; file, to work around the limitation that command-line
+          ;; arguments must be in the system codepage, and therefore
+          ;; might not support the non-ASCII characters in the log
+          ;; message.  Handle also remote files.
+          (if (eq system-type 'windows-nt)
+              (let ((default-directory (file-name-directory file1)))
+                (make-nearby-temp-file "git-msg")))))
+    (cl-flet ((boolean-arg-fn
+               (argument)
+               (lambda (value) (when (equal value "yes") (list argument)))))
+      ;; When operating on the whole tree, better pass "-a" than ".", since "."
+      ;; fails when we're committing a merge.
+      (apply 'vc-git-command nil 0 nil
+             (nconc (if msg-file (list "commit" "-F"
+                                       (file-local-name msg-file))
+                      (list "commit" "-m"))
+                    (let ((args
+                           (log-edit-extract-headers
+                            `(("Author" . "--author")
+                              ("No-Verify" . ,(boolean-arg-fn "--no-verify"))
+                              ("Sign-Off" . ,(boolean-arg-fn "--signoff")))
+                            comment)))
+                      (when msg-file
+                        (let ((coding-system-for-write
+                               (or pcsw vc-git-commits-coding-system)))
+                          (write-region (car args) nil msg-file))
+                        (setq args (cdr args)))
+                      args))))
+    (if (and msg-file (file-exists-p msg-file)) (delete-file msg-file))))
+
 (defun vc-git-find-revision (file rev buffer)
   (let* (process-file-side-effects
 	 (coding-system-for-read 'binary)

bug#64055: [WIP Patch] Enable editing commit messages - vc-git-modify-change-comment

[Sean Whitton]

Hello,

In Dmitry's patch he takes the approach of calling the
expanded-log-entry backend function to get the message to edit.
This is not a real VC backend function -- in fact it's a log-view
feature, log-view-expanded-log-entry-function.

So one thing we could do is add a VC backend action which returns just
the message text that a human might want to edit.
Probably a backend action called `get-change-comment'.

I think, though, that there might be subtle complexities there.  For
example, should there be a FILES argument, or just a REVISION argument?
For Git and Hg it's just REVISION, but we wouldn't want to bake that in.

I think, therefore, that the approach of parsing text out of the
log-view buffer is more future-proof, even though it's complex.
So please see the following WIP patch.
-- >8 --

---
 lisp/vc/log-view.el | 48 +++++++++++++++++++++++++++++++--------------
 lisp/vc/vc-git.el   |  9 +++++++++
 lisp/vc/vc-hg.el    |  8 ++++++++
 3 files changed, 50 insertions(+), 15 deletions(-)

diff --git a/lisp/vc/log-view.el b/lisp/vc/log-view.el
index e9e6602e414..f2bfe642390 100644
--- a/lisp/vc/log-view.el
+++ b/lisp/vc/log-view.el
@@ -520,23 +520,41 @@ log-view-find-revision
                                         log-view-vc-backend))))

+(defun log-view--default-extract-comment-function ()
+  (when (memq log-view-vc-backend '(SCCS RCS CVS SVN))
+    (delete-region (pos-bol) (pos-bol 3)))
+  (goto-char (point-max))
+  (when (eq log-view-vc-backend 'SVN)
+    (delete-region (pos-bol 0) (point)))
+  (buffer-string))
+
+;; We want the possibility of something backend-specific here because
+;; there are all sorts of possibilities for how the comment needs to be
+;; extracted.  For example, if the user has customized a variable like
+;; `vc-git-log-switches' then that could change how to parse out the
+;; message in `vc-git-log-view-mode'.
+(defvar log-view-extract-comment-function
+  #'log-view--default-extract-comment-function
+  "Function to return the free text part of a Log View entry.
+`log-view-extract-comment' calls this with no arguments in a
+temporary buffer containing the full text of the Log View entry.
+The default value works for the SCCS, RCS, CVS and SVN backends.")
+
 (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))))
+  (let* ((entry (or (log-view-current-entry)
+                    (throw 'beginning-of-buffer nil)))
+         (text (if log-view-expanded-log-entry-function
+                   (funcall log-view-expanded-log-entry-function
+                            (cadr entry))
+                 (save-excursion
+                   (goto-char (car entry))
+                   (log-view-end-of-defun)
+                   (buffer-substring (car entry) (point))))))
+    (with-temp-buffer
+      (insert text)
+      (goto-char (point-min))
+      (funcall log-view-extract-comment-function))))

 (declare-function vc-modify-change-comment "vc" (files rev oldcomment))

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 05400523048..0af4e4e4600 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -1577,6 +1577,7 @@ log-view-file-re
 (defvar log-view-font-lock-keywords)
 (defvar log-view-per-file-logs)
 (defvar log-view-expanded-log-entry-function)
+(defvar log-view-extract-comment-function)

 (define-derived-mode vc-git-log-view-mode log-view-mode "Git-Log-View"
   (require 'add-log) ;; We need the faces add-log.
@@ -1592,6 +1593,8 @@ vc-git-log-view-mode
     (setq truncate-lines t)
     (setq-local log-view-expanded-log-entry-function
                 'vc-git-expanded-log-entry))
+  (setq-local log-view-extract-comment-function
+              #'vc-git--extract-comment)
   (setq-local log-view-font-lock-keywords
        (if (not (memq vc-log-view-type '(long log-search with-diff)))
 	   (list (cons (nth 1 vc-git-root-log-format)
@@ -1650,6 +1653,12 @@ vc-git-expanded-log-entry
         (forward-line))
       (buffer-string))))

+(defun vc-git--extract-comment ()
+  (re-search-forward "^    " nil t)
+  (delete-region (point-min) (point))
+  ;; now deindent
+)
+
 (defun vc-git-region-history (file buffer lfrom lto)
   "Insert into BUFFER the history of FILE for lines LFROM to LTO.
 This requires git 1.8.4 or later, for the \"-L\" option of \"git log\"."
diff --git a/lisp/vc/vc-hg.el b/lisp/vc/vc-hg.el
index 876d86dc24f..9a9c9c41997 100644
--- a/lisp/vc/vc-hg.el
+++ b/lisp/vc/vc-hg.el
@@ -425,6 +425,7 @@ log-view-file-re
 (defvar log-view-font-lock-keywords)
 (defvar log-view-per-file-logs)
 (defvar log-view-expanded-log-entry-function)
+(defvar log-view-extract-comment-function)

 (define-derived-mode vc-hg-log-view-mode log-view-mode "Hg-Log-View"
   (require 'add-log) ;; we need the add-log faces
@@ -440,6 +441,8 @@ vc-hg-log-view-mode
     (setq truncate-lines t)
     (setq-local log-view-expanded-log-entry-function
                 'vc-hg-expanded-log-entry))
+  (setq-local log-view-extract-comment-function
+              #'vc-hg--extract-comment)
   (setq-local log-view-font-lock-keywords
        (if (eq vc-log-view-type 'short)
 	   (list (cons (nth 1 vc-hg-root-log-format)
@@ -541,6 +544,11 @@ vc-hg-expanded-log-entry
       (goto-char (point-max))
       (buffer-string))))

+(defun vc-hg--extract-comment ()
+  (forward-line 4)
+  (re-search-forward "summary: *" nil t)
+  (buffer-substring (point) (point-max)))
+
 (defun vc-hg-revision-table (files)
   (let ((default-directory (file-name-directory (car files))))
     (with-temp-buffer

-- 
Sean Whitton

bug#64055: [WIP Patch] Enable editing commit messages - vc-git-modify-change-comment

[Sean Whitton]

Hello,

On Tue 13 Jun 2023 at 06:59pm -04, Morgan Smith wrote:

> diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
> index a3469b71386..18d6f1f47dc 100644
> --- a/lisp/vc/vc-git.el
> +++ b/lisp/vc/vc-git.el
> @@ -1384,6 +1384,24 @@ vc-git-clone
>      (vc-git--out-ok "clone" remote directory))
>    directory)
>
> +(defun vc-git-modify-change-comment (_files rev comment)
> +  "Modify the change comments on REV to COMMENT."
> +  ;; This is very similar to using to "git commit --fixup=amend"
> +  ;; command but it is more precise as it does the rebase matching
> +  ;; with the hash instead of the subject line.  Also we can't use
> +  ;; --fixup non-interactively (it doesn't support -m or -F) so this
> +  ;; is much easier.
> +  (vc-git-command nil 0 nil "commit"
> +                  "--allow-empty"
> +                  "-m" (concat "amend! " rev "\n\n" comment))
> +  ;; We should really be able to do this "non-interactively" but we
> +  ;; can't so we set GIT_SEQUENCE_EDITOR
> +  (let ((process-environment
> +         (cons
> +          "GIT_SEQUENCE_EDITOR=:"
> +          process-environment)))
> +    (vc-git-command nil 0 nil "rebase" "--autosquash" "-i" (concat rev "~1"))))
> +
>  ;;; HISTORY FUNCTIONS

A few notes here:

- We will need to ensure that the commit is actually empty, not just
  allow it to be empty.

  We have vc-git--stash-staged-changes we could use, but on my machine
  git-commit has --fixup=reword:... which is for amending only the
  commit message.  So probably use that.

- We will probably want to pass --autostash to git-rebase, too.

- It is probably best to set GIT_SEQUENCE_EDITOR=true so that we're not
  relying on the shell -- there is no /bin/:, but there is /bin/true.
  This is minor.

> @@ -1576,7 +1594,13 @@ vc-git-expanded-log-entry
>      (apply #'vc-git-command t nil nil
>             `("log"
>               ,revision
> -             "-1"  "--no-color" ,@(ensure-list vc-git-log-switches)
> +             "-1"  "--no-color"
> +             ;; The same as the default "medium" format but it doesn't
> +             ;; put spaces at the beginning of the body.  This is so
> +             ;; we can grab this as the initial value when calling
> +             ;; log-view-modify-change-comment
> +             "--pretty=format:commit %H%nAuthor: %an %ae%nDate:   %ad%n%n%B"
> +             ,@(ensure-list vc-git-log-switches)
>               "--"))
>      (goto-char (point-min))
>      (unless (eobp)

I think it's preferable to use a vc-git--extract-comment as in the patch
I just posted.

-- 
Sean Whitton

bug#64055: [WIP Patch] Enable editing commit messages - vc-git-modify-change-comment

[Sean Whitton]

Hello,

On Thu 10 Oct 2024 at 10:39am +08, Sean Whitton wrote:

> So one thing we could do is add a VC backend action which returns just
> the message text that a human might want to edit.
> Probably a backend action called `get-change-comment'.
>
> I think, though, that there might be subtle complexities there.  For
> example, should there be a FILES argument, or just a REVISION argument?
> For Git and Hg it's just REVISION, but we wouldn't want to bake that in.
>
> I think, therefore, that the approach of parsing text out of the
> log-view buffer is more future-proof, even though it's complex.

Hmm.  We could just include all arguments that might be wanted and then
backend functions can ignore them.

I would appreciate opinions comparing adding a new get-change-comment VC
backend function with something like the WIP I posted.

-- 
Sean Whitton

bug#64055: [WIP Patch] Enable editing commit messages - vc-git-modify-change-comment

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@spwhitton.name>
> Date: Thu, 10 Oct 2024 10:45:27 +0800
> 
> - It is probably best to set GIT_SEQUENCE_EDITOR=true so that we're not
>   relying on the shell -- there is no /bin/:, but there is /bin/true.

Does any of this work on MS-Windows?

bug#64055: [WIP Patch] Enable editing commit messages - vc-git-modify-change-comment

[Sean Whitton]

Hello,

On Thu 10 Oct 2024 at 09:12am +03, Eli Zaretskii wrote:

>> From: Sean Whitton <spwhitton@spwhitton.name>
>> Date: Thu, 10 Oct 2024 10:45:27 +0800
>>
>> - It is probably best to set GIT_SEQUENCE_EDITOR=true so that we're not
>>   relying on the shell -- there is no /bin/:, but there is /bin/true.
>
> Does any of this work on MS-Windows?

I think git on Windows always comes with a POSIX shell, right?

-- 
Sean Whitton

bug#64055: [WIP Patch] Enable editing commit messages - vc-git-modify-change-comment

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@spwhitton.name>
> Cc: Morgan.J.Smith@outlook.com,  64055@debbugs.gnu.org,  dgutov@yandex.ru,
>   rpluim@gmail.com
> Date: Thu, 10 Oct 2024 14:23:09 +0800
> 
> Hello,
> 
> On Thu 10 Oct 2024 at 09:12am +03, Eli Zaretskii wrote:
> 
> >> From: Sean Whitton <spwhitton@spwhitton.name>
> >> Date: Thu, 10 Oct 2024 10:45:27 +0800
> >>
> >> - It is probably best to set GIT_SEQUENCE_EDITOR=true so that we're not
> >>   relying on the shell -- there is no /bin/:, but there is /bin/true.
> >
> > Does any of this work on MS-Windows?
> 
> I think git on Windows always comes with a POSIX shell, right?

It does, but that doesn't mean it invokes the editor specified by
GIT_SEQUENCE_EDITOR via that shell.

Someone should actually test this to see if it works to use : or
/bin/true there.

Or maybe there's a safer value which will yield the same effect?
E.g., even using just "true", without the "/bin/" part is a step
towards a more portable command, because the user could have a port of
GNU Coreutils, which includes 'true', installed.

bug#64055: [WIP Patch] Enable editing commit messages - vc-git-modify-change-comment

[Sean Whitton]

Hello,

On Thu 10 Oct 2024 at 10:36am +03, Eli Zaretskii wrote:

>> From: Sean Whitton <spwhitton@spwhitton.name>
>> Cc: Morgan.J.Smith@outlook.com,  64055@debbugs.gnu.org,  dgutov@yandex.ru,
>>   rpluim@gmail.com
>> Date: Thu, 10 Oct 2024 14:23:09 +0800
>>
>> Hello,
>>
>> On Thu 10 Oct 2024 at 09:12am +03, Eli Zaretskii wrote:
>>
>> >> From: Sean Whitton <spwhitton@spwhitton.name>
>> >> Date: Thu, 10 Oct 2024 10:45:27 +0800
>> >>
>> >> - It is probably best to set GIT_SEQUENCE_EDITOR=true so that we're not
>> >>   relying on the shell -- there is no /bin/:, but there is /bin/true.
>> >
>> > Does any of this work on MS-Windows?
>>
>> I think git on Windows always comes with a POSIX shell, right?
>
> It does, but that doesn't mean it invokes the editor specified by
> GIT_SEQUENCE_EDITOR via that shell.
>
> Someone should actually test this to see if it works to use : or
> /bin/true there.
>
> Or maybe there's a safer value which will yield the same effect?
> E.g., even using just "true", without the "/bin/" part is a step
> towards a more portable command, because the user could have a port of
> GNU Coreutils, which includes 'true', installed.

Yeah, I think 'true' is the most portable.

We should test this on MS-Windows later, indeed.

-- 
Sean Whitton

bug#64055: [WIP Patch] Enable editing commit messages - vc-git-modify-change-comment

[Sean Whitton]

Hello,

On Thu 10 Oct 2024 at 10:48am +08, Sean Whitton wrote:

> I would appreciate opinions comparing adding a new get-change-comment
> VC backend function with something like the WIP I posted.

I went with adding a backend function.  That's now on master.  With the
other changes I've made, I believe that all that remains for this bug is
implementing vc-git-modify-change-comment based on our discussion.

-- 
Sean Whitton

bug#64055: [WIP Patch] Enable editing commit messages - vc-git-modify-change-comment

[Dmitry Gutov]

Hi! Sorry for the late reply, I've been driving a lot the last week.

On 10/10/2024 05:39, Sean Whitton wrote:
> In Dmitry's patch he takes the approach of calling the
> expanded-log-entry backend function to get the message to edit.
> This is not a real VC backend function -- in fact it's a log-view
> feature, log-view-expanded-log-entry-function.

It is, but it is also basically a backend method, i.e. in vc-git.el:

     (setq-local log-view-expanded-log-entry-function
                 'vc-git-expanded-log-entry))

So if we determined that its output could be used for editing, perhaps 
after some massaging (e.g. reindenting and keeping only a subset of the 
headers), that could be a minor win -- fewer methods is better in 
general. I haven't tried to code it, so there could be pitfalls.

Also note that we have 'rfc822-goto-eoh' which can be used to skip to 
the end of the headers. 'log-edit-extract-headers' could be used as 
reference for extracting "Summary", even if it doesn't exactly give us 
the desired info now.

> So one thing we could do is add a VC backend action which returns just
> the message text that a human might want to edit.
> Probably a backend action called `get-change-comment'.

The new vc-git-get-change-comment seems good in terms of functionality. 
I was thinking that the headers such as Author, No-Verify and Sign-Off, 
might be good to show as well, but as long as their values are intact 
after the edit, that's optional.

> I think, though, that there might be subtle complexities there.  For
> example, should there be a FILES argument, or just a REVISION argument?
> For Git and Hg it's just REVISION, but we wouldn't want to bake that in.

Makes sense, e.g. if we end up supporting per-file backends later.

bug#64055: [WIP Patch] Enable editing commit messages - vc-git-modify-change-comment

[Sean Whitton]

Hello,

On Fri 18 Oct 2024 at 03:46am +03, Dmitry Gutov wrote:

> Hi! Sorry for the late reply, I've been driving a lot the last week.

Oh, no problem.

> On 10/10/2024 05:39, Sean Whitton wrote:
>> In Dmitry's patch he takes the approach of calling the
>> expanded-log-entry backend function to get the message to edit.
>> This is not a real VC backend function -- in fact it's a log-view
>> feature, log-view-expanded-log-entry-function.
>
> It is, but it is also basically a backend method, i.e. in vc-git.el:
>
>     (setq-local log-view-expanded-log-entry-function
>                 'vc-git-expanded-log-entry))
>
> So if we determined that its output could be used for editing, perhaps after
> some massaging (e.g. reindenting and keeping only a subset of the headers),
> that could be a minor win -- fewer methods is better in general. I haven't
> tried to code it, so there could be pitfalls.

I thought about it and realised that, for the git case, variables like
vc-git-log-switches and vc-git-shortlog-switches can affect the output,
and could make us misparse it.  And ultimately the benefits of avoiding
a new backend method didn't seem to outweigh having parsing code that
could turn out to be fragile.

> Also note that we have 'rfc822-goto-eoh' which can be used to skip to the end
> of the headers. 'log-edit-extract-headers' could be used as reference for
> extracting "Summary", even if it doesn't exactly give us the desired info now.

Those aren't VC-specific, though, they're based on how Log Edit works
for all backends.  So although it's fiddly parsing, it can be done the
same way always, at least.

> The new vc-git-get-change-comment seems good in terms of functionality. I was
> thinking that the headers such as Author, No-Verify and Sign-Off, might be
> good to show as well, but as long as their values are intact after the edit,
> that's optional.

I thought that it would be nice to include Author, in particular, as you
might need to amend the value.  That would require adding additional
arguments to the modify-change-comment action, though, so I left it for
later if someone wants to implement it.

I'm not sure how No-Verify would interact with the --fixup=reword:
commits we are now planning to use.

Sign-Off would be useful for using the new feature to insert a missing
sign-off.

>> I think, though, that there might be subtle complexities there.  For
>> example, should there be a FILES argument, or just a REVISION argument?
>> For Git and Hg it's just REVISION, but we wouldn't want to bake that in.
>
> Makes sense, e.g. if we end up supporting per-file backends later.

Right.

-- 
Sean Whitton

bug#64055: [WIP Patch] Enable editing commit messages - vc-git-modify-change-comment

[Eli Zaretskii]

> Cc: Morgan Smith <Morgan.J.Smith@outlook.com>, Robert Pluim <rpluim@gmail.com>,
>  Dmitry Gutov <dmitry@gutov.dev>
> From: Sean Whitton <spwhitton@spwhitton.name>
> Date: Thu, 17 Oct 2024 21:27:10 +0800
> 
> Hello,
> 
> On Thu 10 Oct 2024 at 10:48am +08, Sean Whitton wrote:
> 
> > I would appreciate opinions comparing adding a new get-change-comment
> > VC backend function with something like the WIP I posted.
> 
> I went with adding a backend function.  That's now on master.  With the
> other changes I've made, I believe that all that remains for this bug is
> implementing vc-git-modify-change-comment based on our discussion.

Thanks.

The commit log message for the changes doesn't mention the bug number;
please always remember to do that in the future.  In this case, the
discussion leading to the change is long and significant, so including
a pointer to it is important for future forensics.

More importantly, why are we installing changes in VC that are
supported by a single backend?  This is against the VC design
principles.  Let's please install soon implementations for other
relevant backends.  I think this should be very simple, but if you
need help for some backends, feel free to ask for it.

Btw, since CVS has an existing command-line option to amend the log
message of a particular commit, I wonder whether the design that goes
through a separate backend function which extracts the comment first
is valid.

bug#64055: [WIP Patch] Enable editing commit messages - vc-git-modify-change-comment

[Sean Whitton]

Hello,

On Fri 18 Oct 2024 at 08:26am +03, Eli Zaretskii wrote:

> The commit log message for the changes doesn't mention the bug number;
> please always remember to do that in the future.  In this case, the
> discussion leading to the change is long and significant, so including
> a pointer to it is important for future forensics.

In this case, my commits didn't add vc-git-modify-change-comment, and
most of the discussion in this bug has been about that.  So, I thought
that it would be best to include the bug number in the later commit
adding vc-git-modify-change-comment, and not in any commits I've made so
far.  Would you prefer that it had been included in all of them?

> More importantly, why are we installing changes in VC that are
> supported by a single backend?  This is against the VC design
> principles.  Let's please install soon implementations for other
> relevant backends.  I think this should be very simple, but if you
> need help for some backends, feel free to ask for it.

Actually, in this case, the functionality already exists for SCCS, RCS,
CVS, SVN and Hg in log-view-extract-comment, and I was just adding
support for Git.

> Btw, since CVS has an existing command-line option to amend the log
> message of a particular commit, I wonder whether the design that goes
> through a separate backend function which extracts the comment first
> is valid.

We still need to extract the comment first so that the user has some
text to edit.

-- 
Sean Whitton

bug#64055: [WIP Patch] Enable editing commit messages - vc-git-modify-change-comment

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@spwhitton.name>
> Cc: 64055@debbugs.gnu.org,  Morgan.J.Smith@outlook.com,  rpluim@gmail.com,
>   dmitry@gutov.dev
> Date: Fri, 18 Oct 2024 14:20:23 +0800
> 
> On Fri 18 Oct 2024 at 08:26am +03, Eli Zaretskii wrote:
> 
> > The commit log message for the changes doesn't mention the bug number;
> > please always remember to do that in the future.  In this case, the
> > discussion leading to the change is long and significant, so including
> > a pointer to it is important for future forensics.
> 
> In this case, my commits didn't add vc-git-modify-change-comment, and
> most of the discussion in this bug has been about that.  So, I thought
> that it would be best to include the bug number in the later commit
> adding vc-git-modify-change-comment, and not in any commits I've made so
> far.  Would you prefer that it had been included in all of them?

Definitely.  It is otherwise very hard to realize that the discussion
of these changes happened in this particular bug.

> > More importantly, why are we installing changes in VC that are
> > supported by a single backend?  This is against the VC design
> > principles.  Let's please install soon implementations for other
> > relevant backends.  I think this should be very simple, but if you
> > need help for some backends, feel free to ask for it.
> 
> Actually, in this case, the functionality already exists for SCCS, RCS,
> CVS, SVN and Hg in log-view-extract-comment, and I was just adding
> support for Git.

That's definitely not what the comments and the implementation convey.
And I then understand even less why the backend action was added for
Git in the first place.

> > Btw, since CVS has an existing command-line option to amend the log
> > message of a particular commit, I wonder whether the design that goes
> > through a separate backend function which extracts the comment first
> > is valid.
> 
> We still need to extract the comment first so that the user has some
> text to edit.

Are we envisioning the user invoking this from anything other than a
log buffer?  Why would someone want to do that?

bug#64055: Implementation of modifying VC change comments for Git

[Sean Whitton]

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

Hello,

This patch is the rest of this feature.  Eli, would you kindly test on
MS-Windows, please?  Just try to edit the message for an unpushed git
commit and add some Unicode, and see if it works correctly.

-- 
Sean Whitton

[-- Attachment #2: 0001-Support-modifying-VC-change-comments-for-Git.patch --]
[-- Type: text/x-diff, Size: 10737 bytes --]

From 674f7b7c636a4e2ebcb4955b564f8c9c36bd3ddd Mon Sep 17 00:00:00 2001
From: Sean Whitton <spwhitton@spwhitton.name>
Date: Fri, 18 Oct 2024 17:19:45 +0800
Subject: [PATCH] Support modifying VC change comments for Git

* lisp/vc/vc-git.el (vc-git-allow-rewriting-history): New option.
(vc-git--assert-allowed-rewrite, vc-git-modify-change-comment):
New functions (bug#64055).

(vc-git--current-branch): Factor out of vc-git-dir--branch-headers.
(vc-git--log-edit-extract-headers): Factor out of vc-git-checkin.

* etc/NEWS: Announce the new support and option.
---
 etc/NEWS          |  15 +++++
 lisp/vc/vc-git.el | 147 +++++++++++++++++++++++++++++++++++++---------
 2 files changed, 134 insertions(+), 28 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 10d86173235..d2e72bc537a 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -568,6 +568,21 @@ a desktop notification when the song changes, using
 customized using the new user options 'mpc-notifications-title' and
 'mpc-notifications-body'.
 
+** VC
+
+---
+*** Using 'e' from Log View mode to modify change comments now works for Git.
+
+---
+*** New user option 'vc-git-allow-rewriting-history'.
+Many Git commands can change your copy of published change history
+without warning.  If VC commands detect that this could happen, they
+will stop.  You can customize this variable to permit rewriting history
+even though Emacs thinks it is dangerous.
+
+So far, this applies only to the 'log-view-modify-change-comment'
+command.
+
 \f
 * New Modes and Packages in Emacs 31.1
 
diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index f77bf0cc5ff..0680d8e7353 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -252,6 +252,27 @@ vc-git-revision-complete-only-branches
   :type 'boolean
   :version "28.1")
 
+;; The default is nil because only a VC user who also possesses a lot of
+;; Git-specific knowledge can know when it is okay to rewrite history,
+;; and we can't convey to a relatively Git-naïve user the potential
+;; risks in only the space of a minibuffer y/n prompt.
+(defcustom vc-git-allow-rewriting-history nil
+  "When non-nil, permit Git operations that may rewrite published history.
+
+Many Git commands can change your copy of published change history
+without warning.  If this occurs, you won't be able to pull and push in
+the ordinary way until you take special action.  See \"Recovering from
+Upstream Rebase\" in the Man page git-rebase(1).
+
+Normally, Emacs refuses to run Git commands that it thinks will rewrite
+published history.  If you customize this variable to a non-nil value,
+Emacs will instead prompt you to confirm that you really want to perform
+the rewrite.  A value of `no-ask' means to proceed with no prompting."
+  :type '(choice (const :tag "Don't allow" nil)
+                 (const :tag "Prompt to allow" t)
+                 (const :tag "Allow without prompting" no-ask))
+  :version "31.1")
+
 ;; History of Git commands.
 (defvar vc-git-history nil)
 
@@ -728,11 +749,13 @@ vc-git-dir-status-files
                                  :files files
                                  :update-function update-function)))
 
+(defun vc-git--current-branch ()
+  (vc-git--out-match '("symbolic-ref" "HEAD")
+                     "^\\(refs/heads/\\)?\\(.+\\)$" 2))
+
 (defun vc-git-dir--branch-headers ()
   "Return headers for branch-related information."
-  (let ((branch (vc-git--out-match
-                 '("symbolic-ref" "HEAD")
-                 "^\\(refs/heads/\\)?\\(.+\\)$" 2))
+  (let ((branch (vc-git--current-branch))
         tracking remote-url)
     (if branch
         (when-let ((branch-merge
@@ -1082,6 +1105,17 @@ vc-git-checkin-patch
 
 (autoload 'vc-switches "vc")
 
+(defun vc-git--log-edit-extract-headers (comment)
+  (cl-flet ((boolean-arg-fn (argument)
+              (lambda (v) (and (equal v "yes") (list argument)))))
+    (log-edit-extract-headers
+     `(("Author" . "--author")
+       ("Date" . "--date")
+       ("Amend" . ,(boolean-arg-fn "--amend"))
+       ("No-Verify" . ,(boolean-arg-fn "--no-verify"))
+       ("Sign-Off" . ,(boolean-arg-fn "--signoff")))
+     comment)))
+
 (defun vc-git-checkin (files comment &optional _rev)
   (let* ((file1 (or (car files) default-directory))
          (root (vc-git-root file1))
@@ -1180,31 +1214,23 @@ vc-git-checkin
               (vc-git-command nil 0 patch-file "apply" "--cached")
             (delete-file patch-file))))
       (when to-stash (vc-git--stash-staged-changes files)))
-    (cl-flet ((boolean-arg-fn
-               (argument)
-               (lambda (value) (when (equal value "yes") (list argument)))))
-      ;; When operating on the whole tree, better pass "-a" than ".", since "."
-      ;; fails when we're committing a merge.
-      (apply #'vc-git-command nil 0 (if (and only (not vc-git-patch-string)) files)
-             (nconc (if msg-file (list "commit" "-F"
-                                       (file-local-name msg-file))
-                      (list "commit" "-m"))
-                    (let ((args
-                           (log-edit-extract-headers
-                            `(("Author" . "--author")
-                              ("Date" . "--date")
-                              ("Amend" . ,(boolean-arg-fn "--amend"))
-                              ("No-Verify" . ,(boolean-arg-fn "--no-verify"))
-                              ("Sign-Off" . ,(boolean-arg-fn "--signoff")))
-                            comment)))
-                      (when msg-file
-                        (let ((coding-system-for-write
-                               (or pcsw vc-git-commits-coding-system)))
-                          (write-region (car args) nil msg-file))
-                        (setq args (cdr args)))
-                      args)
-                    (unless vc-git-patch-string
-                      (if only (list "--only" "--") '("-a"))))))
+    ;; When operating on the whole tree, better pass "-a" than ".",
+    ;; since "."  fails when we're committing a merge.
+    (apply #'vc-git-command nil 0
+           (if (and only (not vc-git-patch-string)) files)
+           (nconc (if msg-file (list "commit" "-F"
+                                     (file-local-name msg-file))
+                    (list "commit" "-m"))
+                  (let ((args
+                         (vc-git--log-edit-extract-headers comment)))
+                    (when msg-file
+                      (let ((coding-system-for-write
+                             (or pcsw vc-git-commits-coding-system)))
+                        (write-region (car args) nil msg-file))
+                      (setq args (cdr args)))
+                    args)
+                  (unless vc-git-patch-string
+                    (if only (list "--only" "--") '("-a")))))
     (if (and msg-file (file-exists-p msg-file)) (delete-file msg-file))
     (when to-stash
       (let ((cached (make-nearby-temp-file "git-cached")))
@@ -1960,6 +1986,71 @@ vc-git-get-change-comment
     (vc-git-command standard-output 1 nil
                     "log" "--max-count=1" "--pretty=format:%B" rev)))
 
+(defun vc-git--assert-allowed-rewrite (rev)
+  (when (and (not (eq vc-git-allow-rewriting-history 'no-ask))
+             ;; Check there is an upstream.
+             (with-temp-buffer
+               (vc-git--out-ok "config" "--get"
+                               (format "branch.%s.merge"
+                                       (vc-git--current-branch)))))
+    (let ((outgoing (split-string
+                     (with-output-to-string
+                       (vc-git-command standard-output 0 nil "log"
+                                       "--pretty=format:%H"
+                                       "@{upstream}..HEAD")))))
+      (unless (or (cl-member rev outgoing :test #'string-prefix-p)
+                  (and vc-git-allow-rewriting-history
+                       (yes-or-no-p
+                        (format
+"Commit %s looks to be published; are you sure you want to rewrite history?"
+                         rev))))
+        (user-error "Will not rewrite likely-public Git history")))))
+
+(defun vc-git-modify-change-comment (files rev comment)
+  (vc-git--assert-allowed-rewrite rev)
+  (let* ((args (vc-git--log-edit-extract-headers comment))
+         (message (format "amend! %s\n\n%s" rev (pop args)))
+         (msg-file
+          ;; On MS-Windows, pass the message through a file, to work
+          ;; around how command line arguments must be in the system
+          ;; codepage, and therefore might not support non-ASCII.
+          ;;
+          ;; As our other arguments are static, we need not be concerned
+          ;; about the encoding of command line arguments in general.
+          ;; See `vc-git-checkin' for the more complex case.
+          (and (eq system-type 'windows-nt)
+               (let ((default-directory
+                      (or (file-name-directory (or (car files)
+                                                   default-directory))
+                          default-directory)))
+                 (make-nearby-temp-file "git-msg")))))
+    (unwind-protect
+        (progn
+          (when (cl-intersection '("--author" "--date") args
+                                 :test #'string=)
+            ;; 'git rebase --autosquash' cannot alter authorship.
+            ;; See the description of --fixup in git-commit(1).
+            (error
+"Author: and Date: not supported when modifying existing commits"))
+          (when msg-file
+            (let ((coding-system-for-write
+                   (or coding-system-for-write
+                       vc-git-commits-coding-system)))
+              (write-region message nil msg-file)))
+          ;; Regardless of the state of the index and working tree, this
+          ;; will always create an empty commit, thanks to --only.
+          (apply #'vc-git-command nil 0 nil
+                 "commit" "--only" "--allow-empty"
+                 (nconc (if msg-file
+                            (list "-F" (file-local-name msg-file))
+                          (list "-m" message))
+                        args)))
+      (when (and msg-file (file-exists-p msg-file))
+        (delete-file msg-file))))
+  (with-environment-variables (("GIT_SEQUENCE_EDITOR" "true"))
+    (vc-git-command nil 0 nil "rebase" "--autostash" "--autosquash" "-i"
+                    (format "%s~1" rev))))
+
 (defvar vc-git-extra-menu-map
   (let ((map (make-sparse-keymap)))
     (define-key map [git-grep]
-- 
2.45.2

bug#64055: [WIP Patch] Enable editing commit messages - vc-git-modify-change-comment

[Sean Whitton]

Hello,

On Fri 18 Oct 2024 at 12:14pm +03, Eli Zaretskii wrote:

> Definitely.  It is otherwise very hard to realize that the discussion
> of these changes happened in this particular bug.

Right, okay.

> That's definitely not what the comments and the implementation convey.
> And I then understand even less why the backend action was added for
> Git in the first place.

Hmm, I see what you mean.  I should have included more detail in the
commit message and/or comments.

>> > Btw, since CVS has an existing command-line option to amend the log
>> > message of a particular commit, I wonder whether the design that goes
>> > through a separate backend function which extracts the comment first
>> > is valid.
>>
>> We still need to extract the comment first so that the user has some
>> text to edit.
>
> Are we envisioning the user invoking this from anything other than a
> log buffer?  Why would someone want to do that?

I can imagine a few cases where it might be useful, but I don't think
that's the main point.

The main point is that parsing text out of the log view buffer is much
more error-prone.  For example, with a non-distributed VCS like CVS,
what if someone else has edited the comment in the meantime?  It makes
sense to fetch the latest known version instead of relying on buffer
text.

So, I hope that we will eventually completely replace
log-view-extract-comment.

-- 
Sean Whitton