[BUG] fixup commit is dropped during rebase if subject = branch name

[BUG] fixup commit is dropped during rebase if subject = branch name

[Erik Cervin Edin]

Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.

What did you do before the bug happened? (Steps to reproduce your issue)
  dir=rebase-fixup-subject-equals-branch-name
  mkdir $dir
  cd $dir
  git init --initial-branch=main
  git commit -m init --allow-empty
  git tag init

  # failure
  seq 1 3 >> bar && git add bar && git commit -m main
  git tag -f x
  seq 4 6 >> bar && git add bar && git commit -m bar
  seq 7 9 >> bar && git add bar && git commit --fixup :/main
  git -c sequence.editor=: rebase --autosquash --interactive x
  git diff ORIG_HEAD


What did you expect to happen? (Expected behavior)
Identical results as
  # normal
  git reset --hard init
  seq 1 3 >> bar && git add bar && git commit -m main
  git tag -f x
  seq 4 6 >> bar && git add bar && git commit --fixup :/main
  seq 7 9 >> bar && git add bar && git commit -m bar
  git -c sequence.editor=: rebase --autosquash --interactive x
  git diff ORIG_HEAD
except for the order of the commits.

What happened instead? (Actual behavior)
The fixup commit was dropped from the rebase-todo.

What's different between what you expected and what actually happened?
The HEAD is a fixup of a commit with the same subject as the branch
name. It is being rebased on top of the commit it is intended to
fixup, so it should be *picked*. Instead, it is dropped from the
rebase-todo. I expected the autosquash feature to work equally,
independently of the subject and the current branch name.

Anything else you want to add:
This appears to only occur if the subject is equal to the branch being
rebased. I didn't check if this occurs if there is simply 'a'
reference with an identical name in the repository (I'm guessing no).

Please review the rest of the bug report below.
You can delete any lines you don't wish to share.


[System Info]
git version:
git version 2.37.3.windows.1
cpu: x86_64
built from commit: c4992d4fecabd7d111726ecb37e33a3ccb51d6f1
sizeof-long: 4
sizeof-size_t: 8
shell-path: /bin/sh
feature: fsmonitor--daemon
uname: Windows 10.0 19044
compiler info: gnuc: 12.2
libc info: no libc information available
$SHELL (typically, interactive shell): C:\Users\erik\Git\usr\bin\bash.exe


[Enabled Hooks]


-- 
Erik Cervin-Edin

Re: [BUG] fixup commit is dropped during rebase if subject = branch name

[Junio C Hamano]

Erik Cervin Edin <erik@cervined.in> writes:

>   git init --initial-branch=main
>   git commit -m init --allow-empty
>   git tag init

Thanks for a report.  Let me quickly respond with an initial
reaction without being in front of a real computer to run tests on
myself.  Others may give more useful feedback.

>   # failure
>   seq 1 3 >> bar && git add bar && git commit -m main
>   git tag -f x
>   seq 4 6 >> bar && git add bar && git commit -m bar
>   seq 7 9 >> bar && git add bar && git commit --fixup :/main
>   git -c sequence.editor=: rebase --autosquash --interactive x
>   git diff ORIG_HEAD

So near the bottom there are "init", and "x".  The commit title of
"x" is "main" and that is what the fix-up intends to amend.

But then I do not think there is any valid expectation if you say
"keep x intact and rebase everything above", which is what the
command line arguments tell the last command to do.  Perhaps we
should keep all original commits up to that "fixup" one without any
reordering or squashing?

The title of your bug report is also curious.  What happens if you
did

    git branch -m master

just before running the "rebase" command in the above sequence?  I
would have expected to see that in your "expected" section to
contrast the behaviour between "if subject = branch" vs "if subject
!= branch", and the report looks a bit puzzling.

THanks.

Re: [BUG] fixup commit is dropped during rebase if subject = branch name

[Erik Cervin Edin]

On Sat, Sep 17, 2022, 8:04 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> >   # failure
> >   seq 1 3 >> bar && git add bar && git commit -m main
> >   git tag -f x
> >   seq 4 6 >> bar && git add bar && git commit -m bar
> >   seq 7 9 >> bar && git add bar && git commit --fixup :/main
> >   git -c sequence.editor=: rebase --autosquash --interactive x
> >   git diff ORIG_HEAD
>
> So near the bottom there are "init", and "x".  The commit title of
> "x" is "main" and that is what the fix-up intends to amend.

Yes, sorry if that was unclear. So the branch looks like

  474b082 (HEAD -> main) fixup! main
  acd367f bar
  576294e (tag: x) main
  dd27847 (tag: init) init

so
  git -c sequence.editor rebase -i --autosquash x
should yield

  474b082 (HEAD -> main) fixup! main
  acd367f bar
  576294e (tag: x) main
  dd27847 (tag: init) init

ie. no change, but instead yields

  acd367f (HEAD -> main) bar
  576294e (tag: x) main
  dd27847 (tag: init) init

ie. it drops the fixup! main commit but only if it's the first commit, see
second example I posted

On Sat, Sep 17, 2022 at 4:45 PM Erik Cervin Edin <erik@cervined.in> wrote:
>   # normal
>   git reset --hard init
>   seq 1 3 >> bar && git add bar && git commit -m main
>   git tag -f x
>   seq 4 6 >> bar && git add bar && git commit --fixup :/main
>   seq 7 9 >> bar && git add bar && git commit -m bar
>   git -c sequence.editor=: rebase --autosquash --interactive x
>   git diff ORIG_HEAD

yields

  acd367f (HEAD -> main) bar
  474b082 fixup! main
  576294e (tag: x) main
  dd27847 (tag: init) init

as expected.

> But then I do not think there is any valid expectation if you say
> "keep x intact and rebase everything above", which is what the
> command line arguments tell the last command to do.  Perhaps we
> should keep all original commits up to that "fixup" one without any
> reordering or squashing?

I'm not sure I follow but the report is concerning the unexpected
behavior of dropping the commit under these specific conditions. I'm
not 100% sure but as I recall, this only happens in situation where
the fixup may not be applied (and in which case it should remain as
is).

> The title of your bug report is also curious.  What happens if you
> did
>
>     git branch -m master

that works as expected

  git reset --hard init
  seq 1 3 >> bar && git add bar && git commit -m main
  git tag -f x
  seq 4 6 >> bar && git add bar && git commit -m bar
  seq 7 9 >> bar && git add bar && git commit --fixup :/main
  git branch -m master
  git -c sequence.editor=: rebase --autosquash --interactive x

but changing branch -m to switch -c reproduces the bug

  6650ace (HEAD -> master) bar
  7835cd5 (tag: x) main
  368ed2f (tag: init) init

Hope this better clarifies what is going on!

Re: [BUG] fixup commit is dropped during rebase if subject = branch name

[Johannes Altmanninger]

On Sat, Sep 17, 2022 at 04:45:17PM +0200, Erik Cervin Edin wrote:
> Thank you for filling out a Git bug report!
> Please answer the following questions to help us understand your issue.
> 
> What did you do before the bug happened? (Steps to reproduce your issue)
>   dir=rebase-fixup-subject-equals-branch-name
>   mkdir $dir
>   cd $dir
>   git init --initial-branch=main
>   git commit -m init --allow-empty
>   git tag init
> 
>   # failure
>   seq 1 3 >> bar && git add bar && git commit -m main
>   git tag -f x
>   seq 4 6 >> bar && git add bar && git commit -m bar
>   seq 7 9 >> bar && git add bar && git commit --fixup :/main
>   git -c sequence.editor=: rebase --autosquash --interactive x

Huh, this silently discards the fixup commit, without applying it.

If "foo" is a valid refspec, then the autosquash machinery will apply to it
all fixup commits with subject "fixup! foo".
The problem you hit is that "foo" points to the fixup commit itself - which
is the only destination commit that definitely won't work.

Here is a possible fix:

diff --git a/sequencer.c b/sequencer.c
index 79dad522f5..7cbd8c2595 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -6231,3 +6231,3 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
 				 (commit2 =
-				  lookup_commit_reference_by_name(p)) &&
+				  lookup_commit_reference_by_name(p)) != item->commit &&
 				 *commit_todo_item_at(&commit_todo, commit2))

