Re: [PATCH/RFC] grep: add a grep.patternType configuration setting

From: J Smith <dark.panda@gmail.com>
Date: Aug 1, 2012, 22:49 UTC
Message-ID: <CADFUPgdX44pCFhytPj-hHSCPH9UHNBKk5pYkpses86M1ntxvyA@mail.gmail.com>
In-Reply-To: <7vpq7ae0pj.fsf@alter.siamese.dyndns.org>

On Wed, Aug 1, 2012 at 5:55 PM, Junio C Hamano <gitster@pobox.com> wrote:

>
> As the basic structure and the direction looks good, let's start
> nitpicking ;-)

Sounds good.

> We tend to write the commit log message in imperative mood, as if
> you are giving an order to the codebase to "behave this way!".  Also
> we tend to give the justification behind the change first and then
> present the solution.

Sounds good to me. I'll re-word the commit messages in future revisions of the patch.

> With this round, we are not updating an existing a bool variable,
> but are introducing a brand new one; does it still make sense to
> support the boolean values for this new variable?

Yeah, I thought about that, having noticed in your edited patch that the boolean options were still in there for patternType. I do think it would be useful to have a way to get back to the default settings, say on a per-repo basis to override a global setting. I was thinking that a "false" value could provide that, but perhaps a value of "default" would make more sense?

> You want a test that runs "git -c grep.patternType=basic grep -P" or
> something, guarded with LIBPCRE prerequisite, to make sure pcre
> patterns are used because command line -P trumps over configured
> default, too.

Will add.

> Besides, I do not think you are testing the right thing in them
> anyway (notice the lack of "grep." prefix).

Ah geez. Yeah, that's just stupidity.

Show 12 quoted lines

> What does this test?  The last one wins?
>
> For the command line flags, people can do "alias g 'git grep -E'"
> and then countermand the flags in the alias by appending a
> contradicting flag when using it, e.g. "g -G", last one wins is a
> defined and useful semantics, but for configuration variables that
> are meant to take a single value, I do not think we give such a
> strong guarantee on ordering (it may happen to work by accident,
> though).
>
> I would _not_ strongly suggest removing this test, but instead wait
> until we hear from others, as they may disagree.

I'll wait for others and we'll see. I'm not overly attached to them or anything.

Show 7 quoted lines

> As you are expecting the "last one wins" behaviour among
> configuration variables, running a test with -P option would not let
> you catch bugs coming from potentially screwed-up precedence between
> the configuration and command line flags, would it?  At least, leave
> the "-c grep.patterntype=perl" out from here to make sure what the
> variable and the flag tell the command conflict with each other.  I
> would also prefer to see only one "-c grep.patterntype=<foo>" used.

Ah, yes, that was how the test was supposed to be written. That was an oversight.

Show 5 quoted lines

> What does this test?  patterntype trumps extendedregexp?
>
> That may sit better next to the earlier "patterntype says basic but
> extendedregexp says true" test, if you can move this test easily
> there.

Yep, I'll move it around.

Cheers