[PATCH] Add two grep config options

[PATCH] Add two grep config options

[Joe Ratterman]

grep.extended-regexp: Enabling this boolean option has the same effect
as adding "-E" to all "git grep " instantiations.  This can be
disabled by specifying "--no-extended-regexp" on a particular call.

grep.line-numbers: Enabling this boolean option has the same effect as
adding "-n" to all "git grep " instantiations.

Signed-off-by: Joe Ratterman <jratt0@gmail.com>
---
 Documentation/config.txt   |    6 ++++++
 Documentation/git-grep.txt |    9 +++++++++
 builtin/grep.c             |   10 ++++++++++
 3 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index c5e1835..6b084a8 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1101,6 +1101,12 @@ All gitcvs variables except for 'gitcvs.usecrlfattr' and
 is one of "ext" and "pserver") to make them apply only for the given
 access method.
 
+grep.line-numbers::
+	If set to true, enable '-n' option by default.
+
+grep.extended-regexp::
+	If set to true, enable '--extended-regexp' option by default.
+
 gui.commitmsgwidth::
 	Defines how wide the commit message window is in the
 	linkgit:git-gui[1]. "75" is the default.
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index dab0a78..71668e0 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -31,6 +31,15 @@ Look for specified patterns in the tracked files in the work tree, blobs
 registered in the index file, or blobs in given tree objects.
 
 
+CONFIGURATION
+-------------
+
+grep.line-numbers::
+	If set to true, enable '-n' option by default.
+
+grep.extended-regexp::
+	If set to true, enable '--extended-regexp' option by default.
+
 OPTIONS
 -------
 --cached::
diff --git a/builtin/grep.c b/builtin/grep.c
index fdf7131..fed9f65 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -303,6 +303,16 @@ static int grep_config(const char *var, const char *value, void *cb)
 	default: return 0;
 	}
 
+	if (!strcmp(var, "grep.extended-regexp")) {
+		opt->regflags = git_config_bool(var, value);
+		return 0;
+	}
+
+	if (!strcmp(var, "grep.line-numbers")) {
+		opt->linenum = git_config_bool(var, value);
+		return 0;
+	}
+
 	if (!strcmp(var, "color.grep"))
 		opt->color = git_config_colorbool(var, value, -1);
 	else if (!strcmp(var, "color.grep.context"))
-- 
1.7.4.1.4.g0ea33

Re: [PATCH] Add two grep config options

[Junio C Hamano]

Joe Ratterman <jratt0@gmail.com> writes:

> grep.extended-regexp: Enabling this boolean option has the same effect
> as adding "-E" to all "git grep " instantiations.  This can be
> disabled by specifying "--no-extended-regexp" on a particular call.
>
> grep.line-numbers: Enabling this boolean option has the same effect as
> adding "-n" to all "git grep " instantiations.
>
> Signed-off-by: Joe Ratterman <jratt0@gmail.com>

Thanks.

Things to consider:

 - Apply this patch on top of "master",run "git shortlog v1.7.4..HEAD",
   store the output somewhere, and imagine reading that 2 months from now.
   Does a single line in the output about this patch sufficiently tell you
   what it was about?

 - Configuration variables are spelled without hyphens between words (you
   can see "gui.commitmsgwidth" in the context of the patch you sent and
   notice that it is not "gui.commit-msg-width").

 - This will break scripts people have written, knowing that they can rely
   on "grep" they wrote without giving "-E" from their command line will
   use BRE, and force them to update the script with --no-extended-regexp
   for no good reason.  Worse yet, there isn't even --no-line-numbers
   supported to defeat grep.linenumbers configuration to protect such
   scripts.

   I understand that some people would feel that the convenience would
   outweigh the risk of script breakage in this particular case, and I am
   sympathetic to the cause, but I still have to point it out.  Is there
   anything we can do to mitigate the risk somehow?

