checkout: swap the order of ambiguity check for :/ syntax
PATCH 1/1 checkout: swap the order of ambiguity check for :/ syntax
This should speed up "git checkout :/long/path/taken/from/diffstat" because we don't have to walk through the commit graph to see if "long/path/taken/from/diffstat" exists in any commit message.
This is in the the same spirit as 4db86e8 (Update :/abc ambiguity check
- 2013-01-21), but instead of considering this case ("abc" in ":/abc" exists on worktree) ambiguous and shouting, we just assume the user means "pathspec" not "ref".
It's not wonderful, but it's in line with how git-checkout stops caring about ambiguity after the first argument can be resolved as a ref (there's even a test for it, t2010.6). In other words, we assume the user means "ref" not "pathspec" when that happens. Here we simply swap the order of checking specifically for :/ on practical ground.
Last note, to be pedantic, we should check if "abc" from ":/abc" exists as an _index_ entry, not on disk. But chances are they exist in both places anyway...
--- builtin/checkout.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
builtin/checkout.c +16 -0
@@ -973,6 +973,22 @@ static int parse_branchname_arg(int argc, const char **argv,if (!strcmp (arg,"-" )) arg ="@{-1}" ; +if (dash_dash_pos <0 &&starts_with (arg,":/" ) && +check_filename (opts->prefix, arg)) { +/* +* Normally if the first argument is ambiguous, we +* choose to believe the user specifies an extended +* SHA-1 syntax unless it turns out not true, then we +* see if it's a pathspec. +* +* :/ here is an exception because resolving :/abc may +* involve walking through the entire commit +* graph. Expensive and slow. If :/abc points to an +* existing file, ignore ambiguity and go with +* pathspec (i.e. skip get_sha1_mb()). +*/ +return 0 ;/* case (2) */ + }if (get_sha1_mb (arg, rev)) {/* * Either case (3) or (4), with <something> not being
Signature · 1 line
2.8.2.524.g6ff3d78
Quoted 3 lines · Nguyễn Thái Ngọc Duy
It's not wonderful, but it's in line with how git-checkout stops caring about ambiguity after the first argument can be resolved as a ref (there's even a test for it, t2010.6).
But that is justifiable because checkout can only ever take one revision. What follows, if there are any, must be paths, and more importantly, it would be perfectly reasonable if some of them were missing in the working tree ("ow, I accidentally removed that file, I need to resurrect it from the index"). Does the same justification apply to this change?
Quoted 6 lines · Junio C Hamano
Quoted 3 lines · Nguyễn Thái Ngọc Duy
It's not wonderful, but it's in line with how git-checkout stops caring about ambiguity after the first argument can be resolved as a ref (there's even a test for it, t2010.6).
Quoted 6 lines
But that is justifiable because checkout can only ever take one revision. What follows, if there are any, must be paths, and more importantly, it would be perfectly reasonable if some of them were missing in the working tree ("ow, I accidentally removed that file, I need to resurrect it from the index"). Does the same justification apply to this change?
I think there is a misunderstanding. My "after" is in "after the first argument can be resolved, check if it exists in worktree too, if so it's ambiguous and bail". This is usually how we detect ambiguation. But git-checkout does not do the "check if it exists..." clause.
Signature · 1 line
Duy
Quoted 14 lines · Duy Nguyen
Quoted 6 lines · Junio C Hamano
Quoted 3 lines · Nguyễn Thái Ngọc Duy
It's not wonderful, but it's in line with how git-checkout stops caring about ambiguity after the first argument can be resolved as a ref (there's even a test for it, t2010.6).
Quoted 6 lines
But that is justifiable because checkout can only ever take one revision. What follows, if there are any, must be paths, and more importantly, it would be perfectly reasonable if some of them were missing in the working tree ("ow, I accidentally removed that file, I need to resurrect it from the index"). Does the same justification apply to this change?
Quoted 4 lines
I think there is a misunderstanding. My "after" is in "after the first argument can be resolved, check if it exists in worktree too, if so it's ambiguous and bail". This is usually how we detect ambiguation. But git-checkout does not do the "check if it exists..." clause.
Hmph. The "case 4" in the function you touched says
* case 4: git checkout <something> <paths> *
* The first argument must not be ambiguous.
* - If it's *only* a reference, treat it like case (1).
* - If it's only a path, treat it like case (2).
* - else: fail.
Did we break it recently?
Quoted 23 lines · Junio C Hamano
Quoted 14 lines · Duy Nguyen
Quoted 6 lines · Junio C Hamano
Quoted 3 lines · Nguyễn Thái Ngọc Duy
It's not wonderful, but it's in line with how git-checkout stops caring about ambiguity after the first argument can be resolved as a ref (there's even a test for it, t2010.6).
Quoted 6 lines
But that is justifiable because checkout can only ever take one revision. What follows, if there are any, must be paths, and more importantly, it would be perfectly reasonable if some of them were missing in the working tree ("ow, I accidentally removed that file, I need to resurrect it from the index"). Does the same justification apply to this change?
Quoted 4 lines
I think there is a misunderstanding. My "after" is in "after the first argument can be resolved, check if it exists in worktree too, if so it's ambiguous and bail". This is usually how we detect ambiguation. But git-checkout does not do the "check if it exists..." clause.
Hmph. The "case 4" in the function you touched says
Quoted 8 lines
* case 4: git checkout <something> <paths> *
* The first argument must not be ambiguous.
* - If it's *only* a reference, treat it like case (1).
* - If it's only a path, treat it like case (2).
* - else: fail.Did we break it recently?
I was driven by testing and did not read the code carefully (but on the other hand, ambiguation plus dwim is never a pleasant read). Near the end of this function we have
if (!has_dash_dash) {/* case (3).(d) -> (1) */
/*
* Do not complain the most common case
* git checkout branch
* even if there happen to be a file called 'branch';
* it would be extremely annoying.
*/
if (argc)
verify_non_filename(NULL, arg);
argc == 0 would be case 3d, argc != 0 is case 4 and "touch abc; git checkout :/abc def" does complain about :/abc being ambiguous. So no we haven't broken anything yet, but I do with my patch. If I want to make a special case for :/ to speed it up, I need to be careful about this (and I probably need a test too because it's not detected).
While at there, verify_non_filename() in this function (and check_filename() too) should pass the right prefix in argument 1, not NULL, I think. I'll need to do some digging to see why they are NULL in the first place though.
Signature · 1 line
Duy
cover fix checkout ambiguation in subdir
In reply to Nguyễn Thái Ngọc DuyI need some more time (which I don't have) to convince myself about the "git checkout :/abc" patch. But these look like good bug fix/improvement.
Nguyễn Thái Ngọc Duy (3):
checkout: add some spaces between code and comment checkout.txt: document a common case that ignores ambiguation rules
checkout: fix ambiguity check in subdir
Documentation/git-checkout.txt | 9 +++++++++ builtin/checkout.c | 6 +++--- t/t2010-checkout-ambiguous.sh | 9 +++++++++ t/t2024-checkout-dwim.sh | 12 ++++++++++++ 4 files changed, 33 insertions(+), 3 deletions(-)
Signature · 1 line
2.8.2.524.g6ff3d78
PATCH 1/3 checkout: add some spaces between code and comment
--- builtin/checkout.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/checkout.c +1 -1
@@ -1038,7 +1038,7 @@ static int parse_branchname_arg(int argc, const char **argv,if (!*source_tree)/* case (1): want a tree */ die (_ ("reference is not a tree: %s" ), arg); -if (!has_dash_dash) {/* case (3).(d) -> (1) */ +if (!has_dash_dash) {/* case (3).(d) -> (1) */ /* * Do not complain the most common case * git checkout branch
Signature · 1 line
2.8.2.524.g6ff3d78
PATCH 2/3 checkout.txt: document a common case that ignores ambiguation rules
In reply to Nguyễn Thái Ngọc DuyNormally we err on the safe side: if something can be seen as both an SHA1 and a pathspec, we stop and scream. In checkout, there is one exception added in 859fdab (git-checkout: improve error messages, detect ambiguities. - 2008-07-23), to allow the common case "git checkout branch". Let's document this exception.
--- Documentation/git-checkout.txt | 9 +++++++++ 1 file changed, 9 insertions(+)
Documentation/git-checkout.txt +9 -0
@@ -419,6 +419,15 @@ $ git reflog -2 HEAD # or $ git log -g -2 HEAD ------------ +ARGUMENT AMBIGUATION +-------------------- + +When there is only one argument given and it is not `--` (e.g. "git +checkout abc"), "abc" could be seen as either a `<tree-ish>` or a +`<pathspec>`, but Git will assume the argument is a `<tree-ish>`, which is +a common case for switching branches. Use `git checkout -- <pathspec>` +form if you mean it to be a pathspec. + EXAMPLES --------
Signature · 1 line
2.8.2.524.g6ff3d78
Re: [PATCH 2/3] checkout.txt: document a common case that ignores ambiguation rules
Quoted 5 lines · Nguyễn Thái Ngọc Duy
Normally we err on the safe side: if something can be seen as both an SHA1 and a pathspec, we stop and scream. In checkout, there is one exception added in 859fdab (git-checkout: improve error messages, detect ambiguities. - 2008-07-23), to allow the common case "git checkout branch". Let's document this exception.
Good idea, but...
Documentation/git-checkout.txt $ git reflog -2 HEAD # or · line 429
Quoted 8 lines
Documentation/git-checkout.txt +8
-0+ARGUMENT AMBIGUATION +-------------------- + +When there is only one argument given and it is not `--` (e.g. "git +checkout abc"), "abc" could be seen as either a `<tree-ish>` or a +`<pathspec>`, but Git will assume the argument is a `<tree-ish>`, which is +a common case for switching branches. Use `git checkout -- <pathspec>` +form if you mean it to be a pathspec.
... this is far from reasonable. I'd read "but Git will assume the argument is a tree-ish" to mean "git checkout Makefile" would attempt to checkout the Makefile branch and fail.
When there is only one argument given and it is not -- (e.g. "git checkout abc"), and when the argument is both a valid <tree-ish> (e.g. a branch "abc" exists) and a valid <pathspec> (e.g. a file or a directory whose name is "abc" exists), Git would usually ask you to disambiguate. Because checking out a branch is so common an operation, however, "git checkout abc" takes "abc" as a <tree-ish> in such situation. Use git checkout -- <pathspec> if you want to checkout these paths out of the index.
or something like that?
Re: [PATCH 2/3] checkout.txt: document a common case that ignores ambiguation rules
I'll queue the following as "fixup!" for now. It reminds me that the title also needs rewording; we do not have rules to make things ambiguous ;-)
Documentation/git-checkout.txt +9 -6
@@ -419,14 +419,17 @@ $ git reflog -2 HEAD # or $ git log -g -2 HEAD ------------ -ARGUMENT AMBIGUATION --------------------- +ARGUMENT DISAMBIGUATION +----------------------- When there is only one argument given and it is not `--` (e.g. "git -checkout abc"), "abc" could be seen as either a `<tree-ish>` or a -`<pathspec>`, but Git will assume the argument is a `<tree-ish>`, which is -a common case for switching branches. Use `git checkout -- <pathspec>` -form if you mean it to be a pathspec. +checkout abc"), and when the argument is both a valid `<tree-ish>` +(e.g. a branch "abc" exists) and a valid `<pathspec>` (e.g. a file +or a directory whose name is "abc" exists), Git would usually ask +you to disambiguate. Because checking out a branch is so common an +operation, however, "git checkout abc" takes "abc" as a `<tree-ish>` +in such a situation. Use `git checkout -- <pathspec>` if you want +to checkout these paths out of the index. EXAMPLES --------
Signature · 1 line
2.10.0-339-gc0c747f
PATCH 3/3 checkout: fix ambiguity check in subdir
In reply to Nguyễn Thái Ngọc DuyThe two functions in parse_branchname_arg(), verify_non_filename and check_filename, need correct prefix in order to reconstruct the paths and check for their existence. With NULL prefix, they just check paths at top dir instead.
--- builtin/checkout.c | 4 ++-- t/t2010-checkout-ambiguous.sh | 9 +++++++++ t/t2024-checkout-dwim.sh | 12 ++++++++++++ 3 files changed, 23 insertions(+), 2 deletions(-)
builtin/checkout.c +2 -2
@@ -985,7 +985,7 @@ 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 (NULL , arg) || !no_wildcard (arg))) + (check_filename (opts->prefix, arg) || !no_wildcard (arg))) recover_with_dwim =0 ;/* * Accept "git checkout foo" and "git checkout foo --"
@@ -1046,7 +1046,7 @@ static int parse_branchname_arg(int argc, const char **argv,* it would be extremely annoying. */ if (argc) -verify_non_filename (NULL , arg); +verify_non_filename (opts->prefix, arg); }else { argcount++; argv++;
t/t2010-checkout-ambiguous.sh +9 -0
@@ -41,6 +41,15 @@ test_expect_success 'check ambiguity' ' test_must_fail git checkout world all ' +test_expect_success 'check ambiguity in subdir' ' + mkdir sub && + # not ambiguous because sub/world does not exist + git -C sub checkout world ../all && + echo hello >sub/world && + # ambiguous because sub/world does exist + test_must_fail git -C sub checkout world ../all +' + test_expect_success 'disambiguate checking out from a tree-ish' ' echo bye > world && git checkout world -- world &&
t/t2024-checkout-dwim.sh +12 -0
@@ -174,6 +174,18 @@ test_expect_success 'checkout of branch with a file having the same name fails' test_branch master ' +test_expect_success 'checkout of branch with a file in subdir having the same name fails' ' + git checkout -B master && + test_might_fail git branch -D spam && + + >spam && + mkdir sub && + mv spam sub/spam && + test_must_fail git -C sub checkout spam && + test_must_fail git rev-parse --verify refs/heads/spam && + test_branch master +' + test_expect_success 'checkout <branch> -- succeeds, even if a file with the same name exists' ' git checkout -B master && test_might_fail git branch -D spam &&
Signature · 1 line
2.8.2.524.g6ff3d78
Re: [PATCH 3/3] checkout: fix ambiguity check in subdir
Quoted 4 lines · Nguyễn Thái Ngọc Duy
The two functions in
parse_branchname_arg(),verify_non_filenameandcheck_filename, need correct prefix in order to reconstruct the paths and check for their existence. With NULL prefix, they just check paths at top dir instead.
Good eyes. Will queue.
Quoted 73 lines
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>--- builtin/checkout.c | 4 ++-- t/t2010-checkout-ambiguous.sh | 9 +++++++++ t/t2024-checkout-dwim.sh | 12 ++++++++++++ 3 files changed, 23 insertions(+), 2 deletions(-)builtin/checkout.c +2
-2@@ -985,7 +985,7 @@ 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 (NULL , arg) || !no_wildcard (arg))) + (check_filename (opts->prefix, arg) || !no_wildcard (arg))) recover_with_dwim =0 ;/* * Accept "git checkout foo" and "git checkout foo --" @@ -1046,7 +1046,7 @@ static int parse_branchname_arg(int argc, const char **argv,* it would be extremely annoying. */ if (argc) -verify_non_filename (NULL , arg); +verify_non_filename (opts->prefix, arg); }else { argcount++; argv++;t/t2010-checkout-ambiguous.sh +9
-0@@ -41,6 +41,15 @@ test_expect_success 'check ambiguity' ' test_must_fail git checkout world all ' +test_expect_success 'check ambiguity in subdir' ' + mkdir sub && + # not ambiguous because sub/world does not exist + git -C sub checkout world ../all && + echo hello >sub/world && + # ambiguous because sub/world does exist + test_must_fail git -C sub checkout world ../all +' + test_expect_success 'disambiguate checking out from a tree-ish' ' echo bye > world && git checkout world -- world &&t/t2024-checkout-dwim.sh +12
-0@@ -174,6 +174,18 @@ test_expect_success 'checkout of branch with a file having the same name fails' test_branch master ' +test_expect_success 'checkout of branch with a file in subdir having the same name fails' ' + git checkout -B master && + test_might_fail git branch -D spam && + + >spam && + mkdir sub && + mv spam sub/spam && + test_must_fail git -C sub checkout spam && + test_must_fail git rev-parse --verify refs/heads/spam && + test_branch master +' + test_expect_success 'checkout <branch> -- succeeds, even if a file with the same name exists' ' git checkout -B master && test_might_fail git branch -D spam &&