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

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

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

So if this fails, a non-amending commit is done.  Agreed. :)

Regards,
  Stephan

-- 
Stephan Beyer [off-list ref], PGP 0x6EDDD207FCC5040F

On 7/22/08, Johannes Schindelin [off-list ref] 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

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

On 7/23/08, Dmitry Potapov [off-list ref] 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

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

On 7/23/08, Johannes Schindelin [off-list ref] 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