* bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes @ 2022-12-16 18:32 Sean Whitton 2022-12-17 17:06 ` Juri Linkov 2022-12-18 1:08 ` Dmitry Gutov 0 siblings, 2 replies; 26+ messages in thread From: Sean Whitton @ 2022-12-16 18:32 UTC (permalink / raw) To: 60126; +Cc: juri, dgutov [-- 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes 2022-12-16 18:32 bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes Sean Whitton @ 2022-12-17 17:06 ` Juri Linkov 2022-12-18 0:20 ` Sean Whitton 2022-12-18 1:08 ` Dmitry Gutov 1 sibling, 1 reply; 26+ messages in thread From: Juri Linkov @ 2022-12-17 17:06 UTC (permalink / raw) To: Sean Whitton; +Cc: 60126, dgutov > 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes 2022-12-17 17:06 ` Juri Linkov @ 2022-12-18 0:20 ` Sean Whitton 0 siblings, 0 replies; 26+ messages in thread From: Sean Whitton @ 2022-12-18 0:20 UTC (permalink / raw) To: Juri Linkov; +Cc: 60126, dgutov 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes 2022-12-16 18:32 bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes Sean Whitton 2022-12-17 17:06 ` Juri Linkov @ 2022-12-18 1:08 ` Dmitry Gutov 2022-12-19 22:30 ` Sean Whitton 1 sibling, 1 reply; 26+ messages in thread From: Dmitry Gutov @ 2022-12-18 1:08 UTC (permalink / raw) To: Sean Whitton, 60126; +Cc: juri 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes 2022-12-18 1:08 ` Dmitry Gutov @ 2022-12-19 22:30 ` Sean Whitton 2022-12-20 0:53 ` Dmitry Gutov 0 siblings, 1 reply; 26+ messages in thread From: Sean Whitton @ 2022-12-19 22:30 UTC (permalink / raw) To: Dmitry Gutov; +Cc: 60126, juri 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes 2022-12-19 22:30 ` Sean Whitton @ 2022-12-20 0:53 ` Dmitry Gutov 2022-12-20 6:43 ` Sean Whitton 0 siblings, 1 reply; 26+ messages in thread From: Dmitry Gutov @ 2022-12-20 0:53 UTC (permalink / raw) To: Sean Whitton; +Cc: 60126, juri 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes 2022-12-20 0:53 ` Dmitry Gutov @ 2022-12-20 6:43 ` Sean Whitton 2022-12-20 13:47 ` Dmitry Gutov 2022-12-20 15:13 ` Dmitry Gutov 0 siblings, 2 replies; 26+ messages in thread From: Sean Whitton @ 2022-12-20 6:43 UTC (permalink / raw) To: Dmitry Gutov; +Cc: 60126, juri 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes 2022-12-20 6:43 ` Sean Whitton @ 2022-12-20 13:47 ` Dmitry Gutov 2022-12-20 16:47 ` Sean Whitton 2022-12-20 15:13 ` Dmitry Gutov 1 sibling, 1 reply; 26+ messages in thread From: Dmitry Gutov @ 2022-12-20 13:47 UTC (permalink / raw) To: Sean Whitton; +Cc: 60126, juri 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes 2022-12-20 13:47 ` Dmitry Gutov @ 2022-12-20 16:47 ` Sean Whitton 0 siblings, 0 replies; 26+ messages in thread From: Sean Whitton @ 2022-12-20 16:47 UTC (permalink / raw) To: Dmitry Gutov; +Cc: 60126, juri 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes 2022-12-20 6:43 ` Sean Whitton 2022-12-20 13:47 ` Dmitry Gutov @ 2022-12-20 15:13 ` Dmitry Gutov 2022-12-20 17:04 ` Sean Whitton 2022-12-20 17:13 ` Juri Linkov 1 sibling, 2 replies; 26+ messages in thread From: Dmitry Gutov @ 2022-12-20 15:13 UTC (permalink / raw) To: Sean Whitton; +Cc: 60126, juri 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. ^ permalink raw reply related [flat|nested] 26+ messages in thread
* bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes 2022-12-20 15:13 ` Dmitry Gutov @ 2022-12-20 17:04 ` Sean Whitton 2022-12-20 23:10 ` Sean Whitton 2022-12-20 17:13 ` Juri Linkov 1 sibling, 1 reply; 26+ messages in thread From: Sean Whitton @ 2022-12-20 17:04 UTC (permalink / raw) To: Dmitry Gutov; +Cc: 60126, juri 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes 2022-12-20 17:04 ` Sean Whitton @ 2022-12-20 23:10 ` Sean Whitton 2022-12-20 23:41 ` Sean Whitton 2022-12-20 23:45 ` Dmitry Gutov 0 siblings, 2 replies; 26+ messages in thread From: Sean Whitton @ 2022-12-20 23:10 UTC (permalink / raw) To: Dmitry Gutov; +Cc: 60126, juri 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes 2022-12-20 23:10 ` Sean Whitton @ 2022-12-20 23:41 ` Sean Whitton 2022-12-20 23:45 ` Dmitry Gutov 1 sibling, 0 replies; 26+ messages in thread From: Sean Whitton @ 2022-12-20 23:41 UTC (permalink / raw) To: Dmitry Gutov; +Cc: 60126, juri 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes 2022-12-20 23:10 ` Sean Whitton 2022-12-20 23:41 ` Sean Whitton @ 2022-12-20 23:45 ` Dmitry Gutov 2022-12-23 0:12 ` Sean Whitton 1 sibling, 1 reply; 26+ messages in thread From: Dmitry Gutov @ 2022-12-20 23:45 UTC (permalink / raw) To: Sean Whitton; +Cc: 60126, juri 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes 2022-12-20 23:45 ` Dmitry Gutov @ 2022-12-23 0:12 ` Sean Whitton 2022-12-23 3:59 ` Sean Whitton 2022-12-23 22:55 ` Dmitry Gutov 0 siblings, 2 replies; 26+ messages in thread From: Sean Whitton @ 2022-12-23 0:12 UTC (permalink / raw) To: Dmitry Gutov; +Cc: 60126, juri 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes 2022-12-23 0:12 ` Sean Whitton @ 2022-12-23 3:59 ` Sean Whitton 2022-12-23 8:16 ` Eli Zaretskii 2022-12-23 23:18 ` Dmitry Gutov 2022-12-23 22:55 ` Dmitry Gutov 1 sibling, 2 replies; 26+ messages in thread From: Sean Whitton @ 2022-12-23 3:59 UTC (permalink / raw) To: Dmitry Gutov; +Cc: 60126, juri [-- 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes 2022-12-23 3:59 ` Sean Whitton @ 2022-12-23 8:16 ` Eli Zaretskii 2022-12-24 2:03 ` Sean Whitton 2022-12-23 23:18 ` Dmitry Gutov 1 sibling, 1 reply; 26+ messages in thread From: Eli Zaretskii @ 2022-12-23 8:16 UTC (permalink / raw) To: Sean Whitton; +Cc: juri, 60126, dgutov > 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes 2022-12-23 8:16 ` Eli Zaretskii @ 2022-12-24 2:03 ` Sean Whitton 0 siblings, 0 replies; 26+ messages in thread From: Sean Whitton @ 2022-12-24 2:03 UTC (permalink / raw) To: Eli Zaretskii; +Cc: juri, 60126, dgutov 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes 2022-12-23 3:59 ` Sean Whitton 2022-12-23 8:16 ` Eli Zaretskii @ 2022-12-23 23:18 ` Dmitry Gutov 2022-12-24 2:02 ` Sean Whitton 1 sibling, 1 reply; 26+ messages in thread From: Dmitry Gutov @ 2022-12-23 23:18 UTC (permalink / raw) To: Sean Whitton; +Cc: 60126, juri 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes 2022-12-23 23:18 ` Dmitry Gutov @ 2022-12-24 2:02 ` Sean Whitton 2022-12-24 14:50 ` Dmitry Gutov 0 siblings, 1 reply; 26+ messages in thread From: Sean Whitton @ 2022-12-24 2:02 UTC (permalink / raw) To: Dmitry Gutov; +Cc: 60126-done, juri 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes 2022-12-24 2:02 ` Sean Whitton @ 2022-12-24 14:50 ` Dmitry Gutov 2022-12-24 18:22 ` Sean Whitton 0 siblings, 1 reply; 26+ messages in thread From: Dmitry Gutov @ 2022-12-24 14:50 UTC (permalink / raw) To: Sean Whitton; +Cc: 60126-done, juri 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes 2022-12-24 14:50 ` Dmitry Gutov @ 2022-12-24 18:22 ` Sean Whitton 2022-12-24 19:26 ` Dmitry Gutov 0 siblings, 1 reply; 26+ messages in thread From: Sean Whitton @ 2022-12-24 18:22 UTC (permalink / raw) To: Dmitry Gutov; +Cc: 60126-done, juri 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes 2022-12-24 18:22 ` Sean Whitton @ 2022-12-24 19:26 ` Dmitry Gutov 2022-12-24 20:10 ` Sean Whitton 0 siblings, 1 reply; 26+ messages in thread From: Dmitry Gutov @ 2022-12-24 19:26 UTC (permalink / raw) To: Sean Whitton; +Cc: 60126-done, juri 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes 2022-12-24 19:26 ` Dmitry Gutov @ 2022-12-24 20:10 ` Sean Whitton 0 siblings, 0 replies; 26+ messages in thread From: Sean Whitton @ 2022-12-24 20:10 UTC (permalink / raw) To: Dmitry Gutov; +Cc: 60126-done, juri 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes 2022-12-23 0:12 ` Sean Whitton 2022-12-23 3:59 ` Sean Whitton @ 2022-12-23 22:55 ` Dmitry Gutov 1 sibling, 0 replies; 26+ messages in thread From: Dmitry Gutov @ 2022-12-23 22:55 UTC (permalink / raw) To: Sean Whitton; +Cc: 60126, juri 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'. ^ permalink raw reply [flat|nested] 26+ messages in thread
* bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes 2022-12-20 15:13 ` Dmitry Gutov 2022-12-20 17:04 ` Sean Whitton @ 2022-12-20 17:13 ` Juri Linkov 1 sibling, 0 replies; 26+ messages in thread From: Juri Linkov @ 2022-12-20 17:13 UTC (permalink / raw) To: Dmitry Gutov; +Cc: 60126, Sean Whitton > (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. ^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2022-12-24 20:10 UTC | newest] Thread overview: 26+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-12-16 18:32 bug#60126: 30.0.50; vc-git-checkin: Offer to unstage conflicting changes Sean Whitton 2022-12-17 17:06 ` Juri Linkov 2022-12-18 0:20 ` Sean Whitton 2022-12-18 1:08 ` Dmitry Gutov 2022-12-19 22:30 ` Sean Whitton 2022-12-20 0:53 ` Dmitry Gutov 2022-12-20 6:43 ` Sean Whitton 2022-12-20 13:47 ` Dmitry Gutov 2022-12-20 16:47 ` Sean Whitton 2022-12-20 15:13 ` Dmitry Gutov 2022-12-20 17:04 ` Sean Whitton 2022-12-20 23:10 ` Sean Whitton 2022-12-20 23:41 ` Sean Whitton 2022-12-20 23:45 ` Dmitry Gutov 2022-12-23 0:12 ` Sean Whitton 2022-12-23 3:59 ` Sean Whitton 2022-12-23 8:16 ` Eli Zaretskii 2022-12-24 2:03 ` Sean Whitton 2022-12-23 23:18 ` Dmitry Gutov 2022-12-24 2:02 ` Sean Whitton 2022-12-24 14:50 ` Dmitry Gutov 2022-12-24 18:22 ` Sean Whitton 2022-12-24 19:26 ` Dmitry Gutov 2022-12-24 20:10 ` Sean Whitton 2022-12-23 22:55 ` Dmitry Gutov 2022-12-20 17:13 ` Juri Linkov
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).