checkout: disambiguate dwim tracking branches and local files
PATCH 1/1 checkout: disambiguate dwim tracking branches and local files
When checkout dwim is added in [1], it is restricted to only dwim when certain conditions are met and fall back to default checkout behavior otherwise. It turns out falling back could be confusing. One of the conditions to turn
git checkout frotz
to
git checkout -b frotz origin/frotz
is that frotz must not exist as a file. But when the user comes to expect "git checkout frotz" to create the branch "frotz" and there happens to be a file named "frotz", git's silently reverting "frotz" file content is not helping. This is reported in Git mailing list [2] and even used as an example of "Git is bad" elsewhere [3].
We normally try to do the right thing, but when there are multiple "right things" to do, it's best to leave it to the user to decide. Check this case, ask the user to use "--" to disambiguate.
[1] 70c9ac2f19 (DWIM "git checkout frotz" to "git checkout -b frotz
origin/frotz" - 2009-10-18)
[2] https://public-inbox.org/git/CACsJy8B2TVr1g+k+eSQ=pBEO3WN4_LtgLo9gpur8X7Z9GOFL_A@mail.gmail.com/
[3] https://news.ycombinator.com/item?id=18230655
--- builtin/checkout.c | 15 ++++++++++++--- t/t2024-checkout-dwim.sh | 31 +++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 3 deletions(-)
builtin/checkout.c +12 -3
@@ -1079,9 +1079,15 @@ static int parse_branchname_arg(int argc, const char **argv,*/ int recover_with_dwim = dwim_new_local_branch_ok; -if (!has_dash_dash && - (check_filename (opts->prefix, arg) || !no_wildcard (arg))) - recover_with_dwim =0 ; +/* +* If both refs/remotes/origin/master and the file +* 'master'. Don't blindly go for 'master' file +* because it's ambiguous. Leave it for the user to +* decide. +*/ +int disambi_local_file = !has_dash_dash && + (check_filename (opts->prefix, arg) || !no_wildcard (arg)); +/* * Accept "git checkout foo" and "git checkout foo --" * as candidates for dwim.
@@ -1094,6 +1100,9 @@ static int parse_branchname_arg(int argc, const char **argv,const char *remote =unique_tracking_name (arg, rev, dwim_remotes_matched);if (remote) { +if (disambi_local_file) +die (_ ("'%s' could be both a local file and a tracking branch.\n" +"Please use -- to disambiguate" ), arg); *new_branch = arg; arg = remote;/* DWIMmed to create local branch, case (3).(b) */
t/t2024-checkout-dwim.sh +31 -0
@@ -278,4 +278,35 @@ test_expect_success 'loosely defined local base branch is reported correctly' ' test_cmp expect actual ' +test_expect_success 'reject when arg could be part of dwim branch' ' + git remote add foo file://non-existent-place && + git update-ref refs/remotes/foo/dwim-arg HEAD && + echo foo >dwim-arg && + git add dwim-arg && + echo bar >dwim-arg && + test_must_fail git checkout dwim-arg && + test_must_fail git rev-parse refs/heads/dwim-arg -- && + grep bar dwim-arg +' + +test_expect_success 'disambiguate dwim branch and checkout path (1)' ' + git update-ref refs/remotes/foo/dwim-arg1 HEAD && + echo foo >dwim-arg1 && + git add dwim-arg1 && + echo bar >dwim-arg1 && + git checkout -- dwim-arg1 && + test_must_fail git rev-parse refs/heads/dwim-arg1 -- && + grep foo dwim-arg1 +' + +test_expect_success 'disambiguate dwim branch and checkout path (2)' ' + git update-ref refs/remotes/foo/dwim-arg2 HEAD && + echo foo >dwim-arg2 && + git add dwim-arg2 && + echo bar >dwim-arg2 && + git checkout dwim-arg2 -- && + git rev-parse refs/heads/dwim-arg2 -- && + grep bar dwim-arg2 +' + test_done
Signature · 1 line
2.19.1.1231.g84aef82467
Quoted 5 lines · Nguyễn Thái Ngọc Duy
builtin/checkout.c +0
-1@@ -1079,9 +1079,15 @@ static int parse_branchname_arg(int argc, const char **argv,*/ int recover_with_dwim = dwim_new_local_branch_ok; - if (!has_dash_dash &&
builtin/checkout.c static int parse_branchname_arg(int argc, const char **argv, · line 1089
Quoted 10 lines
builtin/checkout.c +8
-2- (check_filename(opts->prefix, arg) || !no_wildcard(arg))) - recover_with_dwim =0 ; +/* +* If both refs/remotes/origin/master and the file +* 'master'. Don't blindly go for 'master' file +* because it's ambiguous. Leave it for the user to +* decide. +*/ +int disambi_local_file = !has_dash_dash && + (check_filename (opts->prefix, arg) || !no_wildcard (arg));
What you are computing on the right hand side is if the arg is ambiguous. And the code that looks at this variable does not disambiguate, but it just punts and makes it responsibility to the user (which is by the way the correct thing to do).
When a file with exact name is in the working tree, we do not declare it is a pathspec and not a rev, as we may be allowed to dwim and create a rev with that name and at that point we'd be in an ambigous situation. If the arg _has_ wildcard, however, it is a strong sign that it *is* a pathspec, isn't it? It is iffy that a single variable that cannot be used to distinguish these two cases is introduced---one of these cases will be mishandled.
Also how does the patch ensures that this new logic does not kick in for those who refuse to let the command dwim to create a new branch?
builtin/checkout.c static int parse_branchname_arg(int argc, const char **argv, · line 1105
Quoted 10 lines
builtin/checkout.c +3
-0/* * Accept "git checkout foo" and "git checkout foo --" * as candidates for dwim. @@ -1094,6 +1100,9 @@ static int parse_branchname_arg(int argc, const char **argv,const char *remote =unique_tracking_name (arg, rev, dwim_remotes_matched);if (remote) { +if (disambi_local_file) +die (_ ("'%s' could be both a local file and a tracking branch.\n" +"Please use -- to disambiguate" ), arg);
Ah, the only user of this variable triggers when recover_with_dwim is true, so that is how dwim-less case is handled, OK.
That still leaves the question if it is OK to handle these two cases the same way in a repository without 'next' branch whose origin has one:
$ >next; git checkout --guess next
$ >next; git checkout --guess 'n??t'Perhaps the variable should be named "local_file_crashes_with_rev" and its the scope narrowed to the same block as "remote" is computed. Or just remove the variable and check the condition right there where you need to. I.e.
if (remote) {
if (!has_dash_dash &&
check_filename(opts->prefix, arg))
die("did you mean a branch or path?: '%s'", arg);
...Quoted 24 lines · Junio C Hamano
Quoted 5 lines · Nguyễn Thái Ngọc Duy
builtin/checkout.c +0
-1@@ -1079,9 +1079,15 @@ static int parse_branchname_arg(int argc, const char **argv,*/ int recover_with_dwim = dwim_new_local_branch_ok; - if (!has_dash_dash &&builtin/checkout.c static int parse_branchname_arg(int argc, const char **argv, · line 1089
Quoted 10 lines
builtin/checkout.c +8
-2- (check_filename(opts->prefix, arg) || !no_wildcard(arg))) - recover_with_dwim =0 ; +/* +* If both refs/remotes/origin/master and the file +* 'master'. Don't blindly go for 'master' file +* because it's ambiguous. Leave it for the user to +* decide. +*/ +int disambi_local_file = !has_dash_dash && + (check_filename (opts->prefix, arg) || !no_wildcard (arg));What you are computing on the right hand side is if the arg is ambiguous. And the code that looks at this variable does not disambiguate, but it just punts and makes it responsibility to the user (which is by the way the correct thing to do).
Quoted 7 lines
When a file with exact name is in the working tree, we do not declare it is a pathspec and not a rev, as we may be allowed to dwim and create a rev with that name and at that point we'd be in an ambigous situation. If the arg _has_ wildcard, however, it is a strong sign that it *is* a pathspec, isn't it? It is iffy that a single variable that cannot be used to distinguish these two cases is introduced---one of these cases will be mishandled.
Is it that different between an exact path name and a pathspec? Suppose it is a pathspec (with wildcards) that matches some paths, and we happen to have the remote branch origin/<that-pathspec>, then it is still ambiguous whether we should go create branch "<that-pathspec>" or go check out the paths matched by the pathspec.
Quoted 2 lines
Also how does the patch ensures that this new logic does not kick in for those who refuse to let the command dwim to create a new branch?
I would hope that it would be "--" to settle all disputes. But it looks like "git checkout foo --" is explicitly a candidate for dwim. We have a hidden option --no-guess to disable dwim. Maybe it's a good idea to make that one visible. It's at least clearer than using "--" even if "--" worked on this case.
Quoted 27 lines
builtin/checkout.c static int parse_branchname_arg(int argc, const char **argv, · line 1105
Quoted 11 lines
builtin/checkout.c +3
-0/* * Accept "git checkout foo" and "git checkout foo --" * as candidates for dwim. @@ -1094,6 +1100,9 @@ static int parse_branchname_arg(int argc, const char **argv,const char *remote =unique_tracking_name (arg, rev, dwim_remotes_matched);if (remote) { +if (disambi_local_file) +die (_ ("'%s' could be both a local file and a tracking branch.\n" +"Please use -- to disambiguate" ), arg);Ah, the only user of this variable triggers when
recover_with_dwimis true, so that is how dwim-less case is handled, OK.That still leaves the question if it is OK to handle these two cases the same way in a repository without 'next' branch whose origin has one:
$ >next; git checkout --guess next $ >next; git checkout --guess 'n??t'Perhaps the variable should be named "
local_file_crashes_with_rev" and its the scope narrowed to the same block as "remote" is computed. Or just remove the variable and check the condition right there where you need to. I.e.
Quoted 6 lines
if (remote) {
if (!has_dash_dash &&
check_filename(opts->prefix, arg))
die("did you mean a branch or path?: '%s'", arg);
...I still think handing both cases the same way is right. In the second case we would not find 'origin/n??t' so we fall back to checking out pathspec 'n??t' anyway (expected from you, I think). But just in case the remote branch 'origin/n??t' exists, we kick and scream.
I think you see 'n??t' as a pathspec, but I'm thinking about a user who sees 'n??t' as a branch name, not pathspec and he would have a different expectation.
Signature · 1 line
Duy
Quoted 5 lines · Duy Nguyen
Is it that different between an exact path name and a pathspec? Suppose it is a pathspec (with wildcards) that matches some paths, and we happen to have the remote branch origin/<that-pathspec>, then it is still ambiguous whether we should go create branch "<that-pathspec>" or go check out the paths matched by the pathspec.
:
A huge difference between these two
$ echo Hello >'n??t' && git add 'n??t' $ git branch 'n??t'
is that the former is taken and the latter is rejected, even though neither of them is particularly a sane or a likely thing.
Isn't that a good enough reason why
$ git checkout 'n??t'
cannot mean checking 'n??t' branch out, be it either an existing local one or auto-vivified one out of a remote-tracking branch?
Quoted 3 lines
I think you see 'n??t' as a pathspec, but I'm thinking about a user who sees 'n??t' as a branch name, not pathspec and he would have a different expectation.
See above for the reason why I think there is no room for different expectations to come in the picture in this case.
v2 cover disambiguate dwim tracking branches and local files
In reply to Nguyễn Thái Ngọc Duyv2 leaves "pathspec with wildcard" case alone. The behavior in this case remains as before.
--no-guess is now made visible in "git checkout -h" and the man page.
PS. Based on git-checkout.txt I don't think any user can work out that "git checkout branch --" can be used to disambiguate. And updating the doc to show that makes it a lot uglier.
Perhaps it's time we add two new semi-aliases, switch-branch and restore-path. They will have the same syntax as checkout in their respective use case, without ambiguation. Semi-aliases because like builtin commands, they cannot be overriden by user aliases.
Nguyễn Thái Ngọc Duy (1):
checkout: disambiguate dwim tracking branches and local files
Documentation/git-checkout.txt | 4 ++++ builtin/checkout.c | 18 +++++++++++++----- t/t2024-checkout-dwim.sh | 31 +++++++++++++++++++++++++++++++ t/t9902-completion.sh | 3 ++- 4 files changed, 50 insertions(+), 6 deletions(-)
Range-diff against v1:
1: 530f5d8f03 ! 1: 0408fdde4d checkout: disambiguate dwim tracking branches and local files
@@ -21,12 +21,36 @@
We normally try to do the right thing, but when there are multiple "right things" to do, it's best to leave it to the user to decide.
- Check this case, ask the user to use "--" to disambiguate.
+ Check this case, ask the user to to disambiguate:
+
+ - "git checkout -- foo" will check out path "foo"
+ - "git checkout foo --" will dwim and create branch "foo" [4]
+
+ For users who do not want dwim, use --no-guess. It's useless in this
+ particular case because "git checkout --no-guess foo --" will just
+ fail. But it could be used by scripts.
[1] 70c9ac2f19 (DWIM "git checkout frotz" to "git checkout -b frotz
origin/frotz" - 2009-10-18)
[2] https://public-inbox.org/git/CACsJy8B2TVr1g+k+eSQ=pBEO3WN4_LtgLo9gpur8X7Z9GOFL_A@mail.gmail.com/
[3] https://news.ycombinator.com/item?id=18230655
+ [4] a047fafc78 (checkout: allow dwim for branch creation for "git
+ checkout $branch --" - 2013-10-18)
+
+ diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
+ --- a/Documentation/git-checkout.txt
+ +++ b/Documentation/git-checkout.txt
+@@
+ Just like linkgit:git-submodule[1], this will detach the
+ submodules HEAD.
+
++--no-guess::
++ Do not attempt to create a branch if a remote tracking branch
++ of the same name exists.
++
+ <branch>::
+ Branch to checkout; if it refers to a branch (i.e., a name that,
+ when prepended with "refs/heads/", is a valid ref), then that
diff --git a/builtin/checkout.c b/builtin/checkout.c --- a/builtin/checkout.c
@@ -37,9 +61,11 @@
- if (!has_dash_dash &&
- (check_filename(opts->prefix, arg) || !no_wildcard(arg)))
-- recover_with_dwim = 0;
+ int could_be_checkout_paths = !has_dash_dash &&
-+ (check_filename(opts->prefix, arg) || !no_wildcard(arg));
++ check_filename(opts->prefix, arg);
++
++ if (!has_dash_dash && !no_wildcard(arg))
+ recover_with_dwim = 0;
+
/*
* Accept "git checkout foo" and "git checkout foo --"
@@ -50,10 +76,39 @@
if (remote) {
+ if (could_be_checkout_paths)
+ die(_("'%s' could be both a local file and a tracking branch.\n"
-+ "Please use -- to disambiguate"), arg);
++ "Please use -- (and optionally --no-guess) to disambiguate"),
++ arg);
*new_branch = arg;
arg = remote;
/* DWIMmed to create local branch, case (3).(b) */
+@@
+ struct checkout_opts opts;
+ struct branch_info new_branch_info;
+ char *conflict_style = NULL;
+- int dwim_new_local_branch = 1;
++ int dwim_new_local_branch, no_dwim_new_local_branch = 0;
+ int dwim_remotes_matched = 0;
+ struct option options[] = {
+ OPT__QUIET(&opts.quiet, N_("suppress progress reporting")),
+@@
+ OPT_BOOL('p', "patch", &opts.patch_mode, N_("select hunks interactively")),
+ OPT_BOOL(0, "ignore-skip-worktree-bits", &opts.ignore_skipworktree,
+ N_("do not limit pathspecs to sparse entries only")),
+- OPT_HIDDEN_BOOL(0, "guess", &dwim_new_local_branch,
+- N_("second guess 'git checkout <no-such-branch>'")),
++ OPT_BOOL(0, "no-guess", &no_dwim_new_local_branch,
++ N_("do not second guess 'git checkout <no-such-branch>'")),
+ OPT_BOOL(0, "ignore-other-worktrees", &opts.ignore_other_worktrees,
+ N_("do not check if another worktree is holding the given ref")),
+ { OPTION_CALLBACK, 0, "recurse-submodules", NULL,
+@@
+ argc = parse_options(argc, argv, prefix, options, checkout_usage,
+ PARSE_OPT_KEEP_DASHDASH);
+
++ dwim_new_local_branch = !no_dwim_new_local_branch;
+ if (opts.show_progress < 0) {
+ if (opts.quiet)
+ opts.show_progress = 0;
diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh --- a/t/t2024-checkout-dwim.sh
@@ -94,3 +149,17 @@
+'
+
test_done
+
+ diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
+ --- a/t/t9902-completion.sh
+ +++ b/t/t9902-completion.sh
+@@
+ --ignore-other-worktrees Z
+ --recurse-submodules Z
+ --progress Z
+- --no-quiet Z
++ --guess Z
++ --no-guess Z
+ --no-... Z
+ EOF
+ '
Signature · 1 line
2.19.1.1318.g5295c6727d
PATCH v2 1/1 checkout: disambiguate dwim tracking branches and local files
When checkout dwim is added in [1], it is restricted to only dwim when certain conditions are met and fall back to default checkout behavior otherwise. It turns out falling back could be confusing. One of the conditions to turn
git checkout frotz
to
git checkout -b frotz origin/frotz
is that frotz must not exist as a file. But when the user comes to expect "git checkout frotz" to create the branch "frotz" and there happens to be a file named "frotz", git's silently reverting "frotz" file content is not helping. This is reported in Git mailing list [2] and even used as an example of "Git is bad" elsewhere [3].
We normally try to do the right thing, but when there are multiple "right things" to do, it's best to leave it to the user to decide. Check this case, ask the user to to disambiguate:
- "git checkout -- foo" will check out path "foo"
- "git checkout foo --" will dwim and create branch "foo" [4]
For users who do not want dwim, use --no-guess. It's useless in this particular case because "git checkout --no-guess foo --" will just fail. But it could be used by scripts.
[1] 70c9ac2f19 (DWIM "git checkout frotz" to "git checkout -b frotz
origin/frotz" - 2009-10-18) [2] https://public-inbox.org/git/CACsJy8B2TVr1g+k+eSQ=pBEO3WN4_LtgLo9gpur8X7Z9GOFL_A@mail.gmail.com/ [3] https://news.ycombinator.com/item?id=18230655 [4] a047fafc78 (checkout: allow dwim for branch creation for "git checkout $branch --" - 2013-10-18)
--- Documentation/git-checkout.txt | 4 ++++ builtin/checkout.c | 18 +++++++++++++----- t/t2024-checkout-dwim.sh | 31 +++++++++++++++++++++++++++++++ t/t9902-completion.sh | 3 ++- 4 files changed, 50 insertions(+), 6 deletions(-)
Documentation/git-checkout.txt +4 -0
@@ -276,6 +276,10 @@ section of linkgit:git-add[1] to learn how to operate the `--patch` mode. Just like linkgit:git-submodule[1], this will detach the submodules HEAD. +--no-guess:: + Do not attempt to create a branch if a remote tracking branch + of the same name exists. + <branch>:: Branch to checkout; if it refers to a branch (i.e., a name that, when prepended with "refs/heads/", is a valid ref), then that
builtin/checkout.c +13 -5
@@ -1079,9 +1079,12 @@ static int parse_branchname_arg(int argc, const char **argv,*/ int recover_with_dwim = dwim_new_local_branch_ok; -if (!has_dash_dash && - (check_filename (opts->prefix, arg) || !no_wildcard (arg))) +int could_be_checkout_paths = !has_dash_dash && +check_filename (opts->prefix, arg); + +if (!has_dash_dash && !no_wildcard (arg)) recover_with_dwim =0 ; +/* * Accept "git checkout foo" and "git checkout foo --" * as candidates for dwim.
@@ -1094,6 +1097,10 @@ static int parse_branchname_arg(int argc, const char **argv,const char *remote =unique_tracking_name (arg, rev, dwim_remotes_matched);if (remote) { +if (could_be_checkout_paths) +die (_ ("'%s' could be both a local file and a tracking branch.\n" +"Please use -- (and optionally --no-guess) to disambiguate" ), + arg); *new_branch = arg; arg = remote;/* DWIMmed to create local branch, case (3).(b) */
@@ -1228,7 +1235,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)struct checkout_opts opts;struct branch_info new_branch_info;char *conflict_style =NULL ; -int dwim_new_local_branch =1 ; +int dwim_new_local_branch, no_dwim_new_local_branch =0 ;int dwim_remotes_matched =0 ;struct option options[] = {OPT__QUIET (&opts.quiet,N_ ("suppress progress reporting" )),
@@ -1258,8 +1265,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)OPT_BOOL ('p' ,"patch" , &opts.patch_mode,N_ ("select hunks interactively" )),OPT_BOOL (0 ,"ignore-skip-worktree-bits" , &opts.ignore_skipworktree,N_ ("do not limit pathspecs to sparse entries only" )), -OPT_HIDDEN_BOOL (0 ,"guess" , &dwim_new_local_branch, -N_ ("second guess 'git checkout <no-such-branch>'" )), +OPT_BOOL (0 ,"no-guess" , &no_dwim_new_local_branch, +N_ ("do not second guess 'git checkout <no-such-branch>'" )),OPT_BOOL (0 ,"ignore-other-worktrees" , &opts.ignore_other_worktrees,N_ ("do not check if another worktree is holding the given ref" )), { OPTION_CALLBACK, 0, "recurse-submodules", NULL,
@@ -1282,6 +1289,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix) argc =parse_options (argc, argv, prefix, options, checkout_usage,PARSE_OPT_KEEP_DASHDASH ); + dwim_new_local_branch = !no_dwim_new_local_branch;if (opts.show_progress <0 ) {if (opts.quiet) opts.show_progress =0 ;
t/t2024-checkout-dwim.sh +31 -0
@@ -278,4 +278,35 @@ test_expect_success 'loosely defined local base branch is reported correctly' ' test_cmp expect actual ' +test_expect_success 'reject when arg could be part of dwim branch' ' + git remote add foo file://non-existent-place && + git update-ref refs/remotes/foo/dwim-arg HEAD && + echo foo >dwim-arg && + git add dwim-arg && + echo bar >dwim-arg && + test_must_fail git checkout dwim-arg && + test_must_fail git rev-parse refs/heads/dwim-arg -- && + grep bar dwim-arg +' + +test_expect_success 'disambiguate dwim branch and checkout path (1)' ' + git update-ref refs/remotes/foo/dwim-arg1 HEAD && + echo foo >dwim-arg1 && + git add dwim-arg1 && + echo bar >dwim-arg1 && + git checkout -- dwim-arg1 && + test_must_fail git rev-parse refs/heads/dwim-arg1 -- && + grep foo dwim-arg1 +' + +test_expect_success 'disambiguate dwim branch and checkout path (2)' ' + git update-ref refs/remotes/foo/dwim-arg2 HEAD && + echo foo >dwim-arg2 && + git add dwim-arg2 && + echo bar >dwim-arg2 && + git checkout dwim-arg2 -- && + git rev-parse refs/heads/dwim-arg2 -- && + grep bar dwim-arg2 +' + test_done
t/t9902-completion.sh +2 -1
@@ -1434,7 +1434,8 @@ test_expect_success 'double dash "git checkout"' ' --ignore-other-worktrees Z --recurse-submodules Z --progress Z - --no-quiet Z + --guess Z + --no-guess Z --no-... Z EOF '
Signature · 1 line
2.19.1.1318.g5295c6727d