Re: [PATCH] Add two grep config options

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 26.03.2011 00:25:
> Joe Ratterman <jratt0@gmail.com> writes:
> 
>> grep.extended-regexp: Enabling this boolean option has the same effect
>> as adding "-E" to all "git grep " instantiations.  This can be
>> disabled by specifying "--no-extended-regexp" on a particular call.
>>
>> grep.line-numbers: Enabling this boolean option has the same effect as
>> adding "-n" to all "git grep " instantiations.
>>
>> Signed-off-by: Joe Ratterman <jratt0@gmail.com>
> 
> Thanks.
> 
> Things to consider:
> 
>  - Apply this patch on top of "master",run "git shortlog v1.7.4..HEAD",
>    store the output somewhere, and imagine reading that 2 months from now.
>    Does a single line in the output about this patch sufficiently tell you
>    what it was about?
> 
>  - Configuration variables are spelled without hyphens between words (you
>    can see "gui.commitmsgwidth" in the context of the patch you sent and
>    notice that it is not "gui.commit-msg-width").
> 
>  - This will break scripts people have written, knowing that they can rely
>    on "grep" they wrote without giving "-E" from their command line will
>    use BRE, and force them to update the script with --no-extended-regexp
>    for no good reason.  Worse yet, there isn't even --no-line-numbers
>    supported to defeat grep.linenumbers configuration to protect such
>    scripts.
> 
>    I understand that some people would feel that the convenience would
>    outweigh the risk of script breakage in this particular case, and I am
>    sympathetic to the cause, but I still have to point it out.  Is there
>    anything we can do to mitigate the risk somehow?

This comes up again and again, and I feel that rather than adding config
options one by one, we should either allow aliases for standard commands
and/or setting default options depending on the mode (ui use vs.
scripting use), so to say a companion to "git -c n=v" which allows

git config ui.grep "-E -n"

I.e. just like "git -c n=v <cmd>" sets up pseudo config before running
cmd, our wrapper could augment argv from "ui.<cmd>".

We could safeguard scripts from this by

- checking istty and
- checking env for GIT_PLUMBING

and setting the latter in git-sh-setup.sh. After a long migration phase,
we could skip the first (fragile) check.

Michael

Re: [PATCH] Add two grep config options

[Jeff King]

On Mon, Mar 28, 2011 at 09:24:26AM +0200, Michael J Gruber wrote:

> >  - This will break scripts people have written, knowing that they can rely
> >    on "grep" they wrote without giving "-E" from their command line will
> >    use BRE, and force them to update the script with --no-extended-regexp
> >    for no good reason.  Worse yet, there isn't even --no-line-numbers
> >    supported to defeat grep.linenumbers configuration to protect such
> >    scripts.
> > 
> >    I understand that some people would feel that the convenience would
> >    outweigh the risk of script breakage in this particular case, and I am
> >    sympathetic to the cause, but I still have to point it out.  Is there
> >    anything we can do to mitigate the risk somehow?
> 
> This comes up again and again, and I feel that rather than adding config
> options one by one, we should either allow aliases for standard commands
> and/or setting default options depending on the mode (ui use vs.
> scripting use), so to say a companion to "git -c n=v" which allows
> 
> git config ui.grep "-E -n"
> 
> I.e. just like "git -c n=v <cmd>" sets up pseudo config before running
> cmd, our wrapper could augment argv from "ui.<cmd>".
> 
> We could safeguard scripts from this by
> 
> - checking istty and
> - checking env for GIT_PLUMBING
> 
> and setting the latter in git-sh-setup.sh. After a long migration phase,
> we could skip the first (fragile) check.

I like the idea of a GIT_PLUMBING variable. It is something that has
come up as a solution to similar problems many times in the past, but it
never ends up getting implemented, because it never solves the problem
at hand. It is something that we would introduce now, and would solve
problems several versions down the road, after people adjusted their
scripts. So nobody ends up doing it.

And probably we would want something like --no-plumbing to switch back
to porcelain mode for a specific command.  Because often scripts do a
bunch of work, and then want to show the user their output in their
favorite format.  Something like:

  . git-sh-setup ;# or manually GIT_PLUMBING=1
  # get list of "interesting" files containing some pattern
  files=`git grep -le "$1"`
  # and then show them
  git --no-plumbing log $files

