[PATCH] rebase --autosquash: fix a potential segfault

[PATCH] rebase --autosquash: fix a potential segfault

[Johannes Schindelin via GitGitGadget]

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

Re: [PATCH] rebase --autosquash: fix a potential segfault

[Junio C Hamano]

"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

Re: [PATCH] rebase --autosquash: fix a potential segfault

[Jeff King]

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

Re: [PATCH] rebase --autosquash: fix a potential segfault

[Taylor Blau]

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

Re: [PATCH] rebase --autosquash: fix a potential segfault

[Junio C Hamano]

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

Re: [PATCH] rebase --autosquash: fix a potential segfault

[Johannes Schindelin]

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

Re: [PATCH] rebase --autosquash: fix a potential segfault

[Jeff King]

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

Re: [PATCH] rebase --autosquash: fix a potential segfault

[Johannes Schindelin]

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

[PATCH v2] rebase --autosquash: fix a potential segfault

[Johannes Schindelin via GitGitGadget]

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

[PATCH v3] rebase --autosquash: fix a potential segfault

[Johannes Schindelin via GitGitGadget]

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

Re: [PATCH] rebase --autosquash: fix a potential segfault

[Andrei Rybak]

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.

Re: [PATCH] rebase --autosquash: fix a potential segfault

[Johannes Schindelin]

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

Re: [PATCH] rebase --autosquash: fix a potential segfault

[Philip Oakley]

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

Re: [PATCH] rebase --autosquash: fix a potential segfault

[Andrei Rybak]

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

Re: [PATCH] rebase --autosquash: fix a potential segfault

[Philip Oakley]

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

Re: [PATCH] rebase --autosquash: fix a potential segfault

[Philip Oakley]

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

Re: [PATCH] rebase --autosquash: fix a potential segfault

[Johannes Schindelin]

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

Re: [PATCH] rebase --autosquash: fix a potential segfault

[Philip Oakley]

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 

[PATCH 0/2] Clarify some of the fixup! documenation

[Philip Oakley]

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

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

[Philip Oakley]

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

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

[Junio C Hamano]

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.

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

[Philip Oakley]

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"

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

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