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

Re: [PATCH v5] stash: assume "push" when command line starts with an option

From
PWPhillip Wood <phillip.wood123@gmail.com>
Date
Apr 21, 2026, 15:28 UTC
Message-ID
<980af898-5a56-4bc1-9222-74ceb9de9fac@gmail.com>
In-Reply-To
<20260419165453.32593-1-deveshigurgaon@gmail.com>
On 19/04/2026 17:54, Deveshi Dwivedi wrote:

This has changed more than I was expecting it to, but I think the approach of checking if argv[0] starts with '-' in cmd_stash() makes sense. I've left a few comments below.

Show 33 quoted lines
> diff --git a/builtin/stash.c b/builtin/stash.c
> index 95c5005b0b..bf04cf58a6 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -1831,9 +1831,8 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
>   }
>   
>   static int push_stash(int argc, const char **argv, const char *prefix,
> -		      int push_assumed)
> +		      const char * const usage[])
>   {
> -	int force_assume = 0;
>   	int keep_index = -1;
>   	int only_staged = 0;
>   	int patch_mode = 0;
> @@ -1868,26 +1867,14 @@ static int push_stash(int argc, const char **argv, const char *prefix,
>   	};
>   	int ret;
>   
> -	if (argc) {
> -		int flags = PARSE_OPT_KEEP_DASHDASH;
> -
> -		if (push_assumed)
> -			flags |= PARSE_OPT_STOP_AT_NON_OPTION;
> -
> +	if (argc)
>   		argc = parse_options(argc, argv, prefix, options,
> -				     push_assumed ? git_stash_usage :
> -				     git_stash_push_usage, flags);
> -		force_assume |= patch_mode;
> -	}
> +				     usage,
> +				     PARSE_OPT_KEEP_DASHDASH);

As all we do with '--' is remove it now that we don't need to check if it was passed, there is not much point in asking parse_options() to keep it for us. You can just pass '0' here and drop the if statement below.

