Re: [GSoC][PATCH v8 18/20] stash: convert `stash--helper.c` into `stash.c`

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Paul,

On Fri, 31 Aug 2018, Paul-Sebastian Ungureanu wrote:

> diff --git a/builtin/stash--helper.c b/builtin/stash.c
> similarity index 90%
> rename from builtin/stash--helper.c
> rename to builtin/stash.c
> index 1269f2548c..3d2316e3f7 100644
> --- a/builtin/stash--helper.c
> +++ b/builtin/stash.c
> @@ -1198,34 +1198,39 @@ static int do_create_stash(struct pathspec ps, const char **stash_msg,
>  
>  static int create_stash(int argc, const char **argv, const char *prefix)
>  {
> -	int include_untracked = 0;
> +	int i;
>  	int ret = 0;
> +	char *to_free = NULL;
>  	const char *stash_msg = NULL;
>  	struct stash_info info;
>  	struct pathspec ps;
> +	struct strbuf stash_msg_buf = STRBUF_INIT;
>  	struct option options[] = {
> -		OPT_BOOL('u', "include-untracked", &include_untracked,
> -			 N_("include untracked files in stash")),
> -		OPT_STRING('m', "message", &stash_msg, N_("message"),
> -			 N_("stash message")),

This was indeed a bit surprising, but after re-reading the commit message,
it became clear what this is all about.

But I wonder...

>  		OPT_END()
>  	};
>  
>  	argc = parse_options(argc, argv, prefix, options,
> -			     git_stash_helper_create_usage,
> +			     git_stash_create_usage,
>  			     0);

As we no longer accept any command-line arguments, (and as the `create`
command *explicitly* allows messages starting with a dash), it would
probably make more sense to test `argc` directly:

	if (!argc)
		usage_with_options(git_stash_create_usage, options);

>  
> +	for (i = 0; i < argc; ++i)
> +		strbuf_addf(&stash_msg_buf, "%s ", argv[i]);

... this adds an extra space at the end, right? I guess this should read
instead:

	for (i = 0; i < argc; i++) {
		if (i > 0)
			strbuf_addch(&stash_msg_buf, ' ');
		strbuf_addstr(&stash_msg_buf, argv[i]);
	}

> +	stash_msg = strbuf_detach(&stash_msg_buf, NULL);

As we no longer have any way to set `stash_msg` via an option, this
variable should go away.

And rather than detaching the strbuf here, it would be better to release
it after the call to `do_create_stash()`. (And the `to_free` variable is
no longer necessary, either.)

> +	to_free = (char *) stash_msg;
> +
>  	memset(&ps, 0, sizeof(ps));
> -	ret = do_create_stash(ps, &stash_msg, include_untracked, 0, &info, 0);
> +	ret = do_create_stash(ps, &stash_msg, 0, 0, &info, 0);
>  
>  	if (!ret)
>  		printf_ln("%s", oid_to_hex(&info.w_commit));
>  
> +	free(to_free);
> +	free((char *) stash_msg);
> +
>  	/*
>  	 * ret can be 1 if there were no changes. In this case, we should
>  	 * not error out.
>  	 */
> -	free((char *) stash_msg);
>  	return ret < 0;
>  }
>  
> @@ -1443,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);

Is this `if (argc)` guard necessary?

