Skip to contentSkip to conversation

rebase --autosquash: fix a potential segfault

PATCH Superseded Johannes Schindelin via GitGitGadget 7 participants 23 messages May 4, 2020 – May 29, 2020 Documentation

lore
Share & tools
lore

Johannes Schindelin via GitGitGadget

author
#0 lore raw

PATCH 1/1 rebase --autosquash: fix a potential segfault

From: Johannes Schindelin <[email protected]>

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 ith 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 <[email protected]>
Helped-by Jeff King <[email protected]>
Signed-off-by Johannes Schindelin <[email protected]>

---
rebase --autosquash: fix a potential segfault

This patch is in response to
https://lore.kernel.org/git/[email protected]/
.

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(-)
sequencer.c +10 -1
index e528225e787..0d4d53d2a49 100644
@@ -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)) {
t/t3415-rebase-autosquash.sh +14 -0
index 093de9005b7..ca135349346 100755
@@ -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

Signature · 1 line
gitgitgadget
#1 lore raw

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 9 lines · writes:
sequencer.c +0 -1
index e528225e787..0d4d53d2a49 100644
@@ -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

sequencer.c int todo_list_rearrange_squash(struct todo_list *todo_list) · line 5278

Quoted 10 lines
sequencer.c +9 -0
+			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.

Quoted 29 lines
sequencer.c +1 -0
+			}
 			tail[i2] = i;
 		} else if (!hashmap_get_from_hash(&subject2item,
 						strhash(subject), subject)) {
t/t3415-rebase-autosquash.sh +14 -0
index 093de9005b7..ca135349346 100755
@@ -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

#2 lore raw
In reply to Johannes Schindelin via GitGitGadget
Quoted 9 lines · Johannes Schindelin via GitGitGadget

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 ith 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)

Quoted 6 lines

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:

Quoted 4 lines
sequencer.c +1 -1
 			if (next[i2] < 0)
 				next[i2] = i;
-			else
+			else if (tail[i2] >= 0)

sequencer.c int todo_list_rearrange_squash(struct todo_list *todo_list) · line 5279

Quoted 10 lines
sequencer.c +9 -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

#3 lore raw
Quoted 35 lines · Jeff King
Quoted 9 lines · Johannes Schindelin via GitGitGadget

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 ith 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)

Quoted 6 lines

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:

Quoted 9 lines
sequencer.c +5 -1
 			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
Quoted 8 lines

sequencer.c int todo_list_rearrange_squash(struct todo_list *todo_list) · line 5279

Quoted 5 lines
sequencer.c +5 -0
+				 */
+				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.

Quoted 21 lines

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

Quoted 2 lines

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.

Quoted 2 lines

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 ;).

Quoted 1 line

-Peff

Thanks,
Taylor

#4 lore raw
Quoted 12 lines · Taylor Blau
Quoted 11 lines

sequencer.c int todo_list_rearrange_squash(struct todo_list *todo_list) · line 5279

