Re: [PATCH 1/1] builtin/pull.c: use config value of autostash

[John Cai <johncai86@gmail.com>]

> On Jan 13, 2022, at 10:14 PM, Philippe Blain <levraiphilippeblain@gmail.com> wrote:
> 
> Hi Junio,
> 
>> Le 2022-01-13 à 19:00, Junio C Hamano a écrit :
>> Junio C Hamano <gitster@pobox.com> writes:
>>> Here is how I would explain your change.
>> This topic is in "Expecting an ack or two" state for some time.
>>  - Philippe, are you OK with the attached patch?  If so throw your
>>    Signed-off-by to this thread.
> 
> I'm not 100% OK since as I remarked to John in [1], I don't think all 4
> tests are necessary, 3 out of 4 are duplicates of existing tests, and I
> would have put the new test in t5520 with 'git pull's other "autostash"
> tests. I hoped that John would incorporate my suggestions in a v2, but he
> seems to be busy, so I'm including an updated patch at the end of this email.
> I only slightly edited the commit message you wrote, so thanks for that.
> 'pb/pull-rebase-autostash-fix' could be replaced by the patch below,
> I would think.
> 
>>  - John, if you find Philippe's implementation a good idea (I think
>>    it is, as it is simpler and cleaner) after reading and
>>    understanding it, please throw an Acked-by or Reviewed-by to this
>>    thread.

I agree-think Philippe’s implementation is a better solution.

I don’t have the bandwidth these couple of days to reroll the patch so if someone else can take over that would be great!

> 
> 
> Thanks,
> 
> Philippe.
> 
> [1] https://lore.kernel.org/git/fe0b7337-3005-d09c-a3b6-65a100799676@gmail.com/
> 
> ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----
> From 28edde4e302e14c900e314268e4eeaeadc240bcb Mon Sep 17 00:00:00 2001
> From: Philippe Blain <levraiphilippeblain@gmail.com>
> Date: Thu, 13 Jan 2022 21:58:05 -0500
> Subject: [PATCH] pull --rebase: honor rebase.autostash when fast-forwarding
> 
> "pull --rebase" internally uses the merge machinery when the other
> history is a descendant of ours (i.e. perform fast-forward).  This
> came from [1], where the discussion was started from a feature
> request to do so.  It is a bit hard to read the rationale behind it
> in the discussion, but it seems that it was an established fact for
> everybody involved that does not even need to be mentioned that
> fast-forwarding done with "rebase" was much undesirable than done
> with "merge", and more importantly, the result left by "merge" is as
> good as (or better than) that by "rebase".
> 
> Except for one thing.  Because "git merge" does not (and should not)
> honor rebase.autostash, "git pull" needs to read it and forward it
> when we use "git merge" as a (hopefully better) substitute for "git
> rebase" during the fast-forwarding.  But we forgot to do so (we only
> add "--[no-]autostash" to the "git merge" command when "git pull" itself
> was invoked with "--[no-]autostash" command line option.
> 
> Make sure "git merge" is run with "--autostash" when
> rebase.autostash is set and used to fast-forward the history on
> behalf of "git rebase".  Incidentally this change also takes care of
> the case where
> 
> - "git pull --rebase" (without other command line options) is run
> - "rebase.autostash" is not set
> - The history fast-forwards
> 
> In such a case, "git merge" is run with an explicit "--no-autostash"
> to prevent it from honoring merge.autostash configuration, which is
> what we want.  After all, we want the "git merge" to pretend as if
> it is "git rebase" while being used for this purpose.
> 
> [1] https://lore.kernel.org/git/xmqqa8cfbkeq.fsf_-_@gitster.mtv.corp.google.com/
> 
> Reported-by: Tilman Vogel <tilman.vogel@web.de>
> Co-authored-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
> ---
> builtin/pull.c  |  7 +++----
> t/t5520-pull.sh | 13 +++++++++++++
> 2 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 1cfaf9f343..9f8a8dd716 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -1036,14 +1036,13 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>        oidclr(&orig_head);
>      if (opt_rebase) {
> -        int autostash = config_autostash;
> -        if (opt_autostash != -1)
> -            autostash = opt_autostash;
> +        if (opt_autostash == -1)
> +            opt_autostash = config_autostash;
>          if (is_null_oid(&orig_head) && !is_cache_unborn())
>            die(_("Updating an unborn branch with changes added to the index."));
> -        if (!autostash)
> +        if (!opt_autostash)
>            require_clean_work_tree(the_repository,
>                N_("pull with rebase"),
>                _("please commit or stash them."), 1, 0);
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index 93ecfcdd24..3e784f18a6 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -330,6 +330,19 @@ test_expect_success '--rebase --autostash fast forward' '
>    test_cmp_rev HEAD to-rebase-ff
> '
> +test_expect_success '--rebase with rebase.autostash succeeds on ff' '
> +    test_when_finished "rm -fr src dst actual" &&
> +    git init src &&
> +    test_commit -C src "initial" file "content" &&
> +    git clone src dst &&
> +    test_commit -C src --printf "more_content" file "more content\ncontent\n" &&
> +    echo "dirty" >>dst/file &&
> +    test_config -C dst rebase.autostash true &&
> +    git -C dst pull --rebase  >actual 2>&1 &&
> +    grep -q "Fast-forward" actual &&
> +    grep -q "Applied autostash." actual
> +'
> +
> test_expect_success '--rebase with conflicts shows advice' '
>    test_when_finished "git rebase --abort; git checkout -f to-rebase" &&
>    git checkout -b seq &&
> 
> base-commit: e9d7761bb94f20acc98824275e317fa82436c25d
> prerequisite-patch-id: 3c6b4be75d7a634bf45f0264b3f04216818a0816
> -- 
> 2.29.2