git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: Josh Steadmon <steadmon@google.com>, git@vger.kernel.org
Subject: Re: [PATCH/RFC] sparse-checkout: take care of "--end-of-options" in set/add
Date: Thu, 21 Dec 2023 16:45:50 -0500	[thread overview]
Message-ID: <20231221214550.GD1446091@coredump.intra.peff.net> (raw)
In-Reply-To: <xmqqsf3vmqug.fsf@gitster.g>

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

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

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

> 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

  reply	other threads:[~2023-12-21 21:45 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-12-20 23:19 [PATCH/RFC] sparse-checkout: take care of "--end-of-options" in set/add Junio C Hamano
2023-12-20 23:55 ` Josh Steadmon
2023-12-21  2:46   ` Junio C Hamano
2023-12-21  8:40     ` Jeff King
2023-12-21 17:02       ` Junio C Hamano
2023-12-21 21:45         ` Jeff King [this message]
2023-12-21 22:04           ` Junio C Hamano
2023-12-23 10:02             ` Jeff King
2023-12-23 15:38               ` rsbecker
2023-12-23 22:45               ` Elijah Newren
2023-12-24  1:02                 ` Elijah Newren
2023-12-21  2:41 ` [RFC/PATCH] archive: "--list" does not take further options Junio C Hamano
2023-12-21  7:30   ` René Scharfe
2023-12-21  8:59     ` Jeff King
2023-12-21 18:13       ` Junio C Hamano
2023-12-21 21:35         ` Jeff King
2023-12-21  8:58   ` Jeff King
2023-12-21  8:36 ` [PATCH/RFC] sparse-checkout: take care of "--end-of-options" in set/add Jeff King
2023-12-21 18:20   ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20231221214550.GD1446091@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=steadmon@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).