One shortcoming of such a scheme, though, is that it is an
all-or-nothing proposal; a script has no way to say "it's OK to take the
user's default for _this_ option, but not for others". For example, in
the script above, it would make sense for the grep call to respect the
user's choice of "-E". So it is tempting to use --no-plumbing there,
too. But we don't necessarily want to respect whatever cruft the user
put into ui.grep, because we care what the output looks like (something
like "-O" would not be helpful).

So what we really want is to let the script "allow" certain options from
the user's preferences. This could be done easily with individual config
options, like:

  git --allow=grep.extended grep ...

where the config code would respect a variable if GIT_PLUMBING is unset,
or if the key is in the allowed list. You could probably also adapt it
to something like:

  git --allow="-E --foo" grep ...

which would allow "-E" and "--foo" in ui.grep, but nothing else.

Now, obviously my script is a toy (it's not a real script I use). In
particular, "-O" would probably be suppressed by the lack of a tty,
anyway. But:

  1. I used a toy to have something readable. Look at something more
     complex, like "git pull". It calls "git merge". Should it be
     respecting "-n" and "--log" from the user's config? Probably.
     Should it respect "--no-ff" or "-s"? I'm not sure. And there are
     lots more examples just in the git.git scripts.

  2. It's not just about safeguarding versus the current set of grep
     options. It's about safeguarding the script against any _future_
     options that don't exist yet.

  3. This might be overengineering a little bit. Most invocations
     probably do fall into either the "run command to get its output"
     category (where you want pure plumbing) or the "run command to show
     things to the user" category (where you will take whatever options
     the user wants). But because the deployment of whatever scheme is
     chosen is going to be so annoying (because it will take many
     versions before we're clear to assume that people are setting
     GIT_PLUMBING), I would rather over-engineer than find out 2 years
     into the process that the flexibility we provide is not sufficient
     and have to start again.

> We could safeguard scripts from this by
> 
> - checking istty and
> - checking env for GIT_PLUMBING

I'm not sure isatty is a good check. In the example above, grep's output
was not going to a tty, but I did want to respect the user's choice of
"-E".

-Peff

Re: [PATCH] Add two grep config options

[Michael J Gruber]

