rebase -i: Should --continue auto-amend after failed exec?

rebase -i: Should --continue auto-amend after failed exec?

[Johannes Sixt]

If after a failed "exec" instruction there are staged changes, then currently rebase
--continue fails with:

.../git-rebase--interactive: line 774: .../.git/rebase-merge/author-script: No such file or directory

But shouldn't this amend the HEAD commit? The documentation is not clear
(from git-rebase.txt):

  The interactive rebase will stop when a command fails (i.e. exits with
  non-0 status) to give you an opportunity to fix the problem. You can
  continue with `git rebase --continue`.

This may be interpreted to work like "edit", and IMO would be a very useful
modus operandi.

Here is a test case.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 47c8371..2146e47 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -527,6 +527,21 @@ test_expect_success 'auto-amend only edited commits after "edit"' '
 	git rebase --abort
 '
 
+test_expect_failure 'auto-amend after failed "exec"' '
+	test_tick &&
+	test_when_finished "git rebase --abort || :" &&
+	(
+		FAKE_LINES="1 exec_false" &&
+		export FAKE_LINES &&
+		test_must_fail git rebase -i HEAD^
+	) &&
+	echo "edited again" > file7 &&
+	git add file7 &&
+	FAKE_COMMIT_MESSAGE="edited file7 again" git rebase --continue &&
+	actual=$(git show HEAD:file7) &&
+	test "edited again" = "$actual"
+'
+
 test_expect_success 'rebase a detached HEAD' '
 	grandparent=$(git rev-parse HEAD~2) &&
 	git checkout $(git rev-parse HEAD) &&

Re: rebase -i: Should --continue auto-amend after failed exec?

[Matthieu Moy]

Johannes Sixt <j6t@kdbg.org> writes:

> If after a failed "exec" instruction there are staged changes, then currently rebase
> --continue fails with:
>
> .../git-rebase--interactive: line 774: .../.git/rebase-merge/author-script: No such file or directory

That's obviously bad, there should at least be an accurate error
message.

> But shouldn't this amend the HEAD commit? The documentation is not clear
> (from git-rebase.txt):
>
>   The interactive rebase will stop when a command fails (i.e. exits with
>   non-0 status) to give you an opportunity to fix the problem. You can
>   continue with `git rebase --continue`.
>
> This may be interpreted to work like "edit", and IMO would be a very useful
> modus operandi.

I'm not sure. What happens in "edit" is that when reaching the "edit"
line, git-rebase--interactive.sh calls die_with_patch, which writes
author information in .git/rebase-merge/author-script, which really
means "I've stopped on this commit, this is the one that we should
implicitely amend with --continue".

The case of "exec" is a bit different: you don't stop "on a commit", but
after doing something else. You can hardly guess whether the staged
changes are meant to amend the existing commit, or to make a new one.

Actually, that could even be

pick deadbeef Existing commit
exec foo > bar.txt; git add bar.txt; git commit -m "added during rebase"
exec false
pick c00ffee Another commit

then auto-amending may be really confusing: should it amend the HEAD
commit that you've just created (this would really go against the logic
of .git/rebase-merge/author-script) or the last picked commit (which you
can't really do since it's not HEAD)?

I think it's best to abort, with an accurate error message pointing the
user to both solutions (commit --amend && rebase --continue or commit &&
rebase --continue). I'll try a patch.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

[PATCH] rebase -i: clean error message for --continue after failed exec

[Matthieu Moy]

If after a failed "exec" instruction there are staged changes, before
this patch, rebase --continue fails with this message, which is obviously
wrong:

.../git-rebase--interactive: line 774: .../.git/rebase-merge/author-script: No such file or directory

