| Message ID | 20190308101655.9767-3-pclouds@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | And new command "restore" | expand |
On Fri, Mar 8, 2019 at 2:17 AM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote: > > This is another departure from 'git checkout' syntax, which uses -- to > separate ref and pathspec. The observation is restore (or "git > checkout ,, <pathspec>") is most often used to restore some files from > the index. If this is correct, we can simplify it by taking a way the > ref, so that we can write > > git restore some-file > > without worrying about some-file being a ref and whether we need to do > > git restore -- some-file > > for safety. If the source of the restore comes from a tree, it will be > in the form of an option with value, e.g. > > git restore --source=this-tree some-file > > This is of course longer to type than using "--". But hopefully it > will not be used as often, and it is clearly easier to understand. > > dwim_new_local_branch is no longer set (or unset) in cmd_restore_files() > because it's irrelevant because we don't really care about dwim-ing. > With accept_ref being unset, dwim can't happen. Nice. :-) > > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > --- > builtin/checkout.c | 42 ++++++++++++++++++++++++++++++++++-------- > 1 file changed, 34 insertions(+), 8 deletions(-) > > diff --git a/builtin/checkout.c b/builtin/checkout.c > index 11dd2ae44c..838343d6aa 100644 > --- a/builtin/checkout.c > +++ b/builtin/checkout.c > @@ -38,7 +38,7 @@ static const char * const switch_branch_usage[] = { > }; > > static const char * const restore_files_usage[] = { restore_files_usage or restore_usage? > - N_("git restore [<options>] [<branch>] -- <file>..."), > + N_("git restore [<options>] [--source=<branch>] <file>..."), > NULL, > }; > > @@ -57,6 +57,7 @@ struct checkout_opts { > int count_checkout_paths; > int overlay_mode; > int dwim_new_local_branch; > + int accept_ref; > int accept_pathspec; > int switch_branch_doing_nothing_is_ok; > int only_merge_on_switching_branches; > @@ -72,6 +73,7 @@ struct checkout_opts { > int branch_exists; > const char *prefix; > struct pathspec pathspec; > + const char *from_treeish; > struct tree *source_tree; > }; > > @@ -1324,6 +1326,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix, > { > struct branch_info new_branch_info; > int dwim_remotes_matched = 0; > + int parseopt_flags = 0; > > memset(&new_branch_info, 0, sizeof(new_branch_info)); > opts->overwrite_ignore = 1; > @@ -1335,8 +1338,13 @@ static int checkout_main(int argc, const char **argv, const char *prefix, > > opts->track = BRANCH_TRACK_UNSPECIFIED; > > - argc = parse_options(argc, argv, prefix, options, usagestr, > - PARSE_OPT_KEEP_DASHDASH); > + if (!opts->accept_pathspec && !opts->accept_ref) > + BUG("make up your mind, you need to take _something_"); hehe. I secretly hope that someone eventually hits that, preferably another git developer making some changes and not an end user, though. > + if (opts->accept_pathspec && opts->accept_ref) > + parseopt_flags = PARSE_OPT_KEEP_DASHDASH; > + > + argc = parse_options(argc, argv, prefix, options, > + usagestr, parseopt_flags); > > if (opts->show_progress < 0) { > if (opts->quiet) > @@ -1393,7 +1401,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix, > * including "last branch" syntax and DWIM-ery for names of > * remote branches, erroring out for invalid or ambiguous cases. > */ > - if (argc) { > + if (argc && opts->accept_ref) { > struct object_id rev; > int dwim_ok = > !opts->patch_mode && > @@ -1405,6 +1413,18 @@ static int checkout_main(int argc, const char **argv, const char *prefix, > &dwim_remotes_matched); > argv += n; > argc -= n; > + } else if (!opts->accept_ref && opts->from_treeish) { > + struct object_id rev; > + > + if (get_oid_mb(opts->from_treeish, &rev)) Going on a slight tangent from your series: get_oid_mb really deserves a comment somewhere that "mb" is "merge base". I had to spelunk through history to find it... > + die(_("could not resolve %s"), opts->from_treeish); > + > + setup_new_branch_info_and_source_tree(&new_branch_info, > + opts, &rev, > + opts->from_treeish); > + > + if (!opts->source_tree) > + die(_("reference is not a tree: %s"), opts->from_treeish); > } > > if (argc) { > @@ -1488,6 +1508,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix) > opts.dwim_new_local_branch = 1; > opts.switch_branch_doing_nothing_is_ok = 1; > opts.only_merge_on_switching_branches = 0; > + opts.accept_ref = 1; > opts.accept_pathspec = 1; > opts.implicit_detach = 1; > > @@ -1519,6 +1540,7 @@ int cmd_switch(int argc, const char **argv, const char *prefix) > > memset(&opts, 0, sizeof(opts)); > opts.dwim_new_local_branch = 0; > + opts.accept_ref = 1; > opts.accept_pathspec = 0; > opts.switch_branch_doing_nothing_is_ok = 0; > opts.only_merge_on_switching_branches = 1; > @@ -1537,15 +1559,19 @@ int cmd_switch(int argc, const char **argv, const char *prefix) > int cmd_restore(int argc, const char **argv, const char *prefix) > { > struct checkout_opts opts; > - struct option *options = NULL; > + struct option *options; > + struct option restore_options[] = { > + OPT_STRING('s', "source", &opts.from_treeish, "<tree-ish>", > + N_("where the checkout from")), > + OPT_END() > + }; > int ret; > > memset(&opts, 0, sizeof(opts)); > - opts.dwim_new_local_branch = 1; > - opts.switch_branch_doing_nothing_is_ok = 0; > + opts.accept_ref = 0; > opts.accept_pathspec = 1; > > - options = parse_options_dup(options); > + options = parse_options_dup(restore_options); > options = add_common_options(&opts, options); > options = add_checkout_path_options(&opts, options); > > -- > 2.21.0.rc1.337.gdf7f8d0522 >
On Fri, Mar 8, 2019 at 5:17 AM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote: > This is another departure from 'git checkout' syntax, which uses -- to > separate ref and pathspec. The observation is restore (or "git > checkout ,, <pathspec>") is most often used to restore some files from What is the ",," thing? > the index. If this is correct, we can simplify it by taking a way the s/a way/away/ > ref, so that we can write > > git restore some-file > > without worrying about some-file being a ref and whether we need to do > > git restore -- some-file > > for safety. If the source of the restore comes from a tree, it will be > in the form of an option with value, e.g. > > git restore --source=this-tree some-file > > This is of course longer to type than using "--". But hopefully it > will not be used as often, and it is clearly easier to understand. > > dwim_new_local_branch is no longer set (or unset) in cmd_restore_files() > because it's irrelevant because we don't really care about dwim-ing. > With accept_ref being unset, dwim can't happen. > > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
diff --git a/builtin/checkout.c b/builtin/checkout.c index 11dd2ae44c..838343d6aa 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -38,7 +38,7 @@ static const char * const switch_branch_usage[] = { }; static const char * const restore_files_usage[] = { - N_("git restore [<options>] [<branch>] -- <file>..."), + N_("git restore [<options>] [--source=<branch>] <file>..."), NULL, }; @@ -57,6 +57,7 @@ struct checkout_opts { int count_checkout_paths; int overlay_mode; int dwim_new_local_branch; + int accept_ref; int accept_pathspec; int switch_branch_doing_nothing_is_ok; int only_merge_on_switching_branches; @@ -72,6 +73,7 @@ struct checkout_opts { int branch_exists; const char *prefix; struct pathspec pathspec; + const char *from_treeish; struct tree *source_tree; }; @@ -1324,6 +1326,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix, { struct branch_info new_branch_info; int dwim_remotes_matched = 0; + int parseopt_flags = 0; memset(&new_branch_info, 0, sizeof(new_branch_info)); opts->overwrite_ignore = 1; @@ -1335,8 +1338,13 @@ static int checkout_main(int argc, const char **argv, const char *prefix, opts->track = BRANCH_TRACK_UNSPECIFIED; - argc = parse_options(argc, argv, prefix, options, usagestr, - PARSE_OPT_KEEP_DASHDASH); + if (!opts->accept_pathspec && !opts->accept_ref) + BUG("make up your mind, you need to take _something_"); + if (opts->accept_pathspec && opts->accept_ref) + parseopt_flags = PARSE_OPT_KEEP_DASHDASH; + + argc = parse_options(argc, argv, prefix, options, + usagestr, parseopt_flags); if (opts->show_progress < 0) { if (opts->quiet) @@ -1393,7 +1401,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix, * including "last branch" syntax and DWIM-ery for names of * remote branches, erroring out for invalid or ambiguous cases. */ - if (argc) { + if (argc && opts->accept_ref) { struct object_id rev; int dwim_ok = !opts->patch_mode && @@ -1405,6 +1413,18 @@ static int checkout_main(int argc, const char **argv, const char *prefix, &dwim_remotes_matched); argv += n; argc -= n; + } else if (!opts->accept_ref && opts->from_treeish) { + struct object_id rev; + + if (get_oid_mb(opts->from_treeish, &rev)) + die(_("could not resolve %s"), opts->from_treeish); + + setup_new_branch_info_and_source_tree(&new_branch_info, + opts, &rev, + opts->from_treeish); + + if (!opts->source_tree) + die(_("reference is not a tree: %s"), opts->from_treeish); } if (argc) { @@ -1488,6 +1508,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix) opts.dwim_new_local_branch = 1; opts.switch_branch_doing_nothing_is_ok = 1; opts.only_merge_on_switching_branches = 0; + opts.accept_ref = 1; opts.accept_pathspec = 1; opts.implicit_detach = 1; @@ -1519,6 +1540,7 @@ int cmd_switch(int argc, const char **argv, const char *prefix) memset(&opts, 0, sizeof(opts)); opts.dwim_new_local_branch = 0; + opts.accept_ref = 1; opts.accept_pathspec = 0; opts.switch_branch_doing_nothing_is_ok = 0; opts.only_merge_on_switching_branches = 1; @@ -1537,15 +1559,19 @@ int cmd_switch(int argc, const char **argv, const char *prefix) int cmd_restore(int argc, const char **argv, const char *prefix) { struct checkout_opts opts; - struct option *options = NULL; + struct option *options; + struct option restore_options[] = { + OPT_STRING('s', "source", &opts.from_treeish, "<tree-ish>", + N_("where the checkout from")), + OPT_END() + }; int ret; memset(&opts, 0, sizeof(opts)); - opts.dwim_new_local_branch = 1; - opts.switch_branch_doing_nothing_is_ok = 0; + opts.accept_ref = 0; opts.accept_pathspec = 1; - options = parse_options_dup(options); + options = parse_options_dup(restore_options); options = add_common_options(&opts, options); options = add_checkout_path_options(&opts, options);
This is another departure from 'git checkout' syntax, which uses -- to separate ref and pathspec. The observation is restore (or "git checkout ,, <pathspec>") is most often used to restore some files from the index. If this is correct, we can simplify it by taking a way the ref, so that we can write git restore some-file without worrying about some-file being a ref and whether we need to do git restore -- some-file for safety. If the source of the restore comes from a tree, it will be in the form of an option with value, e.g. git restore --source=this-tree some-file This is of course longer to type than using "--". But hopefully it will not be used as often, and it is clearly easier to understand. dwim_new_local_branch is no longer set (or unset) in cmd_restore_files() because it's irrelevant because we don't really care about dwim-ing. With accept_ref being unset, dwim can't happen. Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- builtin/checkout.c | 42 ++++++++++++++++++++++++++++++++++-------- 1 file changed, 34 insertions(+), 8 deletions(-)