`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:

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

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

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