Re: [GSoC][PATCH v8 13/20] stash: convert store to builtin

[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--helper.c
> index 02b593e0cd..87568b0f34 100644
> --- a/builtin/stash--helper.c
> +++ b/builtin/stash--helper.c
> @@ -723,6 +728,54 @@ static int show_stash(int argc, const char **argv, const char *prefix)
>  	return diff_result_code(&rev.diffopt, 0);
>  }
>  
> +static int do_store_stash(const char *w_commit, const char *stash_msg,
> +			  int quiet)
> +{
> +	int ret = 0;
> +	int need_to_free = 0;

Not worth sending another iteration, but if you end up doing so, I found
an alternative paradigm more useful: instead of having a Boolean to
indicate whether you need to free, have a `char *to_free` that is
initialized to `NULL`, then unconditionally release that:

	char *to_free = NULL;

	if (!stash_msg)
		stash_msg = to_free = xstrdup("Created via \"git stash store\".");

	[...]

	free(to_free);
	return ret;

This works because `free(NULL)` is defined as a no-op.

> +	struct object_id obj;
> +
> +	if (!stash_msg) {
> +		need_to_free = 1;
> +		stash_msg  = xstrdup("Created via \"git stash store\".");
> +	}
> +
> +	ret = get_oid(w_commit, &obj);

Is `get_oid()` non-quiet? If so, we might need to shut it up when the
`quiet` variable is non-zero. If it is quiet, we should probably mention
something here when `quiet` is zero and `ret` indicates an error with
`get_oid()`.

> +	if (!ret) {
> +		ret = update_ref(stash_msg, ref_stash, &obj, NULL,
> +				 REF_FORCE_CREATE_REFLOG,
> +				 quiet ? UPDATE_REFS_QUIET_ON_ERR :
> +				 UPDATE_REFS_MSG_ON_ERR);
> +	}
> +	if (ret && !quiet)
> +		fprintf_ln(stderr, _("Cannot update %s with %s"),
> +			   ref_stash, w_commit);
> +	if (need_to_free)
> +		free((char *) stash_msg);

Okay, so all we need `stash_msg` for is the `update_ref()` call? And the
fall-back message is constant. So how about

	if (!stash_msg)
		stash_msg = "Created via \"git stash store\".";

? No need to allocate/release memory at all...

> +	return ret;
> +}
> +
> +static int store_stash(int argc, const char **argv, const char *prefix)
> +{
> +	const char *stash_msg = NULL;
> +	struct option options[] = {
> +		OPT__QUIET(&quiet, N_("be quiet, only report errors")),
> +		OPT_STRING('m', "message", &stash_msg, "message", N_("stash message")),
> +		OPT_END()
> +	};
> +
> +	argc = parse_options(argc, argv, prefix, options,
> +			     git_stash_helper_store_usage,
> +			     PARSE_OPT_KEEP_UNKNOWN);
> +
> +	if (argc != 1) {
> +		fprintf_ln(stderr, _("\"git stash store\" requires one <commit> argument"));
> +		return -1;
> +	}
> +
> +	return do_store_stash(argv[0], stash_msg, quiet);
> +}

I guess `store_stash()` and `do_store_stash()` are separate functions to
discern between the higher-level function that parses the command-line,
and the lower-level function that is already libified?

If so:

1. I like it. Your code is already so much better prepared to serve as a proper
   internal API than even some Git old-timers'.

2. It might make sense to move the `get_oid()` call to `store_stash()`, as
   it is also parsing: it is parsing the plain-text representation of the
   revision into the internal representation (object ID).

Neither of these suggestions are blockers, though.

Thanks,
Dscho

>  int cmd_stash__helper(int argc, const char **argv, const char *prefix)
>  {
>  	pid_t pid = getpid();
> @@ -757,6 +810,8 @@ int cmd_stash__helper(int argc, const char **argv, const char *prefix)
>  		return !!list_stash(argc, argv, prefix);
>  	else if (!strcmp(argv[0], "show"))
>  		return !!show_stash(argc, argv, prefix);
> +	else if (!strcmp(argv[0], "store"))
> +		return !!store_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 0d05cbc1e5..5739c51527 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -191,45 +191,6 @@ create_stash () {
>  	die "$(gettext "Cannot record working tree state")"
>  }
>  
> -store_stash () {
> -	while test $# != 0
> -	do
> -		case "$1" in
> -		-m|--message)
> -			shift
> -			stash_msg="$1"
> -			;;
> -		-m*)
> -			stash_msg=${1#-m}
> -			;;
> -		--message=*)
> -			stash_msg=${1#--message=}
> -			;;
> -		-q|--quiet)
> -			quiet=t
> -			;;
> -		*)
> -			break
> -			;;
> -		esac
> -		shift
> -	done
> -	test $# = 1 ||
> -	die "$(eval_gettext "\"$dashless store\" requires one <commit> argument")"
> -
> -	w_commit="$1"
> -	if test -z "$stash_msg"
> -	then
> -		stash_msg="Created via \"git stash store\"."
> -	fi
> -
> -	git update-ref --create-reflog -m "$stash_msg" $ref_stash $w_commit
> -	ret=$?
> -	test $ret != 0 && test -z "$quiet" &&
> -	die "$(eval_gettext "Cannot update \$ref_stash with \$w_commit")"
> -	return $ret
> -}
> -
>  push_stash () {
>  	keep_index=
>  	patch_mode=
> @@ -308,7 +269,7 @@ push_stash () {
>  		clear_stash || die "$(gettext "Cannot initialize stash")"
>  
>  	create_stash -m "$stash_msg" -u "$untracked" -- "$@"
> -	store_stash -m "$stash_msg" -q $w_commit ||
> +	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")"
>  
> @@ -468,7 +429,7 @@ create)
>  	;;
>  store)
>  	shift
> -	store_stash "$@"
> +	git stash--helper store "$@"
>  	;;
>  drop)
>  	shift
> -- 
> 2.19.0.rc0.22.gc26283d74e
> 
>