Re: [PATCH v2 6/7] checkout: split into switch-branch and checkout-files

[Junio C Hamano <gitster@pobox.com>]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> The good old "git checkout" command is still here and will be until
> all (or most of users) are sick of it.

Two comments on the goal (the implementation looked reasonable
assuming the reader agrees with the gaol).

At least to me, the verb "switch" needs two things to switch
between, i.e. "switch A and B", unless it is "switch to X".
Either "switch-to-branch" or simply "switch-to", perhaps?

As I already hinted in my response to Stefan (?) about
checkout-from-tree vs checkout-from-index, a command with multiple
modes of operation is not confusing to people with the right mental
model, and I suspect that having two separate commands for "checking
out a branch" and "checking out paths" that is done by this step
would help users to form the right mental model.  So I tend to think
these two are "training wheels", and suspect that once they got it,
nobody will become "sick of" the single "checkout" command that can
be used to do either.  It's just the matter of being aware what can
be done (which requires the right mental model) and how to tell Git
what the user wants it do (two separate commands, operating mode
option, or just the implied command line syntax---once the user
knows what s/he is doing, these do not make that much a difference).

As to the implementation,

>  	int dwim_new_local_branch;
> +	int accept_pathspec;
> +	int empty_arg_ok;

I think this is a reasonable way to completely avoid the codepath
that considers an unknown arg being parsed could be a pathspec
element (e.g. switch-to-that-branch mode will never want to see
pathspec, so disambiguation logic and/or hint should not kick in).

At the beginning of cmd_switch_branches(), please "explicitly"
assign 0 to accept_pathspec field, after memset(0) clears the whole
structure.  When a reader sees memset(0), it is read as "giving a
clean and bland slate to futz with with a reasonable default", but
for this particular field, i.e. accept_pathspec, there is NO
reasoanble default.  It's not like switch-to-branch mode is the heir
to checkout and the other sibling checkout-files is doing a
non-default behaviour.

Same comment probably would apply to the other one, although I
didn't think too deeply about that one.


