On Wed, Aug 1, 2012 at 5:55 PM, Junio C Hamano <gits...@pobox.com> wrote:
> As the basic structure and the direction looks good, let's start
> nitpicking ;-)
> 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.
> 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.
> 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,
> 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.
> 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
> 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
Yep, I'll move it around.
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html