[PATCH] sequencer: avoid dropping fixup commit that targets self via commit-ish

[Johannes Altmanninger]

Commit 68d5d03bc4 (rebase: teach --autosquash to match on sha1 in
addition to message, 2010-11-04) made --autosquash apply a commit
with subject "fixup! 012345" to the first commit in the todo list
whose OID starts with 012345. So far so good.

More recently, c44a4c650c (rebase -i: rearrange fixup/squash lines
using the rebase--helper, 2017-07-14) reimplemented this logic in C
and introduced two behavior changes.
First, OID matches are given precedence over subject prefix
matches.  Second, instead of prefix-matching OIDs, we use
lookup_commit_reference_by_name().  This means that if 012345 is a
branch name, we will apply the fixup commit to the tip of that branch
(if that is present in the todo list).

Both behavior changes might be motivated by performance concerns
(since the commit message mentions performance).  Looking through
the todo list to find a commit that matches the given prefix can be
more expensive than looking up an OID.  The runtime of the former is
of O(n*m) where n is the size of the todo list and m is the length
of a commit subject. However, if this is really a problem, we could
easily make it O(m) by constructing a trie (prefix tree).

Demonstrate both behavior changes by adding two test cases for
"fixup! foo" where foo is a commit-ish that is not an OID-prefix.
Arguably, this feature is very weird.  If no one uses it we should
consider removing it.

Regardless, there is one bad edge case to fix.  Let refspec "foo" point
to a commit with the subject "fixup! foo". Since rebase --autosquash
finds the fixup target via lookup_commit_reference_by_name(), the
fixup target is the fixup commit itself. Obviously this can't work.
We proceed with the broken invariant and drop the fixup commit
entirely.

The self-fixup was only allowed because the fixup commit was already
added to the preliminary todo list, which it shouldn't be.  Rather,
we should first compute the fixup target and only then add the fixup
commit to the todo list. Make it so, avoiding this error by design,
and add a third test for this case.

Reported-by: Erik Cervin Edin <erik@cervined.in>
Signed-off-by: Johannes Altmanninger <aclopte@gmail.com>
---
 sequencer.c                  |  4 ++--
 t/t3415-rebase-autosquash.sh | 44 ++++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 484ca9aa50..777200a6dc 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -6287,8 +6287,6 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
 			return error(_("the script was already rearranged."));
 		}
 
-		*commit_todo_item_at(&commit_todo, item->commit) = item;
-
 		parse_commit(item->commit);
 		commit_buffer = logmsg_reencode(item->commit, NULL, "UTF-8");
 		find_commit_subject(commit_buffer, &subject);
@@ -6355,6 +6353,8 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
 					strhash(entry->subject));
 			hashmap_put(&subject2item, &entry->entry);
 		}
