Web lists-archives.com

Re: [PATCH v5 4/5] stash: convert branch to builtin




Hi Joel,

On Wed, 4 Apr 2018, Joel Teichroeb wrote:

> diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
> index 520cd746c4..486796bb6a 100644
> --- a/builtin/stash--helper.c
> +++ b/builtin/stash--helper.c
> @@ -502,6 +508,49 @@ static int drop_stash(int argc, const char **argv, const char *prefix)
>  	return ret;
>  }
>  
> +static int branch_stash(int argc, const char **argv, const char *prefix)
> +{
> +	const char *branch = NULL;
> +	int ret;
> +	struct argv_array args = ARGV_ARRAY_INIT;
> +	struct stash_info info;
> +	struct option options[] = {
> +		OPT_END()
> +	};
> +
> +	argc = parse_options(argc, argv, prefix, options,
> +			git_stash_helper_branch_usage, 0);
> +
> +	if (argc == 0)
> +		return error(_("No branch name specified"));

We should probably also test for argc > 2 and error out in that case...

> +
> +	branch = argv[0];
> +
> +	if (get_stash_info(&info, argc - 1, argv + 1))
> +		return -1;
> +
> +	/* Checkout does not currently provide a function for checking out a branch
> +	 * as cmd_checkout does a large amount of sanity checks first that we
> +	 * require here.
> +	 */

While this is true, this code comment is *prone* to become stale. Maybe
move this remark into the commit message?

> +	argv_array_pushl(&args, "checkout", "-b", NULL);
> +	argv_array_push(&args, branch);
> +	argv_array_push(&args, oid_to_hex(&info.b_commit));
> +	ret = cmd_checkout(args.argc, args.argv, prefix);
> +	if (ret) {
> +		free_stash_info(&info);
> +		return -1;
> +	}
> +
> +	ret = do_apply_stash(prefix, &info, 1);
> +	if (!ret && info.is_stash_ref)
> +		ret = do_drop_stash(prefix, &info);

An alternative way to translate that &&-chain would be to do this:

	ret = cmd_checkout(args.argc, args.argv, prefix);
	if (!ret)
		ret = do_apply_stash(prefix, &info, 1);
	if (!ret && info.is_stash_ref)
		ret = do_drop_stash(prefix, &info);

... which might be a bit easier to read and to maintain?

> +
> +	free_stash_info(&info);
> +
> +	return ret;
> +}
> +
>  int cmd_stash__helper(int argc, const char **argv, const char *prefix)
>  {
>  	int result = 0;
> @@ -528,6 +577,8 @@ int cmd_stash__helper(int argc, const char **argv, const char *prefix)
>  		result = clear_stash(argc, argv, prefix);
>  	else if (!strcmp(argv[0], "drop"))
>  		result = drop_stash(argc, argv, prefix);
> +	else if (!strcmp(argv[0], "branch"))
> +		result = branch_stash(argc, argv, prefix);
>  	else {
>  		error(_("unknown subcommand: %s"), argv[0]);
>  		usage_with_options(git_stash_helper_usage, options);
> diff --git a/git-stash.sh b/git-stash.sh
> index 0b8f07b38a..c5fd4c6c44 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -598,20 +598,6 @@ drop_stash () {
>  	clear_stash
>  }
>  
> -apply_to_branch () {
> -	test -n "$1" || die "$(gettext "No branch name specified")"
> -	branch=$1
> -	shift 1
> -
> -	set -- --index "$@"
> -	assert_stash_like "$@"
> -
> -	git checkout -b $branch $REV^ &&
> -	apply_stash "$@" && {
> -		test -z "$IS_STASH_REF" || drop_stash "$@"
> -	}
> -}
> -
>  test "$1" = "-p" && set "push" "$@"
>  
>  PARSE_CACHE='--not-parsed'
> @@ -672,7 +658,8 @@ pop)
>  	;;
>  branch)
>  	shift
> -	apply_to_branch "$@"
> +	cd "$START_DIR"
> +	git stash--helper branch "$@"
>  	;;
>  *)
>  	case $# in

The rest looks obviously good to me (I am not all that sure about the `cd
"$START_DIR"` but it definitely does not hurt).

Ciao,
Dscho