Re: [PATCH v2 04/14] pull: cleanup autostash check

[Elijah Newren <newren@gmail.com>]

On Thu, Dec 3, 2020 at 10:16 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> This essentially reverts commit f15e7cf5cc.
>
> Once commit d9f15d37f1 introduced the autostash option for the merge
> mode, it's not necessary to skip the fast-forward run_merge() when
> autostash is set.

It helps reviewers and future code readers if you provide a little
context when referring to commits, making use of git log's
--pretty=reference option to get the output.  So, for example, here
your commit would read:

"""
This essentially reverts commit f15e7cf5cc (pull: ff --rebase
--autostash works in dirty repo, 2017-06-01).

Once commit d9f15d37f1 (pull: pass --autostash to merge, 2020-04-07)
introduced the autostash option for the merge
mode, it's not necessary to skip the fast-forward run_merge() when
autostash is set.
"""

I still found it slightly hard to follow the explanation even with the
added summaries, though.  An extra sentence at the end of the second
paragraph to make it clear what is being changed ("So, change the code
to fast-forward even when autostash is set.") seems to help.



> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  builtin/pull.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 6279e9ee37..c38548dab8 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -927,7 +927,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;
>
>         if (!getenv("GIT_REFLOG_ACTION"))
>                 set_reflog_message(argc, argv);
> @@ -960,8 +959,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;
>
> @@ -1030,13 +1029,11 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>                      recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) &&
>                     submodule_touches_in_range(the_repository, &rebase_fork_point, &curr_head))
>                         die(_("cannot rebase with locally recorded submodule modifications"));
> -               if (!autostash) {
> -                       if (get_can_ff(&orig_head, &merge_heads.oid[0])) {
> -                               /* we can fast-forward this without invoking rebase */
> -                               opt_ff = "--ff-only";
> -                               ran_ff = 1;
> -                               ret = run_merge();
> -                       }
> +               if (get_can_ff(&orig_head, &merge_heads.oid[0])) {
> +                       /* we can fast-forward this without invoking rebase */
> +                       opt_ff = "--ff-only";
> +                       ran_ff = 1;
> +                       ret = run_merge();
>                 }
>                 if (!ran_ff)
>                         ret = run_rebase(&curr_head, merge_heads.oid, &rebase_fork_point);
> --
> 2.29.2
>