> On Jan 13, 2022, at 10:14 PM, Philippe Blain <levraiphilippeblain@xxxxxxxxx> wrote: > > Hi Junio, > >> Le 2022-01-13 à 19:00, Junio C Hamano a écrit : >> Junio C Hamano <gitster@xxxxxxxxx> 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@xxxxxxxxx/ > > ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----- > From 28edde4e302e14c900e314268e4eeaeadc240bcb Mon Sep 17 00:00:00 2001 > From: Philippe Blain <levraiphilippeblain@xxxxxxxxx> > 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_-_@xxxxxxxxxxxxxxxxxxxxxxxxxxx/ > > Reported-by: Tilman Vogel <tilman.vogel@xxxxxx> > Co-authored-by: Junio C Hamano <gitster@xxxxxxxxx> > Signed-off-by: Philippe Blain <levraiphilippeblain@xxxxxxxxx> > --- > 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
