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

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

[Johannes Schindelin]

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(-)

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

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

[Stephan Beyer]

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 <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F

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

[Avery Pennarun]

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.

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

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

[Junio C Hamano]

"Avery Pennarun" <apenwarr@gmail.com> writes:

> 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?

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

[Avery Pennarun]

On 7/22/08, Junio C Hamano <gitster@pobox.com> wrote:
> "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

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

[Dmitry Potapov]

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

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

[Avery Pennarun]

On 7/23/08, Dmitry Potapov <dpotapov@gmail.com> 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

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

[Johannes Schindelin]

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

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

[Avery Pennarun]

On 7/23/08, Johannes Schindelin <Johannes.Schindelin@gmx.de> 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

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

[Junio C Hamano]

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?

> @@ -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...

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

[Johannes Schindelin]

Hi,

On Wed, 23 Jul 2008, Junio C Hamano wrote:

> 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.

> > @@ -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

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

[Stephan Beyer]

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 <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F

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

[Junio C Hamano]

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)?

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.

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

[Johannes Schindelin]

Hi,

On Fri, 25 Jul 2008, Junio C Hamano wrote:

> 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.

> 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