Re: [PATCH 1/1] builtin/pull.c: use config value of autostash
Show 24 quoted lines
> 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!
Show 102 quoted lines
>
>
> 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.2John Cai <johncai86@gmail.com>