On Wed, Aug 1, 2012 at 5:55 PM, Junio C Hamano <gitster@xxxxxxxxx> 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. > 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. > 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. > 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html