Re: [PATCH/RFC] sparse-checkout: take care of "--end-of-options" in set/add

From: Elijah Newren <newren@gmail.com>
Date: Dec 23, 2023, 22:45 UTC
Message-ID: <CABPp-BF9XZeESHdxdcZ91Vsn5tKqQ6_3tC11e7t9vTFp=uufbg@mail.gmail.com>
In-Reply-To: <20231223100229.GA2016274@coredump.intra.peff.net>

Hi,

On Sat, Dec 23, 2023 at 2:02 AM Jeff King <peff@peff.net> wrote:

Show 26 quoted lines

>
> On Thu, Dec 21, 2023 at 02:04:56PM -0800, Junio C Hamano wrote:
>
> > Jeff King <peff@peff.net> writes:
> >
> > > Right, that is the "gotcha" I mentioned in my other email. Though that
> > > is the way it has behaved historically, my argument is that users are
> > > unreasonable to expect it to work:
> > >
> > >   1. It is not consistent with the rest of Git commands.
> > >
> > >   2. It is inconsistent with respect to existing options (and is an
> > >      accident waiting to happen when new options are added).
> > >
> > > So I'd consider it a bug-fix.
> >
> > So the counter-proposal here is just to drop KEEP_UNKNOWN_OPT and
> > deliberately break them^W^W^Wrealign their expectations?
>
> Yes. :) But keep in mind we are un-breaking other people, like those who
> typo:
>
>   git sparse-checkout --sikp-checks
>
> and don't see an error (instead, we make a garbage entry in the sparse
> checkout file).

I think you mean
     git sparse-checkout set --sikp-checks
or
     git sparse-checkout add --sikp-checks
(i.e. with either 'set' or 'add' in there)

Neither of these are currently an error, but I agree both definitely ought to be. (Without the 'set' or 'add', you currently do get an error, as we'd expect.)

Show 9 quoted lines

> > I do not have much stake in sparse-checkout, so I am fine with that
> > direction.  But I suspect other folks on the list would have users
> > of their own who would rather loudly complain to them if we broke
> > them ;-)
>
> Likewise, I have never used sparse-checkout myself, and I don't care
> _that_ much. My interest is mostly in having various Git commands behave
> consistently. This whole discussion started because the centralized
> --end-of-options fix interacted badly with this unusual behavior.

Well, I care quite a bit about sparse-checkout, and I would rather see Peff's proposed fix, even if some users might have to tweak their commands a little.

Of course, I'm not the only sparse-checkout user representative, but Randall commented elsewhere in this thread that he used sparse-checkout quite a bit, and he feels that "without the --, --sikp-checks should result in an error." In other words, he is basically agreeing that taking Peff's fix seems more important than strict backward compatibility here. (His wording may not sound like that at first glance, probably because of the 'set'/'add' omission in the example command, but I think what he said actually amounts to agreeing with this change.)

Finally, git-sparse-checkout(1) does say "THIS COMMAND IS EXPERIMENTAL. ITS BEHAVIOR...WILL LIKELY CHANGE". I apologize for being pulled from my intent to work on [*], which I think would allow us to finally drop this warning (I'll still get back to it, one way or another...), but I think this is a good case where we should take advantage of the existing warning and simply fix the command.

Anyway, just my $0.02, Elijah

[*] https://lore.kernel.org/git/pull.1367.v4.git.1667714666810.gitgitgadget@gmail.com/