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

[Phillip Wood <phillip.wood123@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.

> 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.

>   
> -	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)

>   	/*
> -	 * `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"?

> -	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


> +	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