Re: [PATCH 1/3] pull: cleanup autostash check

[Elijah Newren <newren@gmail.com>]

On Sat, Jun 12, 2021 at 9:59 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> Currently "git pull --rebase" takes a shortcut in the case a
> fast-forward merge is possible; run_merge() is called with --ff-only.
>
> However, "git merge" didn't have an --autostash option, so, when "git
> pull --rebase --autostash" was called *and* the fast-forward merge
> shortcut was taken, then the pull failed.
>
> This was fixed in commit f15e7cf5cc (pull: ff --rebase --autostash
> works in dirty repo, 2017-06-01) by simply skipping the fast-forward
> merge shortcut.
>
> Later on "git merge" learned the --autostash option [a03b55530a
> (merge: teach --autostash option, 2020-04-07)], and so did "git pull"
> [d9f15d37f1 (pull: pass --autostash to merge, 2020-04-07)].
>
> Therefore it's not necessary to skip the fast-forward merge shortcut
> anymore when called with --rebase --autostash.
>
> Let's always take the fast-forward merge shortcut by essentially
> reverting f15e7cf5cc.
>
> Reviewed-by: Elijah Newren <newren@gmail.com>

I think you are basing the Reviewed-by on
https://lore.kernel.org/git/CABPp-BEsQWsHMAmwc3gmJnXcS+aR-FtoMJxBRQ=BpARP49-L-Q@mail.gmail.com/;
is that correct?  Messages from folks that they seem to like the patch
or believe it looks good should be translated into an Acked-by rather
than a Reviewed-by; from Documentation/SubmittingPatches:

* `Reviewed-by:`, unlike the other tags, can only be offered by the
  reviewer and means that she is completely satisfied that the patch
  is ready for application.  It is usually offered only after a
  detailed review.

Sorry for not catching this when you posted v3 & v4 of your earlier
series.  When your series exploded in size and seemed to just be
accumulating additional changes you wanted to make in the area that
weren't in response to reviewer feedback (I wasn't sure why the new
patches in subsequent rerolls weren't just separate series), I didn't
have the bandwidth to keep up and review them, so I just missed it.

> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  builtin/pull.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index e8927fc2ff..a22293b7db 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -947,7 +947,6 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>         struct oid_array merge_heads = OID_ARRAY_INIT;
>         struct object_id orig_head, curr_head;
>         struct object_id rebase_fork_point;
> -       int autostash;
>         int rebase_unspecified = 0;
>         int can_ff;
>
> @@ -982,8 +981,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>         if (get_oid("HEAD", &orig_head))
>                 oidclr(&orig_head);
>
> -       autostash = config_autostash;
>         if (opt_rebase) {
> +               int autostash = config_autostash;
>                 if (opt_autostash != -1)
>                         autostash = opt_autostash;
>
> @@ -1065,13 +1064,12 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>                      recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) &&
>                     submodule_touches_in_range(the_repository, &upstream, &curr_head))
>                         die(_("cannot rebase with locally recorded submodule modifications"));
> -               if (!autostash) {
> -                       if (can_ff) {
> -                               /* we can fast-forward this without invoking rebase */
> -                               opt_ff = "--ff-only";
> -                               ran_ff = 1;
> -                               ret = run_merge();
> -                       }
> +
> +               if (can_ff) {
> +                       /* we can fast-forward this without invoking rebase */
> +                       opt_ff = "--ff-only";
> +                       ran_ff = 1;
> +                       ret = run_merge();
>                 }
>                 if (!ran_ff)
>                         ret = run_rebase(&newbase, &upstream);
> --
> 2.32.0