git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 19/20] builtin/stash.c: let parse-options parse subcommands

From
SZEDER Gábor <szeder.dev@gmail.com>
Date
Aug 20, 2022, 10:27 UTC
Message-ID
<20220820102731.GC3373722@szeder.dev>
In-Reply-To
<220819.86czcw59w6.gmgdl@evledraar.gmail.com>
On Fri, Aug 19, 2022 at 09:06:32PM +0200, Ævar Arnfjörð Bjarmason wrote:
Show 128 quoted lines
> 
> On Fri, Aug 19 2022, SZEDER Gábor wrote:
> 
> > 'git stash' parses its subcommands with a long list of if-else if
> > statements.  parse-options has just learned to parse subcommands, so
> > let's use that facility instead, with the benefits of shorter code,
> > and listing subcommands for Bash completion.
> >
> > Note that the push_stash() function implementing the 'push' subcommand
> > accepts an extra flag parameter to indicate whether push was assumed,
> > so add a wrapper function with the standard subcommand function
> > signature.
> >
> > Note also that this change "hides" the '-h' option in 'git stash push
> > -h' from the parse_option() call in cmd_stash(), as it comes after the
> > subcommand.  Consequently, from now on it will emit the usage of the
> > 'push' subcommand instead of the usage of 'git stash'.  We had a
> > failing test for this case, which can now be flipped to expect
> > success.
> >
> > Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> > ---
> >  builtin/stash.c  | 53 ++++++++++++++++++++++--------------------------
> >  t/t3903-stash.sh |  2 +-
> >  2 files changed, 25 insertions(+), 30 deletions(-)
> >
> > diff --git a/builtin/stash.c b/builtin/stash.c
> > index a14e832e9f..1ba24c1173 100644
> > --- a/builtin/stash.c
> > +++ b/builtin/stash.c
> > @@ -1739,6 +1739,11 @@ static int push_stash(int argc, const char **argv, const char *prefix,
> >  			     include_untracked, only_staged);
> >  }
> >  
> > +static int push_stash_unassumed(int argc, const char **argv, const char *prefix)
> > +{
> > +	return push_stash(argc, argv, prefix, 0);
> > +}
> > +
> >  static int save_stash(int argc, const char **argv, const char *prefix)
> >  {
> >  	int keep_index = -1;
> > @@ -1787,15 +1792,28 @@ int cmd_stash(int argc, const char **argv, const char *prefix)
> >  	pid_t pid = getpid();
> >  	const char *index_file;
> >  	struct strvec args = STRVEC_INIT;
> > -
> > +	parse_opt_subcommand_fn *fn = NULL;
> >  	struct option options[] = {
> > +		OPT_SUBCOMMAND("apply", &fn, apply_stash),
> > +		OPT_SUBCOMMAND("clear", &fn, clear_stash),
> > +		OPT_SUBCOMMAND("drop", &fn, drop_stash),
> > +		OPT_SUBCOMMAND("pop", &fn, pop_stash),
> > +		OPT_SUBCOMMAND("branch", &fn, branch_stash),
> > +		OPT_SUBCOMMAND("list", &fn, list_stash),
> > +		OPT_SUBCOMMAND("show", &fn, show_stash),
> > +		OPT_SUBCOMMAND("store", &fn, store_stash),
> > +		OPT_SUBCOMMAND("create", &fn, create_stash),
> > +		OPT_SUBCOMMAND("push", &fn, push_stash_unassumed),
> > +		OPT_SUBCOMMAND_F("save", &fn, save_stash, PARSE_OPT_NOCOMPLETE),
> >  		OPT_END()
> >  	};
> >  
> >  	git_config(git_stash_config, NULL);
> >  
> >  	argc = parse_options(argc, argv, prefix, options, git_stash_usage,
> > -			     PARSE_OPT_KEEP_UNKNOWN_OPT | PARSE_OPT_KEEP_DASHDASH);
> > +			     PARSE_OPT_SUBCOMMAND_OPTIONAL |
> > +			     PARSE_OPT_KEEP_UNKNOWN_OPT |
> > +			     PARSE_OPT_KEEP_DASHDASH);
> >  
> >  	prepare_repo_settings(the_repository);
> >  	the_repository->settings.command_requires_full_index = 0;
> > @@ -1804,33 +1822,10 @@ int cmd_stash(int argc, const char **argv, const char *prefix)
> >  	strbuf_addf(&stash_index_path, "%s.stash.%" PRIuMAX, index_file,
> >  		    (uintmax_t)pid);
> >  
> > -	if (!argc)
> > -		return !!push_stash(0, NULL, prefix, 0);
> > -	else if (!strcmp(argv[0], "apply"))
> > -		return !!apply_stash(argc, argv, prefix);
> > -	else if (!strcmp(argv[0], "clear"))
> > -		return !!clear_stash(argc, argv, prefix);
> > -	else if (!strcmp(argv[0], "drop"))
> > -		return !!drop_stash(argc, argv, prefix);
> > -	else if (!strcmp(argv[0], "pop"))
> > -		return !!pop_stash(argc, argv, prefix);
> > -	else if (!strcmp(argv[0], "branch"))
> > -		return !!branch_stash(argc, argv, prefix);
> > -	else if (!strcmp(argv[0], "list"))
> > -		return !!list_stash(argc, argv, prefix);
> > -	else if (!strcmp(argv[0], "show"))
> > -		return !!show_stash(argc, argv, prefix);
> > -	else if (!strcmp(argv[0], "store"))
> > -		return !!store_stash(argc, argv, prefix);
> > -	else if (!strcmp(argv[0], "create"))
> > -		return !!create_stash(argc, argv, prefix);
> > -	else if (!strcmp(argv[0], "push"))
> > -		return !!push_stash(argc, argv, prefix, 0);
> > -	else if (!strcmp(argv[0], "save"))
> > -		return !!save_stash(argc, argv, prefix);
> > -	else if (*argv[0] != '-')
> > -		usage_msg_optf(_("unknown subcommand: %s"),
> > -			       git_stash_usage, options, argv[0]);
> > +	if (fn)
> > +		return !!fn(argc, argv, prefix);
> > +	else if (!argc)
> > +		return !!push_stash_unassumed(0, NULL, prefix);
> >  
> >  	/* Assume 'stash push' */
> >  	strvec_push(&args, "push");
> > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> > index 2a4c3fd61c..376cc8f4ab 100755
> > --- a/t/t3903-stash.sh
> > +++ b/t/t3903-stash.sh
> > @@ -25,7 +25,7 @@ test_expect_success 'usage on main command -h emits a summary of subcommands' '
> >  	grep -F "or: git stash show" usage
> >  '
> >  
> > -test_expect_failure 'usage for subcommands should emit subcommand usage' '
> > +test_expect_success 'usage for subcommands should emit subcommand usage' '
> >  	test_expect_code 129 git stash push -h >usage &&
> >  	grep -F "usage: git stash [push" usage
> >  '
> 
> This isn't on you, but I found the control flow here really confusing. I
> wonder if we could this cleanup before/squashed in. I may have missed
> some cases, but it passes all tests.

