bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes

bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes

[Sean Whitton]

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

X-debbugs-cc: juri@linkov.net, dgutov@yandex.ru

I seem often to end up with a nonempty index when trying to commit patches
using C-x v v from diff-mode, so I came up with this patch.

-- 
Sean Whitton

[-- Attachment #2: 0001-vc-git-checkin-Offer-to-unstage-conflicting-changes.patch --]
[-- Type: text/x-diff, Size: 3372 bytes --]

From b30ed86e2d2277c94a6655e9ebdeb8253805899d Mon Sep 17 00:00:00 2001
From: Sean Whitton <spwhitton@spwhitton.name>
Date: Fri, 16 Dec 2022 11:28:20 -0700
Subject: [PATCH] vc-git-checkin: Offer to unstage conflicting changes

* lisp/vc/vc-git.el (vc-git-checkin): When committing a patch, if
conflicting changes are already staged, offer to clear them, instead
of just immediately failing with "Index not empty".
---
 lisp/vc/vc-git.el | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 2876a983fb0..dcb51bb933f 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -1030,23 +1030,33 @@ vc-git-checkin
         (with-temp-buffer
           (vc-git-command (current-buffer) t nil "diff" "--cached")
           (goto-char (point-min))
-          (let ((pos (point)) file-diff file-beg)
+          (let ((pos (point)) file-name file-diff file-beg)
             (while (not (eobp))
+              (when (looking-at "^diff --git a/.+ b/\\(.+\\)")
+                (setq file-name (match-string 1)))
               (forward-line 1) ; skip current "diff --git" line
               (search-forward "diff --git" nil 'move)
               (move-beginning-of-line 1)
               (setq file-diff (buffer-substring pos (point)))
-              (if (and (setq file-beg (string-search
-                                       file-diff vc-git-patch-string))
-                       ;; Check that file diff ends with an empty string
-                       ;; or the beginning of the next file diff.
-                       (string-match-p "\\`\\'\\|\\`diff --git"
-                                       (substring
-                                        vc-git-patch-string
-                                        (+ file-beg (length file-diff)))))
-                  (setq vc-git-patch-string
-                        (string-replace file-diff "" vc-git-patch-string))
-                (user-error "Index not empty"))
+              (cond ((and (setq file-beg (string-search
+                                          file-diff vc-git-patch-string))
+                          ;; Check that file diff ends with an empty string
+                          ;; or the beginning of the next file diff.
+                          (string-match-p "\\`\\'\\|\\`diff --git"
+                                          (substring
+                                           vc-git-patch-string
+                                           (+ file-beg (length file-diff)))))
+                     (setq vc-git-patch-string
+                           (string-replace file-diff "" vc-git-patch-string)))
+                    ((and file-name
+                          ;; Check the regexp extracted an actual file
+                          ;; from the 'diff --git' line.
+                          (file-exists-p (expand-file-name file-name root))
+                          (yes-or-no-p
+                           (format "Unstage already-staged changes to %s?"
+                                   file-name)))
+                     (vc-git-command nil 0 file-name "reset" "-q" "--"))
+                    (t (user-error "Index not empty")))
               (setq pos (point))))))
       (let ((patch-file (make-nearby-temp-file "git-patch")))
         (with-temp-file patch-file
-- 
2.30.2

bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes

[Juri Linkov]

> I seem often to end up with a nonempty index when trying to commit patches
> using C-x v v from diff-mode, so I came up with this patch.

From the version 30.0.50 in the subject do I correctly infer
that you propose this patch for master?  In master this
could be installed immediately, but for the release branch
I'd prefer first to test it extensively.

bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes

[Sean Whitton]

Hello,

On Sat 17 Dec 2022 at 07:06PM +02, Juri Linkov wrote:

>> I seem often to end up with a nonempty index when trying to commit patches
>> using C-x v v from diff-mode, so I came up with this patch.
>
> From the version 30.0.50 in the subject do I correctly infer
> that you propose this patch for master?  In master this
> could be installed immediately, but for the release branch
> I'd prefer first to test it extensively.

Yes.  It's a new feature, so I don't see how it could go on the release
branch?

-- 
Sean Whitton

bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes

[Dmitry Gutov]

On 16/12/2022 20:32, Sean Whitton wrote:
> I seem often to end up with a nonempty index when trying to commit patches
> using C-x v v from diff-mode, so I came up with this patch.

Thanks!

Perhaps we should offer to stash the changes, though?

Starting with Git 2.13, 'git stash' now has the 'push' subcommand which 
accepts individual file names:

   https://stackoverflow.com/a/5506483/615245

Or make it a three-way choice with read-multiple-choice.

bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes

[Sean Whitton]

Hello,

On Sun 18 Dec 2022 at 03:08AM +02, Dmitry Gutov wrote:

> Perhaps we should offer to stash the changes, though?
>
> Starting with Git 2.13, 'git stash' now has the 'push' subcommand which
> accepts individual file names:
>
>   https://stackoverflow.com/a/5506483/615245
>
> Or make it a three-way choice with read-multiple-choice.

This is a nice suggestion.  A step further would be to unconditionally
stash and unstash.  Given how committing patches with C-x v v works, I
don't believe it can ever be the case that the stash is not applicable
afterwards?  If that's wrong, I'll implement what you suggest.

-- 
Sean Whitton

bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes

[Dmitry Gutov]

On 20/12/2022 00:30, Sean Whitton wrote:
> This is a nice suggestion.  A step further would be to unconditionally
> stash and unstash.  Given how committing patches with C-x v v works, I
> don't believe it can ever be the case that the stash is not applicable
> afterwards?

I'm not sure that's 100% true, given that we'll want to stage the 
contents of the staging area (which are supposedly represented as diffs 
against the last committed state), and our command, while keeping the 
contents of files on disk intact, moves the last commit to a new state.

> If that's wrong, I'll implement what you suggest.

...but we might as well try and experiment. Worst case: the stash won't 
apply cleanly and the user will have to do it by hand. That would mean 
no big loss of information, at least.

bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes

[Sean Whitton]

Hello,

On Tue 20 Dec 2022 at 02:53AM +02, Dmitry Gutov wrote:

> On 20/12/2022 00:30, Sean Whitton wrote:
>> This is a nice suggestion.  A step further would be to unconditionally
>> stash and unstash.  Given how committing patches with C-x v v works, I
>> don't believe it can ever be the case that the stash is not applicable
>> afterwards?
>
> I'm not sure that's 100% true, given that we'll want to stage the contents of
> the staging area (which are supposedly represented as diffs against the last
> committed state), and our command, while keeping the contents of files on disk
> intact, moves the last commit to a new state.

Good point about the representation of the index.

In addition, you can't stash just the content of the staging area, and
not also the working tree, without hacks -- see `magit-stash-save' as
called by `magit-stash-index'.  I don't want to reproduce those hacks,
so I was thinking we would be stashing both the worktree and the index
for any and all files that have any changes staged.  However ...

>> If that's wrong, I'll implement what you suggest.
>
> ...but we might as well try and experiment. Worst case: the stash won't apply
> cleanly and the user will have to do it by hand. That would mean no big loss
> of information, at least.

... for files that are modified by the patch to be committed, it's very
easy to generate situations where git can't or won't apply the stash.
It's true that there isn't a loss of information, but it's a frustrating
context switch for the user, who might not have even realised a stash
would be created, and now has to untangle things.

So, I'm now thinking:

- automatically stash index+worktree for any files with changes staged
  that are *not* modified by the patch to be committed

- offer to unstage any files with changes staged that *are* modified by
  the patch to be committed.

This way, we have to prompt the user only when files involved in the
patch have staged changes, and we shouldn't ever force the user into
dealing with a stash that won't apply.

-- 
Sean Whitton

bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes

[Dmitry Gutov]

On 20/12/2022 08:43, Sean Whitton wrote:
> In addition, you can't stash just the content of the staging area, and
> not also the working tree, without hacks

I think

   git stash push --staged -- file1 file2 ...

does that.

bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes

[Sean Whitton]

Hello,

On Tue 20 Dec 2022 at 03:47PM +02, Dmitry Gutov wrote:

> On 20/12/2022 08:43, Sean Whitton wrote:
>> In addition, you can't stash just the content of the staging area, and
>> not also the working tree, without hacks
>
> I think
>
>   git stash push --staged -- file1 file2 ...
>
> does that.

Ah nice, though, my git doesn't have that option (Debian stable).

-- 
Sean Whitton

bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes

[Dmitry Gutov]

On 20/12/2022 08:43, Sean Whitton wrote:
> So, I'm now thinking:
> 
> - automatically stash index+worktree for any files with changes staged
>    that are*not*  modified by the patch to be committed

I think it's possible to just skip those when checking the index area. 
And then, when committing, specify individual files to commit from the 
index.

E.g. this way:

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index b5959d535c0..dee102d8586 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -1052,7 +1052,7 @@ vc-git-checkin
                 (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)
+      (apply #'vc-git-command nil 0 (if only files)
               (nconc (if msg-file (list "commit" "-F"
                                         (file-local-name msg-file))
                        (list "commit" "-m"))

(I'm not sure if the list of files is passed to this function correctly 
when committing a patch; if not, fixing that would also be needed.)

> - offer to unstage any files with changes staged that*are*  modified by
>    the patch to be committed.

Or we could just abort, like we do now. Up to you (do you encounter this 
particular situation often?).

These could be two separate changes anyway.

bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes

[Sean Whitton]

Hello,

On Tue 20 Dec 2022 at 05:13PM +02, Dmitry Gutov wrote:

> On 20/12/2022 08:43, Sean Whitton wrote:
>> So, I'm now thinking:
>> - automatically stash index+worktree for any files with changes staged
>>    that are*not*  modified by the patch to be committed
>
> I think it's possible to just skip those when checking the index area. And
> then, when committing, specify individual files to commit from the index.

Ah, so it is!

> E.g. this way:
>
> diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
> index b5959d535c0..dee102d8586 100644
> --- a/lisp/vc/vc-git.el
> +++ b/lisp/vc/vc-git.el
> @@ -1052,7 +1052,7 @@ vc-git-checkin
>                 (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)
> +      (apply #'vc-git-command nil 0 (if only files)
>               (nconc (if msg-file (list "commit" "-F"
>                                         (file-local-name msg-file))
>                        (list "commit" "-m"))
>
> (I'm not sure if the list of files is passed to this function correctly when
> committing a patch; if not, fixing that would also be needed.)

It's not usually passed, but it could be.  So I think I need to clear
out files and populate it again during the loop through
vc-git-patch-string.  We'll want this always to happen, so in the commit
command we'll want (and (or only vc-git-patch-string) files).

>> - offer to unstage any files with changes staged that*are*  modified by
>>    the patch to be committed.
>
> Or we could just abort, like we do now. Up to you (do you encounter this
> particular situation often?).

I feel like I do, yes, though I'm not completely sure which of the two
situations I keep finding myself in.  It does no harm to have the
feature, at least.

Thank you for the input.

-- 
Sean Whitton

bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes

[Sean Whitton]

Hello,

On Tue 20 Dec 2022 at 10:04AM -07, Sean Whitton wrote:

> On Tue 20 Dec 2022 at 05:13PM +02, Dmitry Gutov wrote:
>
>> On 20/12/2022 08:43, Sean Whitton wrote:
>>> So, I'm now thinking:
>>> - automatically stash index+worktree for any files with changes staged
>>>    that are*not*  modified by the patch to be committed
>>
>> I think it's possible to just skip those when checking the index area. And
>> then, when committing, specify individual files to commit from the index.
>
> Ah, so it is!

It turns out this doesn't work: if you specify individual files to
commit from the index, then git also includes any changes to those files
in the worktree too, and you can't ask it not to, because --only is
implied whenever files are supplied on the command line.

In other words, when vc-git-patch-string is non-nil, we mustn't pass a
list of files to git.  So if there are files not involved in the commit
with staged changes, we need to stash those.

I tried implementing that, which is not hard, but then we pop that
stash, the staged changes aren't restored to the index.  The result is
that if the user has a mixture of staged and unstaged changes to a file
which is not part of the commit, then afterwards the unstaged changes
will have been unstaged, mixed in with the staged changes again.  In
some circumstances this could constitute a loss of work.

There are a few ways to overcome this.  We can use the --staged option,
but that's only available in very recent versions of git.  We could
perform a complex double-stash:

- git stash push -k -- foo
- git reset -- foo
- git stash push -k -- foo
- [commit]
- git stash pop
- git add -- foo
- git stash pop

Or we could do something like what Magit does to stash only the index.

Any thoughts on what would be best?

In the meantime, I've pushed a version of my previous patch, as I think
it makes sense to implement the stashing, if we do, as a second change.

-- 
Sean Whitton

bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes

[Sean Whitton]

Hello,

On Tue 20 Dec 2022 at 04:10PM -07, Sean Whitton wrote:

> ... afterwards the unstaged changes
> will have been unstaged, mixed in with the staged changes again.

afterwards the staged changes will have been unstaged, mixed in with the
unstaged changes again*

-- 
Sean Whitton

bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes

[Dmitry Gutov]

On 21/12/2022 01:10, Sean Whitton wrote:
> I tried implementing that, which is not hard, but then we pop that
> stash, the staged changes aren't restored to the index.  The result is
> that if the user has a mixture of staged and unstaged changes to a file
> which is not part of the commit, then afterwards the unstaged changes
> will have been unstaged, mixed in with the staged changes again.  In
> some circumstances this could constitute a loss of work.
> 
> There are a few ways to overcome this.  We can use the --staged option,
> but that's only available in very recent versions of git.

IIUC the --staged option is indeed limited to the very new Git, but that 
option is used when creating a stash (when we want to stash the staging 
area only).

When restoring a stash, to reinstate the stashed index, you would use 
the option --index. It's older than --staged (e.g. it's available in Git 
2.22.0, and that's as far back as the docs at git-scm.com/docs go). Not 
sure if it's in Debian Stable or not.

Regarding the alternatives -- double stashing, or the Magit way, it's 
hard to form a strong opinion before examining them in detail (I trust 
you can make a good choice).

For completeness, though, here's a way to implement 'git push --staged' 
with Git plumbing manually: https://stackoverflow.com/a/72582276/615245

And as for a 'git pop --index' substitute, if the stash contains only 
the index area stuff, it might be as easy as

   git diff stash@{0}^..stash@{0} > patch.diff
   git apply --cached patch.diff
   git stash drop

bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes

[Sean Whitton]

Hello,

On Wed 21 Dec 2022 at 01:45AM +02, Dmitry Gutov wrote:

> IIUC the --staged option is indeed limited to the very new Git, but that
> option is used when creating a stash (when we want to stash the staging area
> only).
>
> When restoring a stash, to reinstate the stashed index, you would use the
> option --index. It's older than --staged (e.g. it's available in Git 2.22.0,
> and that's as far back as the docs at git-scm.com/docs go). Not sure if it's
> in Debian Stable or not.

Ah, thanks for reminding me, I was getting mixed up.

Unfortunately it's probably not much use, because 'git stash push -- x'
stashes all staged changes, it turns out, not just those in x.

> Regarding the alternatives -- double stashing, or the Magit way, it's
> hard to form a strong opinion before examining them in detail (I trust
> you can make a good choice).
>
> For completeness, though, here's a way to implement 'git push --staged' with
> Git plumbing manually: https://stackoverflow.com/a/72582276/615245
>
> And as for a 'git pop --index' substitute, if the stash contains only the
> index area stuff, it might be as easy as
>
>   git diff stash@{0}^..stash@{0} > patch.diff
>   git apply --cached patch.diff
>   git stash drop

These references are helpful.  I'll investigate further.

-- 
Sean Whitton

bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes

[Sean Whitton]

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

Hello,

On Thu 22 Dec 2022 at 05:12PM -07, Sean Whitton wrote:

>> For completeness, though, here's a way to implement 'git push --staged' with
>> Git plumbing manually: https://stackoverflow.com/a/72582276/615245
>>
>> And as for a 'git pop --index' substitute, if the stash contains only the
>> index area stuff, it might be as easy as
>>
>>   git diff stash@{0}^..stash@{0} > patch.diff
>>   git apply --cached patch.diff
>>   git stash drop
>
> These references are helpful.  I'll investigate further.

Here is my patch.

It works, except that sometimes the let-binding of process-environment
fails, such that the commands affect the normal index rather than the
temporary index.  Can you see what I'm doing wrong there?

Thanks.

-- 
Sean Whitton

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-vc-git-checkin-Stash-other-staged-changes.patch --]
[-- Type: text/x-patch, Size: 7741 bytes --]

From acea3dcd080c1cfacbddeb257683438de8a9b325 Mon Sep 17 00:00:00 2001
From: Sean Whitton <spwhitton@spwhitton.name>
Date: Thu, 22 Dec 2022 20:54:08 -0700
Subject: [PATCH] vc-git-checkin: Stash other staged changes

* lisp/vc/vc-git.el (vc-git--stash-staged-changes): New function.
(vc-git-checkin): Use new function to avoid needing to unstage changes
unrelated to the patch we want to commit (bug#60126).
---
 lisp/vc/vc-git.el | 97 ++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 83 insertions(+), 14 deletions(-)

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 0a4e9caa614..86e8f5894df 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -1020,22 +1020,36 @@ vc-git-checkin
           ;; message.  Handle also remote files.
           (if (eq system-type 'windows-nt)
               (let ((default-directory (file-name-directory file1)))
-                (make-nearby-temp-file "git-msg")))))
+                (make-nearby-temp-file "git-msg"))))
+         to-stash)
     (when vc-git-patch-string
       (unless (zerop (vc-git-command nil t nil "diff" "--cached" "--quiet"))
-        ;; Check that all staged changes also exist in the patch.
-        ;; This is needed to allow adding/removing files that are
-        ;; currently staged to the index.  So remove the whole file diff
-        ;; from the patch because commit will take it from the index.
+        ;; Check that what's already staged is compatible with what
+        ;; we want to commit (bug#60126).
+        ;;
+        ;; 1. If the changes to a file in the index are identical to
+        ;;    the changes to that file we want to commit, remove the
+        ;;    changes from our patch, and let the commit take them
+        ;;    from the index.  This is necessary for adding and
+        ;;    removing files to work.
+        ;;
+        ;; 2. If the changes to a file in the index are different to
+        ;;    changes to that file we want to commit, then we have to
+        ;;    unstage the changes or abort.
+        ;;
+        ;; 3. If there are changes to a file in the index but we don't
+        ;;    want to commit any changes to that file, we need to
+        ;;    stash those changes before committing.
         (with-temp-buffer
           (vc-git-command (current-buffer) t nil "diff" "--cached")
           (goto-char (point-min))
-          (let ((pos (point)) file-name file-diff file-beg)
+          (let ((pos (point)) file-name file-header file-diff file-beg)
             (while (not (eobp))
               (when (and (looking-at "^diff --git a/\\(.+\\) b/\\(.+\\)")
                          (string= (match-string 1) (match-string 2)))
                 (setq file-name (match-string 1)))
               (forward-line 1) ; skip current "diff --git" line
+              (setq file-header (buffer-substring pos (point)))
               (search-forward "diff --git" nil 'move)
               (move-beginning-of-line 1)
               (setq file-diff (buffer-substring pos (point)))
@@ -1049,12 +1063,15 @@ vc-git-checkin
                                            (+ file-beg (length file-diff)))))
                      (setq vc-git-patch-string
                            (string-replace file-diff "" vc-git-patch-string)))
-                    ((and file-name
-                          (yes-or-no-p
-                           (format "Unstage already-staged changes to %s?"
-                                   file-name)))
-                     (vc-git-command nil 0 file-name "reset" "-q" "--"))
-                    (t (user-error "Index not empty")))
+                    ((string-match (format "^%s" (regexp-quote file-header))
+                                   vc-git-patch-string)
+                     (if (and file-name
+                              (yes-or-no-p
+                               (format "Unstage already-staged changes to %s?"
+                                       file-name)))
+                         (vc-git-command nil 0 file-name "reset" "-q" "--")
+                       (user-error "Index not empty")))
+                    (t (push file-name to-stash)))
               (setq pos (point))))))
       (unless (string-empty-p vc-git-patch-string)
         (let ((patch-file (make-nearby-temp-file "git-patch")))
@@ -1062,7 +1079,8 @@ vc-git-checkin
             (insert vc-git-patch-string))
           (unwind-protect
               (vc-git-command nil 0 patch-file "apply" "--cached")
-            (delete-file patch-file)))))
+            (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)))))
@@ -1088,7 +1106,58 @@ vc-git-checkin
                       args)
                     (unless vc-git-patch-string
                       (if only (list "--only" "--") '("-a"))))))
-    (if (and msg-file (file-exists-p msg-file)) (delete-file msg-file))))
+    (if (and msg-file (file-exists-p msg-file)) (delete-file msg-file))
+    (when to-stash
+      (let ((cached (make-nearby-temp-file "git-cached")))
+        (unwind-protect
+            (progn (with-temp-file cached
+                     (vc-git-command t 0 nil "stash" "show" "-p"))
+                   (vc-git-command nil 0 cached "apply" "--cached"))
+          (delete-file cached))
+        (vc-git-command nil 0 nil "stash" "drop")))))
+
+(defun vc-git--stash-staged-changes (files &optional message)
+  "Stash only the staged changes to FILES with description MESSAGE."
+  ;; This is necessary because even if you pass a list of file names
+  ;; to git-stash(1), it will stash any and all staged changes.
+  (unless (zerop
+           (vc-git-command nil t files "diff" "--cached" "--quiet"))
+    (unless message (setq message "Previously staged changes"))
+    (cl-flet
+        ((git-string (&rest args)
+           (string-trim-right
+            (with-output-to-string
+              (apply #'vc-git-command standard-output 0 nil args)))))
+      (let ((cached (make-nearby-temp-file "git-cached"))
+            tree)
+        ;; Use a temporary index to create a tree object corresponding
+        ;; to the staged changes to FILES.
+        (unwind-protect
+            (progn
+              (with-temp-file cached
+                (vc-git-command t 0 files "diff" "--cached" "--"))
+              (let* ((index (make-nearby-temp-file "git-index"))
+                     (process-environment
+                      (cons (format "GIT_INDEX_FILE=%s" index)
+                            process-environment)))
+                (unwind-protect
+                    (progn
+                      (vc-git-command nil 0 nil "read-tree" "HEAD")
+                      (vc-git-command nil 0 cached "apply" "--cached")
+                      (setq tree (git-string "write-tree")))
+                  (delete-file index))))
+          (delete-file cached))
+        ;; Prepare stash commit object, which has a special structure.
+        (let* ((tree-commit (git-string "commit-tree" "-m" message
+                                        "-p" "HEAD" tree))
+               (stash-commit (git-string "commit-tree" "-m" message
+                                         "-p" "HEAD" "-p" tree-commit
+                                         tree)))
+          ;; Push the new stash entry.
+          (vc-git-command nil 0 nil "update-ref" "--create-reflog"
+                          "-m" message "refs/stash" stash-commit)
+          ;; Unstage the changes we've now stashed.
+          (vc-git-command nil 0 files "reset" "--"))))))
 
 (defun vc-git-find-revision (file rev buffer)
   (let* (process-file-side-effects
-- 
2.30.2

bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes

[Eli Zaretskii]

> Cc: 60126@debbugs.gnu.org, juri@linkov.net
> From: Sean Whitton <spwhitton@spwhitton.name>
> Date: Thu, 22 Dec 2022 20:59:53 -0700
> 
> +        ;; Prepare stash commit object, which has a special structure.
> +        (let* ((tree-commit (git-string "commit-tree" "-m" message

Can 'message' include newlines? if so, we need to either convert the
newlines to spaces, or invoke the command with -F instead, writing the
message to a temporary file.  Because otherwise this will fail on
Windows.

bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes

[Sean Whitton]

Hello,

On Fri 23 Dec 2022 at 10:16AM +02, Eli Zaretskii wrote:

>> Cc: 60126@debbugs.gnu.org, juri@linkov.net
>> From: Sean Whitton <spwhitton@spwhitton.name>
>> Date: Thu, 22 Dec 2022 20:59:53 -0700
>>
>> +        ;; Prepare stash commit object, which has a special structure.
>> +        (let* ((tree-commit (git-string "commit-tree" "-m" message
>
> Can 'message' include newlines? if so, we need to either convert the
> newlines to spaces, or invoke the command with -F instead, writing the
> message to a temporary file.  Because otherwise this will fail on
> Windows.

Ah, thank you.

These "messages" are more like labels, or names, than commit messages.
I've gone ahead and hard coded it for the time being.

-- 
Sean Whitton

bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes

[Dmitry Gutov]

On 23/12/2022 05:59, Sean Whitton wrote:
> It works, except that sometimes the let-binding of process-environment
> fails, such that the commands affect the normal index rather than the
> temporary index.  Can you see what I'm doing wrong there?

Could you describe be "sometimes" occurrences? Does that happen through 
repeating a similar action several times? Or slightly different actions?

The process-environment setup seems fine. We did corrupt it in 1-2 
places in the past using 'setenv', but I don't see anything like that in 
the current codebase. And the effect would probably be different anyway.

bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes

[Sean Whitton]

Hello,

On Sat 24 Dec 2022 at 01:18AM +02, Dmitry Gutov wrote:

> On 23/12/2022 05:59, Sean Whitton wrote:
>> It works, except that sometimes the let-binding of process-environment
>> fails, such that the commands affect the normal index rather than the
>> temporary index.  Can you see what I'm doing wrong there?
>
> Could you describe be "sometimes" occurrences? Does that happen through
> repeating a similar action several times? Or slightly different actions?
>
> The process-environment setup seems fine. We did corrupt it in 1-2 places in
> the past using 'setenv', but I don't see anything like that in the current
> codebase. And the effect would probably be different anyway.

Thank you for looking.  Slightly embarassingly, I can't reproduce the
problem today.  So I've gone ahead and pushed.

I am pretty happy with this approach, in the end.  Compared with other
possible uses of 'git stash', it's quite clean:

- it doesn't touch the worktree copies of the files not involved in the
  commit

- it stashes a single diff, rather than two diffs (one for the worktree
  and one for the index), which is less for the user to deal with if
  manual recovery becomes required.

Thanks again for the helpful discussion on this one.

-- 
Sean Whitton

bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes

[Dmitry Gutov]

On 24/12/2022 04:02, Sean Whitton wrote:
> Hello,
> 
> On Sat 24 Dec 2022 at 01:18AM +02, Dmitry Gutov wrote:
> 
>> On 23/12/2022 05:59, Sean Whitton wrote:
>>> It works, except that sometimes the let-binding of process-environment
>>> fails, such that the commands affect the normal index rather than the
>>> temporary index.  Can you see what I'm doing wrong there?
>> Could you describe be "sometimes" occurrences? Does that happen through
>> repeating a similar action several times? Or slightly different actions?
>>
>> The process-environment setup seems fine. We did corrupt it in 1-2 places in
>> the past using 'setenv', but I don't see anything like that in the current
>> codebase. And the effect would probably be different anyway.
> Thank you for looking.  Slightly embarassingly, I can't reproduce the
> problem today.  So I've gone ahead and pushed.

Thanks!

> I am pretty happy with this approach, in the end.  Compared with other
> possible uses of 'git stash', it's quite clean:
> 
> - it doesn't touch the worktree copies of the files not involved in the
>    commit
> 
> - it stashes a single diff, rather than two diffs (one for the worktree
>    and one for the index), which is less for the user to deal with if
>    manual recovery becomes required.

It does indeed feel like we ended up in a good place. The code was 
pretty complex, though, and got more so.

We don't have tests covering vc-git-checkin-patch at all. Any chance 
you'll fancy working on adding those? Even if you only cover the 
scenarios where the user doesn't get prompted at all (either there's 
nothing staged, or the staged contents match the committed patch).

Writing (and debugging) a test could also help sort out fiddly 
behaviors, like the one you may have seen yesterday.

We have a default implementation for checkin-patch, so adding generic 
test for all major backends could work (in vc-tests.el). One or two 
extra tests could be also predicated on (eq backend 'Git), so that we 
don't yet need to copy the repository setup/teardown code to vc-git.el.

bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes

[Sean Whitton]

Hello,

On Sat 24 Dec 2022 at 04:50PM +02, Dmitry Gutov wrote:

> It does indeed feel like we ended up in a good place. The code was pretty
> complex, though, and got more so.
>
> We don't have tests covering vc-git-checkin-patch at all. Any chance you'll
> fancy working on adding those? Even if you only cover the scenarios where the
> user doesn't get prompted at all (either there's nothing staged, or the staged
> contents match the committed patch).
>
> Writing (and debugging) a test could also help sort out fiddly behaviors, like
> the one you may have seen yesterday.
>
> We have a default implementation for checkin-patch, so adding generic test for
> all major backends could work (in vc-tests.el). One or two extra tests could
> be also predicated on (eq backend 'Git), so that we don't yet need to copy the
> repository setup/teardown code to vc-git.el.

Now that we understand clearly what we want it to do, I bet the code in
vc-checkin-git could be simplified (vc-git--stash-staged-changes is
fine).  So I'll see about doing that, with some tests, as you suggest.

Let me ask you about the parsing of the 'diff --git' lines.  I wasn't
happy with my regexp approach to extracting the filename.  I'm not sure
it can actually fail, but the current codes assumes it can, and that
adds complexity.

The --src-prefix, --dst-prefix and --no-prefix options to git-diff(1)
might be relevant, but then we couldn't use a simple string-match to
find hunks in vc-git-patch-string.

Do you have any better ideas of how to extract the filename from the git
--diff line, or perhaps a proof that my approach can't fail? :)

-- 
Sean Whitton

bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes

[Dmitry Gutov]

On 24/12/2022 20:22, Sean Whitton wrote:

> Now that we understand clearly what we want it to do, I bet the code in
> vc-checkin-git could be simplified (vc-git--stash-staged-changes is
> fine).  So I'll see about doing that, with some tests, as you suggest.
> 
> Let me ask you about the parsing of the 'diff --git' lines.
...
 > Do you have any better ideas of how to extract the filename from the git
 > --diff line, or perhaps a proof that my approach can't fail? :)

I don't know. You could try

   (and (looking-at diff-file-header-re) (match-string 1))

instead, but it matches a different line (one that starts with "---").

> I wasn't
> happy with my regexp approach to extracting the filename.  I'm not sure
> it can actually fail, but the current codes assumes it can, and that
> adds complexity.

Not sure which failure you are referring to, but the process of removing 
already-staged hunks from vc-git-patch-string can indeed fail, because 
the hunks might be staged, or might be not. The idea was to support both 
situations.

> The --src-prefix, --dst-prefix and --no-prefix options to git-diff(1)
> might be relevant, but then we couldn't use a simple string-match to
> find hunks in vc-git-patch-string.

Right.

bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes

[Sean Whitton]

Hello,

On Sat 24 Dec 2022 at 09:26PM +02, Dmitry Gutov wrote:

> On 24/12/2022 20:22, Sean Whitton wrote:
>
>> Now that we understand clearly what we want it to do, I bet the code in
>> vc-checkin-git could be simplified (vc-git--stash-staged-changes is
>> fine).  So I'll see about doing that, with some tests, as you suggest.
>> Let me ask you about the parsing of the 'diff --git' lines.
> ...
>> Do you have any better ideas of how to extract the filename from the git
>> --diff line, or perhaps a proof that my approach can't fail? :)
>
> I don't know. You could try
>
>   (and (looking-at diff-file-header-re) (match-string 1))
>
> instead, but it matches a different line (one that starts with "---").

That seems better, thank you.

>> I wasn't happy with my regexp approach to extracting the filename.
>> I'm not sure it can actually fail, but the current codes assumes it
>> can, and that adds complexity.
>
> Not sure which failure you are referring to, but the process of removing
> already-staged hunks from vc-git-patch-string can indeed fail, because the
> hunks might be staged, or might be not. The idea was to support both
> situations.

I meant that my regexp might fail to correctly parse the 'diff --git'
line, and then the code goes straight to "Index not empty."

>> The --src-prefix, --dst-prefix and --no-prefix options to git-diff(1)
>> might be relevant, but then we couldn't use a simple string-match to
>> find hunks in vc-git-patch-string.
>
> Right.
>

-- 
Sean Whitton

bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes

[Dmitry Gutov]

On 23/12/2022 02:12, Sean Whitton wrote:
> Unfortunately it's probably not much use, because 'git stash push -- x'
> stashes all staged changes, it turns out, not just those in x.

Seems to work fine over here.

And the manual says:

    <pathspec>...
        This option is only valid for push command.

        The new stash entry records the modified states only for the 
files that match the pathspec. The index entries and working tree files 
are then rolled back to the state in HEAD only for
        these files, too, leaving files that do not match the pathspec 
intact.

        For more details, see the pathspec entry in gitglossary(7).

Check out 'man git stash', perhaps your version of Git just doesn't have 
that feature yet. Though I thought it came with the introduction of 'git 
stash push', as opposed to 'git stash save'.

bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes

[Juri Linkov]

> (I'm not sure if the list of files is passed to this function correctly
> when committing a patch; if not, fixing that would also be needed.)

A list of files should be correct, but needs more testing to confirm.