Nguyễn Thái Ngọc Duy <pclo...@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 },