Re: [PATCH 0/7] grep rev/path parsing fixes
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.
Re: [PATCH 0/7] grep rev/path parsing fixes
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
[PATCH 0/7] grep rev/path parsing fixes
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(-) -Peff
