Re: [PATCH 01/20] git.c: update NO_PARSEOPT markings

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Aug 3, 2022, 21:34 UTC
Message-ID: <20220803213430.GB10040@szeder.dev>
In-Reply-To: <xmqqsfmefjja.fsf@gitster.g>

On Tue, Aug 02, 2022 at 02:00:09PM -0700, Junio C Hamano wrote:

Show 13 quoted lines

> SZEDER Gábor <szeder.dev@gmail.com> writes:
> 
> > Dunno.  I don't like this NO_PARSEOPT thing, and instead of testing it
> > I'm thinking about removing it altogether.
> 
> Sorry if this is obvious to the others, but I am confused here.
> 
> Lack of NO_PARSEOPT bit is used as a mark to say "this subcommand
> takes '--git-completion-helper' option to help listing the options,
> so include it in the 'git --list-cmds=parseopt' output", right?  I
> do not mind removing an unused or unnecessary bit at all, but what
> is your plan of getting rid of the bit?  Will we make sure everybody
> supports the "--git-completion-helper" option?

A bit of clarification first: This NO_PARSEOPT flag only matters for git commands for which we don't have a corresponding _git_cmd() completion function in our Bash completion script. If there is such a function, then either:

  - that function has a hardcoded list of --options, for commands that
    don't use parse-options, or

  - it invokes '__gitcomp_builtin cmd' to complete --options with the
    help of '--git-completion-helper'.  Note that this works even if
    said 'cmd' were marked with the NO_PARSEOPT flag, moreover,
    'cmd' doesn't even have to be a builtin, i.e. 'git send-email
    --git-completion-helper' works as Ævar pointed out.

For the many builtin (mostly plumbing) commands that do use parse-options but don't have such a _git_cmd() completion function, we added the generic fallback __git_complete_common() to complete their options with '__gitcomp_builtin cmd'. And this is where the NO_PARSEOPT flag matters, because the completion script only ever invokes that fallback function for commands that are not marked with that flag. See 9f642a7169 (completion: add --option completion for most builtin commands, 2018-03-24) for more details.

So far so good.

Now, instead of relying on (possibly outdated) NO_PARSEOPT markings telling us not to use that fallback function, i.e. not invoke certain commands with '--git-completion-helper', I'm considering whether we could rely on our git commands behaving sensibly when invoked with '--git-completion-helper'. I.e. a git command either supports that option and lists all its --options, or fails fast because of the unknown option without printing anything to stdout. Then perhaps the completion script doesn't even have to pay attention to this error, because it will do the right thing as long as the

  options=$(git builtin --git-completion-helper)

command fills $options with the list of --options or leaves it empty.

AFAICT almost all builtins behave sensibly in this sense (I haven't looked at non-builtin git commands yet), with the exception of:

  - 'git credential' and 'fast-import': these two attempt to read from
    stdin before recognizing the unknown --git-completion-helper
    option and hang.  On one hand, this hang can be easily prevented
    by redirecting stdin from /dev/null (but 'fast-import' leaves
    behind a crash file, which is not nice).  OTOH, I'm inclined to
    consider this behavior as a bug: all commands should fail fast on
    unknown --options.

  - 'git rev-parse --git-completion-helper' prints
    '--git-completion-helper' to stdout.  I forgot about this, but
    this is how this command is supposed to behave.  I guess I'll just
    have to special case this command in the completion script...

Of course we can only do this with git builtin commands, because those are under our control and we can make sure that they behave sensibly. We must not pass '--git-completion-helper' to any 'git-foo' command that might be present in $PATH, because who knows what they might do.

However, I'll definitely need to think more about corner cases, e.g. hitting 'git cmd --<TAB>' while outside a repository doesn't even reaches parse_options() if that command requires a repository. But let's leave that for later and get this series in shape first.