[PATCH v2 0/3] credential: parse subcommands with OPT_SUBCOMMAND

[Wasswa Edson <edsonwasswa54@gmail.com>]

"git credential" dispatches its subcommands by hand with strcmp().
This series converts it to OPT_SUBCOMMAND(), so that each subcommand
gets its own "-h" usage and the subcommand names become available
to completion.

Changes since v1:

 - Split into three patches. The pre-existing mismatch between the
   "-h" output and the SYNOPSIS (1/3) and a refactor moving each
   subcommand into its own function (2/3) are now separate from the
   switch to OPT_SUBCOMMAND() (3/3).

 - Factor the repeated parse_options() call and argument check into
   a helper, which rejects leftover arguments with "too many
   arguments" (Tanga).

 - Add a test for the rejection of extra arguments (Tanga).

 - Drop the claim that the code gets shorter (Tanga).

v1: https://lore.kernel.org/git/20261104202228.24642-1-edsonwasswa54@gmail.com/

Wasswa Edson (3):
  credential: make -h output and SYNOPSIS agree
  credential: move each subcommand into its own function
  credential: parse subcommands with OPT_SUBCOMMAND

 Documentation/git-credential.adoc |   3 +-
 builtin/credential.c              | 143 +++++++++++++++++++++++-------
 git.c                             |   2 +-
 t/t0300-credentials.sh            |  27 ++++++
 t/t0450/adoc-help-mismatches      |   1 -
 5 files changed, 142 insertions(+), 34 deletions(-)