Jeff King venit, vidit, dixit 28.03.2011 13:54:
> On Mon, Mar 28, 2011 at 09:24:26AM +0200, Michael J Gruber wrote:
> 
>>>  - This will break scripts people have written, knowing that they can rely
>>>    on "grep" they wrote without giving "-E" from their command line will
>>>    use BRE, and force them to update the script with --no-extended-regexp
>>>    for no good reason.  Worse yet, there isn't even --no-line-numbers
>>>    supported to defeat grep.linenumbers configuration to protect such
>>>    scripts.
>>>
>>>    I understand that some people would feel that the convenience would
>>>    outweigh the risk of script breakage in this particular case, and I am
>>>    sympathetic to the cause, but I still have to point it out.  Is there
>>>    anything we can do to mitigate the risk somehow?
>>
>> This comes up again and again, and I feel that rather than adding config
>> options one by one, we should either allow aliases for standard commands
>> and/or setting default options depending on the mode (ui use vs.
>> scripting use), so to say a companion to "git -c n=v" which allows
>>
>> git config ui.grep "-E -n"
>>
>> I.e. just like "git -c n=v <cmd>" sets up pseudo config before running
>> cmd, our wrapper could augment argv from "ui.<cmd>".
>>
>> We could safeguard scripts from this by
>>
>> - checking istty and
>> - checking env for GIT_PLUMBING
>>
>> and setting the latter in git-sh-setup.sh. After a long migration phase,
>> we could skip the first (fragile) check.
> 
> I like the idea of a GIT_PLUMBING variable. It is something that has
> come up as a solution to similar problems many times in the past, but it
> never ends up getting implemented, because it never solves the problem
> at hand. It is something that we would introduce now, and would solve
> problems several versions down the road, after people adjusted their
> scripts. So nobody ends up doing it.
> 
> And probably we would want something like --no-plumbing to switch back
> to porcelain mode for a specific command.  Because often scripts do a
> bunch of work, and then want to show the user their output in their
> favorite format.  Something like:
> 
>   . git-sh-setup ;# or manually GIT_PLUMBING=1
>   # get list of "interesting" files containing some pattern
>   files=`git grep -le "$1"`
>   # and then show them
>   git --no-plumbing log $files
> 
> One shortcoming of such a scheme, though, is that it is an
> all-or-nothing proposal; a script has no way to say "it's OK to take the
> user's default for _this_ option, but not for others". For example, in
> the script above, it would make sense for the grep call to respect the
> user's choice of "-E". So it is tempting to use --no-plumbing there,
> too. But we don't necessarily want to respect whatever cruft the user
> put into ui.grep, because we care what the output looks like (something
> like "-O" would not be helpful).
> 
> So what we really want is to let the script "allow" certain options from
> the user's preferences. This could be done easily with individual config
> options, like:
> 
>   git --allow=grep.extended grep ...
> 
> where the config code would respect a variable if GIT_PLUMBING is unset,
> or if the key is in the allowed list. You could probably also adapt it
> to something like:
> 
>   git --allow="-E --foo" grep ...
> 
> which would allow "-E" and "--foo" in ui.grep, but nothing else.
> 
> Now, obviously my script is a toy (it's not a real script I use). In
> particular, "-O" would probably be suppressed by the lack of a tty,
> anyway. But:
> 
>   1. I used a toy to have something readable. Look at something more
>      complex, like "git pull". It calls "git merge". Should it be
>      respecting "-n" and "--log" from the user's config? Probably.
>      Should it respect "--no-ff" or "-s"? I'm not sure. And there are
>      lots more examples just in the git.git scripts.
> 
>   2. It's not just about safeguarding versus the current set of grep
>      options. It's about safeguarding the script against any _future_
>      options that don't exist yet.
> 
>   3. This might be overengineering a little bit. Most invocations
>      probably do fall into either the "run command to get its output"
>      category (where you want pure plumbing) or the "run command to show
>      things to the user" category (where you will take whatever options
>      the user wants). But because the deployment of whatever scheme is
>      chosen is going to be so annoying (because it will take many
>      versions before we're clear to assume that people are setting
>      GIT_PLUMBING), I would rather over-engineer than find out 2 years
>      into the process that the flexibility we provide is not sufficient
>      and have to start again.
> 
>> We could safeguard scripts from this by
>>
>> - checking istty and
>> - checking env for GIT_PLUMBING
> 
> I'm not sure isatty is a good check. In the example above, grep's output
> was not going to a tty, but I did want to respect the user's choice of
> "-E".

I'm not saying it's good either, but it is something that a new git
(i.e. between the time we introduce ui.* and GIT_PLUMBING/--no-plum and
the time we rely on the latter) could do to make use of (and promote)
the new ui.* options.

Michael

Re: [PATCH] Add two grep config options

[Jeff King]

On Mon, Mar 28, 2011 at 02:00:58PM +0200, Michael J Gruber wrote:

> >> We could safeguard scripts from this by
> >>
> >> - checking istty and
> >> - checking env for GIT_PLUMBING
> > 
> > I'm not sure isatty is a good check. In the example above, grep's output
> > was not going to a tty, but I did want to respect the user's choice of
> > "-E".
> 
> I'm not saying it's good either, but it is something that a new git
> (i.e. between the time we introduce ui.* and GIT_PLUMBING/--no-plum and
> the time we rely on the latter) could do to make use of (and promote)
> the new ui.* options.