+
+		*commit_todo_item_at(&commit_todo, item->commit) = item;
 	}
 
 	if (rearranged) {
diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index 78c27496d6..879e628512 100755
--- a/t/t3415-rebase-autosquash.sh
+++ b/t/t3415-rebase-autosquash.sh
@@ -232,6 +232,50 @@ test_expect_success 'auto squash that matches longer sha1' '
 	test_line_count = 1 actual
 '
 
+test_expect_success 'auto squash that matches regex' '
+	git reset --hard base &&
+	git commit --allow-empty -m "hay needle hay" &&
+	git commit --allow-empty -m "fixup! :/[n]eedle" &&
+	GIT_SEQUENCE_EDITOR="cat >tmp" git rebase --autosquash -i HEAD^^ &&
+	sed -ne "/^[^#]/{s/[0-9a-f]\{7,\}/HASH/g;p}" tmp >actual &&
+	cat <<-EOF >expect &&
+	pick HASH hay needle hay # empty
+	fixup HASH fixup! :/[n]eedle # empty
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'auto squash of fixup commit that matches branch name' '
+	git reset --hard base &&
+	git commit --allow-empty -m "wip commit (just a prefix match so overshadowed by branch)" &&
+	git commit --allow-empty -m "tip of wip" &&
+	git branch wip &&
+	git commit --allow-empty -m "unrelated commit" &&
+	git commit --allow-empty -m "fixup! wip" &&
+	GIT_SEQUENCE_EDITOR="cat >tmp" git rebase --autosquash -i HEAD^^^^ &&
+	sed -ne "/^[^#]/{s/[0-9a-f]\{7,\}/HASH/g;p}" tmp >actual &&
+	cat <<-EOF >expect &&
+	pick HASH wip commit (just a prefix match so overshadowed by branch) # empty
+	pick HASH tip of wip # empty
+	fixup HASH fixup! wip # empty
+	pick HASH unrelated commit # empty
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'auto squash of fixup commit that matches branch name which points back to fixup commit' '
+	git reset --hard base &&
+	git commit --allow-empty -m "fixup! self-cycle" &&
+	git branch self-cycle &&
+	GIT_SEQUENCE_EDITOR="cat >tmp" git rebase --autosquash -i HEAD^^ &&
+	sed -ne "/^[^#]/{s/[0-9a-f]\{7,\}/HASH/g;p}" tmp >actual &&
+	cat <<-EOF >expect &&
+	pick HASH second commit
+	pick HASH fixup! self-cycle # empty
+	EOF
+	test_cmp expect actual
+'
+
 test_auto_commit_flags () {
 	git reset --hard base &&
 	echo 1 >file1 &&
-- 
2.37.3.830.gf65be7a4d6

Re: [PATCH] sequencer: avoid dropping fixup commit that targets self via commit-ish

[Erik Cervin Edin]

Thank you for the explanation!

On Sun, Sep 18, 2022 at 2:11 PM Johannes Altmanninger <aclopte@gmail.com> wrote:
>
> Commit 68d5d03bc4 (rebase: teach --autosquash to match on sha1 in
> addition to message, 2010-11-04) made --autosquash apply a commit
> with subject "fixup! 012345" to the first commit in the todo list
> whose OID starts with 012345. So far so good.

I wasn't aware such fixups were recognized. The only way to create
such fixups is manually, correct?

Re: [PATCH] sequencer: avoid dropping fixup commit that targets self via commit-ish

[Johannes Altmanninger]

On Sun, Sep 18, 2022 at 05:05:29PM +0200, Erik Cervin Edin wrote:
> Thank you for the explanation!
> 
> On Sun, Sep 18, 2022 at 2:11 PM Johannes Altmanninger <aclopte@gmail.com> wrote:
> >
> > Commit 68d5d03bc4 (rebase: teach --autosquash to match on sha1 in
> > addition to message, 2010-11-04) made --autosquash apply a commit
> > with subject "fixup! 012345" to the first commit in the todo list
> > whose OID starts with 012345. So far so good.
> 
> I wasn't aware such fixups were recognized.

Neither was I. I've repeatedly had issues where the message created by
"commit --fixup" was ambiguous (user error I know) but I can change my tool
to use OIDs for the cases when I squash them right away..

> The only way to create
> such fixups is manually, correct?

I think so

Re: [PATCH] sequencer: avoid dropping fixup commit that targets self via commit-ish

[Junio C Hamano]

Johannes Altmanninger <aclopte@gmail.com> writes:

> First, OID matches are given precedence over subject prefix
> matches.  Second, instead of prefix-matching OIDs, we use
> lookup_commit_reference_by_name().  This means that if 012345 is a
> branch name, we will apply the fixup commit to the tip of that branch
> (if that is present in the todo list).

Good finding.

I think that both are results from imprecise conversion from the
original done carelessly.  A rewrite does not necessarily have to be
bug-to-bug equivalent, and the precedence between object names vs
subject prefix is something I think does not have to be kept the
same as the original (in other words, a user who gives a token after
"fixup" that is ambiguous between the two deserves whatever the
implementation happens to give).  But use of _by_name() that does
not limit the input to hexadecimal _is_ a problem exactly for the
reason why we have this discussion thread.  The function accepts
more than we want to accept, and anybody who reads the original
commit 68d5d03b (rebase: teach --autosquash to match on sha1 in
addition to message, 2010-11-04) and understands why we added it
wouldn't have used it.

Your solution looks somewhat surprising to me, as I would naively
have thought to fix the use of _by_name() and limit its use only
when the input token is all hexadecimal, or something.  I'd need to
think more to convince myself why this is the right solution.

Thanks.

> Demonstrate both behavior changes by adding two test cases for
> "fixup! foo" where foo is a commit-ish that is not an OID-prefix.
> Arguably, this feature is very weird.  If no one uses it we should
> consider removing it.
>
> Regardless, there is one bad edge case to fix.  Let refspec "foo" point
> to a commit with the subject "fixup! foo". Since rebase --autosquash
> finds the fixup target via lookup_commit_reference_by_name(), the
> fixup target is the fixup commit itself. Obviously this can't work.
> We proceed with the broken invariant and drop the fixup commit
> entirely.
>
> The self-fixup was only allowed because the fixup commit was already
> added to the preliminary todo list, which it shouldn't be.  Rather,
> we should first compute the fixup target and only then add the fixup
> commit to the todo list. Make it so, avoiding this error by design,
> and add a third test for this case.
>
> Reported-by: Erik Cervin Edin <erik@cervined.in>
> Signed-off-by: Johannes Altmanninger <aclopte@gmail.com>
> ---
>  sequencer.c                  |  4 ++--
>  t/t3415-rebase-autosquash.sh | 44 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 46 insertions(+), 2 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 484ca9aa50..777200a6dc 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -6287,8 +6287,6 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
>  			return error(_("the script was already rearranged."));
>  		}
>  
> -		*commit_todo_item_at(&commit_todo, item->commit) = item;
> -
>  		parse_commit(item->commit);
>  		commit_buffer = logmsg_reencode(item->commit, NULL, "UTF-8");
>  		find_commit_subject(commit_buffer, &subject);
> @@ -6355,6 +6353,8 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
>  					strhash(entry->subject));
>  			hashmap_put(&subject2item, &entry->entry);
>  		}
> +
> +		*commit_todo_item_at(&commit_todo, item->commit) = item;
>  	}
>  
>  	if (rearranged) {
> diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
> index 78c27496d6..879e628512 100755
> --- a/t/t3415-rebase-autosquash.sh
> +++ b/t/t3415-rebase-autosquash.sh
> @@ -232,6 +232,50 @@ test_expect_success 'auto squash that matches longer sha1' '
>  	test_line_count = 1 actual
>  '
>  
> +test_expect_success 'auto squash that matches regex' '
> +	git reset --hard base &&
> +	git commit --allow-empty -m "hay needle hay" &&
> +	git commit --allow-empty -m "fixup! :/[n]eedle" &&
> +	GIT_SEQUENCE_EDITOR="cat >tmp" git rebase --autosquash -i HEAD^^ &&
> +	sed -ne "/^[^#]/{s/[0-9a-f]\{7,\}/HASH/g;p}" tmp >actual &&
> +	cat <<-EOF >expect &&
> +	pick HASH hay needle hay # empty
> +	fixup HASH fixup! :/[n]eedle # empty
> +	EOF
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'auto squash of fixup commit that matches branch name' '
> +	git reset --hard base &&
> +	git commit --allow-empty -m "wip commit (just a prefix match so overshadowed by branch)" &&
> +	git commit --allow-empty -m "tip of wip" &&
> +	git branch wip &&
> +	git commit --allow-empty -m "unrelated commit" &&
> +	git commit --allow-empty -m "fixup! wip" &&
> +	GIT_SEQUENCE_EDITOR="cat >tmp" git rebase --autosquash -i HEAD^^^^ &&
> +	sed -ne "/^[^#]/{s/[0-9a-f]\{7,\}/HASH/g;p}" tmp >actual &&
> +	cat <<-EOF >expect &&
> +	pick HASH wip commit (just a prefix match so overshadowed by branch) # empty
> +	pick HASH tip of wip # empty
> +	fixup HASH fixup! wip # empty
> +	pick HASH unrelated commit # empty
> +	EOF
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'auto squash of fixup commit that matches branch name which points back to fixup commit' '
> +	git reset --hard base &&
> +	git commit --allow-empty -m "fixup! self-cycle" &&
> +	git branch self-cycle &&
> +	GIT_SEQUENCE_EDITOR="cat >tmp" git rebase --autosquash -i HEAD^^ &&
> +	sed -ne "/^[^#]/{s/[0-9a-f]\{7,\}/HASH/g;p}" tmp >actual &&
> +	cat <<-EOF >expect &&
> +	pick HASH second commit
> +	pick HASH fixup! self-cycle # empty
> +	EOF
> +	test_cmp expect actual
> +'
> +
>  test_auto_commit_flags () {
>  	git reset --hard base &&
>  	echo 1 >file1 &&

Re: [PATCH] sequencer: avoid dropping fixup commit that targets self via commit-ish

[Junio C Hamano]

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

> ...  But use of _by_name() that does
> not limit the input to hexadecimal _is_ a problem ...

Ah, no, sorry, this was wrong.  The original used "rev-parse -q --verify"
without restricting the "single word" to "sha1 prefix".

Re: [PATCH] sequencer: avoid dropping fixup commit that targets self via commit-ish

[Johannes Altmanninger]

On Mon, Sep 19, 2022 at 09:07:27AM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > ...  But use of _by_name() that does
> > not limit the input to hexadecimal _is_ a problem ...
> 
> Ah, no, sorry, this was wrong.  The original used "rev-parse -q --verify"
> without restricting the "single word" to "sha1 prefix".
> 

Yeah, I've sent a v2 that removes the false verdicts from the commit message.

Even if we restricted it to SHAs it would still be ambiguous.. I guess it
doesn't matter in practice.

Thanks

Re: [PATCH] sequencer: avoid dropping fixup commit that targets self via commit-ish

[Junio C Hamano]

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

> Your solution looks somewhat surprising to me, as I would naively
> have thought to fix the use of _by_name() and limit its use only
> when the input token is all hexadecimal, or something.  I'd need to
> think more to convince myself why this is the right solution.

OK, we try to find what to amend with the current "fixupish" from
the todo slab, which by definition must be something that we have
already dealt with.  The current "fixupish" should not be found
because we haven't finished dealing with it, so delaying the
addition of the current one to the todo slab is a must.

There is no leaving or continuing the loop, other than an error
return that aborts the whole thing, that may break the resulting
todo slab in the normal case due to this change, either.  

The fix makes sense to me.  Will queue.

Thanks.

[PATCH v2] sequencer: avoid dropping fixup commit that targets self via commit-ish

[Johannes Altmanninger]

Commit 68d5d03bc4 (rebase: teach --autosquash to match on sha1 in
addition to message, 2010-11-04) taught autosquash to recognize
subjects like "fixup! 7a235b" where 7a235b is an OID-prefix. It
actually did more than advertised: 7a235b can be an arbitrary
commit-ish (as long as it's not trailed by spaces).

Accidental(?) use of this secret feature revealed a bug where we
would silently drop a fixup commit. The bug can also be triggered
when using an OID-prefix but that's unlikely in practice.

Given a commit with subject "fixup! main" that is the tip of the
branch "main". When computing the fixup target for this commit, we
find the commit itself. This is wrong because, by definition, a fixup
target must be an earlier commit in the todo list. We wrongly find
the current commit because we added it to the todo list prematurely.
Avoid these fixup-cycles by only adding the current commit after we
have finished finding its target.

Reported-by: Erik Cervin Edin <erik@cervined.in>
Signed-off-by: Johannes Altmanninger <aclopte@gmail.com>
---
 sequencer.c                  |  4 ++--
 t/t3415-rebase-autosquash.sh | 26 ++++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 2 deletions(-)

Changes to v1.
- Remove wrong assumptions from commit message. The commit message should
  be clearer now (though I didn't spend too much time on it).
- Drop one test because it's not related to the fix (and doesn't test anything
  I care about) and modify the other test so it requires the fix to pass.

1:  cb2ee0e003 ! 1:  410ca51936 sequencer: avoid dropping fixup commit that targets self via commit-ish
    @@ Commit message
         sequencer: avoid dropping fixup commit that targets self via commit-ish
     
         Commit 68d5d03bc4 (rebase: teach --autosquash to match on sha1 in
    -    addition to message, 2010-11-04) made --autosquash apply a commit
    -    with subject "fixup! 012345" to the first commit in the todo list
    -    whose OID starts with 012345. So far so good.
    +    addition to message, 2010-11-04) taught autosquash to recognize
    +    subjects like "fixup! 7a235b" where 7a235b is an OID-prefix. It
    +    actually did more than advertised: 7a235b can be an arbitrary
    +    commit-ish (as long as it's not trailed by spaces).
     
    -    More recently, c44a4c650c (rebase -i: rearrange fixup/squash lines
    -    using the rebase--helper, 2017-07-14) reimplemented this logic in C
    -    and introduced two behavior changes.
    -    First, OID matches are given precedence over subject prefix
    -    matches.  Second, instead of prefix-matching OIDs, we use
    -    lookup_commit_reference_by_name().  This means that if 012345 is a
    -    branch name, we will apply the fixup commit to the tip of that branch
    -    (if that is present in the todo list).
    +    Accidental(?) use of this secret feature revealed a bug where we
    +    would silently drop a fixup commit. The bug can also be triggered
    +    when using an OID-prefix but that's unlikely in practice.
     
    -    Both behavior changes might be motivated by performance concerns
    -    (since the commit message mentions performance).  Looking through
    -    the todo list to find a commit that matches the given prefix can be
    -    more expensive than looking up an OID.  The runtime of the former is
    -    of O(n*m) where n is the size of the todo list and m is the length
    -    of a commit subject. However, if this is really a problem, we could
    -    easily make it O(m) by constructing a trie (prefix tree).
    -
    -    Demonstrate both behavior changes by adding two test cases for
    -    "fixup! foo" where foo is a commit-ish that is not an OID-prefix.
    -    Arguably, this feature is very weird.  If no one uses it we should
    -    consider removing it.
    -
    -    Regardless, there is one bad edge case to fix.  Let refspec "foo" point
    -    to a commit with the subject "fixup! foo". Since rebase --autosquash
    -    finds the fixup target via lookup_commit_reference_by_name(), the
    -    fixup target is the fixup commit itself. Obviously this can't work.
    -    We proceed with the broken invariant and drop the fixup commit
    -    entirely.
    -
    -    The self-fixup was only allowed because the fixup commit was already
    -    added to the preliminary todo list, which it shouldn't be.  Rather,
    -    we should first compute the fixup target and only then add the fixup
    -    commit to the todo list. Make it so, avoiding this error by design,
    -    and add a third test for this case.
    +    Given a commit with subject "fixup! main" that is the tip of the
    +    branch "main". When computing the fixup target for this commit, we
    +    find the commit itself. This is wrong because, by definition, a fixup
    +    target must be an earlier commit in the todo list. We wrongly find
    +    the current commit because we added it to the todo list prematurely.
    +    Avoid these fixup-cycles by only adding the current commit after we
    +    have finished finding its target.
     
         Reported-by: Erik Cervin Edin <erik@cervined.in>
    -    Signed-off-by: Johannes Altmanninger <aclopte@gmail.com>
    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      ## sequencer.c ##
     @@ sequencer.c: int todo_list_rearrange_squash(struct todo_list *todo_list)
    @@ t/t3415-rebase-autosquash.sh: test_expect_success 'auto squash that matches long
     +test_expect_success 'auto squash that matches regex' '
     +	git reset --hard base &&
     +	git commit --allow-empty -m "hay needle hay" &&
    -+	git commit --allow-empty -m "fixup! :/[n]eedle" &&
    ++	git commit --allow-empty -m "fixup! :/needle" &&
     +	GIT_SEQUENCE_EDITOR="cat >tmp" git rebase --autosquash -i HEAD^^ &&
    -+	sed -ne "/^[^#]/{s/[0-9a-f]\{7,\}/HASH/g;p}" tmp >actual &&
    ++	sed -ne "/^[^#]/{s/[0-9a-f]\{7,\}/HASH/g;p;}" tmp >actual &&
     +	cat <<-EOF >expect &&
     +	pick HASH hay needle hay # empty
    -+	fixup HASH fixup! :/[n]eedle # empty
    -+	EOF
    -+	test_cmp expect actual
    -+'
    -+
    -+test_expect_success 'auto squash of fixup commit that matches branch name' '
    -+	git reset --hard base &&
    -+	git commit --allow-empty -m "wip commit (just a prefix match so overshadowed by branch)" &&
    -+	git commit --allow-empty -m "tip of wip" &&
    -+	git branch wip &&
    -+	git commit --allow-empty -m "unrelated commit" &&
    -+	git commit --allow-empty -m "fixup! wip" &&
    -+	GIT_SEQUENCE_EDITOR="cat >tmp" git rebase --autosquash -i HEAD^^^^ &&
    -+	sed -ne "/^[^#]/{s/[0-9a-f]\{7,\}/HASH/g;p}" tmp >actual &&
    -+	cat <<-EOF >expect &&
    -+	pick HASH wip commit (just a prefix match so overshadowed by branch) # empty
    -+	pick HASH tip of wip # empty
    -+	fixup HASH fixup! wip # empty
    -+	pick HASH unrelated commit # empty
    ++	fixup HASH fixup! :/needle # empty
     +	EOF
     +	test_cmp expect actual
     +'
    @@ t/t3415-rebase-autosquash.sh: test_expect_success 'auto squash that matches long
     +	git commit --allow-empty -m "fixup! self-cycle" &&
     +	git branch self-cycle &&
     +	GIT_SEQUENCE_EDITOR="cat >tmp" git rebase --autosquash -i HEAD^^ &&
    -+	sed -ne "/^[^#]/{s/[0-9a-f]\{7,\}/HASH/g;p}" tmp >actual &&
    ++	sed -ne "/^[^#]/{s/[0-9a-f]\{7,\}/HASH/g;p;}" tmp >actual &&
     +	cat <<-EOF >expect &&
     +	pick HASH second commit
     +	pick HASH fixup! self-cycle # empty


diff --git a/sequencer.c b/sequencer.c
index 484ca9aa50..777200a6dc 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -6287,8 +6287,6 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
 			return error(_("the script was already rearranged."));
 		}
 
-		*commit_todo_item_at(&commit_todo, item->commit) = item;
-
 		parse_commit(item->commit);
 		commit_buffer = logmsg_reencode(item->commit, NULL, "UTF-8");
 		find_commit_subject(commit_buffer, &subject);
@@ -6355,6 +6353,8 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
 					strhash(entry->subject));
 			hashmap_put(&subject2item, &entry->entry);
 		}