Range-diff against v1:
-:  ---------- > 1:  d1bffb83df credential: make -h output and SYNOPSIS agree
-:  ---------- > 2:  c7708ac31b credential: move each subcommand into its own function
1:  85e3f2ce94 ! 3:  7c9842bb9e credential: parse subcommands with OPT_SUBCOMMAND
    @@ Metadata
      ## Commit message ##
         credential: parse subcommands with OPT_SUBCOMMAND
     
    -    "git credential" dispatches its four subcommands (fill, approve,
    -    reject, capability) with a strcmp() chain after an argc check,
    -    instead of using the parse-options API used by other builtins.
    +    Now that each subcommand of "git credential" lives in its own
    +    function, switch the dispatch to OPT_SUBCOMMAND(). This gives each
    +    subcommand its own usage string, so that "git credential <subcommand>
    +    -h" shows help for that subcommand, and makes the subcommand names
    +    available to the completion via --git-completion-helper.
     
    -    Convert the dispatch to OPT_SUBCOMMAND(). Besides making the code
    -    shorter and more idiomatic, each subcommand gets its own usage
    -    string, so "git credential <subcommand> -h" shows a targeted help
    -    text, and the subcommand names are reported via
    -    --git-completion-helper.
    +    Each subcommand takes neither options nor arguments; reject any
    +    with a common helper. Drop NO_PARSEOPT from the git.c entry, as the
    +    command now uses parse-options.
     
    -    Since the builtin now uses parse-options, drop NO_PARSEOPT from its
    -    git.c entry so "git --list-cmds=parseopt" includes it.
    +    This changes the behavior only on error paths:
     
    -    Two pre-existing inconsistencies are fixed while at it:
    +     - A missing or unknown subcommand is reported as such ("need a
    +       subcommand" / "unknown subcommand") before the usage text.
     
    -    - The usage string did not mention "capability" even though it is a
    -      documented subcommand; it now lists all four.
    -    - Documentation/git-credential.adoc marked its SYNOPSIS as a plain
    -      literal block, which t0450 cannot extract; mark it [verse] and drop
    -      "credential" from t0450/adoc-help-mismatches.
    +     - Extra arguments are reported as "too many arguments" before the
    +       usage text of the subcommand.
     
    -    Behavior changes are limited to error paths: a missing or unknown
    -    subcommand is reported as such ("need a subcommand" / "unknown
    -    subcommand") before the usage text, instead of only showing the
    -    usage text, and "git credential <subcommand> -h" exits 0 after
    -    showing the subcommand's usage. Exit codes for failures are
    -    unchanged (129).
    +     - "git credential <subcommand> -h" shows the usage of that
    +       subcommand and exits with 0, instead of failing with the usage of
    +       the whole command.
     
    -    Signed-off-by: Wasswa Edson <edsonwasswa54@gmail.com>
    +    Failures still exit with 129.
     
    - ## Documentation/git-credential.adoc ##
    -@@ Documentation/git-credential.adoc: git-credential - Retrieve and store user credentials
    - 
    - SYNOPSIS
    - --------
    --------------------
    -+[verse]
    - 'git credential' (fill|approve|reject|capability)
    --------------------
    - 
    - DESCRIPTION
    - -----------
    +    Helped-by: Tanga Joris <tangajoris@gmail.com>
    +    Signed-off-by: Wasswa Edson <edsonwasswa54@gmail.com>
     
      ## builtin/credential.c ##
     @@
    @@ builtin/credential.c
      #include "config.h"
     +#include "gettext.h"
     +#include "parse-options.h"
    - 
    --static const char usage_msg[] =
    --	"git credential (fill|approve|reject)";
    ++
     +static const char * const credential_usage[] = {
     +	N_("git credential (fill|approve|reject|capability)"),
     +	NULL,
     +};
    - 
    --int cmd_credential(int argc,
    --		   const char **argv,
    --		   const char *prefix UNUSED,
    --		   struct repository *repo UNUSED)
    ++
     +static const char * const credential_fill_usage[] = {
     +	N_("git credential fill"),
     +	NULL,
    @@ builtin/credential.c
     +	NULL,
     +};
     +
    ++static void parse_no_args(int argc, const char **argv, const char *prefix,
    ++			  const char * const *usagestr)
    ++{
    ++	struct option options[] = {
    ++		OPT_END()
    ++	};
    + 
    +-static const char usage_msg[] =
    +-	"git credential (fill|approve|reject|capability)";
    ++	if (parse_options(argc, argv, prefix, options, usagestr, 0))
    ++		usage_msg_opt(_("too many arguments"), usagestr, options);
    ++}
    + 
    +-static int cmd_credential_fill(int argc UNUSED, const char **argv UNUSED,
    +-			       const char *prefix UNUSED,
     +static int cmd_credential_fill(int argc, const char **argv, const char *prefix,
    -+			       struct repository *repo UNUSED)
    + 			       struct repository *repo UNUSED)
      {
    --	const char *op;
    -+	struct option options[] = { OPT_END() };
      	struct credential c = CREDENTIAL_INIT;
      
    --	repo_config(the_repository, git_default_config, NULL);
    -+	argc = parse_options(argc, argv, prefix, options,
    -+			     credential_fill_usage, 0);
    -+	if (argc)
    -+		usage_with_options(credential_fill_usage, options);
    -+
    -+	if (credential_read(&c, stdin, CREDENTIAL_OP_INITIAL) < 0)
    -+		die("unable to read credential from stdin");
    ++	parse_no_args(argc, argv, prefix, credential_fill_usage);
     +
    -+	credential_fill(the_repository, &c, 0);
    -+	credential_next_state(&c);
    -+	credential_write(&c, stdout, CREDENTIAL_OP_RESPONSE);
    -+
    -+	credential_clear(&c);
    -+	return 0;
    -+}
    + 	if (credential_read(&c, stdin, CREDENTIAL_OP_INITIAL) < 0)
    + 		die("unable to read credential from stdin");
      
    --	show_usage_if_asked(argc, argv, usage_msg);
    --	if (argc != 2)
    --		usage(usage_msg);
    --	op = argv[1];
    -+static int cmd_credential_approve(int argc, const char **argv,
    -+				const char *prefix,
    -+				struct repository *repo UNUSED)
    -+{
    -+	struct option options[] = { OPT_END() };
    -+	struct credential c = CREDENTIAL_INIT;
    +@@ builtin/credential.c: static int cmd_credential_fill(int argc UNUSED, const char **argv UNUSED,
    + 	return 0;
    + }
      
    --	if (!strcmp(op, "capability")) {
    --		credential_set_all_capabilities(&c, CREDENTIAL_OP_INITIAL);
    --		credential_announce_capabilities(&c, stdout);
    --		return 0;
    --	}
    -+	argc = parse_options(argc, argv, prefix, options,
    -+			     credential_approve_usage, 0);
    -+	if (argc)
    -+		usage_with_options(credential_approve_usage, options);
    +-static int cmd_credential_approve(int argc UNUSED, const char **argv UNUSED,
    +-				  const char *prefix UNUSED,
    ++static int cmd_credential_approve(int argc, const char **argv,
    ++				  const char *prefix,
    + 				  struct repository *repo UNUSED)
    + {
    + 	struct credential c = CREDENTIAL_INIT;
      
    ++	parse_no_args(argc, argv, prefix, credential_approve_usage);
    ++
      	if (credential_read(&c, stdin, CREDENTIAL_OP_INITIAL) < 0)
      		die("unable to read credential from stdin");
      
    --	if (!strcmp(op, "fill")) {
    --		credential_fill(the_repository, &c, 0);
    --		credential_next_state(&c);
    --		credential_write(&c, stdout, CREDENTIAL_OP_RESPONSE);
    --	} else if (!strcmp(op, "approve")) {
    --		credential_set_all_capabilities(&c, CREDENTIAL_OP_HELPER);
    --		credential_approve(the_repository, &c);
    --	} else if (!strcmp(op, "reject")) {
    --		credential_set_all_capabilities(&c, CREDENTIAL_OP_HELPER);
    --		credential_reject(the_repository, &c);
    --	} else {
    --		usage(usage_msg);
    --	}
    -+	credential_set_all_capabilities(&c, CREDENTIAL_OP_HELPER);
    -+	credential_approve(the_repository, &c);
    - 
    - 	credential_clear(&c);
    +@@ builtin/credential.c: static int cmd_credential_approve(int argc UNUSED, const char **argv UNUSED,
      	return 0;
      }
    -+
    + 
    +-static int cmd_credential_reject(int argc UNUSED, const char **argv UNUSED,
    +-				 const char *prefix UNUSED,
     +static int cmd_credential_reject(int argc, const char **argv,
     +				 const char *prefix,
    -+				 struct repository *repo UNUSED)
    -+{
    -+	struct option options[] = { OPT_END() };
    -+	struct credential c = CREDENTIAL_INIT;
    -+
    -+	argc = parse_options(argc, argv, prefix, options,
    -+			     credential_reject_usage, 0);
    -+	if (argc)
    -+		usage_with_options(credential_reject_usage, options);
    -+
    -+	if (credential_read(&c, stdin, CREDENTIAL_OP_INITIAL) < 0)
    -+		die("unable to read credential from stdin");
    -+
    -+	credential_set_all_capabilities(&c, CREDENTIAL_OP_HELPER);
    -+	credential_reject(the_repository, &c);
    -+
    -+	credential_clear(&c);
    -+	return 0;
    -+}
    + 				 struct repository *repo UNUSED)
    + {
    + 	struct credential c = CREDENTIAL_INIT;
    + 
    ++	parse_no_args(argc, argv, prefix, credential_reject_usage);
     +
    + 	if (credential_read(&c, stdin, CREDENTIAL_OP_INITIAL) < 0)
    + 		die("unable to read credential from stdin");
    + 
    +@@ builtin/credential.c: static int cmd_credential_reject(int argc UNUSED, const char **argv UNUSED,
    + 	return 0;
    + }
    + 
    +-static int cmd_credential_capability(int argc UNUSED,
    +-				     const char **argv UNUSED,
    +-				     const char *prefix UNUSED,
     +static int cmd_credential_capability(int argc, const char **argv,
     +				     const char *prefix,
    -+				     struct repository *repo UNUSED)
    -+{
    -+	struct option options[] = { OPT_END() };
    -+	struct credential c = CREDENTIAL_INIT;
    -+
    -+	argc = parse_options(argc, argv, prefix, options,
    -+			     credential_capability_usage, 0);
    -+	if (argc)
    -+		usage_with_options(credential_capability_usage, options);
    -+
    -+	credential_set_all_capabilities(&c, CREDENTIAL_OP_INITIAL);
    -+	credential_announce_capabilities(&c, stdout);
    -+
    -+	credential_clear(&c);
    -+	return 0;
    -+}
    + 				     struct repository *repo UNUSED)
    + {
    + 	struct credential c = CREDENTIAL_INIT;
    + 
    ++	parse_no_args(argc, argv, prefix, credential_capability_usage);
     +
    -+int cmd_credential(int argc,
    -+		   const char **argv,
    -+		   const char *prefix,
    -+		   struct repository *repo)
    -+{
    + 	credential_set_all_capabilities(&c, CREDENTIAL_OP_INITIAL);
    + 	credential_announce_capabilities(&c, stdout);
    + 
    +@@ builtin/credential.c: int cmd_credential(int argc,
    + 		   const char *prefix,
    + 		   struct repository *repo)
    + {
    +-	const char *op;
     +	parse_opt_subcommand_fn *fn = NULL;
     +	struct option options[] = {
     +		OPT_SUBCOMMAND("fill", &fn, cmd_credential_fill),
    @@ builtin/credential.c
     +		OPT_SUBCOMMAND("capability", &fn, cmd_credential_capability),
     +		OPT_END()
     +	};
    -+
    -+	repo_config(the_repository, git_default_config, NULL);
    -+
    + 
    + 	repo_config(the_repository, git_default_config, NULL);
    + 
    +-	show_usage_if_asked(argc, argv, usage_msg);
    +-	if (argc != 2)
    +-		usage(usage_msg);
    +-	op = argv[1];
    +-
    +-	if (!strcmp(op, "fill"))
    +-		return cmd_credential_fill(argc - 1, argv + 1, prefix, repo);
    +-	if (!strcmp(op, "approve"))
    +-		return cmd_credential_approve(argc - 1, argv + 1, prefix, repo);
    +-	if (!strcmp(op, "reject"))
    +-		return cmd_credential_reject(argc - 1, argv + 1, prefix, repo);
    +-	if (!strcmp(op, "capability"))
    +-		return cmd_credential_capability(argc - 1, argv + 1, prefix,
    +-						 repo);
    +-
    +-	usage(usage_msg);
     +	argc = parse_options(argc, argv, prefix, options, credential_usage, 0);
     +
     +	return fn(argc, argv, prefix, repo);
    -+}
    + }
     
      ## git.c ##
     @@ git.c: static struct cmd_struct commands[] = {
    @@ t/t0300-credentials.sh: test_expect_success 'interactive prompt is sanitized' '
     +	test_expect_code 129 git credential foo 2>err &&
     +	test_grep "unknown subcommand" err
     +'
    ++
    ++test_expect_success 'credential subcommands reject extra arguments' '
    ++	for sub in fill approve reject capability
    ++	do
    ++		test_expect_code 129 git credential $sub extra \
    ++			</dev/null 2>err &&
    ++		test_grep "too many arguments" err || return 1
    ++	done
    ++'
     +
      test_done
    -
    - ## t/t0450/adoc-help-mismatches ##
    -@@ t/t0450/adoc-help-mismatches: checkout
    - checkout-index
    - clone
    - column
    --credential
    - credential-cache
    - credential-store
    - fetch-pack

base-commit: 37b3475fb530c14109211faca33716a173db61ed
-- 
2.53.0