From be6ed3f3346a24f4be2d771384528b1c87952b8f Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 26 May 2017 15:06:41 -0400 Subject: [PATCH 1/6] t4208: add check for ":/" without matching file The DWIM magic in check_filename() doesn't just recognize ":/". It actually makes sure that the file it points to exists. t4208 checks only the case where the path is present, not the opposite. Since the next patches will be touching this area, let's add a test to make sure it continues working. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t4208-log-magic-pathspec.sh | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/t/t4208-log-magic-pathspec.sh b/t/t4208-log-magic-pathspec.sh index 001343e2fc..70bc64100b 100755 --- a/t/t4208-log-magic-pathspec.sh +++ b/t/t4208-log-magic-pathspec.sh @@ -29,6 +29,12 @@ test_expect_success '"git log -- :/a" should not be ambiguous' ' git log -- :/a ' +# This differs from the ":/a" check above in that :/in looks like a pathspec, +# but doesn't match an actual file. +test_expect_success '"git log :/in" should not be ambiguous' ' + git log :/in +' + test_expect_success '"git log :" should be ambiguous' ' test_must_fail git log : 2>error && test_i18ngrep ambiguous error From a08cbcda1782993d83cf8763a394dab24e2c52b3 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 26 May 2017 15:07:31 -0400 Subject: [PATCH 2/6] check_filename(): refactor ":/" handling We handle arguments with the ":/" pathspec magic specially, making sure the name exists at the top-level. We'll want to handle more pathspec magic in future patches, so let's do a little rearranging to make that easier. Instead of relying on an if/else cascade to avoid the prefix_filename() call, we'll just set prefix to NULL. Likewise, we'll get rid of the "name" variable entirely, and just push the "arg" pointer forward to skip past the magic. That means by the time we get to the prefix-handling, we're set up appropriately whether we saw ":/" or not. Note that this does impact the final error message we produce when stat() fails, as it shows "arg" (which we'll have modified to skip magic and include the prefix). This is a good thing; the original message would say something like "failed to stat ':/foo'", which is confusing (we tried to stat "foo"). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- setup.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/setup.c b/setup.c index 0309c27821..000ffa810e 100644 --- a/setup.c +++ b/setup.c @@ -134,19 +134,20 @@ int path_inside_repo(const char *prefix, const char *path) int check_filename(const char *prefix, const char *arg) { - const char *name; char *to_free = NULL; struct stat st; if (starts_with(arg, ":/")) { if (arg[2] == '\0') /* ":/" is root dir, always exists */ return 1; - name = arg + 2; - } else if (prefix) - name = to_free = prefix_filename(prefix, arg); - else - name = arg; - if (!lstat(name, &st)) { + arg += 2; + prefix = NULL; + } + + if (prefix) + arg = to_free = prefix_filename(prefix, arg); + + if (!lstat(arg, &st)) { free(to_free); return 1; /* file exists */ } From d51c6ee0d4bbeaf28bcd6c5f1681d208ee75763d Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 26 May 2017 15:07:42 -0400 Subject: [PATCH 3/6] check_filename(): use skip_prefix This avoids some magic numbers (and we'll be adding more similar calls in a minute). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- setup.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/setup.c b/setup.c index 000ffa810e..f2a8070bd4 100644 --- a/setup.c +++ b/setup.c @@ -137,10 +137,9 @@ int check_filename(const char *prefix, const char *arg) char *to_free = NULL; struct stat st; - if (starts_with(arg, ":/")) { - if (arg[2] == '\0') /* ":/" is root dir, always exists */ + if (skip_prefix(arg, ":/", &arg)) { + if (!*arg) /* ":/" is root dir, always exists */ return 1; - arg += 2; prefix = NULL; } From 42471bcee44e87669eb17f9fa13c8041a0fc35a1 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 26 May 2017 15:08:39 -0400 Subject: [PATCH 4/6] check_filename(): handle ":^" path magic We special-case "git log :/foo" to work when "foo" exists in the working tree. But :^ (and its alias :!) do not get the same treatment, requiring the user to supply a disambiguating "--". Let's make them work without requiring the user to type the "--". Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- setup.c | 4 ++++ t/t4208-log-magic-pathspec.sh | 13 +++++++++++++ 2 files changed, 17 insertions(+) diff --git a/setup.c b/setup.c index f2a8070bd4..86bb7c9a96 100644 --- a/setup.c +++ b/setup.c @@ -141,6 +141,10 @@ int check_filename(const char *prefix, const char *arg) if (!*arg) /* ":/" is root dir, always exists */ return 1; prefix = NULL; + } else if (skip_prefix(arg, ":!", &arg) || + skip_prefix(arg, ":^", &arg)) { + if (!*arg) /* excluding everything is silly, but allowed */ + return 1; } if (prefix) diff --git a/t/t4208-log-magic-pathspec.sh b/t/t4208-log-magic-pathspec.sh index 70bc64100b..627890d7d8 100755 --- a/t/t4208-log-magic-pathspec.sh +++ b/t/t4208-log-magic-pathspec.sh @@ -52,6 +52,19 @@ test_expect_success 'git log HEAD -- :/' ' test_cmp expected actual ' +test_expect_success '"git log :^sub" is not ambiguous' ' + git log :^sub +' + +test_expect_success '"git log :^does-not-exist" does not match anything' ' + test_must_fail git log :^does-not-exist +' + +test_expect_success '"git log :!" behaves the same as :^' ' + git log :!sub && + test_must_fail git log :!does-not-exist +' + test_expect_success 'command line pathspec parsing for "git log"' ' git reset --hard && >a && From c99eddd83519811d6604b4ce4a8d2001787a30d4 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 26 May 2017 15:10:31 -0400 Subject: [PATCH 5/6] verify_filename(): treat ":(magic)" as a pathspec For commands that take revisions and pathspecs, magic pathspecs like ":(exclude)foo" require the user to specify a disambiguating "--", since they do not match a file in the filesystem, like: git grep foo -- :(exclude)bar This makes them more annoying to use than they need to be. We loosened the rules for wildcards in 28fcc0b71 (pathspec: avoid the need of "--" when wildcard is used, 2015-05-02). Let's do the same for pathspecs with long-form magic. We already handle the short-forms ":/" and ":^" specially in check_filename(), so we don't need to handle them here. And in fact, we could do the same with long-form magic, parsing out the actual filename and making sure it exists. But there are a few reasons not to do it that way: - the parsing gets much more complicated, and we'd want to hand it off to the pathspec code. But that code isn't ready to do this kind of speculative parsing (it's happy to die() when it sees a syntactically invalid pathspec). - not all pathspec magic maps to a filesystem path. E.g., :(attr) should be treated as a pathspec regardless of what is in the filesystem - we can be a bit looser with ":(" than with the short-form ":/", because it is much less likely to have a false positive. Whereas ":/" also means "search for a commit with this regex". Note that because the change is in verify_filename() and not in its helper check_filename(), this doesn't affect the verify_non_filename() case. I.e., if an item that matches our new rule doesn't resolve as an object, we may fallback to treating it as a pathspec (rather than complaining it doesn't exist). But if it does resolve (e.g., as a file in the index that starts with an open-paren), we won't then complain that it's also a valid pathspec. This matches the wildcard-exception behavior. And of course in either case, one can always insert the "--" to get more precise results. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- setup.c | 20 +++++++++++++++++++- t/t4208-log-magic-pathspec.sh | 13 +++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/setup.c b/setup.c index 86bb7c9a96..89fcc12ab7 100644 --- a/setup.c +++ b/setup.c @@ -185,6 +185,24 @@ static void NORETURN die_verify_filename(const char *prefix, } +/* + * Check for arguments that don't resolve as actual files, + * but which look sufficiently like pathspecs that we'll consider + * them such for the purposes of rev/pathspec DWIM parsing. + */ +static int looks_like_pathspec(const char *arg) +{ + /* anything with a wildcard character */ + if (!no_wildcard(arg)) + return 1; + + /* long-form pathspec magic */ + if (starts_with(arg, ":(")) + return 1; + + return 0; +} + /* * Verify a filename that we got as an argument for a pathspec * entry. Note that a filename that begins with "-" never verifies @@ -211,7 +229,7 @@ void verify_filename(const char *prefix, { if (*arg == '-') die("bad flag '%s' used after filename", arg); - if (check_filename(prefix, arg) || !no_wildcard(arg)) + if (check_filename(prefix, arg) || looks_like_pathspec(arg)) return; die_verify_filename(prefix, arg, diagnose_misspelt_rev); } diff --git a/t/t4208-log-magic-pathspec.sh b/t/t4208-log-magic-pathspec.sh index 627890d7d8..935df6a65c 100755 --- a/t/t4208-log-magic-pathspec.sh +++ b/t/t4208-log-magic-pathspec.sh @@ -65,6 +65,19 @@ test_expect_success '"git log :!" behaves the same as :^' ' test_must_fail git log :!does-not-exist ' +test_expect_success '"git log :(exclude)sub" is not ambiguous' ' + git log ":(exclude)sub" +' + +test_expect_success '"git log :(exclude)sub --" must resolve as an object' ' + test_must_fail git log ":(exclude)sub" -- +' + +test_expect_success '"git log :(unknown-magic) complains of bogus magic' ' + test_must_fail git log ":(unknown-magic)" 2>error && + test_i18ngrep pathspec.magic error +' + test_expect_success 'command line pathspec parsing for "git log"' ' git reset --hard && >a && From 2cb47ab6958192a4b4b3b0616b2ab37f6680547f Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 26 May 2017 15:10:53 -0400 Subject: [PATCH 6/6] verify_filename(): flip order of checks The looks_like_pathspec() check is much cheaper than check_filename(), which actually stats the file. Since either is sufficient for our return value, we should do the cheaper one first, potentially short-circuiting the other. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- setup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/setup.c b/setup.c index 89fcc12ab7..1de87ed842 100644 --- a/setup.c +++ b/setup.c @@ -229,7 +229,7 @@ void verify_filename(const char *prefix, { if (*arg == '-') die("bad flag '%s' used after filename", arg); - if (check_filename(prefix, arg) || looks_like_pathspec(arg)) + if (looks_like_pathspec(arg) || check_filename(prefix, arg)) return; die_verify_filename(prefix, arg, diagnose_misspelt_rev); }