We could try auto-amending HEAD, but this goes against the logic of
.git/rebase-merge/author-script (see also the testcase 'auto-amend only
edited commits after "edit"' in t3404-rebase-interactive.sh) to
auto-amend something the user hasn't explicitely asked to edit.

Instead of doing anything automatically, detect the situation and give a
clean error message. While we're there, also clarify the error message in
case '. "$author_script"' fails, which now corresponds to really weird
senario where the author script exists but can't be read.

Test-case-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
Here's my attempt at a patch. After noticing the existance of a test
'auto-amend only edited commits after "edit"', I'm convinced that
auto-amending is not the right thing here.

 git-rebase--interactive.sh    |   18 +++++++++++++++++-
 t/t3404-rebase-interactive.sh |   14 ++++++++++++++
 2 files changed, 31 insertions(+), 1 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index c6ba7c1..5c94506 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -647,8 +647,24 @@ continue)
 	then
 		: Nothing to commit -- skip this
 	else
+		if ! test -f "$author_script"
+		then
+			die "You have staged changes in your working tree. If these changes are meant to be
+squashed into the previous commit, run:
+
+  git commit --amend
+
+If they are meant to go into a new commit, run:
+
+  git commit
+
+In both case, once you're done, continue with:
+
+  git rebase --continue
+"
+		fi
 		. "$author_script" ||
-			die "Cannot find the author identity"
+			die "Error trying to find the author identity to amend commit"
 		current_head=
 		if test -f "$amend"
 		then
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 8538813..b981572 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -527,6 +527,20 @@ test_expect_success 'auto-amend only edited commits after "edit"' '
 	git rebase --abort
 '
 
+test_expect_success 'clean error after failed "exec"' '
+	test_tick &&
+	test_when_finished "git rebase --abort || :" &&
+	(
+		FAKE_LINES="1 exec_false" &&
+		export FAKE_LINES &&
+		test_must_fail git rebase -i HEAD^
+	) &&
+	echo "edited again" > file7 &&
+	git add file7 &&
+	test_must_fail git rebase --continue 2>error &&
+	grep "You have staged changes in your working tree." error
+'
+
 test_expect_success 'rebase a detached HEAD' '
 	grandparent=$(git rev-parse HEAD~2) &&
 	git checkout $(git rev-parse HEAD) &&
-- 
1.7.6.580.gc2862.dirty

Re: [PATCH] rebase -i: clean error message for --continue after failed exec

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> If after a failed "exec" instruction there are staged changes,...

I have to wonder why whatever "exec" runs is mucking with the index in the
first place. Shouldn't we forbid it?

Re: [PATCH] rebase -i: clean error message for --continue after failed exec

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
>
>> If after a failed "exec" instruction there are staged changes,...
>
> I have to wonder why whatever "exec" runs is mucking with the index in the
> first place. Shouldn't we forbid it?

I suspect your patch amounts to the same thing of forbidding, but
detecting the lack of $author_script feels like it is covering up the
symptom and not directly going for the cause of the symptom.

I wonder if doing something like this would be more direct approach to
achieve the same thing.

 git-rebase--interactive.sh |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index c6ba7c1..31026dc 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -472,24 +472,31 @@ do_next () {
 		git rev-parse --verify HEAD > "$state_dir"/stopped-sha
 		${SHELL:-@SHELL_PATH@} -c "$rest" # Actual execution
 		status=$?
+		# Run in subshell because require_clean_work_tree can die.
+		dirty=f
+		(require_clean_work_tree "rebase") || dirty=t
 		if test "$status" -ne 0
 		then
 			warn "Execution failed: $rest"
+			test "$dirty" = f ||
+			warn "and made changes to the index and/or the working tree"
+
 			warn "You can fix the problem, and then run"
 			warn
 			warn "	git rebase --continue"
 			warn
 			exit "$status"
 		fi
-		# Run in subshell because require_clean_work_tree can die.
-		if ! (require_clean_work_tree "rebase")
-		then
+		if test "$dirty" = t
+			warn "Execution succeeded: $rest"
+			warn "but left changes to the index and/or the working tree"
 			warn "Commit or stash your changes, and then run"
 			warn
 			warn "	git rebase --continue"
 			warn
 			exit 1
 		fi
+
 		;;
 	*)
 		warn "Unknown command: $command $sha1 $rest"