>  
>  	parse_pathspec(&ps, 0, PATHSPEC_PREFER_FULL, prefix, argv);
>  	return do_push_stash(ps, stash_msg, quiet, keep_index, patch_mode,
> @@ -1478,7 +1484,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)
> @@ -1494,10 +1500,12 @@ static int save_stash(int argc, const char **argv, const char *prefix)
>  	return ret;
>  }
>  
> -int cmd_stash__helper(int argc, const char **argv, const char *prefix)
> +int cmd_stash(int argc, const char **argv, const char *prefix)
>  {
> +	int i = -1;
>  	pid_t pid = getpid();
>  	const char *index_file;
> +	struct argv_array args = ARGV_ARRAY_INIT;
>  
>  	struct option options[] = {
>  		OPT_END()
> @@ -1505,16 +1513,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);
> @@ -1536,7 +1544,44 @@ 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);
> +	else if (*argv[0] != '-')
> +		usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]),
> +			      git_stash_usage, options);
> +
> +	if (strcmp(argv[0], "-p")) {
> +		while (++i < argc && strcmp(argv[i], "--")) {
> +			/*
> +			 * `akpqu` is a string which contains all short options,
> +			 * except `-m` which is verified separately.
> +			 */
> +			if ((strlen(argv[i]) == 2) && *argv[i] == '-' &&
> +			    strchr("akpqu", argv[i][1]))

I *think* this is missing the `n`.

Thanks,
Dscho

> +				continue;
> +
> +			if (!strcmp(argv[i], "--all") ||
> +			    !strcmp(argv[i], "--keep-index") ||
> +			    !strcmp(argv[i], "--no-keep-index") ||
> +			    !strcmp(argv[i], "--patch") ||
> +			    !strcmp(argv[i], "--quiet") ||
> +			    !strcmp(argv[i], "--include-untracked"))
> +				continue;
> +
> +			/*
> +			 * `-m` and `--message=` are verified separately because
> +			 * they need to be immediately followed by a string
> +			 * (i.e.`-m"foobar"` or `--message="foobar"`).
> +			 */
> +			if ((strlen(argv[i]) > 2 &&
> +			     !strncmp(argv[i], "-m", 2)) ||
> +			    (strlen(argv[i]) > 10 &&
> +			     !strncmp(argv[i], "--message=", 10)))
> +				continue;
> +
> +			usage_with_options(git_stash_usage, options);
> +		}
> +	}
>  
> -	usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]),
> -		      git_stash_helper_usage, options);
> +	argv_array_push(&args, "push");
> +	argv_array_pushv(&args, argv);
> +	return !!push_stash(args.argc, args.argv, prefix);
>  }
> diff --git a/git-stash.sh b/git-stash.sh
> deleted file mode 100755
> index 695f1feba3..0000000000
> --- a/git-stash.sh
> +++ /dev/null
> @@ -1,153 +0,0 @@
> -#!/bin/sh
> -# Copyright (c) 2007, Nanako Shiraishi
> -
> -dashless=$(basename "$0" | sed -e 's/-/ /')
> -USAGE="list [<options>]
> -   or: $dashless show [<stash>]
> -   or: $dashless drop [-q|--quiet] [<stash>]
> -   or: $dashless ( pop | apply ) [--index] [-q|--quiet] [<stash>]
> -   or: $dashless branch <branchname> [<stash>]
> -   or: $dashless save [--patch] [-k|--[no-]keep-index] [-q|--quiet]
> -		      [-u|--include-untracked] [-a|--all] [<message>]
> -   or: $dashless [push [--patch] [-k|--[no-]keep-index] [-q|--quiet]
> -		       [-u|--include-untracked] [-a|--all] [-m <message>]
> -		       [-- <pathspec>...]]
> -   or: $dashless clear"
> -
> -SUBDIRECTORY_OK=Yes
> -OPTIONS_SPEC=
> -START_DIR=$(pwd)
> -. git-sh-setup
> -require_work_tree
> -prefix=$(git rev-parse --show-prefix) || exit 1
> -cd_to_toplevel
> -
> -TMP="$GIT_DIR/.git-stash.$$"
> -TMPindex=${GIT_INDEX_FILE-"$(git rev-parse --git-path index)"}.stash.$$
> -trap 'rm -f "$TMP-"* "$TMPindex"' 0
> -
> -ref_stash=refs/stash
> -
> -if git config --get-colorbool color.interactive; then
> -       help_color="$(git config --get-color color.interactive.help 'red bold')"
> -       reset_color="$(git config --get-color '' reset)"
> -else
> -       help_color=
> -       reset_color=
> -fi
> -
> -#
> -# Parses the remaining options looking for flags and
> -# at most one revision defaulting to ${ref_stash}@{0}
> -# if none found.
> -#
> -# Derives related tree and commit objects from the
> -# revision, if one is found.
> -#
> -# stash records the work tree, and is a merge between the
> -# base commit (first parent) and the index tree (second parent).
> -#
> -#   REV is set to the symbolic version of the specified stash-like commit
> -#   IS_STASH_LIKE is non-blank if ${REV} looks like a stash
> -#   IS_STASH_REF is non-blank if the ${REV} looks like a stash ref
> -#   s is set to the SHA1 of the stash commit
> -#   w_commit is set to the commit containing the working tree
> -#   b_commit is set to the base commit
> -#   i_commit is set to the commit containing the index tree
> -#   u_commit is set to the commit containing the untracked files tree
> -#   w_tree is set to the working tree
> -#   b_tree is set to the base tree
> -#   i_tree is set to the index tree
> -#   u_tree is set to the untracked files tree
> -#
> -#   GIT_QUIET is set to t if -q is specified
> -#   INDEX_OPTION is set to --index if --index is specified.
> -#   FLAGS is set to the remaining flags (if allowed)
> -#
> -# dies if:
> -#   * too many revisions specified
> -#   * no revision is specified and there is no stash stack
> -#   * a revision is specified which cannot be resolve to a SHA1
> -#   * a non-existent stash reference is specified
> -#   * unknown flags were set and ALLOW_UNKNOWN_FLAGS is not "t"
> -#
> -
> -test "$1" = "-p" && set "push" "$@"
> -
> -PARSE_CACHE='--not-parsed'
> -# The default command is "push" if nothing but options are given
> -seen_non_option=
> -for opt
> -do
> -	case "$opt" in
> -	--) break ;;
> -	-*) ;;
> -	*) seen_non_option=t; break ;;
> -	esac
> -done
> -
> -test -n "$seen_non_option" || set "push" "$@"
> -
> -# Main command set
> -case "$1" in
> -list)
> -	shift
> -	git stash--helper list "$@"
> -	;;
> -show)
> -	shift
> -	git stash--helper show "$@"
> -	;;
> -save)
> -	shift
> -	cd "$START_DIR"
> -	git stash--helper save "$@"
> -	;;
> -push)
> -	shift
> -	cd "$START_DIR"
> -	git stash--helper push "$@"
> -	;;
> -apply)
> -	shift
> -	cd "$START_DIR"
> -	git stash--helper apply "$@"
> -	;;
> -clear)
> -	shift
> -	git stash--helper clear "$@"
> -	;;
> -create)
> -	shift
> -	git stash--helper create --message "$*"
> -	;;
> -store)
> -	shift
> -	git stash--helper store "$@"
> -	;;
> -drop)
> -	shift
> -	git stash--helper drop "$@"
> -	;;
> -pop)
> -	shift
> -	cd "$START_DIR"
> -	git stash--helper pop "$@"
> -	;;
> -branch)
> -	shift
> -	cd "$START_DIR"
> -	git stash--helper branch "$@"
> -	;;
> -*)
> -	case $# in
> -	0)
> -		cd "$START_DIR"
> -		git stash--helper push &&
> -		say "$(gettext "(To restore them type \"git stash apply\")")"
> -		;;
> -	*)
> -		usage
> -	esac
> -	;;
> -esac
> diff --git a/git.c b/git.c
> index 3c0e762d7d..78548397cf 100644
> --- a/git.c
> +++ b/git.c
> @@ -544,7 +544,7 @@ static struct cmd_struct commands[] = {
>  	{ "show-index", cmd_show_index },
>  	{ "show-ref", cmd_show_ref, RUN_SETUP },
>  	{ "stage", cmd_add, RUN_SETUP | NEED_WORK_TREE },
> -	{ "stash--helper", cmd_stash__helper, RUN_SETUP | NEED_WORK_TREE },
> +	{ "stash", cmd_stash, RUN_SETUP | NEED_WORK_TREE },
>  	{ "status", cmd_status, RUN_SETUP | NEED_WORK_TREE },
>  	{ "stripspace", cmd_stripspace },
>  	{ "submodule--helper", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX | NO_PARSEOPT },
> -- 
> 2.19.0.rc0.22.gc26283d74e
> 
>