Quoted 8 lines
sequencer.c +8 -0
+				/*
+				 * 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?

Quoted 1 line

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 ;-)

#5 lore raw

Hi Junio,

Quoted 14 lines · Junio C Hamano
Quoted 12 lines · Taylor Blau
Quoted 11 lines

sequencer.c int todo_list_rearrange_squash(struct todo_list *todo_list) · line 5279

Quoted 8 lines
sequencer.c +8 -0
+				/*
+				 * 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?

Quoted 4 lines
Quoted 1 line

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

#6 lore raw
Quoted 10 lines · Johannes Schindelin
Quoted 10 lines
Quoted 9 lines
Quoted 7 lines

sequencer.c int todo_list_rearrange_squash(struct todo_list *todo_list) · line 5279

Quoted 4 lines
sequencer.c +4 -0
+				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.

Quoted 7 lines
Quoted 2 lines

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

#7 lore raw

Hi Peff,

Quoted 15 lines · Jeff King
Quoted 13 lines · Johannes Schindelin
Quoted 12 lines
Quoted 9 lines
Quoted 7 lines

sequencer.c int todo_list_rearrange_squash(struct todo_list *todo_list) · line 5279

Quoted 4 lines
sequencer.c +4 -0
+				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 ;-)

Quoted 6 lines
Quoted 4 lines

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.

Quoted 1 line

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.

Quoted 3 lines

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

author
#8 lore raw

PATCH v2 1/1 rebase --autosquash: fix a potential segfault

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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 ith 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 <[email protected]>
Helped-by Jeff King <[email protected]>
Signed-off-by Johannes Schindelin <[email protected]>

---
rebase --autosquash: fix a potential segfault

This patch is in response to
https://lore.kernel.org/git/[email protected]/
.

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 <[email protected]>
          Helped-by: Jeff King <[email protected]>
     @@ 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(-)
sequencer.c +5 -2
index e528225e787..d579f6d6c06 100644
@@ -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)) {
t/t3415-rebase-autosquash.sh +16 -0
index 093de9005b7..7bab6000dc7 100755
@@ -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

Signature · 1 line
gitgitgadget
author
#9 lore raw

PATCH v3 1/1 rebase --autosquash: fix a potential segfault

From: Johannes Schindelin <[email protected]>

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 ith 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 <[email protected]>
Helped-by Jeff King <[email protected]>
Signed-off-by Johannes Schindelin <[email protected]>

---
rebase --autosquash: fix a potential segfault

This patch is in response to
https://lore.kernel.org/git/[email protected]/
.

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 <[email protected]>
Helped-by Jeff King <[email protected]>
 sequencer.c                  |  7 +++++--
 t/t3415-rebase-autosquash.sh | 16 ++++++++++++++++
 2 files changed, 21 insertions(+), 2 deletions(-)
sequencer.c +5 -2
index e528225e787..d579f6d6c06 100644
@@ -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)) {
t/t3415-rebase-autosquash.sh +16 -0
index 093de9005b7..7bab6000dc7 100755
@@ -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

Signature · 1 line
gitgitgadget
#10 lore raw
In reply to Johannes Schindelin via GitGitGadget
Quoted 10 lines · Johannes Schindelin via GitGitGadget

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.

#11 lore raw

Hi Andrei,

Quoted 17 lines · Andrei Rybak
Quoted 10 lines · Johannes Schindelin via GitGitGadget

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>

Quoted 8 lines

? 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

#12 lore raw
Quoted 27 lines · Johannes Schindelin

Hi Andrei,

Quoted 23 lines · Andrei Rybak
Quoted 10 lines · Johannes Schindelin via GitGitGadget

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

Quoted 6 lines
Quoted 1 line

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

#13 lore raw
Quoted 6 lines · Philip Oakley

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! ".

Quoted 6 lines

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."

#14 lore raw

Hi Andrei,

Quoted 7 lines · Andrei Rybak
Quoted 6 lines · Philip Oakley

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! ".

Quoted 8 lines
Quoted 6 lines

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

#15 lore raw
In reply to Andrei Rybak

Hi all,

Quoted 7 lines · Andrei Rybak
Quoted 6 lines · Philip Oakley

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! ".

Quoted 8 lines
Quoted 6 lines

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

#16 lore raw

Hi Philip,

Quoted 13 lines · Philip Oakley
Quoted 12 lines · Andrei Rybak
Quoted 11 lines · Philip Oakley

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?

Quoted 7 lines
Quoted 3 lines
Quoted 1 line

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

#17 lore raw

Hi Dscho,

Quoted 36 lines · Johannes Schindelin

Hi Philip,

Quoted 11 lines · Philip Oakley
Quoted 10 lines · Andrei Rybak
Quoted 9 lines · Philip Oakley

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.

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines

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.

Quoted 8 lines

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

#18 lore raw

cover Clarify some of the fixup! documenation

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: [email protected]
To: [email protected]
cc: Johannes Schindelin via GitGitGadget <[email protected]>
cc: Andrei Rybak <[email protected]>
cc: Paul Ganssle <[email protected]>
cc: Jeff King <[email protected]>

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(-)
Signature · 1 line
2.26.2.windows.1.13.g9dddff6983
#19 lore raw

PATCH 1/2 doc: fixup/squash: clarify use of <oid-hash> in subject line

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 <[email protected]>

---
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(-)
Documentation/git-rebase.txt +7 -5
index f7a6033607..dfd3d6d0ef 100644
@@ -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
Signature · 1 line
2.26.2.windows.1.13.g9dddff6983
#20 lore raw

Re: [PATCH 1/2] doc: fixup/squash: clarify use of <oid-hash> in subject line

Quoted 2 lines · Philip Oakley

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.

Quoted 2 lines

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????

Quoted 3 lines

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.

Quoted 17 lines
 Documentation/git-rebase.txt | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)
Documentation/git-rebase.txt +3 -3
index 4624cfd288..462cb4c52c 100644
@@ -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

Documentation/git-rebase.txt See also INCOMPATIBLE OPTIONS below. · line 586

Quoted 10 lines
Documentation/git-rebase.txt +7 -0
 	-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.

#21 lore raw

Re: [PATCH 1/2] doc: fixup/squash: clarify use of <oid-hash> in subject line

Quoted 6 lines · Junio C Hamano
Quoted 2 lines · Philip Oakley

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

Quoted 7 lines
Quoted 2 lines

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.

Quoted 6 lines
Quoted 4 lines

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.

Quoted 33 lines

Let me locally recreate a squashed single patch and review _that_ instead.

Quoted 17 lines
 Documentation/git-rebase.txt | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)
Documentation/git-rebase.txt +3 -3
index 4624cfd288..462cb4c52c 100644
@@ -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

Documentation/git-rebase.txt See also INCOMPATIBLE OPTIONS below. · line 586

Quoted 10 lines
Documentation/git-rebase.txt +7 -0
 	-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.

Quoted 6 lines

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.

Quoted 9 lines

"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/[email protected]/
"It's even worse"

#22 lore raw

PATCH 2/2 doc: fixup/squash: remove ellipsis marks, use <line> for clarify

In reply to Philip Oakley

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 <[email protected]>

---

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(-)
Documentation/git-rebase.txt +5 -5
index dfd3d6d0ef..1d8237bfc6 100644
@@ -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].
Signature · 1 line
2.26.2.windows.1.13.g9dddff6983