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

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

[Tyler Brazier]

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:

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();
-     }
      return run_rebase(&curr_head, merge_heads.oid, &rebase_fork_point);
  } else {
      return run_merge();

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

[Jeff King]

[+cc Junio, whose code this is touching]

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

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

> 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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

Sounds like a sensible solution.

> But I notice on the run_merge() code path that we do still invoke
> git-merge.

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

> And rebase has been getting faster as it is moved to C code
> itself. So is this optimization even worth doing anymore?

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

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

[Jeff King]

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

> > But I notice on the run_merge() code path that we do still invoke
> > git-merge.
> 
> ... 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...

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.

> > And rebase has been getting faster as it is moved to C code
> > itself. So is this optimization even worth doing anymore?
> 
> ...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.

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

-Peff

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

[Jeff King]

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

> > ...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.
> 
> Yeah, I'd agree. I had the impression the original change was motivated
> by gut feeling.

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

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

[Tyler Brazier]

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:
> Jeff King <peff@peff.net> writes:
>
>> 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.
>
> 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 ;-).
>

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

[Junio C Hamano]

Tyler Brazier <tylerbrazier@gmail.com> writes:

> On Thu, May 25, 2017 at 6:33 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Jeff King <peff@peff.net> writes:
>>
>>> 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.
>>
>> 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 ;-).

[administrivia: please do not top-post]

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

Please see Documentation/SubmittingPatches.

Thanks.