Re: [PATCH v2 4/8] pull: since --ff-only overrides, handle it first

From: Elijah Newren <newren@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com>,
	"Git Mailing List" <git@vger.kernel.org>,
	"Alex Henrie" <alexhenrie24@gmail.com>,
	"Son Luong Ngoc" <sluongng@gmail.com>,
	"Matthias Baumgarten" <matthias.baumgarten@aixigo.com>,
	"Eric Sunshine" <sunshine@sunshineco.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: Re: [PATCH v2 4/8] pull: since --ff-only overrides, handle it first
Date: Wed, 21 Jul 2021 20:42:09 -0700	[thread overview]
Message-ID: <CABPp-BG--c4Q8bFv20382dyQJgAWqZymT+gGD0eR2v0QB2RQxg@mail.gmail.com> (raw)
In-Reply-To: <xmqqlf5zctey.fsf@gitster.g>

On Wed, Jul 21, 2021 at 1:18 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > -     if (!can_ff) {
> > -             if (opt_ff) {
> > -                     if (!strcmp(opt_ff, "--ff-only"))
> > -                             die_ff_impossible();
> > -             } else {
> > -                     if (rebase_unspecified && opt_verbosity >= 0)
> > -                             show_advice_pull_non_ff();
> > -             }
> > +     /* ff-only takes precedence over rebase */
> > +     if (opt_ff && !strcmp(opt_ff, "--ff-only")) {
> > +             if (!can_ff)
> > +                     die_ff_impossible();
> > +             opt_rebase = REBASE_FALSE;
> >       }
> > +     /* If no action specified and we can't fast forward, then warn. */
> > +     if (!opt_ff && rebase_unspecified && !can_ff)
> > +             show_advice_pull_non_ff();
>
> This part makes sense, but ...
>
> > @@ -1069,13 +1069,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
> >                   submodule_touches_in_range(the_repository, &upstream, &curr_head))
> >                       die(_("cannot rebase with locally recorded submodule modifications"));
> >
> > -             if (can_ff) {
> > -                     /* we can fast-forward this without invoking rebase */
> > -                     opt_ff = "--ff-only";
> > -                     ret = run_merge();
> > -             } else {
> > -                     ret = run_rebase(&newbase, &upstream);
> > -             }
> > +             ret = run_rebase(&newbase, &upstream);
>
> ... as I already pointed out, this does not seem to belong to the
> change.
>
> What makes this hunk necessary?
>
> We used to use run_merge() to fast-forward, now we let run_rebase()
> to first "checkout" their tip, which ends up to be a fast-forward in
> the "can_ff" situation.  As a side effect, opt_ff gets contaminated
> with the current code, but that would not affect what happens after
> this part (i.e. call to rebase_submodules()).

Indeed, you are right.  Sorry about that, I'll re-roll with this hunk
removed, and the modifications to t5520 pulled out as well.