[1/1] sequencer: fix empty commit check when amending

Commit Message

Johannes Schindelin via GitGitGadget Nov. 21, 2019, 2:06 p.m. UTC

From: Phillip Wood <[e-mail]>

This fixes a regression introduced in 356ee4659b ("sequencer: try to
commit without forking 'git commit'", 2017-11-24). When amending a
commit try_to_commit() was using the wrong parent when checking if the
commit would be empty. When amending we need to check against HEAD^ not
HEAD.

t3403 may not seem like the natural home for the new tests but a further
patch series will improve the advice printed by `git commit`. That
series will mutate these tests to check that the advice includes
suggesting `rebase --skip` to skip the fixup that would empty the
commit.

Signed-off-by: Phillip Wood <[e-mail]>
---
 sequencer.c            | 24 +++++++++++++++++++-----
 t/t3403-rebase-skip.sh | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+), 5 deletions(-)

Comments

Junio C Hamano Nov. 22, 2019, 6:40 a.m. UTC | #1

"Phillip Wood via GitGitGadget" <[e-mail]> writes:

> From: Phillip Wood <[e-mail]>
>
> This fixes a regression introduced in 356ee4659b ("sequencer: try to
> commit without forking 'git commit'", 2017-11-24). When amending a
> commit try_to_commit() was using the wrong parent when checking if the
> commit would be empty. When amending we need to check against HEAD^ not
> HEAD.

Thanks.  That makes sense.

If you compare with HEAD and find there is no difference, that would
mean that the "amend" itself is a no-op (i.e. the user said they
want to make changes, but after all changed their mind)?  It might
be something worth noticing, but certainly not in the same way as
"we were told to skip an empty commit---is this one empty?" gets
treated.

Junio C Hamano Nov. 22, 2019, 6:52 a.m. UTC | #2

"Phillip Wood via GitGitGadget" <[e-mail]> writes:

> +	if (!(flags & ALLOW_EMPTY)) {
> +		struct commit *first_parent = current_head;

I would have used first_parent_tree variable instead here.  That
way, you did not have to have an overlong ternary expression down
there, I expect.

> +		if (flags & AMEND_MSG) {
> +			if (current_head->parents) {
> +				first_parent = current_head->parents->item;
> +				if (repo_parse_commit(r, first_parent)) {
> +					res = error(_("could not parse HEAD commit"));
> +					goto out;
> +				}
> +			} else {
> +				first_parent = NULL;
> +			}
> +		}
> +		if (oideq(first_parent ? get_commit_tree_oid(first_parent) :
> +					 the_hash_algo->empty_tree, &tree)) {

Style.  It often makes the code much easier to read when you strive
to break lines at the boundary of larger syntactic units.  In this
(A ? B : C, D) parameter list, ternary A ? B : C binds much tighter
than the comma after it, so if you are breaking line inside it, you
should break line after the comma, too, i.e.

	oideq(first_parent
	      ? get_commit_tree_oid(first_parent)
	      : the_hash_algo->empty_tree,
	      &tree)

to avoid appearing if C and D have closer relationship than B and C,
which your version gives.

But I hope that it becomes a moot point, if we computed first_parent_tree
inside the new "if (flags & AMEND_MSG)" block to leave this oideq()
just

	if (oideq(first_parent_tree, &tree)) {

Phillip Wood Nov. 22, 2019, 11:01 a.m. UTC | #3

Hi Junio

On 22/11/2019 06:40, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <[e-mail]> writes:
> 
>> From: Phillip Wood <[e-mail]>
>>
>> This fixes a regression introduced in 356ee4659b ("sequencer: try to
>> commit without forking 'git commit'", 2017-11-24). When amending a
>> commit try_to_commit() was using the wrong parent when checking if the
>> commit would be empty. When amending we need to check against HEAD^ not
>> HEAD.
> 
> Thanks.  That makes sense.
> 
> If you compare with HEAD and find there is no difference, that would
> mean that the "amend" itself is a no-op (i.e. the user said they
> want to make changes, but after all changed their mind)?  It might
> be something worth noticing, but certainly not in the same way as
> "we were told to skip an empty commit---is this one empty?" gets
> treated.

Yes I'm trying to decide what to do about fixups and squashes that 
become empty. We'd need to do such a check in the sequencer before 
trying to commit I think.

Another thing to think about for the future is what message we want to 
display when a fixup makes the amended commit become empty. If there is 
another fixup following it then the commit may not end up empty by the 
time we've finished applying all the fixups for that commit. If the user 
runs 'reset HEAD^' after the first fixup we'll end up fixing up the 
wrong commit with the later fixups.

Best Wishes

Phillip

Phillip Wood Nov. 22, 2019, 11:09 a.m. UTC | #4

Hi Junio

(sorry dscho I forgot to CC you on this patch)

On 22/11/2019 06:52, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <[e-mail]> writes:
> 
>> +	if (!(flags & ALLOW_EMPTY)) {
>> +		struct commit *first_parent = current_head;
> 
> I would have used first_parent_tree variable instead here.  That
> way, you did not have to have an overlong ternary expression down
> there, I expect.

But current_head may be NULL so we'd end up with the equivalent of the 
ternary expression up here or an if/else to handle it. I thought it was 
clearer to find the parent we want to use and then get the tree from it.

> 
>> +		if (flags & AMEND_MSG) {
>> +			if (current_head->parents) {
>> +				first_parent = current_head->parents->item;
>> +				if (repo_parse_commit(r, first_parent)) {
>> +					res = error(_("could not parse HEAD commit"));
>> +					goto out;
>> +				}
>> +			} else {
>> +				first_parent = NULL;
>> +			}
>> +		}
>> +		if (oideq(first_parent ? get_commit_tree_oid(first_parent) :
>> +					 the_hash_algo->empty_tree, &tree)) {
> 
> Style.  It often makes the code much easier to read when you strive
> to break lines at the boundary of larger syntactic units.  In this
> (A ? B : C, D) parameter list, ternary A ? B : C binds much tighter
> than the comma after it, so if you are breaking line inside it, you
> should break line after the comma, too, i.e.
> 
> 	oideq(first_parent
> 	      ? get_commit_tree_oid(first_parent)
> 	      : the_hash_algo->empty_tree,
> 	      &tree)

I agree that's a clearer way of writing it. I'll re-roll with that

Best Wishes

Phillip

> to avoid appearing if C and D have closer relationship than B and C,
> which your version gives.
> 
> But I hope that it becomes a moot point, if we computed first_parent_tree
> inside the new "if (flags & AMEND_MSG)" block to leave this oideq()
> just
> 
> 	if (oideq(first_parent_tree, &tree)) {
>

Patch

diff --git a/sequencer.c b/sequencer.c
index da2decbd3a..4c8938ae46 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1351,11 +1351,25 @@  static int try_to_commit(struct repository *r,
 		goto out;
 	}
 
-	if (!(flags & ALLOW_EMPTY) && oideq(current_head ?
-					    get_commit_tree_oid(current_head) :
-					    the_hash_algo->empty_tree, &tree)) {
-		res = 1; /* run 'git commit' to display error message */
-		goto out;
+	if (!(flags & ALLOW_EMPTY)) {
+		struct commit *first_parent = current_head;
+
+		if (flags & AMEND_MSG) {
+			if (current_head->parents) {
+				first_parent = current_head->parents->item;
+				if (repo_parse_commit(r, first_parent)) {
+					res = error(_("could not parse HEAD commit"));
+					goto out;
+				}
+			} else {
+				first_parent = NULL;
+			}
+		}
+		if (oideq(first_parent ? get_commit_tree_oid(first_parent) :
+					 the_hash_algo->empty_tree, &tree)) {
+			res = 1; /* run 'git commit' to display error message */
+			goto out;
+		}
 	}
 
 	if (find_hook("prepare-commit-msg")) {
diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh
index 1f5122b632..ee8a8dba52 100755
--- a/t/t3403-rebase-skip.sh
+++ b/t/t3403-rebase-skip.sh
@@ -7,6 +7,8 @@  test_description='git rebase --merge --skip tests'
 
 . ./test-lib.sh
 
+. "$TEST_DIRECTORY"/lib-rebase.sh
+
 # we assume the default git am -3 --skip strategy is tested independently
 # and always works :)
 
@@ -20,6 +22,13 @@  test_expect_success setup '
 	git commit -a -m "hello world" &&
 	echo goodbye >> hello &&
 	git commit -a -m "goodbye" &&
+	git tag goodbye &&
+
+	git checkout --detach &&
+	git checkout HEAD^ . &&
+	test_tick &&
+	git commit -m reverted-goodbye &&
+	git tag reverted-goodbye &&
 
 	git checkout -f skip-reference &&
 	echo moo > hello &&
@@ -76,4 +85,27 @@  test_expect_success 'moved back to branch correctly' '
 
 test_debug 'gitk --all & sleep 1'
 
+test_expect_success 'fixup that empties commit fails' '
+	test_when_finished "git rebase --abort" &&
+	(
+		set_fake_editor &&
+		test_must_fail env FAKE_LINES="1 fixup 2" git rebase -i \
+			goodbye^ reverted-goodbye
+	)
+'
+
+test_expect_success 'squash that empties commit fails' '
+	test_when_finished "git rebase --abort" &&
+	(
+		set_fake_editor &&
+		test_must_fail env FAKE_LINES="1 squash 2" git rebase -i \
+			goodbye^ reverted-goodbye
+	)
+'
+
+# Must be the last test in this file
+test_expect_success '$EDITOR and friends are unchanged' '
+	test_editor_unchanged
+'
+
 test_done