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

[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: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 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

[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

[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: [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

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 17:30:16 +0800
> 
> 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.

Now I'm confused: didn't you just tell me that CVS already had this
implemented, without ever needing the new backend method?  How do we
solve this problem in the CVS case without risking the errors?  What
you say above seems to tell that we do need an implementation of this
method for CVS as well, no?  Or what am I missing?

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

So you do agree with me that we should have this implemented for other
backends, and the sooner the better?

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

[Sean Whitton]

Hello,

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

>> 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 17:30:16 +0800
>>
>> 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.
>
> Now I'm confused: didn't you just tell me that CVS already had this
> implemented, without ever needing the new backend method?  How do we
> solve this problem in the CVS case without risking the errors?  What
> you say above seems to tell that we do need an implementation of this
> method for CVS as well, no?  Or what am I missing?
>
>> So, I hope that we will eventually completely replace
>> log-view-extract-comment.
>
> So you do agree with me that we should have this implemented for other
> backends, and the sooner the better?

The VC design principle here is that commands should be implemented for
more than one version control system, in part because that way we ensure
they really are general version control user operations, and not
something parochial to the way a particular VCS sees the world.

In the context of this bug, the relevant command for this design
principle is log-view-modify-change-comment.  Previously, that command
worked only for src, rcs, sccs, cvs and svn, and partly for Hg.  Soon it
will also work for git.  So, the design principle is respected.

On the other hand, the design principle doesn't apply directly to the
new get-change-comment backend action, because src, rcs, sccs, cvs and
svn do not *require* an implementation of get-change-comment in order
for the log-view-modify-change-comment command to work, so far as I
understand it -- otherwise the feature would not have been implemented
the way it is, or we'd have bug reports.

Git, I think, does require an implementation of get-change-comment, or
at least, I concluded that it's the most natural way to implement
log-view-modify-change-comment for git.

In a similar vein, non-distributed VCS require a receive-file backend
action, but it would not make sense to write a function
vc-git-receive-file.  The VC design principle does not apply directly to
backend actions.  For many backend actions, including
get-change-comment, only certain kinds of VCS will require an
implementation, in order to support the high-level VC user commands.

Nevertheless, although src, rcs, sccs, cvs and svn do not *require*
get-change-comment, I think it would be preferable if they did have an
implementation, instead of using log-view-extract-comment, for the
reasons given previously.  But we are talking about changing working
code.  We should not rush to replace log-view-extract-comment.  We can
do so incrementally and gradually, and it's low priority.

-- 
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: Sun, 20 Oct 2024 08:56:12 +0800
> 
> On the other hand, the design principle doesn't apply directly to the
> new get-change-comment backend action, because src, rcs, sccs, cvs and
> svn do not *require* an implementation of get-change-comment in order
> for the log-view-modify-change-comment command to work, so far as I
> understand it -- otherwise the feature would not have been implemented
> the way it is, or we'd have bug reports.
> 
> Git, I think, does require an implementation of get-change-comment, or
> at least, I concluded that it's the most natural way to implement
> log-view-modify-change-comment for git.

It is this difference between Git and the rest that I don't yet see,
and you didn't explain it, just stated its existence.  Why is the
method necessary for Git, but not for other VCSes? they all produce
similar displays of log messages, complete with Author, Date, etc.,
and so the difficulties of extracting just the change log message
should affect all of them, no?

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

[Sean Whitton]

Hello,

On Sun 20 Oct 2024 at 07:58am +03, Eli Zaretskii wrote:

> It is this difference between Git and the rest that I don't yet see,
> and you didn't explain it, just stated its existence.  Why is the
> method necessary for Git, but not for other VCSes? they all produce
> similar displays of log messages, complete with Author, Date, etc.,
> and so the difficulties of extracting just the change log message
> should affect all of them, no?

Ah, right.

The difference is that with Git you can use variables like
vc-git-log-switches and vc-git-shortlog-switches to radically change the
log output.  We can't realistically deal with all the possibilities from
Lisp.

Whereas, so far as I understand, such customisation of the output is not
available with the older VCS.

-- 
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: Sun, 20 Oct 2024 13:29:59 +0800
> 
> Hello,
> 
> On Sun 20 Oct 2024 at 07:58am +03, Eli Zaretskii wrote:
> 
> > It is this difference between Git and the rest that I don't yet see,
> > and you didn't explain it, just stated its existence.  Why is the
> > method necessary for Git, but not for other VCSes? they all produce
> > similar displays of log messages, complete with Author, Date, etc.,
> > and so the difficulties of extracting just the change log message
> > should affect all of them, no?
> 
> Ah, right.
> 
> The difference is that with Git you can use variables like
> vc-git-log-switches and vc-git-shortlog-switches to radically change the
> log output.  We can't realistically deal with all the possibilities from
> Lisp.
> 
> Whereas, so far as I understand, such customisation of the output is not
> available with the older VCS.

I see vc-hg-log-switches and vc-bzr-log-switches, so I don't think I
understand why you say Git is special here.  What did I miss?

In any case, the reason for having a backend method should be
explained in the comments.

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

[Sean Whitton]

Hello,

On Sun 20 Oct 2024 at 09:09am +03, Eli Zaretskii wrote:

> I see vc-hg-log-switches and vc-bzr-log-switches, so I don't think I
> understand why you say Git is special here.  What did I miss?

By "older VCS" I did not mean to include Hg and bzr, which are special
in the same way as Git.  And indeed, neither vc-bzr or vc-hg have
log-view-modify-change-comment support.  It would be good to add it.

> In any case, the reason for having a backend method should be
> explained in the comments.

I've added a FIXME pointing to this bug.  Is there somewhere else you
have in mind?

-- 
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: Sun, 20 Oct 2024 15:18:38 +0800
> 
> Hello,
> 
> On Sun 20 Oct 2024 at 09:09am +03, Eli Zaretskii wrote:
> 
> > I see vc-hg-log-switches and vc-bzr-log-switches, so I don't think I
> > understand why you say Git is special here.  What did I miss?
> 
> By "older VCS" I did not mean to include Hg and bzr, which are special
> in the same way as Git.  And indeed, neither vc-bzr or vc-hg have
> log-view-modify-change-comment support.  It would be good to add it.

I consider this change incomplete until we add that support at least
for Hg.

> > In any case, the reason for having a backend method should be
> > explained in the comments.
> 
> I've added a FIXME pointing to this bug.  Is there somewhere else you
> have in mind?

I don't see it; did you forget to push?  In any case, I prefer an
explicit explanation, including the reference to the vc-*-log-switches
variables, not just a reference to a bug number.

Thanks.

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

[Sean Whitton]

retitle 64055 31.0.50; log-view-modify-change-comment support for Git and Hg
thanks

On Sun 20 Oct 2024 at 11:20am +03, Eli Zaretskii wrote:

> I consider this change incomplete until we add that support at least
> for Hg.

Fair enough.  I'm retitling the bug.  And after I install the change to
vc-git.el, I won't close this bug.

>> > In any case, the reason for having a backend method should be
>> > explained in the comments.
>>
>> I've added a FIXME pointing to this bug.  Is there somewhere else you
>> have in mind?
>
> I don't see it; did you forget to push?

I did push it.

> In any case, I prefer an explicit explanation, including the reference
> to the vc-*-log-switches variables, not just a reference to a bug
> number.

Added a second FIXME regarding modern VCS.

-- 
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, control@debbugs.gnu.org
> Date: Sun, 20 Oct 2024 16:42:54 +0800
> 
> > In any case, I prefer an explicit explanation, including the reference
> > to the vc-*-log-switches variables, not just a reference to a bug
> > number.
> 
> Added a second FIXME regarding modern VCS.

Thanks, LGTM.

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

[Dmitry Gutov]

On 18/10/2024 07:50, Sean Whitton wrote:
>> 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.

IIUC vc-git-expanded-log-entry is only affected by the former var. We 
could bind it to nil (that could even be done generically, by looking 
for all vars called vc-BACKEND-log-switches).

But I agree fragility could be a problem. It could also be a win to have 
one of the methods to be implemented in terms of the other.

>> 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 think you're parsing out all the headers from the COMMENT argument in 
the posted patch, so this paragraph seems to be moot. Like you say in 
the patch though, it seems changing Author is not supported.

To do that, apparently we'd either need to rebase with a pause (with 
'git amend' in the middle), or use 'git rebase --exec' like in 
https://stackoverflow.com/a/79068024/615245. The latter seems like it 
would apply to all commits in the range, so it might require the same 
dance as 'amend'.

(I can comment on the patch itself tomorrow.)

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

[Sean Whitton]

Hello,

On Sun 20 Oct 2024 at 01:16am +01, Dmitry Gutov wrote:

>> 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 think you're parsing out all the headers from the COMMENT argument in the
> posted patch, so this paragraph seems to be moot.

Yeah, I realised that because we are invoking git-commit(1), it's easy
to support most of them.

> Like you say in the patch though, it seems changing Author is not
> supported.
>
> To do that, apparently we'd either need to rebase with a pause (with 'git
> amend' in the middle), or use 'git rebase --exec' like in
> https://stackoverflow.com/a/79068024/615245. The latter seems like it would
> apply to all commits in the range, so it might require the same dance as
> 'amend'.

Yeah.  We can consider it as a possible future enhancement.
I'm also concerned about it making the command slower -- it's already
not that fast.

> (I can comment on the patch itself tomorrow.)

Cool, thanks.

-- 
Sean Whitton

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

[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

[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

[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: 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: Implementation of modifying VC change comments for Git

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@spwhitton.name>
> Cc: Dmitry Gutov <dgutov@yandex.ru>, Morgan Smith <Morgan.J.Smith@outlook.com>
> Date: Fri, 18 Oct 2024 17:26:28 +0800
> 
> 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.

I'm supposed to type 'e' in the log buffer, edit the comment, then
type "C-c C-c"?  Or should I do something else?

When I do the above, I get an error message:

  vc-do-command: Failed (status 128): git --no-pager commit --only --allow-empty -F c:/Users/EliZ/AppData/Local/Temp/git-msgWaNflu

and the *vc* buffer says:

  fatal: No paths with --include/--only does not make sense.

It's possible my Git is very old, but shouldn't this command work with
old versions as well?

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

[Sean Whitton]

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

Hello,

On Sat 19 Oct 2024 at 01:28pm +03, Eli Zaretskii wrote:

>> From: Sean Whitton <spwhitton@spwhitton.name>
>> Cc: Dmitry Gutov <dgutov@yandex.ru>, Morgan Smith <Morgan.J.Smith@outlook.com>
>> Date: Fri, 18 Oct 2024 17:26:28 +0800
>>
>> 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.
>
> I'm supposed to type 'e' in the log buffer, edit the comment, then
> type "C-c C-c"?

Yeah, that's right.

> When I do the above, I get an error message:
>
>   vc-do-command: Failed (status 128): git --no-pager commit --only
> --allow-empty -F c:/Users/EliZ/AppData/Local/Temp/git-msgWaNflu
>
> and the *vc* buffer says:
>
>   fatal: No paths with --include/--only does not make sense.
>
> It's possible my Git is very old, but shouldn't this command work with
> old versions as well?

I've looked into it and what's required is Git 2.11.1 from early 2017.

I think we can support older by stashing and unstashing.  So please try
the attached patch, which does that.

-- 
Sean Whitton

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

From 3ab65121d229e64915c7b4c89fe53f80cb34ebff Mon Sep 17 00:00:00 2001
From: Sean Whitton <spwhitton@spwhitton.name>
Date: Fri, 18 Oct 2024 17:19:45 +0800
Subject: [PATCH v2] 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 | 167 ++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 154 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..59d5386bc72 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,91 @@ 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 (delete "--amend"
+                       (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"))))
+         (nothing-staged
+          (zerop
+           (vc-git-command nil t nil "diff" "--cached" "--quiet"))))
+    ;; We want to do just
+    ;;
+    ;;     % git commit --only --allow-empty -m...
+    ;;     % git rebase --autostash --autosquash -i REV~1
+    ;;
+    ;; because the first command is guaranteed to create an empty commit
+    ;; regardless of the state of the index and working tree.  However,
+    ;; that requires git.git commit 319d835, released in Git 2.11.1.
+    ;; In order to support older Git we do this longer, slower sequence:
+    ;;
+    ;;     % git stash push
+    ;;     % git commit --allow-empty -m...
+    ;;     % git rebase --autosquash -i REV~1
+    ;;     % git stash pop
+    (unless nothing-staged
+      (vc-git-command nil 0 nil "stash" "push"))
+    (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)))
+          (apply #'vc-git-command nil 0 nil
+                 "commit" "--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" "--autosquash" "-i"
+                      (format "%s~1" rev)))
+    (unless nothing-staged
+      (vc-git-command nil 0 nil "stash" "pop" "--index"))))
+
 (defvar vc-git-extra-menu-map
   (let ((map (make-sparse-keymap)))
     (define-key map [git-grep]
-- 
2.45.2

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

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@spwhitton.name>
> Cc: 64055@debbugs.gnu.org,  dgutov@yandex.ru,  Morgan.J.Smith@outlook.com
> Date: Sun, 20 Oct 2024 13:19:09 +0800
> 
> > When I do the above, I get an error message:
> >
> >   vc-do-command: Failed (status 128): git --no-pager commit --only
> > --allow-empty -F c:/Users/EliZ/AppData/Local/Temp/git-msgWaNflu
> >
> > and the *vc* buffer says:
> >
> >   fatal: No paths with --include/--only does not make sense.
> >
> > It's possible my Git is very old, but shouldn't this command work with
> > old versions as well?
> 
> I've looked into it and what's required is Git 2.11.1 from early 2017.

So someone will have to test the patch on Windows with a new en ough
Git version.

> I think we can support older by stashing and unstashing.  So please try
> the attached patch, which does that.

This seems to work without any errors, and the *vc* buffer says

  Successfully rebased and updated refs/heads/master.

But the log message is not updated, neither in the *vc-change-log*
buffer nor if I manually invoke "git log" from the shell prompt.  It
sounds like the commit message was not amended.  "git stash list" also
shows no relevant stashes.

Let me know how can I help you debug this.

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

[Sean Whitton]

Hello,

On Sun 20 Oct 2024 at 11:32am +03, Eli Zaretskii wrote:

> So someone will have to test the patch on Windows with a new en ough
> Git version.

I don't think so -- the code to support Windows is independent of the
code to support older Git.  So, my tests here on GNU/Linux with newer
Git, together with yours on Windows with older Git, are sufficient.

>> I think we can support older by stashing and unstashing.  So please try
>> the attached patch, which does that.
>
> This seems to work without any errors, and the *vc* buffer says
>
>   Successfully rebased and updated refs/heads/master.
>
> But the log message is not updated, neither in the *vc-change-log*
> buffer nor if I manually invoke "git log" from the shell prompt.  It
> sounds like the commit message was not amended.  "git stash list" also
> shows no relevant stashes.
>
> Let me know how can I help you debug this.

Thank you for testing it.

Let's try commenting out everything except the 'git commit', as done in
the following diff on top of my patch.  Please test this without
anything staged.  Does it create a new commit at the tip of your branch?

The contents of the commit should be a special first line, followed by
your amended commit message.

If not: I've also commented out the deletion of the temporary file, and
sent its name to *Messages*.  If you look in that temporary file, do you
find your amended commit message?

--8<---------------cut here---------------start------------->8---
diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 59d5386bc72..d2ada63f71e 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -2042,8 +2042,8 @@ vc-git-modify-change-comment
     ;;     % git commit --allow-empty -m...
     ;;     % git rebase --autosquash -i REV~1
     ;;     % git stash pop
-    (unless nothing-staged
-      (vc-git-command nil 0 nil "stash" "push"))
+    ;; (unless nothing-staged
+    ;;   (vc-git-command nil 0 nil "stash" "push"))
     (unwind-protect
         (progn
           (when (cl-intersection '("--author" "--date") args
@@ -2064,12 +2064,15 @@ vc-git-modify-change-comment
                           (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" "--autosquash" "-i"
-                      (format "%s~1" rev)))
-    (unless nothing-staged
-      (vc-git-command nil 0 nil "stash" "pop" "--index"))))
+        ;; (delete-file msg-file)
+        ))
+    ;; (with-environment-variables (("GIT_SEQUENCE_EDITOR" "true"))
+    ;;   (vc-git-command nil 0 nil "rebase" "--autosquash" "-i"
+    ;;                   (format "%s~1" rev)))
+    ;; (unless nothing-staged
+    ;;   (vc-git-command nil 0 nil "stash" "pop" "--index"))
+    (message "temporary file is: %s" msg-file)
+    ))

 (defvar vc-git-extra-menu-map
   (let ((map (make-sparse-keymap)))
--8<---------------cut here---------------end--------------->8---

-- 
Sean Whitton

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

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@spwhitton.name>
> Cc: 64055@debbugs.gnu.org,  dgutov@yandex.ru,  Morgan.J.Smith@outlook.com
> Date: Sun, 20 Oct 2024 16:59:32 +0800
> 
> Let's try commenting out everything except the 'git commit', as done in
> the following diff on top of my patch.  Please test this without
> anything staged.  Does it create a new commit at the tip of your branch?

Yes.  But only "git log" shows that additional commit, "C-x v l"
doesn't update the log.

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

[Sean Whitton]

Hello,

On Sun 20 Oct 2024 at 12:19pm +03, Eli Zaretskii wrote:

>> From: Sean Whitton <spwhitton@spwhitton.name>
>> Cc: 64055@debbugs.gnu.org,  dgutov@yandex.ru,  Morgan.J.Smith@outlook.com
>> Date: Sun, 20 Oct 2024 16:59:32 +0800
>>
>> Let's try commenting out everything except the 'git commit', as done in
>> the following diff on top of my patch.  Please test this without
>> anything staged.  Does it create a new commit at the tip of your branch?
>
> Yes.  But only "git log" shows that additional commit, "C-x v l"
> doesn't update the log.

If you press 'g' in the 'C-x v l' buffer, does it appear then?

-- 
Sean Whitton

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

[Sean Whitton]

Hello,

On Sun 20 Oct 2024 at 05:25pm +08, Sean Whitton wrote:

> Hello,
>
> On Sun 20 Oct 2024 at 12:19pm +03, Eli Zaretskii wrote:
>
>>> From: Sean Whitton <spwhitton@spwhitton.name>
>>> Cc: 64055@debbugs.gnu.org,  dgutov@yandex.ru,  Morgan.J.Smith@outlook.com
>>> Date: Sun, 20 Oct 2024 16:59:32 +0800
>>>
>>> Let's try commenting out everything except the 'git commit', as done in
>>> the following diff on top of my patch.  Please test this without
>>> anything staged.  Does it create a new commit at the tip of your branch?
>>
>> Yes.  But only "git log" shows that additional commit, "C-x v l"
>> doesn't update the log.
>
> If you press 'g' in the 'C-x v l' buffer, does it appear then?

Oh, wait, it wouldn't.  Does it appear in 'C-x v L'?

-- 
Sean Whitton

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

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@spwhitton.name>
> Cc: 64055@debbugs.gnu.org,  dgutov@yandex.ru,  Morgan.J.Smith@outlook.com
> Date: Sun, 20 Oct 2024 17:46:53 +0800
> 
> >> Yes.  But only "git log" shows that additional commit, "C-x v l"
> >> doesn't update the log.
> >
> > If you press 'g' in the 'C-x v l' buffer, does it appear then?
> 
> Oh, wait, it wouldn't.  Does it appear in 'C-x v L'?

Yes.

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

[Sean Whitton]

Hello,

On Sun 20 Oct 2024 at 02:24pm +03, Eli Zaretskii wrote:

>> From: Sean Whitton <spwhitton@spwhitton.name>
>> Cc: 64055@debbugs.gnu.org,  dgutov@yandex.ru,  Morgan.J.Smith@outlook.com
>> Date: Sun, 20 Oct 2024 17:46:53 +0800
>>
>> >> Yes.  But only "git log" shows that additional commit, "C-x v l"
>> >> doesn't update the log.
>> >
>> > If you press 'g' in the 'C-x v l' buffer, does it appear then?
>>
>> Oh, wait, it wouldn't.  Does it appear in 'C-x v L'?
>
> Yes.

Okay.  Does the message look like this:

    amend! <sha1 of the commit whose message you edited>

    <your edited message>

If so, then we know that the invocation of 'git commit' is working, and
the problem is somewhere else -- probably the invocation of 'git rebase'.

-- 
Sean Whitton

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

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@spwhitton.name>
> Cc: 64055@debbugs.gnu.org,  dgutov@yandex.ru,  Morgan.J.Smith@outlook.com
> Date: Sun, 20 Oct 2024 21:11:00 +0800
> 
> >> Oh, wait, it wouldn't.  Does it appear in 'C-x v L'?
> >
> > Yes.
> 
> Okay.  Does the message look like this:
> 
>     amend! <sha1 of the commit whose message you edited>
> 
>     <your edited message>

Yes.

> If so, then we know that the invocation of 'git commit' is working, and
> the problem is somewhere else -- probably the invocation of 'git rebase'.

With you, so far.

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

[Dmitry Gutov]

On 20/10/2024 16:52, Eli Zaretskii wrote:
>> From: Sean Whitton<spwhitton@spwhitton.name>
>> Cc:64055@debbugs.gnu.org,dgutov@yandex.ru,Morgan.J.Smith@outlook.com
>> Date: Sun, 20 Oct 2024 21:11:00 +0800
>>
>>>> Oh, wait, it wouldn't.  Does it appear in 'C-x v L'?
>>> Yes.
>> Okay.  Does the message look like this:
>>
>>      amend! <sha1 of the commit whose message you edited>
>>
>>      <your edited message>
> Yes.
> 
>> If so, then we know that the invocation of 'git commit' is working, and
>> the problem is somewhere else -- probably the invocation of 'git rebase'.
> With you, so far.

Eli, it seems you've installed the patch from this bug on master by 
accident, in commit 75fa0cc1ae2.

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

[Sean Whitton]

Hello,

On Mon 21 Oct 2024 at 01:01am +01, Dmitry Gutov wrote:

> Eli, it seems you've installed the patch from this bug on master by accident,
> in commit 75fa0cc1ae2.

The debugging version, it would seem.  I've gone ahead and reverted it.

-- 
Sean Whitton

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

[Eli Zaretskii]

> Date: Mon, 21 Oct 2024 01:01:25 +0100
> Cc: Morgan.J.Smith@outlook.com, 64055@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> 
> >>      amend! <sha1 of the commit whose message you edited>
> >>
> >>      <your edited message>
> > Yes.
> > 
> >> If so, then we know that the invocation of 'git commit' is working, and
> >> the problem is somewhere else -- probably the invocation of 'git rebase'.
> > With you, so far.
> 
> Eli, it seems you've installed the patch from this bug on master by 
> accident, in commit 75fa0cc1ae2.

Oops, thanks, fixed.

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

[Sean Whitton]

Hello,

On Sun 20 Oct 2024 at 04:52pm +03, Eli Zaretskii wrote:

>> From: Sean Whitton <spwhitton@spwhitton.name>
>> Cc: 64055@debbugs.gnu.org,  dgutov@yandex.ru,  Morgan.J.Smith@outlook.com
>> Date: Sun, 20 Oct 2024 21:11:00 +0800
>>
>> >> Oh, wait, it wouldn't.  Does it appear in 'C-x v L'?
>> >
>> > Yes.
>>
>> Okay.  Does the message look like this:
>>
>>     amend! <sha1 of the commit whose message you edited>
>>
>>     <your edited message>
>
> Yes.
>
>> If so, then we know that the invocation of 'git commit' is working, and
>> the problem is somewhere else -- probably the invocation of 'git rebase'.
>
> With you, so far.

I think the next thing to do is try the rebase from the command line.
So, please use the debugging version of my patch to create the special
"amend!" commit again, or use the one from yesterday if you still have
it, or you could manually create it by passing --allow-empty to 'git
commit'.

Then, at the command line, use

    git rebase --autosquash -i REV~1

where REV is the sha1 of the commit whose message you intend to edit.

This should open an editor with a rebase plan in which you see

    - the commit whose message you wanted to change
    - followed by the special amend! commit
    - followed by any other commits following the one whose message you wanted to change

i.e. the amend! commit has been pulled down in history to just after the
commit whose message you want to cahnge.

Just save and exit without making any changes to the plan.

After that, the whole process should be complete: you should have the
same commits as previously except the message should have been updated,
and sha1 hashes will have changed.

Let me know if this all works.

-- 
Sean Whitton

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

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@spwhitton.name>
> Cc: 64055@debbugs.gnu.org,  dgutov@yandex.ru,  Morgan.J.Smith@outlook.com
> Date: Mon, 21 Oct 2024 09:52:02 +0800
> 
> I think the next thing to do is try the rebase from the command line.
> So, please use the debugging version of my patch to create the special
> "amend!" commit again, or use the one from yesterday if you still have
> it, or you could manually create it by passing --allow-empty to 'git
> commit'.
> 
> Then, at the command line, use
> 
>     git rebase --autosquash -i REV~1
> 
> where REV is the sha1 of the commit whose message you intend to edit.
> 
> This should open an editor with a rebase plan in which you see
> 
>     - the commit whose message you wanted to change
>     - followed by the special amend! commit
>     - followed by any other commits following the one whose message you wanted to change
> 
> i.e. the amend! commit has been pulled down in history to just after the
> commit whose message you want to cahnge.
> 
> Just save and exit without making any changes to the plan.
> 
> After that, the whole process should be complete: you should have the
> same commits as previously except the message should have been updated,
> and sha1 hashes will have changed.
> 
> Let me know if this all works.

I'm not sure I understood you correctly, but if I perform all these
step, I'm back at the original tip, with the original log message, not
the edited one.

But let me describe what I did and saw, to make sure I did it
correctly.

>     git rebase --autosquash -i REV~1
> 
> where REV is the sha1 of the commit whose message you intend to edit.

At this point "git log" shows a revision with my amended log message
(let's call this AMENDED-REV), followed by the revision with the
original log message (let's call it ORIG-REV).  So my Git command
looks like this:

  git rebase --autosquash -i ORIG-REV~1

After this step, I see this in the editor (where I indented the text
by 2 columns):

  pick bf73d7e Foobar with some Unicode אבגד ą ě č
  # pick bc3c567b2b136d040fd13373b6594c1ec026fec6 Foobar with some Unicode אבגד ą ě č

  # Rebase 6c6ea73..bc3c567 onto 6c6ea73 (1 command)
  #
  # Commands:
  # p, pick = use commit
  # r, reword = use commit, but edit the commit message
  # e, edit = use commit, but stop for amending
  # s, squash = use commit, but meld into previous commit
  # f, fixup = like "squash", but discard this commit's log message
  # x, exec = run command (the rest of the line) using shell
  # d, drop = remove commit
  #
  # These lines can be re-ordered; they are executed from top to bottom.
  #
  # If you remove a line here THAT COMMIT WILL BE LOST.
  #
  # However, if you remove everything, the rebase will be aborted.
  #
  # Note that empty commits are commented out

Here bf73d7e is ORIG-REV and bc3c567b is AMENDED-REV.  At this point,
I just type "C-x #", since you said "just save and exit" (of course,
saving is not needed, since I didn't edit the buffer).

After that, I see in the shell window:

  $ git rebase --autosquash -i bf73d7e036c5d80cdd112f34255a0ab1ea697c07~1
  Waiting for Emacs...
  Successfully rebased and updated refs/heads/master.

But "git log" shows the original log message for ORIG-REV.

Does this mean it didn't work? or did I misinterpret the steps?

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

[Sean Whitton]

Hello,

Thanks for trying the manual rebase.

On Mon 21 Oct 2024 at 04:48pm +03, Eli Zaretskii wrote:

> But let me describe what I did and saw, to make sure I did it
> correctly.
>
>>     git rebase --autosquash -i REV~1
>> 
>> where REV is the sha1 of the commit whose message you intend to edit.
>
> At this point "git log" shows a revision with my amended log message
> (let's call this AMENDED-REV), followed by the revision with the
> original log message (let's call it ORIG-REV).  So my Git command
> looks like this:
>
>   git rebase --autosquash -i ORIG-REV~1
>
> After this step, I see this in the editor (where I indented the text
> by 2 columns):
>
>   pick bf73d7e Foobar with some Unicode אבגד ą ě č
>   # pick bc3c567b2b136d040fd13373b6594c1ec026fec6 Foobar with some Unicode אבגד ą ě č

There are two things about this output that are unexpected to me:

- The first line of the AMENDED-REV message does not appear to start
  with "amend!".  I thought that you confirmed in a previous message
  that the commit messsage of AMENDED-REV did have this structure:

    amend! <ORIG-REV sha1>

    <AMENDED-REV amended message>

- The line for the AMENDED-REV message is commented out.  That means
  that git-rebase proposes to drop the commit.  I would guess that git
  wants to drop it because it introduces no content changes.

> Does this mean it didn't work? or did I misinterpret the steps?

I think that there are two possibilities.  If you lost the magic
"amend!" commit then please try again with an AMENDED-REV whose commit
message has the special "amend!" structure.

You can create one manually by passing --allow-empty to git-commit, or
you can use the previous version of my patch with everything except the
call to git-commit commented out -- here's the diff doing the
commenting-out again, if you need it.

On the other hand, if your AMENDED-REV does have the special "amend!"
structure, then I think we have to conclude that the older Git you have
does not support these special "amend!" commits at all, and so we can't
support it for log-view-modify-change-comment.

--8<---------------cut here---------------start------------->8---
diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 59d5386bc72..d2ada63f71e 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -2042,8 +2042,8 @@ vc-git-modify-change-comment
     ;;     % git commit --allow-empty -m...
     ;;     % git rebase --autosquash -i REV~1
     ;;     % git stash pop
-    (unless nothing-staged
-      (vc-git-command nil 0 nil "stash" "push"))
+    ;; (unless nothing-staged
+    ;;   (vc-git-command nil 0 nil "stash" "push"))
     (unwind-protect
         (progn
           (when (cl-intersection '("--author" "--date") args
@@ -2064,12 +2064,15 @@ vc-git-modify-change-comment
                           (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" "--autosquash" "-i"
-                      (format "%s~1" rev)))
-    (unless nothing-staged
-      (vc-git-command nil 0 nil "stash" "pop" "--index"))))
+        ;; (delete-file msg-file)
+        ))
+    ;; (with-environment-variables (("GIT_SEQUENCE_EDITOR" "true"))
+    ;;   (vc-git-command nil 0 nil "rebase" "--autosquash" "-i"
+    ;;                   (format "%s~1" rev)))
+    ;; (unless nothing-staged
+    ;;   (vc-git-command nil 0 nil "stash" "pop" "--index"))
+    (message "temporary file is: %s" msg-file)
+    ))

 (defvar vc-git-extra-menu-map
   (let ((map (make-sparse-keymap)))
--8<---------------cut here---------------end--------------->8---

-- 
Sean Whitton

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

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@spwhitton.name>
> Cc: 64055@debbugs.gnu.org,  dgutov@yandex.ru,  Morgan.J.Smith@outlook.com
> Date: Tue, 22 Oct 2024 16:25:52 +0800
> 
> > At this point "git log" shows a revision with my amended log message
> > (let's call this AMENDED-REV), followed by the revision with the
> > original log message (let's call it ORIG-REV).  So my Git command
> > looks like this:
> >
> >   git rebase --autosquash -i ORIG-REV~1
> >
> > After this step, I see this in the editor (where I indented the text
> > by 2 columns):
> >
> >   pick bf73d7e Foobar with some Unicode אבגד ą ě č
> >   # pick bc3c567b2b136d040fd13373b6594c1ec026fec6 Foobar with some Unicode אבגד ą ě č
> 
> There are two things about this output that are unexpected to me:
> 
> - The first line of the AMENDED-REV message does not appear to start
>   with "amend!".  I thought that you confirmed in a previous message
>   that the commit messsage of AMENDED-REV did have this structure:
> 
>     amend! <ORIG-REV sha1>
> 
>     <AMENDED-REV amended message>

It isn't there because you didn't tell me to include it.  The original
commit was lost when I tried to perform the steps the previous time,
so I had to reproduce it, and I used the last part of this step to
produce it:

> So, please use the debugging version of my patch to create the special
> "amend!" commit again, or use the one from yesterday if you still have
> it, or you could manually create it by passing --allow-empty to 'git
> commit'.

IOW, I "manually created it by passing --allow-empty to 'git commit'",
but since you didn't say what log message to give, I used just some
random text, and it didn't include "amend!".  If the log message I use
in this step must have some specific structure or content, please give
more detailed instructions.

> - The line for the AMENDED-REV message is commented out.  That means
>   that git-rebase proposes to drop the commit.  I would guess that git
>   wants to drop it because it introduces no content changes.
> 
> > Does this mean it didn't work? or did I misinterpret the steps?
> 
> I think that there are two possibilities.  If you lost the magic
> "amend!" commit then please try again with an AMENDED-REV whose commit
> message has the special "amend!" structure.

I will, once you give the detailed instructions for how to do it from
the shell prompt.

> You can create one manually by passing --allow-empty to git-commit, or
> you can use the previous version of my patch with everything except the
> call to git-commit commented out -- here's the diff doing the
> commenting-out again, if you need it.

The diff no longer applies to the current master.  And I prefer doing
that manually anyway.  So let's have those instructions in full
detail.

Thanks.

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

[Sean Whitton]

Hello,

On Tue 22 Oct 2024 at 04:27pm +03, Eli Zaretskii wrote:

> It isn't there because you didn't tell me to include it.  The original
> commit was lost when I tried to perform the steps the previous time,
> so I had to reproduce it, and I used the last part of this step to
> produce it:
>
>> So, please use the debugging version of my patch to create the special
>> "amend!" commit again, or use the one from yesterday if you still have
>> it, or you could manually create it by passing --allow-empty to 'git
>> commit'.
>
> IOW, I "manually created it by passing --allow-empty to 'git commit'",
> but since you didn't say what log message to give, I used just some
> random text, and it didn't include "amend!".  If the log message I use
> in this step must have some specific structure or content, please give
> more detailed instructions.
> [...]
> The diff no longer applies to the current master.  And I prefer doing
> that manually anyway.  So let's have those instructions in full
> detail.

Okay, this should do it:

- Manually create a temporary file somewhere outside the repository;
  I'll refer to its absolute file name as TEMP.

- Put the commit message for ORIG-REV into the temporary file.

- Modify that commit message to include the Unicode, or whatever.

- Prepend "amend! <sha1 of ORIG-REV>\n\n".  So for example:

--8<---------------cut here---------------start------------->8---
amend! 212cf3125611b123707feac6f7ffd55a230bc568

Make all the entries in 'eshell-parse-argument-hook' named functions

* lisp/eshell/esh-arg.el (eshell-parse-number, eshell-parse-non-special)
(eshell-parse-whitespace, eshell-parse-comment): New functions...
(eshell-parse-argument-hook): ... use them.   אבגד ą ě č
--8<---------------cut here---------------end--------------->8---

- Save the temporary file.

- Ensure there are no staged changes.
  ('git reset' with no arguments should do it)

- git commit --allow-empty -F TEMP

- git rebase --autosquash -i ORIG-REV~1

-- 
Sean Whitton

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

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@spwhitton.name>
> Cc: 64055@debbugs.gnu.org,  dgutov@yandex.ru,  Morgan.J.Smith@outlook.com
> Date: Tue, 22 Oct 2024 21:42:33 +0800
> 
> - Manually create a temporary file somewhere outside the repository;
>   I'll refer to its absolute file name as TEMP.
> 
> - Put the commit message for ORIG-REV into the temporary file.
> 
> - Modify that commit message to include the Unicode, or whatever.
> 
> - Prepend "amend! <sha1 of ORIG-REV>\n\n".  So for example:
> 
> --8<---------------cut here---------------start------------->8---
> amend! 212cf3125611b123707feac6f7ffd55a230bc568
> 
> Make all the entries in 'eshell-parse-argument-hook' named functions
> 
> * lisp/eshell/esh-arg.el (eshell-parse-number, eshell-parse-non-special)
> (eshell-parse-whitespace, eshell-parse-comment): New functions...
> (eshell-parse-argument-hook): ... use them.   אבגד ą ě č
> --8<---------------cut here---------------end--------------->8---
> 
> - Save the temporary file.
> 
> - Ensure there are no staged changes.
>   ('git reset' with no arguments should do it)
> 
> - git commit --allow-empty -F TEMP
> 
> - git rebase --autosquash -i ORIG-REV~1

This last step shows the following (again, indented 2 columns by me):

  pick bf73d7e Foobar with some Unicode אבגד ą ě č
  # pick d796890ee8cc77ac899954c3e94e5257c5f72615 amend! bf73d7e036c5d80cdd112f34255a0ab1ea697c07

  # Rebase 6c6ea73..d796890 onto 6c6ea73 (1 command)
  #
  # Commands:
  # p, pick = use commit
  # r, reword = use commit, but edit the commit message
  # e, edit = use commit, but stop for amending
  # s, squash = use commit, but meld into previous commit
  # f, fixup = like "squash", but discard this commit's log message
  # x, exec = run command (the rest of the line) using shell
  # d, drop = remove commit
  #
  # These lines can be re-ordered; they are executed from top to bottom.
  #
  # If you remove a line here THAT COMMIT WILL BE LOST.
  #
  # However, if you remove everything, the rebase will be aborted.
  #
  # Note that empty commits are commented out

And if I type "C-x #" to exit, without changing the above in any way
(since you didn't tell me to change anything), I'm back at ORIG-REV
with its original log message.  IOW, the editing of the commit log
message failed.

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

[Sean Whitton]

Hello,

On Tue 22 Oct 2024 at 05:20pm +03, Eli Zaretskii wrote:

> This last step shows the following (again, indented 2 columns by me):
>
>   pick bf73d7e Foobar with some Unicode אבגד ą ě č
>   # pick d796890ee8cc77ac899954c3e94e5257c5f72615 amend!
> bf73d7e036c5d80cdd112f34255a0ab1ea697c07
>
>   # Rebase 6c6ea73..d796890 onto 6c6ea73 (1 command)
>   #
>   # Commands:
>   # p, pick = use commit
>   # r, reword = use commit, but edit the commit message
>   # e, edit = use commit, but stop for amending
>   # s, squash = use commit, but meld into previous commit
>   # f, fixup = like "squash", but discard this commit's log message
>   # x, exec = run command (the rest of the line) using shell
>   # d, drop = remove commit
>   #
>   # These lines can be re-ordered; they are executed from top to bottom.
>   #
>   # If you remove a line here THAT COMMIT WILL BE LOST.
>   #
>   # However, if you remove everything, the rebase will be aborted.
>   #
>   # Note that empty commits are commented out
>
> And if I type "C-x #" to exit, without changing the above in any way
> (since you didn't tell me to change anything), I'm back at ORIG-REV
> with its original log message.  IOW, the editing of the commit log
> message failed.

Thanks.  I think this concludes the testing on old Git on Windows.
There are two conclusions:

- the special handling for MS-Windows character encoding issues works

- the special handling for old Git cannot work, because the older Git
  doesn't recognise the "amend!" commits as special.

So, I'll restore the old sequence of git commands, move the defcustom
into vc.git and call it vc-allow-rewriting-published-history, and
install the change.  (It's late here, so, in the morning.)

-- 
Sean Whitton

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

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@spwhitton.name>
> Cc: 64055@debbugs.gnu.org,  dgutov@yandex.ru,  Morgan.J.Smith@outlook.com
> Date: Sun, 20 Oct 2024 17:25:10 +0800
> 
> Hello,
> 
> On Sun 20 Oct 2024 at 12:19pm +03, Eli Zaretskii wrote:
> 
> >> From: Sean Whitton <spwhitton@spwhitton.name>
> >> Cc: 64055@debbugs.gnu.org,  dgutov@yandex.ru,  Morgan.J.Smith@outlook.com
> >> Date: Sun, 20 Oct 2024 16:59:32 +0800
> >>
> >> Let's try commenting out everything except the 'git commit', as done in
> >> the following diff on top of my patch.  Please test this without
> >> anything staged.  Does it create a new commit at the tip of your branch?
> >
> > Yes.  But only "git log" shows that additional commit, "C-x v l"
> > doesn't update the log.
> 
> If you press 'g' in the 'C-x v l' buffer, does it appear then?

No.  Not even if I restart Emacs.

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

[Dmitry Gutov]

On 20/10/2024 08:19, Sean Whitton wrote:
>> When I do the above, I get an error message:
>>
>>    vc-do-command: Failed (status 128): git --no-pager commit --only
>> --allow-empty -F c:/Users/EliZ/AppData/Local/Temp/git-msgWaNflu
>>
>> and the*vc* buffer says:
>>
>>    fatal: No paths with --include/--only does not make sense.
>>
>> It's possible my Git is very old, but shouldn't this command work with
>> old versions as well?
> I've looked into it and what's required is Git 2.11.1 from early 2017.
> 
> I think we can support older by stashing and unstashing.  So please try
> the attached patch, which does that.

Just to note, looking at https://repology.org/project/git/versions, 
support for Git 2.11 seems ubiquitous enough, even the next-to-last 
Trisquel release (based on Ubuntu 2020) has 2.25. And the very newest 
versions are available in Git for Windows.

And since this is an optional feature, we might as well limit ourselves 
to certain newer Git versions.

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

[Sean Whitton]

Hello,

On Mon 21 Oct 2024 at 12:55am +01, Dmitry Gutov wrote:

> Just to note, looking at https://repology.org/project/git/versions, support
> for Git 2.11 seems ubiquitous enough, even the next-to-last Trisquel release
> (based on Ubuntu 2020) has 2.25. And the very newest versions are available in
> Git for Windows.
>
> And since this is an optional feature, we might as well limit ourselves to
> certain newer Git versions.

AFAICT it's straightforward to support the older git -- basically
replace the --autostash argument to 'git rebase' with a manual stash and
unstash (not because --autostash is unsupported, but because we need the
stash to occur earlier).

If the Windows testing that Eli and I are doing reaches a conclusion
that there are other incompatibilities caused by the older Git, we may
have to abandon trying to support <<2.11.1, so thank you for looking
into the availability.

-- 
Sean Whitton

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

[Dmitry Gutov]

On 20/10/2024 08:19, Sean Whitton wrote:
> +*** 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.

Curious: do we consider Git to be different from others? Or would we 
have corresponding options for Hg, Bzr, (maybe) Svn?

If we think rewriting history dangerous for all, we could mark the 
command as 'disabled' instead.

Otherwise, seems to work well, even though, as you said, a bit slower 
than one might naively expect.

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

[Sean Whitton]

Hello,

On Mon 21 Oct 2024 at 01:09am +01, Dmitry Gutov wrote:

> On 20/10/2024 08:19, Sean Whitton wrote:
>> +*** 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.
>
> Curious: do we consider Git to be different from others? Or would we have
> corresponding options for Hg, Bzr, (maybe) Svn?
>
> If we think rewriting history dangerous for all, we could mark the command as
> 'disabled' instead.

I think that for some VCS editing the commit message is not a form of
rewriting history in any sense.  And we can hope for a future VCS that
keeps change histories for commit messages, so that we might easily
correct our mistakes.  So I don't believe it would make sense to just
disable log-view-modify-change-comment.

We might, though, want to consider a more general
vc-allow-rewriting-history instead of vc-git-allow-rewriting-history,
and use it wherever its applicable -- it will be VCS-dependent where
exactly it applies.

I'm cautious about doing that without more information, because
"rewriting history" is, in my mind, a Git-specific piece of terminology
to begin with.

I think it is probably okay to keep vc-git-allow-rewriting-history even
if we decide to deprecate it in favour of a more general variable at
some point.

> Otherwise, seems to work well, even though, as you said, a bit slower than one
> might naively expect.

Ah, thank you for testing.

I'm not whether the slowness is coming from the safety check in
vc-git--assert-allowed-rewrite or from the rebase command, or both.  I
do know that rebases don't tend to be fast.

-- 
Sean Whitton

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

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@spwhitton.name>
> Cc: Eli Zaretskii <eliz@gnu.org>,  Morgan.J.Smith@outlook.com,
>   64055@debbugs.gnu.org
> Date: Mon, 21 Oct 2024 10:01:28 +0800
> 
> > On 20/10/2024 08:19, Sean Whitton wrote:
> >> +*** 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.
> >
> > Curious: do we consider Git to be different from others? Or would we have
> > corresponding options for Hg, Bzr, (maybe) Svn?
> >
> > If we think rewriting history dangerous for all, we could mark the command as
> > 'disabled' instead.
> 
> I think that for some VCS editing the commit message is not a form of
> rewriting history in any sense.  And we can hope for a future VCS that
> keeps change histories for commit messages, so that we might easily
> correct our mistakes.  So I don't believe it would make sense to just
> disable log-view-modify-change-comment.
> 
> We might, though, want to consider a more general
> vc-allow-rewriting-history instead of vc-git-allow-rewriting-history,
> and use it wherever its applicable -- it will be VCS-dependent where
> exactly it applies.
> 
> I'm cautious about doing that without more information, because
> "rewriting history" is, in my mind, a Git-specific piece of terminology
> to begin with.

Which aspects of "rewriting history" you consider Git-specific?  I
think any dVCS has the same issues with that, and supports similar
workflows, so the concept is relevant to all of them.

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

[Sean Whitton]

Hello,

On Mon 21 Oct 2024 at 08:39am +03, Eli Zaretskii wrote:

> Which aspects of "rewriting history" you consider Git-specific?  I
> think any dVCS has the same issues with that, and supports similar
> workflows, so the concept is relevant to all of them.

Mainly the terminology.

If others think it is fine to go ahead and call the defcustom
vc-allow-rewriting-history, I am happy to do so.

-- 
Sean Whitton

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

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@spwhitton.name>
> Cc: dgutov@yandex.ru,  Morgan.J.Smith@outlook.com,  64055@debbugs.gnu.org
> Date: Mon, 21 Oct 2024 13:46:39 +0800
> 
> Hello,
> 
> On Mon 21 Oct 2024 at 08:39am +03, Eli Zaretskii wrote:
> 
> > Which aspects of "rewriting history" you consider Git-specific?  I
> > think any dVCS has the same issues with that, and supports similar
> > workflows, so the concept is relevant to all of them.
> 
> Mainly the terminology.

I think both rebasing and non-FF pushes can happen with all dVCSes, so
the issue is indeed more general than only with Git.

> If others think it is fine to go ahead and call the defcustom
> vc-allow-rewriting-history, I am happy to do so.

I think we should indeed rename it, yes.

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

[Dmitry Gutov]

On 21/10/2024 08:46, Sean Whitton wrote:
> On Mon 21 Oct 2024 at 08:39am +03, Eli Zaretskii wrote:
> 
>> Which aspects of "rewriting history" you consider Git-specific?  I
>> think any dVCS has the same issues with that, and supports similar
>> workflows, so the concept is relevant to all of them.
> Mainly the terminology.
> 
> If others think it is fine to go ahead and call the defcustom
> vc-allow-rewriting-history, I am happy to do so.

Speaking of Git terminology, "rewriting history" can refer to both 
rewriting published commits (often frowned upon) and rewriting local 
history (can be fine and is often encouraged). The proposed name seems 
to conflate the two (although the docstring does clarify that it's 
referring to the former).

In practice, this also often depends on the upstream branch - e.g. a 
branch "feature/xyz" worked on a single developer might be fine with 
force-pushes.

I guess my point was we could do with only a prompt warning the user 
that the published history will be rewritten (proceeding on if they 
agree). Showing an error is a safer choice, but I suppose then the error 
message could reference the option to customize.

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

[Sean Whitton]

Hello,

On Mon 21 Oct 2024 at 08:56pm +01, Dmitry Gutov wrote:

> On 21/10/2024 08:46, Sean Whitton wrote:
>> On Mon 21 Oct 2024 at 08:39am +03, Eli Zaretskii wrote:
>>
>>> Which aspects of "rewriting history" you consider Git-specific?  I
>>> think any dVCS has the same issues with that, and supports similar
>>> workflows, so the concept is relevant to all of them.
>> Mainly the terminology.
>> If others think it is fine to go ahead and call the defcustom
>> vc-allow-rewriting-history, I am happy to do so.
>
> Speaking of Git terminology, "rewriting history" can refer to both rewriting
> published commits (often frowned upon) and rewriting local history (can be
> fine and is often encouraged). The proposed name seems to conflate the two
> (although the docstring does clarify that it's referring to the former).
>
> In practice, this also often depends on the upstream branch - e.g. a branch
> "feature/xyz" worked on a single developer might be fine with force-pushes.

Yeah.  I'll use vc-allow-rewriting-published-history.  It's not perfect
but it does a better job covering these various issues.

> I guess my point was we could do with only a prompt warning the user that the
> published history will be rewritten (proceeding on if they agree). Showing an
> error is a safer choice, but I suppose then the error message could reference
> the option to customize.

I think that's what's implemented, except for referencing the option
name, which I'll add, thanks.

-- 
Sean Whitton

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

[Dmitry Gutov]

On 20/10/2024 08:19, Sean Whitton wrote:
> +         (nothing-staged
> +          (zerop
> +           (vc-git-command nil t nil "diff" "--cached" "--quiet"))))

Testing it a little more, this seems insufficient because rebase aborts 
on non-staged changes too. This seems to cover both cases:

          (no-changes
           (zerop
            (vc-git-command nil t nil "diff" "HEAD" "--quiet"))))

Regarding the command's latency it seems to be distributed like this in 
my current example:

allowed rewrite?
Elapsed time: 0.028043s
stash push maybe
Elapsed time: 0.072001s
rebase autosquash
Elapsed time: 0.047211s
stash pop
Elapsed time: 0.032561s

So all steps are somewhat expensive, but stashing is #1 in that. Overall 
it seems fine, not a problem for the interactive command.

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

[Sean Whitton]

Hello,

On Mon 21 Oct 2024 at 09:08pm +01, Dmitry Gutov wrote:

> On 20/10/2024 08:19, Sean Whitton wrote:
>> +         (nothing-staged
>> +          (zerop
>> +           (vc-git-command nil t nil "diff" "--cached" "--quiet"))))
>
> Testing it a little more, this seems insufficient because rebase aborts on
> non-staged changes too. This seems to cover both cases:
>
>          (no-changes
>           (zerop
>            (vc-git-command nil t nil "diff" "HEAD" "--quiet"))))

Thanks.  I forgot that --autostash covers that case already.

-- 
Sean Whitton

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

[Dmitry Gutov]

Hi Sean,

On 20/10/2024 08:19, Sean Whitton wrote:
> +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.

Coming back to this text - do you think the first and the last sentences 
might mismatch? First we talk about "many Git commands", next sentence 
"VC commands", plural. And in the end we say this only applies to one.

Maybe we can rephrase it like this:

  Some VCS commands can change your copy of published change history
  without warning.  In VC we try to detect before that happens, and stop.
  You can customize this variable to permit rewriting history
  even though Emacs thinks it is dangerous.

  So far, this applies only to using 'e' from Log View mode for Git.

BTW, there is another existing command which can end up changing 
published history: 'vc-git-log-edit-toggle-amend'. I wonder what will be 
our plan for it. Maybe we just add the same check there.

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

[Sean Whitton]

Hello,

On Sat 26 Oct 2024 at 04:58am +03, Dmitry Gutov wrote:

> Hi Sean,
>
> On 20/10/2024 08:19, Sean Whitton wrote:
>> +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.
>
> Coming back to this text - do you think the first and the last sentences might
> mismatch? First we talk about "many Git commands", next sentence "VC
> commands", plural. And in the end we say this only applies to one.
>
> Maybe we can rephrase it like this:
>
>  Some VCS commands can change your copy of published change history
>  without warning.  In VC we try to detect before that happens, and stop.
>  You can customize this variable to permit rewriting history
>  even though Emacs thinks it is dangerous.
>
>  So far, this applies only to using 'e' from Log View mode for Git.

Whether or not there is a strict mismatch, I like your new text more.
Please install it.

> BTW, there is another existing command which can end up changing
> published history: 'vc-git-log-edit-toggle-amend'. I wonder what will
> be our plan for it. Maybe we just add the same check there.

Thanks for reminding me about this.  Yes, I think it should be fine to
just add (vc-git--assert-allowed-rewrite "HEAD") or similar.  Would you
like to do it along with your NEWS change?  I guess, then, dropping the
"So far ..." sentence.

-- 
Sean Whitton

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

[Dmitry Gutov]

On 26/10/2024 06:12, Sean Whitton wrote:
>> Maybe we can rephrase it like this:
>>
>>   Some VCS commands can change your copy of published change history
>>   without warning.  In VC we try to detect before that happens, and stop.
>>   You can customize this variable to permit rewriting history
>>   even though Emacs thinks it is dangerous.
>>
>>   So far, this applies only to using 'e' from Log View mode for Git.
> Whether or not there is a strict mismatch, I like your new text more.
> Please install it.
> 
>> BTW, there is another existing command which can end up changing
>> published history: 'vc-git-log-edit-toggle-amend'. I wonder what will
>> be our plan for it. Maybe we just add the same check there.
> Thanks for reminding me about this.  Yes, I think it should be fine to
> just add (vc-git--assert-allowed-rewrite "HEAD") or similar.  Would you
> like to do it along with your NEWS change?  I guess, then, dropping the
> "So far ..." sentence.

Great! Pushed in 2030b8c7f24.

Speaking of the description in NEWS, completion just reminded me that we 
also have vc-hg-log-edit-toggle-amend, maybe we could also do something 
about it.

I also got to thinking about the older VCSes supporting 
'modify-change-comment'. OT1H it also modifies "published history" in 
those instances. OT2H it does that in a way that won't lead to later 
conflict, because none of those systems are distributed. So I suppose we 
don't need to worry about them.

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

[Sean Whitton]

Hello,

On Sun 27 Oct 2024 at 03:05am +02, Dmitry Gutov wrote:

> On 26/10/2024 06:12, Sean Whitton wrote:
>>> Maybe we can rephrase it like this:
>>>
>>>   Some VCS commands can change your copy of published change history
>>>   without warning.  In VC we try to detect before that happens, and stop.
>>>   You can customize this variable to permit rewriting history
>>>   even though Emacs thinks it is dangerous.
>>>
>>>   So far, this applies only to using 'e' from Log View mode for Git.
>> Whether or not there is a strict mismatch, I like your new text more.
>> Please install it.
>>
>>> BTW, there is another existing command which can end up changing
>>> published history: 'vc-git-log-edit-toggle-amend'. I wonder what will
>>> be our plan for it. Maybe we just add the same check there.
>> Thanks for reminding me about this.  Yes, I think it should be fine to
>> just add (vc-git--assert-allowed-rewrite "HEAD") or similar.  Would you
>> like to do it along with your NEWS change?  I guess, then, dropping the
>> "So far ..." sentence.
>
> Great! Pushed in 2030b8c7f24.

Thanks.

> Speaking of the description in NEWS, completion just reminded me that
> we also have vc-hg-log-edit-toggle-amend, maybe we could also do
> something about it.

Yes, would be good to.

> I also got to thinking about the older VCSes supporting
> 'modify-change-comment'. OT1H it also modifies "published history" in those
> instances. OT2H it does that in a way that won't lead to later conflict,
> because none of those systems are distributed. So I suppose we don't need to
> worry about them.

Yes, I think that's right.

-- 
Sean Whitton