Johannes Altmanninger <aclopte@xxxxxxxxx> 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@xxxxxxxxxxx>
> Signed-off-by: Johannes Altmanninger <aclopte@xxxxxxxxx>
> ---
> 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 &&
