* [PATCH 1/7] grep: move thread initialization a little lower
2017-02-14 6:00 ` [PATCH 0/7] grep rev/path parsing fixes Jeff King
@ 2017-02-14 6:02 ` Jeff King
2017-02-14 18:46 ` Brandon Williams
2017-02-14 6:03 ` [PATCH 2/7] grep: do not unnecessarily query repo for "--" Jeff King
` (7 subsequent siblings)
8 siblings, 1 reply; 22+ messages in thread
From: Jeff King @ 2017-02-14 6:02 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, gitster
Originally, we set up the threads for grep before parsing
the non-option arguments. In 53b8d931b (grep: disable
threading in non-worktree case, 2011-12-12), the thread code
got bumped lower in the function because it now needed to
know whether we got any revision arguments.
That put a big block of code in between the parsing of revs
and the parsing of pathspecs, both of which share some loop
variables. That makes it harder to read the code than the
original, where the shared loops were right next to each
other.
Let's bump the thread initialization until after all of the
parsing is done.
Signed-off-by: Jeff King <peff@peff.net>
---
I double-checked to make sure no other code was relying on
the thread setup having happened. I think we could actually
bump it quite a bit lower (to right before we actually start
grepping), but I doubt it matters much in practice.
builtin/grep.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 2c727ef49..5a282c4d0 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1169,6 +1169,20 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
break;
}
+ /* The rest are paths */
+ if (!seen_dashdash) {
+ int j;
+ for (j = i; j < argc; j++)
+ verify_filename(prefix, argv[j], j == i);
+ }
+
+ parse_pathspec(&pathspec, 0,
+ PATHSPEC_PREFER_CWD |
+ (opt.max_depth != -1 ? PATHSPEC_MAXDEPTH_VALID : 0),
+ prefix, argv + i);
+ pathspec.max_depth = opt.max_depth;
+ pathspec.recursive = 1;
+
#ifndef NO_PTHREADS
if (list.nr || cached || show_in_pager)
num_threads = 0;
@@ -1190,20 +1204,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
}
#endif
- /* The rest are paths */
- if (!seen_dashdash) {
- int j;
- for (j = i; j < argc; j++)
- verify_filename(prefix, argv[j], j == i);
- }
-
- parse_pathspec(&pathspec, 0,
- PATHSPEC_PREFER_CWD |
- (opt.max_depth != -1 ? PATHSPEC_MAXDEPTH_VALID : 0),
- prefix, argv + i);
- pathspec.max_depth = opt.max_depth;
- pathspec.recursive = 1;
-
if (recurse_submodules) {
gitmodules_config();
compile_submodule_options(&opt, &pathspec, cached, untracked,
--
2.12.0.rc1.471.ga79ec8999
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 1/7] grep: move thread initialization a little lower
2017-02-14 6:02 ` [PATCH 1/7] grep: move thread initialization a little lower Jeff King
@ 2017-02-14 18:46 ` Brandon Williams
0 siblings, 0 replies; 22+ messages in thread
From: Brandon Williams @ 2017-02-14 18:46 UTC (permalink / raw)
To: Jeff King; +Cc: Jonathan Tan, git, gitster
On 02/14, Jeff King wrote:
> Originally, we set up the threads for grep before parsing
> the non-option arguments. In 53b8d931b (grep: disable
> threading in non-worktree case, 2011-12-12), the thread code
> got bumped lower in the function because it now needed to
> know whether we got any revision arguments.
>
> That put a big block of code in between the parsing of revs
> and the parsing of pathspecs, both of which share some loop
> variables. That makes it harder to read the code than the
> original, where the shared loops were right next to each
> other.
>
> Let's bump the thread initialization until after all of the
> parsing is done.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I double-checked to make sure no other code was relying on
> the thread setup having happened. I think we could actually
> bump it quite a bit lower (to right before we actually start
> grepping), but I doubt it matters much in practice.
Looks good. And yes I don't believe anything needs the thread
initialization to happen earlier.
--
Brandon Williams
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2/7] grep: do not unnecessarily query repo for "--"
2017-02-14 6:00 ` [PATCH 0/7] grep rev/path parsing fixes Jeff King
2017-02-14 6:02 ` [PATCH 1/7] grep: move thread initialization a little lower Jeff King
@ 2017-02-14 6:03 ` Jeff King
2017-02-14 6:03 ` [PATCH 3/7] t7810: make "--no-index --" test more robust Jeff King
` (6 subsequent siblings)
8 siblings, 0 replies; 22+ messages in thread
From: Jeff King @ 2017-02-14 6:03 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, gitster
From: Jonathan Tan <jonathantanmy@google.com>
When running a command of the form
git grep --no-index pattern -- path
in the absence of a Git repository, an error message will be printed:
fatal: BUG: setup_git_env called without repository
This is because "git grep" tries to interpret "--" as a rev. "git grep"
has always tried to first interpret "--" as a rev for at least a few
years, but this issue was upgraded from a pessimization to a bug in
commit 59332d1 ("Resurrect "git grep --no-index"", 2010-02-06), which
calls get_sha1 regardless of whether --no-index was specified. This bug
appeared to be benign until commit b1ef400 ("setup_git_env: avoid blind
fall-back to ".git"", 2016-10-20) when Git was taught to die in this
situation. (This "git grep" bug appears to be one of the bugs that
commit b1ef400 is meant to flush out.)
Therefore, always interpret "--" as signaling the end of options,
instead of trying to interpret it as a rev first.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Jeff King <peff@peff.net>
---
Unchanged from your original.
builtin/grep.c | 9 +++++----
t/t7810-grep.sh | 12 ++++++++++++
2 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 5a282c4d0..081e1b57a 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1154,6 +1154,11 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
const char *arg = argv[i];
unsigned char sha1[20];
struct object_context oc;
+ if (!strcmp(arg, "--")) {
+ i++;
+ seen_dashdash = 1;
+ break;
+ }
/* Is it a rev? */
if (!get_sha1_with_context(arg, 0, sha1, &oc)) {
struct object *object = parse_object_or_die(sha1, arg);
@@ -1162,10 +1167,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
add_object_array_with_path(object, arg, &list, oc.mode, oc.path);
continue;
}
- if (!strcmp(arg, "--")) {
- i++;
- seen_dashdash = 1;
- }
break;
}
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 19f0108f8..29202f0e7 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -982,6 +982,18 @@ test_expect_success 'grep -e -- -- path' '
test_cmp expected actual
'
+test_expect_success 'grep --no-index pattern -- path' '
+ rm -fr non &&
+ mkdir -p non/git &&
+ (
+ GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
+ export GIT_CEILING_DIRECTORIES &&
+ cd non/git &&
+ echo hello >hello &&
+ git grep --no-index o -- .
+ )
+'
+
cat >expected <<EOF
hello.c:int main(int argc, const char **argv)
hello.c: printf("Hello world.\n");
--
2.12.0.rc1.471.ga79ec8999
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 3/7] t7810: make "--no-index --" test more robust
2017-02-14 6:00 ` [PATCH 0/7] grep rev/path parsing fixes Jeff King
2017-02-14 6:02 ` [PATCH 1/7] grep: move thread initialization a little lower Jeff King
2017-02-14 6:03 ` [PATCH 2/7] grep: do not unnecessarily query repo for "--" Jeff King
@ 2017-02-14 6:03 ` Jeff King
2017-02-14 6:04 ` [PATCH 4/7] grep: re-order rev-parsing loop Jeff King
` (5 subsequent siblings)
8 siblings, 0 replies; 22+ messages in thread
From: Jeff King @ 2017-02-14 6:03 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, gitster
This makes sure that we actually use the pathspecs after
"--" correctly (as opposed to just seeing if grep errored
out).
Signed-off-by: Jeff King <peff@peff.net>
---
This could be squashed into the previous patch.
I didn't end up using the "nongit" helper, because we actually have to
do some other setup inside the subshell (this is the reason the earlier
tests in this script don't use the helper, either).
t/t7810-grep.sh | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 29202f0e7..2c1f7373e 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -990,7 +990,10 @@ test_expect_success 'grep --no-index pattern -- path' '
export GIT_CEILING_DIRECTORIES &&
cd non/git &&
echo hello >hello &&
- git grep --no-index o -- .
+ echo goodbye >goodbye &&
+ echo hello:hello >expect &&
+ git grep --no-index o -- hello >actual &&
+ test_cmp expect actual
)
'
--
2.12.0.rc1.471.ga79ec8999
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 4/7] grep: re-order rev-parsing loop
2017-02-14 6:00 ` [PATCH 0/7] grep rev/path parsing fixes Jeff King
` (2 preceding siblings ...)
2017-02-14 6:03 ` [PATCH 3/7] t7810: make "--no-index --" test more robust Jeff King
@ 2017-02-14 6:04 ` Jeff King
2017-02-14 18:48 ` Brandon Williams
2017-02-14 6:05 ` [PATCH 5/7] grep: fix "--" rev/pathspec disambiguation Jeff King
` (4 subsequent siblings)
8 siblings, 1 reply; 22+ messages in thread
From: Jeff King @ 2017-02-14 6:04 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, gitster
We loop over the arguments, but every branch of the loop
hits either a "continue" or a "break". Surely we can make
this simpler.
The final conditional is:
if (arg is a rev) {
... handle rev ...
continue;
}
break;
We can rewrite this as:
if (arg is not a rev)
break;
... handle rev ...
That makes the flow a little bit simpler, and will make
things much easier to follow when we add more logic in
future patches.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/grep.c | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 081e1b57a..461347adb 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1154,20 +1154,22 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
const char *arg = argv[i];
unsigned char sha1[20];
struct object_context oc;
+ struct object *object;
+
if (!strcmp(arg, "--")) {
i++;
seen_dashdash = 1;
break;
}
- /* Is it a rev? */
- if (!get_sha1_with_context(arg, 0, sha1, &oc)) {
- struct object *object = parse_object_or_die(sha1, arg);
- if (!seen_dashdash)
- verify_non_filename(prefix, arg);
- add_object_array_with_path(object, arg, &list, oc.mode, oc.path);
- continue;
- }
- break;
+
+ /* Stop at the first non-rev */
+ if (get_sha1_with_context(arg, 0, sha1, &oc))
+ break;
+
+ object = parse_object_or_die(sha1, arg);
+ if (!seen_dashdash)
+ verify_non_filename(prefix, arg);
+ add_object_array_with_path(object, arg, &list, oc.mode, oc.path);
}
/* The rest are paths */
--
2.12.0.rc1.471.ga79ec8999
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 4/7] grep: re-order rev-parsing loop
2017-02-14 6:04 ` [PATCH 4/7] grep: re-order rev-parsing loop Jeff King
@ 2017-02-14 18:48 ` Brandon Williams
0 siblings, 0 replies; 22+ messages in thread
From: Brandon Williams @ 2017-02-14 18:48 UTC (permalink / raw)
To: Jeff King; +Cc: Jonathan Tan, git, gitster
On 02/14, Jeff King wrote:
> - /* Is it a rev? */
> - if (!get_sha1_with_context(arg, 0, sha1, &oc)) {
> - struct object *object = parse_object_or_die(sha1, arg);
> - if (!seen_dashdash)
> - verify_non_filename(prefix, arg);
> - add_object_array_with_path(object, arg, &list, oc.mode, oc.path);
> - continue;
> - }
> - break;
> +
> + /* Stop at the first non-rev */
> + if (get_sha1_with_context(arg, 0, sha1, &oc))
> + break;
> +
> + object = parse_object_or_die(sha1, arg);
> + if (!seen_dashdash)
> + verify_non_filename(prefix, arg);
> + add_object_array_with_path(object, arg, &list, oc.mode, oc.path);
This is much more readable!
--
Brandon Williams
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 5/7] grep: fix "--" rev/pathspec disambiguation
2017-02-14 6:00 ` [PATCH 0/7] grep rev/path parsing fixes Jeff King
` (3 preceding siblings ...)
2017-02-14 6:04 ` [PATCH 4/7] grep: re-order rev-parsing loop Jeff King
@ 2017-02-14 6:05 ` Jeff King
2017-02-14 18:56 ` Brandon Williams
2017-02-14 19:18 ` Junio C Hamano
2017-02-14 6:07 ` [PATCH 6/7] grep: avoid resolving revision names in --no-index case Jeff King
` (3 subsequent siblings)
8 siblings, 2 replies; 22+ messages in thread
From: Jeff King @ 2017-02-14 6:05 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, gitster
If we see "git grep pattern rev -- file" then we apply the
usual rev/pathspec disambiguation rules: any "rev" before
the "--" must be a revision, and we do not need to apply the
verify_non_filename() check.
But there are two bugs here:
1. We keep a seen_dashdash flag to handle this case, but
we set it in the same left-to-right pass over the
arguments in which we parse "rev".
So when we see "rev", we do not yet know that there is
a "--", and we mistakenly complain if there is a
matching file.
We can fix this by making a preliminary pass over the
arguments to find the "--", and only then checking the rev
arguments.
2. If we can't resolve "rev" but there isn't a dashdash,
that's OK. We treat it like a path, and complain later
if it doesn't exist.
But if there _is_ a dashdash, then we know it must be a
rev, and should treat it as such, complaining if it
does not resolve. The current code instead ignores it
and tries to treat it like a path.
This patch fixes both bugs, and tries to comment the parsing
flow a bit better.
It adds tests that cover the two bugs, but also some related
situations (which already worked, but this confirms that our
fixes did not break anything).
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/grep.c | 29 ++++++++++++++++++++++++-----
t/t7810-grep.sh | 33 +++++++++++++++++++++++++++++++++
2 files changed, 57 insertions(+), 5 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 461347adb..e83b33bda 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1149,7 +1149,22 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
compile_grep_patterns(&opt);
- /* Check revs and then paths */
+ /*
+ * We have to find "--" in a separate pass, because its presence
+ * influences how we will parse arguments that come before it.
+ */
+ for (i = 0; i < argc; i++) {
+ if (!strcmp(argv[i], "--")) {
+ seen_dashdash = 1;
+ break;
+ }
+ }
+
+ /*
+ * Resolve any rev arguments. If we have a dashdash, then everything up
+ * to it must resolve as a rev. If not, then we stop at the first
+ * non-rev and assume everything else is a path.
+ */
for (i = 0; i < argc; i++) {
const char *arg = argv[i];
unsigned char sha1[20];
@@ -1158,13 +1173,14 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
if (!strcmp(arg, "--")) {
i++;
- seen_dashdash = 1;
break;
}
- /* Stop at the first non-rev */
- if (get_sha1_with_context(arg, 0, sha1, &oc))
+ if (get_sha1_with_context(arg, 0, sha1, &oc)) {
+ if (seen_dashdash)
+ die(_("unable to resolve revision: %s"), arg);
break;
+ }
object = parse_object_or_die(sha1, arg);
if (!seen_dashdash)
@@ -1172,7 +1188,10 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
add_object_array_with_path(object, arg, &list, oc.mode, oc.path);
}
- /* The rest are paths */
+ /*
+ * Anything left over is presumed to be a path. But in the non-dashdash
+ * "do what I mean" case, we verify and complain when that isn't true.
+ */
if (!seen_dashdash) {
int j;
for (j = i; j < argc; j++)
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 2c1f7373e..a6011f9b1 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -982,6 +982,39 @@ test_expect_success 'grep -e -- -- path' '
test_cmp expected actual
'
+test_expect_success 'dashdash disambiguates rev as rev' '
+ test_when_finished "rm -f master" &&
+ echo content >master &&
+ echo master:hello.c >expect &&
+ git grep -l o master -- hello.c >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'dashdash disambiguates pathspec as pathspec' '
+ test_when_finished "git rm -f master" &&
+ echo content >master &&
+ git add master &&
+ echo master:content >expect &&
+ git grep o -- master >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'report bogus arg without dashdash' '
+ test_must_fail git grep o does-not-exist
+'
+
+test_expect_success 'report bogus rev with dashdash' '
+ test_must_fail git grep o hello.c --
+'
+
+test_expect_success 'allow non-existent path with dashdash' '
+ # We need a real match so grep exits with success.
+ tree=$(git ls-tree HEAD |
+ sed s/hello.c/not-in-working-tree/ |
+ git mktree) &&
+ git grep o "$tree" -- not-in-working-tree
+'
+
test_expect_success 'grep --no-index pattern -- path' '
rm -fr non &&
mkdir -p non/git &&
--
2.12.0.rc1.471.ga79ec8999
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 5/7] grep: fix "--" rev/pathspec disambiguation
2017-02-14 6:05 ` [PATCH 5/7] grep: fix "--" rev/pathspec disambiguation Jeff King
@ 2017-02-14 18:56 ` Brandon Williams
2017-02-14 19:51 ` Jeff King
2017-02-14 19:18 ` Junio C Hamano
1 sibling, 1 reply; 22+ messages in thread
From: Brandon Williams @ 2017-02-14 18:56 UTC (permalink / raw)
To: Jeff King; +Cc: Jonathan Tan, git, gitster
On 02/14, Jeff King wrote:
> - /* Check revs and then paths */
> + /*
> + * We have to find "--" in a separate pass, because its presence
> + * influences how we will parse arguments that come before it.
> + */
> + for (i = 0; i < argc; i++) {
> + if (!strcmp(argv[i], "--")) {
> + seen_dashdash = 1;
> + break;
> + }
> + }
So this simply checks if "--" is an argument that was provided. This
then allows grep to know ahead of time how to handle revs/paths
preceding a "--" or in the absences of the "--". Seems sensible to me.
> +
> + /*
> + * Resolve any rev arguments. If we have a dashdash, then everything up
> + * to it must resolve as a rev. If not, then we stop at the first
> + * non-rev and assume everything else is a path.
> + */
> for (i = 0; i < argc; i++) {
> const char *arg = argv[i];
> unsigned char sha1[20];
> @@ -1158,13 +1173,14 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>
> if (!strcmp(arg, "--")) {
> i++;
> - seen_dashdash = 1;
> break;
> }
>
> - /* Stop at the first non-rev */
> - if (get_sha1_with_context(arg, 0, sha1, &oc))
> + if (get_sha1_with_context(arg, 0, sha1, &oc)) {
> + if (seen_dashdash)
> + die(_("unable to resolve revision: %s"), arg);
> break;
> + }
>
> object = parse_object_or_die(sha1, arg);
> if (!seen_dashdash)
> @@ -1172,7 +1188,10 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
> add_object_array_with_path(object, arg, &list, oc.mode, oc.path);
> }
>
> - /* The rest are paths */
> + /*
> + * Anything left over is presumed to be a path. But in the non-dashdash
> + * "do what I mean" case, we verify and complain when that isn't true.
> + */
--
Brandon Williams
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 5/7] grep: fix "--" rev/pathspec disambiguation
2017-02-14 18:56 ` Brandon Williams
@ 2017-02-14 19:51 ` Jeff King
0 siblings, 0 replies; 22+ messages in thread
From: Jeff King @ 2017-02-14 19:51 UTC (permalink / raw)
To: Brandon Williams; +Cc: Jonathan Tan, git, gitster
On Tue, Feb 14, 2017 at 10:56:21AM -0800, Brandon Williams wrote:
> On 02/14, Jeff King wrote:
> > - /* Check revs and then paths */
> > + /*
> > + * We have to find "--" in a separate pass, because its presence
> > + * influences how we will parse arguments that come before it.
> > + */
> > + for (i = 0; i < argc; i++) {
> > + if (!strcmp(argv[i], "--")) {
> > + seen_dashdash = 1;
> > + break;
> > + }
> > + }
>
> So this simply checks if "--" is an argument that was provided. This
> then allows grep to know ahead of time how to handle revs/paths
> preceding a "--" or in the absences of the "--". Seems sensible to me.
By the way, we have to check again later for "--" when parsing the revs
themselves. In theory you could set seen_dashdash to the offset of the
dashdash in the array, and do the iteration more like:
for (i = 0; i < dashdash_pos; i++)
handle_rev(argv[i]);
for (i = dashdash_pos + 1; i < argc; i++)
handle_path(argv[i]);
But our loops also handle the case where there is no "--" at all, and I
think that approach ends up convoluting the logic. I didn't go very far
in that direction before giving it up, though.
-Peff
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 5/7] grep: fix "--" rev/pathspec disambiguation
2017-02-14 6:05 ` [PATCH 5/7] grep: fix "--" rev/pathspec disambiguation Jeff King
2017-02-14 18:56 ` Brandon Williams
@ 2017-02-14 19:18 ` Junio C Hamano
1 sibling, 0 replies; 22+ messages in thread
From: Junio C Hamano @ 2017-02-14 19:18 UTC (permalink / raw)
To: Jeff King; +Cc: Jonathan Tan, git
Jeff King <peff@peff.net> writes:
> If we see "git grep pattern rev -- file" then we apply the
> usual rev/pathspec disambiguation rules: any "rev" before
> the "--" must be a revision, and we do not need to apply the
> verify_non_filename() check.
>
> But there are two bugs here:
>
> 1. We keep a seen_dashdash flag to handle this case, but
> we set it in the same left-to-right pass over the
> arguments in which we parse "rev".
>
> So when we see "rev", we do not yet know that there is
> a "--", and we mistakenly complain if there is a
> matching file.
>
> We can fix this by making a preliminary pass over the
> arguments to find the "--", and only then checking the rev
> arguments.
>
> 2. If we can't resolve "rev" but there isn't a dashdash,
> that's OK. We treat it like a path, and complain later
> if it doesn't exist.
>
> But if there _is_ a dashdash, then we know it must be a
> rev, and should treat it as such, complaining if it
> does not resolve. The current code instead ignores it
> and tries to treat it like a path.
>
> This patch fixes both bugs, and tries to comment the parsing
> flow a bit better.
Good. Thanks.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 6/7] grep: avoid resolving revision names in --no-index case
2017-02-14 6:00 ` [PATCH 0/7] grep rev/path parsing fixes Jeff King
` (4 preceding siblings ...)
2017-02-14 6:05 ` [PATCH 5/7] grep: fix "--" rev/pathspec disambiguation Jeff King
@ 2017-02-14 6:07 ` Jeff King
2017-02-14 16:53 ` Jonathan Tan
2017-02-14 6:08 ` [PATCH 7/7] grep: do not diagnose misspelt revs with --no-index Jeff King
` (2 subsequent siblings)
8 siblings, 1 reply; 22+ messages in thread
From: Jeff King @ 2017-02-14 6:07 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, gitster
We disallow the use of revisions with --no-index, but we
don't actually check and complain until well after we've
parsed the revisions.
This is the cause of a few problems:
1. We shouldn't be calling get_sha1() at all when we aren't
in a repository, as it might access the ref or object
databases. For now, this should generally just return
failure, but eventually it will become a BUG().
2. When there's a "--" disambiguator and you're outside a
repository, we'll complain early with "unable to resolve
revision". But we can give a much more specific error.
3. When there isn't a "--" disambiguator, we still do the
normal rev/path checks. This is silly, as we know we
cannot have any revs with --no-index. Everything we see
must be a path.
Outside of a repository this doesn't matter (since we
know it won't resolve), but inside one, we may complain
unnecessarily if a filename happens to also match a
refname.
This patch skips the get_sha1() call entirely in the
no-index case, and behaves as if it failed (with the
exception of giving a better error message).
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/grep.c | 6 ++++++
t/t7810-grep.sh | 13 +++++++++++++
2 files changed, 19 insertions(+)
diff --git a/builtin/grep.c b/builtin/grep.c
index e83b33bda..c4c632594 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1176,6 +1176,12 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
break;
}
+ if (!use_index) {
+ if (seen_dashdash)
+ die(_("--no-index cannot be used with revs"));
+ break;
+ }
+
if (get_sha1_with_context(arg, 0, sha1, &oc)) {
if (seen_dashdash)
die(_("unable to resolve revision: %s"), arg);
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index a6011f9b1..c051c7ee8 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -1030,6 +1030,19 @@ test_expect_success 'grep --no-index pattern -- path' '
)
'
+test_expect_success 'grep --no-index complains of revs' '
+ test_must_fail git grep --no-index o master -- 2>err &&
+ test_i18ngrep "no-index cannot be used with revs" err
+'
+
+test_expect_success 'grep --no-index prefers paths to revs' '
+ test_when_finished "rm -f master" &&
+ echo content >master &&
+ echo master:content >expect &&
+ git grep --no-index o master >actual &&
+ test_cmp expect actual
+'
+
cat >expected <<EOF
hello.c:int main(int argc, const char **argv)
hello.c: printf("Hello world.\n");
--
2.12.0.rc1.471.ga79ec8999
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 6/7] grep: avoid resolving revision names in --no-index case
2017-02-14 6:07 ` [PATCH 6/7] grep: avoid resolving revision names in --no-index case Jeff King
@ 2017-02-14 16:53 ` Jonathan Tan
2017-02-14 18:04 ` Jeff King
0 siblings, 1 reply; 22+ messages in thread
From: Jonathan Tan @ 2017-02-14 16:53 UTC (permalink / raw)
To: Jeff King; +Cc: git, gitster
On 02/13/2017 10:07 PM, Jeff King wrote:
> diff --git a/builtin/grep.c b/builtin/grep.c
> index e83b33bda..c4c632594 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -1176,6 +1176,12 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
> break;
> }
>
> + if (!use_index) {
> + if (seen_dashdash)
> + die(_("--no-index cannot be used with revs"));
There is a subsequent check that prints "--no-index or --untracked
cannot be used with revs." - maybe we should just expand this part to
incorporate that case. (That is, write `if (!use_index || untracked)`
instead of `if (!use_index)`.) This also allows us to preserve the error
message, which might be useful for someone using a translated version of
Git.
> + break;
> + }
> +
> if (get_sha1_with_context(arg, 0, sha1, &oc)) {
> if (seen_dashdash)
> die(_("unable to resolve revision: %s"), arg);
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 6/7] grep: avoid resolving revision names in --no-index case
2017-02-14 16:53 ` Jonathan Tan
@ 2017-02-14 18:04 ` Jeff King
2017-02-14 18:19 ` Jonathan Tan
0 siblings, 1 reply; 22+ messages in thread
From: Jeff King @ 2017-02-14 18:04 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, gitster
On Tue, Feb 14, 2017 at 08:53:04AM -0800, Jonathan Tan wrote:
> On 02/13/2017 10:07 PM, Jeff King wrote:
> > diff --git a/builtin/grep.c b/builtin/grep.c
> > index e83b33bda..c4c632594 100644
> > --- a/builtin/grep.c
> > +++ b/builtin/grep.c
> > @@ -1176,6 +1176,12 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
> > break;
> > }
> >
> > + if (!use_index) {
> > + if (seen_dashdash)
> > + die(_("--no-index cannot be used with revs"));
>
> There is a subsequent check that prints "--no-index or --untracked cannot be
> used with revs." - maybe we should just expand this part to incorporate that
> case. (That is, write `if (!use_index || untracked)` instead of `if
> (!use_index)`.) This also allows us to preserve the error message, which
> might be useful for someone using a translated version of Git.
I wasn't sure if we wanted to treat "untracked" in the same way.
Certainly we can catch the error here for the seen_dashdash case, but is
it also the case that:
echo content >master
git grep --untracked pattern master
should treat "master" as a path?
-Peff
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 6/7] grep: avoid resolving revision names in --no-index case
2017-02-14 18:04 ` Jeff King
@ 2017-02-14 18:19 ` Jonathan Tan
2017-02-14 21:54 ` [PATCH 8/7] grep: treat revs the same for --untracked as for --no-index Jeff King
0 siblings, 1 reply; 22+ messages in thread
From: Jonathan Tan @ 2017-02-14 18:19 UTC (permalink / raw)
To: Jeff King; +Cc: git, gitster
On 02/14/2017 10:04 AM, Jeff King wrote:
> On Tue, Feb 14, 2017 at 08:53:04AM -0800, Jonathan Tan wrote:
>
>> On 02/13/2017 10:07 PM, Jeff King wrote:
>>> diff --git a/builtin/grep.c b/builtin/grep.c
>>> index e83b33bda..c4c632594 100644
>>> --- a/builtin/grep.c
>>> +++ b/builtin/grep.c
>>> @@ -1176,6 +1176,12 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>>> break;
>>> }
>>>
>>> + if (!use_index) {
>>> + if (seen_dashdash)
>>> + die(_("--no-index cannot be used with revs"));
>>
>> There is a subsequent check that prints "--no-index or --untracked cannot be
>> used with revs." - maybe we should just expand this part to incorporate that
>> case. (That is, write `if (!use_index || untracked)` instead of `if
>> (!use_index)`.) This also allows us to preserve the error message, which
>> might be useful for someone using a translated version of Git.
>
> I wasn't sure if we wanted to treat "untracked" in the same way.
> Certainly we can catch the error here for the seen_dashdash case, but is
> it also the case that:
>
> echo content >master
> git grep --untracked pattern master
>
> should treat "master" as a path?
>
> -Peff
It is already the case that it cannot be treated as a rev:
$ git grep --untracked pattern master
fatal: --no-index or --untracked cannot be used with revs.
So I think it would be better if it was treated as a path, for
consistency with --no-index. I'm OK either way, though.
^ permalink raw reply [flat|nested] 22+ messages in thread* [PATCH 8/7] grep: treat revs the same for --untracked as for --no-index
2017-02-14 18:19 ` Jonathan Tan
@ 2017-02-14 21:54 ` Jeff King
2017-02-14 21:58 ` Junio C Hamano
0 siblings, 1 reply; 22+ messages in thread
From: Jeff King @ 2017-02-14 21:54 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, gitster
On Tue, Feb 14, 2017 at 10:19:56AM -0800, Jonathan Tan wrote:
> > I wasn't sure if we wanted to treat "untracked" in the same way.
> > Certainly we can catch the error here for the seen_dashdash case, but is
> > it also the case that:
> >
> > echo content >master
> > git grep --untracked pattern master
> >
> > should treat "master" as a path?
>
> It is already the case that it cannot be treated as a rev:
>
> $ git grep --untracked pattern master
> fatal: --no-index or --untracked cannot be used with revs.
>
> So I think it would be better if it was treated as a path, for consistency
> with --no-index. I'm OK either way, though.
Right, it's always been disallowed. But the early detection changes a
few user-visible behaviors like the exact error message, and how
disambiguation works (see below). I think the arguments for making that
change for --no-index are stronger than for --untracked. But it probably
makes sense for --untracked, too.
Here's a patch on top of my series that lumps the two together again. It
_could_ be squashed into the earlier patch, but I think I prefer keeping
it separate.
-- >8 --
Subject: [PATCH] grep: treat revs the same for --untracked as for --no-index
git-grep has always disallowed grepping in a tree (as
opposed to the working directory) with both --untracked
and --no-index. But we traditionally did so by first
collecting the revs, and then complaining when any were
provided.
The --no-index option recently learned to detect revs
much earlier. This has two user-visible effects:
- we don't bother to resolve revision names at all. So
when there's a rev/path ambiguity, we always choose to
treat it as a path.
- likewise, when you do specify a revision without "--",
the error you get is "no such path" and not "--untracked
cannot be used with revs".
The rationale for doing this with --no-index is that it is
meant to be used outside a repository, and so parsing revs
at all does not make sense.
This patch gives --untracked the same treatment. While it
_is_ meant to be used in a repository, it is explicitly
about grepping the non-repository contents. Telling the user
"we found a rev, but you are not allowed to use revs" is
not really helpful compared to "we treated your argument as
a path, and could not find it".
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/grep.c | 10 +++++-----
t/t7810-grep.sh | 2 +-
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 1454bef49..9304c33e7 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -967,6 +967,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
int dummy;
int use_index = 1;
int pattern_type_arg = GREP_PATTERN_TYPE_UNSPECIFIED;
+ int allow_revs;
struct option options[] = {
OPT_BOOL(0, "cached", &cached,
@@ -1165,6 +1166,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
* to it must resolve as a rev. If not, then we stop at the first
* non-rev and assume everything else is a path.
*/
+ allow_revs = use_index && !untracked;
for (i = 0; i < argc; i++) {
const char *arg = argv[i];
unsigned char sha1[20];
@@ -1176,9 +1178,9 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
break;
}
- if (!use_index) {
+ if (!allow_revs) {
if (seen_dashdash)
- die(_("--no-index cannot be used with revs"));
+ die(_("--no-index or --untracked cannot be used with revs"));
break;
}
@@ -1201,7 +1203,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
if (!seen_dashdash) {
int j;
for (j = i; j < argc; j++)
- verify_filename(prefix, argv[j], j == i && use_index);
+ verify_filename(prefix, argv[j], j == i && allow_revs);
}
parse_pathspec(&pathspec, 0,
@@ -1273,8 +1275,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
if (!use_index || untracked) {
int use_exclude = (opt_exclude < 0) ? use_index : !!opt_exclude;
- if (list.nr)
- die(_("--no-index or --untracked cannot be used with revs."));
hit = grep_directory(&opt, &pathspec, use_exclude, use_index);
} else if (0 <= opt_exclude) {
die(_("--[no-]exclude-standard cannot be used for tracked contents."));
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 0ff9f6cae..cee42097b 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -1032,7 +1032,7 @@ test_expect_success 'grep --no-index pattern -- path' '
test_expect_success 'grep --no-index complains of revs' '
test_must_fail git grep --no-index o master -- 2>err &&
- test_i18ngrep "no-index cannot be used with revs" err
+ test_i18ngrep "cannot be used with revs" err
'
test_expect_success 'grep --no-index prefers paths to revs' '
--
2.12.0.rc1.479.g59880b11e
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 8/7] grep: treat revs the same for --untracked as for --no-index
2017-02-14 21:54 ` [PATCH 8/7] grep: treat revs the same for --untracked as for --no-index Jeff King
@ 2017-02-14 21:58 ` Junio C Hamano
0 siblings, 0 replies; 22+ messages in thread
From: Junio C Hamano @ 2017-02-14 21:58 UTC (permalink / raw)
To: Jeff King; +Cc: Jonathan Tan, git
Jeff King <peff@peff.net> writes:
> ...
> The rationale for doing this with --no-index is that it is
> meant to be used outside a repository, and so parsing revs
> at all does not make sense.
>
> This patch gives --untracked the same treatment. While it
> _is_ meant to be used in a repository, it is explicitly
> about grepping the non-repository contents. Telling the user
> "we found a rev, but you are not allowed to use revs" is
> not really helpful compared to "we treated your argument as
> a path, and could not find it".
Yup, both sounds very sensible. Thanks.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 7/7] grep: do not diagnose misspelt revs with --no-index
2017-02-14 6:00 ` [PATCH 0/7] grep rev/path parsing fixes Jeff King
` (5 preceding siblings ...)
2017-02-14 6:07 ` [PATCH 6/7] grep: avoid resolving revision names in --no-index case Jeff King
@ 2017-02-14 6:08 ` Jeff King
2017-02-14 6:10 ` [PATCH 0/7] grep rev/path parsing fixes Jeff King
2017-02-14 16:58 ` Jonathan Tan
8 siblings, 0 replies; 22+ messages in thread
From: Jeff King @ 2017-02-14 6:08 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, gitster
If we are using --no-index, then our arguments cannot be
revs in the first place. Not only is it pointless to
diagnose them, but if we are not in a repository, we should
not be trying to resolve any names.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/grep.c | 2 +-
t/t7810-grep.sh | 5 +++++
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index c4c632594..1454bef49 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1201,7 +1201,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
if (!seen_dashdash) {
int j;
for (j = i; j < argc; j++)
- verify_filename(prefix, argv[j], j == i);
+ verify_filename(prefix, argv[j], j == i && use_index);
}
parse_pathspec(&pathspec, 0,
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index c051c7ee8..0ff9f6cae 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -1043,6 +1043,11 @@ test_expect_success 'grep --no-index prefers paths to revs' '
test_cmp expect actual
'
+test_expect_success 'grep --no-index does not "diagnose" revs' '
+ test_must_fail git grep --no-index o :1:hello.c 2>err &&
+ test_i18ngrep ! -i "did you mean" err
+'
+
cat >expected <<EOF
hello.c:int main(int argc, const char **argv)
hello.c: printf("Hello world.\n");
--
2.12.0.rc1.471.ga79ec8999
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 0/7] grep rev/path parsing fixes
2017-02-14 6:00 ` [PATCH 0/7] grep rev/path parsing fixes Jeff King
` (6 preceding siblings ...)
2017-02-14 6:08 ` [PATCH 7/7] grep: do not diagnose misspelt revs with --no-index Jeff King
@ 2017-02-14 6:10 ` Jeff King
2017-02-14 16:58 ` Jonathan Tan
8 siblings, 0 replies; 22+ messages in thread
From: Jeff King @ 2017-02-14 6:10 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, gitster
On Tue, Feb 14, 2017 at 01:00:21AM -0500, Jeff King wrote:
> On Mon, Feb 13, 2017 at 08:20:37PM -0500, Jeff King wrote:
>
> > > If there is a repo and "foo" is a rev, the "--no-index or --untracked
> > > cannot be used with revs." error would occur. If there is a repo and
> > > "foo" is not a rev, this command would proceed as usual. If there is no
> > > repo, the "setup_git_env called without repository" error would occur.
> > > (This is my understanding from reading the code - I haven't tested it
> > > out.)
> >
> > Yes, it's easy to see that "git grep --no-index foo bar" outside of a
> > repo generates the same BUG. I suspect that "--no-index" should just
> > disable looking up revs entirely, even if we are actually in a
> > repository directory.
>
> I've fixed that, along with a few other bugs and cleanups. The complete
> series is below. Patch 2 is your (untouched) patch. My suggestions for
> your test are in patch 3, which can either remain on its own or be
> squashed in.
>
> [1/7]: grep: move thread initialization a little lower
> [2/7]: grep: do not unnecessarily query repo for "--"
> [3/7]: t7810: make "--no-index --" test more robust
> [4/7]: grep: re-order rev-parsing loop
> [5/7]: grep: fix "--" rev/pathspec disambiguation
> [6/7]: grep: avoid resolving revision names in --no-index case
> [7/7]: grep: do not diagnose misspelt revs with --no-index
>
> builtin/grep.c | 78 +++++++++++++++++++++++++++++++++++++++------------------
> t/t7810-grep.sh | 66 ++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 119 insertions(+), 25 deletions(-)
Just to clarify: these are all existing bugs, and I think these are
probably maint-worthy patches (even the --no-index ones; though we don't
BUG on the out-of-repo without the patch from 'next', the code is still
doing the wrong thing in subtle ways).
But AFAIK they are all much older bugs than the upcoming v2.12, so there
is no pressing need to fit them into the upcoming release.
-Peff
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 0/7] grep rev/path parsing fixes
2017-02-14 6:00 ` [PATCH 0/7] grep rev/path parsing fixes Jeff King
` (7 preceding siblings ...)
2017-02-14 6:10 ` [PATCH 0/7] grep rev/path parsing fixes Jeff King
@ 2017-02-14 16:58 ` Jonathan Tan
8 siblings, 0 replies; 22+ messages in thread
From: Jonathan Tan @ 2017-02-14 16:58 UTC (permalink / raw)
To: Jeff King; +Cc: git, gitster
On 02/13/2017 10:00 PM, Jeff King wrote:
> I've fixed that, along with a few other bugs and cleanups. The complete
> series is below. Patch 2 is your (untouched) patch. My suggestions for
> your test are in patch 3, which can either remain on its own or be
> squashed in.
>
> [1/7]: grep: move thread initialization a little lower
> [2/7]: grep: do not unnecessarily query repo for "--"
> [3/7]: t7810: make "--no-index --" test more robust
> [4/7]: grep: re-order rev-parsing loop
> [5/7]: grep: fix "--" rev/pathspec disambiguation
> [6/7]: grep: avoid resolving revision names in --no-index case
> [7/7]: grep: do not diagnose misspelt revs with --no-index
Thanks - these look good to me. I replied to 6/7 with a comment, but I
also think that these are good as-is. Also, 3/7 can probably be squashed in.
^ permalink raw reply [flat|nested] 22+ messages in thread