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

From: Jeff King <peff@peff.net>
Date: Dec 21, 2023, 21:45 UTC
Message-ID: <20231221214550.GD1446091@coredump.intra.peff.net>
In-Reply-To: <xmqqsf3vmqug.fsf@gitster.g>

On Thu, Dec 21, 2023 at 09:02:15AM -0800, Junio C Hamano wrote:

Show 14 quoted lines

> > Yeah. I think it is in the same boat as the other two, in that I believe
> > that the KEEP_UNKNOWN_OPT flag is counter-productive and should just be
> > dropped.
> 
> If we dropped KEEP_UNKNOWN_OPT, however, the pattern that is
> currently accepted will stop working, e.g.,
> 
>  $ git sparse-checkout [add/set] --[no-]cone --foo bar
> 
> as we would barf with "--foo: unknown option", and the users who are
> used to this sloppy command line parsing we had for the past few
> years must now write "--end-of-options" before "--foo".  After all,
> the reason why the original authors of this code used KEEP_UNKNOWN
> is likely to deal with path patterns that begin with dashes.

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.

Show 11 quoted lines

> The patch in the message that started this thread may not be
> correct, either, I am afraid.  For either of these:
> 
>  $ git sparse-checkout [add/set] --[no-]cone foo --end-of-options bar
>  $ git sparse-checkout [add/set] --[no-]cone --foo --end-of-options bar
> 
> we would see that "foo" (or "--foo") is not "--end-of-options", and
> we end up using three patterns "foo" (or "--foo"),
> "--end-of-options", and "bar", I suspect.  I wonder if we should
> notice the "foo" or "--foo" that were not understood and error out,
> instead.

Yes, I agree that "--foo --end-of-options" should barf. And of course that happens naturally if you just let parse-options do its job by not passing the KEEP_UNKNOWN_OPT flag. ;)

I'm not sure about "foo". We do allow out-of-order options/args, so isn't it correct to keep it as a non-option argument?

Show 11 quoted lines

> But after all, it is not absolutely necessary to notice and barf.
> The ONLY practical use of the "--end-of-options" mechanism is to
> allow us to write (this applies to any git subcommand):
> 
>  #!/bin/sh
>  git cmd --hard --coded --options --end-of-options "$@"
> 
> in scripts to protect the intended operation from mistaking the
> end-user input as options.  And with a script written carefully to
> do so, all the args that appear before "--end-of-options" would be
> recognizable by the command line parser.

Yes, but if you misspell "--otpions", it magically becomes a parameter rather than having the command barf and complain. That is not the end of the world, but it is unfriendly and inconsistent with the rest of Git.

-Peff