>  	/*
>  	 * If new checkout options are added, skip_merge_working_tree
> @@ -1056,7 +1068,7 @@ static int parse_branchname_arg(int argc, const char **argv,
>  	arg = argv[0];
>  	dash_dash_pos = -1;
>  	for (i = 0; i < argc; i++) {
> -		if (!strcmp(argv[i], "--")) {
> +		if (opts->accept_pathspec && !strcmp(argv[i], "--")) {
>  			dash_dash_pos = i;
>  			break;
>  		}
> @@ -1067,6 +1079,8 @@ static int parse_branchname_arg(int argc, const char **argv,
>  		has_dash_dash = 1; /* case (3) or (1) */
>  	else if (dash_dash_pos >= 2)
>  		die(_("only one reference expected, %d given."), dash_dash_pos);
> +	else if (!opts->accept_pathspec)
> +		has_dash_dash = 1;
>  
>  	if (!strcmp(arg, "-"))
>  		arg = "@{-1}";
> @@ -1291,30 +1305,23 @@ static struct option *add_checkout_path_options(struct checkout_opts *opts,
>  	return newopts;
>  }
>  
> -int cmd_checkout(int argc, const char **argv, const char *prefix)
> +static int checkout_main(int argc, const char **argv, const char *prefix,
> +			 struct checkout_opts *opts, struct option *options,
> +			 const char * const usagestr[])
>  {
> -	struct checkout_opts real_opts;
> -	struct checkout_opts *opts = &real_opts;
>  	struct branch_info new_branch_info;
>  	int dwim_remotes_matched = 0;
> -	struct option *options = NULL;
>  
> -	memset(opts, 0, sizeof(*opts));
>  	memset(&new_branch_info, 0, sizeof(new_branch_info));
>  	opts->overwrite_ignore = 1;
>  	opts->prefix = prefix;
>  	opts->show_progress = -1;
> -	opts->dwim_new_local_branch = 1;
>  
>  	git_config(git_checkout_config, opts);
>  
>  	opts->track = BRANCH_TRACK_UNSPECIFIED;
>  
> -	options = add_common_options(opts, options);
> -	options = add_switch_branch_options(opts, options);
> -	options = add_checkout_path_options(opts, options);
> -
> -	argc = parse_options(argc, argv, prefix, options, checkout_usage,
> +	argc = parse_options(argc, argv, prefix, options, usagestr,
>  			     PARSE_OPT_KEEP_DASHDASH);
>  
>  	if (opts->show_progress < 0) {
> @@ -1381,7 +1388,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
>  					     &dwim_remotes_matched);
>  		argv += n;
>  		argc -= n;
> -	}
> +	} else if (!opts->empty_arg_ok)
> +		usage_with_options(usagestr, options);
>  
>  	if (argc) {
>  		parse_pathspec(&opts->pathspec, 0,
> @@ -1443,3 +1451,60 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
>  		return checkout_branch(opts, &new_branch_info);
>  	}
>  }
> +
> +int cmd_checkout(int argc, const char **argv, const char *prefix)
> +{
> +	struct checkout_opts opts;
> +	struct option *options = NULL;
> +	int ret;
> +
> +	memset(&opts, 0, sizeof(opts));
> +	opts.dwim_new_local_branch = 1;
> +	opts.accept_pathspec = 1;
> +	opts.empty_arg_ok = 1;
> +
> +	options = add_common_options(&opts, options);
> +	options = add_switch_branch_options(&opts, options);
> +	options = add_checkout_path_options(&opts, options);
> +
> +	ret = checkout_main(argc, argv, prefix, &opts,
> +			    options, checkout_usage);
> +	FREE_AND_NULL(options);
> +	return ret;
> +}
> +
> +int cmd_switch_branch(int argc, const char **argv, const char *prefix)
> +{
> +	struct checkout_opts opts;
> +	struct option *options = NULL;
> +	int ret;
> +
> +	memset(&opts, 0, sizeof(opts));
> +	opts.dwim_new_local_branch = 1;
> +
> +	options = add_common_options(&opts, options);
> +	options = add_switch_branch_options(&opts, options);
> +
> +	ret = checkout_main(argc, argv, prefix, &opts,
> +			    options, switch_branch_usage);
> +	FREE_AND_NULL(options);
> +	return ret;
> +}
> +
> +int cmd_checkout_files(int argc, const char **argv, const char *prefix)
> +{
> +	struct checkout_opts opts;
> +	struct option *options = NULL;
> +	int ret;
> +
> +	memset(&opts, 0, sizeof(opts));
> +	opts.accept_pathspec = 1;
> +
> +	options = add_common_options(&opts, options);
> +	options = add_checkout_path_options(&opts, options);
> +
> +	ret = checkout_main(argc, argv, prefix, &opts,
> +			    options, checkout_files_usage);
> +	FREE_AND_NULL(options);
> +	return ret;
> +}
> diff --git a/git.c b/git.c
> index 2f604a41ea..3b86ba765c 100644
> --- a/git.c
> +++ b/git.c
> @@ -457,6 +457,7 @@ static struct cmd_struct commands[] = {
>  	{ "check-mailmap", cmd_check_mailmap, RUN_SETUP },
>  	{ "check-ref-format", cmd_check_ref_format, NO_PARSEOPT  },
>  	{ "checkout", cmd_checkout, RUN_SETUP | NEED_WORK_TREE },
> +	{ "checkout-files", cmd_checkout_files, RUN_SETUP | NEED_WORK_TREE },
>  	{ "checkout-index", cmd_checkout_index,
>  		RUN_SETUP | NEED_WORK_TREE},
>  	{ "cherry", cmd_cherry, RUN_SETUP },
> @@ -557,6 +558,7 @@ static struct cmd_struct commands[] = {
>  	{ "status", cmd_status, RUN_SETUP | NEED_WORK_TREE },
>  	{ "stripspace", cmd_stripspace },
>  	{ "submodule--helper", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX | NO_PARSEOPT },
> +	{ "switch-branch", cmd_switch_branch, RUN_SETUP | NEED_WORK_TREE },
>  	{ "symbolic-ref", cmd_symbolic_ref, RUN_SETUP },
>  	{ "tag", cmd_tag, RUN_SETUP | DELAY_PAGER_CONFIG },
>  	{ "unpack-file", cmd_unpack_file, RUN_SETUP | NO_PARSEOPT },