[v2,6/7] checkout: split into switch-branch and checkout-files
diff mbox series

Message ID 20181127165211.24763-7-pclouds@gmail.com
State New, archived
Headers show
Series
  • Introduce new commands switch-branch and checkout-files
Related show

Commit Message

Duy Nguyen Nov. 27, 2018, 4:52 p.m. UTC
"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(-)

Comments

Junio C Hamano Nov. 28, 2018, 6:03 a.m. UTC | #1
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 },
Duy Nguyen Nov. 28, 2018, 3:30 p.m. UTC | #2
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.
Stefan Beller Nov. 28, 2018, 7:08 p.m. UTC | #3
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.
Duy Nguyen Nov. 28, 2018, 7:18 p.m. UTC | #4
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.
Stefan Xenos Nov. 28, 2018, 11:22 p.m. UTC | #5
> 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 Xenos Nov. 28, 2018, 11:26 p.m. UTC | #6
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 Xenos Nov. 28, 2018, 11:37 p.m. UTC | #7
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
Junio C Hamano Nov. 29, 2018, 5:55 a.m. UTC | #8
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.
Junio C Hamano Nov. 29, 2018, 5:59 a.m. UTC | #9
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.
Duy Nguyen Nov. 29, 2018, 3:36 p.m. UTC | #10
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.
Duy Nguyen Nov. 29, 2018, 3:46 p.m. UTC | #11
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.
Stefan Beller Nov. 29, 2018, 6:14 p.m. UTC | #12
> > 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
Duy Nguyen Nov. 29, 2018, 6:30 p.m. UTC | #13
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.
Stefan Xenos Nov. 29, 2018, 7:29 p.m. UTC | #14
>
> 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".

Patch
diff mbox series

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 },