The example I gave was a false negative (we could have used the user's
preference but the isatty check said no). Which is OK for a transitional
period, because we err on the side of being conservative.  But I wonder
if there are false positives (i.e., cases where the isatty check says
it's OK, but we are breaking a script).  Maybe something where the
script prepares a BRE to hand to git-grep, but we want to show the user
the output in their usual way.

That seems pretty contrived, though. Maybe it is a non-issue.

-Peff

Re: [PATCH] Add two grep config options

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> One shortcoming of such a scheme, though, is that it is an
> all-or-nothing proposal...

I fully agree with this assessment, and I think that was the primary
reason that we rejected --plumbing / GIT_PLUMB (i.e. as too naive to be
useful).

> So what we really want is to let the script "allow" certain options from
> the user's preferences. This could be done easily with individual config
> options, like:
>
>   git --allow=grep.extended grep ...

I think this is probably the right thing to do _if_ we wanted to add such
a configuration variable and give a way to let script writers protect
themselves.

But would any user go all that trouble, just not to say "-nE" from the
command line (or use an alias that was designed not to crash with
scripts)?

Re: [PATCH] Add two grep config options

[Jeff King]

On Mon, Mar 28, 2011 at 10:12:23AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > One shortcoming of such a scheme, though, is that it is an
> > all-or-nothing proposal...
> 
> I fully agree with this assessment, and I think that was the primary
> reason that we rejected --plumbing / GIT_PLUMB (i.e. as too naive to be
> useful).
> 
> > So what we really want is to let the script "allow" certain options from
> > the user's preferences. This could be done easily with individual config
> > options, like:
> >
> >   git --allow=grep.extended grep ...
> 
> I think this is probably the right thing to do _if_ we wanted to add such
> a configuration variable and give a way to let script writers protect
> themselves.

Note that "git --allow=grep.extended" is not useful without
GIT_PLUMBING=1. Otherwise, users at the command line would have to
individually allow each config option, which makes them pointless.

Probably you already figured that out, but I wasn't sure from what you
wrote.

> But would any user go all that trouble, just not to say "-nE" from the
> command line (or use an alias that was designed not to crash with
> scripts)?

I dunno. I generally prefer to use extended regexps when I can, but I
don't remember ever having been annoyed that they are not the default
with git grep. Perhaps because 99% of my greps are for literal symbol
names (whereas in my editor, I am often doing substitutions, and I am
continually annoyed at having to use backslash to make my parentheses
magical).

-Peff

Re: [PATCH] Add two grep config options

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I dunno. I generally prefer to use extended regexps when I can, but I
> don't remember ever having been annoyed that they are not the default
> with git grep. Perhaps because 99% of my greps are for literal symbol
> names (whereas in my editor, I am often doing substitutions, and I am
> continually annoyed at having to use backslash to make my parentheses
> magical).

I do not think I've ever been annoyed that "GNU grep" needs -E to enable
extended regexps, either.

As much as I hate GREP_OPTIONS and GREP_COLORS environment variables that
force us to write a "sane_grep" wrapper to run "grep" while disabling
these, I have to admit that the approach to use these environment
variables is far easier to disable than having a set of configuration
variables that cover many commands we have, which is harder to disable
selectively.

Having discussed all this, with an addition of --[no-]line-number synonym
to the existing -n option, I don't think this particular incompatibility
is a huge issue.  At the worst, we might want to wait until a version bump
like 1.8.0, but I personally think a prominent notice in the release notes
in 1.7.5 would be sufficient.

Re: [PATCH] Add two grep config options

[Joe Ratterman]

On Mon, Mar 28, 2011 at 12:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
> But would any user go all that trouble, just not to say "-nE" from the
> command line (or use an alias that was designed not to crash with
> scripts)?
>

I use the $GREP_OPTIONS environment variable to add -n and -P (pcre)
to my grep commands by default.  Coming from a strong Perl background,
I don't know the *exact* difference between basic and extended, so I
do not usually realize I need -E until it gets a little too
complicated.  I generally forget line numbers until I want to start
editing the file, making me run the grep command a second time.  For
me, it is much easier to have them in the environment/config-file,
since I never want to avoid them.

Joe

Re: [PATCH] Add two grep config options

[Bert Wesarg]

On Mon, Mar 28, 2011 at 09:24, Michael J Gruber
<git@drmicha.warpmail.net> wrote:
> We could safeguard scripts from this by
>
> - checking istty and

That should consider running under the pager too.

I had proposed this scheme last year to TopGit as a way to
differentiate between ui and plumbing. But as Jeff pointed out, this
is probably not enough.

Bert

> - checking env for GIT_PLUMBING
>

Re: [PATCH] Add two grep config options

[Joe Ratterman]

On Mon, Mar 28, 2011 at 7:13 AM, Bert Wesarg <bert.wesarg@googlemail.com> wrote:
>
> On Mon, Mar 28, 2011 at 09:24, Michael J Gruber
> <git@drmicha.warpmail.net> wrote:
> > We could safeguard scripts from this by
> >
> > - checking istty and
>
> That should consider running under the pager too.
>
> I had proposed this scheme last year to TopGit as a way to
> differentiate between ui and plumbing. But as Jeff pointed out, this
> is probably not enough.
>
> Bert
>
> > - checking env for GIT_PLUMBING
> >


Thanks for looking over this and taking the time to comment.  I had
not considered the importance of the first line, nor had I noticed the
lack of hyphens in the other options--I copied the name of
grep.extended-regexp from the --extended-regexp grep option.  I can
change those things if the general idea is accepted.  I like the idea
of a single option config key have takes a list of command-line flags,
but I'm not 100% sure I know enough about git code internals to
implement it.  I'll look.

With regard to disabling the line numbering, GNU grep supports both -n
and --line-number.  Adding the latter option to git grep allows for
the long form --no-line-number.  Another option would be to use -N for
the negative, like -h and -H.  GNU grep doesn't support either of
those.


Thanks,
Joe Ratterman

Re: [PATCH] Add two grep config options

[Junio C Hamano]

Joe Ratterman <jratt0@gmail.com> writes:

> With regard to disabling the line numbering, GNU grep supports both -n
> and --line-number.  Adding the latter option to git grep allows for
> the long form --no-line-number.

Yes, that is something we would probably want regardless of your patch we
are discussing in this thread, if only for helping people who are used to
the GNU way.

[PATCH] grep: allow -E and -n to be turned on by default via configuration

[Junio C Hamano]

From: Joe Ratterman <jratt0@gmail.com>

Add two configration variables grep.extendedRegexp and grep.lineNumbers to
allow the user to skip typing -E and -n on the command line, respectively.

Scripts that are meant to be used by random users and/or in random
repositories now have use -G and/or --no-line-number options as
appropriately to override the settings in the repository or user's
~/.gitconfig settings. Just because the script didn't say "git grep -n" no
longer guarantees that the output from the command will not have line
numbers.

Signed-off-by: Joe Ratterman <jratt0@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * Having discussed the b/c issue to death, here is an updated version of
   your patch to be applied on top of your --[no-]line-number patch.

   I addressed both the title for shortlog and names of the variables, but
   two more important fixes are not to overwrite the whole regflags and
   not to assume that the value of REG_EXTENDED is 01.

   I did not add tests, but we do need one perhaps at the end of t7810 to
   move things forward.

 Documentation/config.txt   |    6 ++++++
 Documentation/git-grep.txt |   10 ++++++++++
 builtin/grep.c             |   10 ++++++++++
 3 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 8ea55d4..1ae5d80 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1098,6 +1098,12 @@ All gitcvs variables except for 'gitcvs.usecrlfattr' and
 is one of "ext" and "pserver") to make them apply only for the given
 access method.
 
