threads / patch / 14600

patchrebase -i: only automatically amend commit if HEAD did not change

Subject: [PATCH] rebase -i: only automatically amend commit if HEAD did not change

## tl;dr

14 messages between Jul 22, 2008 and Jul 25, 2008. Diffs are folded; open one to read it.

replies: 13people: 5as markdown or json

Johannes Schindelin· Jul 22, 2008, 21:36 UTC · lore

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 <johannes.schindelin@gmx.de>
---
 git-rebase--interactive.sh |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)
Show changes to git-rebase--interactive.sh +4 −2
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· Jul 22, 2008, 21:48 UTC · re: Johannes Schindelin · lore

Re: [PATCH] rebase -i: only automatically amend commit if HEAD did not change

Hi,
Johannes Schindelin wrote:
Show 6 quoted lines
> 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. ;-)
Show 11 quoted lines
> @@ -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 <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F
Avery Pennarun· Jul 22, 2008, 22:22 UTC · re: Johannes Schindelin · lore

Re: [PATCH] rebase -i: only automatically amend commit if HEAD did not change

On 7/22/08, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
Show 6 quoted lines
>  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· Jul 22, 2008, 23:55 UTC · re: Avery Pennarun · lore

Re: [PATCH] rebase -i: only automatically amend commit if HEAD did not change

"Avery Pennarun" <apenwarr@gmail.com> writes:
Show 16 quoted lines
> On 7/22/08, Johannes Schindelin <Johannes.Schindelin@gmx.de> 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?

Avery Pennarun· Jul 23, 2008, 15:55 UTC · re: Junio C Hamano · lore

Re: [PATCH] rebase -i: only automatically amend commit if HEAD did not change

On 7/22/08, Junio C Hamano <gitster@pobox.com> wrote:
Show 9 quoted lines
> "Avery Pennarun" <apenwarr@gmail.com> 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
Dmitry Potapov· Jul 23, 2008, 12:01 UTC · re: Avery Pennarun · lore

Re: [PATCH] rebase -i: only automatically amend commit if HEAD did not change

On Tue, Jul 22, 2008 at 06:22:34PM -0400, Avery Pennarun wrote:
Show 8 quoted lines
> 
> 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.

Show 7 quoted lines
> 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· Jul 23, 2008, 15:53 UTC · re: Dmitry Potapov · lore

Re: [PATCH] rebase -i: only automatically amend commit if HEAD did not change

On 7/23/08, Dmitry Potapov <dpotapov@gmail.com> wrote:
Show 12 quoted lines
> 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
Johannes Schindelin· Jul 23, 2008, 16:09 UTC · re: Avery Pennarun · lore

Re: [PATCH] rebase -i: only automatically amend commit if HEAD did not change

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· Jul 23, 2008, 16:19 UTC · re: Johannes Schindelin · lore

Re: [PATCH] rebase -i: only automatically amend commit if HEAD did not change

On 7/23/08, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
Show 6 quoted lines
>  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· Jul 23, 2008, 23:41 UTC · re: Johannes Schindelin · lore

Re: [PATCH] rebase -i: only automatically amend commit if HEAD did not change

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 6 quoted lines
> 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?

Show 11 quoted lines
> @@ -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· Jul 24, 2008, 12:20 UTC · re: Junio C Hamano · lore

Re: [PATCH] rebase -i: only automatically amend commit if HEAD did not change

Hi,
On Wed, 23 Jul 2008, Junio C Hamano wrote:
Show 11 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> 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
	<mark one commit as edit>
	<Whoa! While editing, I realize that this contains an unrelated 
	 bugfix>

$ git reset HEAD^ $ git add -p $ git commit

	<Edit a bit here, a bit there>
$ git rebase --continue
Sure it is a pilot error.  It hit this pilot, too.
Show 13 quoted lines
> > @@ -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· Jul 24, 2008, 12:35 UTC · re: Johannes Schindelin · lore

Re: [PATCH] rebase -i: only automatically amend commit if HEAD did not change

Hi,
Johannes Schindelin wrote:
Show 7 quoted lines
> 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 <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F
Junio C Hamano· Jul 25, 2008, 08:44 UTC · re: Johannes Schindelin · lore

Re: [PATCH] rebase -i: only automatically amend commit if HEAD did not change

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 22 quoted lines
>> At what point in what valid workflow sequence does HEAD become different 
>> from dotest/amend?
>
> $ rebase -i HEAD~5
>
> 	<mark one commit as edit>
>
> 	<Whoa! While editing, I realize that this contains an unrelated 
> 	 bugfix>
>
> $ git reset HEAD^
> $ git add -p
> $ git commit
>
> 	<Edit a bit here, a bit there>
>
> $ 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· Jul 25, 2008, 10:35 UTC · re: Junio C Hamano · lore

Re: [PATCH] rebase -i: only automatically amend commit if HEAD did not change

Hi,
On Fri, 25 Jul 2008, Junio C Hamano wrote:
Show 28 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> At what point in what valid workflow sequence does HEAD become 
> >> different from dotest/amend?
> >
> > $ rebase -i HEAD~5
> >
> > 	<mark one commit as edit>
> >
> > 	<Whoa! While editing, I realize that this contains an unrelated 
> > 	 bugfix>
> >
> > $ git reset HEAD^
> > $ git add -p
> > $ git commit
> >
> > 	<Edit a bit here, a bit there>
> >
> > $ 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.

Show 15 quoted lines
> 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}".
Show 20 quoted lines
>         $ 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

← back to recent threads