Re: [PATCH v2 06/14] pull: move default warning

From: Felipe Contreras <felipe.contreras@gmail.com>
To: Elijah Newren <newren@gmail.com>
Cc: "Git Mailing List" <git@vger.kernel.org>,
	"Vít Ondruch" <vondruch@redhat.com>,
	"Alex Henrie" <alexhenrie24@gmail.com>,
	"Junio C Hamano" <gitster@pobox.com>, "Jeff King" <peff@peff.net>,
	"Philip Oakley" <philipoakley@iee.email>,
	"Theodore Y . Ts'o" <tytso@mit.edu>,
	"Jacob Keller" <jacob.keller@gmail.com>,
	"John Keeping" <john@keeping.me.uk>,
	"Richard Hansen" <rhansen@rhansen.org>,
	"Brian M. Carlson" <sandals@crustytoothpaste.net>,
	"W. Trevor King" <wking@tremily.us>
Subject: Re: [PATCH v2 06/14] pull: move default warning
Date: Fri, 4 Dec 2020 17:36:28 -0600	[thread overview]
Message-ID: <CAMP44s2T4HSVAwx6D3sO5c8b_tSKT174eASTjmb61kKmm=JcwQ@mail.gmail.com> (raw)
In-Reply-To: <CABPp-BHH0baiECvtvVOoHR82upJ1C+0hy-ukS9Hi2ZFHh19nOg@mail.gmail.com>

On Fri, Dec 4, 2020 at 5:18 PM Elijah Newren <newren@gmail.com> wrote:
>
> On Thu, Dec 3, 2020 at 10:16 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> >
> > Up to the point where can check if we can fast-forward or not.
>
> Seem to be missing some subjects in that sentence.  ;-)  Perhaps:
>
> Move the default warning to the point where we can check if we can
> fast-forward or not.

Yes, I missed a "we".

I usually continue what I said in the subject, but lately while
reviewing my own patches in lore.kernel.org I've found it hard to
understand the first paragraph since it's not easy to find the
subject.

Maybe I should stop doing that.

> > No functional changes.
>
> You didn't explain the reasoning for the change here, though I suspect
> it makes it easier to change the default to ff-only later.

Right, I noticed that after the nth time I reviewed the series.

It's not actually to change the default to ff-only, it's mainly to
display the annoying warning only when a non-fast-forward pull is
being attempted.

I'll add that to the commit message.

> However,
> looking over the patch and pulling up the code, I actually find it
> pretty odd that this warning was in a function named
> config_get_rebase().  The warning is not rebase-specific, and so
> clearly does not belong there.  And for such a function name, the only
> kinds of warnings I'd expect are ones where the user configured some
> option but set it to a value that cannot make sense.  So it all around
> seems like the wrong place to me, and I find your patch to be a good
> cleanup.  It would benefit from a slightly improved commit message
> though.  :-)

Indeed. The only reason it's there is that it's the only place we know
the user has not done either --rebase, or --merge (--no-rebase), or
the config equivalents.

I fixed that by introducing a default_mode flag, but in other patch
series I replace default_mode with a check for REBASE_DEFAULT (which
doesn't exist yet).

Cheers.

-- 
Felipe Contreras