* [PATCH] rebase --autosquash: fix a potential segfault @ 2020-05-04 20:40 Johannes Schindelin via GitGitGadget 2020-05-04 21:19 ` Junio C Hamano ` (3 more replies) 0 siblings, 4 replies; 23+ messages in thread From: Johannes Schindelin via GitGitGadget @ 2020-05-04 20:40 UTC (permalink / raw) To: git; +Cc: Paul Ganssle, Jeff King, Johannes Schindelin, Johannes Schindelin From: Johannes Schindelin <johannes.schindelin@gmx.de> When rearranging the todo list so that the fixups/squashes are reordered just after the commits they intend to fix up, we use two arrays to maintain that list: `next` and `tail`. The idea is that `next[i]`, if set to a non-negative value, contains the index of the item that should be rearranged just after the `i`th item. To avoid having to walk the entire `next` chain when appending another fixup/squash, we also store the end of the `next` chain in `last[i]`. The logic we currently use to update these array items is based on the assumption that given a fixup/squash item at index `i`, we just found the index `i2` indicating the first item in that fixup chain. However, as reported by Paul Ganssle, that need not be true: the special form `fixup! <commit-hash>` is allowed to point to _another_ fixup commit in the middle of the fixup chain. Example: * 0192a To fixup * 02f12 fixup! To fixup * 03763 fixup! To fixup * 04ecb fixup! 02f12 Note how the fourth commit targets the second commit, which is already a fixup that targets the first commit. The good news is that it is easy to fix this: we can detect the situation by looking at `last[i2]` (which will be `-1` if `i2` is actually in the middle of a fixup chain), and in that case we simply need to squeeze the current item into the middle of the `next` chain, without touching `last` (i.e. leaving the end index of the fixup chain alone). Reported-by: Paul Ganssle <paul@ganssle.io> Helped-by: Jeff King <peff@peff.net> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- rebase --autosquash: fix a potential segfault This patch is in response to https://lore.kernel.org/git/017dbc40-8d21-00fb-7b0e-6708d2dcb366@ganssle.io/ . Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-625%2Fdscho%2Fautosquash-segfault-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-625/dscho/autosquash-segfault-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/625 sequencer.c | 11 ++++++++++- t/t3415-rebase-autosquash.sh | 14 ++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/sequencer.c b/sequencer.c index e528225e787..0d4d53d2a49 100644 --- a/sequencer.c +++ b/sequencer.c @@ -5266,8 +5266,17 @@ int todo_list_rearrange_squash(struct todo_list *todo_list) TODO_FIXUP : TODO_SQUASH; if (next[i2] < 0) next[i2] = i; - else + else if (tail[i2] >= 0) next[tail[i2]] = i; + else { + /* + * i2 refers to a fixup commit in the middle of + * a fixup chain + */ + next[i] = next[i2]; + next[i2] = i; + continue; + } tail[i2] = i; } else if (!hashmap_get_from_hash(&subject2item, strhash(subject), subject)) { diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh index 093de9005b7..ca135349346 100755 --- a/t/t3415-rebase-autosquash.sh +++ b/t/t3415-rebase-autosquash.sh @@ -424,4 +424,18 @@ test_expect_success 'abort last squash' ' ! grep first actual ' +test_expect_success 'fixup a fixup' ' + echo 0to-fixup >file0 && + test_tick && + git commit -m "to-fixup" file0 && + test_tick && + git commit --squash HEAD -m X --allow-empty && + test_tick && + git commit --squash HEAD^ -m Y --allow-empty && + test_tick && + git commit -m "squash! $(git rev-parse HEAD^)" -m Z --allow-empty && + git rebase -ki --autosquash HEAD~4 && + test XZY = $(git show | tr -cd X-Z) +' + test_done base-commit: af6b65d45ef179ed52087e80cb089f6b2349f4ec -- gitgitgadget ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH] rebase --autosquash: fix a potential segfault 2020-05-04 20:40 [PATCH] rebase --autosquash: fix a potential segfault Johannes Schindelin via GitGitGadget @ 2020-05-04 21:19 ` Junio C Hamano 2020-05-04 21:33 ` Jeff King ` (2 subsequent siblings) 3 siblings, 0 replies; 23+ messages in thread From: Junio C Hamano @ 2020-05-04 21:19 UTC (permalink / raw) To: Johannes Schindelin via GitGitGadget Cc: git, Paul Ganssle, Jeff King, Johannes Schindelin "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes: > diff --git a/sequencer.c b/sequencer.c > index e528225e787..0d4d53d2a49 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -5266,8 +5266,17 @@ int todo_list_rearrange_squash(struct todo_list *todo_list) > TODO_FIXUP : TODO_SQUASH; > if (next[i2] < 0) > next[i2] = i; > - else > + else if (tail[i2] >= 0) > next[tail[i2]] = i; > + else { > + /* > + * i2 refers to a fixup commit in the middle of > + * a fixup chain > + */ > + next[i] = next[i2]; > + next[i2] = i; > + continue; OK, this would catch the case even when fixing up a fix-up of antoher fix-up, so we won't need further "else if" in the future ;-) I suspect that this breakage is as old as 2.14, introduced by c44a4c65 (rebase -i: rearrange fixup/squash lines using the rebase--helper, 2017-07-14), but perhaps we won't need to backmerge the fix that far. We don't even backport security fixes beyond 2.17 (which is two years old). Just in case I'll queue this immediately on top of f2a04904 (sequencer: refactor rearrange_squash() to work on a todo_list, 2019-03-05); that would give us a potential to cover as far back to 2.20 series. Thanks. > + } > tail[i2] = i; > } else if (!hashmap_get_from_hash(&subject2item, > strhash(subject), subject)) { > diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh > index 093de9005b7..ca135349346 100755 > --- a/t/t3415-rebase-autosquash.sh > +++ b/t/t3415-rebase-autosquash.sh > @@ -424,4 +424,18 @@ test_expect_success 'abort last squash' ' > ! grep first actual > ' > > +test_expect_success 'fixup a fixup' ' > + echo 0to-fixup >file0 && > + test_tick && > + git commit -m "to-fixup" file0 && > + test_tick && > + git commit --squash HEAD -m X --allow-empty && > + test_tick && > + git commit --squash HEAD^ -m Y --allow-empty && > + test_tick && > + git commit -m "squash! $(git rev-parse HEAD^)" -m Z --allow-empty && > + git rebase -ki --autosquash HEAD~4 && > + test XZY = $(git show | tr -cd X-Z) > +' > + > test_done > > base-commit: af6b65d45ef179ed52087e80cb089f6b2349f4ec ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] rebase --autosquash: fix a potential segfault 2020-05-04 20:40 [PATCH] rebase --autosquash: fix a potential segfault Johannes Schindelin via GitGitGadget 2020-05-04 21:19 ` Junio C Hamano @ 2020-05-04 21:33 ` Jeff King 2020-05-04 22:09 ` Taylor Blau 2020-05-05 22:33 ` [PATCH v2] " Johannes Schindelin via GitGitGadget 2020-05-06 15:12 ` [PATCH] " Andrei Rybak 3 siblings, 1 reply; 23+ messages in thread From: Jeff King @ 2020-05-04 21:33 UTC (permalink / raw) To: Johannes Schindelin via GitGitGadget Cc: git, Paul Ganssle, Johannes Schindelin On Mon, May 04, 2020 at 08:40:04PM +0000, Johannes Schindelin via GitGitGadget wrote: > When rearranging the todo list so that the fixups/squashes are reordered > just after the commits they intend to fix up, we use two arrays to > maintain that list: `next` and `tail`. > > The idea is that `next[i]`, if set to a non-negative value, contains the > index of the item that should be rearranged just after the `i`th item. > > To avoid having to walk the entire `next` chain when appending another > fixup/squash, we also store the end of the `next` chain in `last[i]`. s/last/tail/, I think? (and below) > The good news is that it is easy to fix this: we can detect the > situation by looking at `last[i2]` (which will be `-1` if `i2` is > actually in the middle of a fixup chain), and in that case we simply > need to squeeze the current item into the middle of the `next` chain, > without touching `last` (i.e. leaving the end index of the fixup chain > alone). OK, good. I definitely had figured out how to detect the case, but wasn't quite sure how to manipulate next. But your fix here makes sense: > if (next[i2] < 0) > next[i2] = i; > - else > + else if (tail[i2] >= 0) > next[tail[i2]] = i; > + else { > + /* > + * i2 refers to a fixup commit in the middle of > + * a fixup chain > + */ > + next[i] = next[i2]; > + next[i2] = i; > + continue; > + } I do have one question, though. What happens if we add a second fixup-of-a-fixup? We'd see its "next" slot filled, but now pointing to the first fixup-of-a-fixup. And we'd add ourselves at the front of that list. So I think: 1234 foo 5678 !fixup foo abcd !fixup 5678 dbaf !fixup 5678 would end up reordering abcd and dbaf (putting dbaf first), wouldn't it? But when I tested it doesn't seem to: git init git commit -m base --allow-empty git commit --squash HEAD -m 'this is the first squash' --allow-empty s=$(git rev-parse HEAD) git commit -m "squash! $s" -m 'this is the second squash' --allow-empty git commit -m "squash! $s" -m 'this is the third squash' --allow-empty git rebase -ki --autosquash --root So I think there's something I don't quite understand about how the chain of "next" works. If you can enlighten me, I'd be grateful. But your patch does seem to work as advertised. It might be worth adding the double-squash-of-squash to the test. -Peff ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] rebase --autosquash: fix a potential segfault 2020-05-04 21:33 ` Jeff King @ 2020-05-04 22:09 ` Taylor Blau 2020-05-05 20:30 ` Junio C Hamano 0 siblings, 1 reply; 23+ messages in thread From: Taylor Blau @ 2020-05-04 22:09 UTC (permalink / raw) To: Jeff King Cc: Johannes Schindelin via GitGitGadget, git, Paul Ganssle, Johannes Schindelin On Mon, May 04, 2020 at 05:33:26PM -0400, Jeff King wrote: > On Mon, May 04, 2020 at 08:40:04PM +0000, Johannes Schindelin via GitGitGadget wrote: > > > When rearranging the todo list so that the fixups/squashes are reordered > > just after the commits they intend to fix up, we use two arrays to > > maintain that list: `next` and `tail`. > > > > The idea is that `next[i]`, if set to a non-negative value, contains the > > index of the item that should be rearranged just after the `i`th item. > > > > To avoid having to walk the entire `next` chain when appending another > > fixup/squash, we also store the end of the `next` chain in `last[i]`. > > s/last/tail/, I think? (and below) > > > The good news is that it is easy to fix this: we can detect the > > situation by looking at `last[i2]` (which will be `-1` if `i2` is > > actually in the middle of a fixup chain), and in that case we simply > > need to squeeze the current item into the middle of the `next` chain, > > without touching `last` (i.e. leaving the end index of the fixup chain > > alone). > > OK, good. I definitely had figured out how to detect the case, but > wasn't quite sure how to manipulate next. > > But your fix here makes sense: > > > if (next[i2] < 0) > > next[i2] = i; > > - else > > + else if (tail[i2] >= 0) > > next[tail[i2]] = i; > > + else { > > + /* > > + * i2 refers to a fixup commit in the middle of > > + * a fixup chain > > + */ > > + next[i] = next[i2]; > > + next[i2] = i; > > + continue; > > + } > > I do have one question, though. What happens if we add a second > fixup-of-a-fixup? Thanks for asking this question, I was a little curious about it, too. > We'd see its "next" slot filled, but now pointing to the first > fixup-of-a-fixup. And we'd add ourselves at the front of that list. So I > think: > > 1234 foo > 5678 !fixup foo > abcd !fixup 5678 > dbaf !fixup 5678 > > would end up reordering abcd and dbaf (putting dbaf first), wouldn't it? > > But when I tested it doesn't seem to: > > git init > git commit -m base --allow-empty > git commit --squash HEAD -m 'this is the first squash' --allow-empty > s=$(git rev-parse HEAD) > git commit -m "squash! $s" -m 'this is the second squash' --allow-empty > git commit -m "squash! $s" -m 'this is the third squash' --allow-empty > git rebase -ki --autosquash --root > > So I think there's something I don't quite understand about how the > chain of "next" works. If you can enlighten me, I'd be grateful. Ditto. > But your patch does seem to work as advertised. It might be worth adding > the double-squash-of-squash to the test. Yes, I think that this is a good, worthwhile addition to the patch. Sorry Johannes for suggesting that you do more work on an already-great patch. No good deed goes unpunished, I guess ;). > -Peff Thanks, Taylor ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] rebase --autosquash: fix a potential segfault 2020-05-04 22:09 ` Taylor Blau @ 2020-05-05 20:30 ` Junio C Hamano 2020-05-06 21:35 ` Johannes Schindelin 0 siblings, 1 reply; 23+ messages in thread From: Junio C Hamano @ 2020-05-05 20:30 UTC (permalink / raw) To: Taylor Blau Cc: Jeff King, Johannes Schindelin via GitGitGadget, git, Paul Ganssle, Johannes Schindelin Taylor Blau <me@ttaylorr.com> writes: >> > + /* >> > + * i2 refers to a fixup commit in the middle of >> > + * a fixup chain >> > + */ >> > + next[i] = next[i2]; >> > + next[i2] = i; >> > + continue; >> > + } >> >> I do have one question, though. What happens if we add a second >> fixup-of-a-fixup? > > Thanks for asking this question, I was a little curious about it, too. Interesting that three people looked at the same patch and asked the same question in different ways ;-) ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] rebase --autosquash: fix a potential segfault 2020-05-05 20:30 ` Junio C Hamano @ 2020-05-06 21:35 ` Johannes Schindelin 2020-05-07 19:17 ` Jeff King 0 siblings, 1 reply; 23+ messages in thread From: Johannes Schindelin @ 2020-05-06 21:35 UTC (permalink / raw) To: Junio C Hamano Cc: Taylor Blau, Jeff King, Johannes Schindelin via GitGitGadget, git, Paul Ganssle Hi Junio, On Tue, 5 May 2020, Junio C Hamano wrote: > Taylor Blau <me@ttaylorr.com> writes: > > >> > + /* > >> > + * i2 refers to a fixup commit in the middle of > >> > + * a fixup chain > >> > + */ > >> > + next[i] = next[i2]; > >> > + next[i2] = i; > >> > + continue; > >> > + } > >> > >> I do have one question, though. What happens if we add a second > >> fixup-of-a-fixup? > > > > Thanks for asking this question, I was a little curious about it, too. > > Interesting that three people looked at the same patch and asked the > same question in different ways ;-) Indeed! I am very grateful, as I had missed that, and it helped me figure out a better way to do it, and v2 looks a lot nicer, too. Ciao, Dscho ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] rebase --autosquash: fix a potential segfault 2020-05-06 21:35 ` Johannes Schindelin @ 2020-05-07 19:17 ` Jeff King 2020-05-08 23:45 ` Johannes Schindelin 0 siblings, 1 reply; 23+ messages in thread From: Jeff King @ 2020-05-07 19:17 UTC (permalink / raw) To: Johannes Schindelin Cc: Junio C Hamano, Taylor Blau, Johannes Schindelin via GitGitGadget, git, Paul Ganssle On Wed, May 06, 2020 at 11:35:48PM +0200, Johannes Schindelin wrote: > > >> > + next[i] = next[i2]; > > >> > + next[i2] = i; > > >> > + continue; > > >> > + } > > >> > > >> I do have one question, though. What happens if we add a second > > >> fixup-of-a-fixup? > > > > > > Thanks for asking this question, I was a little curious about it, too. > > > > Interesting that three people looked at the same patch and asked the > > same question in different ways ;-) > > Indeed! > > I am very grateful, as I had missed that, and it helped me figure out a > better way to do it, and v2 looks a lot nicer, too. OK, so your v2 addresses that. Does that mean it was broken in v1? If so, then why didn't my test reveal it? I'm not really doubting that your v2 works so much as trying to un-confuse myself about the whole situation (which in turn might lead to a more intelligent review). -Peff ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] rebase --autosquash: fix a potential segfault 2020-05-07 19:17 ` Jeff King @ 2020-05-08 23:45 ` Johannes Schindelin 0 siblings, 0 replies; 23+ messages in thread From: Johannes Schindelin @ 2020-05-08 23:45 UTC (permalink / raw) To: Jeff King Cc: Junio C Hamano, Taylor Blau, Johannes Schindelin via GitGitGadget, git, Paul Ganssle Hi Peff, On Thu, 7 May 2020, Jeff King wrote: > On Wed, May 06, 2020 at 11:35:48PM +0200, Johannes Schindelin wrote: > > > > >> > + next[i] = next[i2]; > > > >> > + next[i2] = i; > > > >> > + continue; > > > >> > + } > > > >> > > > >> I do have one question, though. What happens if we add a second > > > >> fixup-of-a-fixup? > > > > > > > > Thanks for asking this question, I was a little curious about it, too. > > > > > > Interesting that three people looked at the same patch and asked the > > > same question in different ways ;-) > > > > Indeed! > > > > I am very grateful, as I had missed that, and it helped me figure out a > > better way to do it, and v2 looks a lot nicer, too. > > OK, so your v2 addresses that. Does that mean it was broken in v1? Yes. > If so, then why didn't my test reveal it? Let's disect this: i hash oneline #0 1234 foo #1 5678 !fixup foo #2 abcd !fixup 5678 #3 dbaf !fixup 5678 Let's follow the original code, i.e. before my v1: When #1 is processed, i.e. when `i == 1`, it finds `i2 == 0` as target. So it sets `next[0]` as well as `tail[0]` to 1. Then #2 is processed, i.e. `i == 2`, and it finds `i2 == 1` as target. It sets `next[1]` as well as `tail[1]` to 2. Now #3 is processed, i.e. it also finds `i2 == 1` as target, so it looks at next[1], sees that it is already non-negative, so it sets `next[tail[1]]`, i.e. `next[2]` to 3. It also sets `tail[1]` to 3, but nobody cares about that because there is no further todo command. Now, let's follow the code with my v1: It actually does the same as before! Why, you ask? Because at no stage is there any non-negative `next[j]` whose corresponding `tail[j]` is negative. (Except after #3 was processed, at that stage, `next[2]` is non-negative but `tail[2]` still is negative, but as I said, noone cares because there are no remaining todo commands.) So the crucial part to trigger this bug is to have a regular `fixup! <oneline>` _between_ the `fixup! <oneline>` and the `fixup! <hash>` targeting the latter. So I think I can modify your example accordingly: 1234 foo 5678 fixup! foo 90ab fixup! foo abcd fixup! 5678 dbaf fixup! 5678 Or using your actual shell commands: git commit -m base --allow-empty git commit --squash HEAD -m 'this is the first squash' --allow-empty s=$(git rev-parse HEAD) git commit --fixup HEAD^ --allow-empty # This is the crucial command git commit -m "squash! $s" -m 'this is the second squash' --allow-empty git commit -m "squash! $s" -m 'this is the third squash' --allow-empty git rebase -ki --autosquash --root Note the cricual command `git commit --fixup HEAD^`. When processing that, `i == 2` and `i2 == 0` (as for `i == 1`), and before v1, this would have set `next[1]` but `tail[0]`! With v1, this would have led to #4 and #5 being exchanged. With v2, the role of `tail` would have been extended to not only talk about the beginning of a fixup/squash chain, but about _any_ target of a fixup/squash, even if it is in the middle of a chain. So why does this work? Why does it still do the right thing _even after_ inserting a fixup in the middle of a chain? That's the beauty: if I insert anything in the middle of it, the `tail` of the actual beginning of the fixup/squash chain won't need to be changed. It still points to the end of that chain. All I need to ensure is that item `i` is not just appended to the "chain" starting at `i2`, but that it is _inserted_ at the end of that chain in case that it is actually part of a larger chain, i.e. that its `next[i]` is set correctly before making it the immediate successor of the target commit. Since all of the elements in `next` and `tail` are initialized to `-1` (i.e. "no next fixup/squash item after this"), it will even do the right thing when it should actually append: it will set `next[i]` to `-1` in that case. > I'm not really doubting that your v2 works so much as trying to > un-confuse myself about the whole situation (which in turn might lead to > a more intelligent review). I wish I was quicker in my responses because I think that this is really helpful a conversation. By "forcing my hand" on a thorough explanation, you really help me get clarity for myself about the actual underlying issues. So even if I still think that v2 is correct after writing up the above explanation, the degree of my confidence increased substantially. Thanks, Dscho ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2] rebase --autosquash: fix a potential segfault 2020-05-04 20:40 [PATCH] rebase --autosquash: fix a potential segfault Johannes Schindelin via GitGitGadget 2020-05-04 21:19 ` Junio C Hamano 2020-05-04 21:33 ` Jeff King @ 2020-05-05 22:33 ` Johannes Schindelin via GitGitGadget 2020-05-09 19:23 ` [PATCH v3] " Johannes Schindelin via GitGitGadget 2020-05-06 15:12 ` [PATCH] " Andrei Rybak 3 siblings, 1 reply; 23+ messages in thread From: Johannes Schindelin via GitGitGadget @ 2020-05-05 22:33 UTC (permalink / raw) To: git Cc: Paul Ganssle, Jeff King, Taylor Blau, Junio C Hamano, Johannes Schindelin, Johannes Schindelin From: Johannes Schindelin <johannes.schindelin@gmx.de> When rearranging the todo list so that the fixups/squashes are reordered just after the commits they intend to fix up, we use two arrays to maintain that list: `next` and `tail`. The idea is that `next[i]`, if set to a non-negative value, contains the index of the item that should be rearranged just after the `i`th item. To avoid having to walk the entire `next` chain when appending another fixup/squash, we also store the end of the `next` chain in `tail[i]`. The logic we currently use to update these array items is based on the assumption that given a fixup/squash item at index `i`, we just found the index `i2` indicating the first item in that fixup chain. However, as reported by Paul Ganssle, that need not be true: the special form `fixup! <commit-hash>` is allowed to point to _another_ fixup commit in the middle of the fixup chain. Example: * 0192a To fixup * 02f12 fixup! To fixup * 03763 fixup! To fixup * 04ecb fixup! 02f12 Note how the fourth commit targets the second commit, which is already a fixup that targets the first commit. The good news is that it is easy to fix this: we use the correct condition (we now possibly set `tail[i2]` even for fixups in the middle) and we _also_ have to ensure that we _insert_ the item rather than _append_ it, i.e. we need to set `next[i2]` accordingly (it might still be set to `-1` if it was actually appended). Reported-by: Paul Ganssle <paul@ganssle.io> Helped-by: Jeff King <peff@peff.net> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- rebase --autosquash: fix a potential segfault This patch is in response to https://lore.kernel.org/git/017dbc40-8d21-00fb-7b0e-6708d2dcb366@ganssle.io/ . Changes since v1: * Fixed the order of two or more fixups-of-fixups (oddly enough, this simplified the patch...) * Extended the test to have two fixups-of-fixups, ensuring their order is preserved. Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-625%2Fdscho%2Fautosquash-segfault-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-625/dscho/autosquash-segfault-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/625 Range-diff vs v1: 1: bb820acc342 ! 1: de029422324 rebase --autosquash: fix a potential segfault @@ Commit message index of the item that should be rearranged just after the `i`th item. To avoid having to walk the entire `next` chain when appending another - fixup/squash, we also store the end of the `next` chain in `last[i]`. + fixup/squash, we also store the end of the `next` chain in `tail[i]`. The logic we currently use to update these array items is based on the assumption that given a fixup/squash item at index `i`, we just found @@ Commit message Note how the fourth commit targets the second commit, which is already a fixup that targets the first commit. - The good news is that it is easy to fix this: we can detect the - situation by looking at `last[i2]` (which will be `-1` if `i2` is - actually in the middle of a fixup chain), and in that case we simply - need to squeeze the current item into the middle of the `next` chain, - without touching `last` (i.e. leaving the end index of the fixup chain - alone). + The good news is that it is easy to fix this: we use the correct + condition (we now possibly set `tail[i2]` even for fixups in the middle) + and we _also_ have to ensure that we _insert_ the item rather than + _append_ it, i.e. we need to set `next[i2]` accordingly (it might still + be set to `-1` if it was actually appended). Reported-by: Paul Ganssle <paul@ganssle.io> Helped-by: Jeff King <peff@peff.net> @@ Commit message ## sequencer.c ## @@ sequencer.c: int todo_list_rearrange_squash(struct todo_list *todo_list) + todo_list->items[i].command = + starts_with(subject, "fixup!") ? TODO_FIXUP : TODO_SQUASH; - if (next[i2] < 0) +- if (next[i2] < 0) ++ if (tail[i2] < 0) { ++ next[i] = next[i2]; next[i2] = i; - else -+ else if (tail[i2] >= 0) ++ } else { ++ next[i] = next[tail[i2]]; next[tail[i2]] = i; -+ else { -+ /* -+ * i2 refers to a fixup commit in the middle of -+ * a fixup chain -+ */ -+ next[i] = next[i2]; -+ next[i2] = i; -+ continue; + } tail[i2] = i; } else if (!hashmap_get_from_hash(&subject2item, @@ t/t3415-rebase-autosquash.sh: test_expect_success 'abort last squash' ' + git commit --squash HEAD^ -m Y --allow-empty && + test_tick && + git commit -m "squash! $(git rev-parse HEAD^)" -m Z --allow-empty && -+ git rebase -ki --autosquash HEAD~4 && -+ test XZY = $(git show | tr -cd X-Z) ++ test_tick && ++ git commit -m "squash! $(git rev-parse HEAD^^)" -m W --allow-empty && ++ git rebase -ki --autosquash HEAD~5 && ++ test XZWY = $(git show | tr -cd W-Z) +' + test_done sequencer.c | 7 +++++-- t/t3415-rebase-autosquash.sh | 16 ++++++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/sequencer.c b/sequencer.c index e528225e787..d579f6d6c06 100644 --- a/sequencer.c +++ b/sequencer.c @@ -5264,10 +5264,13 @@ int todo_list_rearrange_squash(struct todo_list *todo_list) todo_list->items[i].command = starts_with(subject, "fixup!") ? TODO_FIXUP : TODO_SQUASH; - if (next[i2] < 0) + if (tail[i2] < 0) { + next[i] = next[i2]; next[i2] = i; - else + } else { + next[i] = next[tail[i2]]; next[tail[i2]] = i; + } tail[i2] = i; } else if (!hashmap_get_from_hash(&subject2item, strhash(subject), subject)) { diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh index 093de9005b7..7bab6000dc7 100755 --- a/t/t3415-rebase-autosquash.sh +++ b/t/t3415-rebase-autosquash.sh @@ -424,4 +424,20 @@ test_expect_success 'abort last squash' ' ! grep first actual ' +test_expect_success 'fixup a fixup' ' + echo 0to-fixup >file0 && + test_tick && + git commit -m "to-fixup" file0 && + test_tick && + git commit --squash HEAD -m X --allow-empty && + test_tick && + git commit --squash HEAD^ -m Y --allow-empty && + test_tick && + git commit -m "squash! $(git rev-parse HEAD^)" -m Z --allow-empty && + test_tick && + git commit -m "squash! $(git rev-parse HEAD^^)" -m W --allow-empty && + git rebase -ki --autosquash HEAD~5 && + test XZWY = $(git show | tr -cd W-Z) +' + test_done base-commit: af6b65d45ef179ed52087e80cb089f6b2349f4ec -- gitgitgadget ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v3] rebase --autosquash: fix a potential segfault 2020-05-05 22:33 ` [PATCH v2] " Johannes Schindelin via GitGitGadget @ 2020-05-09 19:23 ` Johannes Schindelin via GitGitGadget 0 siblings, 0 replies; 23+ messages in thread From: Johannes Schindelin via GitGitGadget @ 2020-05-09 19:23 UTC (permalink / raw) To: git Cc: Paul Ganssle, Jeff King, Taylor Blau, Junio C Hamano, Johannes Schindelin, Johannes Schindelin From: Johannes Schindelin <johannes.schindelin@gmx.de> When rearranging the todo list so that the fixups/squashes are reordered just after the commits they intend to fix up, we use two arrays to maintain that list: `next` and `tail`. The idea is that `next[i]`, if set to a non-negative value, contains the index of the item that should be rearranged just after the `i`th item. To avoid having to walk the entire `next` chain when appending another fixup/squash, we also store the end of the `next` chain in `tail[i]`. The logic we currently use to update these array items is based on the assumption that given a fixup/squash item at index `i`, we just found the index `i2` indicating the first item in that fixup chain. However, as reported by Paul Ganssle, that need not be true: the special form `fixup! <commit-hash>` is allowed to point to _another_ fixup commit in the middle of the fixup chain. Example: * 0192a To fixup * 02f12 fixup! To fixup * 03763 fixup! To fixup * 04ecb fixup! 02f12 Note how the fourth commit targets the second commit, which is already a fixup that targets the first commit. Previously, we would update `next` and `tail` under our assumption that every `fixup!` commit would find the start of the `fixup!`/`squash!` chain. This would lead to a segmentation fault because we would actually end up with a `next[i]` pointing to a `fixup!` but the corresponding `tail[i]` pointing nowhere, which would the lead to a segmentation fault. Let's fix this by _inserting_, rather than _appending_, the item. In other words, if we make a given line successor of another line, we do not simply forget any previously set successor of the latter, but make it a successor of the former. In the above example, at the point when we insert 04ecb just after 02f12, 03763 would already be recorded as a successor of 04ecb, and we now "squeeze in" 04ecb. To complete the idea, we now no longer assume that `next[i]` pointing to a line means that `last[i]` points to a line, too. Instead, we extend the concept of `last` to cover also partial `fixup!`/`squash!` chains, i.e. chains starting in the middle of a larger such chain. In the above example, after processing all lines, `last[0]` (corresponding to 0192a) would point to 03763, which indeed is the end of the overall `fixup!` chain, and `last[1]` (corresponding to 02f12) would point to 04ecb (which is the last `fixup!` targeting 02f12, but it has 03763 as successor, i.e. it is not the end of overall `fixup!` chain). Reported-by: Paul Ganssle <paul@ganssle.io> Helped-by: Jeff King <peff@peff.net> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- rebase --autosquash: fix a potential segfault This patch is in response to https://lore.kernel.org/git/017dbc40-8d21-00fb-7b0e-6708d2dcb366@ganssle.io/ . Changes since v2: * Explained the fix more verbosely in the commit message. Changes since v1: * Fixed the order of two or more fixups-of-fixups (oddly enough, this simplified the patch...) * Extended the test to have two fixups-of-fixups, ensuring their order is preserved. Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-625%2Fdscho%2Fautosquash-segfault-v3 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-625/dscho/autosquash-segfault-v3 Pull-Request: https://github.com/gitgitgadget/git/pull/625 Range-diff vs v2: 1: de029422324 ! 1: c1c4607da0e rebase --autosquash: fix a potential segfault @@ Commit message Note how the fourth commit targets the second commit, which is already a fixup that targets the first commit. - The good news is that it is easy to fix this: we use the correct - condition (we now possibly set `tail[i2]` even for fixups in the middle) - and we _also_ have to ensure that we _insert_ the item rather than - _append_ it, i.e. we need to set `next[i2]` accordingly (it might still - be set to `-1` if it was actually appended). + Previously, we would update `next` and `tail` under our assumption that + every `fixup!` commit would find the start of the `fixup!`/`squash!` + chain. This would lead to a segmentation fault because we would actually + end up with a `next[i]` pointing to a `fixup!` but the corresponding + `tail[i]` pointing nowhere, which would the lead to a segmentation + fault. + + Let's fix this by _inserting_, rather than _appending_, the item. In + other words, if we make a given line successor of another line, we do + not simply forget any previously set successor of the latter, but make + it a successor of the former. + + In the above example, at the point when we insert 04ecb just after + 02f12, 03763 would already be recorded as a successor of 04ecb, and we + now "squeeze in" 04ecb. + + To complete the idea, we now no longer assume that `next[i]` pointing to + a line means that `last[i]` points to a line, too. Instead, we extend + the concept of `last` to cover also partial `fixup!`/`squash!` chains, + i.e. chains starting in the middle of a larger such chain. + + In the above example, after processing all lines, `last[0]` + (corresponding to 0192a) would point to 03763, which indeed is the end + of the overall `fixup!` chain, and `last[1]` (corresponding to 02f12) + would point to 04ecb (which is the last `fixup!` targeting 02f12, but it + has 03763 as successor, i.e. it is not the end of overall `fixup!` + chain). Reported-by: Paul Ganssle <paul@ganssle.io> Helped-by: Jeff King <peff@peff.net> sequencer.c | 7 +++++-- t/t3415-rebase-autosquash.sh | 16 ++++++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/sequencer.c b/sequencer.c index e528225e787..d579f6d6c06 100644 --- a/sequencer.c +++ b/sequencer.c @@ -5264,10 +5264,13 @@ int todo_list_rearrange_squash(struct todo_list *todo_list) todo_list->items[i].command = starts_with(subject, "fixup!") ? TODO_FIXUP : TODO_SQUASH; - if (next[i2] < 0) + if (tail[i2] < 0) { + next[i] = next[i2]; next[i2] = i; - else + } else { + next[i] = next[tail[i2]]; next[tail[i2]] = i; + } tail[i2] = i; } else if (!hashmap_get_from_hash(&subject2item, strhash(subject), subject)) { diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh index 093de9005b7..7bab6000dc7 100755 --- a/t/t3415-rebase-autosquash.sh +++ b/t/t3415-rebase-autosquash.sh @@ -424,4 +424,20 @@ test_expect_success 'abort last squash' ' ! grep first actual ' +test_expect_success 'fixup a fixup' ' + echo 0to-fixup >file0 && + test_tick && + git commit -m "to-fixup" file0 && + test_tick && + git commit --squash HEAD -m X --allow-empty && + test_tick && + git commit --squash HEAD^ -m Y --allow-empty && + test_tick && + git commit -m "squash! $(git rev-parse HEAD^)" -m Z --allow-empty && + test_tick && + git commit -m "squash! $(git rev-parse HEAD^^)" -m W --allow-empty && + git rebase -ki --autosquash HEAD~5 && + test XZWY = $(git show | tr -cd W-Z) +' + test_done base-commit: af6b65d45ef179ed52087e80cb089f6b2349f4ec -- gitgitgadget ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH] rebase --autosquash: fix a potential segfault 2020-05-04 20:40 [PATCH] rebase --autosquash: fix a potential segfault Johannes Schindelin via GitGitGadget ` (2 preceding siblings ...) 2020-05-05 22:33 ` [PATCH v2] " Johannes Schindelin via GitGitGadget @ 2020-05-06 15:12 ` Andrei Rybak 2020-05-07 14:27 ` Johannes Schindelin 3 siblings, 1 reply; 23+ messages in thread From: Andrei Rybak @ 2020-05-06 15:12 UTC (permalink / raw) To: Johannes Schindelin via GitGitGadget, git Cc: Paul Ganssle, Jeff King, Johannes Schindelin On 2020-05-04 22:40, Johannes Schindelin via GitGitGadget wrote: > However, as reported by Paul Ganssle, that need not be true: the special > form `fixup! <commit-hash>` is allowed to point to _another_ fixup > commit in the middle of the fixup chain. > > Example: > > * 0192a To fixup > * 02f12 fixup! To fixup > * 03763 fixup! To fixup > * 04ecb fixup! 02f12 Could you please clarify if I'm understanding this correctly: does this affect the fixups-of-a-fixup which were created by git commit --fixup=<pointer to previous fixup! commit> ? For example: * 0192a To fixup * 02f12 fixup! To fixup * 03763 fixup! To fixup * 04ecb fixup! fixup! To fixup Where 04ecb was created by pointing option --fixup at 02f12. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] rebase --autosquash: fix a potential segfault 2020-05-06 15:12 ` [PATCH] " Andrei Rybak @ 2020-05-07 14:27 ` Johannes Schindelin 2020-05-08 16:43 ` Philip Oakley 0 siblings, 1 reply; 23+ messages in thread From: Johannes Schindelin @ 2020-05-07 14:27 UTC (permalink / raw) To: Andrei Rybak Cc: Johannes Schindelin via GitGitGadget, git, Paul Ganssle, Jeff King Hi Andrei, On Wed, 6 May 2020, Andrei Rybak wrote: > On 2020-05-04 22:40, Johannes Schindelin via GitGitGadget wrote: > > However, as reported by Paul Ganssle, that need not be true: the special > > form `fixup! <commit-hash>` is allowed to point to _another_ fixup > > commit in the middle of the fixup chain. > > > > Example: > > > > * 0192a To fixup > > * 02f12 fixup! To fixup > > * 03763 fixup! To fixup > > * 04ecb fixup! 02f12 > > Could you please clarify if I'm understanding this correctly: does this > affect the fixups-of-a-fixup which were created by > > git commit --fixup=<pointer to previous fixup! commit> > > ? For example: > > * 0192a To fixup > * 02f12 fixup! To fixup > * 03763 fixup! To fixup > * 04ecb fixup! fixup! To fixup > > Where 04ecb was created by pointing option --fixup at 02f12. No, it only affects commits whose oneline (i.e. the first line of the commit message) is `fixup! <commit-hash>`. Ciao, Johannes ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] rebase --autosquash: fix a potential segfault 2020-05-07 14:27 ` Johannes Schindelin @ 2020-05-08 16:43 ` Philip Oakley 2020-05-08 16:57 ` Andrei Rybak 0 siblings, 1 reply; 23+ messages in thread From: Philip Oakley @ 2020-05-08 16:43 UTC (permalink / raw) To: Johannes Schindelin, Andrei Rybak Cc: Johannes Schindelin via GitGitGadget, git, Paul Ganssle, Jeff King On 07/05/2020 15:27, Johannes Schindelin wrote: > Hi Andrei, > > On Wed, 6 May 2020, Andrei Rybak wrote: > >> On 2020-05-04 22:40, Johannes Schindelin via GitGitGadget wrote: >>> However, as reported by Paul Ganssle, that need not be true: the special >>> form `fixup! <commit-hash>` is allowed to point to _another_ fixup >>> commit in the middle of the fixup chain. >>> >>> Example: >>> >>> * 0192a To fixup >>> * 02f12 fixup! To fixup >>> * 03763 fixup! To fixup >>> * 04ecb fixup! 02f12 >> Could you please clarify if I'm understanding this correctly: does this >> affect the fixups-of-a-fixup which were created by >> >> git commit --fixup=<pointer to previous fixup! commit> >> >> ? For example: >> >> * 0192a To fixup >> * 02f12 fixup! To fixup >> * 03763 fixup! To fixup >> * 04ecb fixup! fixup! To fixup >> >> Where 04ecb was created by pointing option --fixup at 02f12. > No, it only affects commits whose oneline (i.e. the first line of the > commit message) is `fixup! <commit-hash>`. > > Ciao, > Johannes Is this ability to have a commit message `fixup! <commit-hash>` documented? I've looked a few times in the past and didn't find it. The docs for `git commit --fixup=` doesn't put the oid in the commit's subject line, rather it puts the subject of the referent commit after the "fixup! ". Searching from a different direction I've just seen it is mentioned in the v1.7.4 release notes. Would a doc fix to clarify this be appropriate or have I missed something? Philip ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] rebase --autosquash: fix a potential segfault 2020-05-08 16:43 ` Philip Oakley @ 2020-05-08 16:57 ` Andrei Rybak 2020-05-08 17:21 ` Philip Oakley 2020-05-18 16:47 ` Philip Oakley 0 siblings, 2 replies; 23+ messages in thread From: Andrei Rybak @ 2020-05-08 16:57 UTC (permalink / raw) To: Philip Oakley, Johannes Schindelin Cc: Johannes Schindelin via GitGitGadget, git, Paul Ganssle, Jeff King On 2020-05-08 18:43, Philip Oakley wrote: > On 07/05/2020 15:27, Johannes Schindelin wrote: > Is this ability to have a commit message `fixup! <commit-hash>` documented? > I've looked a few times in the past and didn't find it. The docs for > `git commit --fixup=` doesn't put the oid in the commit's subject line, > rather it puts the subject of the referent commit after the "fixup! ". > > Searching from a different direction I've just seen it is mentioned in > the v1.7.4 release notes. > > Would a doc fix to clarify this be appropriate or have I missed something? > > Philip Yes, it's documented in description of --autosquash: "A commit matches the `...` if the commit subject matches, or if the `...` refers to the commit's hash." ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] rebase --autosquash: fix a potential segfault 2020-05-08 16:57 ` Andrei Rybak @ 2020-05-08 17:21 ` Philip Oakley 2020-05-18 16:47 ` Philip Oakley 1 sibling, 0 replies; 23+ messages in thread From: Philip Oakley @ 2020-05-08 17:21 UTC (permalink / raw) To: Andrei Rybak, Johannes Schindelin Cc: Johannes Schindelin via GitGitGadget, git, Paul Ganssle, Jeff King Hi Andrei, On 08/05/2020 17:57, Andrei Rybak wrote: > On 2020-05-08 18:43, Philip Oakley wrote: >> On 07/05/2020 15:27, Johannes Schindelin wrote: >> Is this ability to have a commit message `fixup! <commit-hash>` documented? >> I've looked a few times in the past and didn't find it. The docs for >> `git commit --fixup=` doesn't put the oid in the commit's subject line, >> rather it puts the subject of the referent commit after the "fixup! ". >> >> Searching from a different direction I've just seen it is mentioned in >> the v1.7.4 release notes. >> >> Would a doc fix to clarify this be appropriate or have I missed something? >> >> Philip > Yes, it's documented in description of --autosquash: "A commit matches the `...` > if the commit subject matches, or if the `...` refers to the commit's hash." I've never read it that way, especially given the `git commit fixup=` description. That one strongly suggests that one starts with the subject and then finds the commit id from that. We do see in the side discussion that the todo list uses the oid, rather than the subject, which is given verbatim, but the docs are rather opaque on the question of oid vs subject. If it is only the second guessing of the meaning of those three dots then maybe the docs do need a bit of clarification. Philip ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] rebase --autosquash: fix a potential segfault 2020-05-08 16:57 ` Andrei Rybak 2020-05-08 17:21 ` Philip Oakley @ 2020-05-18 16:47 ` Philip Oakley 2020-05-18 3:27 ` Johannes Schindelin 1 sibling, 1 reply; 23+ messages in thread From: Philip Oakley @ 2020-05-18 16:47 UTC (permalink / raw) To: Andrei Rybak, Johannes Schindelin Cc: Johannes Schindelin via GitGitGadget, git, Paul Ganssle, Jeff King Hi all, On 08/05/2020 17:57, Andrei Rybak wrote: > On 2020-05-08 18:43, Philip Oakley wrote: >> On 07/05/2020 15:27, Johannes Schindelin wrote: >> Is this ability to have a commit message `fixup! <commit-hash>` documented? >> I've looked a few times in the past and didn't find it. The docs for >> `git commit --fixup=` doesn't put the oid in the commit's subject line, >> rather it puts the subject of the referent commit after the "fixup! ". >> >> Searching from a different direction I've just seen it is mentioned in >> the v1.7.4 release notes. >> >> Would a doc fix to clarify this be appropriate or have I missed something? >> >> Philip > Yes, it's documented in description of --autosquash: "A commit matches the `...` > if the commit subject matches, or if the `...` refers to the commit's hash." The docs don't clarify if a full oid has is required, or a unique abbreviation within the repository, or just unique within the rebase instruction sheet. I'd presume that all it needed was the latter, but could easily expect the former (full oid) to be 'matching', while the mid ground may be fairly simple to code... Philip ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] rebase --autosquash: fix a potential segfault 2020-05-18 16:47 ` Philip Oakley @ 2020-05-18 3:27 ` Johannes Schindelin 2020-05-25 17:29 ` Philip Oakley 0 siblings, 1 reply; 23+ messages in thread From: Johannes Schindelin @ 2020-05-18 3:27 UTC (permalink / raw) To: Philip Oakley Cc: Andrei Rybak, Johannes Schindelin via GitGitGadget, git, Paul Ganssle, Jeff King [-- Attachment #1: Type: text/plain, Size: 1925 bytes --] Hi Philip, On Mon, 18 May 2020, Philip Oakley wrote: > On 08/05/2020 17:57, Andrei Rybak wrote: > > On 2020-05-08 18:43, Philip Oakley wrote: > >> On 07/05/2020 15:27, Johannes Schindelin wrote: > >> Is this ability to have a commit message `fixup! <commit-hash>` documented? > >> I've looked a few times in the past and didn't find it. The docs for > >> `git commit --fixup=` doesn't put the oid in the commit's subject line, > >> rather it puts the subject of the referent commit after the "fixup! ". > >> > >> Searching from a different direction I've just seen it is mentioned in > >> the v1.7.4 release notes. > >> > >> Would a doc fix to clarify this be appropriate or have I missed something? > >> > >> Philip > > Yes, it's documented in description of --autosquash: "A commit matches the `...` > > if the commit subject matches, or if the `...` refers to the commit's hash." > > The docs don't clarify if a full oid has is required, or a unique > abbreviation within the repository, or just unique within the rebase > instruction sheet. It's even worse: _any_ valid reference will do. As you can see from https://github.com/git/git/blob/efcab5b7a3d2/sequencer.c#L5359-L5381, the search goes like this: - For the remainder of the `fixup! <remainder>` line: 1. If it is identical to the oneline of any commit mentioned in a previously-seen `pick` line, pick that as target. 2. Otherwise, if the remainder can be looked up as a commit (think: `fixup! master~3`) _and_ that commit was mentioned in a previously-seen `pick` line, pick that as target. 3. If all else fails, and if the remainder is the _start_ of a oneline of a commit previously seen in a `pick` line, pick that as target (if multiple lines match, use the first one). Do feel free to put that into a native-speaker form of a patch to improve the documentation. Ciao, Dscho ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] rebase --autosquash: fix a potential segfault 2020-05-18 3:27 ` Johannes Schindelin @ 2020-05-25 17:29 ` Philip Oakley 2020-05-25 21:36 ` [PATCH 0/2] Clarify some of the fixup! documenation Philip Oakley 0 siblings, 1 reply; 23+ messages in thread From: Philip Oakley @ 2020-05-25 17:29 UTC (permalink / raw) To: Johannes Schindelin Cc: Andrei Rybak, Johannes Schindelin via GitGitGadget, git, Paul Ganssle, Jeff King Hi Dscho, On 18/05/2020 04:27, Johannes Schindelin wrote: > Hi Philip, > > On Mon, 18 May 2020, Philip Oakley wrote: > >> On 08/05/2020 17:57, Andrei Rybak wrote: >>> On 2020-05-08 18:43, Philip Oakley wrote: >>>> On 07/05/2020 15:27, Johannes Schindelin wrote: >>>> Is this ability to have a commit message `fixup! <commit-hash>` documented? >>>> I've looked a few times in the past and didn't find it. The docs for >>>> `git commit --fixup=` doesn't put the oid in the commit's subject line, >>>> rather it puts the subject of the referent commit after the "fixup! ". >>>> >>>> Searching from a different direction I've just seen it is mentioned in >>>> the v1.7.4 release notes. >>>> >>>> Would a doc fix to clarify this be appropriate or have I missed something? >>>> >>>> Philip >>> Yes, it's documented in description of --autosquash: "A commit matches the `...` >>> if the commit subject matches, or if the `...` refers to the commit's hash." >> The docs don't clarify if a full oid has is required, or a unique >> abbreviation within the repository, or just unique within the rebase >> instruction sheet. > It's even worse: _any_ valid reference will do. As you can see from > https://github.com/git/git/blob/efcab5b7a3d2/sequencer.c#L5359-L5381, the > search goes like this: > > - For the remainder of the `fixup! <remainder>` line: > > 1. If it is identical to the oneline of any commit mentioned in a > previously-seen `pick` line, pick that as target. > > 2. Otherwise, if the remainder can be looked up as a commit > (think: `fixup! master~3`) _and_ that commit was mentioned in > a previously-seen `pick` line, pick that as target. > > 3. If all else fails, and if the remainder is the _start_ of a > oneline of a commit previously seen in a `pick` line, pick that > as target (if multiple lines match, use the first one). > > Do feel free to put that into a native-speaker form of a patch to improve > the documentation. > > Sorry for the delay on a reply to this one. I do have a small couple of patches to slightly improve the docs. Hope to send soon. I'm thinking that for the longer term it may need a section covering fixup/squash, so as to cover all the different user interaction stages, e.g. the commit options, and commit message; and then the initial interactive instruction sheet, and on-going edits of the instruction sheet when the rebase pauses. In particular, the (assuming proper understanding) the interjection between the almost identical 1 & 3 [identical vs start of the oneline of a commit in the `pick` (insn) list...], of the ability to specify an almost arbitrary rev. I still have to check the code to see what it does/tries to do. Philip ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 0/2] Clarify some of the fixup! documenation 2020-05-25 17:29 ` Philip Oakley @ 2020-05-25 21:36 ` Philip Oakley 2020-05-25 21:36 ` [PATCH 1/2] doc: fixup/squash: clarify use of <oid-hash> in subject line Philip Oakley 2020-05-25 21:36 ` [PATCH 2/2] doc: fixup/squash: remove ellipsis marks, use <line> for clarify Philip Oakley 0 siblings, 2 replies; 23+ messages in thread From: Philip Oakley @ 2020-05-25 21:36 UTC (permalink / raw) To: git Cc: Johannes Schindelin via GitGitGadget, Andrei Rybak, Paul Ganssle, Jeff King It's easy to think that all fixup! commit messages must match a previous subject line based on commit's --fixup option. It had this developer confused for many years. Lets at least do a some small changes to make the use of the commit hash in the fixup message more obvious. Philip In-Reply-To: 9a9e7432-7a74-f46e-9a77-b8acaa9a974f@iee.email To: git@vger.kernel.org cc: Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com> cc: Andrei Rybak <rybak.a.v@gmail.com> cc: Paul Ganssle <paul@ganssle.io> cc: Jeff King <peff@peff.net> Philip Oakley (2): doc: fixup/squash: clarify use of <oid-hash> in subject line doc: fixup/squash: remove ellipsis marks, use <line> for clarify Documentation/git-rebase.txt | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) -- 2.26.2.windows.1.13.g9dddff6983 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 1/2] doc: fixup/squash: clarify use of <oid-hash> in subject line 2020-05-25 21:36 ` [PATCH 0/2] Clarify some of the fixup! documenation Philip Oakley @ 2020-05-25 21:36 ` Philip Oakley 2020-05-27 17:35 ` Junio C Hamano 2020-05-25 21:36 ` [PATCH 2/2] doc: fixup/squash: remove ellipsis marks, use <line> for clarify Philip Oakley 1 sibling, 1 reply; 23+ messages in thread From: Philip Oakley @ 2020-05-25 21:36 UTC (permalink / raw) To: git The option to use the oid hash is buried deep within the fixup/squash documenation. Split the paragraph so that the option choice is more obvious. Signed-off-by: Philip Oakley <philipoakley@iee.email> --- The use of ellision `...` isn't great, as it gives no hint or clue, leaving the subsequent test with a difficult explanation. Clarify if a full oid has is required, or a unique abbreviation within the respository, or just uniques within the rebase instruction? This is a minimal change that sidesteps the chance to rewrite/clarify the potential wider confusions over specifying the <commit> being referred to in the fixup/squash process. --- Documentation/git-rebase.txt | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt index f7a6033607..dfd3d6d0ef 100644 --- a/Documentation/git-rebase.txt +++ b/Documentation/git-rebase.txt @@ -539,11 +539,13 @@ See also INCOMPATIBLE OPTIONS below. matches the same `...`, automatically modify the todo list of rebase -i so that the commit marked for squashing comes right after the commit to be modified, and change the action of the moved commit - from `pick` to `squash` (or `fixup`). A commit matches the `...` if - the commit subject matches, or if the `...` refers to the commit's - hash. As a fall-back, partial matches of the commit subject work, - too. The recommended way to create fixup/squash commits is by using - the `--fixup`/`--squash` options of linkgit:git-commit[1]. + from `pick` to `squash` (or `fixup`). ++ +A commit matches the `...` if +the commit subject matches, or if the `...` refers to the commit's +hash. As a fall-back, partial matches of the commit subject work, +too. The recommended way to create fixup/squash commits is by using +the `--fixup`/`--squash` options of linkgit:git-commit[1]. + If the `--autosquash` option is enabled by default using the configuration variable `rebase.autoSquash`, this option can be -- 2.26.2.windows.1.13.g9dddff6983 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 1/2] doc: fixup/squash: clarify use of <oid-hash> in subject line 2020-05-25 21:36 ` [PATCH 1/2] doc: fixup/squash: clarify use of <oid-hash> in subject line Philip Oakley @ 2020-05-27 17:35 ` Junio C Hamano 2020-05-29 11:41 ` Philip Oakley 0 siblings, 1 reply; 23+ messages in thread From: Junio C Hamano @ 2020-05-27 17:35 UTC (permalink / raw) To: Philip Oakley; +Cc: git Philip Oakley <philipoakley@iee.email> writes: > The use of ellision `...` isn't great, as it gives no hint or clue, > leaving the subsequent test with a difficult explanation. True. If you are planning to correct it in 2/2, then I think it makes more sense to squash that in to have a single patch. > Clarify if a full oid has is required, or a unique abbreviation within > the respository, or just uniques within the rebase instruction? Puzzled. You must know the answer to "do we need a full object name, or is it sufficient to have anything that gives us a unique commit object name?" so why not write it in the patch instead of asking the question here? Or do you not know the answer and this is a RFC/WIP patch???? > This is a minimal change that sidesteps the chance to rewrite/clarify > the potential wider confusions over specifying the <commit> being > referred to in the fixup/squash process. Hmph. So this step cannot be reviewed to judge if it is a good change by itself? Let me locally recreate a squashed single patch and review _that_ instead. > Documentation/git-rebase.txt | 18 ++++++++++-------- > 1 file changed, 10 insertions(+), 8 deletions(-) > > diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt > index 4624cfd288..462cb4c52c 100644 > --- a/Documentation/git-rebase.txt > +++ b/Documentation/git-rebase.txt > @@ -571,16 +571,18 @@ See also INCOMPATIBLE OPTIONS below. > > --autosquash:: > --no-autosquash:: > - When the commit log message begins with "squash! ..." (or > - "fixup! ..."), and there is already a commit in the todo list that > - matches the same `...`, automatically modify the todo list of rebase > + When the commit log message begins with "squash! <line>" (or > + "fixup! <line>"), and there is already a commit in the todo list that > + matches the same `<line>`, automatically modify the todo list of rebase > -i so that the commit marked for squashing comes right after the > commit to be modified, and change the action of the moved commit > + from `pick` to `squash` (or `fixup`). > ++ > +A commit matches the `<line>` if > +the commit subject matches, or if the `<line>` refers to the commit's > +hash. As a fall-back, partial matches of the commit subject work, > +too. The recommended way to create fixup/squash commits is by using > +the `--fixup`/`--squash` options of linkgit:git-commit[1]. > + Overall it looks much better than the original. The original did not even attempt to define what is a "match" for the purpose of this option, so the ellipses may have been OK, but once we need to refer to what is there, we need a name to refer to it and ellipses no longer are sufficient, and using the step 1/2 alone would not make any sense. We definitely should take the step 2/2 together with it. "A commit matches the <line> if the commit subject matches" is not a great definition of what a "match" is, though. The readers are left in the same darkness about what constitutes a "match" of <line> against "the commit subject". If you define this "subject matches" as a substring match, for example, you do not even have to say "as a fall-back"---it is by (the updated version of your) definition that how the commit subject and <line> matches so there is no need to allow any fall-back involved. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/2] doc: fixup/squash: clarify use of <oid-hash> in subject line 2020-05-27 17:35 ` Junio C Hamano @ 2020-05-29 11:41 ` Philip Oakley 0 siblings, 0 replies; 23+ messages in thread From: Philip Oakley @ 2020-05-29 11:41 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On 27/05/2020 18:35, Junio C Hamano wrote: > Philip Oakley <philipoakley@iee.email> writes: > >> The use of ellision `...` isn't great, as it gives no hint or clue, >> leaving the subsequent test with a difficult explanation. > True. If you are planning to correct it in 2/2, then I think it > makes more sense to squash that in to have a single patch. OK >> Clarify if a full oid has is required, or a unique abbreviation within >> the respository, or just uniques within the rebase instruction? > Puzzled. You must know the answer to "do we need a full object > name, or is it sufficient to have anything that gives us a unique > commit object name?" so why not write it in the patch instead of > asking the question here? Or do you not know the answer and this is > a RFC/WIP patch???? This was a left over note about deeper questions outside of this patch series. > >> This is a minimal change that sidesteps the chance to rewrite/clarify >> the potential wider confusions over specifying the <commit> being >> referred to in the fixup/squash process. > Hmph. So this step cannot be reviewed to judge if it is a good > change by itself? I was working on 'small incremental steps' here. > > Let me locally recreate a squashed single patch and review _that_ > instead. > >> Documentation/git-rebase.txt | 18 ++++++++++-------- >> 1 file changed, 10 insertions(+), 8 deletions(-) >> >> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt >> index 4624cfd288..462cb4c52c 100644 >> --- a/Documentation/git-rebase.txt >> +++ b/Documentation/git-rebase.txt >> @@ -571,16 +571,18 @@ See also INCOMPATIBLE OPTIONS below. >> >> --autosquash:: >> --no-autosquash:: >> - When the commit log message begins with "squash! ..." (or >> - "fixup! ..."), and there is already a commit in the todo list that >> - matches the same `...`, automatically modify the todo list of rebase >> + When the commit log message begins with "squash! <line>" (or >> + "fixup! <line>"), and there is already a commit in the todo list that >> + matches the same `<line>`, automatically modify the todo list of rebase >> -i so that the commit marked for squashing comes right after the >> commit to be modified, and change the action of the moved commit >> + from `pick` to `squash` (or `fixup`). >> ++ >> +A commit matches the `<line>` if >> +the commit subject matches, or if the `<line>` refers to the commit's >> +hash. As a fall-back, partial matches of the commit subject work, >> +too. The recommended way to create fixup/squash commits is by using >> +the `--fixup`/`--squash` options of linkgit:git-commit[1]. >> + > Overall it looks much better than the original. > > The original did not even attempt to define what is a "match" for > the purpose of this option, so the ellipses may have been OK, but > once we need to refer to what is there, we need a name to refer to > it and ellipses no longer are sufficient, and using the step 1/2 > alone would not make any sense. We definitely should take the step > 2/2 together with it. I'd taken the idea of being able to name the thing as step 1, to get past the Newspeak problem. > > "A commit matches the <line> if the commit subject matches" is not a > great definition of what a "match" is, though. The readers are left > in the same darkness about what constitutes a "match" of <line> > against "the commit subject". If you define this "subject matches" > as a substring match, for example, you do not even have to say "as a > fall-back"---it is by (the updated version of your) definition that > how the commit subject and <line> matches so there is no need to > allow any fall-back involved. The fall back does include the commit hash, and (not yet in this series) is the extra information that Dscho provided at [1], so it's not a simple substring match, nor partial string match. Part of this reader confusion comes out of the `commit --fixup` option that effectively directs the reader away from using a hash, to using the target's full commit message for the fixup! line. At this stage, the aim is to make the option for the use of the commit hash a bit more visible within the text. Even after many years of reading, it still didn't stand out in the old 'wall of text', hence the all important paragraph break I'll combine the two patches at this stage. Philip [1] https://public-inbox.org/git/nycvar.QRO.7.76.6.2005180522230.55@tvgsbejvaqbjf.bet/ "It's even worse" ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 2/2] doc: fixup/squash: remove ellipsis marks, use <line> for clarify 2020-05-25 21:36 ` [PATCH 0/2] Clarify some of the fixup! documenation Philip Oakley 2020-05-25 21:36 ` [PATCH 1/2] doc: fixup/squash: clarify use of <oid-hash> in subject line Philip Oakley @ 2020-05-25 21:36 ` Philip Oakley 1 sibling, 0 replies; 23+ messages in thread From: Philip Oakley @ 2020-05-25 21:36 UTC (permalink / raw) To: git Ellipsis marks fail to hint at the typoe or style of the missing content. Tell the reader what is missing, for easier comprehension. Signed-off-by: Philip Oakley <philipoakley@iee.email> --- The fixup/squash process could probably benefit from its own section as there are many places for user interaction with the process. This is a minimal shift toward such an improvement. --- Documentation/git-rebase.txt | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt index dfd3d6d0ef..1d8237bfc6 100644 --- a/Documentation/git-rebase.txt +++ b/Documentation/git-rebase.txt @@ -534,15 +534,15 @@ See also INCOMPATIBLE OPTIONS below. --autosquash:: --no-autosquash:: - When the commit log message begins with "squash! ..." (or - "fixup! ..."), and there is already a commit in the todo list that - matches the same `...`, automatically modify the todo list of rebase + When the commit log message begins with "squash! <line>" (or + "fixup! <line>"), and there is already a commit in the todo list that + matches the same `<line>`, automatically modify the todo list of rebase -i so that the commit marked for squashing comes right after the commit to be modified, and change the action of the moved commit from `pick` to `squash` (or `fixup`). + -A commit matches the `...` if -the commit subject matches, or if the `...` refers to the commit's +A commit matches the `<line>` if +the commit subject matches, or if the `<line>` refers to the commit's hash. As a fall-back, partial matches of the commit subject work, too. The recommended way to create fixup/squash commits is by using the `--fixup`/`--squash` options of linkgit:git-commit[1]. -- 2.26.2.windows.1.13.g9dddff6983 ^ permalink raw reply related [flat|nested] 23+ messages in thread
end of thread, other threads:[~2020-05-29 11:41 UTC | newest] Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-04 20:40 [PATCH] rebase --autosquash: fix a potential segfault Johannes Schindelin via GitGitGadget 2020-05-04 21:19 ` Junio C Hamano 2020-05-04 21:33 ` Jeff King 2020-05-04 22:09 ` Taylor Blau 2020-05-05 20:30 ` Junio C Hamano 2020-05-06 21:35 ` Johannes Schindelin 2020-05-07 19:17 ` Jeff King 2020-05-08 23:45 ` Johannes Schindelin 2020-05-05 22:33 ` [PATCH v2] " Johannes Schindelin via GitGitGadget 2020-05-09 19:23 ` [PATCH v3] " Johannes Schindelin via GitGitGadget 2020-05-06 15:12 ` [PATCH] " Andrei Rybak 2020-05-07 14:27 ` Johannes Schindelin 2020-05-08 16:43 ` Philip Oakley 2020-05-08 16:57 ` Andrei Rybak 2020-05-08 17:21 ` Philip Oakley 2020-05-18 16:47 ` Philip Oakley 2020-05-18 3:27 ` Johannes Schindelin 2020-05-25 17:29 ` Philip Oakley 2020-05-25 21:36 ` [PATCH 0/2] Clarify some of the fixup! documenation Philip Oakley 2020-05-25 21:36 ` [PATCH 1/2] doc: fixup/squash: clarify use of <oid-hash> in subject line Philip Oakley 2020-05-27 17:35 ` Junio C Hamano 2020-05-29 11:41 ` Philip Oakley 2020-05-25 21:36 ` [PATCH 2/2] doc: fixup/squash: remove ellipsis marks, use <line> for clarify Philip Oakley
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).