Re: [PATCH/RFC] grep: add a grep.patternType configuration setting
Alright, I have revised the patch and fixed up the nits that were picked and made a quick modification. I've added a setting for grep.patternType for default which can restore the default grep pattern matching behaviour and restores the functionality back to grep.extendedRegexp. I added this functionality for situations like where you would have grep.patternType set to, say, perl in your $HOME/.gitconfig but don't want that functionality set in a specific repo and would rather to have it fall back to the older grep.extendedRegexp behaviour so you can set it to default in the repo's .git/config. This change also lets us determine the final set of pattern type options in one place rather than the current code which does two checks -- once when we call grep_config to determine the configuration options and then another a few lines later when we call it for the arguments given to grep. Now we capture the values we receive from grep.patternType and grep.extendedRegexp in the grep_opt struct as pattern_type_option and extended_regexp_option, capture the pattern type argument given to the command itself, and then make the final determination for the options to be used in one place rather than the current manner. I think it should be more obvious this way. I'll post the latest patch shortly for review if this sounds reasonable. Cheers. -- 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
Re: [PATCH/RFC] grep: add a grep.patternType configuration setting
J Smith dark.pa...@gmail.com writes:
As the basic structure and the direction looks good, let's start
nitpicking ;-)
Adds the grep.patternType configuration setting which sets the default
pattern matching behavior. The values basic, extended, fixed, and
perl can be used to set --basic-regexp, --extended-regexp,
--fixed-strings, and --perl-regexp options by default respectively.
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.
There is grep.extendedRegexp configuration variable to
enable the -E command line flag by default, but there is no
equivalent for the -P (pcre) flag. We could keep adding
grep.fooRegexp variables for different regular expression
variants, but that will be unwieldy.
Instead, add a grep.patternType variable that can be set
to basic, extended, fixed and perl to use
--basic-regexp, --extended-regexp, --fixed-strings,
and --perl-regexp options by default respectively.
Ignore grep.extendedRegexp when grep.patternType is set.
A value of true is equivalent to extended as with grep.extendedRegexp,
and a value of false leaves the pattern type as unspecified and follows
the default grep behavior.
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?
This setting overrides the value set in grep.extendedRegexp which will
be ignored completely if grep.patternType is set.
---
Sign-off?
Documentation/config.txt | 11 ++-
Documentation/git-grep.txt | 11 ++-
builtin/grep.c | 106 -
grep.h | 9 +++
t/t7810-grep.sh| 187
+
5 files changed, 284 insertions(+), 40 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index a95e5a4..38d56d8 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1210,8 +1210,17 @@ gitweb.snapshot::
grep.lineNumber::
If set to true, enable '-n' option by default.
+grep.patternType::
+ Sets the default matching behavior. This option can be set to a
+ boolean value or one of 'basic', 'extended', 'fixed', or 'perl'
+ which will enable the '--basic-regexp', '--extended-regexp',
+ '--fixed-strings' or '--perl-regexp' options accordingly. The value
+ of true is equivalent to 'extended' while false leaves the
+ settings in their default state.
Perhaps s/Sets the/The/ or at least s/Sets/Set/ (notice that the
description for grep.extendedRegexp says enable foo, not enables
foo).
The same comment as above applies to the boolean-ness part.
grep.extendedRegexp::
- If set to true, enable '--extended-regexp' option by default.
+ If set to true, enable '--extended-regexp' option by default. This
+ option is ignored when the 'grep.patternType' option is set.
We are not going to make grep.patternType a boolean, so when ... is
set is fine, but if we were to allow grep.patternType to be set to
false, the description gives ambiguity to some readers who do.
Perhaps s/is set/is given/ is safer.
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 3bec036..f56f67f 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -42,8 +42,17 @@ CONFIGURATION
grep.lineNumber::
If set to true, enable '-n' option by default.
+grep.patternType::
...
grep.extendedRegexp::
- If set to true, enable '--extended-regexp' option by default.
+ If set to true, enable '--extended-regexp' option by default. This
+ option is ignored when the 'grep.patternType' option is set.
Likewise.
diff --git a/builtin/grep.c b/builtin/grep.c
index 29adb0a..1de7e76 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -260,6 +260,55 @@ static int wait_all(void)
}
#endif
+static int parse_pattern_type_arg(const char *opt, const char *arg)
+{
+ switch (git_config_maybe_bool(opt, arg)) {
+ case 1:
+ return GREP_PATTERN_TYPE_ERE;
+ case 0:
+ return GREP_PATTERN_TYPE_UNSPECIFIED;
+ default:
+ if (!strcmp(arg, basic))
+ return GREP_PATTERN_TYPE_BRE;
+ else if (!strcmp(arg, extended))
+ return GREP_PATTERN_TYPE_ERE;
+ else if (!strcmp(arg, fixed))
+ return GREP_PATTERN_TYPE_FIXED;
+ else if (!strcmp(arg, perl))
+ return GREP_PATTERN_TYPE_PCRE;
+ die(bad %s argument: %s, opt, arg);
+ }
Let's not do maybe-bool, as we are not upgrading an old bool-only
variable any more.
+static void grep_pattern_type_options(const int pattern_type,
Re: [PATCH/RFC] grep: add a grep.patternType configuration setting
On Wed, 01 Aug 2012 14:55:52 -0700 Junio C. Hamano wrote: J Smith dark.pa...@gmail.com writes: grep.extendedRegexp:: -If set to true, enable '--extended-regexp' option by default. +If set to true, enable '--extended-regexp' option by default. This +option is ignored when the 'grep.patternType' option is set. We are not going to make grep.patternType a boolean, so when ... is set is fine, but if we were to allow grep.patternType to be set to false, the description gives ambiguity to some readers who do. Perhaps s/is set/is given/ is safer. I'm not a native speaker, but to me is given implies command line (the meaning is clear here, it just sounds a bit weird). If it's not just me, is used or has a value might be better. -- Štěpán -- 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
Re: [PATCH/RFC] grep: add a grep.patternType configuration setting
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 ;-) 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
