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

Hi,

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:

[+cc Junio, whose code this is touching]

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

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

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

Jeff King <peff@peff.net> writes:

Sounds like a sensible solution.

... wouldn't that what we want even when the merge happens to be a fast-forward one? I am not sure what you meant by this, but...

...that might be something worth thinking about---my gut feeling tells me something but we should go by a measurement, not by gut feeling of a random somebody.

Thanks.

On Wed, May 24, 2017 at 07:40:08AM +0900, Junio C Hamano wrote:

I just meant that if the point of the optimization was to avoid invoking git-rebase (because it's slow), then we're still not optimizing out a process. It only helps at all because "rebase" (being a shell script) may be slower to start and realize it's a fast-forward than "merge".

But once that is no longer true of git-rebase, then there is no purpose to the optimization.

Yeah, I'd agree. I had the impression the original change was motivated by gut feeling.

-Peff

On Thu, May 25, 2017 at 02:04:07PM -0400, Jeff King wrote:

Hmph. On Linux, at least, I do not see that using "git merge" to fast-forward is appreciably faster:

Here are timings for:

  git reset --hard HEAD~10 && git pull --rebase

in a git.git repo, using builds of git from various commits (all best-of-five; the timings include the reset for simplicity, but presumably it costs the same in each case):

  - 33b842a1e^ (just prior to the switch to git-merge)
    real  0m0.256s
    user  0m0.096s
    sys	  0m0.020s

  - 33b842a1e  (using git-merge)
    real  0m0.227s
    user  0m0.092s
    sys	  0m0.020s

So a little faster, but there seems to be 20-30ms of noise in my timings anyway (the average for the "prior" case did seem to be higher, though). It's possible that the difference would be more stark on Windows, where the cost of the shell script would be higher.

The same test with the current master performs the same as 33b842a1e. But if I then remove the optimization, as Tyler's patch did at the start of this thread, the timings are similar to 33b842a1e^.

So I dunno. It does not seem appreciably faster, but what little speedup it does provide is the same even with a more modern rebase. Which is probably because that rebase isn't actually doing much in the first place, so the optimized bits from Dscho's rebase--helper are not kicking in yet.

Anyway. All this has shown me is that it's probably pointless to do this timing at all on Linux. Somebody on Windows might get better results.

But regardless, we need to do something. Correctness must trump optimizations, and the question is whether we can throw out the whole conditional, or if we should just restrict when it kicks in.

-Peff

Jeff King <peff@peff.net> writes:

Yes. I personally do not mind going with the simplest approach. The optimization thing is relatively new and we were perfectly happy without it before ;-).

Does git accept outside pull requests? I wouldn't mind committing the fix for this once it's been decided what the fix should be. (It might help my resume ;)

On Thu, May 25, 2017 at 6:33 PM, Junio C Hamano <gitster@pobox.com> wrote:

Tyler Brazier <tylerbrazier@gmail.com> writes:

[administrivia: please do not top-post]

Please see Documentation/SubmittingPatches.

Thanks.