Re: [GSoC][PATCH v7 23/26] stash: convert `stash--helper.c` into `stash.c`

[Thomas Gummerer <t.gummerer@gmail.com>]

On 08/08, Paul-Sebastian Ungureanu wrote:
> The old shell script `git-stash.sh`  was removed and replaced
> entirely by `builtin/stash.c`. In order to do that, `create` and
> `push` were adapted to work without `stash.sh`. For example, before
> this commit, `git stash create` called `git stash--helper create
> --message "$*"`. If it called `git stash--helper create "$@"`, then
> some of these changes wouldn't have been necessary.
> 
> This commit also removes the word `helper` since now stash is
> called directly and not by a shell script.
> ---
>  .gitignore                           |   1 -
>  Makefile                             |   3 +-
>  builtin.h                            |   2 +-
>  builtin/{stash--helper.c => stash.c} | 132 ++++++++++++-----------
>  git-stash.sh                         | 153 ---------------------------
>  git.c                                |   2 +-
>  6 files changed, 74 insertions(+), 219 deletions(-)
>  rename builtin/{stash--helper.c => stash.c} (91%)
>  delete mode 100755 git-stash.sh
> 
> diff --git a/builtin/stash--helper.c b/builtin/stash.c
> similarity index 91%
> rename from builtin/stash--helper.c
> rename to builtin/stash.c
> index f54a476e3..0ef88408a 100644
> --- a/builtin/stash--helper.c
> +++ b/builtin/stash.c
>
> [...]
>
> @@ -1445,9 +1448,10 @@ static int push_stash(int argc, const char **argv, const char *prefix)
>  		OPT_END()
>  	};
>  
> -	argc = parse_options(argc, argv, prefix, options,
> -			     git_stash_helper_push_usage,
> -			     0);
> +	if (argc)
> +		argc = parse_options(argc, argv, prefix, options,
> +				     git_stash_push_usage,
> +				     0);

This change is a bit surprising here.  Why is this necessary?  I
thought parse_options would handle no arguments just fine?

>  	return do_push_stash(argc, argv, prefix, keep_index, patch_mode,
>  			     include_untracked, quiet, stash_msg);
> @@ -1479,7 +1483,7 @@ static int save_stash(int argc, const char **argv, const char *prefix)
>  	};
>  
>  	argc = parse_options(argc, argv, prefix, options,
> -			     git_stash_helper_save_usage,
> +			     git_stash_save_usage,
>  			     0);
>  
>  	for (i = 0; i < argc; ++i)
> @@ -1491,7 +1495,7 @@ static int save_stash(int argc, const char **argv, const char *prefix)
>  			     include_untracked, quiet, stash_msg);
>  }
>  
> -int cmd_stash__helper(int argc, const char **argv, const char *prefix)
> +int cmd_stash(int argc, const char **argv, const char *prefix)
>  {
>  	pid_t pid = getpid();
>  	const char *index_file;
> @@ -1502,16 +1506,16 @@ int cmd_stash__helper(int argc, const char **argv, const char *prefix)
>  
>  	git_config(git_default_config, NULL);
>  
> -	argc = parse_options(argc, argv, prefix, options, git_stash_helper_usage,
> +	argc = parse_options(argc, argv, prefix, options, git_stash_usage,
>  			     PARSE_OPT_KEEP_UNKNOWN | PARSE_OPT_KEEP_DASHDASH);
>  
>  	index_file = get_index_file();
>  	strbuf_addf(&stash_index_path, "%s.stash.%" PRIuMAX, index_file,
>  		    (uintmax_t)pid);
>  
> -	if (argc < 1)
> -		usage_with_options(git_stash_helper_usage, options);
> -	if (!strcmp(argv[0], "apply"))
> +	if (argc == 0)
> +		return !!push_stash(0, NULL, prefix);
> +	else if (!strcmp(argv[0], "apply"))
>  		return !!apply_stash(argc, argv, prefix);
>  	else if (!strcmp(argv[0], "clear"))
>  		return !!clear_stash(argc, argv, prefix);
> @@ -1533,7 +1537,13 @@ int cmd_stash__helper(int argc, const char **argv, const char *prefix)
>  		return !!push_stash(argc, argv, prefix);
>  	else if (!strcmp(argv[0], "save"))
>  		return !!save_stash(argc, argv, prefix);
> +	if (*argv[0] == '-') {
> +		struct argv_array args = ARGV_ARRAY_INIT;
> +		argv_array_push(&args, "push");
> +		argv_array_pushv(&args, argv);
> +		return !!push_stash(args.argc, args.argv, prefix);
> +	}

This is a bit different than what the current code does.  Currently
the rules for when a plain 'git stash' becomes 'git stash push' are
the following:

- If there are no arguments.
- If all arguments are option arguments.
- If the first argument of 'git stash' is '-p'.
- If the first argument of 'git stash' is '--'.

This is to avoid someone typing 'git stash -q drop' for example, and
then being surprised that a new stash was created instead of an old
one being dropped, which what we have above would do.

For more reasoning about these aliasing rules see also the thread at [1].

[1]: https://public-inbox.org/git/20170213200950.m3bcyp52wd25p737@sigill.intra.peff.net/

>  
>  	usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]),
> -		      git_stash_helper_usage, options);
> +		      git_stash_usage, options);
>  }
> diff --git a/git-stash.sh b/git-stash.sh
> deleted file mode 100755
> index 695f1feba..000000000
> --- a/git-stash.sh
> +++ /dev/null
> @@ -1,153 +0,0 @@
>
> [...]