+
+		*commit_todo_item_at(&commit_todo, item->commit) = item;
 	}
 
 	if (rearranged) {
diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index 78c27496d6..98af865268 100755
--- a/t/t3415-rebase-autosquash.sh
+++ b/t/t3415-rebase-autosquash.sh
@@ -232,6 +232,32 @@ test_expect_success 'auto squash that matches longer sha1' '
 	test_line_count = 1 actual
 '
 
+test_expect_success 'auto squash that matches regex' '
+	git reset --hard base &&
+	git commit --allow-empty -m "hay needle hay" &&
+	git commit --allow-empty -m "fixup! :/needle" &&
+	GIT_SEQUENCE_EDITOR="cat >tmp" git rebase --autosquash -i HEAD^^ &&
+	sed -ne "/^[^#]/{s/[0-9a-f]\{7,\}/HASH/g;p;}" tmp >actual &&
+	cat <<-EOF >expect &&
+	pick HASH hay needle hay # empty
+	fixup HASH fixup! :/needle # empty
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'auto squash of fixup commit that matches branch name which points back to fixup commit' '
+	git reset --hard base &&
+	git commit --allow-empty -m "fixup! self-cycle" &&
+	git branch self-cycle &&
+	GIT_SEQUENCE_EDITOR="cat >tmp" git rebase --autosquash -i HEAD^^ &&
+	sed -ne "/^[^#]/{s/[0-9a-f]\{7,\}/HASH/g;p;}" tmp >actual &&
+	cat <<-EOF >expect &&
+	pick HASH second commit
+	pick HASH fixup! self-cycle # empty
+	EOF
+	test_cmp expect actual
+'
+
 test_auto_commit_flags () {
 	git reset --hard base &&
 	echo 1 >file1 &&
-- 
2.37.3.830.gf65be7a4d6

Re: [PATCH v2] sequencer: avoid dropping fixup commit that targets self via commit-ish

[Phillip Wood]

Hi Johannes

On 20/09/2022 04:11, Johannes Altmanninger wrote:
> Commit 68d5d03bc4 (rebase: teach --autosquash to match on sha1 in
> addition to message, 2010-11-04) taught autosquash to recognize
> subjects like "fixup! 7a235b" where 7a235b is an OID-prefix. It
> actually did more than advertised: 7a235b can be an arbitrary
> commit-ish (as long as it's not trailed by spaces).
> 
> Accidental(?) use of this secret feature revealed a bug where we
> would silently drop a fixup commit. The bug can also be triggered
> when using an OID-prefix but that's unlikely in practice.
> 
> Given a commit with subject "fixup! main" that is the tip of the
> branch "main". When computing the fixup target for this commit, we
> find the commit itself. This is wrong because, by definition, a fixup
> target must be an earlier commit in the todo list. We wrongly find
> the current commit because we added it to the todo list prematurely.
> Avoid these fixup-cycles by only adding the current commit after we
> have finished finding its target.

Thanks for working on this, the fix for the fixup self reference looks 
good. It's unfortunate that the implementation is not stricter when 
parsing "fixup! <oid>" but it is more or less consistent with the shell 
version which used "git rev-parse $subject"[1]. We should think about 
being stricter but this fix avoids on of the worst pitfalls of our lax 
parsing.

Best Wishes

Phillip

[1] With regard to the oid vs subject prefix issue, I think the shell 
version chose to fixup the first commit that matched either the oid or 
the subject. At least the C version is consistent in preferring an oid 
match over a subject prefix match even if I wish it was the other way round.

> Reported-by: Erik Cervin Edin <erik@cervined.in>
> Signed-off-by: Johannes Altmanninger <aclopte@gmail.com>
> ---
>   sequencer.c                  |  4 ++--
>   t/t3415-rebase-autosquash.sh | 26 ++++++++++++++++++++++++++
>   2 files changed, 28 insertions(+), 2 deletions(-)
> 
> Changes to v1.
> - Remove wrong assumptions from commit message. The commit message should
>    be clearer now (though I didn't spend too much time on it).
> - Drop one test because it's not related to the fix (and doesn't test anything
>    I care about) and modify the other test so it requires the fix to pass.
> 
> 1:  cb2ee0e003 ! 1:  410ca51936 sequencer: avoid dropping fixup commit that targets self via commit-ish
>      @@ Commit message
>           sequencer: avoid dropping fixup commit that targets self via commit-ish
>       
>           Commit 68d5d03bc4 (rebase: teach --autosquash to match on sha1 in
>      -    addition to message, 2010-11-04) made --autosquash apply a commit
>      -    with subject "fixup! 012345" to the first commit in the todo list
>      -    whose OID starts with 012345. So far so good.
>      +    addition to message, 2010-11-04) taught autosquash to recognize
>      +    subjects like "fixup! 7a235b" where 7a235b is an OID-prefix. It
>      +    actually did more than advertised: 7a235b can be an arbitrary
>      +    commit-ish (as long as it's not trailed by spaces).
>       
>      -    More recently, c44a4c650c (rebase -i: rearrange fixup/squash lines
>      -    using the rebase--helper, 2017-07-14) reimplemented this logic in C
>      -    and introduced two behavior changes.
>      -    First, OID matches are given precedence over subject prefix
>      -    matches.  Second, instead of prefix-matching OIDs, we use
>      -    lookup_commit_reference_by_name().  This means that if 012345 is a
>      -    branch name, we will apply the fixup commit to the tip of that branch
>      -    (if that is present in the todo list).
>      +    Accidental(?) use of this secret feature revealed a bug where we
>      +    would silently drop a fixup commit. The bug can also be triggered
>      +    when using an OID-prefix but that's unlikely in practice.
>       
>      -    Both behavior changes might be motivated by performance concerns
>      -    (since the commit message mentions performance).  Looking through
>      -    the todo list to find a commit that matches the given prefix can be
>      -    more expensive than looking up an OID.  The runtime of the former is
>      -    of O(n*m) where n is the size of the todo list and m is the length
>      -    of a commit subject. However, if this is really a problem, we could
>      -    easily make it O(m) by constructing a trie (prefix tree).
>      -
>      -    Demonstrate both behavior changes by adding two test cases for
>      -    "fixup! foo" where foo is a commit-ish that is not an OID-prefix.
>      -    Arguably, this feature is very weird.  If no one uses it we should
>      -    consider removing it.
>      -
>      -    Regardless, there is one bad edge case to fix.  Let refspec "foo" point
>      -    to a commit with the subject "fixup! foo". Since rebase --autosquash
>      -    finds the fixup target via lookup_commit_reference_by_name(), the
>      -    fixup target is the fixup commit itself. Obviously this can't work.
>      -    We proceed with the broken invariant and drop the fixup commit
>      -    entirely.
>      -
>      -    The self-fixup was only allowed because the fixup commit was already
>      -    added to the preliminary todo list, which it shouldn't be.  Rather,
>      -    we should first compute the fixup target and only then add the fixup
>      -    commit to the todo list. Make it so, avoiding this error by design,
>      -    and add a third test for this case.
>      +    Given a commit with subject "fixup! main" that is the tip of the
>      +    branch "main". When computing the fixup target for this commit, we
>      +    find the commit itself. This is wrong because, by definition, a fixup
>      +    target must be an earlier commit in the todo list. We wrongly find
>      +    the current commit because we added it to the todo list prematurely.
>      +    Avoid these fixup-cycles by only adding the current commit after we
>      +    have finished finding its target.
>       
>           Reported-by: Erik Cervin Edin <erik@cervined.in>
>      -    Signed-off-by: Johannes Altmanninger <aclopte@gmail.com>
>      -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
>       
>        ## sequencer.c ##
>       @@ sequencer.c: int todo_list_rearrange_squash(struct todo_list *todo_list)
>      @@ t/t3415-rebase-autosquash.sh: test_expect_success 'auto squash that matches long
>       +test_expect_success 'auto squash that matches regex' '
>       +	git reset --hard base &&
>       +	git commit --allow-empty -m "hay needle hay" &&
>      -+	git commit --allow-empty -m "fixup! :/[n]eedle" &&
>      ++	git commit --allow-empty -m "fixup! :/needle" &&
>       +	GIT_SEQUENCE_EDITOR="cat >tmp" git rebase --autosquash -i HEAD^^ &&
>      -+	sed -ne "/^[^#]/{s/[0-9a-f]\{7,\}/HASH/g;p}" tmp >actual &&
>      ++	sed -ne "/^[^#]/{s/[0-9a-f]\{7,\}/HASH/g;p;}" tmp >actual &&
>       +	cat <<-EOF >expect &&
>       +	pick HASH hay needle hay # empty
>      -+	fixup HASH fixup! :/[n]eedle # empty
>      -+	EOF
>      -+	test_cmp expect actual
>      -+'
>      -+
>      -+test_expect_success 'auto squash of fixup commit that matches branch name' '
>      -+	git reset --hard base &&
>      -+	git commit --allow-empty -m "wip commit (just a prefix match so overshadowed by branch)" &&
>      -+	git commit --allow-empty -m "tip of wip" &&
>      -+	git branch wip &&
>      -+	git commit --allow-empty -m "unrelated commit" &&
>      -+	git commit --allow-empty -m "fixup! wip" &&
>      -+	GIT_SEQUENCE_EDITOR="cat >tmp" git rebase --autosquash -i HEAD^^^^ &&
>      -+	sed -ne "/^[^#]/{s/[0-9a-f]\{7,\}/HASH/g;p}" tmp >actual &&
>      -+	cat <<-EOF >expect &&
>      -+	pick HASH wip commit (just a prefix match so overshadowed by branch) # empty
>      -+	pick HASH tip of wip # empty
>      -+	fixup HASH fixup! wip # empty
>      -+	pick HASH unrelated commit # empty
>      ++	fixup HASH fixup! :/needle # empty
>       +	EOF
>       +	test_cmp expect actual
>       +'
>      @@ t/t3415-rebase-autosquash.sh: test_expect_success 'auto squash that matches long
>       +	git commit --allow-empty -m "fixup! self-cycle" &&
>       +	git branch self-cycle &&
>       +	GIT_SEQUENCE_EDITOR="cat >tmp" git rebase --autosquash -i HEAD^^ &&
>      -+	sed -ne "/^[^#]/{s/[0-9a-f]\{7,\}/HASH/g;p}" tmp >actual &&
>      ++	sed -ne "/^[^#]/{s/[0-9a-f]\{7,\}/HASH/g;p;}" tmp >actual &&
>       +	cat <<-EOF >expect &&
>       +	pick HASH second commit
>       +	pick HASH fixup! self-cycle # empty
> 
> 
> diff --git a/sequencer.c b/sequencer.c
> index 484ca9aa50..777200a6dc 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -6287,8 +6287,6 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
>   			return error(_("the script was already rearranged."));
>   		}
>   
> -		*commit_todo_item_at(&commit_todo, item->commit) = item;
> -
>   		parse_commit(item->commit);
>   		commit_buffer = logmsg_reencode(item->commit, NULL, "UTF-8");
>   		find_commit_subject(commit_buffer, &subject);
> @@ -6355,6 +6353,8 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
>   					strhash(entry->subject));
>   			hashmap_put(&subject2item, &entry->entry);
>   		}
> +
> +		*commit_todo_item_at(&commit_todo, item->commit) = item;
>   	}
>   
>   	if (rearranged) {
> diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
> index 78c27496d6..98af865268 100755
> --- a/t/t3415-rebase-autosquash.sh
> +++ b/t/t3415-rebase-autosquash.sh
> @@ -232,6 +232,32 @@ test_expect_success 'auto squash that matches longer sha1' '
>   	test_line_count = 1 actual
>   '
>   
> +test_expect_success 'auto squash that matches regex' '
> +	git reset --hard base &&
> +	git commit --allow-empty -m "hay needle hay" &&
> +	git commit --allow-empty -m "fixup! :/needle" &&
> +	GIT_SEQUENCE_EDITOR="cat >tmp" git rebase --autosquash -i HEAD^^ &&
> +	sed -ne "/^[^#]/{s/[0-9a-f]\{7,\}/HASH/g;p;}" tmp >actual &&
> +	cat <<-EOF >expect &&
> +	pick HASH hay needle hay # empty
> +	fixup HASH fixup! :/needle # empty
> +	EOF
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'auto squash of fixup commit that matches branch name which points back to fixup commit' '
> +	git reset --hard base &&
> +	git commit --allow-empty -m "fixup! self-cycle" &&
> +	git branch self-cycle &&
> +	GIT_SEQUENCE_EDITOR="cat >tmp" git rebase --autosquash -i HEAD^^ &&
> +	sed -ne "/^[^#]/{s/[0-9a-f]\{7,\}/HASH/g;p;}" tmp >actual &&
> +	cat <<-EOF >expect &&
> +	pick HASH second commit
> +	pick HASH fixup! self-cycle # empty
> +	EOF
> +	test_cmp expect actual
> +'
> +
>   test_auto_commit_flags () {
>   	git reset --hard base &&
>   	echo 1 >file1 &&

Re: [PATCH v2] sequencer: avoid dropping fixup commit that targets self via commit-ish

[Junio C Hamano]

Johannes Altmanninger <aclopte@gmail.com> writes:

> +test_expect_success 'auto squash that matches regex' '
> +	git reset --hard base &&
> +	git commit --allow-empty -m "hay needle hay" &&
> +	git commit --allow-empty -m "fixup! :/needle" &&
> +	GIT_SEQUENCE_EDITOR="cat >tmp" git rebase --autosquash -i HEAD^^ &&
> +	sed -ne "/^[^#]/{s/[0-9a-f]\{7,\}/HASH/g;p;}" tmp >actual &&
> +	cat <<-EOF >expect &&
> +	pick HASH hay needle hay # empty
> +	fixup HASH fixup! :/needle # empty
> +	EOF
> +	test_cmp expect actual
> +'

hint: Waiting for your editor to close the file...
Successfully rebased and updated refs/heads/main.
--- expect      2022-09-21 18:45:27.617530848 +0000
+++ actual      2022-09-21 18:45:27.613530478 +0000
@@ -1,2 +1,2 @@
 pick HASH hay needle hay # empty
-fixup HASH fixup! :/needle # empty
+pick HASH fixup! :/needle # empty
not ok 11 - auto squash that matches regex

That does not look very good X-<.

Re: [PATCH v2] sequencer: avoid dropping fixup commit that targets self via commit-ish

[Johannes Altmanninger]

On Wed, Sep 21, 2022 at 11:47:26AM -0700, Junio C Hamano wrote:
> Johannes Altmanninger <aclopte@gmail.com> writes:
> 
> > +test_expect_success 'auto squash that matches regex' '
> > +	git reset --hard base &&
> > +	git commit --allow-empty -m "hay needle hay" &&
> > +	git commit --allow-empty -m "fixup! :/needle" &&
> > +	GIT_SEQUENCE_EDITOR="cat >tmp" git rebase --autosquash -i HEAD^^ &&
> > +	sed -ne "/^[^#]/{s/[0-9a-f]\{7,\}/HASH/g;p;}" tmp >actual &&
> > +	cat <<-EOF >expect &&
> > +	pick HASH hay needle hay # empty
> > +	fixup HASH fixup! :/needle # empty
> > +	EOF
> > +	test_cmp expect actual
> > +'
> 
> hint: Waiting for your editor to close the file...
> Successfully rebased and updated refs/heads/main.
> --- expect      2022-09-21 18:45:27.617530848 +0000
> +++ actual      2022-09-21 18:45:27.613530478 +0000
> @@ -1,2 +1,2 @@
>  pick HASH hay needle hay # empty
> -fixup HASH fixup! :/needle # empty
> +pick HASH fixup! :/needle # empty
> not ok 11 - auto squash that matches regex
> 
> That does not look very good X-<.

Sorry the v2 of this test case is very misleading, should probably drop this
test entirely.  It's been a long a day so I'll send v3 another day (if needed).

Re: [PATCH v2] sequencer: avoid dropping fixup commit that targets self via commit-ish

[Junio C Hamano]

Johannes Altmanninger <aclopte@gmail.com> writes:

>> hint: Waiting for your editor to close the file...
>> Successfully rebased and updated refs/heads/main.
>> --- expect      2022-09-21 18:45:27.617530848 +0000
>> +++ actual      2022-09-21 18:45:27.613530478 +0000
>> @@ -1,2 +1,2 @@
>>  pick HASH hay needle hay # empty
>> -fixup HASH fixup! :/needle # empty
>> +pick HASH fixup! :/needle # empty
>> not ok 11 - auto squash that matches regex
>> 
>> That does not look very good X-<.
>
> Sorry the v2 of this test case is very misleading, should probably drop this
> test entirely.  It's been a long a day so I'll send v3 another day (if needed).

Thanks.

[PATCH v3] sequencer: avoid dropping fixup commit that targets self via commit-ish

[Johannes Altmanninger]

Commit 68d5d03bc4 (rebase: teach --autosquash to match on sha1 in
addition to message, 2010-11-04) taught autosquash to recognize
subjects like "fixup! 7a235b" where 7a235b is an OID-prefix. It
actually did more than advertised: 7a235b can be an arbitrary
commit-ish (as long as it's not trailed by spaces).

Accidental(?) use of this secret feature revealed a bug where we
would silently drop a fixup commit. The bug can also be triggered
when using an OID-prefix but that's unlikely in practice.

Let the commit with subject "fixup! main" be the tip of the "main"
branch. When computing the fixup target for this commit, we find
the commit itself. This is wrong because, by definition, a fixup
target must be an earlier commit in the todo list. We wrongly find
the current commit because we added it to the todo list prematurely.
Avoid these fixup-cycles by only adding the current commit to the
todo list after we have finished looking for the fixup target.

Reported-by: Erik Cervin Edin <erik@cervined.in>
Signed-off-by: Johannes Altmanninger <aclopte@gmail.com>
---
 sequencer.c                  |  4 ++--
 t/t3415-rebase-autosquash.sh | 13 +++++++++++++
 2 files changed, 15 insertions(+), 2 deletions(-)

Changes since v2:
- minor commit message rewording and clarification
- dropped a test that was based on a wrong understanding of --autosquash.

  I saw b425461cb5 (SQUASH??? resurrect previous version of the tests,
  2022-09-21) today. I'm leaning towards not adding them back because
  those tests were only added to reflect the current behavior.  but that
  behavior is not something I care about.
  We might even want to change this behavior, by restricting OID fixup
  targets to SHAs only. So I don't think these tests are desirable,
  unless they help others understand the current state of affairs?

1:  37e00c51bd ! 1:  a25c886a78 sequencer: avoid dropping fixup commit that targets self via commit-ish
    @@ Commit message
         would silently drop a fixup commit. The bug can also be triggered
         when using an OID-prefix but that's unlikely in practice.
     
    -    Given a commit with subject "fixup! main" that is the tip of the
    -    branch "main". When computing the fixup target for this commit, we
    -    find the commit itself. This is wrong because, by definition, a fixup
    +    Let the commit with subject "fixup! main" be the tip of the "main"
    +    branch. When computing the fixup target for this commit, we find
    +    the commit itself. This is wrong because, by definition, a fixup
         target must be an earlier commit in the todo list. We wrongly find
         the current commit because we added it to the todo list prematurely.
    -    Avoid these fixup-cycles by only adding the current commit after we
    -    have finished finding its target.
    +    Avoid these fixup-cycles by only adding the current commit to the
    +    todo list after we have finished looking for the fixup target.
     
         Reported-by: Erik Cervin Edin <erik@cervined.in>
     
      ## sequencer.c ##
     @@ sequencer.c: int todo_list_rearrange_squash(struct todo_list *todo_list)
    @@ t/t3415-rebase-autosquash.sh: test_expect_success 'auto squash that matches long
      	test_line_count = 1 actual
      '
      
    -+test_expect_success 'auto squash that matches regex' '
    -+	git reset --hard base &&
    -+	git commit --allow-empty -m "hay needle hay" &&
    -+	git commit --allow-empty -m "fixup! :/needle" &&
    -+	GIT_SEQUENCE_EDITOR="cat >tmp" git rebase --autosquash -i HEAD^^ &&
    -+	sed -ne "/^[^#]/{s/[0-9a-f]\{7,\}/HASH/g;p;}" tmp >actual &&
    -+	cat <<-EOF >expect &&
    -+	pick HASH hay needle hay # empty
    -+	fixup HASH fixup! :/needle # empty
    -+	EOF
    -+	test_cmp expect actual
    -+'
    -+
     +test_expect_success 'auto squash of fixup commit that matches branch name which points back to fixup commit' '
     +	git reset --hard base &&
     +	git commit --allow-empty -m "fixup! self-cycle" &&


diff --git a/sequencer.c b/sequencer.c
index 484ca9aa50..777200a6dc 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -6287,8 +6287,6 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
 			return error(_("the script was already rearranged."));
 		}
 
-		*commit_todo_item_at(&commit_todo, item->commit) = item;
-
 		parse_commit(item->commit);
 		commit_buffer = logmsg_reencode(item->commit, NULL, "UTF-8");
 		find_commit_subject(commit_buffer, &subject);
@@ -6355,6 +6353,8 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
 					strhash(entry->subject));
 			hashmap_put(&subject2item, &entry->entry);
 		}
