Re: [GSoC][PATCH v8 17/20] stash: convert save to builtin

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

Hi Paul,

On Fri, 31 Aug 2018, 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()`).
> 
> The `-m` option is no longer supported as it might not make
> sense to have two ways of passing a message. Even if this is
> a change in behaviour, the documentation remains the same
> because the `-m` parameter was omitted before.

It makes me slightly nervous to remove the `-m` option like this. And I
wonder how difficult it would be to allow for the `-m` option, still, just
to keep the occasional script running that might rely on this feature.

I could imagine that it would be as easy as

	const char *stash_msg = NULL;
	[...]
		OPT_STRING('m', "message", &stash_msg, "message",
			   N_("stash message")),
	[...]
	struct strbuf buf = STRBUF_INIT;

	[... parse_options() ...]
	if (argc)
		stash_msg = strbuf_join_argv(&buf, argc, argv, ' ');

	[...]

	strbuf_release(&buf);

where `strbuf_join_argv()` would be implemented like this (and I would put
it into strbuf.c and strbuf.h:

	const char *strbuf_join_argv(struct strbuf *buf,
				     int argc, const char **argv, char delim)
	{
		if (!argc)
			return buf->buf;

		strbuf_addstr(buf, *argv);
		while (--i) {
			strbuf_addch(buf, delim);
			strbuf_addstr(buf, *(++argv);
		}

		return buf->buf;
	}

> diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
> index e5153a63ea..1269f2548c 100644
> --- a/builtin/stash--helper.c
> +++ b/builtin/stash--helper.c
> @@ -1444,6 +1452,48 @@ static int push_stash(int argc, const char **argv, const char *prefix)
>  			     include_untracked);
>  }
>  
> +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;
> +	int ret = 0;
> +	const char *stash_msg = NULL;
> +	char *to_free = NULL;
> +	struct strbuf stash_msg_buf = STRBUF_INIT;
> +	struct pathspec ps;
> +	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__QUIET(&quiet, N_("quiet mode")),
> +		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_END()
> +	};
> +
> +	argc = parse_options(argc, argv, prefix, options,
> +			     git_stash_helper_save_usage,
> +			     0);
> +
> +	for (i = 0; i < argc; ++i)
> +		strbuf_addf(&stash_msg_buf, "%s ", argv[i]);

I think I saw this in another patch already, and now it makes sense to
introduce the `strbuf_join_argv()` function, methinks.

> +	stash_msg = strbuf_detach(&stash_msg_buf, NULL);
> +	to_free = (char *) stash_msg;
> +
> +	memset(&ps, 0, sizeof(ps));
> +	ret = do_push_stash(ps, stash_msg, quiet, keep_index, patch_mode,
> +			    include_untracked);
> +
> +	free(to_free);

As before, I would recommend to use stash_msg_buf.buf directly and call
strbuf_release() in the end (and drop the `to_free` variable altogether).

The rest looks good to me!

Thanks,
Dscho

> +	return ret;
> +}
> +
>  int cmd_stash__helper(int argc, const char **argv, const char *prefix)
>  {
>  	pid_t pid = getpid();
> @@ -1484,6 +1534,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);
> diff --git a/git-stash.sh b/git-stash.sh
> index c3146f62ab..695f1feba3 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -36,314 +36,6 @@ else
>         reset_color=
>  fi
>  
> -no_changes () {
> -	git diff-index --quiet --cached HEAD --ignore-submodules -- "$@" &&
> -	git diff-files --quiet --ignore-submodules -- "$@" &&
> -	(test -z "$untracked" || test -z "$(untracked_files "$@")")
> -}
> -
> -untracked_files () {
> -	if test "$1" = "-z"
> -	then
> -		shift
> -		z=-z
> -	else
> -		z=
> -	fi
> -	excl_opt=--exclude-standard
> -	test "$untracked" = "all" && excl_opt=
> -	git ls-files -o $z $excl_opt -- "$@"
> -}
> -
> -clear_stash () {
> -	if test $# != 0
> -	then
> -		die "$(gettext "git stash clear with parameters is unimplemented")"
> -	fi
> -	if current=$(git rev-parse --verify --quiet $ref_stash)
> -	then
> -		git update-ref -d $ref_stash $current
> -	fi
> -}
> -
> -create_stash () {
> -	stash_msg=
> -	untracked=
> -	while test $# != 0
> -	do
> -		case "$1" in
> -		-m|--message)
> -			shift
> -			stash_msg=${1?"BUG: create_stash () -m requires an argument"}
> -			;;
> -		-m*)
> -			stash_msg=${1#-m}
> -			;;
> -		--message=*)
> -			stash_msg=${1#--message=}
> -			;;
> -		-u|--include-untracked)
> -			shift
> -			untracked=${1?"BUG: create_stash () -u requires an argument"}
> -			;;
> -		--)
> -			shift
> -			break
> -			;;
> -		esac
> -		shift
> -	done
> -
> -	git update-index -q --refresh
> -	if no_changes "$@"
> -	then
> -		exit 0
> -	fi
> -
> -	# state of the base commit
> -	if b_commit=$(git rev-parse --verify HEAD)
> -	then
> -		head=$(git rev-list --oneline -n 1 HEAD --)
> -	else
> -		die "$(gettext "You do not have the initial commit yet")"
> -	fi
> -
> -	if branch=$(git symbolic-ref -q HEAD)
> -	then
> -		branch=${branch#refs/heads/}
> -	else
> -		branch='(no branch)'
> -	fi
> -	msg=$(printf '%s: %s' "$branch" "$head")
> -
> -	# state of the index
> -	i_tree=$(git write-tree) &&
> -	i_commit=$(printf 'index on %s\n' "$msg" |
> -		git commit-tree $i_tree -p $b_commit) ||
> -		die "$(gettext "Cannot save the current index state")"
> -
> -	if test -n "$untracked"
> -	then
> -		# Untracked files are stored by themselves in a parentless commit, for
> -		# ease of unpacking later.
> -		u_commit=$(
> -			untracked_files -z "$@" | (
> -				GIT_INDEX_FILE="$TMPindex" &&
> -				export GIT_INDEX_FILE &&
> -				rm -f "$TMPindex" &&
> -				git update-index -z --add --remove --stdin &&
> -				u_tree=$(git write-tree) &&
> -				printf 'untracked files on %s\n' "$msg" | git commit-tree $u_tree  &&
> -				rm -f "$TMPindex"
> -		) ) || die "$(gettext "Cannot save the untracked files")"
> -
> -		untracked_commit_option="-p $u_commit";
> -	else
> -		untracked_commit_option=
> -	fi
> -
> -	if test -z "$patch_mode"
> -	then
> -
> -		# state of the working tree
> -		w_tree=$( (
> -			git read-tree --index-output="$TMPindex" -m $i_tree &&
> -			GIT_INDEX_FILE="$TMPindex" &&
> -			export GIT_INDEX_FILE &&
> -			git diff-index --name-only -z HEAD -- "$@" >"$TMP-stagenames" &&
> -			git update-index -z --add --remove --stdin <"$TMP-stagenames" &&
> -			git write-tree &&
> -			rm -f "$TMPindex"
> -		) ) ||
> -			die "$(gettext "Cannot save the current worktree state")"
> -
> -	else
> -
> -		rm -f "$TMP-index" &&
> -		GIT_INDEX_FILE="$TMP-index" git read-tree HEAD &&
> -
> -		# find out what the user wants
> -		GIT_INDEX_FILE="$TMP-index" \
> -			git add--interactive --patch=stash -- "$@" &&
> -
> -		# state of the working tree
> -		w_tree=$(GIT_INDEX_FILE="$TMP-index" git write-tree) ||
> -		die "$(gettext "Cannot save the current worktree state")"
> -
> -		git diff-tree -p HEAD $w_tree -- >"$TMP-patch" &&
> -		test -s "$TMP-patch" ||
> -		die "$(gettext "No changes selected")"
> -
> -		rm -f "$TMP-index" ||
> -		die "$(gettext "Cannot remove temporary index (can't happen)")"
> -
> -	fi
> -
> -	# create the stash
> -	if test -z "$stash_msg"
> -	then
> -		stash_msg=$(printf 'WIP on %s' "$msg")
> -	else
> -		stash_msg=$(printf 'On %s: %s' "$branch" "$stash_msg")
> -	fi
> -	w_commit=$(printf '%s\n' "$stash_msg" |
> -	git commit-tree $w_tree -p $b_commit -p $i_commit $untracked_commit_option) ||
> -	die "$(gettext "Cannot record working tree state")"
> -}
> -
> -push_stash () {
> -	keep_index=
> -	patch_mode=
> -	untracked=
> -	stash_msg=
> -	while test $# != 0
> -	do
> -		case "$1" in
> -		-k|--keep-index)
> -			keep_index=t
> -			;;
> -		--no-keep-index)
> -			keep_index=n
> -			;;
> -		-p|--patch)
> -			patch_mode=t
> -			# only default to keep if we don't already have an override
> -			test -z "$keep_index" && keep_index=t
> -			;;
> -		-q|--quiet)
> -			GIT_QUIET=t
> -			;;
> -		-u|--include-untracked)
> -			untracked=untracked
> -			;;
> -		-a|--all)
> -			untracked=all
> -			;;
> -		-m|--message)
> -			shift
> -			test -z ${1+x} && usage
> -			stash_msg=$1
> -			;;
> -		-m*)
> -			stash_msg=${1#-m}
> -			;;
> -		--message=*)
> -			stash_msg=${1#--message=}
> -			;;
> -		--help)
> -			show_help
> -			;;
> -		--)
> -			shift
> -			break
> -			;;
> -		-*)
> -			option="$1"
> -			eval_gettextln "error: unknown option for 'stash push': \$option"
> -			usage
> -			;;
> -		*)
> -			break
> -			;;
> -		esac
> -		shift
> -	done
> -
> -	eval "set $(git rev-parse --sq --prefix "$prefix" -- "$@")"
> -
> -	if test -n "$patch_mode" && test -n "$untracked"
> -	then
> -		die "$(gettext "Can't use --patch and --include-untracked or --all at the same time")"
> -	fi
> -
> -	test -n "$untracked" || git ls-files --error-unmatch -- "$@" >/dev/null || exit 1
> -
> -	git update-index -q --refresh
> -	if no_changes "$@"
> -	then
> -		say "$(gettext "No local changes to save")"
> -		exit 0
> -	fi
> -
> -	git reflog exists $ref_stash ||
> -		clear_stash || die "$(gettext "Cannot initialize stash")"
> -
> -	create_stash -m "$stash_msg" -u "$untracked" -- "$@"
> -	git stash--helper store -m "$stash_msg" -q $w_commit ||
> -	die "$(gettext "Cannot save the current status")"
> -	say "$(eval_gettext "Saved working directory and index state \$stash_msg")"
> -
> -	if test -z "$patch_mode"
> -	then
> -		test "$untracked" = "all" && CLEAN_X_OPTION=-x || CLEAN_X_OPTION=
> -		if test -n "$untracked" && test $# = 0
> -		then
> -			git clean --force --quiet -d $CLEAN_X_OPTION
> -		fi
> -
> -		if test $# != 0
> -		then
> -			test -z "$untracked" && UPDATE_OPTION="-u" || UPDATE_OPTION=
> -			test "$untracked" = "all" && FORCE_OPTION="--force" || FORCE_OPTION=
> -			git add $UPDATE_OPTION $FORCE_OPTION -- "$@"
> -			git diff-index -p --cached --binary HEAD -- "$@" |
> -			git apply --index -R
> -		else
> -			git reset --hard -q
> -		fi
> -
> -		if test "$keep_index" = "t" && test -n "$i_tree"
> -		then
> -			git read-tree --reset $i_tree
> -			git ls-files -z --modified -- "$@" |
> -			git checkout-index -z --force --stdin
> -		fi
> -	else
> -		git apply -R < "$TMP-patch" ||
> -		die "$(gettext "Cannot remove worktree changes")"
> -
> -		if test "$keep_index" != "t"
> -		then
> -			git reset -q -- "$@"
> -		fi
> -	fi
> -}
> -
> -save_stash () {
> -	push_options=
> -	while test $# != 0
> -	do
> -		case "$1" in
> -		--)
> -			shift
> -			break
> -			;;
> -		-*)
> -			# pass all options through to push_stash
> -			push_options="$push_options $1"
> -			;;
> -		*)
> -			break
> -			;;
> -		esac
> -		shift
> -	done
> -
> -	stash_msg="$*"
> -
> -	if test -z "$stash_msg"
> -	then
> -		push_stash $push_options
> -	else
> -		push_stash $push_options -m "$stash_msg"
> -	fi
> -}
> -
> -show_help () {
> -	exec git help stash
> -	exit 1
> -}
> -
>  #
>  # Parses the remaining options looking for flags and
>  # at most one revision defaulting to ${ref_stash}@{0}
> @@ -408,7 +100,8 @@ show)
>  	;;
>  save)
>  	shift
> -	save_stash "$@"
> +	cd "$START_DIR"
> +	git stash--helper save "$@"
>  	;;
>  push)
>  	shift
> -- 
> 2.19.0.rc0.22.gc26283d74e
> 
>