[PATCH] Add two grep config options

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 <redacted>
---
 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

Joe Ratterman [off-list ref] 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 <redacted>

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?

Junio C Hamano venit, vidit, dixit 26.03.2011 00:25:

Joe Ratterman [off-list ref] writes:

quoted

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 <redacted>

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

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

quoted

 - 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

Jeff King venit, vidit, dixit 28.03.2011 13:54:

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

quoted

quoted

 - 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.

quoted

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

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

quoted

quoted

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

On Mon, Mar 28, 2011 at 09:24, Michael J Gruber
[off-list ref] 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

On Mon, Mar 28, 2011 at 7:13 AM, Bert Wesarg [off-list ref] wrote:

On Mon, Mar 28, 2011 at 09:24, Michael J Gruber
[off-list ref] wrote:

quoted

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

quoted

- 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