{"thread":{"id":"58451","subject":"[BUG] fixup commit is dropped during rebase if subject = branch name","startedAt":"2022-09-17T14:46:00Z","lastAt":"2022-09-24T22:29:45Z","messageCount":19,"participants":["Erik Cervin Edin","Junio C Hamano","Johannes Altmanninger","Phillip Wood"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"463140","messageId":"CA+JQ7M_Xwxa48ggu88rhA9dG6R3u820Tgu8B2Kg-uMbEVjy3Vg@mail.gmail.com","threadId":"58451","inReplyTo":null,"subject":"[BUG] fixup commit is dropped during rebase if subject = branch name","fromName":"Erik Cervin Edin","fromEmail":"erik@cervined.in","sentAt":"2022-09-17T14:45:17Z","receivedAt":"2022-09-17T14:46:00Z","isPatch":false,"sender":{"key":"erik@cervined.in","avatar":null},"body":"Thank you for filling out a Git bug report!\nPlease answer the following questions to help us understand your issue.\n\nWhat did you do before the bug happened? (Steps to reproduce your issue)\n dir=rebase-fixup-subject-equals-branch-name\n mkdir $dir\n cd $dir\n git init --initial-branch=main\n git commit -m init --allow-empty\n git tag init\n\n # failure\n seq 1 3 >> bar && git add bar && git commit -m main\n git tag -f x\n seq 4 6 >> bar && git add bar && git commit -m bar\n seq 7 9 >> bar && git add bar && git commit --fixup :/main\n git -c sequence.editor=: rebase --autosquash --interactive x\n git diff ORIG_HEAD\n\n\nWhat did you expect to happen? (Expected behavior)\nIdentical results as\n # normal\n git reset --hard init\n seq 1 3 >> bar && git add bar && git commit -m main\n git tag -f x\n seq 4 6 >> bar && git add bar && git commit --fixup :/main\n seq 7 9 >> bar && git add bar && git commit -m bar\n git -c sequence.editor=: rebase --autosquash --interactive x\n git diff ORIG_HEAD\nexcept for the order of the commits.\n\nWhat happened instead? (Actual behavior)\nThe fixup commit was dropped from the rebase-todo.\n\nWhat's different between what you expected and what actually happened?\nThe HEAD is a fixup of a commit with the same subject as the branch\nname. It is being rebased on top of the commit it is intended to\nfixup, so it should be *picked*. Instead, it is dropped from the\nrebase-todo. I expected the autosquash feature to work equally,\nindependently of the subject and the current branch name.\n\nAnything else you want to add:\nThis appears to only occur if the subject is equal to the branch being\nrebased. I didn't check if this occurs if there is simply 'a'\nreference with an identical name in the repository (I'm guessing no).\n\nPlease review the rest of the bug report below.\nYou can delete any lines you don't wish to share.\n\n\n[System Info]\ngit version:\ngit version 2.37.3.windows.1\ncpu: x86_64\nbuilt from commit: c4992d4fecabd7d111726ecb37e33a3ccb51d6f1\nsizeof-long: 4\nsizeof-size_t: 8\nshell-path: /bin/sh\nfeature: fsmonitor--daemon\nuname: Windows 10.0 19044\ncompiler info: gnuc: 12.2\nlibc info: no libc information available\n$SHELL (typically, interactive shell): C:\\Users\\erik\\Git\\usr\\bin\\bash.exe\n\n\n[Enabled Hooks]\n\n\n-- \nErik Cervin-Edin\n"},{"id":"463142","messageId":"xmqqpmftev3c.fsf@gitster.g","threadId":"58451","inReplyTo":"CA+JQ7M_Xwxa48ggu88rhA9dG6R3u820Tgu8B2Kg-uMbEVjy3Vg@mail.gmail.com","subject":"Re: [BUG] fixup commit is dropped during rebase if subject = branch name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-17T18:04:07Z","receivedAt":"2022-09-17T18:04:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erik Cervin Edin writes:\n\n> git init --initial-branch=main\n> git commit -m init --allow-empty\n> git tag init\n\nThanks for a report. Let me quickly respond with an initial\nreaction without being in front of a real computer to run tests on\nmyself. Others may give more useful feedback.\n\n> # failure\n> seq 1 3 >> bar && git add bar && git commit -m main\n> git tag -f x\n> seq 4 6 >> bar && git add bar && git commit -m bar\n> seq 7 9 >> bar && git add bar && git commit --fixup :/main\n> git -c sequence.editor=: rebase --autosquash --interactive x\n> git diff ORIG_HEAD\n\nSo near the bottom there are \"init\", and \"x\". The commit title of\n\"x\" is \"main\" and that is what the fix-up intends to amend.\n\nBut then I do not think there is any valid expectation if you say\n\"keep x intact and rebase everything above\", which is what the\ncommand line arguments tell the last command to do. Perhaps we\nshould keep all original commits up to that \"fixup\" one without any\nreordering or squashing?\n\nThe title of your bug report is also curious. What happens if you\ndid\n\n git branch -m master\n\njust before running the \"rebase\" command in the above sequence? I\nwould have expected to see that in your \"expected\" section to\ncontrast the behaviour between \"if subject = branch\" vs \"if subject\n!= branch\", and the report looks a bit puzzling.\n\nTHanks.\n"},{"id":"463144","messageId":"YyZWDkZWAkS7q+Wf@gmail.com","threadId":"58451","inReplyTo":"CA+JQ7M_Xwxa48ggu88rhA9dG6R3u820Tgu8B2Kg-uMbEVjy3Vg@mail.gmail.com","subject":"Re: [BUG] fixup commit is dropped during rebase if subject = branch name","fromName":"Johannes Altmanninger","fromEmail":"aclopte@gmail.com","sentAt":"2022-09-17T23:19:42Z","receivedAt":"2022-09-17T23:21:05Z","isPatch":false,"sender":{"key":"aclopte@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6853872?v=4"},"body":"On Sat, Sep 17, 2022 at 04:45:17PM +0200, Erik Cervin Edin wrote:\n> Thank you for filling out a Git bug report!\n> Please answer the following questions to help us understand your issue.\n> \n> What did you do before the bug happened? (Steps to reproduce your issue)\n> dir=rebase-fixup-subject-equals-branch-name\n> mkdir $dir\n> cd $dir\n> git init --initial-branch=main\n> git commit -m init --allow-empty\n> git tag init\n> \n> # failure\n> seq 1 3 >> bar && git add bar && git commit -m main\n> git tag -f x\n> seq 4 6 >> bar && git add bar && git commit -m bar\n> seq 7 9 >> bar && git add bar && git commit --fixup :/main\n> git -c sequence.editor=: rebase --autosquash --interactive x\n\nHuh, this silently discards the fixup commit, without applying it.\n\nIf \"foo\" is a valid refspec, then the autosquash machinery will apply to it\nall fixup commits with subject \"fixup! foo\".\nThe problem you hit is that \"foo\" points to the fixup commit itself - which\nis the only destination commit that definitely won't work.\n\nHere is a possible fix:\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 79dad522f5..7cbd8c2595 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -6231,3 +6231,3 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)\n \t\t\t\t (commit2 =\n-\t\t\t\t lookup_commit_reference_by_name(p)) &&\n+\t\t\t\t lookup_commit_reference_by_name(p)) != item->commit &&\n \t\t\t\t *commit_todo_item_at(&commit_todo, commit2))\n"},{"id":"463151","messageId":"20220918121053.880225-1-aclopte@gmail.com","threadId":"58451","inReplyTo":"YyZWDkZWAkS7q+Wf@gmail.com","subject":"[PATCH] sequencer: avoid dropping fixup commit that targets self via commit-ish","fromName":"Johannes Altmanninger","fromEmail":"aclopte@gmail.com","sentAt":"2022-09-18T12:10:53Z","receivedAt":"2022-09-18T12:11:23Z","isPatch":true,"sender":{"key":"aclopte@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6853872?v=4"},"body":"Commit 68d5d03bc4 (rebase: teach --autosquash to match on sha1 in\naddition to message, 2010-11-04) made --autosquash apply a commit\nwith subject \"fixup! 012345\" to the first commit in the todo list\nwhose OID starts with 012345. So far so good.\n\nMore recently, c44a4c650c (rebase -i: rearrange fixup/squash lines\nusing the rebase--helper, 2017-07-14) reimplemented this logic in C\nand introduced two behavior changes.\nFirst, OID matches are given precedence over subject prefix\nmatches. Second, instead of prefix-matching OIDs, we use\nlookup_commit_reference_by_name(). This means that if 012345 is a\nbranch name, we will apply the fixup commit to the tip of that branch\n(if that is present in the todo list).\n\nBoth behavior changes might be motivated by performance concerns\n(since the commit message mentions performance). Looking through\nthe todo list to find a commit that matches the given prefix can be\nmore expensive than looking up an OID. The runtime of the former is\nof O(n*m) where n is the size of the todo list and m is the length\nof a commit subject. However, if this is really a problem, we could\neasily make it O(m) by constructing a trie (prefix tree).\n\nDemonstrate both behavior changes by adding two test cases for\n\"fixup! foo\" where foo is a commit-ish that is not an OID-prefix.\nArguably, this feature is very weird. If no one uses it we should\nconsider removing it.\n\nRegardless, there is one bad edge case to fix. Let refspec \"foo\" point\nto a commit with the subject \"fixup! foo\". Since rebase --autosquash\nfinds the fixup target via lookup_commit_reference_by_name(), the\nfixup target is the fixup commit itself. Obviously this can't work.\nWe proceed with the broken invariant and drop the fixup commit\nentirely.\n\nThe self-fixup was only allowed because the fixup commit was already\nadded to the preliminary todo list, which it shouldn't be. Rather,\nwe should first compute the fixup target and only then add the fixup\ncommit to the todo list. Make it so, avoiding this error by design,\nand add a third test for this case.\n\nReported-by: Erik Cervin Edin \nSigned-off-by: Johannes Altmanninger \n---\n sequencer.c | 4 ++--\n t/t3415-rebase-autosquash.sh | 44 ++++++++++++++++++++++++++++++++++++\n 2 files changed, 46 insertions(+), 2 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 484ca9aa50..777200a6dc 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -6287,8 +6287,6 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)\n \t\t\treturn error(_(\"the script was already rearranged.\"));\n \t\t}\n \n-\t\t*commit_todo_item_at(&commit_todo, item->commit) = item;\n-\n \t\tparse_commit(item->commit);\n \t\tcommit_buffer = logmsg_reencode(item->commit, NULL, \"UTF-8\");\n \t\tfind_commit_subject(commit_buffer, &subject);\n@@ -6355,6 +6353,8 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)\n \t\t\t\t\tstrhash(entry->subject));\n \t\t\thashmap_put(&subject2item, &entry->entry);\n \t\t}\n+\n+\t\t*commit_todo_item_at(&commit_todo, item->commit) = item;\n \t}\n \n \tif (rearranged) {\ndiff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh\nindex 78c27496d6..879e628512 100755\n--- a/t/t3415-rebase-autosquash.sh\n+++ b/t/t3415-rebase-autosquash.sh\n@@ -232,6 +232,50 @@ test_expect_success 'auto squash that matches longer sha1' '\n \ttest_line_count = 1 actual\n '\n \n+test_expect_success 'auto squash that matches regex' '\n+\tgit reset --hard base &&\n+\tgit commit --allow-empty -m \"hay needle hay\" &&\n+\tgit commit --allow-empty -m \"fixup! :/[n]eedle\" &&\n+\tGIT_SEQUENCE_EDITOR=\"cat >tmp\" git rebase --autosquash -i HEAD^^ &&\n+\tsed -ne \"/^[^#]/{s/[0-9a-f]\\{7,\\}/HASH/g;p}\" tmp >actual &&\n+\tcat <<-EOF >expect &&\n+\tpick HASH hay needle hay # empty\n+\tfixup HASH fixup! :/[n]eedle # empty\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'auto squash of fixup commit that matches branch name' '\n+\tgit reset --hard base &&\n+\tgit commit --allow-empty -m \"wip commit (just a prefix match so overshadowed by branch)\" &&\n+\tgit commit --allow-empty -m \"tip of wip\" &&\n+\tgit branch wip &&\n+\tgit commit --allow-empty -m \"unrelated commit\" &&\n+\tgit commit --allow-empty -m \"fixup! wip\" &&\n+\tGIT_SEQUENCE_EDITOR=\"cat >tmp\" git rebase --autosquash -i HEAD^^^^ &&\n+\tsed -ne \"/^[^#]/{s/[0-9a-f]\\{7,\\}/HASH/g;p}\" tmp >actual &&\n+\tcat <<-EOF >expect &&\n+\tpick HASH wip commit (just a prefix match so overshadowed by branch) # empty\n+\tpick HASH tip of wip # empty\n+\tfixup HASH fixup! wip # empty\n+\tpick HASH unrelated commit # empty\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'auto squash of fixup commit that matches branch name which points back to fixup commit' '\n+\tgit reset --hard base &&\n+\tgit commit --allow-empty -m \"fixup! self-cycle\" &&\n+\tgit branch self-cycle &&\n+\tGIT_SEQUENCE_EDITOR=\"cat >tmp\" git rebase --autosquash -i HEAD^^ &&\n+\tsed -ne \"/^[^#]/{s/[0-9a-f]\\{7,\\}/HASH/g;p}\" tmp >actual &&\n+\tcat <<-EOF >expect &&\n+\tpick HASH second commit\n+\tpick HASH fixup! self-cycle # empty\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_auto_commit_flags () {\n \tgit reset --hard base &&\n \techo 1 >file1 &&\n-- \n2.37.3.830.gf65be7a4d6\n\n"},{"id":"463153","messageId":"CA+JQ7M9_o-0W1orXnPRGzoLziE0HBNkLe0HugGNFKxy0LPsgXA@mail.gmail.com","threadId":"58451","inReplyTo":"xmqqpmftev3c.fsf@gitster.g","subject":"Re: [BUG] fixup commit is dropped during rebase if subject = branch name","fromName":"Erik Cervin Edin","fromEmail":"erik@cervined.in","sentAt":"2022-09-18T14:55:21Z","receivedAt":"2022-09-18T14:56:05Z","isPatch":false,"sender":{"key":"erik@cervined.in","avatar":null},"body":"On Sat, Sep 17, 2022, 8:04 PM Junio C Hamano wrote:\n>\n> > # failure\n> > seq 1 3 >> bar && git add bar && git commit -m main\n> > git tag -f x\n> > seq 4 6 >> bar && git add bar && git commit -m bar\n> > seq 7 9 >> bar && git add bar && git commit --fixup :/main\n> > git -c sequence.editor=: rebase --autosquash --interactive x\n> > git diff ORIG_HEAD\n>\n> So near the bottom there are \"init\", and \"x\". The commit title of\n> \"x\" is \"main\" and that is what the fix-up intends to amend.\n\nYes, sorry if that was unclear. So the branch looks like\n\n 474b082 (HEAD -> main) fixup! main\n acd367f bar\n 576294e (tag: x) main\n dd27847 (tag: init) init\n\nso\n git -c sequence.editor rebase -i --autosquash x\nshould yield\n\n 474b082 (HEAD -> main) fixup! main\n acd367f bar\n 576294e (tag: x) main\n dd27847 (tag: init) init\n\nie. no change, but instead yields\n\n acd367f (HEAD -> main) bar\n 576294e (tag: x) main\n dd27847 (tag: init) init\n\nie. it drops the fixup! main commit but only if it's the first commit, see\nsecond example I posted\n\nOn Sat, Sep 17, 2022 at 4:45 PM Erik Cervin Edin wrote:\n> # normal\n> git reset --hard init\n> seq 1 3 >> bar && git add bar && git commit -m main\n> git tag -f x\n> seq 4 6 >> bar && git add bar && git commit --fixup :/main\n> seq 7 9 >> bar && git add bar && git commit -m bar\n> git -c sequence.editor=: rebase --autosquash --interactive x\n> git diff ORIG_HEAD\n\nyields\n\n acd367f (HEAD -> main) bar\n 474b082 fixup! main\n 576294e (tag: x) main\n dd27847 (tag: init) init\n\nas expected.\n\n> But then I do not think there is any valid expectation if you say\n> \"keep x intact and rebase everything above\", which is what the\n> command line arguments tell the last command to do. Perhaps we\n> should keep all original commits up to that \"fixup\" one without any\n> reordering or squashing?\n\nI'm not sure I follow but the report is concerning the unexpected\nbehavior of dropping the commit under these specific conditions. I'm\nnot 100% sure but as I recall, this only happens in situation where\nthe fixup may not be applied (and in which case it should remain as\nis).\n\n> The title of your bug report is also curious. What happens if you\n> did\n>\n> git branch -m master\n\nthat works as expected\n\n git reset --hard init\n seq 1 3 >> bar && git add bar && git commit -m main\n git tag -f x\n seq 4 6 >> bar && git add bar && git commit -m bar\n seq 7 9 >> bar && git add bar && git commit --fixup :/main\n git branch -m master\n git -c sequence.editor=: rebase --autosquash --interactive x\n\nbut changing branch -m to switch -c reproduces the bug\n\n 6650ace (HEAD -> master) bar\n 7835cd5 (tag: x) main\n 368ed2f (tag: init) init\n\nHope this better clarifies what is going on!\n"},{"id":"463154","messageId":"CA+JQ7M8JX64M2m=+-NTnbRo64jy7_G=TYafj6UXv=R7UJo0vdQ@mail.gmail.com","threadId":"58451","inReplyTo":"20220918121053.880225-1-aclopte@gmail.com","subject":"Re: [PATCH] sequencer: avoid dropping fixup commit that targets self via commit-ish","fromName":"Erik Cervin Edin","fromEmail":"erik@cervined.in","sentAt":"2022-09-18T15:05:29Z","receivedAt":"2022-09-18T15:06:11Z","isPatch":true,"sender":{"key":"erik@cervined.in","avatar":null},"body":"Thank you for the explanation!\n\nOn Sun, Sep 18, 2022 at 2:11 PM Johannes Altmanninger wrote:\n>\n> Commit 68d5d03bc4 (rebase: teach --autosquash to match on sha1 in\n> addition to message, 2010-11-04) made --autosquash apply a commit\n> with subject \"fixup! 012345\" to the first commit in the todo list\n> whose OID starts with 012345. So far so good.\n\nI wasn't aware such fixups were recognized. The only way to create\nsuch fixups is manually, correct?\n"},{"id":"463156","messageId":"YydbSeX7g/ldeq8U@gmail.com","threadId":"58451","inReplyTo":"CA+JQ7M8JX64M2m=+-NTnbRo64jy7_G=TYafj6UXv=R7UJo0vdQ@mail.gmail.com","subject":"Re: [PATCH] sequencer: avoid dropping fixup commit that targets self via commit-ish","fromName":"Johannes Altmanninger","fromEmail":"aclopte@gmail.com","sentAt":"2022-09-18T17:54:17Z","receivedAt":"2022-09-18T17:54:46Z","isPatch":true,"sender":{"key":"aclopte@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6853872?v=4"},"body":"On Sun, Sep 18, 2022 at 05:05:29PM +0200, Erik Cervin Edin wrote:\n> Thank you for the explanation!\n> \n> On Sun, Sep 18, 2022 at 2:11 PM Johannes Altmanninger wrote:\n> >\n> > Commit 68d5d03bc4 (rebase: teach --autosquash to match on sha1 in\n> > addition to message, 2010-11-04) made --autosquash apply a commit\n> > with subject \"fixup! 012345\" to the first commit in the todo list\n> > whose OID starts with 012345. So far so good.\n> \n> I wasn't aware such fixups were recognized.\n\nNeither was I. I've repeatedly had issues where the message created by\n\"commit --fixup\" was ambiguous (user error I know) but I can change my tool\nto use OIDs for the cases when I squash them right away..\n\n> The only way to create\n> such fixups is manually, correct?\n\nI think so\n"},{"id":"463160","messageId":"xmqqh714dv7n.fsf@gitster.g","threadId":"58451","inReplyTo":"20220918121053.880225-1-aclopte@gmail.com","subject":"Re: [PATCH] sequencer: avoid dropping fixup commit that targets self via commit-ish","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-19T01:11:24Z","receivedAt":"2022-09-19T01:11:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Altmanninger writes:\n\n> First, OID matches are given precedence over subject prefix\n> matches. Second, instead of prefix-matching OIDs, we use\n> lookup_commit_reference_by_name(). This means that if 012345 is a\n> branch name, we will apply the fixup commit to the tip of that branch\n> (if that is present in the todo list).\n\nGood finding.\n\nI think that both are results from imprecise conversion from the\noriginal done carelessly. A rewrite does not necessarily have to be\nbug-to-bug equivalent, and the precedence between object names vs\nsubject prefix is something I think does not have to be kept the\nsame as the original (in other words, a user who gives a token after\n\"fixup\" that is ambiguous between the two deserves whatever the\nimplementation happens to give). But use of _by_name() that does\nnot limit the input to hexadecimal _is_ a problem exactly for the\nreason why we have this discussion thread. The function accepts\nmore than we want to accept, and anybody who reads the original\ncommit 68d5d03b (rebase: teach --autosquash to match on sha1 in\naddition to message, 2010-11-04) and understands why we added it\nwouldn't have used it.\n\nYour solution looks somewhat surprising to me, as I would naively\nhave thought to fix the use of _by_name() and limit its use only\nwhen the input token is all hexadecimal, or something. I'd need to\nthink more to convince myself why this is the right solution.\n\nThanks.\n\n> Demonstrate both behavior changes by adding two test cases for\n> \"fixup! foo\" where foo is a commit-ish that is not an OID-prefix.\n> Arguably, this feature is very weird. If no one uses it we should\n> consider removing it.\n>\n> Regardless, there is one bad edge case to fix. Let refspec \"foo\" point\n> to a commit with the subject \"fixup! foo\". Since rebase --autosquash\n> finds the fixup target via lookup_commit_reference_by_name(), the\n> fixup target is the fixup commit itself. Obviously this can't work.\n> We proceed with the broken invariant and drop the fixup commit\n> entirely.\n>\n> The self-fixup was only allowed because the fixup commit was already\n> added to the preliminary todo list, which it shouldn't be. Rather,\n> we should first compute the fixup target and only then add the fixup\n> commit to the todo list. Make it so, avoiding this error by design,\n> and add a third test for this case.\n>\n> Reported-by: Erik Cervin Edin \n> Signed-off-by: Johannes Altmanninger \n> ---\n> sequencer.c | 4 ++--\n> t/t3415-rebase-autosquash.sh | 44 ++++++++++++++++++++++++++++++++++++\n> 2 files changed, 46 insertions(+), 2 deletions(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index 484ca9aa50..777200a6dc 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -6287,8 +6287,6 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)\n> \t\t\treturn error(_(\"the script was already rearranged.\"));\n> \t\t}\n> \n> -\t\t*commit_todo_item_at(&commit_todo, item->commit) = item;\n> -\n> \t\tparse_commit(item->commit);\n> \t\tcommit_buffer = logmsg_reencode(item->commit, NULL, \"UTF-8\");\n> \t\tfind_commit_subject(commit_buffer, &subject);\n> @@ -6355,6 +6353,8 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)\n> \t\t\t\t\tstrhash(entry->subject));\n> \t\t\thashmap_put(&subject2item, &entry->entry);\n> \t\t}\n> +\n> +\t\t*commit_todo_item_at(&commit_todo, item->commit) = item;\n> \t}\n> \n> \tif (rearranged) {\n> diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh\n> index 78c27496d6..879e628512 100755\n> --- a/t/t3415-rebase-autosquash.sh\n> +++ b/t/t3415-rebase-autosquash.sh\n> @@ -232,6 +232,50 @@ test_expect_success 'auto squash that matches longer sha1' '\n> \ttest_line_count = 1 actual\n> '\n> \n> +test_expect_success 'auto squash that matches regex' '\n> +\tgit reset --hard base &&\n> +\tgit commit --allow-empty -m \"hay needle hay\" &&\n> +\tgit commit --allow-empty -m \"fixup! :/[n]eedle\" &&\n> +\tGIT_SEQUENCE_EDITOR=\"cat >tmp\" git rebase --autosquash -i HEAD^^ &&\n> +\tsed -ne \"/^[^#]/{s/[0-9a-f]\\{7,\\}/HASH/g;p}\" tmp >actual &&\n> +\tcat <<-EOF >expect &&\n> +\tpick HASH hay needle hay # empty\n> +\tfixup HASH fixup! :/[n]eedle # empty\n> +\tEOF\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'auto squash of fixup commit that matches branch name' '\n> +\tgit reset --hard base &&\n> +\tgit commit --allow-empty -m \"wip commit (just a prefix match so overshadowed by branch)\" &&\n> +\tgit commit --allow-empty -m \"tip of wip\" &&\n> +\tgit branch wip &&\n> +\tgit commit --allow-empty -m \"unrelated commit\" &&\n> +\tgit commit --allow-empty -m \"fixup! wip\" &&\n> +\tGIT_SEQUENCE_EDITOR=\"cat >tmp\" git rebase --autosquash -i HEAD^^^^ &&\n> +\tsed -ne \"/^[^#]/{s/[0-9a-f]\\{7,\\}/HASH/g;p}\" tmp >actual &&\n> +\tcat <<-EOF >expect &&\n> +\tpick HASH wip commit (just a prefix match so overshadowed by branch) # empty\n> +\tpick HASH tip of wip # empty\n> +\tfixup HASH fixup! wip # empty\n> +\tpick HASH unrelated commit # empty\n> +\tEOF\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'auto squash of fixup commit that matches branch name which points back to fixup commit' '\n> +\tgit reset --hard base &&\n> +\tgit commit --allow-empty -m \"fixup! self-cycle\" &&\n> +\tgit branch self-cycle &&\n> +\tGIT_SEQUENCE_EDITOR=\"cat >tmp\" git rebase --autosquash -i HEAD^^ &&\n> +\tsed -ne \"/^[^#]/{s/[0-9a-f]\\{7,\\}/HASH/g;p}\" tmp >actual &&\n> +\tcat <<-EOF >expect &&\n> +\tpick HASH second commit\n> +\tpick HASH fixup! self-cycle # empty\n> +\tEOF\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_auto_commit_flags () {\n> \tgit reset --hard base &&\n> \techo 1 >file1 &&\n"},{"id":"463184","messageId":"xmqqpmfrcpq8.fsf@gitster.g","threadId":"58451","inReplyTo":"xmqqh714dv7n.fsf@gitster.g","subject":"Re: [PATCH] sequencer: avoid dropping fixup commit that targets self via commit-ish","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-19T16:07:27Z","receivedAt":"2022-09-19T16:07:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> ... But use of _by_name() that does\n> not limit the input to hexadecimal _is_ a problem ...\n\nAh, no, sorry, this was wrong. The original used \"rev-parse -q --verify\"\nwithout restricting the \"single word\" to \"sha1 prefix\".\n\n"},{"id":"463188","messageId":"xmqqleqfcoz3.fsf@gitster.g","threadId":"58451","inReplyTo":"xmqqh714dv7n.fsf@gitster.g","subject":"Re: [PATCH] sequencer: avoid dropping fixup commit that targets self via commit-ish","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-19T16:23:44Z","receivedAt":"2022-09-19T16:23:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Your solution looks somewhat surprising to me, as I would naively\n> have thought to fix the use of _by_name() and limit its use only\n> when the input token is all hexadecimal, or something. I'd need to\n> think more to convince myself why this is the right solution.\n\nOK, we try to find what to amend with the current \"fixupish\" from\nthe todo slab, which by definition must be something that we have\nalready dealt with. The current \"fixupish\" should not be found\nbecause we haven't finished dealing with it, so delaying the\naddition of the current one to the todo slab is a must.\n\nThere is no leaving or continuing the loop, other than an error\nreturn that aborts the whole thing, that may break the resulting\ntodo slab in the normal case due to this change, either. \n\nThe fix makes sense to me. Will queue.\n\nThanks.\n"},{"id":"463240","messageId":"xmqqmtav7ygq.fsf@gitster.g","threadId":"58451","inReplyTo":"20220918121053.880225-1-aclopte@gmail.com","subject":"Re: [PATCH] sequencer: avoid dropping fixup commit that targets self via commit-ish","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-19T23:09:57Z","receivedAt":"2022-09-19T23:10:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Altmanninger writes:\n\n> +test_expect_success 'auto squash of fixup commit that matches branch name which points back to fixup commit' '\n> ...\n> +\tsed -ne \"/^[^#]/{s/[0-9a-f]\\{7,\\}/HASH/g;p}\" tmp >actual &&\n\nThe construct is rejected by sed implementations of BSD descent, it\nseems.\n\nhttps://github.com/git/git/actions/runs/3084784922/jobs/4987337058#step:4:1844\n\n++ sed -ne '/^[^#]/{s/[0-9a-f]\\{7,\\}/HASH/g;p}' tmp\nsed: 1: \"/^[^#]/{s/[0-9a-f]\\{7,\\ ...\": extra characters at the end of p command\nerror: last command exited with $?=1\nnot ok 11 - auto squash that matches regex\n\nHere is a fix-up that can be squashed in.\n\nThanks.\n\n t/t3415-rebase-autosquash.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh\nindex 879e628512..d65d2258c3 100755\n--- a/t/t3415-rebase-autosquash.sh\n+++ b/t/t3415-rebase-autosquash.sh\n@@ -237,7 +237,7 @@ test_expect_success 'auto squash that matches regex' '\n \tgit commit --allow-empty -m \"hay needle hay\" &&\n \tgit commit --allow-empty -m \"fixup! :/[n]eedle\" &&\n \tGIT_SEQUENCE_EDITOR=\"cat >tmp\" git rebase --autosquash -i HEAD^^ &&\n-\tsed -ne \"/^[^#]/{s/[0-9a-f]\\{7,\\}/HASH/g;p}\" tmp >actual &&\n+\tsed -ne \"/^[^#]/{s/[0-9a-f]\\{7,\\}/HASH/g;p;}\" tmp >actual &&\n \tcat <<-EOF >expect &&\n \tpick HASH hay needle hay # empty\n \tfixup HASH fixup! :/[n]eedle # empty\n@@ -253,7 +253,7 @@ test_expect_success 'auto squash of fixup commit that matches branch name' '\n \tgit commit --allow-empty -m \"unrelated commit\" &&\n \tgit commit --allow-empty -m \"fixup! wip\" &&\n \tGIT_SEQUENCE_EDITOR=\"cat >tmp\" git rebase --autosquash -i HEAD^^^^ &&\n-\tsed -ne \"/^[^#]/{s/[0-9a-f]\\{7,\\}/HASH/g;p}\" tmp >actual &&\n+\tsed -ne \"/^[^#]/{s/[0-9a-f]\\{7,\\}/HASH/g;p;}\" tmp >actual &&\n \tcat <<-EOF >expect &&\n \tpick HASH wip commit (just a prefix match so overshadowed by branch) # empty\n \tpick HASH tip of wip # empty\n@@ -268,7 +268,7 @@ test_expect_success 'auto squash of fixup commit that matches branch name which\n \tgit commit --allow-empty -m \"fixup! self-cycle\" &&\n \tgit branch self-cycle &&\n \tGIT_SEQUENCE_EDITOR=\"cat >tmp\" git rebase --autosquash -i HEAD^^ &&\n-\tsed -ne \"/^[^#]/{s/[0-9a-f]\\{7,\\}/HASH/g;p}\" tmp >actual &&\n+\tsed -ne \"/^[^#]/{s/[0-9a-f]\\{7,\\}/HASH/g;p;}\" tmp >actual &&\n \tcat <<-EOF >expect &&\n \tpick HASH second commit\n \tpick HASH fixup! self-cycle # empty\n-- \n2.38.0-rc0-146-g9b794391bb\n\n"},{"id":"463267","messageId":"20220920031140.1220220-1-aclopte@gmail.com","threadId":"58451","inReplyTo":"xmqqleqfcoz3.fsf@gitster.g","subject":"[PATCH v2] sequencer: avoid dropping fixup commit that targets self via commit-ish","fromName":"Johannes Altmanninger","fromEmail":"aclopte@gmail.com","sentAt":"2022-09-20T03:11:40Z","receivedAt":"2022-09-20T03:12:30Z","isPatch":true,"sender":{"key":"aclopte@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6853872?v=4"},"body":"Commit 68d5d03bc4 (rebase: teach --autosquash to match on sha1 in\naddition to message, 2010-11-04) taught autosquash to recognize\nsubjects like \"fixup! 7a235b\" where 7a235b is an OID-prefix. It\nactually did more than advertised: 7a235b can be an arbitrary\ncommit-ish (as long as it's not trailed by spaces).\n\nAccidental(?) use of this secret feature revealed a bug where we\nwould silently drop a fixup commit. The bug can also be triggered\nwhen using an OID-prefix but that's unlikely in practice.\n\nGiven a commit with subject \"fixup! main\" that is the tip of the\nbranch \"main\". When computing the fixup target for this commit, we\nfind the commit itself. This is wrong because, by definition, a fixup\ntarget must be an earlier commit in the todo list. We wrongly find\nthe current commit because we added it to the todo list prematurely.\nAvoid these fixup-cycles by only adding the current commit after we\nhave finished finding its target.\n\nReported-by: Erik Cervin Edin \nSigned-off-by: Johannes Altmanninger \n---\n sequencer.c | 4 ++--\n t/t3415-rebase-autosquash.sh | 26 ++++++++++++++++++++++++++\n 2 files changed, 28 insertions(+), 2 deletions(-)\n\nChanges to v1.\n- Remove wrong assumptions from commit message. The commit message should\n be clearer now (though I didn't spend too much time on it).\n- Drop one test because it's not related to the fix (and doesn't test anything\n I care about) and modify the other test so it requires the fix to pass.\n\n1: cb2ee0e003 ! 1: 410ca51936 sequencer: avoid dropping fixup commit that targets self via commit-ish\n @@ Commit message\n sequencer: avoid dropping fixup commit that targets self via commit-ish\n \n Commit 68d5d03bc4 (rebase: teach --autosquash to match on sha1 in\n - addition to message, 2010-11-04) made --autosquash apply a commit\n - with subject \"fixup! 012345\" to the first commit in the todo list\n - whose OID starts with 012345. So far so good.\n + addition to message, 2010-11-04) taught autosquash to recognize\n + subjects like \"fixup! 7a235b\" where 7a235b is an OID-prefix. It\n + actually did more than advertised: 7a235b can be an arbitrary\n + commit-ish (as long as it's not trailed by spaces).\n \n - More recently, c44a4c650c (rebase -i: rearrange fixup/squash lines\n - using the rebase--helper, 2017-07-14) reimplemented this logic in C\n - and introduced two behavior changes.\n - First, OID matches are given precedence over subject prefix\n - matches. Second, instead of prefix-matching OIDs, we use\n - lookup_commit_reference_by_name(). This means that if 012345 is a\n - branch name, we will apply the fixup commit to the tip of that branch\n - (if that is present in the todo list).\n + Accidental(?) use of this secret feature revealed a bug where we\n + would silently drop a fixup commit. The bug can also be triggered\n + when using an OID-prefix but that's unlikely in practice.\n \n - Both behavior changes might be motivated by performance concerns\n - (since the commit message mentions performance). Looking through\n - the todo list to find a commit that matches the given prefix can be\n - more expensive than looking up an OID. The runtime of the former is\n - of O(n*m) where n is the size of the todo list and m is the length\n - of a commit subject. However, if this is really a problem, we could\n - easily make it O(m) by constructing a trie (prefix tree).\n -\n - Demonstrate both behavior changes by adding two test cases for\n - \"fixup! foo\" where foo is a commit-ish that is not an OID-prefix.\n - Arguably, this feature is very weird. If no one uses it we should\n - consider removing it.\n -\n - Regardless, there is one bad edge case to fix. Let refspec \"foo\" point\n - to a commit with the subject \"fixup! foo\". Since rebase --autosquash\n - finds the fixup target via lookup_commit_reference_by_name(), the\n - fixup target is the fixup commit itself. Obviously this can't work.\n - We proceed with the broken invariant and drop the fixup commit\n - entirely.\n -\n - The self-fixup was only allowed because the fixup commit was already\n - added to the preliminary todo list, which it shouldn't be. Rather,\n - we should first compute the fixup target and only then add the fixup\n - commit to the todo list. Make it so, avoiding this error by design,\n - and add a third test for this case.\n + Given a commit with subject \"fixup! main\" that is the tip of the\n + branch \"main\". When computing the fixup target for this commit, we\n + find the commit itself. This is wrong because, by definition, a fixup\n + target must be an earlier commit in the todo list. We wrongly find\n + the current commit because we added it to the todo list prematurely.\n + Avoid these fixup-cycles by only adding the current commit after we\n + have finished finding its target.\n \n Reported-by: Erik Cervin Edin \n - Signed-off-by: Johannes Altmanninger \n - Signed-off-by: Junio C Hamano \n \n ## sequencer.c ##\n @@ sequencer.c: int todo_list_rearrange_squash(struct todo_list *todo_list)\n @@ t/t3415-rebase-autosquash.sh: test_expect_success 'auto squash that matches long\n +test_expect_success 'auto squash that matches regex' '\n +\tgit reset --hard base &&\n +\tgit commit --allow-empty -m \"hay needle hay\" &&\n -+\tgit commit --allow-empty -m \"fixup! :/[n]eedle\" &&\n ++\tgit commit --allow-empty -m \"fixup! :/needle\" &&\n +\tGIT_SEQUENCE_EDITOR=\"cat >tmp\" git rebase --autosquash -i HEAD^^ &&\n -+\tsed -ne \"/^[^#]/{s/[0-9a-f]\\{7,\\}/HASH/g;p}\" tmp >actual &&\n ++\tsed -ne \"/^[^#]/{s/[0-9a-f]\\{7,\\}/HASH/g;p;}\" tmp >actual &&\n +\tcat <<-EOF >expect &&\n +\tpick HASH hay needle hay # empty\n -+\tfixup HASH fixup! :/[n]eedle # empty\n -+\tEOF\n -+\ttest_cmp expect actual\n -+'\n -+\n -+test_expect_success 'auto squash of fixup commit that matches branch name' '\n -+\tgit reset --hard base &&\n -+\tgit commit --allow-empty -m \"wip commit (just a prefix match so overshadowed by branch)\" &&\n -+\tgit commit --allow-empty -m \"tip of wip\" &&\n -+\tgit branch wip &&\n -+\tgit commit --allow-empty -m \"unrelated commit\" &&\n -+\tgit commit --allow-empty -m \"fixup! wip\" &&\n -+\tGIT_SEQUENCE_EDITOR=\"cat >tmp\" git rebase --autosquash -i HEAD^^^^ &&\n -+\tsed -ne \"/^[^#]/{s/[0-9a-f]\\{7,\\}/HASH/g;p}\" tmp >actual &&\n -+\tcat <<-EOF >expect &&\n -+\tpick HASH wip commit (just a prefix match so overshadowed by branch) # empty\n -+\tpick HASH tip of wip # empty\n -+\tfixup HASH fixup! wip # empty\n -+\tpick HASH unrelated commit # empty\n ++\tfixup HASH fixup! :/needle # empty\n +\tEOF\n +\ttest_cmp expect actual\n +'\n @@ t/t3415-rebase-autosquash.sh: test_expect_success 'auto squash that matches long\n +\tgit commit --allow-empty -m \"fixup! self-cycle\" &&\n +\tgit branch self-cycle &&\n +\tGIT_SEQUENCE_EDITOR=\"cat >tmp\" git rebase --autosquash -i HEAD^^ &&\n -+\tsed -ne \"/^[^#]/{s/[0-9a-f]\\{7,\\}/HASH/g;p}\" tmp >actual &&\n ++\tsed -ne \"/^[^#]/{s/[0-9a-f]\\{7,\\}/HASH/g;p;}\" tmp >actual &&\n +\tcat <<-EOF >expect &&\n +\tpick HASH second commit\n +\tpick HASH fixup! self-cycle # empty\n\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 484ca9aa50..777200a6dc 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -6287,8 +6287,6 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)\n \t\t\treturn error(_(\"the script was already rearranged.\"));\n \t\t}\n \n-\t\t*commit_todo_item_at(&commit_todo, item->commit) = item;\n-\n \t\tparse_commit(item->commit);\n \t\tcommit_buffer = logmsg_reencode(item->commit, NULL, \"UTF-8\");\n \t\tfind_commit_subject(commit_buffer, &subject);\n@@ -6355,6 +6353,8 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)\n \t\t\t\t\tstrhash(entry->subject));\n \t\t\thashmap_put(&subject2item, &entry->entry);\n \t\t}\n+\n+\t\t*commit_todo_item_at(&commit_todo, item->commit) = item;\n \t}\n \n \tif (rearranged) {\ndiff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh\nindex 78c27496d6..98af865268 100755\n--- a/t/t3415-rebase-autosquash.sh\n+++ b/t/t3415-rebase-autosquash.sh\n@@ -232,6 +232,32 @@ test_expect_success 'auto squash that matches longer sha1' '\n \ttest_line_count = 1 actual\n '\n \n+test_expect_success 'auto squash that matches regex' '\n+\tgit reset --hard base &&\n+\tgit commit --allow-empty -m \"hay needle hay\" &&\n+\tgit commit --allow-empty -m \"fixup! :/needle\" &&\n+\tGIT_SEQUENCE_EDITOR=\"cat >tmp\" git rebase --autosquash -i HEAD^^ &&\n+\tsed -ne \"/^[^#]/{s/[0-9a-f]\\{7,\\}/HASH/g;p;}\" tmp >actual &&\n+\tcat <<-EOF >expect &&\n+\tpick HASH hay needle hay # empty\n+\tfixup HASH fixup! :/needle # empty\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'auto squash of fixup commit that matches branch name which points back to fixup commit' '\n+\tgit reset --hard base &&\n+\tgit commit --allow-empty -m \"fixup! self-cycle\" &&\n+\tgit branch self-cycle &&\n+\tGIT_SEQUENCE_EDITOR=\"cat >tmp\" git rebase --autosquash -i HEAD^^ &&\n+\tsed -ne \"/^[^#]/{s/[0-9a-f]\\{7,\\}/HASH/g;p;}\" tmp >actual &&\n+\tcat <<-EOF >expect &&\n+\tpick HASH second commit\n+\tpick HASH fixup! self-cycle # empty\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_auto_commit_flags () {\n \tgit reset --hard base &&\n \techo 1 >file1 &&\n-- \n2.37.3.830.gf65be7a4d6\n\n"},{"id":"463268","messageId":"YykxcU+UYzZgK+AT@gmail.com","threadId":"58451","inReplyTo":"xmqqpmfrcpq8.fsf@gitster.g","subject":"Re: [PATCH] sequencer: avoid dropping fixup commit that targets self via commit-ish","fromName":"Johannes Altmanninger","fromEmail":"aclopte@gmail.com","sentAt":"2022-09-20T03:20:17Z","receivedAt":"2022-09-20T03:20:58Z","isPatch":true,"sender":{"key":"aclopte@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6853872?v=4"},"body":"On Mon, Sep 19, 2022 at 09:07:27AM -0700, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> > ... But use of _by_name() that does\n> > not limit the input to hexadecimal _is_ a problem ...\n> \n> Ah, no, sorry, this was wrong. The original used \"rev-parse -q --verify\"\n> without restricting the \"single word\" to \"sha1 prefix\".\n> \n\nYeah, I've sent a v2 that removes the false verdicts from the commit message.\n\nEven if we restricted it to SHAs it would still be ambiguous.. I guess it\ndoesn't matter in practice.\n\nThanks\n"},{"id":"463269","messageId":"YykzGTMuKUGM793U@gmail.com","threadId":"58451","inReplyTo":"xmqqmtav7ygq.fsf@gitster.g","subject":"Re: [PATCH] sequencer: avoid dropping fixup commit that targets self via commit-ish","fromName":"Johannes Altmanninger","fromEmail":"aclopte@gmail.com","sentAt":"2022-09-20T03:27:21Z","receivedAt":"2022-09-20T03:27:49Z","isPatch":true,"sender":{"key":"aclopte@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6853872?v=4"},"body":"On Mon, Sep 19, 2022 at 04:09:57PM -0700, Junio C Hamano wrote:\n> Johannes Altmanninger writes:\n> \n> > +test_expect_success 'auto squash of fixup commit that matches branch name which points back to fixup commit' '\n> > ...\n> > +\tsed -ne \"/^[^#]/{s/[0-9a-f]\\{7,\\}/HASH/g;p}\" tmp >actual &&\n\nFWIW I copied and adapted this one from the remerge-diff tests. Not sure\nwhat other tests use. Maybe the HASH replacement is worth a test helper even\nthough it is a heuristic that can go wrong.\n"},{"id":"463291","messageId":"8909c02d-3fd0-a0bb-ebc2-0a640febce53@dunelm.org.uk","threadId":"58451","inReplyTo":"20220920031140.1220220-1-aclopte@gmail.com","subject":"Re: [PATCH v2] sequencer: avoid dropping fixup commit that targets self via commit-ish","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-09-20T08:26:27Z","receivedAt":"2022-09-20T08:29:38Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Johannes\n\nOn 20/09/2022 04:11, Johannes Altmanninger wrote:\n> Commit 68d5d03bc4 (rebase: teach --autosquash to match on sha1 in\n> addition to message, 2010-11-04) taught autosquash to recognize\n> subjects like \"fixup! 7a235b\" where 7a235b is an OID-prefix. It\n> actually did more than advertised: 7a235b can be an arbitrary\n> commit-ish (as long as it's not trailed by spaces).\n> \n> Accidental(?) use of this secret feature revealed a bug where we\n> would silently drop a fixup commit. The bug can also be triggered\n> when using an OID-prefix but that's unlikely in practice.\n> \n> Given a commit with subject \"fixup! main\" that is the tip of the\n> branch \"main\". When computing the fixup target for this commit, we\n> find the commit itself. This is wrong because, by definition, a fixup\n> target must be an earlier commit in the todo list. We wrongly find\n> the current commit because we added it to the todo list prematurely.\n> Avoid these fixup-cycles by only adding the current commit after we\n> have finished finding its target.\n\nThanks for working on this, the fix for the fixup self reference looks \ngood. It's unfortunate that the implementation is not stricter when \nparsing \"fixup! \" but it is more or less consistent with the shell \nversion which used \"git rev-parse $subject\"[1]. We should think about \nbeing stricter but this fix avoids on of the worst pitfalls of our lax \nparsing.\n\nBest Wishes\n\nPhillip\n\n[1] With regard to the oid vs subject prefix issue, I think the shell \nversion chose to fixup the first commit that matched either the oid or \nthe subject. At least the C version is consistent in preferring an oid \nmatch over a subject prefix match even if I wish it was the other way round.\n\n> Reported-by: Erik Cervin Edin \n> Signed-off-by: Johannes Altmanninger \n> ---\n> sequencer.c | 4 ++--\n> t/t3415-rebase-autosquash.sh | 26 ++++++++++++++++++++++++++\n> 2 files changed, 28 insertions(+), 2 deletions(-)\n> \n> Changes to v1.\n> - Remove wrong assumptions from commit message. The commit message should\n> be clearer now (though I didn't spend too much time on it).\n> - Drop one test because it's not related to the fix (and doesn't test anything\n> I care about) and modify the other test so it requires the fix to pass.\n> \n> 1: cb2ee0e003 ! 1: 410ca51936 sequencer: avoid dropping fixup commit that targets self via commit-ish\n> @@ Commit message\n> sequencer: avoid dropping fixup commit that targets self via commit-ish\n> \n> Commit 68d5d03bc4 (rebase: teach --autosquash to match on sha1 in\n> - addition to message, 2010-11-04) made --autosquash apply a commit\n> - with subject \"fixup! 012345\" to the first commit in the todo list\n> - whose OID starts with 012345. So far so good.\n> + addition to message, 2010-11-04) taught autosquash to recognize\n> + subjects like \"fixup! 7a235b\" where 7a235b is an OID-prefix. It\n> + actually did more than advertised: 7a235b can be an arbitrary\n> + commit-ish (as long as it's not trailed by spaces).\n> \n> - More recently, c44a4c650c (rebase -i: rearrange fixup/squash lines\n> - using the rebase--helper, 2017-07-14) reimplemented this logic in C\n> - and introduced two behavior changes.\n> - First, OID matches are given precedence over subject prefix\n> - matches. Second, instead of prefix-matching OIDs, we use\n> - lookup_commit_reference_by_name(). This means that if 012345 is a\n> - branch name, we will apply the fixup commit to the tip of that branch\n> - (if that is present in the todo list).\n> + Accidental(?) use of this secret feature revealed a bug where we\n> + would silently drop a fixup commit. The bug can also be triggered\n> + when using an OID-prefix but that's unlikely in practice.\n> \n> - Both behavior changes might be motivated by performance concerns\n> - (since the commit message mentions performance). Looking through\n> - the todo list to find a commit that matches the given prefix can be\n> - more expensive than looking up an OID. The runtime of the former is\n> - of O(n*m) where n is the size of the todo list and m is the length\n> - of a commit subject. However, if this is really a problem, we could\n> - easily make it O(m) by constructing a trie (prefix tree).\n> -\n> - Demonstrate both behavior changes by adding two test cases for\n> - \"fixup! foo\" where foo is a commit-ish that is not an OID-prefix.\n> - Arguably, this feature is very weird. If no one uses it we should\n> - consider removing it.\n> -\n> - Regardless, there is one bad edge case to fix. Let refspec \"foo\" point\n> - to a commit with the subject \"fixup! foo\". Since rebase --autosquash\n> - finds the fixup target via lookup_commit_reference_by_name(), the\n> - fixup target is the fixup commit itself. Obviously this can't work.\n> - We proceed with the broken invariant and drop the fixup commit\n> - entirely.\n> -\n> - The self-fixup was only allowed because the fixup commit was already\n> - added to the preliminary todo list, which it shouldn't be. Rather,\n> - we should first compute the fixup target and only then add the fixup\n> - commit to the todo list. Make it so, avoiding this error by design,\n> - and add a third test for this case.\n> + Given a commit with subject \"fixup! main\" that is the tip of the\n> + branch \"main\". When computing the fixup target for this commit, we\n> + find the commit itself. This is wrong because, by definition, a fixup\n> + target must be an earlier commit in the todo list. We wrongly find\n> + the current commit because we added it to the todo list prematurely.\n> + Avoid these fixup-cycles by only adding the current commit after we\n> + have finished finding its target.\n> \n> Reported-by: Erik Cervin Edin \n> - Signed-off-by: Johannes Altmanninger \n> - Signed-off-by: Junio C Hamano \n> \n> ## sequencer.c ##\n> @@ sequencer.c: int todo_list_rearrange_squash(struct todo_list *todo_list)\n> @@ t/t3415-rebase-autosquash.sh: test_expect_success 'auto squash that matches long\n> +test_expect_success 'auto squash that matches regex' '\n> +\tgit reset --hard base &&\n> +\tgit commit --allow-empty -m \"hay needle hay\" &&\n> -+\tgit commit --allow-empty -m \"fixup! :/[n]eedle\" &&\n> ++\tgit commit --allow-empty -m \"fixup! :/needle\" &&\n> +\tGIT_SEQUENCE_EDITOR=\"cat >tmp\" git rebase --autosquash -i HEAD^^ &&\n> -+\tsed -ne \"/^[^#]/{s/[0-9a-f]\\{7,\\}/HASH/g;p}\" tmp >actual &&\n> ++\tsed -ne \"/^[^#]/{s/[0-9a-f]\\{7,\\}/HASH/g;p;}\" tmp >actual &&\n> +\tcat <<-EOF >expect &&\n> +\tpick HASH hay needle hay # empty\n> -+\tfixup HASH fixup! :/[n]eedle # empty\n> -+\tEOF\n> -+\ttest_cmp expect actual\n> -+'\n> -+\n> -+test_expect_success 'auto squash of fixup commit that matches branch name' '\n> -+\tgit reset --hard base &&\n> -+\tgit commit --allow-empty -m \"wip commit (just a prefix match so overshadowed by branch)\" &&\n> -+\tgit commit --allow-empty -m \"tip of wip\" &&\n> -+\tgit branch wip &&\n> -+\tgit commit --allow-empty -m \"unrelated commit\" &&\n> -+\tgit commit --allow-empty -m \"fixup! wip\" &&\n> -+\tGIT_SEQUENCE_EDITOR=\"cat >tmp\" git rebase --autosquash -i HEAD^^^^ &&\n> -+\tsed -ne \"/^[^#]/{s/[0-9a-f]\\{7,\\}/HASH/g;p}\" tmp >actual &&\n> -+\tcat <<-EOF >expect &&\n> -+\tpick HASH wip commit (just a prefix match so overshadowed by branch) # empty\n> -+\tpick HASH tip of wip # empty\n> -+\tfixup HASH fixup! wip # empty\n> -+\tpick HASH unrelated commit # empty\n> ++\tfixup HASH fixup! :/needle # empty\n> +\tEOF\n> +\ttest_cmp expect actual\n> +'\n> @@ t/t3415-rebase-autosquash.sh: test_expect_success 'auto squash that matches long\n> +\tgit commit --allow-empty -m \"fixup! self-cycle\" &&\n> +\tgit branch self-cycle &&\n> +\tGIT_SEQUENCE_EDITOR=\"cat >tmp\" git rebase --autosquash -i HEAD^^ &&\n> -+\tsed -ne \"/^[^#]/{s/[0-9a-f]\\{7,\\}/HASH/g;p}\" tmp >actual &&\n> ++\tsed -ne \"/^[^#]/{s/[0-9a-f]\\{7,\\}/HASH/g;p;}\" tmp >actual &&\n> +\tcat <<-EOF >expect &&\n> +\tpick HASH second commit\n> +\tpick HASH fixup! self-cycle # empty\n> \n> \n> diff --git a/sequencer.c b/sequencer.c\n> index 484ca9aa50..777200a6dc 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -6287,8 +6287,6 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)\n> \t\t\treturn error(_(\"the script was already rearranged.\"));\n> \t\t}\n> \n> -\t\t*commit_todo_item_at(&commit_todo, item->commit) = item;\n> -\n> \t\tparse_commit(item->commit);\n> \t\tcommit_buffer = logmsg_reencode(item->commit, NULL, \"UTF-8\");\n> \t\tfind_commit_subject(commit_buffer, &subject);\n> @@ -6355,6 +6353,8 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)\n> \t\t\t\t\tstrhash(entry->subject));\n> \t\t\thashmap_put(&subject2item, &entry->entry);\n> \t\t}\n> +\n> +\t\t*commit_todo_item_at(&commit_todo, item->commit) = item;\n> \t}\n> \n> \tif (rearranged) {\n> diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh\n> index 78c27496d6..98af865268 100755\n> --- a/t/t3415-rebase-autosquash.sh\n> +++ b/t/t3415-rebase-autosquash.sh\n> @@ -232,6 +232,32 @@ test_expect_success 'auto squash that matches longer sha1' '\n> \ttest_line_count = 1 actual\n> '\n> \n> +test_expect_success 'auto squash that matches regex' '\n> +\tgit reset --hard base &&\n> +\tgit commit --allow-empty -m \"hay needle hay\" &&\n> +\tgit commit --allow-empty -m \"fixup! :/needle\" &&\n> +\tGIT_SEQUENCE_EDITOR=\"cat >tmp\" git rebase --autosquash -i HEAD^^ &&\n> +\tsed -ne \"/^[^#]/{s/[0-9a-f]\\{7,\\}/HASH/g;p;}\" tmp >actual &&\n> +\tcat <<-EOF >expect &&\n> +\tpick HASH hay needle hay # empty\n> +\tfixup HASH fixup! :/needle # empty\n> +\tEOF\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'auto squash of fixup commit that matches branch name which points back to fixup commit' '\n> +\tgit reset --hard base &&\n> +\tgit commit --allow-empty -m \"fixup! self-cycle\" &&\n> +\tgit branch self-cycle &&\n> +\tGIT_SEQUENCE_EDITOR=\"cat >tmp\" git rebase --autosquash -i HEAD^^ &&\n> +\tsed -ne \"/^[^#]/{s/[0-9a-f]\\{7,\\}/HASH/g;p;}\" tmp >actual &&\n> +\tcat <<-EOF >expect &&\n> +\tpick HASH second commit\n> +\tpick HASH fixup! self-cycle # empty\n> +\tEOF\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_auto_commit_flags () {\n> \tgit reset --hard base &&\n> \techo 1 >file1 &&\n"},{"id":"463383","messageId":"xmqqa66s36pt.fsf@gitster.g","threadId":"58451","inReplyTo":"20220920031140.1220220-1-aclopte@gmail.com","subject":"Re: [PATCH v2] sequencer: avoid dropping fixup commit that targets self via commit-ish","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-21T18:47:26Z","receivedAt":"2022-09-21T18:47:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Altmanninger writes:\n\n> +test_expect_success 'auto squash that matches regex' '\n> +\tgit reset --hard base &&\n> +\tgit commit --allow-empty -m \"hay needle hay\" &&\n> +\tgit commit --allow-empty -m \"fixup! :/needle\" &&\n> +\tGIT_SEQUENCE_EDITOR=\"cat >tmp\" git rebase --autosquash -i HEAD^^ &&\n> +\tsed -ne \"/^[^#]/{s/[0-9a-f]\\{7,\\}/HASH/g;p;}\" tmp >actual &&\n> +\tcat <<-EOF >expect &&\n> +\tpick HASH hay needle hay # empty\n> +\tfixup HASH fixup! :/needle # empty\n> +\tEOF\n> +\ttest_cmp expect actual\n> +'\n\nhint: Waiting for your editor to close the file...\nSuccessfully rebased and updated refs/heads/main.\n--- expect 2022-09-21 18:45:27.617530848 +0000\n+++ actual 2022-09-21 18:45:27.613530478 +0000\n@@ -1,2 +1,2 @@\n pick HASH hay needle hay # empty\n-fixup HASH fixup! :/needle # empty\n+pick HASH fixup! :/needle # empty\nnot ok 11 - auto squash that matches regex\n\nThat does not look very good X-<.\n"},{"id":"463415","messageId":"YyvdwbE6oCeNn035@gmail.com","threadId":"58451","inReplyTo":"xmqqa66s36pt.fsf@gitster.g","subject":"Re: [PATCH v2] sequencer: avoid dropping fixup commit that targets self via commit-ish","fromName":"Johannes Altmanninger","fromEmail":"aclopte@gmail.com","sentAt":"2022-09-22T04:00:01Z","receivedAt":"2022-09-22T04:00:38Z","isPatch":true,"sender":{"key":"aclopte@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6853872?v=4"},"body":"On Wed, Sep 21, 2022 at 11:47:26AM -0700, Junio C Hamano wrote:\n> Johannes Altmanninger writes:\n> \n> > +test_expect_success 'auto squash that matches regex' '\n> > +\tgit reset --hard base &&\n> > +\tgit commit --allow-empty -m \"hay needle hay\" &&\n> > +\tgit commit --allow-empty -m \"fixup! :/needle\" &&\n> > +\tGIT_SEQUENCE_EDITOR=\"cat >tmp\" git rebase --autosquash -i HEAD^^ &&\n> > +\tsed -ne \"/^[^#]/{s/[0-9a-f]\\{7,\\}/HASH/g;p;}\" tmp >actual &&\n> > +\tcat <<-EOF >expect &&\n> > +\tpick HASH hay needle hay # empty\n> > +\tfixup HASH fixup! :/needle # empty\n> > +\tEOF\n> > +\ttest_cmp expect actual\n> > +'\n> \n> hint: Waiting for your editor to close the file...\n> Successfully rebased and updated refs/heads/main.\n> --- expect 2022-09-21 18:45:27.617530848 +0000\n> +++ actual 2022-09-21 18:45:27.613530478 +0000\n> @@ -1,2 +1,2 @@\n> pick HASH hay needle hay # empty\n> -fixup HASH fixup! :/needle # empty\n> +pick HASH fixup! :/needle # empty\n> not ok 11 - auto squash that matches regex\n> \n> That does not look very good X-<.\n\nSorry the v2 of this test case is very misleading, should probably drop this\ntest entirely. It's been a long a day so I'll send v3 another day (if needed).\n"},{"id":"463468","messageId":"xmqqpmfnxl1o.fsf@gitster.g","threadId":"58451","inReplyTo":"YyvdwbE6oCeNn035@gmail.com","subject":"Re: [PATCH v2] sequencer: avoid dropping fixup commit that targets self via commit-ish","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-22T19:32:03Z","receivedAt":"2022-09-22T19:32:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Altmanninger writes:\n\n>> hint: Waiting for your editor to close the file...\n>> Successfully rebased and updated refs/heads/main.\n>> --- expect 2022-09-21 18:45:27.617530848 +0000\n>> +++ actual 2022-09-21 18:45:27.613530478 +0000\n>> @@ -1,2 +1,2 @@\n>> pick HASH hay needle hay # empty\n>> -fixup HASH fixup! :/needle # empty\n>> +pick HASH fixup! :/needle # empty\n>> not ok 11 - auto squash that matches regex\n>> \n>> That does not look very good X-<.\n>\n> Sorry the v2 of this test case is very misleading, should probably drop this\n> test entirely. It's been a long a day so I'll send v3 another day (if needed).\n\nThanks.\n"},{"id":"463586","messageId":"20220924222904.1784975-1-aclopte@gmail.com","threadId":"58451","inReplyTo":"xmqqpmfnxl1o.fsf@gitster.g","subject":"[PATCH v3] sequencer: avoid dropping fixup commit that targets self via commit-ish","fromName":"Johannes Altmanninger","fromEmail":"aclopte@gmail.com","sentAt":"2022-09-24T22:29:04Z","receivedAt":"2022-09-24T22:29:45Z","isPatch":true,"sender":{"key":"aclopte@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6853872?v=4"},"body":"Commit 68d5d03bc4 (rebase: teach --autosquash to match on sha1 in\naddition to message, 2010-11-04) taught autosquash to recognize\nsubjects like \"fixup! 7a235b\" where 7a235b is an OID-prefix. It\nactually did more than advertised: 7a235b can be an arbitrary\ncommit-ish (as long as it's not trailed by spaces).\n\nAccidental(?) use of this secret feature revealed a bug where we\nwould silently drop a fixup commit. The bug can also be triggered\nwhen using an OID-prefix but that's unlikely in practice.\n\nLet the commit with subject \"fixup! main\" be the tip of the \"main\"\nbranch. When computing the fixup target for this commit, we find\nthe commit itself. This is wrong because, by definition, a fixup\ntarget must be an earlier commit in the todo list. We wrongly find\nthe current commit because we added it to the todo list prematurely.\nAvoid these fixup-cycles by only adding the current commit to the\ntodo list after we have finished looking for the fixup target.\n\nReported-by: Erik Cervin Edin \nSigned-off-by: Johannes Altmanninger \n---\n sequencer.c | 4 ++--\n t/t3415-rebase-autosquash.sh | 13 +++++++++++++\n 2 files changed, 15 insertions(+), 2 deletions(-)\n\nChanges since v2:\n- minor commit message rewording and clarification\n- dropped a test that was based on a wrong understanding of --autosquash.\n\n I saw b425461cb5 (SQUASH??? resurrect previous version of the tests,\n 2022-09-21) today. I'm leaning towards not adding them back because\n those tests were only added to reflect the current behavior. but that\n behavior is not something I care about.\n We might even want to change this behavior, by restricting OID fixup\n targets to SHAs only. So I don't think these tests are desirable,\n unless they help others understand the current state of affairs?\n\n1: 37e00c51bd ! 1: a25c886a78 sequencer: avoid dropping fixup commit that targets self via commit-ish\n @@ Commit message\n would silently drop a fixup commit. The bug can also be triggered\n when using an OID-prefix but that's unlikely in practice.\n \n - Given a commit with subject \"fixup! main\" that is the tip of the\n - branch \"main\". When computing the fixup target for this commit, we\n - find the commit itself. This is wrong because, by definition, a fixup\n + Let the commit with subject \"fixup! main\" be the tip of the \"main\"\n + branch. When computing the fixup target for this commit, we find\n + the commit itself. This is wrong because, by definition, a fixup\n target must be an earlier commit in the todo list. We wrongly find\n the current commit because we added it to the todo list prematurely.\n - Avoid these fixup-cycles by only adding the current commit after we\n - have finished finding its target.\n + Avoid these fixup-cycles by only adding the current commit to the\n + todo list after we have finished looking for the fixup target.\n \n Reported-by: Erik Cervin Edin \n \n ## sequencer.c ##\n @@ sequencer.c: int todo_list_rearrange_squash(struct todo_list *todo_list)\n @@ t/t3415-rebase-autosquash.sh: test_expect_success 'auto squash that matches long\n \ttest_line_count = 1 actual\n '\n \n -+test_expect_success 'auto squash that matches regex' '\n -+\tgit reset --hard base &&\n -+\tgit commit --allow-empty -m \"hay needle hay\" &&\n -+\tgit commit --allow-empty -m \"fixup! :/needle\" &&\n -+\tGIT_SEQUENCE_EDITOR=\"cat >tmp\" git rebase --autosquash -i HEAD^^ &&\n -+\tsed -ne \"/^[^#]/{s/[0-9a-f]\\{7,\\}/HASH/g;p;}\" tmp >actual &&\n -+\tcat <<-EOF >expect &&\n -+\tpick HASH hay needle hay # empty\n -+\tfixup HASH fixup! :/needle # empty\n -+\tEOF\n -+\ttest_cmp expect actual\n -+'\n -+\n +test_expect_success 'auto squash of fixup commit that matches branch name which points back to fixup commit' '\n +\tgit reset --hard base &&\n +\tgit commit --allow-empty -m \"fixup! self-cycle\" &&\n\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 484ca9aa50..777200a6dc 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -6287,8 +6287,6 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)\n \t\t\treturn error(_(\"the script was already rearranged.\"));\n \t\t}\n \n-\t\t*commit_todo_item_at(&commit_todo, item->commit) = item;\n-\n \t\tparse_commit(item->commit);\n \t\tcommit_buffer = logmsg_reencode(item->commit, NULL, \"UTF-8\");\n \t\tfind_commit_subject(commit_buffer, &subject);\n@@ -6355,6 +6353,8 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)\n \t\t\t\t\tstrhash(entry->subject));\n \t\t\thashmap_put(&subject2item, &entry->entry);\n \t\t}\n+\n+\t\t*commit_todo_item_at(&commit_todo, item->commit) = item;\n \t}\n \n \tif (rearranged) {\ndiff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh\nindex 78c27496d6..a364530d76 100755\n--- a/t/t3415-rebase-autosquash.sh\n+++ b/t/t3415-rebase-autosquash.sh\n@@ -232,6 +232,19 @@ test_expect_success 'auto squash that matches longer sha1' '\n \ttest_line_count = 1 actual\n '\n \n+test_expect_success 'auto squash of fixup commit that matches branch name which points back to fixup commit' '\n+\tgit reset --hard base &&\n+\tgit commit --allow-empty -m \"fixup! self-cycle\" &&\n+\tgit branch self-cycle &&\n+\tGIT_SEQUENCE_EDITOR=\"cat >tmp\" git rebase --autosquash -i HEAD^^ &&\n+\tsed -ne \"/^[^#]/{s/[0-9a-f]\\{7,\\}/HASH/g;p;}\" tmp >actual &&\n+\tcat <<-EOF >expect &&\n+\tpick HASH second commit\n+\tpick HASH fixup! self-cycle # empty\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_auto_commit_flags () {\n \tgit reset --hard base &&\n \techo 1 >file1 &&\n-- \n2.37.3.830.gf65be7a4d6\n\n"}]}