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

From: Thomas Gummerer <t.gummerer@gmail.com>
Date: Mar 25, 2018, 17:02 UTC
Message-ID: <20180325170236.GB10909@hank>
In-Reply-To: <20180324173707.17699-3-joel@teichroeb.net>

On 03/24, Joel Teichroeb wrote:

Show 54 quoted lines

> ---
>  builtin/stash--helper.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  git-stash.sh            |  3 ++-
>  2 files changed, 46 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
> index e9a9574f40..18c4aba665 100644
> --- a/builtin/stash--helper.c
> +++ b/builtin/stash--helper.c
> @@ -12,6 +12,7 @@
>  
>  static const char * const git_stash_helper_usage[] = {
>  	N_("git stash--helper apply [--index] [-q|--quiet] [<stash>]"),
> +	N_("git stash--helper branch <branchname> [<stash>]"),
>  	NULL
>  };
>  
> @@ -20,6 +21,11 @@ static const char * const git_stash_helper_apply_usage[] = {
>  	NULL
>  };
>  
> +static const char * const git_stash_helper_branch_usage[] = {
> +	N_("git stash--helper branch <branchname> [<stash>]"),
> +	NULL
> +};
> +
>  static const char *ref_stash = "refs/stash";
>  static int quiet;
>  static char stash_index_path[PATH_MAX];
> @@ -307,6 +313,42 @@ static int apply_stash(int argc, const char **argv, const char *prefix)
>  	return do_apply_stash(prefix, &info, index);
>  }
>  
> +static int branch_stash(int argc, const char **argv, const char *prefix)
> +{
> +	const char *commit = NULL, *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) {
> +		branch = argv[0];
> +		if (argc == 2)
> +			commit = argv[1];
> +	}
> +
> +	if (get_stash_info(&info, commit))
> +		return -1;

I see this is supposed to do something similar to what 'assert_stash_like' was doing. However we never end up die'ing with "... is not a a stash-like commit" here from what I can see. I think I can see where this is coming from, and I missed it when reading over 1/4 here. I'll go back and comment there, where I think we're going slightly wrong.

Either way while I tripped over the 'get_stash_info' call here, I think it's the right thing to do.

Show 10 quoted lines

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

'do_drop_stash' is only defined in the next patch. Maybe maybe 2/4 and 3/4 need to swap places?

All patches should compile individually, and all tests should pass for each patch, so we maintain bisectability of the codebase.

Show 33 quoted lines

> +
> +	return ret;
> +}
> +
>  int cmd_stash__helper(int argc, const char **argv, const char *prefix)
>  {
>  	int result = 0;
> @@ -329,6 +371,8 @@ int cmd_stash__helper(int argc, const char **argv, const char *prefix)
>  		usage_with_options(git_stash_helper_usage, options);
>  	else if (!strcmp(argv[0], "apply"))
>  		result = apply_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 92c084eb17..360643ad4e 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -736,7 +736,8 @@ pop)
>  	;;
>  branch)
>  	shift
> -	apply_to_branch "$@"
> +	cd "$START_DIR"
> +	git stash--helper branch "$@"
>  	;;
>  *)
>  	case $# in
> -- 
> 2.16.2
>