I think it would be better to leave this for later. I did notice that something is fishy around this 'force_assume' flag (argv[0] can never possibly be "-p") added in 8c3713cede (stash: eliminate crude option parsing, 2020-02-17), and even after your patch some of that fishiness remains (e.g. argv[0] can't possibly be "--" either). Unfortunately, the commit message doesn't explains what was intended, and I couldn't spend time on figuring it out, either.

Show 110 quoted lines
> I.e. the whole business of pushing this "did we assume?" around goes
> away if we simply pass the command-line as-is to push_stash(), and ask
> it to determine this.
> 
> diff --git a/builtin/stash.c b/builtin/stash.c
> index 1ba24c11737..a588389d66b 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -1665,10 +1665,8 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
>  	return ret;
>  }
>  
> -static int push_stash(int argc, const char **argv, const char *prefix,
> -		      int push_assumed)
> +static int push_stash(int argc, const char **argv, const char *prefix)
>  {
> -	int force_assume = 0;
>  	int keep_index = -1;
>  	int only_staged = 0;
>  	int patch_mode = 0;
> @@ -1696,20 +1694,22 @@ static int push_stash(int argc, const char **argv, const char *prefix,
>  		OPT_PATHSPEC_FILE_NUL(&pathspec_file_nul),
>  		OPT_END()
>  	};
> +	int push_assumed = argc == 1 || strcmp(argv[1], "push");
>  
> -	if (argc) {
> -		force_assume = !strcmp(argv[0], "-p");
> -		argc = parse_options(argc, argv, prefix, options,
> -				     push_assumed ? git_stash_usage :
> -				     git_stash_push_usage,
> -				     PARSE_OPT_KEEP_DASHDASH);
> +	if (!push_assumed) {
> +		argc--;
> +		argv++;
>  	}
>  
> +	argc = parse_options(argc, argv, prefix, options,
> +			     push_assumed ? git_stash_usage :
> +			     git_stash_push_usage,
> +			     PARSE_OPT_KEEP_DASHDASH);
>  	if (argc) {
>  		if (!strcmp(argv[0], "--")) {
>  			argc--;
>  			argv++;
> -		} else if (push_assumed && !force_assume) {
> +		} else if (push_assumed) {
>  			die("subcommand wasn't specified; 'push' can't be assumed due to unexpected token '%s'",
>  			    argv[0]);
>  		}
> @@ -1739,11 +1739,6 @@ static int push_stash(int argc, const char **argv, const char *prefix,
>  			     include_untracked, only_staged);
>  }
>  
> -static int push_stash_unassumed(int argc, const char **argv, const char *prefix)
> -{
> -	return push_stash(argc, argv, prefix, 0);
> -}
> -
>  static int save_stash(int argc, const char **argv, const char *prefix)
>  {
>  	int keep_index = -1;
> @@ -1791,8 +1786,7 @@ int cmd_stash(int argc, const char **argv, const char *prefix)
>  {
>  	pid_t pid = getpid();
>  	const char *index_file;
> -	struct strvec args = STRVEC_INIT;
> -	parse_opt_subcommand_fn *fn = NULL;
> +	parse_opt_subcommand_fn *fn = push_stash;
>  	struct option options[] = {
>  		OPT_SUBCOMMAND("apply", &fn, apply_stash),
>  		OPT_SUBCOMMAND("clear", &fn, clear_stash),
> @@ -1803,7 +1797,7 @@ int cmd_stash(int argc, const char **argv, const char *prefix)
>  		OPT_SUBCOMMAND("show", &fn, show_stash),
>  		OPT_SUBCOMMAND("store", &fn, store_stash),
>  		OPT_SUBCOMMAND("create", &fn, create_stash),
> -		OPT_SUBCOMMAND("push", &fn, push_stash_unassumed),
> +		OPT_SUBCOMMAND("push", &fn, push_stash),
>  		OPT_SUBCOMMAND_F("save", &fn, save_stash, PARSE_OPT_NOCOMPLETE),
>  		OPT_END()
>  	};
> @@ -1811,6 +1805,7 @@ int cmd_stash(int argc, const char **argv, const char *prefix)
>  	git_config(git_stash_config, NULL);
>  
>  	argc = parse_options(argc, argv, prefix, options, git_stash_usage,
> +			     PARSE_OPT_KEEP_ARGV0 |
>  			     PARSE_OPT_SUBCOMMAND_OPTIONAL |
>  			     PARSE_OPT_KEEP_UNKNOWN_OPT |
>  			     PARSE_OPT_KEEP_DASHDASH);
> @@ -1821,14 +1816,11 @@ int cmd_stash(int argc, const char **argv, const char *prefix)
>  	index_file = get_index_file();
>  	strbuf_addf(&stash_index_path, "%s.stash.%" PRIuMAX, index_file,
>  		    (uintmax_t)pid);
> +	
> +	if (fn != push_stash) {
> +		argc--;
> +		argv++;
> +	}
>  
> -	if (fn)
> -		return !!fn(argc, argv, prefix);
> -	else if (!argc)
> -		return !!push_stash_unassumed(0, NULL, prefix);
> -
> -	/* Assume 'stash push' */
> -	strvec_push(&args, "push");
> -	strvec_pushv(&args, argv);
> -	return !!push_stash(args.nr, args.v, prefix, 1);
> +	return !!fn(argc, argv, prefix);
>  }
Previous: Ævar Arnfjörð BjarmasonNext: SZEDER Gábor
Message 84 of 97 in “parse-options: handle subcommands”
  1. 00/20 parse-options: handle subcommandsSZEDER Gábor, Jul 25, 2022
  2. 01/20 git.c: update NO_PARSEOPT markingsSZEDER Gábor, Jul 25, 2022
  3. Ævar Arnfjörð BjarmasonJul 25, 2022
  4. SZEDER GáborAug 2, 2022
  5. Junio C HamanoAug 2, 2022
  6. Ævar Arnfjörð BjarmasonAug 3, 2022
  7. SZEDER GáborAug 3, 2022
  8. Ævar Arnfjörð BjarmasonAug 4, 2022
  9. Junio C HamanoAug 11, 2022
  10. SZEDER GáborAug 12, 2022
  11. Junio C HamanoAug 12, 2022
  12. SZEDER GáborJul 26, 2022
  13. 02/20 t3301-notes.sh: check that default operation mode doesn't take argumentsSZEDER Gábor, Jul 25, 2022
  14. 03/20 t5505-remote.sh: check the behavior without a subcommandSZEDER Gábor, Jul 25, 2022
  15. Ævar Arnfjörð BjarmasonJul 25, 2022
  16. 04/20 t0040-parse-options: test parse_options() with various 'parse_opt_flags'SZEDER Gábor, Jul 25, 2022
  17. Ævar Arnfjörð BjarmasonJul 25, 2022
  18. SZEDER GáborAug 12, 2022
  19. 05/20 api-parse-options.txt: fix description of OPT_CMDMODESZEDER Gábor, Jul 25, 2022
  20. 06/20 parse-options: PARSE_OPT_KEEP_UNKNOWN only applies to --optionsSZEDER Gábor, Jul 25, 2022
  21. 07/20 parse-options: clarify the limitations of PARSE_OPT_NODASHSZEDER Gábor, Jul 25, 2022
  22. 08/20 parse-options: drop leading space from '--git-completion-helper' outputSZEDER Gábor, Jul 25, 2022
  23. 10/20 builtin/bundle.c: let parse-options parse subcommandsSZEDER Gábor, Jul 25, 2022
  24. 09/20 parse-options: add support for parsing subcommandsSZEDER Gábor, Jul 25, 2022
  25. Ævar Arnfjörð BjarmasonJul 25, 2022
  26. SZEDER GáborJul 25, 2022
  27. Ævar Arnfjörð BjarmasonJul 25, 2022
  28. SZEDER GáborJul 25, 2022
  29. SZEDER GáborAug 12, 2022
  30. Junio C HamanoJul 25, 2022
  31. 11/20 builtin/commit-graph.c: let parse-options parse subcommandsSZEDER Gábor, Jul 25, 2022
  32. 12/20 builtin/gc.c: let parse-options parse 'git maintenance's subcommandsSZEDER Gábor, Jul 25, 2022
  33. 13/20 builtin/hook.c: let parse-option parse subcommandsSZEDER Gábor, Jul 25, 2022
  34. 15/20 builtin/notes.c: let parse-options parse subcommandsSZEDER Gábor, Jul 25, 2022
  35. Junio C HamanoJul 25, 2022
  36. 14/20 builtin/multi-pack-index.c: let parse-options parse subcommandsSZEDER Gábor, Jul 25, 2022
  37. 16/20 builtin/reflog.c: let parse-options parse subcommandsSZEDER Gábor, Jul 25, 2022
  38. 17/20 builtin/remote.c: let parse-options parse subcommandsSZEDER Gábor, Jul 25, 2022
  39. 18/20 builtin/sparse-checkout.c: let parse-options parse subcommandsSZEDER Gábor, Jul 25, 2022
  40. 19/20 builtin/stash.c: let parse-options parse subcommandsSZEDER Gábor, Jul 25, 2022
  41. 20/20 builtin/worktree.c: let parse-options parse subcommandsSZEDER Gábor, Jul 25, 2022
  42. Derrick StoleeJul 25, 2022
  43. SZEDER GáborJul 25, 2022
  44. Derrick StoleeJul 25, 2022
  45. Ævar Arnfjörð BjarmasonJul 25, 2022
  46. Junio C HamanoJul 25, 2022
  47. Johannes SchindelinJul 26, 2022
  48. Ævar Arnfjörð BjarmasonJul 26, 2022
  49. 00/20 parse-options: handle subcommandsSZEDER Gábor, Aug 19, 2022
  50. 09/20 parse-options: add support for parsing subcommandsSZEDER Gábor, Aug 19, 2022
  51. Ævar Arnfjörð BjarmasonAug 19, 2022
  52. Ævar Arnfjörð BjarmasonAug 19, 2022
  53. 15/20 builtin/notes.c: let parse-options parse subcommandsSZEDER Gábor, Aug 19, 2022
  54. Ævar Arnfjörð BjarmasonAug 19, 2022
  55. SZEDER GáborAug 21, 2022
  56. 17/20 builtin/remote.c: let parse-options parse subcommandsSZEDER Gábor, Aug 19, 2022
  57. 02/20 t3301-notes.sh: check that default operation mode doesn't take argumentsSZEDER Gábor, Aug 19, 2022
  58. 13/20 builtin/hook.c: let parse-options parse subcommandsSZEDER Gábor, Aug 19, 2022
  59. 10/20 builtin/bundle.c: let parse-options parse subcommandsSZEDER Gábor, Aug 19, 2022
  60. Ævar Arnfjörð BjarmasonAug 19, 2022
  61. 12/20 builtin/gc.c: let parse-options parse 'git maintenance's subcommandsSZEDER Gábor, Aug 19, 2022
  62. Junio C HamanoAug 19, 2022
  63. 16/20 builtin/reflog.c: let parse-options parse subcommandsSZEDER Gábor, Aug 19, 2022
  64. Ævar Arnfjörð BjarmasonAug 19, 2022
  65. 11/20 builtin/commit-graph.c: let parse-options parse subcommandsSZEDER Gábor, Aug 19, 2022
  66. Ævar Arnfjörð BjarmasonAug 19, 2022
  67. Ævar Arnfjörð BjarmasonAug 19, 2022
  68. SZEDER GáborAug 19, 2022
  69. 18/20 builtin/sparse-checkout.c: let parse-options parse subcommandsSZEDER Gábor, Aug 19, 2022
  70. 01/20 git.c: update NO_PARSEOPT markingsSZEDER Gábor, Aug 19, 2022
  71. 14/20 builtin/multi-pack-index.c: let parse-options parse subcommandsSZEDER Gábor, Aug 19, 2022
  72. Ævar Arnfjörð BjarmasonAug 19, 2022
  73. 04/20 t0040-parse-options: test parse_options() with various 'parse_opt_flags'SZEDER Gábor, Aug 19, 2022
  74. Ævar Arnfjörð BjarmasonAug 19, 2022
  75. SZEDER GáborAug 20, 2022
  76. Junio C HamanoAug 19, 2022
  77. SZEDER GáborAug 20, 2022
  78. Junio C HamanoAug 20, 2022
  79. 08/20 parse-options: drop leading space from '--git-completion-helper' outputSZEDER Gábor, Aug 19, 2022
  80. Ævar Arnfjörð BjarmasonAug 19, 2022
  81. SZEDER GáborAug 19, 2022
  82. 19/20 builtin/stash.c: let parse-options parse subcommandsSZEDER Gábor, Aug 19, 2022
  83. Ævar Arnfjörð BjarmasonAug 19, 2022
  84. SZEDER GáborAug 20, 2022
  85. 06/20 parse-options: PARSE_OPT_KEEP_UNKNOWN only applies to --optionsSZEDER Gábor, Aug 19, 2022
  86. 05/20 api-parse-options.txt: fix description of OPT_CMDMODESZEDER Gábor, Aug 19, 2022
  87. 20/20 builtin/worktree.c: let parse-options parse subcommandsSZEDER Gábor, Aug 19, 2022
  88. 07/20 parse-options: clarify the limitations of PARSE_OPT_NODASHSZEDER Gábor, Aug 19, 2022
  89. 03/20 t5505-remote.sh: check the behavior without a subcommandSZEDER Gábor, Aug 19, 2022
  90. 0/5 parse-options: minor cleanups for handling subcommandsSZEDER Gábor, Sep 5, 2022
  91. 2/5 test-parse-options.c: don't use for loop initial declarationSZEDER Gábor, Sep 5, 2022
  92. 1/5 t0040-parse-options: remove leftover debuggingSZEDER Gábor, Sep 5, 2022
  93. 3/5 test-parse-options.c: fix style of comparison with zeroSZEDER Gábor, Sep 5, 2022
  94. 4/5 notes: simplify default operation mode arguments checkSZEDER Gábor, Sep 5, 2022
  95. 5/5 notes, remote: show unknown subcommands between `'SZEDER Gábor, Sep 5, 2022
  96. Junio C HamanoSep 7, 2022
  97. SZEDER GáborSep 7, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.