| Message ID | 20181127165211.24763-7-pclouds@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce new commands switch-branch and checkout-files | expand |
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 },
On Wed, Nov 28, 2018 at 7:03 AM Junio C Hamano <gitster@pobox.com> wrote: > > 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. Since the other one is already "checkout-files", maybe this one could just be "checkout-branch". > 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). I would hope this becomes better defaults and being used 90% of time. Even though I know "git checkout" quite well, it still bites me from time to time. Having the right mental model is one thing. Having to think a bit every time to write "git checkout" with the right syntax, and whether you need "--" (that ambiguation problem can still bite you from time to time), is frankly something I'd rather avoid.
On Wed, Nov 28, 2018 at 7:31 AM Duy Nguyen <pclouds@gmail.com> wrote: > > On Wed, Nov 28, 2018 at 7:03 AM Junio C Hamano <gitster@pobox.com> wrote: > > > > 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. > > Since the other one is already "checkout-files", maybe this one could > just be "checkout-branch". I dislike the checkout-* names, as we already have checkout-index as plumbing, so it would be confusing as to which checkout-* command should be used when and why as it seems the co-index moves content *from index* to the working tree, but the co-files moves content *to files*, whereas checkout-branch is neither 'moving' to or from a branch but rather 'switching' to that branch.
On Wed, Nov 28, 2018 at 8:08 PM Stefan Beller <sbeller@google.com> wrote: > > On Wed, Nov 28, 2018 at 7:31 AM Duy Nguyen <pclouds@gmail.com> wrote: > > > > On Wed, Nov 28, 2018 at 7:03 AM Junio C Hamano <gitster@pobox.com> wrote: > > > > > > 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. > > > > Since the other one is already "checkout-files", maybe this one could > > just be "checkout-branch". > > I dislike the checkout-* names, as we already have checkout-index > as plumbing, so it would be confusing as to which checkout-* command > should be used when and why as it seems the co-index moves > content *from index* to the working tree, but the co-files moves content > *to files*, whereas checkout-branch is neither 'moving' to or from a branch > but rather 'switching' to that branch. OK back to square one. Another thing I noticed, not sure if it matters, is that these two commands will be the only ones with a hyphen in them in "git help". But I guess it's even harder to find one-word command names for these.
> Since the other one is already "checkout-files", maybe this one could just be "checkout-branch". I rather like switch-branch and dislike the word "checkout" since it has been overloaded in git for so long (does it mean moving HEAD or copying files to my working tree?) > nobody will become "sick of" the single "checkout" command that can I have to admit I'm already sick of the checkout command. :-p I can see myself using these two new commands 100% of the time and never missing the old one. Some behaviors I'd expect to see from these commands (I haven't yet checked to see if you've already done this): git checkout-files <tree-ish> should reset all the files in the repository regardless of the current directory - it should produce the same effect as "git reset --hard <tree-ish> && git reset HEAD@{1}". It should also delete locally-created files that aren't present in <tree-ish>, such that the final working tree is exactly identical to what was committed in that tree-ish. git checkout-files foo -- myfile.txt should delete myfile.txt if it is present locally but not present in foo. git checkout-files foo -- . should recursively checkout all files in the current folder and all subfolders, and delete any locally-created files if they're not present in foo. git checkout-files should never move HEAD in any circumstance. Suggestion: If git checkout-files overwrites or deletes any locally-modified files from the workspace or index, those files could be auto-stashed. That would make it easy to restore them in the event of a mistyped command. Auto-stashing could be suppressed with a command-line argument (with alternate behaviors being fail-if-modified or always-overwrite). Idea: If git checkout-files modifies the submodules file, it could also auto-update the submodules. (For example, with something like "git submodule update --init --recursive --progress"). - Stefan On Wed, Nov 28, 2018 at 7:31 AM Duy Nguyen <pclouds@gmail.com> wrote: > > On Wed, Nov 28, 2018 at 7:03 AM Junio C Hamano <gitster@pobox.com> wrote: > > > > 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. > > Since the other one is already "checkout-files", maybe this one could > just be "checkout-branch". > > > 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). > > I would hope this becomes better defaults and being used 90% of time. > Even though I know "git checkout" quite well, it still bites me from > time to time. Having the right mental model is one thing. Having to > think a bit every time to write "git checkout" with the right syntax, > and whether you need "--" (that ambiguation problem can still bite you > from time to time), is frankly something I'd rather avoid. > -- > Duy
Although I have no problem with "switch-branch" as a command name, some alternative names we might consider for switch-branch might be: chbranch swbranch switch branch change (as a subcommand for the "branch" command) I've personally been using "chbranch" as an alias for this functionality for some time. - Stefan On Wed, Nov 28, 2018 at 3:22 PM Stefan Xenos <sxenos@google.com> wrote: > > > Since the other one is already "checkout-files", maybe this one could just be "checkout-branch". > > I rather like switch-branch and dislike the word "checkout" since it > has been overloaded in git for so long (does it mean moving HEAD or > copying files to my working tree?) > > > nobody will become "sick of" the single "checkout" command that can > > I have to admit I'm already sick of the checkout command. :-p I can > see myself using these two new commands 100% of the time and never > missing the old one. > > Some behaviors I'd expect to see from these commands (I haven't yet > checked to see if you've already done this): > > git checkout-files <tree-ish> > should reset all the files in the repository regardless of the current > directory - it should produce the same effect as "git reset --hard > <tree-ish> && git reset HEAD@{1}". It should also delete > locally-created files that aren't present in <tree-ish>, such that the > final working tree is exactly identical to what was committed in that > tree-ish. > > git checkout-files foo -- myfile.txt > should delete myfile.txt if it is present locally but not present in foo. > > git checkout-files foo -- . > should recursively checkout all files in the current folder and all > subfolders, and delete any locally-created files if they're not > present in foo. > > git checkout-files should never move HEAD in any circumstance. > > Suggestion: > If git checkout-files overwrites or deletes any locally-modified files > from the workspace or index, those files could be auto-stashed. That > would make it easy to restore them in the event of a mistyped command. > Auto-stashing could be suppressed with a command-line argument (with > alternate behaviors being fail-if-modified or always-overwrite). > > Idea: > If git checkout-files modifies the submodules file, it could also > auto-update the submodules. (For example, with something like "git > submodule update --init --recursive --progress"). > > - Stefan > On Wed, Nov 28, 2018 at 7:31 AM Duy Nguyen <pclouds@gmail.com> wrote: > > > > On Wed, Nov 28, 2018 at 7:03 AM Junio C Hamano <gitster@pobox.com> wrote: > > > > > > 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. > > > > Since the other one is already "checkout-files", maybe this one could > > just be "checkout-branch". > > > > > 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). > > > > I would hope this becomes better defaults and being used 90% of time. > > Even though I know "git checkout" quite well, it still bites me from > > time to time. Having the right mental model is one thing. Having to > > think a bit every time to write "git checkout" with the right syntax, > > and whether you need "--" (that ambiguation problem can still bite you > > from time to time), is frankly something I'd rather avoid. > > -- > > Duy
More thoughts: git switch-branch should never detach HEAD unless asked to do so explicitly. That also means that "git switch-branch" shouldn't accept any of the non-branch tree-ish arguments that would have caused "git checkout" to do so. On Wed, Nov 28, 2018 at 3:26 PM Stefan Xenos <sxenos@google.com> wrote: > > Although I have no problem with "switch-branch" as a command name, > some alternative names we might consider for switch-branch might be: > > chbranch > swbranch > switch > branch change (as a subcommand for the "branch" command) > > I've personally been using "chbranch" as an alias for this > functionality for some time. > > - Stefan > On Wed, Nov 28, 2018 at 3:22 PM Stefan Xenos <sxenos@google.com> wrote: > > > > > Since the other one is already "checkout-files", maybe this one could just be "checkout-branch". > > > > I rather like switch-branch and dislike the word "checkout" since it > > has been overloaded in git for so long (does it mean moving HEAD or > > copying files to my working tree?) > > > > > nobody will become "sick of" the single "checkout" command that can > > > > I have to admit I'm already sick of the checkout command. :-p I can > > see myself using these two new commands 100% of the time and never > > missing the old one. > > > > Some behaviors I'd expect to see from these commands (I haven't yet > > checked to see if you've already done this): > > > > git checkout-files <tree-ish> > > should reset all the files in the repository regardless of the current > > directory - it should produce the same effect as "git reset --hard > > <tree-ish> && git reset HEAD@{1}". It should also delete > > locally-created files that aren't present in <tree-ish>, such that the > > final working tree is exactly identical to what was committed in that > > tree-ish. > > > > git checkout-files foo -- myfile.txt > > should delete myfile.txt if it is present locally but not present in foo. > > > > git checkout-files foo -- . > > should recursively checkout all files in the current folder and all > > subfolders, and delete any locally-created files if they're not > > present in foo. > > > > git checkout-files should never move HEAD in any circumstance. > > > > Suggestion: > > If git checkout-files overwrites or deletes any locally-modified files > > from the workspace or index, those files could be auto-stashed. That > > would make it easy to restore them in the event of a mistyped command. > > Auto-stashing could be suppressed with a command-line argument (with > > alternate behaviors being fail-if-modified or always-overwrite). > > > > Idea: > > If git checkout-files modifies the submodules file, it could also > > auto-update the submodules. (For example, with something like "git > > submodule update --init --recursive --progress"). > > > > - Stefan > > On Wed, Nov 28, 2018 at 7:31 AM Duy Nguyen <pclouds@gmail.com> wrote: > > > > > > On Wed, Nov 28, 2018 at 7:03 AM Junio C Hamano <gitster@pobox.com> wrote: > > > > > > > > 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. > > > > > > Since the other one is already "checkout-files", maybe this one could > > > just be "checkout-branch". > > > > > > > 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). > > > > > > I would hope this becomes better defaults and being used 90% of time. > > > Even though I know "git checkout" quite well, it still bites me from > > > time to time. Having the right mental model is one thing. Having to > > > think a bit every time to write "git checkout" with the right syntax, > > > and whether you need "--" (that ambiguation problem can still bite you > > > from time to time), is frankly something I'd rather avoid. > > > -- > > > Duy
Stefan Beller <sbeller@google.com> writes: > I dislike the checkout-* names, as we already have checkout-index > as plumbing, so it would be confusing as to which checkout-* command > should be used when and why as it seems the co-index moves > content *from index* to the working tree, but the co-files moves content > *to files*, whereas checkout-branch is neither 'moving' to or from a branch > but rather 'switching' to that branch. To me, "switching to work on the branch", is like "checking the book out from the library to read". IOW, "check the branch out to work on" does not have to involve *any* moving of contents.
Stefan Xenos <sxenos@google.com> writes: > Although I have no problem with "switch-branch" as a command name, > some alternative names we might consider for switch-branch might be: > > chbranch > swbranch Please never go in that direction. So far, we made a conscious effort to keep the names of most frequently used subcommand to proper words that can be understood by coders (IOW, I expect they know what 'grep' is, even though that may not be a 'proper word'). > switch > branch change (as a subcommand for the "branch" command) It is more like moving to the branch to work on. I think 'switch' is something SVN veterans may find familiar. Both are much better than ch/swbranch that are overly cryptic.
On Thu, Nov 29, 2018 at 6:59 AM Junio C Hamano <gitster@pobox.com> wrote: > > Stefan Xenos <sxenos@google.com> writes: > > > Although I have no problem with "switch-branch" as a command name, > > some alternative names we might consider for switch-branch might be: > > > > chbranch > > swbranch > > Please never go in that direction. So far, we made a conscious > effort to keep the names of most frequently used subcommand to > proper words that can be understood by coders (IOW, I expect they > know what 'grep' is, even though that may not be a 'proper word'). > > > switch > > branch change (as a subcommand for the "branch" command) > > It is more like moving to the branch to work on. I think 'switch' > is something SVN veterans may find familiar. Both are much better > than ch/swbranch that are overly cryptic. OK I'll go with switch-branch and restore-files in the next round. And perhaps consider just 'switch' and 'restore' later.
On Thu, Nov 29, 2018 at 12:22 AM Stefan Xenos <sxenos@google.com> wrote: > Some behaviors I'd expect to see from these commands (I haven't yet > checked to see if you've already done this): > > git checkout-files <tree-ish> > should reset all the files in the repository regardless of the current > directory - it should produce the same effect as "git reset --hard > <tree-ish> && git reset HEAD@{1}". It should also delete > locally-created files that aren't present in <tree-ish>, such that the > final working tree is exactly identical to what was committed in that > tree-ish. > > git checkout-files foo -- myfile.txt > should delete myfile.txt if it is present locally but not present in foo. > > git checkout-files foo -- . > should recursively checkout all files in the current folder and all > subfolders, and delete any locally-created files if they're not > present in foo. I think all these are the same as the non-overlay mode Thomas mentioned [1]. Once he implements that in git-checkout, we can make it default in checkout-files. Two things though. I plan to get rid of "--" in checkout-files. The main use case (I think) is reset from index, so you can just write git checkout-files somefiles and if you want to get it from the tree(-ish) "foo", you do git checkout-files --from=foo somefiles This form is easier to read (and even guess before you read man pages) and leaves no room for ambiguation. The second thing is, I plan to forbid "git checkout-files" without arguments. If you want to reset the entire worktree, do git checkout-files :/ or just current dir git checkout-files . Which brings us back to your "git checkout-files <tree-ish>" use case above. It should be treat the same way in my opinion, so we either do git checkout-files --from=tree-ish :/ or git checkout-files --from=tree-ish . But "git checkout-files --from=tree-ish" alone is rejected. [1] https://public-inbox.org/git/xmqqwoowo1fk.fsf@gitster-ct.c.googlers.com/T/#mdb076d178ccf0ae3dba0fd63143f99278047da93 > Suggestion: > If git checkout-files overwrites or deletes any locally-modified files > from the workspace or index, those files could be auto-stashed. That > would make it easy to restore them in the event of a mistyped command. > Auto-stashing could be suppressed with a command-line argument (with > alternate behaviors being fail-if-modified or always-overwrite). Stashing I think is not the right tool for this. When you stash, you plan to retrieve it back later but here you should rarely ever need to unstash until the accident. For recovery from accidents like this, I have another thing in queue to achieve the same (I'm calling it "backup log" now). So we will have the same functionality, just with different means. > Idea: > If git checkout-files modifies the submodules file, it could also > auto-update the submodules. (For example, with something like "git > submodule update --init --recursive --progress"). This one is tricky because we should deal with submodule autoupdate consistently across all porcelain commands (or at least common ones), not just checkout-files. I'd prefer to delay this until later. Once we figure out what to do with other commands, then we can still change defaults for checkout-files.
> > Idea: > > If git checkout-files modifies the submodules file, it could also > > auto-update the submodules. (For example, with something like "git > > submodule update --init --recursive --progress"). > > This one is tricky because we should deal with submodule autoupdate > consistently across all porcelain commands (or at least common ones), > not just checkout-files. I'd prefer to delay this until later. Once we > figure out what to do with other commands, then we can still change > defaults for checkout-files. checkout/reset are respecting the submodule.recurse setting for this already, and as your patches only change the UX frontend git -c submodule.recurse checkout-files <pathsspec> would also touch submodules. Given that deep down in the submodules it's all about files again, we could think checkout-files is still a good name. I think Stefan X. is asking for making submodule.recurse to default to true, which is indeed unrelated to this. Stefan
On Thu, Nov 29, 2018 at 7:14 PM Stefan Beller <sbeller@google.com> wrote: > > > > Idea: > > > If git checkout-files modifies the submodules file, it could also > > > auto-update the submodules. (For example, with something like "git > > > submodule update --init --recursive --progress"). > > > > This one is tricky because we should deal with submodule autoupdate > > consistently across all porcelain commands (or at least common ones), > > not just checkout-files. I'd prefer to delay this until later. Once we > > figure out what to do with other commands, then we can still change > > defaults for checkout-files. > > checkout/reset are respecting the submodule.recurse setting for this > already, and as your patches only change the UX frontend > > git -c submodule.recurse checkout-files <pathsspec> > > would also touch submodules. Given that deep down in > the submodules it's all about files again, we could think > checkout-files is still a good name. > > I think Stefan X. is asking for making submodule.recurse > to default to true, which is indeed unrelated to this. Yes and I'm concerned that checkout-files now recurses into submodules this by default but grep for example does not. That just adds more confusion.
> > Which brings us back to your "git checkout-files <tree-ish>" use case > above. It should be treat the same way in my opinion, so we either do > > git checkout-files --from=tree-ish :/ > > or > > git checkout-files --from=tree-ish . > > But "git checkout-files --from=tree-ish" alone is rejected. Agreed. Those arguments are better. The gist of my comment was the treatment of newly created local files rather than the form of the arguments, but it sounds like you've got that under control, too. > > Suggestion: > > If git checkout-files overwrites or deletes any locally-modified files > > from the workspace or index, those files could be auto-stashed. That > > would make it easy to restore them in the event of a mistyped command. > > Auto-stashing could be suppressed with a command-line argument (with > > alternate behaviors being fail-if-modified or always-overwrite). > > Stashing I think is not the right tool for this. When you stash, you > plan to retrieve it back later but here you should rarely ever need to > unstash until the accident. For recovery from accidents like this, I > have another thing in queue to achieve the same (I'm calling it > "backup log" now). So we will have the same functionality, just with > different means. Yes, this makes sense too. You wouldn't want to pollute the stash list with autogenerated things the user probably doesn't want. > This one is tricky because we should deal with submodule autoupdate > consistently across all porcelain commands (or at least common ones), > not just checkout-files. This is also a good point. I'd like it if submodules just behaved like a single giant repository for most commands, but you're right that this is something that should be done intentionally for all the commands at one rather than just for a single command. I also like your new names "switch-branch" and "restore-files".
diff --git a/builtin.h b/builtin.h index 6538932e99..d4a66e5f79 100644 --- a/builtin.h +++ b/builtin.h @@ -138,6 +138,7 @@ extern int cmd_branch(int argc, const char **argv, const char *prefix); extern int cmd_bundle(int argc, const char **argv, const char *prefix); extern int cmd_cat_file(int argc, const char **argv, const char *prefix); extern int cmd_checkout(int argc, const char **argv, const char *prefix); +extern int cmd_checkout_files(int argc, const char **argv, const char *prefix); extern int cmd_checkout_index(int argc, const char **argv, const char *prefix); extern int cmd_check_attr(int argc, const char **argv, const char *prefix); extern int cmd_check_ignore(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 d9dbd2d40d..c09d2da47a 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 checkout_files_usage[] = { + N_("git checkout-files [<options>] [<branch>] -- <file>..."), + NULL, +}; + struct checkout_opts { int patch_mode; int quiet; @@ -45,6 +55,8 @@ struct checkout_opts { int ignore_other_worktrees; int show_progress; int dwim_new_local_branch; + int accept_pathspec; + int empty_arg_ok; /* * 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 },
"git checkout" doing too many things is a source of confusion for many users (and it even bites old timers sometimes). To rememdy that, the command is now split in two: switch-branch and checkout-files. The switch-branch command is all about switching branches, detaching, DWIM-ing new branch... It does not accept pathspecs and it always requires a ref (in contrast, "git checkout" without arguments works) The checkout-files command on the other hand is all about resetting certain files in worktree, either from the index or from a specific tree. It could accept a tree-ish, but it will never touch HEAD or the ref it points to. The good old "git checkout" command is still here and will be until all (or most of users) are sick of it. Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- builtin.h | 2 + builtin/checkout.c | 91 +++++++++++++++++++++++++++++++++++++++------- git.c | 2 + 3 files changed, 82 insertions(+), 13 deletions(-)