+
+		*commit_todo_item_at(&commit_todo, item->commit) = item;
 	}
 
 	if (rearranged) {
diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index 78c27496d6..a364530d76 100755
--- a/t/t3415-rebase-autosquash.sh
+++ b/t/t3415-rebase-autosquash.sh
@@ -232,6 +232,19 @@ test_expect_success 'auto squash that matches longer sha1' '
 	test_line_count = 1 actual
 '
 
+test_expect_success 'auto squash of fixup commit that matches branch name which points back to fixup commit' '
+	git reset --hard base &&
+	git commit --allow-empty -m "fixup! self-cycle" &&
+	git branch self-cycle &&
+	GIT_SEQUENCE_EDITOR="cat >tmp" git rebase --autosquash -i HEAD^^ &&
+	sed -ne "/^[^#]/{s/[0-9a-f]\{7,\}/HASH/g;p;}" tmp >actual &&
+	cat <<-EOF >expect &&
+	pick HASH second commit
+	pick HASH fixup! self-cycle # empty
+	EOF
+	test_cmp expect actual
+'
+
 test_auto_commit_flags () {
 	git reset --hard base &&
 	echo 1 >file1 &&
-- 
2.37.3.830.gf65be7a4d6

Re: [PATCH] sequencer: avoid dropping fixup commit that targets self via commit-ish

[Junio C Hamano]

Johannes Altmanninger <aclopte@gmail.com> writes:

> +test_expect_success 'auto squash of fixup commit that matches branch name which points back to fixup commit' '
> ...
> +	sed -ne "/^[^#]/{s/[0-9a-f]\{7,\}/HASH/g;p}" tmp >actual &&

The construct is rejected by sed implementations of BSD descent, it
seems.

https://github.com/git/git/actions/runs/3084784922/jobs/4987337058#step:4:1844

++ sed -ne '/^[^#]/{s/[0-9a-f]\{7,\}/HASH/g;p}' tmp
sed: 1: "/^[^#]/{s/[0-9a-f]\{7,\ ...": extra characters at the end of p command
error: last command exited with $?=1
not ok 11 - auto squash that matches regex

Here is a fix-up that can be squashed in.

Thanks.

 t/t3415-rebase-autosquash.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index 879e628512..d65d2258c3 100755
--- a/t/t3415-rebase-autosquash.sh
+++ b/t/t3415-rebase-autosquash.sh
@@ -237,7 +237,7 @@ test_expect_success 'auto squash that matches regex' '
 	git commit --allow-empty -m "hay needle hay" &&
 	git commit --allow-empty -m "fixup! :/[n]eedle" &&
 	GIT_SEQUENCE_EDITOR="cat >tmp" git rebase --autosquash -i HEAD^^ &&
-	sed -ne "/^[^#]/{s/[0-9a-f]\{7,\}/HASH/g;p}" tmp >actual &&
+	sed -ne "/^[^#]/{s/[0-9a-f]\{7,\}/HASH/g;p;}" tmp >actual &&
 	cat <<-EOF >expect &&
 	pick HASH hay needle hay # empty
 	fixup HASH fixup! :/[n]eedle # empty
@@ -253,7 +253,7 @@ test_expect_success 'auto squash of fixup commit that matches branch name' '
 	git commit --allow-empty -m "unrelated commit" &&
 	git commit --allow-empty -m "fixup! wip" &&
 	GIT_SEQUENCE_EDITOR="cat >tmp" git rebase --autosquash -i HEAD^^^^ &&
-	sed -ne "/^[^#]/{s/[0-9a-f]\{7,\}/HASH/g;p}" tmp >actual &&
+	sed -ne "/^[^#]/{s/[0-9a-f]\{7,\}/HASH/g;p;}" tmp >actual &&
 	cat <<-EOF >expect &&
 	pick HASH wip commit (just a prefix match so overshadowed by branch) # empty
 	pick HASH tip of wip # empty
@@ -268,7 +268,7 @@ test_expect_success 'auto squash of fixup commit that matches branch name which
 	git commit --allow-empty -m "fixup! self-cycle" &&
 	git branch self-cycle &&
 	GIT_SEQUENCE_EDITOR="cat >tmp" git rebase --autosquash -i HEAD^^ &&
-	sed -ne "/^[^#]/{s/[0-9a-f]\{7,\}/HASH/g;p}" tmp >actual &&
+	sed -ne "/^[^#]/{s/[0-9a-f]\{7,\}/HASH/g;p;}" tmp >actual &&
 	cat <<-EOF >expect &&
 	pick HASH second commit
 	pick HASH fixup! self-cycle # empty
-- 
2.38.0-rc0-146-g9b794391bb

Re: [PATCH] sequencer: avoid dropping fixup commit that targets self via commit-ish

[Johannes Altmanninger]

On Mon, Sep 19, 2022 at 04:09:57PM -0700, Junio C Hamano wrote:
> Johannes Altmanninger <aclopte@gmail.com> writes:
> 
> > +test_expect_success 'auto squash of fixup commit that matches branch name which points back to fixup commit' '
> > ...
> > +	sed -ne "/^[^#]/{s/[0-9a-f]\{7,\}/HASH/g;p}" tmp >actual &&

FWIW I copied and adapted this one from the remerge-diff tests.  Not sure
what other tests use. Maybe the HASH replacement is worth a test helper even
though it is a heuristic that can go wrong.