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.