| Message ID | 20181120174554.GA29910@duynguyen.home (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] Introduce two new commands, switch-branch and restore-paths | expand |
On 11/20, Duy Nguyen wrote: > On Mon, Nov 19, 2018 at 04:19:53PM +0100, Duy Nguyen wrote: > > I promise to come back with something better (at least it still > > sounds better in my mind). If that idea does not work out, we can > > come back and see if we can improve this. > > So this is it. The patch isn't pretty, mostly as a proof of > concept. Just look at the three functions at the bottom of checkout.c, > which is the main thing. > > This patch tries to split "git checkout" command in two new ones: > > - git switch-branch is all about switching branches > - git restore-paths (maybe restore-file is better) for checking out > paths > > The main idea is these two commands will co-exist with the good old > 'git checkout', which will NOT be deprecated. Old timers will still > use "git checkout". But new people should be introduced to the new two > instead. And the new ones are just as capable as "git checkout". > > Since the three commands will co-exist (with duplicate functionality), > maintenance cost must be kept to minimum. The way I did this is simply > split the command line options into three pieces: common, > switch-branch and checkout-paths. "git checkout" has all three, the > other two have common and another piece. > > With this, a new option added to git checkout will be automatically > available in either switch-branch or checkout-paths. Bug fixes apply > to all relevant commands. > > Later on, we could start to add a bit more stuff in, e.g. some form of > disambiguation is no longer needed when running as switch-branch, or > restore-paths. > > So, what do you think? I like the idea of splitting those commands up, in fact it is something I've been considering working on myself. I do think we should consider if we want to change the behaviour of those new commands in any way compared to 'git checkout', since we're starting with a clean slate. One thing in particular that I have in mind is something I'm currently working on, namely adding a --index flag to 'git checkout', which would make 'git checkout' work in non-overlay mode (for more discussion on that see also [*1*]. I got something working, that needs to be polished a bit and am hoping to send that to the list sometime soon. I wonder if such the --index behaviour could be the default in restore-paths command? Most of the underlying machinery for 'checkout' could and should of course still be shared between the commands. *1*: <xmqq4loqplou.fsf@gitster.mtv.corp.google.com> > -- 8< -- > diff --git a/builtin.h b/builtin.h > index 6538932e99..6e321ec8a4 100644 > --- a/builtin.h > +++ b/builtin.h > @@ -214,6 +214,7 @@ extern int cmd_remote_fd(int argc, const char **argv, const char *prefix); > extern int cmd_repack(int argc, const char **argv, const char *prefix); > extern int cmd_rerere(int argc, const char **argv, const char *prefix); > extern int cmd_reset(int argc, const char **argv, const char *prefix); > +extern int cmd_restore_paths(int argc, const char **argv, const char *prefix); > extern int cmd_rev_list(int argc, const char **argv, const char *prefix); > extern int cmd_rev_parse(int argc, const char **argv, const char *prefix); > extern int cmd_revert(int argc, const char **argv, const char *prefix); > @@ -227,6 +228,7 @@ extern int cmd_show_index(int argc, const char **argv, const char *prefix); > extern int cmd_status(int argc, const char **argv, const char *prefix); > extern int cmd_stripspace(int argc, const char **argv, const char *prefix); > extern int cmd_submodule__helper(int argc, const char **argv, const char *prefix); > +extern int cmd_switch_branch(int argc, const char **argv, const char *prefix); > extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix); > extern int cmd_tag(int argc, const char **argv, const char *prefix); > extern int cmd_tar_tree(int argc, const char **argv, const char *prefix); > diff --git a/builtin/checkout.c b/builtin/checkout.c > index acdafc6e4c..868ca3c223 100644 > --- a/builtin/checkout.c > +++ b/builtin/checkout.c > @@ -33,6 +33,16 @@ static const char * const checkout_usage[] = { > NULL, > }; > > +static const char * const switch_branch_usage[] = { > + N_("git switch-branch [<options>] <branch>"), > + NULL, > +}; > + > +static const char * const restore_paths_usage[] = { > + N_("git restore-paths [<options>] [<branch>] -- <file>..."), > + NULL, > +}; > + > struct checkout_opts { > int patch_mode; > int quiet; > @@ -44,6 +54,7 @@ struct checkout_opts { > int ignore_skipworktree; > int ignore_other_worktrees; > int show_progress; > + int dwim_new_local_branch; > /* > * If new checkout options are added, skip_merge_working_tree > * should be updated accordingly. > @@ -55,6 +66,7 @@ struct checkout_opts { > int new_branch_log; > enum branch_track track; > struct diff_options diff_options; > + char *conflict_style; > > int branch_exists; > const char *prefix; > @@ -1223,78 +1235,105 @@ static int checkout_branch(struct checkout_opts *opts, > return switch_branches(opts, new_branch_info); > } > > -int cmd_checkout(int argc, const char **argv, const char *prefix) > +static struct option *add_common_options(struct checkout_opts *opts, > + struct option *prevopts) > { > - struct checkout_opts opts; > - struct branch_info new_branch_info; > - char *conflict_style = NULL; > - int dwim_new_local_branch = 1; > - int dwim_remotes_matched = 0; > struct option options[] = { > - OPT__QUIET(&opts.quiet, N_("suppress progress reporting")), > - OPT_STRING('b', NULL, &opts.new_branch, N_("branch"), > + OPT__QUIET(&opts->quiet, N_("suppress progress reporting")), > + OPT_BOOL(0, "ignore-skip-worktree-bits", &opts->ignore_skipworktree, > + N_("do not limit pathspecs to sparse entries only")), > + { OPTION_CALLBACK, 0, "recurse-submodules", NULL, > + "checkout", "control recursive updating of submodules", > + PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater }, > + OPT_BOOL(0, "progress", &opts->show_progress, N_("force progress reporting")), > + OPT__FORCE(&opts->force, N_("force checkout (throw away local modifications)"), > + PARSE_OPT_NOCOMPLETE), > + OPT_STRING(0, "conflict", &opts->conflict_style, N_("style"), > + N_("conflict style (merge or diff3)")), > + OPT_END() > + }; > + struct option *newopts = parse_options_concat(prevopts, options); > + free(prevopts); > + return newopts; > +} > + > +static struct option *add_switch_branch_options(struct checkout_opts *opts, > + struct option *prevopts) > +{ > + struct option options[] = { > + OPT_STRING('b', NULL, &opts->new_branch, N_("branch"), > N_("create and checkout a new branch")), > - OPT_STRING('B', NULL, &opts.new_branch_force, N_("branch"), > + OPT_STRING('B', NULL, &opts->new_branch_force, N_("branch"), > N_("create/reset and checkout a branch")), > - OPT_BOOL('l', NULL, &opts.new_branch_log, N_("create reflog for new branch")), > - OPT_BOOL(0, "detach", &opts.force_detach, N_("detach HEAD at named commit")), > - OPT_SET_INT('t', "track", &opts.track, N_("set upstream info for new branch"), > + OPT_BOOL('l', NULL, &opts->new_branch_log, N_("create reflog for new branch")), > + OPT_BOOL(0, "detach", &opts->force_detach, N_("detach HEAD at named commit")), > + OPT_SET_INT('t', "track", &opts->track, N_("set upstream info for new branch"), > BRANCH_TRACK_EXPLICIT), > - OPT_STRING(0, "orphan", &opts.new_orphan_branch, N_("new-branch"), N_("new unparented branch")), > - OPT_SET_INT_F('2', "ours", &opts.writeout_stage, > + OPT_STRING(0, "orphan", &opts->new_orphan_branch, N_("new-branch"), N_("new unparented branch")), > + OPT_BOOL('m', "merge", &opts->merge, N_("perform a 3-way merge with the new branch")), > + OPT_HIDDEN_BOOL(0, "guess", &opts->dwim_new_local_branch, > + N_("second guess 'git checkout <no-such-branch>'")), > + OPT_BOOL(0, "ignore-other-worktrees", &opts->ignore_other_worktrees, > + N_("do not check if another worktree is holding the given ref")), > + OPT_END() > + }; > + struct option *newopts = parse_options_concat(prevopts, options); > + free(prevopts); > + return newopts; > +} > + > +static struct option *add_checkout_path_options(struct checkout_opts *opts, > + struct option *prevopts) > +{ > + struct option options[] = { > + OPT_SET_INT_F('2', "ours", &opts->writeout_stage, > N_("checkout our version for unmerged files"), > 2, PARSE_OPT_NONEG), > - OPT_SET_INT_F('3', "theirs", &opts.writeout_stage, > + OPT_SET_INT_F('3', "theirs", &opts->writeout_stage, > N_("checkout their version for unmerged files"), > 3, PARSE_OPT_NONEG), > - OPT__FORCE(&opts.force, N_("force checkout (throw away local modifications)"), > - PARSE_OPT_NOCOMPLETE), > - OPT_BOOL('m', "merge", &opts.merge, N_("perform a 3-way merge with the new branch")), > - OPT_BOOL_F(0, "overwrite-ignore", &opts.overwrite_ignore, > - N_("update ignored files (default)"), > - PARSE_OPT_NOCOMPLETE), > - OPT_STRING(0, "conflict", &conflict_style, N_("style"), > - N_("conflict style (merge or diff3)")), > - OPT_BOOL('p', "patch", &opts.patch_mode, N_("select hunks interactively")), > - OPT_BOOL(0, "ignore-skip-worktree-bits", &opts.ignore_skipworktree, > - N_("do not limit pathspecs to sparse entries only")), > - OPT_HIDDEN_BOOL(0, "guess", &dwim_new_local_branch, > - N_("second guess 'git checkout <no-such-branch>'")), > - OPT_BOOL(0, "ignore-other-worktrees", &opts.ignore_other_worktrees, > - N_("do not check if another worktree is holding the given ref")), > - { OPTION_CALLBACK, 0, "recurse-submodules", NULL, > - "checkout", "control recursive updating of submodules", > - PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater }, > - OPT_BOOL(0, "progress", &opts.show_progress, N_("force progress reporting")), > - OPT_END(), > + OPT_BOOL('p', "patch", &opts->patch_mode, N_("select hunks interactively")), > + OPT_END() > }; > + struct option *newopts = parse_options_concat(prevopts, options); > + free(prevopts); > + return newopts; > +} > > - memset(&opts, 0, sizeof(opts)); > +static int checkout_main(int argc, const char **argv, const char *prefix, > + struct checkout_opts *opts, struct option *options, > + const char * const usagestr[]) > +{ > + struct branch_info new_branch_info; > + int dwim_remotes_matched = 0; > + > + memset(opts, 0, sizeof(*opts)); > + opts->dwim_new_local_branch = 1; > memset(&new_branch_info, 0, sizeof(new_branch_info)); > - opts.overwrite_ignore = 1; > - opts.prefix = prefix; > - opts.show_progress = -1; > + opts->overwrite_ignore = 1; > + opts->prefix = prefix; > + opts->show_progress = -1; > > - git_config(git_checkout_config, &opts); > + git_config(git_checkout_config, opts); > > - opts.track = BRANCH_TRACK_UNSPECIFIED; > + opts->track = BRANCH_TRACK_UNSPECIFIED; > > - 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) { > - if (opts.quiet) > - opts.show_progress = 0; > + if (opts->show_progress < 0) { > + if (opts->quiet) > + opts->show_progress = 0; > else > - opts.show_progress = isatty(2); > + opts->show_progress = isatty(2); > } > > - if (conflict_style) { > - opts.merge = 1; /* implied */ > - git_xmerge_config("merge.conflictstyle", conflict_style, NULL); > + if (opts->conflict_style) { > + opts->merge = 1; /* implied */ > + git_xmerge_config("merge.conflictstyle", opts->conflict_style, NULL); > } > > - if ((!!opts.new_branch + !!opts.new_branch_force + !!opts.new_orphan_branch) > 1) > + if ((!!opts->new_branch + !!opts->new_branch_force + !!opts->new_orphan_branch) > 1) > die(_("-b, -B and --orphan are mutually exclusive")); > > /* > @@ -1302,14 +1341,14 @@ int cmd_checkout(int argc, const char **argv, const char *prefix) > * and new_branch_force and new_orphan_branch will tell us which one of > * -b/-B/--orphan is being used. > */ > - if (opts.new_branch_force) > - opts.new_branch = opts.new_branch_force; > + if (opts->new_branch_force) > + opts->new_branch = opts->new_branch_force; > > - if (opts.new_orphan_branch) > - opts.new_branch = opts.new_orphan_branch; > + if (opts->new_orphan_branch) > + opts->new_branch = opts->new_orphan_branch; > > /* --track without -b/-B/--orphan should DWIM */ > - if (opts.track != BRANCH_TRACK_UNSPECIFIED && !opts.new_branch) { > + if (opts->track != BRANCH_TRACK_UNSPECIFIED && !opts->new_branch) { > const char *argv0 = argv[0]; > if (!argc || !strcmp(argv0, "--")) > die(_("--track needs a branch name")); > @@ -1318,7 +1357,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix) > argv0 = strchr(argv0, '/'); > if (!argv0 || !argv0[1]) > die(_("missing branch name; try -b")); > - opts.new_branch = argv0 + 1; > + opts->new_branch = argv0 + 1; > } > > /* > @@ -1337,56 +1376,56 @@ int cmd_checkout(int argc, const char **argv, const char *prefix) > if (argc) { > struct object_id rev; > int dwim_ok = > - !opts.patch_mode && > - dwim_new_local_branch && > - opts.track == BRANCH_TRACK_UNSPECIFIED && > - !opts.new_branch; > + !opts->patch_mode && > + opts->dwim_new_local_branch && > + opts->track == BRANCH_TRACK_UNSPECIFIED && > + !opts->new_branch; > int n = parse_branchname_arg(argc, argv, dwim_ok, > - &new_branch_info, &opts, &rev, > + &new_branch_info, opts, &rev, > &dwim_remotes_matched); > argv += n; > argc -= n; > } > > if (argc) { > - parse_pathspec(&opts.pathspec, 0, > - opts.patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0, > + parse_pathspec(&opts->pathspec, 0, > + opts->patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0, > prefix, argv); > > - if (!opts.pathspec.nr) > + if (!opts->pathspec.nr) > die(_("invalid path specification")); > > /* > * Try to give more helpful suggestion. > * new_branch && argc > 1 will be caught later. > */ > - if (opts.new_branch && argc == 1) > + if (opts->new_branch && argc == 1) > die(_("'%s' is not a commit and a branch '%s' cannot be created from it"), > - argv[0], opts.new_branch); > + argv[0], opts->new_branch); > > - if (opts.force_detach) > + if (opts->force_detach) > die(_("git checkout: --detach does not take a path argument '%s'"), > argv[0]); > > - if (1 < !!opts.writeout_stage + !!opts.force + !!opts.merge) > + if (1 < !!opts->writeout_stage + !!opts->force + !!opts->merge) > die(_("git checkout: --ours/--theirs, --force and --merge are incompatible when\n" > "checking out of the index.")); > } > > - if (opts.new_branch) { > + if (opts->new_branch) { > struct strbuf buf = STRBUF_INIT; > > - if (opts.new_branch_force) > - opts.branch_exists = validate_branchname(opts.new_branch, &buf); > + if (opts->new_branch_force) > + opts->branch_exists = validate_branchname(opts->new_branch, &buf); > else > - opts.branch_exists = > - validate_new_branchname(opts.new_branch, &buf, 0); > + opts->branch_exists = > + validate_new_branchname(opts->new_branch, &buf, 0); > strbuf_release(&buf); > } > > UNLEAK(opts); > - if (opts.patch_mode || opts.pathspec.nr) { > - int ret = checkout_paths(&opts, new_branch_info.name); > + if (opts->patch_mode || opts->pathspec.nr) { > + int ret = checkout_paths(opts, new_branch_info.name); > if (ret && dwim_remotes_matched > 1 && > advice_checkout_ambiguous_remote_branch_name) > advise(_("'%s' matched more than one remote tracking branch.\n" > @@ -1405,6 +1444,49 @@ int cmd_checkout(int argc, const char **argv, const char *prefix) > dwim_remotes_matched); > return ret; > } else { > - return checkout_branch(&opts, &new_branch_info); > + 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; > + > + 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; > + > + 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_restore_paths(int argc, const char **argv, const char *prefix) > +{ > + struct checkout_opts opts; > + struct option *options = NULL; > + int ret; > + > + options = add_common_options(&opts, options); > + options = add_checkout_path_options(&opts, options); > + ret = checkout_main(argc, argv, prefix, &opts, > + options, restore_paths_usage); > + FREE_AND_NULL(options); > + return ret; > +} > diff --git a/git.c b/git.c > index 2f604a41ea..e8a76a99da 100644 > --- a/git.c > +++ b/git.c > @@ -542,6 +542,7 @@ static struct cmd_struct commands[] = { > { "replace", cmd_replace, RUN_SETUP }, > { "rerere", cmd_rerere, RUN_SETUP }, > { "reset", cmd_reset, RUN_SETUP }, > + { "restore-paths", cmd_restore_paths, RUN_SETUP | NEED_WORK_TREE }, > { "rev-list", cmd_rev_list, RUN_SETUP | NO_PARSEOPT }, > { "rev-parse", cmd_rev_parse, NO_PARSEOPT }, > { "revert", cmd_revert, RUN_SETUP | NEED_WORK_TREE }, > @@ -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 }, > diff --git a/parse-options-cb.c b/parse-options-cb.c > index 8c9edce52f..c609d52926 100644 > --- a/parse-options-cb.c > +++ b/parse-options-cb.c > @@ -126,7 +126,7 @@ struct option *parse_options_concat(struct option *a, struct option *b) > struct option *ret; > size_t i, a_len = 0, b_len = 0; > > - for (i = 0; a[i].type != OPTION_END; i++) > + for (i = 0; a && a[i].type != OPTION_END; i++) > a_len++; > for (i = 0; b[i].type != OPTION_END; i++) > b_len++; > -- 8< --
Thomas Gummerer <t.gummerer@gmail.com> writes: > I like the idea of splitting those commands up, in fact it is > something I've been considering working on myself. I do think we > should consider if we want to change the behaviour of those new > commands in any way compared to 'git checkout', since we're starting > with a clean slate. > > One thing in particular that I have in mind is something I'm currently > working on, namely adding a --index flag to 'git checkout', which > would make 'git checkout' work in non-overlay mode (for more > discussion on that see also [*1*]. Ah, thanks for reminding me of that. That explains why I felt uneasy to see "restore" in the proposed command name. In short, I think "checkout --index <tree> <pathspec>", i.e. if the <pathspec> matches a directory in <tree> and the current index and/or the working tree has tracked paths in that directory that do not exist in <tree>, the operation _removes_ these paths so that the result matches <tree>, should become the default of "I want to check out these paths from the named tree-ish". The current one is not exactly "checking out the paths" in that it ignores and does not check out the absense of paths in <tree>. That operation sounds more like "restoring paths out of a given tree". If the tree does not have some paths, these paths won't be "restored" from that tree, so "restore" matches the current "overlay what's taken out of the given tree on top of what is already in the index and the working tree, without checking out the absense of paths" better from that point of view.
On Mon, Nov 26, 2018 at 4:03 AM Junio C Hamano <gitster@pobox.com> wrote: > > Thomas Gummerer <t.gummerer@gmail.com> writes: > > > I like the idea of splitting those commands up, in fact it is > > something I've been considering working on myself. I do think we > > should consider if we want to change the behaviour of those new > > commands in any way compared to 'git checkout', since we're starting > > with a clean slate. Better defaults? Hell yes! > > One thing in particular that I have in mind is something I'm currently > > working on, namely adding a --index flag to 'git checkout', which > > would make 'git checkout' work in non-overlay mode (for more > > discussion on that see also [*1*]. > > Ah, thanks for reminding me of that. That explains why I felt > uneasy to see "restore" in the proposed command name. About that name. I didn't want to start the command name with checkout to avoid completion conflict (the obvious choice was checkout-path, the function name behind it). And I didn't find any other good name, so I picked "restore" out of git-checkout.txt's one-line description. If we end up with a better command name, perhaps reword that line too.
On Tue, Nov 20 2018, Duy Nguyen wrote: > On Mon, Nov 19, 2018 at 04:19:53PM +0100, Duy Nguyen wrote: >> I promise to come back with something better (at least it still >> sounds better in my mind). If that idea does not work out, we can >> come back and see if we can improve this. > > So this is it. The patch isn't pretty, mostly as a proof of > concept. Just look at the three functions at the bottom of checkout.c, > which is the main thing. > > This patch tries to split "git checkout" command in two new ones: > > - git switch-branch is all about switching branches Isn't this going to also take the other ref arguments 'git checkout' takes now? I.e. tags, detached HEADs etc? I'm reminded of the discussion about what "range-diff" should be called :) > - git restore-paths (maybe restore-file is better) for checking out > paths If it takes globs/dirs then a plural is probably better. > The main idea is these two commands will co-exist with the good old > 'git checkout', which will NOT be deprecated. Old timers will still > use "git checkout". But new people should be introduced to the new two > instead. And the new ones are just as capable as "git checkout". > > Since the three commands will co-exist (with duplicate functionality), > maintenance cost must be kept to minimum. The way I did this is simply > split the command line options into three pieces: common, > switch-branch and checkout-paths. "git checkout" has all three, the > other two have common and another piece. > > With this, a new option added to git checkout will be automatically > available in either switch-branch or checkout-paths. Bug fixes apply > to all relevant commands. > > Later on, we could start to add a bit more stuff in, e.g. some form of > disambiguation is no longer needed when running as switch-branch, or > restore-paths. > > So, what do you think? That "git checkout" does too many things is something that keeps coming up in online discussions about Git's UI. Two things: a) It would really help to have some comparison of cases where these split commands are much clearer or less ambiguous than git-checkout. I can think of some (e.g. branch with the same name as a file) but having some overall picture of what the new UI looks like with solved / not solved cases would be nice. Also a comparison with other SCMs people find less confusing (svn, hg, bzr, ...) b) I think we really need to have some end-game where we'd actually switch away from "checkout" (which we could still auto-route to new commands in perpetuity, but print a warning or error). Otherwise we'll just end up with https://xkcd.com/927/ and more UI confusion for all.
On Mon, Nov 26, 2018 at 5:01 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > So, what do you think? > > That "git checkout" does too many things is something that keeps coming > up in online discussions about Git's UI. Two things: > > a) It would really help to have some comparison of cases where these > split commands are much clearer or less ambiguous than > git-checkout. I can think of some (e.g. branch with the same name as > a file) but having some overall picture of what the new UI looks like > with solved / not solved cases would be nice. Also a comparison with > other SCMs people find less confusing (svn, hg, bzr, ...) Less ambiguous is indeed one of the reasons I wanted to do this. > b) I think we really need to have some end-game where we'd actually > switch away from "checkout" (which we could still auto-route to new > commands in perpetuity, but print a warning or error). Otherwise > we'll just end up with https://xkcd.com/927/ and more UI confusion > for all. I'm not going to remove "git checkout". Not until the majority of users are really in favor of the new ones. So my end-game plan is to just promote the two new commands in man pages, tutorial, advice... Perhaps at some point "git checkout" is also removed from "git help". But that's about it. If you want to stick with "git checkout", it's there (and not nagging you to move to new ones).
On Mon, Nov 26, 2018 at 8:01 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > > On Tue, Nov 20 2018, Duy Nguyen wrote: > > > On Mon, Nov 19, 2018 at 04:19:53PM +0100, Duy Nguyen wrote: > >> I promise to come back with something better (at least it still > >> sounds better in my mind). If that idea does not work out, we can > >> come back and see if we can improve this. > > > > So this is it. The patch isn't pretty, mostly as a proof of > > concept. Just look at the three functions at the bottom of checkout.c, > > which is the main thing. > > > > This patch tries to split "git checkout" command in two new ones: > > > > - git switch-branch is all about switching branches > > Isn't this going to also take the other ref arguments 'git checkout' > takes now? I.e. tags, detached HEADs etc? I'm reminded of the discussion > about what "range-diff" should be called :) Heh, good call. :-) Note that the color of a bikeshed has fewer functional implications than coming up with proper names in your API exposed to millions of users, as cognitive associations playing mind tricks, can have a huge impact on their productivity. ;-) In a neighboring thread there is discussion of the concept of a 'change' (and evolving the change locally), which is yet another thing in the refs-space. 'switch-branch' sounds like a good name for a beginner who is just getting started, but as soon as they discover that there is more than branches (detached HEAD via commits, tags, remote tracking "branches"), this name may be confusing. So it would not be a good choice for the intermediate Git user. The old time power user would not care as they have 'checkout' in their muscle memory, aliased to 'co', but maybe they'd find it nice for explaining to new users. So I'd be thrilled to find a name that serves users on all levels. Maybe we need to step back and consider what the command does. And from that I would name it "rewire-HEAD-and-update-index&worktree" and then simplify from there. For the beginner user, the concept of HEAD might be overwhelming, such that we don't want to have that in there. So I'd be tempted to suggest to call it "switch-to-ref", but that would be wrong in the corner case as well: When using that with a remote tracking branch, you don't "switch to it" by putting it into your HEAD, but you merely checkout the commit that it's pointing at. > > > - git restore-paths (maybe restore-file is better) for checking out > > paths "content-to-path", maybe(?) as it moves the content (as given by commit or implicitly assuming the index when omitted) into that path(, again). (I am not enthused about this, as you can similarly argue for content-to-paths, content-to-worktree, which then could split up into "index-to-worktree [pathspec]" as well as "tree-to-worktree <commit>". also the notion of X-to-Y seems a novel concept in our naming, so maybe verb-noun is better, hence restore-path or "fix-paths" may be better) > > Later on, we could start to add a bit more stuff in, e.g. some form of > > disambiguation is no longer needed when running as switch-branch, or > > restore-paths. > > > > So, what do you think? The patch looks interestingly small :-) > That "git checkout" does too many things is something that keeps coming > up in online discussions about Git's UI. Two things: > > a) It would really help to have some comparison of cases where these > split commands are much clearer or less ambiguous than > git-checkout. I can think of some (e.g. branch with the same name as > a file) but having some overall picture of what the new UI looks like > with solved / not solved cases would be nice. Also a comparison with > other SCMs people find less confusing (svn, hg, bzr, ...) How do other SCMs solve this issue? (What is their design space? How many commands do they have for what git-checkout does all-in-one?) > b) I think we really need to have some end-game where we'd actually > switch away from "checkout" (which we could still auto-route to new > commands in perpetuity, but print a warning or error). Otherwise > we'll just end up with https://xkcd.com/927/ and more UI confusion > for all. Heh, that situation is only avoided when the new command has clear advantages over the old, and ISTM that we can only compete on UX and better defaults, so maybe I'd push for making it more logical, maybe so: git tree-to-worktree # git checkout <commit> -- <path> git index-to-worktree # git checkout -- <path> git rev-to-ref # git checkout <commit> Just food for thought, specifically the last one would be hilarious if we'd end up with it. Stefan
Stefan Beller <sbeller@google.com> writes: > Maybe we need to step back and consider what the command does. > And from that I would name it "rewire-HEAD-and-update-index&worktree" > and then simplify from there. For the beginner user, the concept of > HEAD might be overwhelming, such that we don't want to have that > in there. I'd have to say that it is totally backwards. Use of HEAD, ref, etc. is merely a means to what the end users want to achieve, which is to switch to a "branch" (in air quotes, as the word in this sentence means a bit broader than "a ref that is in refs/heads/"), to choose which lineage of commits to work on to grow or reshape the history. Most of the time, you would be working on a branch (that is a ref that is in refs/heads/), sometimes you would be working on an unnamed "branch" (i.e. detached HEAD state, only difference between being on it and a normal branch is whether it is named---the history manipulation can happen exactly the same way). In other words, your initial motivation of stepping back and thinking about what the command is about is very good. But in the context of checking out a branch, the concept the end user works with are at the level of "branch", not HEAD, ref, index, working tree (all of which are underlying implementation details that let you work on manipulating the history, represented by the "branch"). > "content-to-path", maybe(?) ... Path is not a place. A path t/Makefile is a shared name of a place in a tree, the index, or in the working tree. Renaming "git add" to "content-to-path" because it adds the content to the index at path may be equally OK, but it misses the essense (which is that it is to add to the index and not to anythng else like a tree or the working tree). I actually think the easiest-to-understand shorthand for the operation "checking out the contents at the path to the working tree" is "checkout". These... > git tree-to-worktree # git checkout <commit> -- <path> > git index-to-worktree # git checkout -- <path> ...are interesting and worth learning lessons from. These would be something people would suggest when users start making noises about "restore-to-path" or "content-to-path" overloads three different operations and is confusing. I think "restore-to-path" that can take contents for individual paths out of either a treeish or the index and update the index and the working tree, depending on what the user tells it to do, is not confusing to the end users. At the conceptual level, the users need to have a mental model that has three places (tree-ish, index and working tree) that can hold contents to make use of these operations, so it is only the matter of how to express it clearly and concisely. For that matter, "checkout <branch>", "checkout <treeish> -- <path>" and "checkout -- <path>" already is a trio of clear and cocncise way to spell three distinct operations, so with the right mental model, it may not be so confusing to the users. And "checkout-branch", "checkout-contents-from-tree", and "checkout-contents-from-index" longhands may be a good way to help new users form the right mental model, as they are more explicit in their names; once they form the right mental model (that is, there are three places the contents live, and there are a few operations to move contents from the two places to the working tree, i.e. what traditionally is called "checking things out"), they may gradulate to the shorthand form.
diff --git a/builtin.h b/builtin.h index 6538932e99..6e321ec8a4 100644 --- a/builtin.h +++ b/builtin.h @@ -214,6 +214,7 @@ extern int cmd_remote_fd(int argc, const char **argv, const char *prefix); extern int cmd_repack(int argc, const char **argv, const char *prefix); extern int cmd_rerere(int argc, const char **argv, const char *prefix); extern int cmd_reset(int argc, const char **argv, const char *prefix); +extern int cmd_restore_paths(int argc, const char **argv, const char *prefix); extern int cmd_rev_list(int argc, const char **argv, const char *prefix); extern int cmd_rev_parse(int argc, const char **argv, const char *prefix); extern int cmd_revert(int argc, const char **argv, const char *prefix); @@ -227,6 +228,7 @@ extern int cmd_show_index(int argc, const char **argv, const char *prefix); extern int cmd_status(int argc, const char **argv, const char *prefix); extern int cmd_stripspace(int argc, const char **argv, const char *prefix); extern int cmd_submodule__helper(int argc, const char **argv, const char *prefix); +extern int cmd_switch_branch(int argc, const char **argv, const char *prefix); extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix); extern int cmd_tag(int argc, const char **argv, const char *prefix); extern int cmd_tar_tree(int argc, const char **argv, const char *prefix); diff --git a/builtin/checkout.c b/builtin/checkout.c index acdafc6e4c..868ca3c223 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -33,6 +33,16 @@ static const char * const checkout_usage[] = { NULL, }; +static const char * const switch_branch_usage[] = { + N_("git switch-branch [<options>] <branch>"), + NULL, +}; + +static const char * const restore_paths_usage[] = { + N_("git restore-paths [<options>] [<branch>] -- <file>..."), + NULL, +}; + struct checkout_opts { int patch_mode; int quiet; @@ -44,6 +54,7 @@ struct checkout_opts { int ignore_skipworktree; int ignore_other_worktrees; int show_progress; + int dwim_new_local_branch; /* * If new checkout options are added, skip_merge_working_tree * should be updated accordingly. @@ -55,6 +66,7 @@ struct checkout_opts { int new_branch_log; enum branch_track track; struct diff_options diff_options; + char *conflict_style; int branch_exists; const char *prefix; @@ -1223,78 +1235,105 @@ static int checkout_branch(struct checkout_opts *opts, return switch_branches(opts, new_branch_info); } -int cmd_checkout(int argc, const char **argv, const char *prefix) +static struct option *add_common_options(struct checkout_opts *opts, + struct option *prevopts) { - struct checkout_opts opts; - struct branch_info new_branch_info; - char *conflict_style = NULL; - int dwim_new_local_branch = 1; - int dwim_remotes_matched = 0; struct option options[] = { - OPT__QUIET(&opts.quiet, N_("suppress progress reporting")), - OPT_STRING('b', NULL, &opts.new_branch, N_("branch"), + OPT__QUIET(&opts->quiet, N_("suppress progress reporting")), + OPT_BOOL(0, "ignore-skip-worktree-bits", &opts->ignore_skipworktree, + N_("do not limit pathspecs to sparse entries only")), + { OPTION_CALLBACK, 0, "recurse-submodules", NULL, + "checkout", "control recursive updating of submodules", + PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater }, + OPT_BOOL(0, "progress", &opts->show_progress, N_("force progress reporting")), + OPT__FORCE(&opts->force, N_("force checkout (throw away local modifications)"), + PARSE_OPT_NOCOMPLETE), + OPT_STRING(0, "conflict", &opts->conflict_style, N_("style"), + N_("conflict style (merge or diff3)")), + OPT_END() + }; + struct option *newopts = parse_options_concat(prevopts, options); + free(prevopts); + return newopts; +} + +static struct option *add_switch_branch_options(struct checkout_opts *opts, + struct option *prevopts) +{ + struct option options[] = { + OPT_STRING('b', NULL, &opts->new_branch, N_("branch"), N_("create and checkout a new branch")), - OPT_STRING('B', NULL, &opts.new_branch_force, N_("branch"), + OPT_STRING('B', NULL, &opts->new_branch_force, N_("branch"), N_("create/reset and checkout a branch")), - OPT_BOOL('l', NULL, &opts.new_branch_log, N_("create reflog for new branch")), - OPT_BOOL(0, "detach", &opts.force_detach, N_("detach HEAD at named commit")), - OPT_SET_INT('t', "track", &opts.track, N_("set upstream info for new branch"), + OPT_BOOL('l', NULL, &opts->new_branch_log, N_("create reflog for new branch")), + OPT_BOOL(0, "detach", &opts->force_detach, N_("detach HEAD at named commit")), + OPT_SET_INT('t', "track", &opts->track, N_("set upstream info for new branch"), BRANCH_TRACK_EXPLICIT), - OPT_STRING(0, "orphan", &opts.new_orphan_branch, N_("new-branch"), N_("new unparented branch")), - OPT_SET_INT_F('2', "ours", &opts.writeout_stage, + OPT_STRING(0, "orphan", &opts->new_orphan_branch, N_("new-branch"), N_("new unparented branch")), + OPT_BOOL('m', "merge", &opts->merge, N_("perform a 3-way merge with the new branch")), + OPT_HIDDEN_BOOL(0, "guess", &opts->dwim_new_local_branch, + N_("second guess 'git checkout <no-such-branch>'")), + OPT_BOOL(0, "ignore-other-worktrees", &opts->ignore_other_worktrees, + N_("do not check if another worktree is holding the given ref")), + OPT_END() + }; + struct option *newopts = parse_options_concat(prevopts, options); + free(prevopts); + return newopts; +} + +static struct option *add_checkout_path_options(struct checkout_opts *opts, + struct option *prevopts) +{ + struct option options[] = { + OPT_SET_INT_F('2', "ours", &opts->writeout_stage, N_("checkout our version for unmerged files"), 2, PARSE_OPT_NONEG), - OPT_SET_INT_F('3', "theirs", &opts.writeout_stage, + OPT_SET_INT_F('3', "theirs", &opts->writeout_stage, N_("checkout their version for unmerged files"), 3, PARSE_OPT_NONEG), - OPT__FORCE(&opts.force, N_("force checkout (throw away local modifications)"), - PARSE_OPT_NOCOMPLETE), - OPT_BOOL('m', "merge", &opts.merge, N_("perform a 3-way merge with the new branch")), - OPT_BOOL_F(0, "overwrite-ignore", &opts.overwrite_ignore, - N_("update ignored files (default)"), - PARSE_OPT_NOCOMPLETE), - OPT_STRING(0, "conflict", &conflict_style, N_("style"), - N_("conflict style (merge or diff3)")), - OPT_BOOL('p', "patch", &opts.patch_mode, N_("select hunks interactively")), - OPT_BOOL(0, "ignore-skip-worktree-bits", &opts.ignore_skipworktree, - N_("do not limit pathspecs to sparse entries only")), - OPT_HIDDEN_BOOL(0, "guess", &dwim_new_local_branch, - N_("second guess 'git checkout <no-such-branch>'")), - OPT_BOOL(0, "ignore-other-worktrees", &opts.ignore_other_worktrees, - N_("do not check if another worktree is holding the given ref")), - { OPTION_CALLBACK, 0, "recurse-submodules", NULL, - "checkout", "control recursive updating of submodules", - PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater }, - OPT_BOOL(0, "progress", &opts.show_progress, N_("force progress reporting")), - OPT_END(), + OPT_BOOL('p', "patch", &opts->patch_mode, N_("select hunks interactively")), + OPT_END() }; + struct option *newopts = parse_options_concat(prevopts, options); + free(prevopts); + return newopts; +} - memset(&opts, 0, sizeof(opts)); +static int checkout_main(int argc, const char **argv, const char *prefix, + struct checkout_opts *opts, struct option *options, + const char * const usagestr[]) +{ + struct branch_info new_branch_info; + int dwim_remotes_matched = 0; + + memset(opts, 0, sizeof(*opts)); + opts->dwim_new_local_branch = 1; memset(&new_branch_info, 0, sizeof(new_branch_info)); - opts.overwrite_ignore = 1; - opts.prefix = prefix; - opts.show_progress = -1; + opts->overwrite_ignore = 1; + opts->prefix = prefix; + opts->show_progress = -1; - git_config(git_checkout_config, &opts); + git_config(git_checkout_config, opts); - opts.track = BRANCH_TRACK_UNSPECIFIED; + opts->track = BRANCH_TRACK_UNSPECIFIED; - 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) { - if (opts.quiet) - opts.show_progress = 0; + if (opts->show_progress < 0) { + if (opts->quiet) + opts->show_progress = 0; else - opts.show_progress = isatty(2); + opts->show_progress = isatty(2); } - if (conflict_style) { - opts.merge = 1; /* implied */ - git_xmerge_config("merge.conflictstyle", conflict_style, NULL); + if (opts->conflict_style) { + opts->merge = 1; /* implied */ + git_xmerge_config("merge.conflictstyle", opts->conflict_style, NULL); } - if ((!!opts.new_branch + !!opts.new_branch_force + !!opts.new_orphan_branch) > 1) + if ((!!opts->new_branch + !!opts->new_branch_force + !!opts->new_orphan_branch) > 1) die(_("-b, -B and --orphan are mutually exclusive")); /* @@ -1302,14 +1341,14 @@ int cmd_checkout(int argc, const char **argv, const char *prefix) * and new_branch_force and new_orphan_branch will tell us which one of * -b/-B/--orphan is being used. */ - if (opts.new_branch_force) - opts.new_branch = opts.new_branch_force; + if (opts->new_branch_force) + opts->new_branch = opts->new_branch_force; - if (opts.new_orphan_branch) - opts.new_branch = opts.new_orphan_branch; + if (opts->new_orphan_branch) + opts->new_branch = opts->new_orphan_branch; /* --track without -b/-B/--orphan should DWIM */ - if (opts.track != BRANCH_TRACK_UNSPECIFIED && !opts.new_branch) { + if (opts->track != BRANCH_TRACK_UNSPECIFIED && !opts->new_branch) { const char *argv0 = argv[0]; if (!argc || !strcmp(argv0, "--")) die(_("--track needs a branch name")); @@ -1318,7 +1357,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix) argv0 = strchr(argv0, '/'); if (!argv0 || !argv0[1]) die(_("missing branch name; try -b")); - opts.new_branch = argv0 + 1; + opts->new_branch = argv0 + 1; } /* @@ -1337,56 +1376,56 @@ int cmd_checkout(int argc, const char **argv, const char *prefix) if (argc) { struct object_id rev; int dwim_ok = - !opts.patch_mode && - dwim_new_local_branch && - opts.track == BRANCH_TRACK_UNSPECIFIED && - !opts.new_branch; + !opts->patch_mode && + opts->dwim_new_local_branch && + opts->track == BRANCH_TRACK_UNSPECIFIED && + !opts->new_branch; int n = parse_branchname_arg(argc, argv, dwim_ok, - &new_branch_info, &opts, &rev, + &new_branch_info, opts, &rev, &dwim_remotes_matched); argv += n; argc -= n; } if (argc) { - parse_pathspec(&opts.pathspec, 0, - opts.patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0, + parse_pathspec(&opts->pathspec, 0, + opts->patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0, prefix, argv); - if (!opts.pathspec.nr) + if (!opts->pathspec.nr) die(_("invalid path specification")); /* * Try to give more helpful suggestion. * new_branch && argc > 1 will be caught later. */ - if (opts.new_branch && argc == 1) + if (opts->new_branch && argc == 1) die(_("'%s' is not a commit and a branch '%s' cannot be created from it"), - argv[0], opts.new_branch); + argv[0], opts->new_branch); - if (opts.force_detach) + if (opts->force_detach) die(_("git checkout: --detach does not take a path argument '%s'"), argv[0]); - if (1 < !!opts.writeout_stage + !!opts.force + !!opts.merge) + if (1 < !!opts->writeout_stage + !!opts->force + !!opts->merge) die(_("git checkout: --ours/--theirs, --force and --merge are incompatible when\n" "checking out of the index.")); } - if (opts.new_branch) { + if (opts->new_branch) { struct strbuf buf = STRBUF_INIT; - if (opts.new_branch_force) - opts.branch_exists = validate_branchname(opts.new_branch, &buf); + if (opts->new_branch_force) + opts->branch_exists = validate_branchname(opts->new_branch, &buf); else - opts.branch_exists = - validate_new_branchname(opts.new_branch, &buf, 0); + opts->branch_exists = + validate_new_branchname(opts->new_branch, &buf, 0); strbuf_release(&buf); } UNLEAK(opts); - if (opts.patch_mode || opts.pathspec.nr) { - int ret = checkout_paths(&opts, new_branch_info.name); + if (opts->patch_mode || opts->pathspec.nr) { + int ret = checkout_paths(opts, new_branch_info.name); if (ret && dwim_remotes_matched > 1 && advice_checkout_ambiguous_remote_branch_name) advise(_("'%s' matched more than one remote tracking branch.\n" @@ -1405,6 +1444,49 @@ int cmd_checkout(int argc, const char **argv, const char *prefix) dwim_remotes_matched); return ret; } else { - return checkout_branch(&opts, &new_branch_info); + 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; + + 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; + + 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_restore_paths(int argc, const char **argv, const char *prefix) +{ + struct checkout_opts opts; + struct option *options = NULL; + int ret; + + options = add_common_options(&opts, options); + options = add_checkout_path_options(&opts, options); + ret = checkout_main(argc, argv, prefix, &opts, + options, restore_paths_usage); + FREE_AND_NULL(options); + return ret; +} diff --git a/git.c b/git.c index 2f604a41ea..e8a76a99da 100644 --- a/git.c +++ b/git.c @@ -542,6 +542,7 @@ static struct cmd_struct commands[] = { { "replace", cmd_replace, RUN_SETUP }, { "rerere", cmd_rerere, RUN_SETUP }, { "reset", cmd_reset, RUN_SETUP }, + { "restore-paths", cmd_restore_paths, RUN_SETUP | NEED_WORK_TREE }, { "rev-list", cmd_rev_list, RUN_SETUP | NO_PARSEOPT }, { "rev-parse", cmd_rev_parse, NO_PARSEOPT }, { "revert", cmd_revert, RUN_SETUP | NEED_WORK_TREE }, @@ -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 }, diff --git a/parse-options-cb.c b/parse-options-cb.c index 8c9edce52f..c609d52926 100644 --- a/parse-options-cb.c +++ b/parse-options-cb.c @@ -126,7 +126,7 @@ struct option *parse_options_concat(struct option *a, struct option *b) struct option *ret; size_t i, a_len = 0, b_len = 0; - for (i = 0; a[i].type != OPTION_END; i++) + for (i = 0; a && a[i].type != OPTION_END; i++) a_len++; for (i = 0; b[i].type != OPTION_END; i++) b_len++;