+grep.lineNumbers::
+	If set to true, enable '-n' option by default.
+
+grep.extendedRegexp::
+	If set to true, enable '--extended-regexp' option by default.
+
 gui.commitmsgwidth::
 	Defines how wide the commit message window is in the
 	linkgit:git-gui[1]. "75" is the default.
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 132505e..d7c2427 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -31,6 +31,16 @@ Look for specified patterns in the tracked files in the work tree, blobs
 registered in the index file, or blobs in given tree objects.
 
 
+CONFIGURATION
+-------------
+
+grep.lineNumbers::
+	If set to true, enable '-n' option by default.
+
+grep.extendedRegexp::
+	If set to true, enable '--extended-regexp' option by default.
+
+
 OPTIONS
 -------
 --cached::
diff --git a/builtin/grep.c b/builtin/grep.c
index 85e9583..62d8b11 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -302,6 +302,16 @@ static int grep_config(const char *var, const char *value, void *cb)
 	default: return 0;
 	}
 
+	if (!strcmp(var, "grep.extendedregexp")) {
+		opt->regflags |= git_config_bool(var, value) ? REG_EXTENDED : 0;
+		return 0;
+	}
+
+	if (!strcmp(var, "grep.linenumbers")) {
+		opt->linenum = git_config_bool(var, value);
+		return 0;
+	}
+
 	if (!strcmp(var, "color.grep"))
 		opt->color = git_config_colorbool(var, value, -1);
 	else if (!strcmp(var, "color.grep.context"))
