Re: [GSoC][PATCH v7 22/26] stash: convert save to builtin

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

On 08/08, Paul-Sebastian Ungureanu wrote:
> Add stash save to the helper and delete functions which are no
> longer needed (`show_help()`, `save_stash()`, `push_stash()`,
> `create_stash()`, `clear_stash()`, `untracked_files()` and
> `no_changes()`).
> ---
>  builtin/stash--helper.c |  48 +++++++
>  git-stash.sh            | 311 +---------------------------------------
>  2 files changed, 50 insertions(+), 309 deletions(-)
> 
> diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
> index 5c27f5dcf..f54a476e3 100644
> --- a/builtin/stash--helper.c
> +++ b/builtin/stash--helper.c
> @@ -26,6 +26,8 @@ static const char * const git_stash_helper_usage[] = {
>  	N_("git stash--helper [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n"
>  	   "          [-u|--include-untracked] [-a|--all] [-m|--message <message>]\n"
>  	   "          [--] [<pathspec>...]]"),
> +	N_("git stash--helper save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n"
> +	   "          [-u|--include-untracked] [-a|--all] [<message>]"),
>  	NULL
>  };
>  
> @@ -81,6 +83,12 @@ static const char * const git_stash_helper_push_usage[] = {
>  	NULL
>  };
>  
> +static const char * const git_stash_helper_save_usage[] = {
> +	N_("git stash--helper save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n"
> +	   "          [-u|--include-untracked] [-a|--all] [<message>]"),
> +	NULL
> +};
> +
>  static const char *ref_stash = "refs/stash";
>  static int quiet;
>  static struct strbuf stash_index_path = STRBUF_INIT;
> @@ -1445,6 +1453,44 @@ static int push_stash(int argc, const char **argv, const char *prefix)
>  			     include_untracked, quiet, stash_msg);
>  }
>  
> +static int save_stash(int argc, const char **argv, const char *prefix)
> +{
> +	int i;
> +	int keep_index = -1;
> +	int patch_mode = 0;
> +	int include_untracked = 0;
> +	int quiet = 0;
> +	char *stash_msg = NULL;
> +	struct strbuf alt_stash_msg = STRBUF_INIT;
> +	struct option options[] = {
> +		OPT_SET_INT('k', "keep-index", &keep_index,
> +			N_("keep index"), 1),
> +		OPT_BOOL('p', "patch", &patch_mode,
> +			N_("stash in patch mode")),
> +		OPT_BOOL('q', "quiet", &quiet,
> +			N_("quiet mode")),

This could be OPT__QUIET again as mentioned in the previous patch as
well.

> +		OPT_BOOL('u', "include-untracked", &include_untracked,
> +			 N_("include untracked files in stash")),
> +		OPT_SET_INT('a', "all", &include_untracked,
> +			    N_("include ignore files"), 2),
> +		OPT_STRING('m', "message", &stash_msg, N_("message"),
> +			 N_("stash message")),
> +		OPT_END()
> +	};
> +
> +	argc = parse_options(argc, argv, prefix, options,
> +			     git_stash_helper_save_usage,
> +			     0);
> +
> +	for (i = 0; i < argc; ++i)
> +		strbuf_addf(&alt_stash_msg, "%s ", argv[i]);
> +
> +	stash_msg = strbuf_detach(&alt_stash_msg, NULL);

We unconditionally overwrite 'stash_msg' here, even if a '-m'
parameter was given earlier, which I don't think is what we intended
here.  I think we started "supporting" (not erroring out on rather)
the '-m' flag accidentally (my bad, sorry) in 'git stash save' rather
than intentionally, and indeed it has the same behaviour as the code
above.

However I think we should just not add support the '-m' flag here, as
it doesn't make a lot of sense to have two ways of passing a message.

We also never free the memory we get back here from 'strbuf_detach'.
As this is not code in 'libgit.a' that's probably fine, and we can
just add an 'UNLEAK(stash_msg)' here I think.

It may generally be interesting to consider using a leak checker, and
see how far we can get in making this leak free.  It may not be
possible to make 'git stash' completely leak free, as the underlying
APIs may not be, but it may be interesting to see how far we can get
for the new code only.

> +
> +	return do_push_stash(0, NULL, prefix, keep_index, patch_mode,
> +			     include_untracked, quiet, stash_msg);
> +}
> +
>  int cmd_stash__helper(int argc, const char **argv, const char *prefix)
>  {
>  	pid_t pid = getpid();
> @@ -1485,6 +1531,8 @@ int cmd_stash__helper(int argc, const char **argv, const char *prefix)
>  		return !!create_stash(argc, argv, prefix);
>  	else if (!strcmp(argv[0], "push"))
>  		return !!push_stash(argc, argv, prefix);
> +	else if (!strcmp(argv[0], "save"))
> +		return !!save_stash(argc, argv, prefix);
>  
>  	usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]),
>  		      git_stash_helper_usage, options);
>
> [...]