Thomas Gummerer [off-list ref] writes:
On 01/11, Duy Nguyen wrote:
quoted
On Sun, Jan 10, 2016 at 9:19 PM, Thomas Gummerer [off-list ref] wrote:
quoted
Currently when git grep is used outside of a git repository without the
--no-index option git simply dies. For convenience, implicitly make git
grep behave like git grep --no-index when it is called outside of a git
repository.
Should we have a line about this behavior in git-grep.txt, maybe the
description section?
Yes good point, the behavior change should definitely be documented.
quoted
I wonder if anybody wants the old behavior (e.g.
non-zero exit code when running outside a repo). If there is such a
case (*), we may need an option to revert it back (--no-no-index seems
ridiculous, maybe --use-index). The safest way though, is introduce a
new option like --use-index=<always|optional|never> then you can make
an grep alias with --use-index=optional.
You're right. I couldn't think of a reason why someone would rely on
the old behavior, but maybe I missed something. I like the idea of
introducing the --use-index=... option.
I don't like that, though ;-)
"We run 'git grep' in random places and relied on it to fail when
run in somewhere not under control of Git." feels so flawed at
multiple levels that I doubt it deserves to be kept working. For
one thing, "git grep" is not the way to tell something is under
control of Git (rev-parse would be a better thing for scriptor to
use). For another, how would such a script tell between "not a
git repository" and there was no hits?
So I do agree that automatic fallback needs to be documented and
advertised as a feature (or even a bugfix), I do not think we want
to add knobs to keep such a broken script working.
How should we handle priority between --no-index and --use-index,
should we just give --no-index priority if it is set and ignore the
new --use-index option, or is there some other way?
quoted
(*) I've been hitting really weird real-world use cases so I'm a bit paranoid..
--
Duy
On 01/11, Junio C Hamano wrote:
Thomas Gummerer [off-list ref] writes:
quoted
On 01/11, Duy Nguyen wrote:
quoted
On Sun, Jan 10, 2016 at 9:19 PM, Thomas Gummerer [off-list ref] wrote:
quoted
Currently when git grep is used outside of a git repository without the
--no-index option git simply dies. For convenience, implicitly make git
grep behave like git grep --no-index when it is called outside of a git
repository.
Should we have a line about this behavior in git-grep.txt, maybe the
description section?
Yes good point, the behavior change should definitely be documented.
quoted
I wonder if anybody wants the old behavior (e.g.
non-zero exit code when running outside a repo). If there is such a
case (*), we may need an option to revert it back (--no-no-index seems
ridiculous, maybe --use-index). The safest way though, is introduce a
new option like --use-index=<always|optional|never> then you can make
an grep alias with --use-index=optional.
You're right. I couldn't think of a reason why someone would rely on
the old behavior, but maybe I missed something. I like the idea of
introducing the --use-index=... option.
I don't like that, though ;-)
"We run 'git grep' in random places and relied on it to fail when
run in somewhere not under control of Git." feels so flawed at
multiple levels that I doubt it deserves to be kept working. For
one thing, "git grep" is not the way to tell something is under
control of Git (rev-parse would be a better thing for scriptor to
use). For another, how would such a script tell between "not a
git repository" and there was no hits?
I agree that scripts don't deserve to be kept working in that case.
What about a user though who accidentally runs git grep outside of a
repository, and is usually warned by git failing quickly, whereas with
the changed behavior some time might go by until the user realizes the
error. Not sure if we want to support this use case or not?
So I do agree that automatic fallback needs to be documented and
advertised as a feature (or even a bugfix), I do not think we want
to add knobs to keep such a broken script working.
quoted
How should we handle priority between --no-index and --use-index,
should we just give --no-index priority if it is set and ignore the
new --use-index option, or is there some other way?
quoted
(*) I've been hitting really weird real-world use cases so I'm a bit paranoid..
--
Duy
--
Thomas Gummerer
On Mon, Jan 11, 2016 at 06:48:17PM +0100, Thomas Gummerer wrote:
quoted
"We run 'git grep' in random places and relied on it to fail when
run in somewhere not under control of Git." feels so flawed at
multiple levels that I doubt it deserves to be kept working. For
one thing, "git grep" is not the way to tell something is under
control of Git (rev-parse would be a better thing for scriptor to
use). For another, how would such a script tell between "not a
git repository" and there was no hits?
I agree that scripts don't deserve to be kept working in that case.
What about a user though who accidentally runs git grep outside of a
repository, and is usually warned by git failing quickly, whereas with
the changed behavior some time might go by until the user realizes the
error. Not sure if we want to support this use case or not?
Yeah, I don't think git would be _wrong_ here, but I could certainly see
it being annoying. Several times a week I probably run `git grep` in my
home directory, and after seeing its error, realize "oops, I meant to
`cd git`".
Having it spew nonsense results, and/or appear to hang while it
literally reads every file on my disk would be at least a minor
annoyance.
But I don't think any kind of command-line flag would help that; I'm not
going to start typing "git grep --use-index=never" for every invocation.
I think the only sensible mitigation would be a config option, so that
people who rarely use `--no-index` (and are OK with having to specify
it) do not get punished by false positives.
I dunno. Maybe I would find the new behavior so useful I would be OK
with the occasional false-positive. But when we make a release with the
new behavior and somebody _does_ complain, it sure would be nice not to
have to say "deal with it; it's the new behavior and there is no escape
hatch".
-Peff