Re: `pull --rebase --autostash` fails when fast forward in dirty repo

From: Jeff King <peff@peff.net>
Date: May 23, 2017, 13:12 UTC
Message-ID: <20170523131231.zqjkymypbilv6tyf@sigill.intra.peff.net>
In-Reply-To: <CAAZatrCaoB7EXVrCvC9RKmO02G5xcp8GPBaJefHfv7zAXVpL3Q@mail.gmail.com>

[+cc Junio, whose code this is touching]

On Sun, May 21, 2017 at 12:17:06AM -0500, Tyler Brazier wrote:

Show 6 quoted lines

> This script explains and tests what's going on:
> https://gist.github.com/tylerbrazier/4478e76fe44bf6657d4d3da6c789531d
> 
> pull is failing because it shortcuts to --ff-only then calls
> run_merge(), which does not know how to autostash. Removing the
> shortcut fixes the problem:

So I guess the ideal solution would be for us to do the autostash ourselves, run the fast-forward merge, and then pop the stash. In theory that's just "git stash push" followed by "git stash pop", but looking at the implementation in git-rebase.sh, it looks like it's a little more complicated than that.

Disabling the optimization sounds like a reasonable interim workaround, but...

Show 14 quoted lines

> diff --git a/builtin/pull.c b/builtin/pull.c
> index dd1a4a94e..225a59f5f 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -868,11 +868,6 @@ int cmd_pull(int argc, const char **argv, const
> char *prefix)
>       head = lookup_commit_reference(orig_head.hash);
>       commit_list_insert(head, &list);
>       merge_head = lookup_commit_reference(merge_heads.oid[0].hash);
> -     if (is_descendant_of(merge_head, list)) {
> -         /* we can fast-forward this without invoking rebase */
> -         opt_ff = "--ff-only";
> -         return run_merge();
> -     }

...we can probably restrict it to when autostash is in use, like:

  /*
   * If this is a fast-forward, we can skip calling rebase and
   * just do the merge ourselves. But we don't know about
   * autostash, so use the real rebase command when it's in effect.
   */
  if (!autostash && is_descendant_of(merge_head, list)) {
	opt_ff = "--ff-only";
	return run_merge();
  }

AFAICT from the commit introducing this code (33b842a1e9), and the surrounding discussion:

  http://public-inbox.org/git/OF95D98CB6.47969C1C-ONC1257FE1.0058D980-C1257FE1.0059986D@dakosy.de/T/#u

this is purely an optimization to avoid invoking rebase, so it's OK to skip (and interestingly, the autostash question was raised in that thread but not resolved).

But I notice on the run_merge() code path that we do still invoke git-merge. And rebase has been getting faster as it is moved to C code itself. So is this optimization even worth doing anymore?

-Peff