Re: [PATCH] checkout: swap the order of ambiguity check for :/ syntax
On Fri, Aug 26, 2016 at 12:19 AM, Junio C Hamanowrote: > Duy Nguyen writes: > >> On Wed, Aug 24, 2016 at 11:35 PM, Junio C Hamano wrote: >>> Nguyễn Thái Ngọc Duy writes: >>> 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? >> >> 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 > * > * 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. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] checkout: swap the order of ambiguity check for :/ syntax
Duy Nguyenwrites: > On Wed, Aug 24, 2016 at 11:35 PM, Junio C Hamano wrote: >> Nguyễn Thái Ngọc Duy writes: >> >>> 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? > > 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 * * 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? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] checkout: swap the order of ambiguity check for :/ syntax
On Wed, Aug 24, 2016 at 11:35 PM, Junio C Hamanowrote: > Nguyễn Thái Ngọc Duy writes: > >> 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? 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. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] checkout: swap the order of ambiguity check for :/ syntax
Nguyễn Thái Ngọc Duywrites: > 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? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] 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... Signed-off-by: Nguyễn Thái Ngọc Duy--- builtin/checkout.c | 16 1 file changed, 16 insertions(+) diff --git a/builtin/checkout.c b/builtin/checkout.c index 8672d07..6f016db 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -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 not being -- 2.8.2.524.g6ff3d78 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html