[v3,6/7] rebase --continue: refuse to commit after failed command

Commit Message

Phillip Wood Aug. 1, 2023, 3:23 p.m. UTC

From: Phillip Wood <[e-mail]>

If a commit cannot be picked because it would overwrite an untracked
file then "git rebase --continue" should refuse to commit any staged
changes as the commit was not picked. This is implemented by refusing to
commit if the message file is missing. The message file is chosen for
this check because it is only written when "git rebase" stops for the
user to resolve merge conflicts.

Existing commands that refuse to commit staged changes when continuing
such as a failed "exec" rely on checking for the absence of the author
script in run_git_commit(). This prevents the staged changes from being
committed but prints

    error: could not open '.git/rebase-merge/author-script' for
    reading

before the message about not being able to commit. This is confusing to
users and so checking for the message file instead improves the user
experience. The existing test for refusing to commit after a failed exec
is updated to check that we do not print the error message about a
missing author script anymore.

Signed-off-by: Phillip Wood <[e-mail]>
---
 sequencer.c                   |  5 +++++
 t/t3404-rebase-interactive.sh | 18 +++++++++++++++++-
 t/t3430-rebase-merges.sh      |  4 ++++
 3 files changed, 26 insertions(+), 1 deletion(-)

Comments

Johannes Schindelin Aug. 23, 2023, 9:01 a.m. UTC | #1

Hi Phillip,

On Tue, 1 Aug 2023, Phillip Wood via GitGitGadget wrote:

> From: Phillip Wood <[e-mail]>
>
> If a commit cannot be picked because it would overwrite an untracked
> file then "git rebase --continue" should refuse to commit any staged
> changes as the commit was not picked. This is implemented by refusing to
> commit if the message file is missing. The message file is chosen for
> this check because it is only written when "git rebase" stops for the
> user to resolve merge conflicts.
>
> Existing commands that refuse to commit staged changes when continuing
> such as a failed "exec" rely on checking for the absence of the author
> script in run_git_commit(). This prevents the staged changes from being
> committed but prints
>
>     error: could not open '.git/rebase-merge/author-script' for
>     reading
>
> before the message about not being able to commit. This is confusing to
> users and so checking for the message file instead improves the user
> experience. The existing test for refusing to commit after a failed exec
> is updated to check that we do not print the error message about a
> missing author script anymore.

I am delighted to see an improvement of the user experience!

However, I could imagine that users would still be confused when seeing
the advice about staged changes, even if nothing was staged at all.

Could you introduce a new advice message specifically for the case where
untracked files are in the way and prevent changes from being applied?

Thank you,
Johannes

