{"thread":{"id":"53393","subject":"[PATCH] rebase --autosquash: fix a potential segfault","startedAt":"2020-05-04T20:40:10Z","lastAt":"2020-05-29T11:41:07Z","messageCount":23,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Jeff King","Taylor Blau","Andrei Rybak","Johannes Schindelin","Philip Oakley"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"396984","messageId":"pull.625.git.1588624804554.gitgitgadget@gmail.com","threadId":"53393","inReplyTo":null,"subject":"[PATCH] rebase --autosquash: fix a potential segfault","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-05-04T20:40:04Z","receivedAt":"2020-05-04T20:40:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen rearranging the todo list so that the fixups/squashes are reordered\njust after the commits they intend to fix up, we use two arrays to\nmaintain that list: `next` and `tail`.\n\nThe idea is that `next[i]`, if set to a non-negative value, contains the\nindex of the item that should be rearranged just after the `i`th item.\n\nTo avoid having to walk the entire `next` chain when appending another\nfixup/squash, we also store the end of the `next` chain in `last[i]`.\n\nThe logic we currently use to update these array items is based on the\nassumption that given a fixup/squash item at index `i`, we just found\nthe index `i2` indicating the first item in that fixup chain.\n\nHowever, as reported by Paul Ganssle, that need not be true: the special\nform `fixup! ` is allowed to point to _another_ fixup\ncommit in the middle of the fixup chain.\n\nExample:\n\n\t* 0192a To fixup\n\t* 02f12 fixup! To fixup\n\t* 03763 fixup! To fixup\n\t* 04ecb fixup! 02f12\n\nNote how the fourth commit targets the second commit, which is already a\nfixup that targets the first commit.\n\nThe good news is that it is easy to fix this: we can detect the\nsituation by looking at `last[i2]` (which will be `-1` if `i2` is\nactually in the middle of a fixup chain), and in that case we simply\nneed to squeeze the current item into the middle of the `next` chain,\nwithout touching `last` (i.e. leaving the end index of the fixup chain\nalone).\n\nReported-by: Paul Ganssle \nHelped-by: Jeff King \nSigned-off-by: Johannes Schindelin \n---\n rebase --autosquash: fix a potential segfault\n \n This patch is in response to \n https://lore.kernel.org/git/017dbc40-8d21-00fb-7b0e-6708d2dcb366@ganssle.io/\n .\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-625%2Fdscho%2Fautosquash-segfault-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-625/dscho/autosquash-segfault-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/625\n\n sequencer.c | 11 ++++++++++-\n t/t3415-rebase-autosquash.sh | 14 ++++++++++++++\n 2 files changed, 24 insertions(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex e528225e787..0d4d53d2a49 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -5266,8 +5266,17 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)\n \t\t\t\tTODO_FIXUP : TODO_SQUASH;\n \t\t\tif (next[i2] < 0)\n \t\t\t\tnext[i2] = i;\n-\t\t\telse\n+\t\t\telse if (tail[i2] >= 0)\n \t\t\t\tnext[tail[i2]] = i;\n+\t\t\telse {\n+\t\t\t\t/*\n+\t\t\t\t * i2 refers to a fixup commit in the middle of\n+\t\t\t\t * a fixup chain\n+\t\t\t\t */\n+\t\t\t\tnext[i] = next[i2];\n+\t\t\t\tnext[i2] = i;\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t\ttail[i2] = i;\n \t\t} else if (!hashmap_get_from_hash(&subject2item,\n \t\t\t\t\t\tstrhash(subject), subject)) {\ndiff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh\nindex 093de9005b7..ca135349346 100755\n--- a/t/t3415-rebase-autosquash.sh\n+++ b/t/t3415-rebase-autosquash.sh\n@@ -424,4 +424,18 @@ test_expect_success 'abort last squash' '\n \t! grep first actual\n '\n \n+test_expect_success 'fixup a fixup' '\n+\techo 0to-fixup >file0 &&\n+\ttest_tick &&\n+\tgit commit -m \"to-fixup\" file0 &&\n+\ttest_tick &&\n+\tgit commit --squash HEAD -m X --allow-empty &&\n+\ttest_tick &&\n+\tgit commit --squash HEAD^ -m Y --allow-empty &&\n+\ttest_tick &&\n+\tgit commit -m \"squash! $(git rev-parse HEAD^)\" -m Z --allow-empty &&\n+\tgit rebase -ki --autosquash HEAD~4 &&\n+\ttest XZY = $(git show | tr -cd X-Z)\n+'\n+\n test_done\n\nbase-commit: af6b65d45ef179ed52087e80cb089f6b2349f4ec\n-- \ngitgitgadget\n"},{"id":"396989","messageId":"xmqq4ksvflep.fsf@gitster.c.googlers.com","threadId":"53393","inReplyTo":"pull.625.git.1588624804554.gitgitgadget@gmail.com","subject":"Re: [PATCH] rebase --autosquash: fix a potential segfault","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-04T21:19:26Z","receivedAt":"2020-05-04T21:19:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> diff --git a/sequencer.c b/sequencer.c\n> index e528225e787..0d4d53d2a49 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -5266,8 +5266,17 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)\n> \t\t\t\tTODO_FIXUP : TODO_SQUASH;\n> \t\t\tif (next[i2] < 0)\n> \t\t\t\tnext[i2] = i;\n> -\t\t\telse\n> +\t\t\telse if (tail[i2] >= 0)\n> \t\t\t\tnext[tail[i2]] = i;\n> +\t\t\telse {\n> +\t\t\t\t/*\n> +\t\t\t\t * i2 refers to a fixup commit in the middle of\n> +\t\t\t\t * a fixup chain\n> +\t\t\t\t */\n> +\t\t\t\tnext[i] = next[i2];\n> +\t\t\t\tnext[i2] = i;\n> +\t\t\t\tcontinue;\n\nOK, this would catch the case even when fixing up a fix-up of\nantoher fix-up, so we won't need further \"else if\" in the future ;-)\n\nI suspect that this breakage is as old as 2.14, introduced by\nc44a4c65 (rebase -i: rearrange fixup/squash lines using the\nrebase--helper, 2017-07-14), but perhaps we won't need to backmerge\nthe fix that far. We don't even backport security fixes beyond 2.17\n(which is two years old).\n\nJust in case I'll queue this immediately on top of f2a04904\n(sequencer: refactor rearrange_squash() to work on a todo_list,\n2019-03-05); that would give us a potential to cover as far back to\n2.20 series.\n\nThanks.\n\n> +\t\t\t}\n> \t\t\ttail[i2] = i;\n> \t\t} else if (!hashmap_get_from_hash(&subject2item,\n> \t\t\t\t\t\tstrhash(subject), subject)) {\n> diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh\n> index 093de9005b7..ca135349346 100755\n> --- a/t/t3415-rebase-autosquash.sh\n> +++ b/t/t3415-rebase-autosquash.sh\n> @@ -424,4 +424,18 @@ test_expect_success 'abort last squash' '\n> \t! grep first actual\n> '\n> \n> +test_expect_success 'fixup a fixup' '\n> +\techo 0to-fixup >file0 &&\n> +\ttest_tick &&\n> +\tgit commit -m \"to-fixup\" file0 &&\n> +\ttest_tick &&\n> +\tgit commit --squash HEAD -m X --allow-empty &&\n> +\ttest_tick &&\n> +\tgit commit --squash HEAD^ -m Y --allow-empty &&\n> +\ttest_tick &&\n> +\tgit commit -m \"squash! $(git rev-parse HEAD^)\" -m Z --allow-empty &&\n> +\tgit rebase -ki --autosquash HEAD~4 &&\n> +\ttest XZY = $(git show | tr -cd X-Z)\n> +'\n> +\n> test_done\n>\n> base-commit: af6b65d45ef179ed52087e80cb089f6b2349f4ec\n"},{"id":"396994","messageId":"20200504213326.GA31037@coredump.intra.peff.net","threadId":"53393","inReplyTo":"pull.625.git.1588624804554.gitgitgadget@gmail.com","subject":"Re: [PATCH] rebase --autosquash: fix a potential segfault","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-04T21:33:26Z","receivedAt":"2020-05-04T21:33:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 04, 2020 at 08:40:04PM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> When rearranging the todo list so that the fixups/squashes are reordered\n> just after the commits they intend to fix up, we use two arrays to\n> maintain that list: `next` and `tail`.\n> \n> The idea is that `next[i]`, if set to a non-negative value, contains the\n> index of the item that should be rearranged just after the `i`th item.\n> \n> To avoid having to walk the entire `next` chain when appending another\n> fixup/squash, we also store the end of the `next` chain in `last[i]`.\n\ns/last/tail/, I think? (and below)\n\n> The good news is that it is easy to fix this: we can detect the\n> situation by looking at `last[i2]` (which will be `-1` if `i2` is\n> actually in the middle of a fixup chain), and in that case we simply\n> need to squeeze the current item into the middle of the `next` chain,\n> without touching `last` (i.e. leaving the end index of the fixup chain\n> alone).\n\nOK, good. I definitely had figured out how to detect the case, but\nwasn't quite sure how to manipulate next.\n\nBut your fix here makes sense:\n\n> \t\t\tif (next[i2] < 0)\n> \t\t\t\tnext[i2] = i;\n> -\t\t\telse\n> +\t\t\telse if (tail[i2] >= 0)\n> \t\t\t\tnext[tail[i2]] = i;\n> +\t\t\telse {\n> +\t\t\t\t/*\n> +\t\t\t\t * i2 refers to a fixup commit in the middle of\n> +\t\t\t\t * a fixup chain\n> +\t\t\t\t */\n> +\t\t\t\tnext[i] = next[i2];\n> +\t\t\t\tnext[i2] = i;\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n\nI do have one question, though. What happens if we add a second\nfixup-of-a-fixup?\n\nWe'd see its \"next\" slot filled, but now pointing to the first\nfixup-of-a-fixup. And we'd add ourselves at the front of that list. So I\nthink:\n\n 1234 foo\n 5678 !fixup foo\n abcd !fixup 5678\n dbaf !fixup 5678\n\nwould end up reordering abcd and dbaf (putting dbaf first), wouldn't it?\n\nBut when I tested it doesn't seem to:\n\n git init\n git commit -m base --allow-empty\n git commit --squash HEAD -m 'this is the first squash' --allow-empty\n s=$(git rev-parse HEAD)\n git commit -m \"squash! $s\" -m 'this is the second squash' --allow-empty\n git commit -m \"squash! $s\" -m 'this is the third squash' --allow-empty\n git rebase -ki --autosquash --root\n\nSo I think there's something I don't quite understand about how the\nchain of \"next\" works. If you can enlighten me, I'd be grateful.\n\nBut your patch does seem to work as advertised. It might be worth adding\nthe double-squash-of-squash to the test.\n\n-Peff\n"},{"id":"397001","messageId":"20200504220916.GF45250@syl.local","threadId":"53393","inReplyTo":"20200504213326.GA31037@coredump.intra.peff.net","subject":"Re: [PATCH] rebase --autosquash: fix a potential segfault","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-04T22:09:16Z","receivedAt":"2020-05-04T22:09:20Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, May 04, 2020 at 05:33:26PM -0400, Jeff King wrote:\n> On Mon, May 04, 2020 at 08:40:04PM +0000, Johannes Schindelin via GitGitGadget wrote:\n>\n> > When rearranging the todo list so that the fixups/squashes are reordered\n> > just after the commits they intend to fix up, we use two arrays to\n> > maintain that list: `next` and `tail`.\n> >\n> > The idea is that `next[i]`, if set to a non-negative value, contains the\n> > index of the item that should be rearranged just after the `i`th item.\n> >\n> > To avoid having to walk the entire `next` chain when appending another\n> > fixup/squash, we also store the end of the `next` chain in `last[i]`.\n>\n> s/last/tail/, I think? (and below)\n>\n> > The good news is that it is easy to fix this: we can detect the\n> > situation by looking at `last[i2]` (which will be `-1` if `i2` is\n> > actually in the middle of a fixup chain), and in that case we simply\n> > need to squeeze the current item into the middle of the `next` chain,\n> > without touching `last` (i.e. leaving the end index of the fixup chain\n> > alone).\n>\n> OK, good. I definitely had figured out how to detect the case, but\n> wasn't quite sure how to manipulate next.\n>\n> But your fix here makes sense:\n>\n> > \t\t\tif (next[i2] < 0)\n> > \t\t\t\tnext[i2] = i;\n> > -\t\t\telse\n> > +\t\t\telse if (tail[i2] >= 0)\n> > \t\t\t\tnext[tail[i2]] = i;\n> > +\t\t\telse {\n> > +\t\t\t\t/*\n> > +\t\t\t\t * i2 refers to a fixup commit in the middle of\n> > +\t\t\t\t * a fixup chain\n> > +\t\t\t\t */\n> > +\t\t\t\tnext[i] = next[i2];\n> > +\t\t\t\tnext[i2] = i;\n> > +\t\t\t\tcontinue;\n> > +\t\t\t}\n>\n> I do have one question, though. What happens if we add a second\n> fixup-of-a-fixup?\n\nThanks for asking this question, I was a little curious about it, too.\n\n> We'd see its \"next\" slot filled, but now pointing to the first\n> fixup-of-a-fixup. And we'd add ourselves at the front of that list. So I\n> think:\n>\n> 1234 foo\n> 5678 !fixup foo\n> abcd !fixup 5678\n> dbaf !fixup 5678\n>\n> would end up reordering abcd and dbaf (putting dbaf first), wouldn't it?\n>\n> But when I tested it doesn't seem to:\n>\n> git init\n> git commit -m base --allow-empty\n> git commit --squash HEAD -m 'this is the first squash' --allow-empty\n> s=$(git rev-parse HEAD)\n> git commit -m \"squash! $s\" -m 'this is the second squash' --allow-empty\n> git commit -m \"squash! $s\" -m 'this is the third squash' --allow-empty\n> git rebase -ki --autosquash --root\n>\n> So I think there's something I don't quite understand about how the\n> chain of \"next\" works. If you can enlighten me, I'd be grateful.\n\nDitto.\n\n> But your patch does seem to work as advertised. It might be worth adding\n> the double-squash-of-squash to the test.\n\nYes, I think that this is a good, worthwhile addition to the patch.\nSorry Johannes for suggesting that you do more work on an already-great\npatch. No good deed goes unpunished, I guess ;).\n\n> -Peff\n\nThanks,\nTaylor\n"},{"id":"397133","messageId":"xmqqv9la9lbn.fsf@gitster.c.googlers.com","threadId":"53393","inReplyTo":"20200504220916.GF45250@syl.local","subject":"Re: [PATCH] rebase --autosquash: fix a potential segfault","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-05T20:30:04Z","receivedAt":"2020-05-05T20:30:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n>> > +\t\t\t\t/*\n>> > +\t\t\t\t * i2 refers to a fixup commit in the middle of\n>> > +\t\t\t\t * a fixup chain\n>> > +\t\t\t\t */\n>> > +\t\t\t\tnext[i] = next[i2];\n>> > +\t\t\t\tnext[i2] = i;\n>> > +\t\t\t\tcontinue;\n>> > +\t\t\t}\n>>\n>> I do have one question, though. What happens if we add a second\n>> fixup-of-a-fixup?\n>\n> Thanks for asking this question, I was a little curious about it, too.\n\nInteresting that three people looked at the same patch and asked the\nsame question in different ways ;-)\n\n"},{"id":"397143","messageId":"pull.625.v2.git.1588717980225.gitgitgadget@gmail.com","threadId":"53393","inReplyTo":"pull.625.git.1588624804554.gitgitgadget@gmail.com","subject":"[PATCH v2] rebase --autosquash: fix a potential segfault","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-05-05T22:33:00Z","receivedAt":"2020-05-05T22:33:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen rearranging the todo list so that the fixups/squashes are reordered\njust after the commits they intend to fix up, we use two arrays to\nmaintain that list: `next` and `tail`.\n\nThe idea is that `next[i]`, if set to a non-negative value, contains the\nindex of the item that should be rearranged just after the `i`th item.\n\nTo avoid having to walk the entire `next` chain when appending another\nfixup/squash, we also store the end of the `next` chain in `tail[i]`.\n\nThe logic we currently use to update these array items is based on the\nassumption that given a fixup/squash item at index `i`, we just found\nthe index `i2` indicating the first item in that fixup chain.\n\nHowever, as reported by Paul Ganssle, that need not be true: the special\nform `fixup! ` is allowed to point to _another_ fixup\ncommit in the middle of the fixup chain.\n\nExample:\n\n\t* 0192a To fixup\n\t* 02f12 fixup! To fixup\n\t* 03763 fixup! To fixup\n\t* 04ecb fixup! 02f12\n\nNote how the fourth commit targets the second commit, which is already a\nfixup that targets the first commit.\n\nThe good news is that it is easy to fix this: we use the correct\ncondition (we now possibly set `tail[i2]` even for fixups in the middle)\nand we _also_ have to ensure that we _insert_ the item rather than\n_append_ it, i.e. we need to set `next[i2]` accordingly (it might still\nbe set to `-1` if it was actually appended).\n\nReported-by: Paul Ganssle \nHelped-by: Jeff King \nSigned-off-by: Johannes Schindelin \n---\n rebase --autosquash: fix a potential segfault\n \n This patch is in response to \n https://lore.kernel.org/git/017dbc40-8d21-00fb-7b0e-6708d2dcb366@ganssle.io/\n .\n \n Changes since v1:\n \n * Fixed the order of two or more fixups-of-fixups (oddly enough, this\n simplified the patch...)\n * Extended the test to have two fixups-of-fixups, ensuring their order\n is preserved.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-625%2Fdscho%2Fautosquash-segfault-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-625/dscho/autosquash-segfault-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/625\n\nRange-diff vs v1:\n\n 1: bb820acc342 ! 1: de029422324 rebase --autosquash: fix a potential segfault\n @@ Commit message\n index of the item that should be rearranged just after the `i`th item.\n \n To avoid having to walk the entire `next` chain when appending another\n - fixup/squash, we also store the end of the `next` chain in `last[i]`.\n + fixup/squash, we also store the end of the `next` chain in `tail[i]`.\n \n The logic we currently use to update these array items is based on the\n assumption that given a fixup/squash item at index `i`, we just found\n @@ Commit message\n Note how the fourth commit targets the second commit, which is already a\n fixup that targets the first commit.\n \n - The good news is that it is easy to fix this: we can detect the\n - situation by looking at `last[i2]` (which will be `-1` if `i2` is\n - actually in the middle of a fixup chain), and in that case we simply\n - need to squeeze the current item into the middle of the `next` chain,\n - without touching `last` (i.e. leaving the end index of the fixup chain\n - alone).\n + The good news is that it is easy to fix this: we use the correct\n + condition (we now possibly set `tail[i2]` even for fixups in the middle)\n + and we _also_ have to ensure that we _insert_ the item rather than\n + _append_ it, i.e. we need to set `next[i2]` accordingly (it might still\n + be set to `-1` if it was actually appended).\n \n Reported-by: Paul Ganssle \n Helped-by: Jeff King \n @@ Commit message\n \n ## sequencer.c ##\n @@ sequencer.c: int todo_list_rearrange_squash(struct todo_list *todo_list)\n + \t\t\ttodo_list->items[i].command =\n + \t\t\t\tstarts_with(subject, \"fixup!\") ?\n \t\t\t\tTODO_FIXUP : TODO_SQUASH;\n - \t\t\tif (next[i2] < 0)\n +-\t\t\tif (next[i2] < 0)\n ++\t\t\tif (tail[i2] < 0) {\n ++\t\t\t\tnext[i] = next[i2];\n \t\t\t\tnext[i2] = i;\n -\t\t\telse\n -+\t\t\telse if (tail[i2] >= 0)\n ++\t\t\t} else {\n ++\t\t\t\tnext[i] = next[tail[i2]];\n \t\t\t\tnext[tail[i2]] = i;\n -+\t\t\telse {\n -+\t\t\t\t/*\n -+\t\t\t\t * i2 refers to a fixup commit in the middle of\n -+\t\t\t\t * a fixup chain\n -+\t\t\t\t */\n -+\t\t\t\tnext[i] = next[i2];\n -+\t\t\t\tnext[i2] = i;\n -+\t\t\t\tcontinue;\n +\t\t\t}\n \t\t\ttail[i2] = i;\n \t\t} else if (!hashmap_get_from_hash(&subject2item,\n @@ t/t3415-rebase-autosquash.sh: test_expect_success 'abort last squash' '\n +\tgit commit --squash HEAD^ -m Y --allow-empty &&\n +\ttest_tick &&\n +\tgit commit -m \"squash! $(git rev-parse HEAD^)\" -m Z --allow-empty &&\n -+\tgit rebase -ki --autosquash HEAD~4 &&\n -+\ttest XZY = $(git show | tr -cd X-Z)\n ++\ttest_tick &&\n ++\tgit commit -m \"squash! $(git rev-parse HEAD^^)\" -m W --allow-empty &&\n ++\tgit rebase -ki --autosquash HEAD~5 &&\n ++\ttest XZWY = $(git show | tr -cd W-Z)\n +'\n +\n test_done\n\n\n sequencer.c | 7 +++++--\n t/t3415-rebase-autosquash.sh | 16 ++++++++++++++++\n 2 files changed, 21 insertions(+), 2 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex e528225e787..d579f6d6c06 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -5264,10 +5264,13 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)\n \t\t\ttodo_list->items[i].command =\n \t\t\t\tstarts_with(subject, \"fixup!\") ?\n \t\t\t\tTODO_FIXUP : TODO_SQUASH;\n-\t\t\tif (next[i2] < 0)\n+\t\t\tif (tail[i2] < 0) {\n+\t\t\t\tnext[i] = next[i2];\n \t\t\t\tnext[i2] = i;\n-\t\t\telse\n+\t\t\t} else {\n+\t\t\t\tnext[i] = next[tail[i2]];\n \t\t\t\tnext[tail[i2]] = i;\n+\t\t\t}\n \t\t\ttail[i2] = i;\n \t\t} else if (!hashmap_get_from_hash(&subject2item,\n \t\t\t\t\t\tstrhash(subject), subject)) {\ndiff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh\nindex 093de9005b7..7bab6000dc7 100755\n--- a/t/t3415-rebase-autosquash.sh\n+++ b/t/t3415-rebase-autosquash.sh\n@@ -424,4 +424,20 @@ test_expect_success 'abort last squash' '\n \t! grep first actual\n '\n \n+test_expect_success 'fixup a fixup' '\n+\techo 0to-fixup >file0 &&\n+\ttest_tick &&\n+\tgit commit -m \"to-fixup\" file0 &&\n+\ttest_tick &&\n+\tgit commit --squash HEAD -m X --allow-empty &&\n+\ttest_tick &&\n+\tgit commit --squash HEAD^ -m Y --allow-empty &&\n+\ttest_tick &&\n+\tgit commit -m \"squash! $(git rev-parse HEAD^)\" -m Z --allow-empty &&\n+\ttest_tick &&\n+\tgit commit -m \"squash! $(git rev-parse HEAD^^)\" -m W --allow-empty &&\n+\tgit rebase -ki --autosquash HEAD~5 &&\n+\ttest XZWY = $(git show | tr -cd W-Z)\n+'\n+\n test_done\n\nbase-commit: af6b65d45ef179ed52087e80cb089f6b2349f4ec\n-- \ngitgitgadget\n"},{"id":"397190","messageId":"2367cf9d-2e37-b8c2-6881-f3e6c951a460@gmail.com","threadId":"53393","inReplyTo":"pull.625.git.1588624804554.gitgitgadget@gmail.com","subject":"Re: [PATCH] rebase --autosquash: fix a potential segfault","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2020-05-06T15:12:44Z","receivedAt":"2020-05-06T15:12:50Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"On 2020-05-04 22:40, Johannes Schindelin via GitGitGadget wrote:\n> However, as reported by Paul Ganssle, that need not be true: the special\n> form `fixup! ` is allowed to point to _another_ fixup\n> commit in the middle of the fixup chain.\n>\n> Example:\n>\n> \t* 0192a To fixup\n> \t* 02f12 fixup! To fixup\n> \t* 03763 fixup! To fixup\n> \t* 04ecb fixup! 02f12\n\nCould you please clarify if I'm understanding this correctly: does this\naffect the fixups-of-a-fixup which were created by\n\n\tgit commit --fixup=\n\n? For example:\n\n\t* 0192a To fixup\t\n\t* 02f12 fixup! To fixup\t\n\t* 03763 fixup! To fixup\t\n\t* 04ecb fixup! fixup! To fixup\n\nWhere 04ecb was created by pointing option --fixup at 02f12.\n"},{"id":"397240","messageId":"nycvar.QRO.7.76.6.2005062334170.56@tvgsbejvaqbjf.bet","threadId":"53393","inReplyTo":"xmqqv9la9lbn.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] rebase --autosquash: fix a potential segfault","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-05-06T21:35:48Z","receivedAt":"2020-05-06T21:36:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 5 May 2020, Junio C Hamano wrote:\n\n> Taylor Blau writes:\n>\n> >> > +\t\t\t\t/*\n> >> > +\t\t\t\t * i2 refers to a fixup commit in the middle of\n> >> > +\t\t\t\t * a fixup chain\n> >> > +\t\t\t\t */\n> >> > +\t\t\t\tnext[i] = next[i2];\n> >> > +\t\t\t\tnext[i2] = i;\n> >> > +\t\t\t\tcontinue;\n> >> > +\t\t\t}\n> >>\n> >> I do have one question, though. What happens if we add a second\n> >> fixup-of-a-fixup?\n> >\n> > Thanks for asking this question, I was a little curious about it, too.\n>\n> Interesting that three people looked at the same patch and asked the\n> same question in different ways ;-)\n\nIndeed!\n\nI am very grateful, as I had missed that, and it helped me figure out a\nbetter way to do it, and v2 looks a lot nicer, too.\n\nCiao,\nDscho\n"},{"id":"397310","messageId":"nycvar.QRO.7.76.6.2005071626340.56@tvgsbejvaqbjf.bet","threadId":"53393","inReplyTo":"2367cf9d-2e37-b8c2-6881-f3e6c951a460@gmail.com","subject":"Re: [PATCH] rebase --autosquash: fix a potential segfault","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-05-07T14:27:36Z","receivedAt":"2020-05-07T14:37:57Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Andrei,\n\nOn Wed, 6 May 2020, Andrei Rybak wrote:\n\n> On 2020-05-04 22:40, Johannes Schindelin via GitGitGadget wrote:\n> > However, as reported by Paul Ganssle, that need not be true: the special\n> > form `fixup! ` is allowed to point to _another_ fixup\n> > commit in the middle of the fixup chain.\n> >\n> > Example:\n> >\n> > \t* 0192a To fixup\n> > \t* 02f12 fixup! To fixup\n> > \t* 03763 fixup! To fixup\n> > \t* 04ecb fixup! 02f12\n>\n> Could you please clarify if I'm understanding this correctly: does this\n> affect the fixups-of-a-fixup which were created by\n>\n> \tgit commit --fixup=\n>\n> ? For example:\n>\n> \t* 0192a To fixup\n> \t* 02f12 fixup! To fixup\n> \t* 03763 fixup! To fixup\n> \t* 04ecb fixup! fixup! To fixup\n>\n> Where 04ecb was created by pointing option --fixup at 02f12.\n\nNo, it only affects commits whose oneline (i.e. the first line of the\ncommit message) is `fixup! `.\n\nCiao,\nJohannes\n"},{"id":"397336","messageId":"20200507191714.GA25306@coredump.intra.peff.net","threadId":"53393","inReplyTo":"nycvar.QRO.7.76.6.2005062334170.56@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] rebase --autosquash: fix a potential segfault","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-07T19:17:14Z","receivedAt":"2020-05-07T19:17:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 06, 2020 at 11:35:48PM +0200, Johannes Schindelin wrote:\n\n> > >> > +\t\t\t\tnext[i] = next[i2];\n> > >> > +\t\t\t\tnext[i2] = i;\n> > >> > +\t\t\t\tcontinue;\n> > >> > +\t\t\t}\n> > >>\n> > >> I do have one question, though. What happens if we add a second\n> > >> fixup-of-a-fixup?\n> > >\n> > > Thanks for asking this question, I was a little curious about it, too.\n> >\n> > Interesting that three people looked at the same patch and asked the\n> > same question in different ways ;-)\n> \n> Indeed!\n> \n> I am very grateful, as I had missed that, and it helped me figure out a\n> better way to do it, and v2 looks a lot nicer, too.\n\nOK, so your v2 addresses that. Does that mean it was broken in v1? If\nso, then why didn't my test reveal it?\n\nI'm not really doubting that your v2 works so much as trying to\nun-confuse myself about the whole situation (which in turn might lead to\na more intelligent review).\n\n-Peff\n"},{"id":"397421","messageId":"1a03a7b4-f436-83c5-f825-3b68c07785e9@iee.email","threadId":"53393","inReplyTo":"nycvar.QRO.7.76.6.2005071626340.56@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] rebase --autosquash: fix a potential segfault","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.email","sentAt":"2020-05-08T16:43:53Z","receivedAt":"2020-05-08T16:43:57Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"On 07/05/2020 15:27, Johannes Schindelin wrote:\n> Hi Andrei,\n>\n> On Wed, 6 May 2020, Andrei Rybak wrote:\n>\n>> On 2020-05-04 22:40, Johannes Schindelin via GitGitGadget wrote:\n>>> However, as reported by Paul Ganssle, that need not be true: the special\n>>> form `fixup! ` is allowed to point to _another_ fixup\n>>> commit in the middle of the fixup chain.\n>>>\n>>> Example:\n>>>\n>>> \t* 0192a To fixup\n>>> \t* 02f12 fixup! To fixup\n>>> \t* 03763 fixup! To fixup\n>>> \t* 04ecb fixup! 02f12\n>> Could you please clarify if I'm understanding this correctly: does this\n>> affect the fixups-of-a-fixup which were created by\n>>\n>> \tgit commit --fixup=\n>>\n>> ? For example:\n>>\n>> \t* 0192a To fixup\n>> \t* 02f12 fixup! To fixup\n>> \t* 03763 fixup! To fixup\n>> \t* 04ecb fixup! fixup! To fixup\n>>\n>> Where 04ecb was created by pointing option --fixup at 02f12.\n> No, it only affects commits whose oneline (i.e. the first line of the\n> commit message) is `fixup! `.\n>\n> Ciao,\n> Johannes\nIs this ability to have a commit message `fixup! ` documented?\n\nI've looked a few times in the past and didn't find it. The docs for\n`git commit --fixup=` doesn't put the oid in the commit's subject line,\nrather it puts the subject of the referent commit after the \"fixup! \".\n\nSearching from a different direction I've just seen it is mentioned in\nthe v1.7.4 release notes.\n\nWould a doc fix to clarify this be appropriate or have I missed something?\n\nPhilip\n\n"},{"id":"397422","messageId":"4f2ddbba-a9ba-a96a-36c1-b233ea861575@gmail.com","threadId":"53393","inReplyTo":"1a03a7b4-f436-83c5-f825-3b68c07785e9@iee.email","subject":"Re: [PATCH] rebase --autosquash: fix a potential segfault","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2020-05-08T16:57:51Z","receivedAt":"2020-05-08T16:57:57Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"On 2020-05-08 18:43, Philip Oakley wrote:\n> On 07/05/2020 15:27, Johannes Schindelin wrote:\n> Is this ability to have a commit message `fixup! ` documented?\n> I've looked a few times in the past and didn't find it. The docs for\n> `git commit --fixup=` doesn't put the oid in the commit's subject line,\n> rather it puts the subject of the referent commit after the \"fixup! \".\n>\n> Searching from a different direction I've just seen it is mentioned in\n> the v1.7.4 release notes.\n>\n> Would a doc fix to clarify this be appropriate or have I missed something?\n>\n> Philip\n\nYes, it's documented in description of --autosquash: \"A commit matches the `...`\nif the commit subject matches, or if the `...` refers to the commit's hash.\"\n"},{"id":"397425","messageId":"fba3874f-865f-53cc-5901-9dabd4a292c7@iee.email","threadId":"53393","inReplyTo":"4f2ddbba-a9ba-a96a-36c1-b233ea861575@gmail.com","subject":"Re: [PATCH] rebase --autosquash: fix a potential segfault","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.email","sentAt":"2020-05-08T17:21:15Z","receivedAt":"2020-05-08T17:21:19Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"Hi Andrei,\nOn 08/05/2020 17:57, Andrei Rybak wrote:\n> On 2020-05-08 18:43, Philip Oakley wrote:\n>> On 07/05/2020 15:27, Johannes Schindelin wrote:\n>> Is this ability to have a commit message `fixup! ` documented?\n>> I've looked a few times in the past and didn't find it. The docs for\n>> `git commit --fixup=` doesn't put the oid in the commit's subject line,\n>> rather it puts the subject of the referent commit after the \"fixup! \".\n>>\n>> Searching from a different direction I've just seen it is mentioned in\n>> the v1.7.4 release notes.\n>>\n>> Would a doc fix to clarify this be appropriate or have I missed something?\n>>\n>> Philip\n> Yes, it's documented in description of --autosquash: \"A commit matches the `...`\n> if the commit subject matches, or if the `...` refers to the commit's hash.\"\nI've never read it that way, especially given the `git commit fixup=`\ndescription. That one strongly suggests that one starts with the subject\nand then finds the commit id from that.\n\nWe do see in the side discussion that the todo list uses the oid, rather\nthan the subject, which is given verbatim, but the docs are rather\nopaque on the question of oid vs subject. \n\nIf it is only the second guessing of the meaning of those three dots\nthen maybe the docs do need a  bit of clarification.\n\nPhilip\n"},{"id":"397443","messageId":"nycvar.QRO.7.76.6.2005090112530.56@tvgsbejvaqbjf.bet","threadId":"53393","inReplyTo":"20200507191714.GA25306@coredump.intra.peff.net","subject":"Re: [PATCH] rebase --autosquash: fix a potential segfault","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-05-08T23:45:03Z","receivedAt":"2020-05-08T23:45:20Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\n\nOn Thu, 7 May 2020, Jeff King wrote:\n\n> On Wed, May 06, 2020 at 11:35:48PM +0200, Johannes Schindelin wrote:\n>\n> > > >> > +\t\t\t\tnext[i] = next[i2];\n> > > >> > +\t\t\t\tnext[i2] = i;\n> > > >> > +\t\t\t\tcontinue;\n> > > >> > +\t\t\t}\n> > > >>\n> > > >> I do have one question, though. What happens if we add a second\n> > > >> fixup-of-a-fixup?\n> > > >\n> > > > Thanks for asking this question, I was a little curious about it, too.\n> > >\n> > > Interesting that three people looked at the same patch and asked the\n> > > same question in different ways ;-)\n> >\n> > Indeed!\n> >\n> > I am very grateful, as I had missed that, and it helped me figure out a\n> > better way to do it, and v2 looks a lot nicer, too.\n>\n> OK, so your v2 addresses that. Does that mean it was broken in v1?\n\nYes.\n\n> If so, then why didn't my test reveal it?\n\nLet's disect this:\n\n i hash oneline\n#0 1234 foo\n#1 5678 !fixup foo\n#2 abcd !fixup 5678\n#3 dbaf !fixup 5678\n\nLet's follow the original code, i.e. before my v1:\n\nWhen #1 is processed, i.e. when `i == 1`, it finds `i2 == 0` as target. So\nit sets `next[0]` as well as `tail[0]` to 1.\n\nThen #2 is processed, i.e. `i == 2`, and it finds `i2 == 1` as target. It\nsets `next[1]` as well as `tail[1]` to 2.\n\nNow #3 is processed, i.e. it also finds `i2 == 1` as target, so it looks\nat next[1], sees that it is already non-negative, so it sets\n`next[tail[1]]`, i.e. `next[2]` to 3. It also sets `tail[1]` to 3, but\nnobody cares about that because there is no further todo command.\n\nNow, let's follow the code with my v1:\n\nIt actually does the same as before! Why, you ask? Because at no stage is\nthere any non-negative `next[j]` whose corresponding `tail[j]` is\nnegative. (Except after #3 was processed, at that stage, `next[2]` is\nnon-negative but `tail[2]` still is negative, but as I said, noone cares\nbecause there are no remaining todo commands.)\n\nSo the crucial part to trigger this bug is to have a regular `fixup!\n` _between_ the `fixup! ` and the `fixup! `\ntargeting the latter. So I think I can modify your example accordingly:\n\n\t1234 foo\n\t5678 fixup! foo\n\t90ab fixup! foo\n\tabcd fixup! 5678\n\tdbaf fixup! 5678\n\nOr using your actual shell commands:\n\n git commit -m base --allow-empty\n git commit --squash HEAD -m 'this is the first squash' --allow-empty\n s=$(git rev-parse HEAD)\n git commit --fixup HEAD^ --allow-empty # This is the crucial command\n git commit -m \"squash! $s\" -m 'this is the second squash' --allow-empty\n git commit -m \"squash! $s\" -m 'this is the third squash' --allow-empty\n git rebase -ki --autosquash --root\n\nNote the cricual command `git commit --fixup HEAD^`. When processing that,\n`i == 2` and `i2 == 0` (as for `i == 1`), and before v1, this would have\nset `next[1]` but `tail[0]`! With v1, this would have led to #4 and #5\nbeing exchanged. With v2, the role of `tail` would have been extended to\nnot only talk about the beginning of a fixup/squash chain, but about _any_\ntarget of a fixup/squash, even if it is in the middle of a chain.\n\nSo why does this work? Why does it still do the right thing _even after_\ninserting a fixup in the middle of a chain?\n\nThat's the beauty: if I insert anything in the middle of it, the `tail` of\nthe actual beginning of the fixup/squash chain won't need to be changed.\nIt still points to the end of that chain.\n\nAll I need to ensure is that item `i` is not just appended to the \"chain\"\nstarting at `i2`, but that it is _inserted_ at the end of that chain in\ncase that it is actually part of a larger chain, i.e. that its `next[i]`\nis set correctly before making it the immediate successor of the target\ncommit. Since all of the elements in `next` and `tail` are initialized to\n`-1` (i.e. \"no next fixup/squash item after this\"), it will even do the\nright thing when it should actually append: it will set `next[i]` to `-1`\nin that case.\n\n> I'm not really doubting that your v2 works so much as trying to\n> un-confuse myself about the whole situation (which in turn might lead to\n> a more intelligent review).\n\nI wish I was quicker in my responses because I think that this is really\nhelpful a conversation. By \"forcing my hand\" on a thorough explanation,\nyou really help me get clarity for myself about the actual underlying\nissues. So even if I still think that v2 is correct after writing up the\nabove explanation, the degree of my confidence increased substantially.\n\nThanks,\nDscho\n"},{"id":"397468","messageId":"pull.625.v3.git.1589052219528.gitgitgadget@gmail.com","threadId":"53393","inReplyTo":"pull.625.v2.git.1588717980225.gitgitgadget@gmail.com","subject":"[PATCH v3] rebase --autosquash: fix a potential segfault","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-05-09T19:23:39Z","receivedAt":"2020-05-09T19:23:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen rearranging the todo list so that the fixups/squashes are reordered\njust after the commits they intend to fix up, we use two arrays to\nmaintain that list: `next` and `tail`.\n\nThe idea is that `next[i]`, if set to a non-negative value, contains the\nindex of the item that should be rearranged just after the `i`th item.\n\nTo avoid having to walk the entire `next` chain when appending another\nfixup/squash, we also store the end of the `next` chain in `tail[i]`.\n\nThe logic we currently use to update these array items is based on the\nassumption that given a fixup/squash item at index `i`, we just found\nthe index `i2` indicating the first item in that fixup chain.\n\nHowever, as reported by Paul Ganssle, that need not be true: the special\nform `fixup! ` is allowed to point to _another_ fixup\ncommit in the middle of the fixup chain.\n\nExample:\n\n\t* 0192a To fixup\n\t* 02f12 fixup! To fixup\n\t* 03763 fixup! To fixup\n\t* 04ecb fixup! 02f12\n\nNote how the fourth commit targets the second commit, which is already a\nfixup that targets the first commit.\n\nPreviously, we would update `next` and `tail` under our assumption that\nevery `fixup!` commit would find the start of the `fixup!`/`squash!`\nchain. This would lead to a segmentation fault because we would actually\nend up with a `next[i]` pointing to a `fixup!` but the corresponding\n`tail[i]` pointing nowhere, which would the lead to a segmentation\nfault.\n\nLet's fix this by _inserting_, rather than _appending_, the item. In\nother words, if we make a given line successor of another line, we do\nnot simply forget any previously set successor of the latter, but make\nit a successor of the former.\n\nIn the above example, at the point when we insert 04ecb just after\n02f12, 03763 would already be recorded as a successor of 04ecb, and we\nnow \"squeeze in\" 04ecb.\n\nTo complete the idea, we now no longer assume that `next[i]` pointing to\na line means that `last[i]` points to a line, too. Instead, we extend\nthe concept of `last` to cover also partial `fixup!`/`squash!` chains,\ni.e. chains starting in the middle of a larger such chain.\n\nIn the above example, after processing all lines, `last[0]`\n(corresponding to 0192a) would point to 03763, which indeed is the end\nof the overall `fixup!` chain, and `last[1]` (corresponding to 02f12)\nwould point to 04ecb (which is the last `fixup!` targeting 02f12, but it\nhas 03763 as successor, i.e. it is not the end of overall `fixup!`\nchain).\n\nReported-by: Paul Ganssle \nHelped-by: Jeff King \nSigned-off-by: Johannes Schindelin \n---\n rebase --autosquash: fix a potential segfault\n \n This patch is in response to \n https://lore.kernel.org/git/017dbc40-8d21-00fb-7b0e-6708d2dcb366@ganssle.io/\n .\n \n Changes since v2:\n \n * Explained the fix more verbosely in the commit message.\n \n Changes since v1:\n \n * Fixed the order of two or more fixups-of-fixups (oddly enough, this\n simplified the patch...)\n * Extended the test to have two fixups-of-fixups, ensuring their order\n is preserved.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-625%2Fdscho%2Fautosquash-segfault-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-625/dscho/autosquash-segfault-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/625\n\nRange-diff vs v2:\n\n 1: de029422324 ! 1: c1c4607da0e rebase --autosquash: fix a potential segfault\n @@ Commit message\n Note how the fourth commit targets the second commit, which is already a\n fixup that targets the first commit.\n \n - The good news is that it is easy to fix this: we use the correct\n - condition (we now possibly set `tail[i2]` even for fixups in the middle)\n - and we _also_ have to ensure that we _insert_ the item rather than\n - _append_ it, i.e. we need to set `next[i2]` accordingly (it might still\n - be set to `-1` if it was actually appended).\n + Previously, we would update `next` and `tail` under our assumption that\n + every `fixup!` commit would find the start of the `fixup!`/`squash!`\n + chain. This would lead to a segmentation fault because we would actually\n + end up with a `next[i]` pointing to a `fixup!` but the corresponding\n + `tail[i]` pointing nowhere, which would the lead to a segmentation\n + fault.\n +\n + Let's fix this by _inserting_, rather than _appending_, the item. In\n + other words, if we make a given line successor of another line, we do\n + not simply forget any previously set successor of the latter, but make\n + it a successor of the former.\n +\n + In the above example, at the point when we insert 04ecb just after\n + 02f12, 03763 would already be recorded as a successor of 04ecb, and we\n + now \"squeeze in\" 04ecb.\n +\n + To complete the idea, we now no longer assume that `next[i]` pointing to\n + a line means that `last[i]` points to a line, too. Instead, we extend\n + the concept of `last` to cover also partial `fixup!`/`squash!` chains,\n + i.e. chains starting in the middle of a larger such chain.\n +\n + In the above example, after processing all lines, `last[0]`\n + (corresponding to 0192a) would point to 03763, which indeed is the end\n + of the overall `fixup!` chain, and `last[1]` (corresponding to 02f12)\n + would point to 04ecb (which is the last `fixup!` targeting 02f12, but it\n + has 03763 as successor, i.e. it is not the end of overall `fixup!`\n + chain).\n \n Reported-by: Paul Ganssle \n Helped-by: Jeff King \n\n\n sequencer.c | 7 +++++--\n t/t3415-rebase-autosquash.sh | 16 ++++++++++++++++\n 2 files changed, 21 insertions(+), 2 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex e528225e787..d579f6d6c06 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -5264,10 +5264,13 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)\n \t\t\ttodo_list->items[i].command =\n \t\t\t\tstarts_with(subject, \"fixup!\") ?\n \t\t\t\tTODO_FIXUP : TODO_SQUASH;\n-\t\t\tif (next[i2] < 0)\n+\t\t\tif (tail[i2] < 0) {\n+\t\t\t\tnext[i] = next[i2];\n \t\t\t\tnext[i2] = i;\n-\t\t\telse\n+\t\t\t} else {\n+\t\t\t\tnext[i] = next[tail[i2]];\n \t\t\t\tnext[tail[i2]] = i;\n+\t\t\t}\n \t\t\ttail[i2] = i;\n \t\t} else if (!hashmap_get_from_hash(&subject2item,\n \t\t\t\t\t\tstrhash(subject), subject)) {\ndiff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh\nindex 093de9005b7..7bab6000dc7 100755\n--- a/t/t3415-rebase-autosquash.sh\n+++ b/t/t3415-rebase-autosquash.sh\n@@ -424,4 +424,20 @@ test_expect_success 'abort last squash' '\n \t! grep first actual\n '\n \n+test_expect_success 'fixup a fixup' '\n+\techo 0to-fixup >file0 &&\n+\ttest_tick &&\n+\tgit commit -m \"to-fixup\" file0 &&\n+\ttest_tick &&\n+\tgit commit --squash HEAD -m X --allow-empty &&\n+\ttest_tick &&\n+\tgit commit --squash HEAD^ -m Y --allow-empty &&\n+\ttest_tick &&\n+\tgit commit -m \"squash! $(git rev-parse HEAD^)\" -m Z --allow-empty &&\n+\ttest_tick &&\n+\tgit commit -m \"squash! $(git rev-parse HEAD^^)\" -m W --allow-empty &&\n+\tgit rebase -ki --autosquash HEAD~5 &&\n+\ttest XZWY = $(git show | tr -cd W-Z)\n+'\n+\n test_done\n\nbase-commit: af6b65d45ef179ed52087e80cb089f6b2349f4ec\n-- \ngitgitgadget\n"},{"id":"398096","messageId":"fc616e86-dfc1-fccf-d22b-ad2aee0d1b16@iee.email","threadId":"53393","inReplyTo":"4f2ddbba-a9ba-a96a-36c1-b233ea861575@gmail.com","subject":"Re: [PATCH] rebase --autosquash: fix a potential segfault","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.email","sentAt":"2020-05-18T16:47:13Z","receivedAt":"2020-05-18T16:47:18Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"Hi all,\n\nOn 08/05/2020 17:57, Andrei Rybak wrote:\n> On 2020-05-08 18:43, Philip Oakley wrote:\n>> On 07/05/2020 15:27, Johannes Schindelin wrote:\n>> Is this ability to have a commit message `fixup! ` documented?\n>> I've looked a few times in the past and didn't find it. The docs for\n>> `git commit --fixup=` doesn't put the oid in the commit's subject line,\n>> rather it puts the subject of the referent commit after the \"fixup! \".\n>>\n>> Searching from a different direction I've just seen it is mentioned in\n>> the v1.7.4 release notes.\n>>\n>> Would a doc fix to clarify this be appropriate or have I missed something?\n>>\n>> Philip\n> Yes, it's documented in description of --autosquash: \"A commit matches the `...`\n> if the commit subject matches, or if the `...` refers to the commit's hash.\"\n\nThe docs don't  clarify if a full oid has is required, or a unique\nabbreviation within the repository, or just unique within the rebase\ninstruction sheet.\n\nI'd presume that all it needed was the latter, but could easily expect\nthe former (full oid) to be 'matching', while the mid ground may be\nfairly simple to code...\n\nPhilip\n"},{"id":"398134","messageId":"nycvar.QRO.7.76.6.2005180522230.55@tvgsbejvaqbjf.bet","threadId":"53393","inReplyTo":"fc616e86-dfc1-fccf-d22b-ad2aee0d1b16@iee.email","subject":"Re: [PATCH] rebase --autosquash: fix a potential segfault","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-05-18T03:27:50Z","receivedAt":"2020-05-18T19:33:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Philip,\n\nOn Mon, 18 May 2020, Philip Oakley wrote:\n\n> On 08/05/2020 17:57, Andrei Rybak wrote:\n> > On 2020-05-08 18:43, Philip Oakley wrote:\n> >> On 07/05/2020 15:27, Johannes Schindelin wrote:\n> >> Is this ability to have a commit message `fixup! ` documented?\n> >> I've looked a few times in the past and didn't find it. The docs for\n> >> `git commit --fixup=` doesn't put the oid in the commit's subject line,\n> >> rather it puts the subject of the referent commit after the \"fixup! \".\n> >>\n> >> Searching from a different direction I've just seen it is mentioned in\n> >> the v1.7.4 release notes.\n> >>\n> >> Would a doc fix to clarify this be appropriate or have I missed something?\n> >>\n> >> Philip\n> > Yes, it's documented in description of --autosquash: \"A commit matches the `...`\n> > if the commit subject matches, or if the `...` refers to the commit's hash.\"\n>\n> The docs don't  clarify if a full oid has is required, or a unique\n> abbreviation within the repository, or just unique within the rebase\n> instruction sheet.\n\nIt's even worse: _any_ valid reference will do. As you can see from\nhttps://github.com/git/git/blob/efcab5b7a3d2/sequencer.c#L5359-L5381, the\nsearch goes like this:\n\n - For the remainder of the `fixup! ` line:\n\n\t1. If it is identical to the oneline of any commit mentioned in a\n\t previously-seen `pick` line, pick that as target.\n\n\t2. Otherwise, if the remainder can be looked up as a commit\n\t (think: `fixup! master~3`) _and_ that commit was mentioned in\n\t a previously-seen `pick` line, pick that as target.\n\n\t3. If all else fails, and if the remainder is the _start_ of a\n\t oneline of a commit previously seen in a `pick` line, pick that\n\t as target (if multiple lines match, use the first one).\n\nDo feel free to put that into a native-speaker form of a patch to improve\nthe documentation.\n\nCiao,\nDscho\n"},{"id":"398498","messageId":"9a9e7432-7a74-f46e-9a77-b8acaa9a974f@iee.email","threadId":"53393","inReplyTo":"nycvar.QRO.7.76.6.2005180522230.55@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] rebase --autosquash: fix a potential segfault","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.email","sentAt":"2020-05-25T17:29:47Z","receivedAt":"2020-05-25T17:29:52Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"Hi Dscho,\n\nOn 18/05/2020 04:27, Johannes Schindelin wrote:\n> Hi Philip,\n>\n> On Mon, 18 May 2020, Philip Oakley wrote:\n>\n>> On 08/05/2020 17:57, Andrei Rybak wrote:\n>>> On 2020-05-08 18:43, Philip Oakley wrote:\n>>>> On 07/05/2020 15:27, Johannes Schindelin wrote:\n>>>> Is this ability to have a commit message `fixup! ` documented?\n>>>> I've looked a few times in the past and didn't find it. The docs for\n>>>> `git commit --fixup=` doesn't put the oid in the commit's subject line,\n>>>> rather it puts the subject of the referent commit after the \"fixup! \".\n>>>>\n>>>> Searching from a different direction I've just seen it is mentioned in\n>>>> the v1.7.4 release notes.\n>>>>\n>>>> Would a doc fix to clarify this be appropriate or have I missed something?\n>>>>\n>>>> Philip\n>>> Yes, it's documented in description of --autosquash: \"A commit matches the `...`\n>>> if the commit subject matches, or if the `...` refers to the commit's hash.\"\n>> The docs don't  clarify if a full oid has is required, or a unique\n>> abbreviation within the repository, or just unique within the rebase\n>> instruction sheet.\n> It's even worse: _any_ valid reference will do. As you can see from\n> https://github.com/git/git/blob/efcab5b7a3d2/sequencer.c#L5359-L5381, the\n> search goes like this:\n>\n> - For the remainder of the `fixup! ` line:\n>\n> \t1. If it is identical to the oneline of any commit mentioned in a\n> \t previously-seen `pick` line, pick that as target.\n>\n> \t2. Otherwise, if the remainder can be looked up as a commit\n> \t (think: `fixup! master~3`) _and_ that commit was mentioned in\n> \t a previously-seen `pick` line, pick that as target.\n>\n> \t3. If all else fails, and if the remainder is the _start_ of a\n> \t oneline of a commit previously seen in a `pick` line, pick that\n> \t as target (if multiple lines match, use the first one).\n>\n> Do feel free to put that into a native-speaker form of a patch to improve\n> the documentation.\n>\n>\nSorry for the delay on a reply to this one.  I do have a small couple of\npatches to slightly improve the docs. Hope to send soon.\n\nI'm thinking that for the longer term it may need a section covering\nfixup/squash, so as to cover all the different user interaction stages,\ne.g. the commit options, and commit message; and then the initial\ninteractive instruction sheet, and on-going edits of the instruction\nsheet when the rebase pauses.\n\nIn particular, the (assuming proper understanding) the interjection\nbetween the almost identical 1 & 3  [identical vs start of the oneline\nof a commit in the `pick` (insn) list...], of the ability to specify an\nalmost arbitrary rev. I still have to check the code to see what it\ndoes/tries to do.\n\nPhilip \n"},{"id":"398554","messageId":"20200525213632.1626-1-philipoakley@iee.email","threadId":"53393","inReplyTo":"9a9e7432-7a74-f46e-9a77-b8acaa9a974f@iee.email","subject":"[PATCH 0/2] Clarify some of the fixup! documenation","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.email","sentAt":"2020-05-25T21:36:30Z","receivedAt":"2020-05-25T21:44:52Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"It's easy to think that all fixup! commit messages must match a\nprevious subject line based on commit's --fixup option. It had this\ndeveloper confused for many years.\n\nLets at least do a some small changes to make the use of the commit\nhash in the fixup message more obvious.\n\nPhilip\n\nIn-Reply-To: 9a9e7432-7a74-f46e-9a77-b8acaa9a974f@iee.email\nTo: git@vger.kernel.org\ncc: Johannes Schindelin via GitGitGadget \ncc: Andrei Rybak \ncc: Paul Ganssle \ncc: Jeff King \n\n\nPhilip Oakley (2):\n doc: fixup/squash: clarify use of in subject line\n doc: fixup/squash: remove ellipsis marks, use for clarify\n\n Documentation/git-rebase.txt | 18 ++++++++++--------\n 1 file changed, 10 insertions(+), 8 deletions(-)\n\n-- \n2.26.2.windows.1.13.g9dddff6983\n\n"},{"id":"398555","messageId":"20200525213632.1626-3-philipoakley@iee.email","threadId":"53393","inReplyTo":"20200525213632.1626-1-philipoakley@iee.email","subject":"[PATCH 2/2] doc: fixup/squash: remove ellipsis marks, use for clarify","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.email","sentAt":"2020-05-25T21:36:32Z","receivedAt":"2020-05-25T21:44:53Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"Ellipsis marks fail to hint at the typoe or style of the missing content.\n\nTell the reader what is missing, for easier comprehension.\n\nSigned-off-by: Philip Oakley \n---\n\nThe fixup/squash process could probably benefit from its own section\nas there are many places for user interaction with the process.\nThis is a minimal shift toward such an improvement.\n---\n Documentation/git-rebase.txt | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex dfd3d6d0ef..1d8237bfc6 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -534,15 +534,15 @@ See also INCOMPATIBLE OPTIONS below.\n \n --autosquash::\n --no-autosquash::\n-\tWhen the commit log message begins with \"squash! ...\" (or\n-\t\"fixup! ...\"), and there is already a commit in the todo list that\n-\tmatches the same `...`, automatically modify the todo list of rebase\n+\tWhen the commit log message begins with \"squash! \" (or\n+\t\"fixup! \"), and there is already a commit in the todo list that\n+\tmatches the same ``, automatically modify the todo list of rebase\n \t-i so that the commit marked for squashing comes right after the\n \tcommit to be modified, and change the action of the moved commit\n \tfrom `pick` to `squash` (or `fixup`).\n +\n-A commit matches the `...` if\n-the commit subject matches, or if the `...` refers to the commit's\n+A commit matches the `` if\n+the commit subject matches, or if the `` refers to the commit's\n hash. As a fall-back, partial matches of the commit subject work,\n too. The recommended way to create fixup/squash commits is by using\n the `--fixup`/`--squash` options of linkgit:git-commit[1].\n-- \n2.26.2.windows.1.13.g9dddff6983\n\n"},{"id":"398556","messageId":"20200525213632.1626-2-philipoakley@iee.email","threadId":"53393","inReplyTo":"20200525213632.1626-1-philipoakley@iee.email","subject":"[PATCH 1/2] doc: fixup/squash: clarify use of in subject line","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.email","sentAt":"2020-05-25T21:36:31Z","receivedAt":"2020-05-25T21:44:54Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"The option to use the oid hash is buried deep within the fixup/squash\ndocumenation. Split the paragraph so that the option choice is\nmore obvious.\n\nSigned-off-by: Philip Oakley \n---\nThe use of ellision `...` isn't great, as it gives no hint or clue,\nleaving the subsequent test with a difficult explanation.\n\nClarify if a full oid has is required, or a unique abbreviation within\nthe respository, or just uniques within the rebase instruction?\n\nThis is a minimal change that sidesteps the chance to rewrite/clarify\nthe potential wider confusions over specifying the being\nreferred to in the fixup/squash process.\n---\n Documentation/git-rebase.txt | 12 +++++++-----\n 1 file changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex f7a6033607..dfd3d6d0ef 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -539,11 +539,13 @@ See also INCOMPATIBLE OPTIONS below.\n \tmatches the same `...`, automatically modify the todo list of rebase\n \t-i so that the commit marked for squashing comes right after the\n \tcommit to be modified, and change the action of the moved commit\n-\tfrom `pick` to `squash` (or `fixup`). A commit matches the `...` if\n-\tthe commit subject matches, or if the `...` refers to the commit's\n-\thash. As a fall-back, partial matches of the commit subject work,\n-\ttoo. The recommended way to create fixup/squash commits is by using\n-\tthe `--fixup`/`--squash` options of linkgit:git-commit[1].\n+\tfrom `pick` to `squash` (or `fixup`).\n++\n+A commit matches the `...` if\n+the commit subject matches, or if the `...` refers to the commit's\n+hash. As a fall-back, partial matches of the commit subject work,\n+too. The recommended way to create fixup/squash commits is by using\n+the `--fixup`/`--squash` options of linkgit:git-commit[1].\n +\n If the `--autosquash` option is enabled by default using the\n configuration variable `rebase.autoSquash`, this option can be\n-- \n2.26.2.windows.1.13.g9dddff6983\n\n"},{"id":"398673","messageId":"xmqqmu5tl1qa.fsf@gitster.c.googlers.com","threadId":"53393","inReplyTo":"20200525213632.1626-2-philipoakley@iee.email","subject":"Re: [PATCH 1/2] doc: fixup/squash: clarify use of in subject line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-27T17:35:57Z","receivedAt":"2020-05-27T17:36:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Philip Oakley writes:\n\n> The use of ellision `...` isn't great, as it gives no hint or clue,\n> leaving the subsequent test with a difficult explanation.\n\nTrue. If you are planning to correct it in 2/2, then I think it\nmakes more sense to squash that in to have a single patch.\n\n> Clarify if a full oid has is required, or a unique abbreviation within\n> the respository, or just uniques within the rebase instruction?\n\nPuzzled. You must know the answer to \"do we need a full object\nname, or is it sufficient to have anything that gives us a unique\ncommit object name?\" so why not write it in the patch instead of\nasking the question here? Or do you not know the answer and this is\na RFC/WIP patch????\n\n> This is a minimal change that sidesteps the chance to rewrite/clarify\n> the potential wider confusions over specifying the being\n> referred to in the fixup/squash process.\n\nHmph. So this step cannot be reviewed to judge if it is a good\nchange by itself?\n\nLet me locally recreate a squashed single patch and review _that_\ninstead.\n\n> Documentation/git-rebase.txt | 18 ++++++++++--------\n> 1 file changed, 10 insertions(+), 8 deletions(-)\n> \n> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\n> index 4624cfd288..462cb4c52c 100644\n> --- a/Documentation/git-rebase.txt\n> +++ b/Documentation/git-rebase.txt\n> @@ -571,16 +571,18 @@ See also INCOMPATIBLE OPTIONS below.\n> \n> --autosquash::\n> --no-autosquash::\n> -\tWhen the commit log message begins with \"squash! ...\" (or\n> -\t\"fixup! ...\"), and there is already a commit in the todo list that\n> -\tmatches the same `...`, automatically modify the todo list of rebase\n> +\tWhen the commit log message begins with \"squash! \" (or\n> +\t\"fixup! \"), and there is already a commit in the todo list that\n> +\tmatches the same ``, automatically modify the todo list of rebase\n> \t-i so that the commit marked for squashing comes right after the\n> \tcommit to be modified, and change the action of the moved commit\n> +\tfrom `pick` to `squash` (or `fixup`).\n> ++\n> +A commit matches the `` if\n> +the commit subject matches, or if the `` refers to the commit's\n> +hash. As a fall-back, partial matches of the commit subject work,\n> +too. The recommended way to create fixup/squash commits is by using\n> +the `--fixup`/`--squash` options of linkgit:git-commit[1].\n> +\n\nOverall it looks much better than the original.\n\nThe original did not even attempt to define what is a \"match\" for\nthe purpose of this option, so the ellipses may have been OK, but\nonce we need to refer to what is there, we need a name to refer to\nit and ellipses no longer are sufficient, and using the step 1/2\nalone would not make any sense. We definitely should take the step\n2/2 together with it.\n\n\"A commit matches the if the commit subject matches\" is not a\ngreat definition of what a \"match\" is, though. The readers are left\nin the same darkness about what constitutes a \"match\" of \nagainst \"the commit subject\". If you define this \"subject matches\"\nas a substring match, for example, you do not even have to say \"as a\nfall-back\"---it is by (the updated version of your) definition that\nhow the commit subject and matches so there is no need to\nallow any fall-back involved.\n\n\n\n"},{"id":"398878","messageId":"a2ef3923-f3d5-d0cc-61b2-326adaf4ef6b@iee.email","threadId":"53393","inReplyTo":"xmqqmu5tl1qa.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/2] doc: fixup/squash: clarify use of in subject line","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.email","sentAt":"2020-05-29T11:41:01Z","receivedAt":"2020-05-29T11:41:07Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"On 27/05/2020 18:35, Junio C Hamano wrote:\n> Philip Oakley writes:\n>\n>> The use of ellision `...` isn't great, as it gives no hint or clue,\n>> leaving the subsequent test with a difficult explanation.\n> True. If you are planning to correct it in 2/2, then I think it\n> makes more sense to squash that in to have a single patch.\nOK\n>> Clarify if a full oid has is required, or a unique abbreviation within\n>> the respository, or just uniques within the rebase instruction?\n> Puzzled. You must know the answer to \"do we need a full object\n> name, or is it sufficient to have anything that gives us a unique\n> commit object name?\" so why not write it in the patch instead of\n> asking the question here? Or do you not know the answer and this is\n> a RFC/WIP patch????\nThis was a left over note about deeper questions outside of this patch\nseries.\n>\n>> This is a minimal change that sidesteps the chance to rewrite/clarify\n>> the potential wider confusions over specifying the being\n>> referred to in the fixup/squash process.\n> Hmph. So this step cannot be reviewed to judge if it is a good\n> change by itself?\n\nI was working on 'small incremental steps' here.\n>\n> Let me locally recreate a squashed single patch and review _that_\n> instead.\n>\n>> Documentation/git-rebase.txt | 18 ++++++++++--------\n>> 1 file changed, 10 insertions(+), 8 deletions(-)\n>>\n>> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\n>> index 4624cfd288..462cb4c52c 100644\n>> --- a/Documentation/git-rebase.txt\n>> +++ b/Documentation/git-rebase.txt\n>> @@ -571,16 +571,18 @@ See also INCOMPATIBLE OPTIONS below.\n>> \n>> --autosquash::\n>> --no-autosquash::\n>> -\tWhen the commit log message begins with \"squash! ...\" (or\n>> -\t\"fixup! ...\"), and there is already a commit in the todo list that\n>> -\tmatches the same `...`, automatically modify the todo list of rebase\n>> +\tWhen the commit log message begins with \"squash! \" (or\n>> +\t\"fixup! \"), and there is already a commit in the todo list that\n>> +\tmatches the same ``, automatically modify the todo list of rebase\n>> \t-i so that the commit marked for squashing comes right after the\n>> \tcommit to be modified, and change the action of the moved commit\n>> +\tfrom `pick` to `squash` (or `fixup`).\n>> ++\n>> +A commit matches the `` if\n>> +the commit subject matches, or if the `` refers to the commit's\n>> +hash. As a fall-back, partial matches of the commit subject work,\n>> +too. The recommended way to create fixup/squash commits is by using\n>> +the `--fixup`/`--squash` options of linkgit:git-commit[1].\n>> +\n> Overall it looks much better than the original.\n>\n> The original did not even attempt to define what is a \"match\" for\n> the purpose of this option, so the ellipses may have been OK, but\n> once we need to refer to what is there, we need a name to refer to\n> it and ellipses no longer are sufficient, and using the step 1/2\n> alone would not make any sense. We definitely should take the step\n> 2/2 together with it.\nI'd taken the idea of being able to name the thing as step 1, to get\npast the Newspeak problem.\n>\n> \"A commit matches the if the commit subject matches\" is not a\n> great definition of what a \"match\" is, though. The readers are left\n> in the same darkness about what constitutes a \"match\" of \n> against \"the commit subject\". If you define this \"subject matches\"\n> as a substring match, for example, you do not even have to say \"as a\n> fall-back\"---it is by (the updated version of your) definition that\n> how the commit subject and matches so there is no need to\n> allow any fall-back involved.\nThe fall back does include the commit hash, and (not yet in this series)\nis the extra information that Dscho provided at [1], so it's not a\nsimple substring match, nor partial string match.\nPart of this reader confusion comes out of the `commit --fixup` option\nthat effectively directs the reader away from using a hash, to using the\ntarget's full commit message for the fixup! line.\n\nAt this stage, the aim is to make the option for the use of the commit\nhash a bit more visible within the text. Even after many years of\nreading, it still didn't stand out in the old 'wall of text', hence the\nall important paragraph break\n\nI'll combine the two patches at this stage.\n\nPhilip\n\n[1]\nhttps://public-inbox.org/git/nycvar.QRO.7.76.6.2005180522230.55@tvgsbejvaqbjf.bet/    \n\"It's even worse\"\n"}]}