Re: [PATCH v2 1/2] rebase: remove completely useless -C option

From: Elijah Newren <newren@gmail.com>
Date: Jan 20, 2023, 06:42 UTC
Message-ID: <CABPp-BHCKweQR=i8sZx2Nrh3YTXCThys7=EeN+jsxvLb=L_KVg@mail.gmail.com>
In-Reply-To: <CAPig+cQj4Gi+ikkPz3wffqGZVz5uALGzYV25nio3k4+cthP9Uw@mail.gmail.com>

On Thu, Jan 19, 2023 at 9:40 PM Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 41 quoted lines

>
> On Fri, Jan 20, 2023 at 12:14 AM Elijah Newren via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > The `-C` option to rebase was introduced with 67dad687ad ("add -C[NUM]
> > to git-am", 2007-02-08).  Based on feedback on the patch, the author
> > added the -C option not just to git-am but also to git-rebase.  But did
> > it such that the option was just passed through to git-am (which passes
> > it along to git-apply), with no corresponding option to format-patch.
> >
> > As per the git-apply documentation for the `-C` option:
> >     Ensure at least <n> lines of surrounding context match...When fewer
> >     lines of surrounding context exist they all must match.
> >
> > The fact that format-patch was not passed a -U option to increase the
> > number of context lines meant that there would still only be 3 lines of
> > context to match on.  So, anyone attempting to use this option in
> > git-rebase would just be spinning their wheels and wasting time.  I was
> > one such user a number of years ago.
> >
> > Since this option can at best waste users time and is nothing more than
> > a confusing no-op, and has never been anything other than a confusing
> > no-op, and no one has ever bothered to create a testcase for it that
> > goes beyond option parsing, simply excise the option.
> >
> > Signed-off-by: Elijah Newren <newren@gmail.com>
>
> Is there a chance that this patch could break some existing tooling or
> someone's workflow or alias?  If so, perhaps it would be better to
> continue recognizing it, but as a proper no-op? That is:
>
> (1) continue accepting the option but mark it PARSE_OPT_HIDDEN or some such
>
> (2) completely ignore the option
>
> (3) in case someone runs across it in some existing script or example
> and wants to know what it does, leave one mention of it in the
> documentation, such as:
>
>     -C<n>
>         Does nothing. This option is accepted for backward compatibility
>         only. Do not use.

If an option becomes a no-op over time due to other changes, this would likely be the route I'd lean towards. I'm just not sure it's merited in this case.

We already turned numerous no-op choices in rebase into errors with
commit 8295ed690b ("rebase: make the backend configurable via config
setting", 2020-02-15), where we started pointing out that apply-based
and merge-based options were incompatible (as opposed to silently
accepting competing options and ignoring ones not supported by
whichever backend happened to trump at the time based on the options.)
 So, there's recent precedent to jump directly to errors with no-ops
in rebase.  Those cases are slightly different in that options were
only conditionally no-ops, whereas in this case we have an option that
is unconditionally a no-op, suggesting we might be able to do
something stronger.

On top of that, I just can't find any evidence of anyone ever using this option: (a) it was only put in as an afterthought by the original author (who was only really interested in git-am -C), (b) there are absolutely no testcases in the testsuite covering its behavior, (c) my searches of the mailing list and google don't turn up any hits though admittedly it's hard to figure out what to search on, and (d) while I tried to use the option at times, it was only because I was doing work to make backends consistent and switch the default one and trying to understand all the inner workings, and even then I gave up on it and punted it down the road.

And, of course, the option never worked as advertised anyway.

That combination makes me think nuking it would go completely unnoticed outside this mailing list. If others would rather see the more cautious route despite all the above, I can implement it, but likely alter your step (2) from ignoring into throwing an error message (or at the very least a warning).