P.S.: To save both you and me time, here is my ACK for patch 7/7
(actually, the entire patch series, but _maybe_ you want to change
"impove" -> "improve" in the cover letter's subject) ;-)

>
> Signed-off-by: Phillip Wood <[e-mail]>
> ---
>  sequencer.c                   |  5 +++++
>  t/t3404-rebase-interactive.sh | 18 +++++++++++++++++-
>  t/t3430-rebase-merges.sh      |  4 ++++
>  3 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index e25abfd2fb4..a90b015e79c 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4977,6 +4977,11 @@ static int commit_staged_changes(struct repository *r,
>
>  	is_clean = !has_uncommitted_changes(r, 0);
>
> +	if (!is_clean && !file_exists(rebase_path_message())) {
> +		const char *gpg_opt = gpg_sign_opt_quoted(opts);
> +
> +		return error(_(staged_changes_advice), gpg_opt, gpg_opt);
> +	}
>  	if (file_exists(rebase_path_amend())) {
>  		struct strbuf rev = STRBUF_INIT;
>  		struct object_id head, to_amend;
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 6d3788c588b..a8ad398956a 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -604,7 +604,8 @@ test_expect_success 'clean error after failed "exec"' '
>  	echo "edited again" > file7 &&
>  	git add file7 &&
>  	test_must_fail git rebase --continue 2>error &&
> -	test_i18ngrep "you have staged changes in your working tree" error
> +	test_i18ngrep "you have staged changes in your working tree" error &&
> +	test_i18ngrep ! "could not open.*for reading" error
>  '
>
>  test_expect_success 'rebase a detached HEAD' '
> @@ -1290,6 +1291,11 @@ test_expect_success 'rebase -i commits that overwrite untracked files (pick)' '
>  	test_cmp_rev REBASE_HEAD I &&
>  	rm file6 &&
>  	test_path_is_missing .git/rebase-merge/patch &&
> +	echo changed >file1 &&
> +	git add file1 &&
> +	test_must_fail git rebase --continue 2>err &&
> +	grep "error: you have staged changes in your working tree" err &&
> +	git reset --hard HEAD &&
>  	git rebase --continue &&
>  	test_cmp_rev HEAD I
>  '
> @@ -1310,6 +1316,11 @@ test_expect_success 'rebase -i commits that overwrite untracked files (squash)'
>  	test_cmp_rev REBASE_HEAD I &&
>  	rm file6 &&
>  	test_path_is_missing .git/rebase-merge/patch &&
> +	echo changed >file1 &&
> +	git add file1 &&
> +	test_must_fail git rebase --continue 2>err &&
> +	grep "error: you have staged changes in your working tree" err &&
> +	git reset --hard HEAD &&
>  	git rebase --continue &&
>  	test $(git cat-file commit HEAD | sed -ne \$p) = I &&
>  	git reset --hard original-branch2
> @@ -1330,6 +1341,11 @@ test_expect_success 'rebase -i commits that overwrite untracked files (no ff)' '
>  	test_cmp_rev REBASE_HEAD I &&
>  	rm file6 &&
>  	test_path_is_missing .git/rebase-merge/patch &&
> +	echo changed >file1 &&
> +	git add file1 &&
> +	test_must_fail git rebase --continue 2>err &&
> +	grep "error: you have staged changes in your working tree" err &&
> +	git reset --hard HEAD &&
>  	git rebase --continue &&
>  	test $(git cat-file commit HEAD | sed -ne \$p) = I
>  '
> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
> index 4938ebb1c17..804ff819782 100755
> --- a/t/t3430-rebase-merges.sh
> +++ b/t/t3430-rebase-merges.sh
> @@ -169,6 +169,10 @@ test_expect_success 'failed `merge -C` writes patch (may be rescheduled, too)' '
>  	grep "^merge -C .* G$" .git/rebase-merge/done &&
>  	grep "^merge -C .* G$" .git/rebase-merge/git-rebase-todo &&
>  	test_path_is_missing .git/rebase-merge/patch &&
> +	echo changed >file1 &&
> +	git add file1 &&
> +	test_must_fail git rebase --continue 2>err &&
> +	grep "error: you have staged changes in your working tree" err &&
>
>  	: fail because of merge conflict &&
>  	git reset --hard conflicting-G &&
> --
> gitgitgadget
>
>

Phillip Wood Sept. 4, 2023, 2:37 p.m. UTC | #2

Hi Dscho

On 23/08/2023 10:01, Johannes Schindelin wrote:
> Hi Phillip,
> 
> On Tue, 1 Aug 2023, Phillip Wood via GitGitGadget wrote:
> 
>> From: Phillip Wood <[e-mail]>
>>
>> If a commit cannot be picked because it would overwrite an untracked
>> file then "git rebase --continue" should refuse to commit any staged
>> changes as the commit was not picked. This is implemented by refusing to
>> commit if the message file is missing. The message file is chosen for
>> this check because it is only written when "git rebase" stops for the
>> user to resolve merge conflicts.
>>
>> Existing commands that refuse to commit staged changes when continuing
>> such as a failed "exec" rely on checking for the absence of the author
>> script in run_git_commit(). This prevents the staged changes from being
>> committed but prints
>>
>>      error: could not open '.git/rebase-merge/author-script' for
>>      reading
>>
>> before the message about not being able to commit. This is confusing to
>> users and so checking for the message file instead improves the user
>> experience. The existing test for refusing to commit after a failed exec
>> is updated to check that we do not print the error message about a
>> missing author script anymore.
> 
> I am delighted to see an improvement of the user experience!
> 
> However, I could imagine that users would still be confused when seeing
> the advice about staged changes, even if nothing was staged at all.

If nothing is staged then this message wont trigger because is_clean 
will be false.

> Could you introduce a new advice message specifically for the case where
> untracked files are in the way and prevent changes from being applied?

We have an advice message now that is printed when the rebase stops in 
that case. The message here is printed when the user runs "rebase 
--continue" with staged changes and we're not expecting to commit 
anything because the commit couldn't be picked or we're containing from 
a break command or bad exec/label/reset etc.


> P.S.: To save both you and me time, here is my ACK for patch 7/7
> (actually, the entire patch series, but _maybe_ you want to change
> "impove" -> "improve" in the cover letter's subject) ;-)

Thanks for taking the time to read through the patches and for you 
comments. I'll fix the typo when I re-roll

Best Wishes

Phillip

>>
>> Signed-off-by: Phillip Wood <[e-mail]>
>> ---
>>   sequencer.c                   |  5 +++++
>>   t/t3404-rebase-interactive.sh | 18 +++++++++++++++++-
>>   t/t3430-rebase-merges.sh      |  4 ++++
>>   3 files changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/sequencer.c b/sequencer.c
>> index e25abfd2fb4..a90b015e79c 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -4977,6 +4977,11 @@ static int commit_staged_changes(struct repository *r,
>>
>>   	is_clean = !has_uncommitted_changes(r, 0);
>>
>> +	if (!is_clean && !file_exists(rebase_path_message())) {
>> +		const char *gpg_opt = gpg_sign_opt_quoted(opts);
>> +
>> +		return error(_(staged_changes_advice), gpg_opt, gpg_opt);
>> +	}
>>   	if (file_exists(rebase_path_amend())) {
>>   		struct strbuf rev = STRBUF_INIT;
>>   		struct object_id head, to_amend;
>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
>> index 6d3788c588b..a8ad398956a 100755
>> --- a/t/t3404-rebase-interactive.sh
>> +++ b/t/t3404-rebase-interactive.sh
>> @@ -604,7 +604,8 @@ test_expect_success 'clean error after failed "exec"' '
>>   	echo "edited again" > file7 &&
>>   	git add file7 &&
>>   	test_must_fail git rebase --continue 2>error &&
>> -	test_i18ngrep "you have staged changes in your working tree" error
>> +	test_i18ngrep "you have staged changes in your working tree" error &&
>> +	test_i18ngrep ! "could not open.*for reading" error
>>   '
>>
>>   test_expect_success 'rebase a detached HEAD' '
>> @@ -1290,6 +1291,11 @@ test_expect_success 'rebase -i commits that overwrite untracked files (pick)' '
>>   	test_cmp_rev REBASE_HEAD I &&
>>   	rm file6 &&
>>   	test_path_is_missing .git/rebase-merge/patch &&
>> +	echo changed >file1 &&
>> +	git add file1 &&
>> +	test_must_fail git rebase --continue 2>err &&
>> +	grep "error: you have staged changes in your working tree" err &&
>> +	git reset --hard HEAD &&
>>   	git rebase --continue &&
>>   	test_cmp_rev HEAD I
>>   '
>> @@ -1310,6 +1316,11 @@ test_expect_success 'rebase -i commits that overwrite untracked files (squash)'
>>   	test_cmp_rev REBASE_HEAD I &&
>>   	rm file6 &&
>>   	test_path_is_missing .git/rebase-merge/patch &&
>> +	echo changed >file1 &&
>> +	git add file1 &&
>> +	test_must_fail git rebase --continue 2>err &&
>> +	grep "error: you have staged changes in your working tree" err &&
>> +	git reset --hard HEAD &&
>>   	git rebase --continue &&
>>   	test $(git cat-file commit HEAD | sed -ne \$p) = I &&
>>   	git reset --hard original-branch2
>> @@ -1330,6 +1341,11 @@ test_expect_success 'rebase -i commits that overwrite untracked files (no ff)' '
>>   	test_cmp_rev REBASE_HEAD I &&
>>   	rm file6 &&
>>   	test_path_is_missing .git/rebase-merge/patch &&
>> +	echo changed >file1 &&
>> +	git add file1 &&
>> +	test_must_fail git rebase --continue 2>err &&
>> +	grep "error: you have staged changes in your working tree" err &&
>> +	git reset --hard HEAD &&
>>   	git rebase --continue &&
>>   	test $(git cat-file commit HEAD | sed -ne \$p) = I
>>   '
>> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
>> index 4938ebb1c17..804ff819782 100755
>> --- a/t/t3430-rebase-merges.sh
>> +++ b/t/t3430-rebase-merges.sh
>> @@ -169,6 +169,10 @@ test_expect_success 'failed `merge -C` writes patch (may be rescheduled, too)' '
>>   	grep "^merge -C .* G$" .git/rebase-merge/done &&
>>   	grep "^merge -C .* G$" .git/rebase-merge/git-rebase-todo &&
>>   	test_path_is_missing .git/rebase-merge/patch &&
>> +	echo changed >file1 &&
>> +	git add file1 &&
>> +	test_must_fail git rebase --continue 2>err &&
>> +	grep "error: you have staged changes in your working tree" err &&
>>
>>   	: fail because of merge conflict &&
>>   	git reset --hard conflicting-G &&
>> --
>> gitgitgadget
>>
>>

Johannes Schindelin Sept. 5, 2023, 11:17 a.m. UTC | #3

Hi Phillip,

On Mon, 4 Sep 2023, Phillip Wood wrote:

> On 23/08/2023 10:01, Johannes Schindelin wrote:
>
> > On Tue, 1 Aug 2023, Phillip Wood via GitGitGadget wrote:
> >
> > > From: Phillip Wood <[e-mail]>
> > >
> > > If a commit cannot be picked because it would overwrite an untracked
> > > file then "git rebase --continue" should refuse to commit any staged
> > > changes as the commit was not picked. This is implemented by refusing to
> > > commit if the message file is missing. The message file is chosen for
> > > this check because it is only written when "git rebase" stops for the
> > > user to resolve merge conflicts.
> > >
> > > Existing commands that refuse to commit staged changes when continuing
> > > such as a failed "exec" rely on checking for the absence of the author
> > > script in run_git_commit(). This prevents the staged changes from being
> > > committed but prints
> > >
> > >      error: could not open '.git/rebase-merge/author-script' for
> > >      reading
> > >
> > > before the message about not being able to commit. This is confusing to
> > > users and so checking for the message file instead improves the user
> > > experience. The existing test for refusing to commit after a failed exec
> > > is updated to check that we do not print the error message about a
> > > missing author script anymore.
> >
> > I am delighted to see an improvement of the user experience!
> >
> > However, I could imagine that users would still be confused when seeing
> > the advice about staged changes, even if nothing was staged at all.
>
> If nothing is staged then this message wont trigger because is_clean will be
> false.

Ah. I managed to get confused by the first sentence of the commit message
already. You clearly talk about "any staged changes". As in "*iff* there
are any staged changes". Which I missed.

A further contributing factor for my misunderstading was the slightly
convoluted logic where `is_clean` is set to true if there are _not_ any
uncommitted changes, and then we ask if `is_clean` is _not_ true. Reminds
me of Smullyan's Knights & Knaves [*1*].

With your patch, there are now four users of the `is_clean` value, and
all but one of them ask for the negated value.

It's not really the responsibility of this patch series, but I could
imagine that it would be nicer to future readers if a patch was added that
would invert the meaning of that variable and rename it to
`needs_committing`. At least to me, that would make the intention of the
code eminently clearer.

Ciao,
Johannes

Footnote *1*: https://en.wikipedia.org/wiki/Knights_and_Knaves

Junio C Hamano Sept. 5, 2023, 2:57 p.m. UTC | #4

Johannes Schindelin <[e-mail]> writes:

> With your patch, there are now four users of the `is_clean` value, and
> all but one of them ask for the negated value.

Excellent observation.  That strongly argues for the flipping of
polarity, i.e. many people want to know "is it unclean/dirty?".  It
is funny that the name of the helper function where the value comes
from, i.e. has_uncommitted_changes(), has the desired polarity.

> It's not really the responsibility of this patch series, but I could
> imagine that it would be nicer to future readers if a patch was added that
> would invert the meaning of that variable and rename it to
> `needs_committing`. At least to me, that would make the intention of the
> code eminently clearer.

While I agree, after reading the code, that it would make it easier
to follow to flip the polarity of the variable, I would advise
against renaming from state based naming (is it dirty?) to action
based naming (must we commit?), *if* the variable is checked to
sometimes see if we has something that we _could_ commit, while some
other times to see if we _must_ commit before we can let the user
proceed.

"Does the index hold some changes to be committed?" is better
question than "Must we commit?" or "Could we commit?" to derive the
name of this variable from if that is the case, I would think.

Phillip Wood Sept. 5, 2023, 3:25 p.m. UTC | #5

Hi Johannes

On 05/09/2023 12:17, Johannes Schindelin wrote:
> A further contributing factor for my misunderstading was the slightly
> convoluted logic where `is_clean` is set to true if there are _not_ any
> uncommitted changes, and then we ask if `is_clean` is _not_ true. Reminds
> me of Smullyan's Knights & Knaves [*1*].

I agree 'is_clean' is confusing (I have the same problem with 
merge_recursive() and friends where a return value of zero means that 
there were conflicts)

> With your patch, there are now four users of the `is_clean` value, and
> all but one of them ask for the negated value.
> 
> It's not really the responsibility of this patch series, but I could
> imagine that it would be nicer to future readers if a patch was added that
> would invert the meaning of that variable and rename it to
> `needs_committing`. At least to me, that would make the intention of the
> code eminently clearer.

Inverting and renaming 'is_clean' is a good idea, I might leave it to a 
follow up series though.

Best Wishes

Phillip

> Ciao,
> Johannes
> 
> Footnote *1*: https://en.wikipedia.org/wiki/Knights_and_Knaves

Patch

diff --git a/sequencer.c b/sequencer.c
index e25abfd2fb4..a90b015e79c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4977,6 +4977,11 @@  static int commit_staged_changes(struct repository *r,
 
 	is_clean = !has_uncommitted_changes(r, 0);
 
+	if (!is_clean && !file_exists(rebase_path_message())) {
+		const char *gpg_opt = gpg_sign_opt_quoted(opts);
+
+		return error(_(staged_changes_advice), gpg_opt, gpg_opt);
+	}
 	if (file_exists(rebase_path_amend())) {
 		struct strbuf rev = STRBUF_INIT;
 		struct object_id head, to_amend;
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 6d3788c588b..a8ad398956a 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -604,7 +604,8 @@  test_expect_success 'clean error after failed "exec"' '
 	echo "edited again" > file7 &&
 	git add file7 &&
 	test_must_fail git rebase --continue 2>error &&
-	test_i18ngrep "you have staged changes in your working tree" error
+	test_i18ngrep "you have staged changes in your working tree" error &&
+	test_i18ngrep ! "could not open.*for reading" error
 '
 
 test_expect_success 'rebase a detached HEAD' '
@@ -1290,6 +1291,11 @@  test_expect_success 'rebase -i commits that overwrite untracked files (pick)' '
 	test_cmp_rev REBASE_HEAD I &&
 	rm file6 &&
 	test_path_is_missing .git/rebase-merge/patch &&
+	echo changed >file1 &&
+	git add file1 &&
+	test_must_fail git rebase --continue 2>err &&
+	grep "error: you have staged changes in your working tree" err &&
+	git reset --hard HEAD &&
 	git rebase --continue &&
 	test_cmp_rev HEAD I
 '
@@ -1310,6 +1316,11 @@  test_expect_success 'rebase -i commits that overwrite untracked files (squash)'
 	test_cmp_rev REBASE_HEAD I &&
 	rm file6 &&
 	test_path_is_missing .git/rebase-merge/patch &&
+	echo changed >file1 &&
+	git add file1 &&
+	test_must_fail git rebase --continue 2>err &&
+	grep "error: you have staged changes in your working tree" err &&
+	git reset --hard HEAD &&
 	git rebase --continue &&
 	test $(git cat-file commit HEAD | sed -ne \$p) = I &&
 	git reset --hard original-branch2
@@ -1330,6 +1341,11 @@  test_expect_success 'rebase -i commits that overwrite untracked files (no ff)' '
 	test_cmp_rev REBASE_HEAD I &&
 	rm file6 &&
 	test_path_is_missing .git/rebase-merge/patch &&
+	echo changed >file1 &&
+	git add file1 &&
+	test_must_fail git rebase --continue 2>err &&
+	grep "error: you have staged changes in your working tree" err &&
+	git reset --hard HEAD &&
 	git rebase --continue &&
 	test $(git cat-file commit HEAD | sed -ne \$p) = I
 '
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 4938ebb1c17..804ff819782 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -169,6 +169,10 @@  test_expect_success 'failed `merge -C` writes patch (may be rescheduled, too)' '
 	grep "^merge -C .* G$" .git/rebase-merge/done &&
 	grep "^merge -C .* G$" .git/rebase-merge/git-rebase-todo &&
 	test_path_is_missing .git/rebase-merge/patch &&
+	echo changed >file1 &&
+	git add file1 &&
+	test_must_fail git rebase --continue 2>err &&
+	grep "error: you have staged changes in your working tree" err &&
 
 	: fail because of merge conflict &&
 	git reset --hard conflicting-G &&