Message ID | 20181110120707.25846-1-pclouds@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | checkout: disambiguate dwim tracking branches and local files | expand |
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes: > @@ -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)); 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? > /* > * 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); ...
On Mon, Nov 12, 2018 at 7:44 AM Junio C Hamano <gitster@pobox.com> wrote: > > Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes: > > > @@ -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)); > > 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. 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. > 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. > > > /* > > * 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); > ... > 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.
Duy Nguyen <pclouds@gmail.com> writes: > 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? > 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.
diff --git a/builtin/checkout.c b/builtin/checkout.c index acdafc6e4c..17d48166d1 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -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) */ diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh index 69b6774d10..fa0718c730 100755 --- a/t/t2024-checkout-dwim.sh +++ b/t/t2024-checkout-dwim.sh @@ -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
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 Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- builtin/checkout.c | 15 ++++++++++++--- t/t2024-checkout-dwim.sh | 31 +++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 3 deletions(-)