# [PATCH] rebase -i: only automatically amend commit if HEAD did not change 14 messages from 2008-07-22 to 2008-07-25. Participants: Johannes Schindelin, Stephan Beyer, Avery Pennarun, Junio C Hamano, Dmitry Potapov. Thread: https://gitlist.dev/t/14600 ## Johannes Schindelin, 2008-07-22 21:36 Subject: [PATCH] rebase -i: only automatically amend commit if HEAD did not change Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0807222235520.8986%40racer ``` If the user called "rebase -i", marked a commit as "edit", "rebase --continue" would automatically amend the commit when there were staged changes. However, this is actively wrong when the current commit is not the one marked with "edit". So guard against this. Signed-off-by: Johannes Schindelin --- git-rebase--interactive.sh | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-) diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index e63a864..444f393 100755 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -276,7 +276,7 @@ do_next () { pick_one $sha1 || die_with_patch $sha1 "Could not apply $sha1... $rest" make_patch $sha1 - : > "$DOTEST"/amend + git rev-parse HEAD > "$DOTEST"/amend warn warn "You can amend the commit now, with" warn @@ -419,7 +419,9 @@ do else . "$DOTEST"/author-script || die "Cannot find the author identity" - if test -f "$DOTEST"/amend + if test -f "$DOTEST"/amend && + test $(git rev-parse HEAD) = \ + $(cat "$DOTEST"/amend) then git reset --soft HEAD^ || die "Cannot rewind the HEAD" -- 1.6.0.rc0.22.gf2096d.dirty ``` ## Stephan Beyer, 2008-07-22 21:48 Subject: Re: [PATCH] rebase -i: only automatically amend commit if HEAD did not change Message-ID: <20080722214858.GC5904@leksak.fem-net> URL: https://gitlist.dev/e/20080722214858.GC5904%40leksak.fem-net In-Reply-To: ``` Hi, Johannes Schindelin wrote: > If the user called "rebase -i", marked a commit as "edit", "rebase > --continue" would automatically amend the commit when there were > staged changes. > > However, this is actively wrong when the current commit is not the > one marked with "edit". So guard against this. Hmm, I like it. ;-) > @@ -419,7 +419,9 @@ do > else > . "$DOTEST"/author-script || > die "Cannot find the author identity" > - if test -f "$DOTEST"/amend > + if test -f "$DOTEST"/amend && > + test $(git rev-parse HEAD) = \ > + $(cat "$DOTEST"/amend) > then > git reset --soft HEAD^ || > die "Cannot rewind the HEAD" So if this fails, a non-amending commit is done. Agreed. :) Regards, Stephan -- Stephan Beyer , PGP 0x6EDDD207FCC5040F ``` ## Avery Pennarun, 2008-07-22 22:22 Subject: Re: [PATCH] rebase -i: only automatically amend commit if HEAD did not change Message-ID: <32541b130807221522r2a43c49cl6400f00dbe7451a0@mail.gmail.com> URL: https://gitlist.dev/e/32541b130807221522r2a43c49cl6400f00dbe7451a0%40mail.gmail.com In-Reply-To: ``` On 7/22/08, Johannes Schindelin wrote: > If the user called "rebase -i", marked a commit as "edit", "rebase > --continue" would automatically amend the commit when there were > staged changes. > > However, this is actively wrong when the current commit is not the > one marked with "edit". So guard against this. This patch is perhaps a symptom of something I've been meaning to ask about for a while. Why doesn't "edit" just stage the commit and not auto-commit it at all? That way an amend would *never* be necessary, and rebase --continue would always do a commit -a (if there was anything left to commit). The special case fixed by this patch would thus not be needed. It would also make it more obvious how to remove files from a commit, for example, without having to learn about "git reset". For that matter, you wouldn't have to learn about "git commit --amend" either, and it would save typing. It would also be a little more consistent with "squash", which already lets you edit the commit message by default. Just a thought. Presumably it was implemented the way it is for some reason, but I haven't seen any discussion about it. Have fun, Avery ``` ## Junio C Hamano, 2008-07-22 23:55 Subject: Re: [PATCH] rebase -i: only automatically amend commit if HEAD did not change Message-ID: <7viquxjwyf.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7viquxjwyf.fsf%40gitster.siamese.dyndns.org In-Reply-To: <32541b130807221522r2a43c49cl6400f00dbe7451a0@mail.gmail.com> ``` "Avery Pennarun" writes: > On 7/22/08, Johannes Schindelin wrote: >> If the user called "rebase -i", marked a commit as "edit", "rebase >> --continue" would automatically amend the commit when there were >> staged changes. >> >> However, this is actively wrong when the current commit is not the >> one marked with "edit". So guard against this. > > This patch is perhaps a symptom of something I've been meaning to ask > about for a while. > > Why doesn't "edit" just stage the commit and not auto-commit it at > all? That way an amend would *never* be necessary, and rebase > --continue would always do a commit -a (if there was anything left to > commit). The special case fixed by this patch would thus not be > needed. That would actually be in-line with the way how "rebase --skip" does the resetting without asking the user to do so, wouldn't it? ``` ## Dmitry Potapov, 2008-07-23 12:01 Subject: Re: [PATCH] rebase -i: only automatically amend commit if HEAD did not change Message-ID: <20080723120104.GQ2925@dpotapov.dyndns.org> URL: https://gitlist.dev/e/20080723120104.GQ2925%40dpotapov.dyndns.org In-Reply-To: <32541b130807221522r2a43c49cl6400f00dbe7451a0@mail.gmail.com> ``` On Tue, Jul 22, 2008 at 06:22:34PM -0400, Avery Pennarun wrote: > > This patch is perhaps a symptom of something I've been meaning to ask > about for a while. > > Why doesn't "edit" just stage the commit and not auto-commit it at > all? That way an amend would *never* be necessary, and rebase > --continue would always do a commit -a (if there was anything left to > commit). Actually, it would be better to refuse to continue if there are unstaged changes in the work tree, and if all changes are staged then just do git commit. > The special case fixed by this patch would thus not be > needed. > > It would also make it more obvious how to remove files from a commit, > for example, without having to learn about "git reset". For that > matter, you wouldn't have to learn about "git commit --amend" either, > and it would save typing. It would not only save typing, but also help to avoid costly mistakes where users, being taught to use "git commit --amend" after editing during git-rebase, fire this command automatically after a conflict resolution and get two commits accidently squashed. So, I completely agree that the current auto-commit behavior is not very user friendly... Dmitry ``` ## Avery Pennarun, 2008-07-23 15:53 Subject: Re: [PATCH] rebase -i: only automatically amend commit if HEAD did not change Message-ID: <32541b130807230853y136f41bdmf221637e35d601c3@mail.gmail.com> URL: https://gitlist.dev/e/32541b130807230853y136f41bdmf221637e35d601c3%40mail.gmail.com In-Reply-To: <20080723120104.GQ2925@dpotapov.dyndns.org> ``` On 7/23/08, Dmitry Potapov wrote: > On Tue, Jul 22, 2008 at 06:22:34PM -0400, Avery Pennarun wrote: > > This patch is perhaps a symptom of something I've been meaning to ask > > about for a while. > > > > Why doesn't "edit" just stage the commit and not auto-commit it at > > all? That way an amend would *never* be necessary, and rebase > > --continue would always do a commit -a (if there was anything left to > > commit). > > Actually, it would be better to refuse to continue if there are unstaged > changes in the work tree, and if all changes are staged then just do git > commit. I'm not sure about that. The auto-committing on --continue has never annoyed me, and in fact I greatly appreciate that I can just "git rebase --continue" after making changes and the expected thing will happen. After all, if I screw it up and commit too much at once, I can always just rebase one more time. However, taking out the auto-commit wouldn't pain me too much if others want it that way. It would be somewhat more typing, but at least makes easy to understand exactly what's going on. > It would not only save typing, but also help to avoid costly mistakes > where users, being taught to use "git commit --amend" after editing > during git-rebase, fire this command automatically after a conflict > resolution and get two commits accidently squashed. Yes! Good point. I forgot about this, but I've been bitten by it a couple of times. Have fun, Avery ``` ## Avery Pennarun, 2008-07-23 15:55 Subject: Re: [PATCH] rebase -i: only automatically amend commit if HEAD did not change Message-ID: <32541b130807230855y19bbedfah38dce9391ff4f481@mail.gmail.com> URL: https://gitlist.dev/e/32541b130807230855y19bbedfah38dce9391ff4f481%40mail.gmail.com In-Reply-To: <7viquxjwyf.fsf@gitster.siamese.dyndns.org> ``` On 7/22/08, Junio C Hamano wrote: > "Avery Pennarun" writes: > > Why doesn't "edit" just stage the commit and not auto-commit it at > > all? That way an amend would *never* be necessary, and rebase > > --continue would always do a commit -a (if there was anything left to > > commit). The special case fixed by this patch would thus not be > > needed. > > That would actually be in-line with the way how "rebase --skip" does the > resetting without asking the user to do so, wouldn't it? I'm not sure exactly what you mean, but I think with my proposed change, "rebase --skip" would do "git reset --hard HEAD" while "rebase --continue" would do "git commit -a", so it would be nice and symmetrical, where currently it isn't exactly. Have fun, Avery ``` ## Johannes Schindelin, 2008-07-23 16:09 Subject: Re: [PATCH] rebase -i: only automatically amend commit if HEAD did not change Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0807231708520.8986%40racer In-Reply-To: <32541b130807230853y136f41bdmf221637e35d601c3@mail.gmail.com> ``` Hi, On Wed, 23 Jul 2008, Avery Pennarun wrote: > However, taking out the auto-commit wouldn't pain me too much if > others want it that way. IMO the -rc0 cycle is a particularly ill-chosen time to discuss behavior changes like this. Hth, Dscho ``` ## Avery Pennarun, 2008-07-23 16:19 Subject: Re: [PATCH] rebase -i: only automatically amend commit if HEAD did not change Message-ID: <32541b130807230919j64634e94ybb61b55fcb27582@mail.gmail.com> URL: https://gitlist.dev/e/32541b130807230919j64634e94ybb61b55fcb27582%40mail.gmail.com In-Reply-To: ``` On 7/23/08, Johannes Schindelin wrote: > On Wed, 23 Jul 2008, Avery Pennarun wrote: > > However, taking out the auto-commit wouldn't pain me too much if > > others want it that way. > > IMO the -rc0 cycle is a particularly ill-chosen time to discuss behavior > changes like this. Probably, but it relates to the discussion of the current patch. Would it be better to change the rebase behaviour now and then possibly again in the next version, or just leave it as-is for now? Avery ``` ## Junio C Hamano, 2008-07-23 23:41 Subject: Re: [PATCH] rebase -i: only automatically amend commit if HEAD did not change Message-ID: <7vmyk888z5.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vmyk888z5.fsf%40gitster.siamese.dyndns.org In-Reply-To: ``` Johannes Schindelin writes: > If the user called "rebase -i", marked a commit as "edit", "rebase > --continue" would automatically amend the commit when there were > staged changes. > > However, this is actively wrong when the current commit is not the > one marked with "edit". So guard against this. At what point in what valid workflow sequence does HEAD become different from dotest/amend? > @@ -419,7 +419,9 @@ do > else > . "$DOTEST"/author-script || > die "Cannot find the author identity" > - if test -f "$DOTEST"/amend > + if test -f "$DOTEST"/amend && > + test $(git rev-parse HEAD) = \ > + $(cat "$DOTEST"/amend) > then > git reset --soft HEAD^ || > die "Cannot rewind the HEAD" In what way is this "guarding against it"? It goes on and makes an unrelated commit without erroring out nor giving indication to the user what is going on, doesn't it? I am sure you meant well and the code is good, but I find that the explanation is a bit lacking... ``` ## Johannes Schindelin, 2008-07-24 12:20 Subject: Re: [PATCH] rebase -i: only automatically amend commit if HEAD did not change Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0807241311450.8986%40racer In-Reply-To: <7vmyk888z5.fsf@gitster.siamese.dyndns.org> ``` Hi, On Wed, 23 Jul 2008, Junio C Hamano wrote: > Johannes Schindelin writes: > > > If the user called "rebase -i", marked a commit as "edit", "rebase > > --continue" would automatically amend the commit when there were > > staged changes. > > > > However, this is actively wrong when the current commit is not the one > > marked with "edit". So guard against this. > > At what point in what valid workflow sequence does HEAD become different > from dotest/amend? $ rebase -i HEAD~5 $ git reset HEAD^ $ git add -p $ git commit $ git rebase --continue Sure it is a pilot error. It hit this pilot, too. > > @@ -419,7 +419,9 @@ do > > else > > . "$DOTEST"/author-script || > > die "Cannot find the author identity" > > - if test -f "$DOTEST"/amend > > + if test -f "$DOTEST"/amend && > > + test $(git rev-parse HEAD) = \ > > + $(cat "$DOTEST"/amend) > > then > > git reset --soft HEAD^ || > > die "Cannot rewind the HEAD" > > In what way is this "guarding against it"? In the way that the user certainly did not mean to amend _this_ HEAD. Another HEAD was marked with "edit". By not amending, the user has a chance to fix anything unintended in another rebase -i more easily, since the two commits have not been squashed together. 'course, the user should have seen in the editor that popped up that she is squashing two different commits, should have deleted the whole commit message to abort, make the independent commit herself (finding the correct commit to steal the commit message from), then run rebase --continue (which would no longer commit anything, there being nothing to). However, that course of action is a bit unintuitive. The way it runs with my patch, at least a user has a chance to fix it up without a Git expert standing nearby. I will definitely keep this in my personal fork, even in my personal fork of "master" during the rc period. But if you think it is not worth it, and others seem to be utterly disinterested (instead discussing behavior changes), I will not push further. Ciao, Dscho ``` ## Stephan Beyer, 2008-07-24 12:35 Subject: Re: [PATCH] rebase -i: only automatically amend commit if HEAD did not change Message-ID: <20080724123556.GA7125@leksak.fem-net> URL: https://gitlist.dev/e/20080724123556.GA7125%40leksak.fem-net In-Reply-To: ``` Hi, Johannes Schindelin wrote: > The way it runs with my patch, at least a user has a chance to fix it up > without a Git expert standing nearby. > > I will definitely keep this in my personal fork, even in my personal > fork of "master" during the rc period. But if you think it is not worth > it, and others seem to be utterly disinterested (instead discussing > behavior changes), I will not push further. I don't know how much my opinion counts here, but I think it definitely is worth it because I've ran into this *several* times -- "Ooops, where is my commit? ... ... ... Ah! F***!", which meant that I had to had to split accidentally squashed commits. Regards, Stephan -- Stephan Beyer , PGP 0x6EDDD207FCC5040F ``` ## Junio C Hamano, 2008-07-25 08:44 Subject: Re: [PATCH] rebase -i: only automatically amend commit if HEAD did not change Message-ID: <7vod4muzck.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vod4muzck.fsf%40gitster.siamese.dyndns.org In-Reply-To: ``` Johannes Schindelin writes: >> At what point in what valid workflow sequence does HEAD become different >> from dotest/amend? > > $ rebase -i HEAD~5 > > > > bugfix> > > $ git reset HEAD^ > $ git add -p > $ git commit > > > > $ git rebase --continue > > Sure it is a pilot error. It hit this pilot, too. > ... > In the way that the user certainly did not mean to amend _this_ HEAD. > Another HEAD was marked with "edit". Ok; after this "refraining from incorrectly squashing them", how would the user edit the one the user originally intended to edit (I am not complaining, but asking for information)? So in your workflow example, when there is no pilot error, is this the "ideal" sequence? $ git rebase -i HEAD~5 .. mark one as edit .. ah, the one I wanted to just "edit" actually need to be .. split into two because it has some other thing I need to change $ git reset HEAD^ $ git add -p $ git stash --keep-index .. test to verify the initial part $ git commit ;# first part of split commit $ git stash pop .. test test $ git add -p $ git rebase --continue ;# gives you the editor to edit I wonder if we can make the transcript of the "pilot error" case look like this: $ git rebase -i HEAD~5 ... $ git reset HEAD^ .. same as above up to... .. test to verify the initial part $ git rebase --continue ;# oops .. gives you the editor to edit the message. .. makes a commit, and says: committed initial part of the change, stopping. .. ah, the command noticed it and did not escape, thanks! $ git stash pop .. test test $ git add -p $ git rebase --continue ;# gives you the editor to edit .. and goes on this time. ``` ## Johannes Schindelin, 2008-07-25 10:35 Subject: Re: [PATCH] rebase -i: only automatically amend commit if HEAD did not change Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0807251231560.11976%40eeepc-johanness In-Reply-To: <7vod4muzck.fsf@gitster.siamese.dyndns.org> ``` Hi, On Fri, 25 Jul 2008, Junio C Hamano wrote: > Johannes Schindelin writes: > > >> At what point in what valid workflow sequence does HEAD become > >> different from dotest/amend? > > > > $ rebase -i HEAD~5 > > > > > > > > > bugfix> > > > > $ git reset HEAD^ > > $ git add -p > > $ git commit > > > > > > > > $ git rebase --continue > > > > Sure it is a pilot error. It hit this pilot, too. > > ... > > In the way that the user certainly did not mean to amend _this_ HEAD. > > Another HEAD was marked with "edit". > > Ok; after this "refraining from incorrectly squashing them", how would the > user edit the one the user originally intended to edit (I am not > complaining, but asking for information)? In this case, the two commits are two commits, the editor popped up with rebase --continue contains the original commit message, and all is fine. > So in your workflow example, when there is no pilot error, is this the > "ideal" sequence? > > $ git rebase -i HEAD~5 > .. mark one as edit > .. ah, the one I wanted to just "edit" actually need to be > .. split into two because it has some other thing I need to change > $ git reset HEAD^ > $ git add -p > $ git stash --keep-index > .. test to verify the initial part > $ git commit ;# first part of split commit > $ git stash pop > .. test test > $ git add -p Without pilot error, here has to come a "git commit -c HEAD@{1}". > $ git rebase --continue ;# gives you the editor to edit > > I wonder if we can make the transcript of the "pilot error" case look like > this: > > $ git rebase -i HEAD~5 > ... > $ git reset HEAD^ > .. same as above up to... > .. test to verify the initial part > $ git rebase --continue ;# oops > .. gives you the editor to edit the message. > .. makes a commit, and says: > committed initial part of the change, stopping. > .. ah, the command noticed it and did not escape, thanks! > $ git stash pop > .. test test > $ git add -p > $ git rebase --continue ;# gives you the editor to edit > .. and goes on this time. Possible. The first hunk (IIRC) of my patch would stay the same, but the second hunk would not fall through to the interactive commit, instead testing if amend exists, _and_ is different from HEAD, and if both are the case, say something helpful and exit. Ciao, Dscho ```