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

From: SZEDER Gábor <[email-masked]>
Date: Tue, 2 Aug 2022 19:37:54 +0200

On Mon, Jul 25, 2022 at 04:31:40PM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> On Mon, Jul 25 2022, SZEDER Gábor wrote:
> 
> > Our Bash completion script can complete --options for commands using
> > parse-options even when that command doesn't have a dedicated
> > completion function, but to do so the completion script must know
> > which commands use parse-options and which don't.  Therefore, commands
> > not using parse-options are marked in 'git.c's command list with the
> > NO_PARSEOPT flag.
> >
> > Update this list, and remove this flag from the commands that by now
> > use parse-options.
> >
> > Signed-off-by: SZEDER Gábor <[email-masked]>
> > ---
> >  git.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/git.c b/git.c
> > index e5d62fa5a9..c4282f194a 100644
> > --- a/git.c
> > +++ b/git.c
> > @@ -489,14 +489,14 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
> >  static struct cmd_struct commands[] = {
> >  	{ "add", cmd_add, RUN_SETUP | NEED_WORK_TREE },
> >  	{ "am", cmd_am, RUN_SETUP | NEED_WORK_TREE },
> > -	{ "annotate", cmd_annotate, RUN_SETUP | NO_PARSEOPT },
> > +	{ "annotate", cmd_annotate, RUN_SETUP },
> >  	{ "apply", cmd_apply, RUN_SETUP_GENTLY },
> >  	{ "archive", cmd_archive, RUN_SETUP_GENTLY },
> >  	{ "bisect--helper", cmd_bisect__helper, RUN_SETUP },
> >  	{ "blame", cmd_blame, RUN_SETUP },
> >  	{ "branch", cmd_branch, RUN_SETUP | DELAY_PAGER_CONFIG },
> >  	{ "bugreport", cmd_bugreport, RUN_SETUP_GENTLY },
> > -	{ "bundle", cmd_bundle, RUN_SETUP_GENTLY | NO_PARSEOPT },
> > +	{ "bundle", cmd_bundle, RUN_SETUP_GENTLY },
> >  	{ "cat-file", cmd_cat_file, RUN_SETUP },
> >  	{ "check-attr", cmd_check_attr, RUN_SETUP },
> >  	{ "check-ignore", cmd_check_ignore, RUN_SETUP | NEED_WORK_TREE },
> > @@ -514,7 +514,7 @@ static struct cmd_struct commands[] = {
> >  	{ "column", cmd_column, RUN_SETUP_GENTLY },
> >  	{ "commit", cmd_commit, RUN_SETUP | NEED_WORK_TREE },
> >  	{ "commit-graph", cmd_commit_graph, RUN_SETUP },
> > -	{ "commit-tree", cmd_commit_tree, RUN_SETUP | NO_PARSEOPT },
> > +	{ "commit-tree", cmd_commit_tree, RUN_SETUP },
> >  	{ "config", cmd_config, RUN_SETUP_GENTLY | DELAY_PAGER_CONFIG },
> >  	{ "count-objects", cmd_count_objects, RUN_SETUP },
> >  	{ "credential", cmd_credential, RUN_SETUP_GENTLY | NO_PARSEOPT },
> > @@ -553,7 +553,7 @@ static struct cmd_struct commands[] = {
> >  	{ "ls-files", cmd_ls_files, RUN_SETUP },
> >  	{ "ls-remote", cmd_ls_remote, RUN_SETUP_GENTLY },
> >  	{ "ls-tree", cmd_ls_tree, RUN_SETUP },
> > -	{ "mailinfo", cmd_mailinfo, RUN_SETUP_GENTLY | NO_PARSEOPT },
> > +	{ "mailinfo", cmd_mailinfo, RUN_SETUP_GENTLY },
> >  	{ "mailsplit", cmd_mailsplit, NO_PARSEOPT },
> >  	{ "maintenance", cmd_maintenance, RUN_SETUP | NO_PARSEOPT },
> >  	{ "merge", cmd_merge, RUN_SETUP | NEED_WORK_TREE },
> > @@ -566,7 +566,7 @@ static struct cmd_struct commands[] = {
> >  	{ "merge-recursive-theirs", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
> >  	{ "merge-subtree", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
> >  	{ "merge-tree", cmd_merge_tree, RUN_SETUP },
> > -	{ "mktag", cmd_mktag, RUN_SETUP | NO_PARSEOPT },
> > +	{ "mktag", cmd_mktag, RUN_SETUP },
> >  	{ "mktree", cmd_mktree, RUN_SETUP },
> >  	{ "multi-pack-index", cmd_multi_pack_index, RUN_SETUP },
> >  	{ "mv", cmd_mv, RUN_SETUP | NEED_WORK_TREE },
> > @@ -627,7 +627,7 @@ static struct cmd_struct commands[] = {
> >  	{ "verify-tag", cmd_verify_tag, RUN_SETUP },
> >  	{ "version", cmd_version },
> >  	{ "whatchanged", cmd_whatchanged, RUN_SETUP },
> > -	{ "worktree", cmd_worktree, RUN_SETUP | NO_PARSEOPT },
> > +	{ "worktree", cmd_worktree, RUN_SETUP },
> >  	{ "write-tree", cmd_write_tree, RUN_SETUP },
> >  };
> 
> I have a superset of this change in my local tree:
> https://github.com/avar/git/commit/f0076de17fd
> 
> I.e. "parseopt" here really means "handles --git-completion-helper"

Indeed, with additional subtleties, like it doesn't try to read from
stdin before handling '--git-completion-helper'.

> so
> we can also address the diff family here. See the test that's associated
> with the change (which you might want to grab in any case, i.e. to have
> tests in this area).

Dunno.  I don't like this NO_PARSEOPT thing, and instead of testing it
I'm thinking about removing it altogether.  It seems to be doable, but
of course conflicts with the upcoming 'completion-subcommand' patch
series, so I'll leave it for later.

  https://github.com/szeder/git/commits/completion-poc-builtins