* [BUG] fixup commit is dropped during rebase if subject = branch name
@ 2022-09-17 14:45 Erik Cervin Edin
2022-09-17 18:04 ` Junio C Hamano
2022-09-17 23:19 ` Johannes Altmanninger
0 siblings, 2 replies; 19+ messages in thread
From: Erik Cervin Edin @ 2022-09-17 14:45 UTC (permalink / raw)
To: Git Mailing List
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [BUG] fixup commit is dropped during rebase if subject = branch name
2022-09-17 14:45 [BUG] fixup commit is dropped during rebase if subject = branch name Erik Cervin Edin
@ 2022-09-17 18:04 ` Junio C Hamano
2022-09-18 14:55 ` Erik Cervin Edin
2022-09-17 23:19 ` Johannes Altmanninger
1 sibling, 1 reply; 19+ messages in thread
From: Junio C Hamano @ 2022-09-17 18:04 UTC (permalink / raw)
To: Erik Cervin Edin; +Cc: Git Mailing List
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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [BUG] fixup commit is dropped during rebase if subject = branch name
2022-09-17 14:45 [BUG] fixup commit is dropped during rebase if subject = branch name Erik Cervin Edin
2022-09-17 18:04 ` Junio C Hamano
@ 2022-09-17 23:19 ` Johannes Altmanninger
2022-09-18 12:10 ` [PATCH] sequencer: avoid dropping fixup commit that targets self via commit-ish Johannes Altmanninger
1 sibling, 1 reply; 19+ messages in thread
From: Johannes Altmanninger @ 2022-09-17 23:19 UTC (permalink / raw)
To: Erik Cervin Edin; +Cc: Git Mailing List
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))
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH] sequencer: avoid dropping fixup commit that targets self via commit-ish
2022-09-17 23:19 ` Johannes Altmanninger
@ 2022-09-18 12:10 ` Johannes Altmanninger
2022-09-18 15:05 ` Erik Cervin Edin
` (2 more replies)
0 siblings, 3 replies; 19+ messages in thread
From: Johannes Altmanninger @ 2022-09-18 12:10 UTC (permalink / raw)
To: Erik Cervin Edin, Junio C Hamano; +Cc: git, 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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [BUG] fixup commit is dropped during rebase if subject = branch name
2022-09-17 18:04 ` Junio C Hamano
@ 2022-09-18 14:55 ` Erik Cervin Edin
0 siblings, 0 replies; 19+ messages in thread
From: Erik Cervin Edin @ 2022-09-18 14:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
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!
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] sequencer: avoid dropping fixup commit that targets self via commit-ish
2022-09-18 12:10 ` [PATCH] sequencer: avoid dropping fixup commit that targets self via commit-ish Johannes Altmanninger
@ 2022-09-18 15:05 ` Erik Cervin Edin
2022-09-18 17:54 ` Johannes Altmanninger
2022-09-19 1:11 ` Junio C Hamano
2022-09-19 23:09 ` [PATCH] " Junio C Hamano
2 siblings, 1 reply; 19+ messages in thread
From: Erik Cervin Edin @ 2022-09-18 15:05 UTC (permalink / raw)
To: Johannes Altmanninger; +Cc: Junio C Hamano, git
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?
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] sequencer: avoid dropping fixup commit that targets self via commit-ish
2022-09-18 15:05 ` Erik Cervin Edin
@ 2022-09-18 17:54 ` Johannes Altmanninger
0 siblings, 0 replies; 19+ messages in thread
From: Johannes Altmanninger @ 2022-09-18 17:54 UTC (permalink / raw)
To: Erik Cervin Edin; +Cc: Junio C Hamano, git
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] sequencer: avoid dropping fixup commit that targets self via commit-ish
2022-09-18 12:10 ` [PATCH] sequencer: avoid dropping fixup commit that targets self via commit-ish Johannes Altmanninger
2022-09-18 15:05 ` Erik Cervin Edin
@ 2022-09-19 1:11 ` Junio C Hamano
2022-09-19 16:07 ` Junio C Hamano
2022-09-19 16:23 ` Junio C Hamano
2022-09-19 23:09 ` [PATCH] " Junio C Hamano
2 siblings, 2 replies; 19+ messages in thread
From: Junio C Hamano @ 2022-09-19 1:11 UTC (permalink / raw)
To: Johannes Altmanninger; +Cc: Erik Cervin Edin, git
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 &&
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] sequencer: avoid dropping fixup commit that targets self via commit-ish
2022-09-19 1:11 ` Junio C Hamano
@ 2022-09-19 16:07 ` Junio C Hamano
2022-09-20 3:20 ` Johannes Altmanninger
2022-09-19 16:23 ` Junio C Hamano
1 sibling, 1 reply; 19+ messages in thread
From: Junio C Hamano @ 2022-09-19 16:07 UTC (permalink / raw)
To: Johannes Altmanninger; +Cc: Erik Cervin Edin, git
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".
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] sequencer: avoid dropping fixup commit that targets self via commit-ish
2022-09-19 1:11 ` Junio C Hamano
2022-09-19 16:07 ` Junio C Hamano
@ 2022-09-19 16:23 ` Junio C Hamano
2022-09-20 3:11 ` [PATCH v2] " Johannes Altmanninger
1 sibling, 1 reply; 19+ messages in thread
From: Junio C Hamano @ 2022-09-19 16:23 UTC (permalink / raw)
To: Johannes Altmanninger; +Cc: Erik Cervin Edin, git
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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] sequencer: avoid dropping fixup commit that targets self via commit-ish
2022-09-18 12:10 ` [PATCH] sequencer: avoid dropping fixup commit that targets self via commit-ish Johannes Altmanninger
2022-09-18 15:05 ` Erik Cervin Edin
2022-09-19 1:11 ` Junio C Hamano
@ 2022-09-19 23:09 ` Junio C Hamano
2022-09-20 3:27 ` Johannes Altmanninger
2 siblings, 1 reply; 19+ messages in thread
From: Junio C Hamano @ 2022-09-19 23:09 UTC (permalink / raw)
To: Johannes Altmanninger; +Cc: Erik Cervin Edin, git
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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v2] sequencer: avoid dropping fixup commit that targets self via commit-ish
2022-09-19 16:23 ` Junio C Hamano
@ 2022-09-20 3:11 ` Johannes Altmanninger
2022-09-20 8:26 ` Phillip Wood
2022-09-21 18:47 ` Junio C Hamano
0 siblings, 2 replies; 19+ messages in thread
From: Johannes Altmanninger @ 2022-09-20 3:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Erik Cervin Edin, git, 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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] sequencer: avoid dropping fixup commit that targets self via commit-ish
2022-09-19 16:07 ` Junio C Hamano
@ 2022-09-20 3:20 ` Johannes Altmanninger
0 siblings, 0 replies; 19+ messages in thread
From: Johannes Altmanninger @ 2022-09-20 3:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Erik Cervin Edin, git
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] sequencer: avoid dropping fixup commit that targets self via commit-ish
2022-09-19 23:09 ` [PATCH] " Junio C Hamano
@ 2022-09-20 3:27 ` Johannes Altmanninger
0 siblings, 0 replies; 19+ messages in thread
From: Johannes Altmanninger @ 2022-09-20 3:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Erik Cervin Edin, git
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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] sequencer: avoid dropping fixup commit that targets self via commit-ish
2022-09-20 3:11 ` [PATCH v2] " Johannes Altmanninger
@ 2022-09-20 8:26 ` Phillip Wood
2022-09-21 18:47 ` Junio C Hamano
1 sibling, 0 replies; 19+ messages in thread
From: Phillip Wood @ 2022-09-20 8:26 UTC (permalink / raw)
To: Johannes Altmanninger, Junio C Hamano; +Cc: Erik Cervin Edin, git
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 &&
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] sequencer: avoid dropping fixup commit that targets self via commit-ish
2022-09-20 3:11 ` [PATCH v2] " Johannes Altmanninger
2022-09-20 8:26 ` Phillip Wood
@ 2022-09-21 18:47 ` Junio C Hamano
2022-09-22 4:00 ` Johannes Altmanninger
1 sibling, 1 reply; 19+ messages in thread
From: Junio C Hamano @ 2022-09-21 18:47 UTC (permalink / raw)
To: Johannes Altmanninger; +Cc: Erik Cervin Edin, git
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-<.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] sequencer: avoid dropping fixup commit that targets self via commit-ish
2022-09-21 18:47 ` Junio C Hamano
@ 2022-09-22 4:00 ` Johannes Altmanninger
2022-09-22 19:32 ` Junio C Hamano
0 siblings, 1 reply; 19+ messages in thread
From: Johannes Altmanninger @ 2022-09-22 4:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Erik Cervin Edin, git
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).
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] sequencer: avoid dropping fixup commit that targets self via commit-ish
2022-09-22 4:00 ` Johannes Altmanninger
@ 2022-09-22 19:32 ` Junio C Hamano
2022-09-24 22:29 ` [PATCH v3] " Johannes Altmanninger
0 siblings, 1 reply; 19+ messages in thread
From: Junio C Hamano @ 2022-09-22 19:32 UTC (permalink / raw)
To: Johannes Altmanninger; +Cc: Erik Cervin Edin, git
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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v3] sequencer: avoid dropping fixup commit that targets self via commit-ish
2022-09-22 19:32 ` Junio C Hamano
@ 2022-09-24 22:29 ` Johannes Altmanninger
0 siblings, 0 replies; 19+ messages in thread
From: Johannes Altmanninger @ 2022-09-24 22:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Erik Cervin Edin, git, 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
^ permalink raw reply related [flat|nested] 19+ messages in thread
end of thread, other threads:[~2022-09-24 22:29 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-09-17 14:45 [BUG] fixup commit is dropped during rebase if subject = branch name Erik Cervin Edin
2022-09-17 18:04 ` Junio C Hamano
2022-09-18 14:55 ` Erik Cervin Edin
2022-09-17 23:19 ` Johannes Altmanninger
2022-09-18 12:10 ` [PATCH] sequencer: avoid dropping fixup commit that targets self via commit-ish Johannes Altmanninger
2022-09-18 15:05 ` Erik Cervin Edin
2022-09-18 17:54 ` Johannes Altmanninger
2022-09-19 1:11 ` Junio C Hamano
2022-09-19 16:07 ` Junio C Hamano
2022-09-20 3:20 ` Johannes Altmanninger
2022-09-19 16:23 ` Junio C Hamano
2022-09-20 3:11 ` [PATCH v2] " Johannes Altmanninger
2022-09-20 8:26 ` Phillip Wood
2022-09-21 18:47 ` Junio C Hamano
2022-09-22 4:00 ` Johannes Altmanninger
2022-09-22 19:32 ` Junio C Hamano
2022-09-24 22:29 ` [PATCH v3] " Johannes Altmanninger
2022-09-19 23:09 ` [PATCH] " Junio C Hamano
2022-09-20 3:27 ` Johannes Altmanninger
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).