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

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Dec 4, 2020, 23:36 UTC
Message-ID: <CAMP44s2T4HSVAwx6D3sO5c8b_tSKT174eASTjmb61kKmm=JcwQ@mail.gmail.com>
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:

Show 10 quoted lines

>
> 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.

Show 10 quoted lines

> 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