Re: [PATCH] rebase -i: clean error message for --continue after failed exec

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
>>
>>> If after a failed "exec" instruction there are staged changes,...
>>
>> I have to wonder why whatever "exec" runs is mucking with the index in the
>> first place. Shouldn't we forbid it?
>
> I suspect your patch amounts to the same thing of forbidding, but
> detecting the lack of $author_script feels like it is covering up the
> symptom and not directly going for the cause of the symptom.
>
> I wonder if doing something like this would be more direct approach to
> achieve the same thing.

Not the same thing, but both patches could go well together.

Mine covers

  pick deadbeef
  exec make test
  # :-( make test failed, I'm going to fix it
  hack hack hack
  git add changes
  # OK, seems fixed.
  git rebase --continue
  # --> rebase tells me I forgot to commit my fixup patch

i.e. the user changed the index interactively, not within exec. Yours
covers the case where the command itself changes the index.

> +		# Run in subshell because require_clean_work_tree can die.
> +		dirty=f
> +		(require_clean_work_tree "rebase") || dirty=t

This will display error messages like

  Cannot rebase: You have unstaged changes

and right after

>  			warn "Execution failed: $rest"
> +			test "$dirty" = f ||
> +			warn "and made changes to the index and/or the working tree"

which sounds redundant. This should probably be

(require_clean_work_tree "rebase" 2>/dev/null) || dirty=t

but looking more closely at the patch, you're not the one introducing
this, it was already there since 92c62a3f.

>  		fi
> +
>  		;;
>  	*)

I think this one can be removed, there's usually no blank line before ;;
in the code.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH] rebase -i: clean error message for --continue after failed exec

[Johannes Sixt]

Am 25.08.2011 09:09, schrieb Matthieu Moy:
> Junio C Hamano<gitster@pobox.com>  writes:
>> I wonder if doing something like this would be more direct approach to
>> achieve the same thing.
>
> Not the same thing, but both patches could go well together.
>
> Mine covers
>
>    pick deadbeef
>    exec make test
>    # :-( make test failed, I'm going to fix it
>    hack hack hack
>    git add changes
>    # OK, seems fixed.
>    git rebase --continue
>    # -->  rebase tells me I forgot to commit my fixup patch

This is exactly my use-case that discovered the problem and which I would 
like to see fixed, FWIW.

-- Hannes

Re: [PATCH] rebase -i: clean error message for --continue after failed exec

[Jeff King]

On Wed, Aug 24, 2011 at 11:42:27AM -0700, Junio C Hamano wrote:

> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
> 
> > If after a failed "exec" instruction there are staged changes,...
> 
> I have to wonder why whatever "exec" runs is mucking with the index in the
> first place. Shouldn't we forbid it?

Certainly my only user has ever been "exec make test". But I wonder if
somebody is crazy enough to auto-generate some content and commit it.
OTOH, shouldn't it then be their responsibility to make the commit?
I.e., I can see at least the potential for mucking with the index, but I
really don't see a reason for _leaving_ the index in a mucked state.

-Peff

Re: [PATCH] rebase -i: clean error message for --continue after failed exec

[Jeff King]

On Wed, Aug 24, 2011 at 04:20:27PM -0400, Jeff King wrote:

> Certainly my only user has ever been "exec make test". But I wonder if

Er, s/user/use.

> somebody is crazy enough to auto-generate some content and commit it.
> OTOH, shouldn't it then be their responsibility to make the commit?
> I.e., I can see at least the potential for mucking with the index, but I
> really don't see a reason for _leaving_ the index in a mucked state.

Having just read your followup patch, it looks sane. Exec commands are
free to do whatever they like with the index as long as it is left in a
clean state. That keeps the door open for semi-sane use cases, but will
catch unintended index manipulation.

-Peff