Show 41 quoted lines
>   
> -	if (argc) {
> -		if (!strcmp(argv[0], "--")) {
> -			argc--;
> -			argv++;
> -		} else if (push_assumed && !force_assume) {
> -			die("subcommand wasn't specified; 'push' can't be assumed due to unexpected token '%s'",
> -			    argv[0]);
> -		}
> +	if (argc && !strcmp(argv[0], "--")) {
> +		argc--;
> +		argv++;
>   	}
>   
>   	parse_pathspec(&ps, 0, PATHSPEC_PREFER_FULL | PATHSPEC_PREFIX_ORIGIN,
> @@ -1935,7 +1922,7 @@ static int push_stash(int argc, const char **argv, const char *prefix,
>   static int push_stash_unassumed(int argc, const char **argv, const char *prefix,
>   				struct repository *repo UNUSED)
>   {
> -	return push_stash(argc, argv, prefix, 0);
> +	return push_stash(argc, argv, prefix, git_stash_push_usage);
>   }
>   
>   static int save_stash(int argc, const char **argv, const char *prefix,
> @@ -2387,7 +2374,6 @@ int cmd_stash(int argc,
>   {
>   	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),
> @@ -2427,19 +2413,22 @@ int cmd_stash(int argc,
>   	else if (!argc)
>   		return !!push_stash_unassumed(0, NULL, prefix, repo);
>   
> -	/* Assume 'stash push' */
> -	strvec_push(&args, "push");
> -	strvec_pushv(&args, argv);> +	if (argv[0][0] != '-')
> +		die("subcommand wasn't specified; 'push' can't be assumed due to unexpected token '%s'",
> +		    argv[0]);

If there's no option then we die straight away which makes sense. Part of me wonders if we should also show the usage for "git stash" in case the user misspelled a subcommand name. The other part of me finds the way git is so keen to print reams of usage at the slightest provocation quite annoying, but at least the "git stash" usage is fairly small. Looking at the history we used to complain about an unknown subcommand but that was changed without any explanation in 8c3713cede (stash: eliminate crude option parsing, 2020-02-17)

Show 9 quoted lines
>   	/*
> -	 * `push_stash()` ends up modifying the array, which causes memory
> -	 * leaks if we didn't copy the array here.
> +	 * When the command line starts with an option, assume 'push'.
> +	 * Unshift "push" into argv so that parse_options() skips it
> +	 * as the subcommand name.  Use git_stash_usage so that invalid
> +	 * options show the general stash usage rather than the
> +	 * push-specific usage.
>   	 */

I'm not really sure if this change to the usage is an improvement or not. If we document that an option without a subcommand means "push" then isn't it confusing to show the usage for "git stash" rather than "git stash push"?

Show 9 quoted lines
> -	DUP_ARRAY(args_copy, args.v, args.nr);
> -
> -	ret = !!push_stash(args.nr, args_copy, prefix, 1);
> -
> -	strvec_clear(&args);
> +	ALLOC_ARRAY(args_copy, argc + 1);
> +	args_copy[0] = "push";
> +	memcpy(&args_copy[1], argv, argc * sizeof(const char *));
> +	argc++;

Taking a shallow copy avoids the memory leak mentioned in the comment you edited above. That might be worth doing but it should be done as a separate preparatory step because it is orthogonal to the other changes here. We should use COPY_ARRAY() rather than memcpy() and we should also allocate 'argc + 2' and copy 'argc + 1' elements to keep the array NULL terminated as it was prior to 2e875b6cb4 (builtin/stash: fix various trivial memory leaks, 2024-08-01).

Thanks
Phillip
Show 47 quoted lines
> +	ret = !!push_stash(argc, args_copy, prefix, git_stash_usage);
>   	free(args_copy);
>   	return ret;
>   }
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index 70879941c2..836cc29a6b 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -410,8 +410,34 @@ test_expect_success 'stash --staged with binary file' '
>   '
>   
>   test_expect_success 'dont assume push with non-option args' '
> -	test_must_fail git stash -q drop 2>err &&
> -	test_grep -e "subcommand wasn'\''t specified; '\''push'\'' can'\''t be assumed due to unexpected token '\''drop'\''" err
> +	test_must_fail git stash someunknown 2>err &&
> +	test_grep "subcommand wasn${SQ}t specified; ${SQ}push${SQ} can${SQ}t be assumed due to unexpected token ${SQ}someunknown${SQ}" err
> +'
> +
> +test_expect_success 'assume push when command line starts with option' '
> +	test_when_finished "git reset --hard" &&
> +	test_when_finished "rm -f untracked-file" &&
> +	echo changed >file &&
> +	git add file &&
> +	git stash -m "implied push" file &&
> +	git stash pop &&
> +
> +	git add file &&
> +	git stash --staged file &&
> +	git stash pop &&
> +
> +	git add file &&
> +	git stash --keep-index file &&
> +	git stash pop &&
> +
> +	git add file &&
> +	git stash --no-keep-index file &&
> +	git stash pop &&
> +
> +	echo untracked >untracked-file &&
> +	git stash --include-untracked untracked-file &&
> +	test_path_is_missing untracked-file &&
> +	git stash pop
>   '
>   
>   test_expect_success 'stash --invalid-option' '
> 
> base-commit: 2855562ca6a9c6b0e7bc780b050c1e83c9fcfbd0
Previous: Deveshi Dwivedi
Message 17 of 17 in “stash: infer "push" when push-specific options are given”
  1. stash: infer "push" when push-specific options are givenDeveshi Dwivedi, Apr 4, 2026
  2. Mirko FainaApr 4, 2026
  3. stash: infer "push" when push-specific options are givenDeveshi Dwivedi, Apr 4, 2026
  4. Mirko FainaApr 4, 2026
  5. Deveshi DwivediApr 5, 2026
  6. stash: infer "push" when push-specific options are givenDeveshi Dwivedi, Apr 5, 2026
  7. Mirko FainaApr 6, 2026
  8. Phillip WoodApr 7, 2026
  9. Deveshi DwivediApr 9, 2026
  10. Mirko FainaApr 9, 2026
  11. Junio C HamanoApr 9, 2026
  12. Junio C HamanoApr 9, 2026
  13. stash: infer "push" when command line starts with an optionDeveshi Dwivedi, Apr 12, 2026
  14. Phillip WoodApr 13, 2026
  15. Junio C HamanoApr 13, 2026
  16. stash: assume "push" when command line starts with an optionDeveshi Dwivedi, Apr 19, 2026
  17. Phillip WoodApr 21, 2026

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.