From 1cc777de6f4396f578e0f5e6b1a5ec23b96a52f0 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 10 Feb 2016 16:12:34 -0500 Subject: checkout: reorder check_filename conditional If we have a "--" flag, we should not be doing DWIM magic based on whether arguments can be filenames. Reorder the conditional to avoid the check_filename() call entirely in this case. The outcome is the same, but the short-circuit makes the dependency more clear. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/builtin/checkout.c b/builtin/checkout.c index 3e141fc..d34f58e 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -965,7 +965,7 @@ static int parse_branchname_arg(int argc, const char **argv, */ int recover_with_dwim = dwim_new_local_branch_ok; - if (check_filename(NULL, arg) && !has_dash_dash) + if (!has_dash_dash && check_filename(NULL, arg)) recover_with_dwim = 0; /* * Accept "git checkout foo" and "git checkout foo --" -- cgit v0.10.2-6-g49f6 From df714f81a709dda9552f137ccd2a3510119298ad Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 10 Feb 2016 16:14:46 -0500 Subject: check_filename: tighten dwim-wildcard ambiguity When specifying both revisions and pathnames, we allow " -- " to be spelled without the "--" as long as it is not ambiguous. The original logic was something like: 1. Resolve each item with get_sha1(). If successful, we know it can be a . Verify that it _isn't_ a filename, using verify_non_filename(), and complain of ambiguity otherwise. 2. If get_sha1() didn't succeed, make sure that it _is_ a file, using verify_filename(). If not, complain that it is neither a nor a . Both verify_filename() and verify_non_filename() rely on check_filename(), which definitely said "yes, this is a file" or "no, it is not" using lstat(). Commit 28fcc0b (pathspec: avoid the need of "--" when wildcard is used, 2015-05-02) introduced a convenience feature: check_filename() will consider anything with wildcard meta-characters as a possible filename, without even checking the filesystem. This works well for case 2. For such a wildcard, we would previously have died and said "it is neither". Post-28fcc0b, we assume it's a pathspec and proceed. But it makes some instances of case 1 worse. We may have an extended sha1 expression that contains meta-characters (e.g., "HEAD^{/foo.*bar}"), and we now complain that it's also a filename, due to the wildcard characters (even though that wildcard would not match anything in the filesystem). One solution would be to actually expand the pathname and see if it matches anything on the filesystem. But that's potentially expensive, and we do not have to be so rigorous for this DWIM magic (if you want rigor, use "--"). Instead, we can just use different rules for cases 1 and 2. When we know something is a rev, we will complain only if it meets a much higher standard for "this is also a file"; namely that it actually exists in the filesystem. Case 2 remains the same: we use the looser "it could be a filename" standard introduced by 28fcc0b. We can accomplish this by pulling the wildcard logic out of check_filename() and putting it into verify_filename(). Its partner verify_non_filename() does not need a change, since check_filename() goes back to implementing the "higher standard". Besides these two callers of check_filename(), there is one other: git-checkout does a similar DWIM itself. It hits this code path only after get_sha1() has returned failure, making it case 2, which gets the special wildcard treatment. Note that we drop the tests in t2019 in favor of a more complete set in t6133. t2019 was not the right place for them (it's about refname ambiguity, not dwim parsing ambiguity), and the second test explicitly checked for the opposite result of the case we are fixing here (which didn't really make any sense; as shown by the test_must_fail in the test, it would only serve to annoy people). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/builtin/checkout.c b/builtin/checkout.c index d34f58e..e5c16af 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -965,7 +965,8 @@ 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)) + if (!has_dash_dash && + (check_filename(NULL, arg) || !no_wildcard(arg))) recover_with_dwim = 0; /* * Accept "git checkout foo" and "git checkout foo --" diff --git a/setup.c b/setup.c index 1055b82..69a8ada 100644 --- a/setup.c +++ b/setup.c @@ -140,9 +140,7 @@ int check_filename(const char *prefix, const char *arg) if (arg[2] == '\0') /* ":/" is root dir, always exists */ return 1; name = arg + 2; - } else if (!no_wildcard(arg)) - return 1; - else if (prefix) + } else if (prefix) name = prefix_filename(prefix, strlen(prefix), arg); else name = arg; @@ -203,7 +201,7 @@ void verify_filename(const char *prefix, { if (*arg == '-') die("bad flag '%s' used after filename", arg); - if (check_filename(prefix, arg)) + if (check_filename(prefix, arg) || !no_wildcard(arg)) return; die_verify_filename(prefix, arg, diagnose_misspelt_rev); } diff --git a/t/t2019-checkout-ambiguous-ref.sh b/t/t2019-checkout-ambiguous-ref.sh index 199b22d..b99d519 100755 --- a/t/t2019-checkout-ambiguous-ref.sh +++ b/t/t2019-checkout-ambiguous-ref.sh @@ -56,30 +56,4 @@ test_expect_success VAGUENESS_SUCCESS 'checkout reports switch to branch' ' test_i18ngrep ! "^HEAD is now at" stderr ' -test_expect_success 'wildcard ambiguation, paths win' ' - git init ambi && - ( - cd ambi && - echo a >a.c && - git add a.c && - echo b >a.c && - git checkout "*.c" && - echo a >expect && - test_cmp expect a.c - ) -' - -test_expect_success !MINGW 'wildcard ambiguation, refs lose' ' - git init ambi2 && - ( - cd ambi2 && - echo a >"*.c" && - git add . && - test_must_fail git show :"*.c" && - git show :"*.c" -- >actual && - echo a >expect && - test_cmp expect actual - ) -' - test_done diff --git a/t/t6133-pathspec-rev-dwim.sh b/t/t6133-pathspec-rev-dwim.sh new file mode 100755 index 0000000..8e5b338 --- /dev/null +++ b/t/t6133-pathspec-rev-dwim.sh @@ -0,0 +1,38 @@ +#!/bin/sh + +test_description='test dwim of revs versus pathspecs in revision parser' +. ./test-lib.sh + +test_expect_success 'setup' ' + test_commit base && + echo content >"br[ack]ets" && + git add . && + test_tick && + git commit -m brackets +' + +test_expect_success 'non-rev wildcard dwims to pathspec' ' + git log -- "*.t" >expect && + git log "*.t" >actual && + test_cmp expect actual +' + +test_expect_success 'tree:path with metacharacters dwims to rev' ' + git show "HEAD:br[ack]ets" -- >expect && + git show "HEAD:br[ack]ets" >actual && + test_cmp expect actual +' + +test_expect_success '^{foo} with metacharacters dwims to rev' ' + git log "HEAD^{/b.*}" -- >expect && + git log "HEAD^{/b.*}" >actual && + test_cmp expect actual +' + +test_expect_success '@{foo} with metacharacters dwims to rev' ' + git log "HEAD@{now [or thereabouts]}" -- >expect && + git log "HEAD@{now [or thereabouts]}" >actual && + test_cmp expect actual +' + +test_done -- cgit v0.10.2-6-g49f6 From aac4fac1689f7d1414523d2ac96f1c737ee53b71 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 10 Feb 2016 16:19:25 -0500 Subject: get_sha1: don't die() on bogus search strings The get_sha1() function generally returns an error code rather than dying, and we sometimes speculatively call it with something that may be a revision or a pathspec, in order to see which one it might be. If it sees a bogus ":/" search string, though, it complains, without giving the caller the opportunity to recover. We can demonstrate this in t6133 by looking for ":/*.t", which should mean "*.t at the root of the tree", but instead dies because of the invalid regex (the "*" has nothing to operate on). We can fix this by returning an error rather than calling die(). Unfortunately, the tradeoff is that the error message is slightly worse in cases where we _do_ know we have a rev. E.g., running "git log ':/*.t' --" before yielded: fatal: Invalid search pattern: *.t and now we get only: fatal: bad revision ':/*.t' There's not a simple way to fix this short of passing a "quiet" flag all the way through the get_sha1() stack. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/sha1_name.c b/sha1_name.c index 6d10f05..6a2195f 100644 --- a/sha1_name.c +++ b/sha1_name.c @@ -858,12 +858,12 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1, if (prefix[0] == '!') { if (prefix[1] != '!') - die ("Invalid search pattern: %s", prefix); + return -1; prefix++; } if (regcomp(®ex, prefix, REG_EXTENDED)) - die("Invalid search pattern: %s", prefix); + return -1; for (l = list; l; l = l->next) { l->item->object.flags |= ONELINE_SEEN; diff --git a/t/t6133-pathspec-rev-dwim.sh b/t/t6133-pathspec-rev-dwim.sh index 8e5b338..a290ffc 100755 --- a/t/t6133-pathspec-rev-dwim.sh +++ b/t/t6133-pathspec-rev-dwim.sh @@ -35,4 +35,14 @@ test_expect_success '@{foo} with metacharacters dwims to rev' ' test_cmp expect actual ' +test_expect_success ':/*.t from a subdir dwims to a pathspec' ' + mkdir subdir && + ( + cd subdir && + git log -- ":/*.t" >expect && + git log ":/*.t" >actual && + test_cmp expect actual + ) +' + test_done -- cgit v0.10.2-6-g49f6