-- 
1.7.4.2.412.g61e8a

Re: [PATCH] grep: allow -E and -n to be turned on by default via configuration

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> +	if (!strcmp(var, "grep.extendedregexp")) {
> +		opt->regflags |= git_config_bool(var, value) ? REG_EXTENDED : 0;
> +		return 0;
> +	}

Heh, I am an idiot.  This should be more like

	if (git_config_bool(var, value))
        	opt->regflags |= REG_EXTENDED;
	else
        	opt->regflags &= ~REG_EXTENDED;
	return 0;

Re: [PATCH] grep: allow -E and -n to be turned on by default via configuration

[Joe Ratterman]

On Mon, Mar 28, 2011 at 5:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
> +grep.lineNumbers::
> +       If set to true, enable '-n' option by default.
> +
> +grep.extendedRegexp::
> +       If set to true, enable '--extended-regexp' option by default.
> +

I know my original patch was plural, but I since noticed that the GNU
grep --line-number option is singular.  I used the same thing in my
patch to add that option to git grep.  Should this one be singular?


> +grep.lineNumbers::
> +       If set to true, enable '-n' option by default.
> +
> +grep.extendedRegexp::
> +       If set to true, enable '--extended-regexp' option by default.
> +
> +
...
> +       if (!strcmp(var, "grep.extendedregexp")) {
> +               opt->regflags |= git_config_bool(var, value) ? REG_EXTENDED : 0;
> +               return 0;
> +       }
> +
> +       if (!strcmp(var, "grep.linenumbers")) {
> +               opt->linenum = git_config_bool(var, value);

We need to match the case between the docs and the code or use
strcasecmp().  Most config options I could find used all lower-case.



Joe

Re: [PATCH] grep: allow -E and -n to be turned on by default via configuration

[Junio C Hamano]

Joe Ratterman <jratt0@gmail.com> writes:

> On Mon, Mar 28, 2011 at 5:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> +grep.lineNumbers::
>> +       If set to true, enable '-n' option by default.
>> +
>> +grep.extendedRegexp::
>> +       If set to true, enable '--extended-regexp' option by default.
>> +
>
> I know my original patch was plural, but I since noticed that the GNU
> grep --line-number option is singular.  I used the same thing in my
> patch to add that option to git grep.  Should this one be singular?

Good eyes.

We should match what we are trying to mimic, and we need to be internally
consistent. So --[no-]line-number should be singular to mimic GNU (which
is how your othr patch is done---I don't have to amend what I already
queued).  And we should match the variable by naming it "grep.lineNumber"
to the command line option.

>> +       if (!strcmp(var, "grep.linenumbers")) {
>> +               opt->linenum = git_config_bool(var, value);
>
> We need to match the case between the docs and the code or use
> strcasecmp().

In git_config() callback functions, the variable name is supposed to be
already downcased by the caller, except for the second-level name in
three-level names, like "branch.Foo.merge" where the second-level name is
case sensitive.  Feeding var we got from the caller and lowercased
variable name we expect to strcmp() is the right way to write them when
you are dealing with configuration variable names.

Some places might unnecessarily and incorrectly use strcasecmp() but if
so, we should be fixing them instead.  We shouldn't mimic bad code.

In documentation we customary write them in camelCase only for
readability.

Re: [PATCH] grep: allow -E and -n to be turned on by default via configuration

[Joe Ratterman]

On Mon, Mar 28, 2011 at 5:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
>   I did not add tests, but we do need one perhaps at the end of t7810 to
>   move things forward.


What sort of tests would be appropriate?  I cannot find anything that
tests -E/--extended-regexp.  I copied the first test that used "git
grep -n" and changed it to these:
1)  git -c grep.linenumber=false grep -n  #  Expected to have line numbers
2)  git -c grep.linenumber=true grep  #  Expected to have line numbers
3)  git -c grep.linenumber=true grep --no-line-number  #  Expected to
have no line numbers


Joe

[PATCH v3] grep: allow -E and -n to be turned on by default via configuration

[Joe Ratterman]

