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

From: Jeff King <peff@peff.net>
Date: May 4, 2020, 21:33 UTC
Message-ID: <20200504213326.GA31037@coredump.intra.peff.net>
In-Reply-To: <pull.625.git.1588624804554.gitgitgadget@gmail.com>

On Mon, May 04, 2020 at 08:40:04PM +0000, Johannes Schindelin via GitGitGadget wrote:

Show 9 quoted lines

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

Show 6 quoted 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:

Show 14 quoted lines

>  			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