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