Add two configration variables grep.extendedRegexp and grep.lineNumbers to
allow the user to skip typing -E and -n on the command line, respectively.

Scripts that are meant to be used by random users and/or in random
repositories now have use -G and/or --no-line-number options as
appropriately to override the settings in the repository or user's
~/.gitconfig settings. Just because the script didn't say "git grep -n" no
longer guarantees that the output from the command will not have line
numbers.

Signed-off-by: Joe Ratterman <jratt0@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

This adds 3 tests to t7810-grep.sh:
1)  git -c grep.linenumber=false grep -n  #  Expected to have line numbers
2)  git -c grep.linenumber=true  grep     #  Expected to have line numbers
3)  git -c grep.linenumber=true  grep --no-line-number  #  Expected to
have no line numbers


 Documentation/config.txt   |    6 ++++++
 Documentation/git-grep.txt |   10 ++++++++++
 builtin/grep.c             |   13 +++++++++++++
 t/t7810-grep.sh            |   24 +++++++++++++++++++++++-
 4 files changed, 52 insertions(+), 1 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 701fba9..c63458d 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1092,6 +1092,12 @@ All gitcvs variables except for 'gitcvs.usecrlfattr' and
 is one of "ext" and "pserver") to make them apply only for the given
 access method.
 
+grep.lineNumber::
+	If set to true, enable '-n' option by default.
+
+grep.extendedRegexp::
+	If set to true, enable '--extended-regexp' option by default.
+
 gui.commitmsgwidth::
 	Defines how wide the commit message window is in the
 	linkgit:git-gui[1]. "75" is the default.
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 791d4d4..16a0466 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -31,6 +31,16 @@ Look for specified patterns in the tracked files in the work tree, blobs
 registered in the index file, or blobs in given tree objects.
 
 
+CONFIGURATION
+-------------
+
+grep.lineNumber::
+	If set to true, enable '-n' option by default.
+
+grep.extendedRegexp::
+	If set to true, enable '--extended-regexp' option by default.
+
+
 OPTIONS
 -------
 --cached::
diff --git a/builtin/grep.c b/builtin/grep.c
index bb0f932..dcc5a76 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -302,6 +302,19 @@ static int grep_config(const char *var, const char *value, void *cb)
 	default: return 0;
 	}
 
+	if (!strcmp(var, "grep.extendedregexp")) {
+		if (git_config_bool(var, value))
+			opt->regflags |= REG_EXTENDED;
+		else
+			opt->regflags &= ~REG_EXTENDED;
+		return 0;
+	}
+
+	if (!strcmp(var, "grep.linenumber")) {
+		opt->linenum = git_config_bool(var, value);
+		return 0;
+	}
+
 	if (!strcmp(var, "color.grep"))
 		opt->color = git_config_colorbool(var, value, -1);
 	else if (!strcmp(var, "color.grep.context"))
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index c877758..be95bb2 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -59,7 +59,29 @@ do
 			echo ${HC}file:4:foo mmap bar_mmap
 			echo ${HC}file:5:foo_mmap bar mmap baz
 		} >expected &&
-		git grep -n -w -e mmap $H >actual &&
+		git -c grep.linenumber=false grep -n -w -e mmap $H >actual &&
+		test_cmp expected actual
+	'
+
+	test_expect_success "grep -w $L" '
+		{
+			echo ${HC}file:1:foo mmap bar
+			echo ${HC}file:3:foo_mmap bar mmap
+			echo ${HC}file:4:foo mmap bar_mmap
+			echo ${HC}file:5:foo_mmap bar mmap baz
+		} >expected &&
+		git -c grep.linenumber=true grep -w -e mmap $H >actual &&
+		test_cmp expected actual
+	'
+
+	test_expect_success "grep -w $L" '
+		{
+			echo ${HC}file:foo mmap bar
+			echo ${HC}file:foo_mmap bar mmap
+			echo ${HC}file:foo mmap bar_mmap
+			echo ${HC}file:foo_mmap bar mmap baz
+		} >expected &&
+		git -c grep.linenumber=true grep --no-line-number -w -e mmap $H >actual &&
 		test_cmp expected actual
 	'
 
-- 
1.7.4.1.4.g0ea33