{"thread":{"id":"26864","subject":"[PATCH] Add two grep config options","startedAt":"2011-03-25T21:21:11Z","lastAt":"2011-03-30T19:31:05Z","messageCount":19,"participants":["Joe Ratterman","Junio C Hamano","Michael J Gruber","Jeff King","Bert Wesarg"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"164328","messageId":"1301088071-918-1-git-send-email-jratt0@gmail.com","threadId":"26864","inReplyTo":null,"subject":"[PATCH] Add two grep config options","fromName":"Joe Ratterman","fromEmail":"jratt0@gmail.com","sentAt":"2011-03-25T21:21:11Z","receivedAt":"2011-03-25T21:21:11Z","isPatch":true,"sender":{"key":"jratt0@gmail.com","avatar":"https://avatars.githubusercontent.com/u/66082737?v=4"},"body":"grep.extended-regexp: Enabling this boolean option has the same effect\nas adding \"-E\" to all \"git grep \" instantiations. This can be\ndisabled by specifying \"--no-extended-regexp\" on a particular call.\n\ngrep.line-numbers: Enabling this boolean option has the same effect as\nadding \"-n\" to all \"git grep \" instantiations.\n\nSigned-off-by: Joe Ratterman \n---\n Documentation/config.txt | 6 ++++++\n Documentation/git-grep.txt | 9 +++++++++\n builtin/grep.c | 10 ++++++++++\n 3 files changed, 25 insertions(+), 0 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex c5e1835..6b084a8 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1101,6 +1101,12 @@ All gitcvs variables except for 'gitcvs.usecrlfattr' and\n is one of \"ext\" and \"pserver\") to make them apply only for the given\n access method.\n \n+grep.line-numbers::\n+\tIf set to true, enable '-n' option by default.\n+\n+grep.extended-regexp::\n+\tIf set to true, enable '--extended-regexp' option by default.\n+\n gui.commitmsgwidth::\n \tDefines how wide the commit message window is in the\n \tlinkgit:git-gui[1]. \"75\" is the default.\ndiff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\nindex dab0a78..71668e0 100644\n--- a/Documentation/git-grep.txt\n+++ b/Documentation/git-grep.txt\n@@ -31,6 +31,15 @@ Look for specified patterns in the tracked files in the work tree, blobs\n registered in the index file, or blobs in given tree objects.\n \n \n+CONFIGURATION\n+-------------\n+\n+grep.line-numbers::\n+\tIf set to true, enable '-n' option by default.\n+\n+grep.extended-regexp::\n+\tIf set to true, enable '--extended-regexp' option by default.\n+\n OPTIONS\n -------\n --cached::\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex fdf7131..fed9f65 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -303,6 +303,16 @@ static int grep_config(const char *var, const char *value, void *cb)\n \tdefault: return 0;\n \t}\n \n+\tif (!strcmp(var, \"grep.extended-regexp\")) {\n+\t\topt->regflags = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n+\tif (!strcmp(var, \"grep.line-numbers\")) {\n+\t\topt->linenum = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n \tif (!strcmp(var, \"color.grep\"))\n \t\topt->color = git_config_colorbool(var, value, -1);\n \telse if (!strcmp(var, \"color.grep.context\"))\n-- \n1.7.4.1.4.g0ea33\n"},{"id":"164342","messageId":"7vlj024wal.fsf@alter.siamese.dyndns.org","threadId":"26864","inReplyTo":"1301088071-918-1-git-send-email-jratt0@gmail.com","subject":"Re: [PATCH] Add two grep config options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-25T23:25:54Z","receivedAt":"2011-03-25T23:25:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joe Ratterman writes:\n\n> grep.extended-regexp: Enabling this boolean option has the same effect\n> as adding \"-E\" to all \"git grep \" instantiations. This can be\n> disabled by specifying \"--no-extended-regexp\" on a particular call.\n>\n> grep.line-numbers: Enabling this boolean option has the same effect as\n> adding \"-n\" to all \"git grep \" instantiations.\n>\n> Signed-off-by: Joe Ratterman \n\nThanks.\n\nThings to consider:\n\n - Apply this patch on top of \"master\",run \"git shortlog v1.7.4..HEAD\",\n store the output somewhere, and imagine reading that 2 months from now.\n Does a single line in the output about this patch sufficiently tell you\n what it was about?\n\n - Configuration variables are spelled without hyphens between words (you\n can see \"gui.commitmsgwidth\" in the context of the patch you sent and\n notice that it is not \"gui.commit-msg-width\").\n\n - This will break scripts people have written, knowing that they can rely\n on \"grep\" they wrote without giving \"-E\" from their command line will\n use BRE, and force them to update the script with --no-extended-regexp\n for no good reason. Worse yet, there isn't even --no-line-numbers\n supported to defeat grep.linenumbers configuration to protect such\n scripts.\n\n I understand that some people would feel that the convenience would\n outweigh the risk of script breakage in this particular case, and I am\n sympathetic to the cause, but I still have to point it out. Is there\n anything we can do to mitigate the risk somehow?\n"},{"id":"164409","messageId":"4D9037AA.9090601@drmicha.warpmail.net","threadId":"26864","inReplyTo":"7vlj024wal.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Add two grep config options","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-03-28T07:24:26Z","receivedAt":"2011-03-28T07:24:26Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 26.03.2011 00:25:\n> Joe Ratterman writes:\n> \n>> grep.extended-regexp: Enabling this boolean option has the same effect\n>> as adding \"-E\" to all \"git grep \" instantiations. This can be\n>> disabled by specifying \"--no-extended-regexp\" on a particular call.\n>>\n>> grep.line-numbers: Enabling this boolean option has the same effect as\n>> adding \"-n\" to all \"git grep \" instantiations.\n>>\n>> Signed-off-by: Joe Ratterman \n> \n> Thanks.\n> \n> Things to consider:\n> \n> - Apply this patch on top of \"master\",run \"git shortlog v1.7.4..HEAD\",\n> store the output somewhere, and imagine reading that 2 months from now.\n> Does a single line in the output about this patch sufficiently tell you\n> what it was about?\n> \n> - Configuration variables are spelled without hyphens between words (you\n> can see \"gui.commitmsgwidth\" in the context of the patch you sent and\n> notice that it is not \"gui.commit-msg-width\").\n> \n> - This will break scripts people have written, knowing that they can rely\n> on \"grep\" they wrote without giving \"-E\" from their command line will\n> use BRE, and force them to update the script with --no-extended-regexp\n> for no good reason. Worse yet, there isn't even --no-line-numbers\n> supported to defeat grep.linenumbers configuration to protect such\n> scripts.\n> \n> I understand that some people would feel that the convenience would\n> outweigh the risk of script breakage in this particular case, and I am\n> sympathetic to the cause, but I still have to point it out. Is there\n> anything we can do to mitigate the risk somehow?\n\nThis comes up again and again, and I feel that rather than adding config\noptions one by one, we should either allow aliases for standard commands\nand/or setting default options depending on the mode (ui use vs.\nscripting use), so to say a companion to \"git -c n=v\" which allows\n\ngit config ui.grep \"-E -n\"\n\nI.e. just like \"git -c n=v \" sets up pseudo config before running\ncmd, our wrapper could augment argv from \"ui.\".\n\nWe could safeguard scripts from this by\n\n- checking istty and\n- checking env for GIT_PLUMBING\n\nand setting the latter in git-sh-setup.sh. After a long migration phase,\nwe could skip the first (fragile) check.\n\nMichael\n"},{"id":"164420","messageId":"20110328115421.GA9232@sigill.intra.peff.net","threadId":"26864","inReplyTo":"4D9037AA.9090601@drmicha.warpmail.net","subject":"Re: [PATCH] Add two grep config options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-28T11:54:21Z","receivedAt":"2011-03-28T11:54:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 28, 2011 at 09:24:26AM +0200, Michael J Gruber wrote:\n\n> > - This will break scripts people have written, knowing that they can rely\n> > on \"grep\" they wrote without giving \"-E\" from their command line will\n> > use BRE, and force them to update the script with --no-extended-regexp\n> > for no good reason. Worse yet, there isn't even --no-line-numbers\n> > supported to defeat grep.linenumbers configuration to protect such\n> > scripts.\n> > \n> > I understand that some people would feel that the convenience would\n> > outweigh the risk of script breakage in this particular case, and I am\n> > sympathetic to the cause, but I still have to point it out. Is there\n> > anything we can do to mitigate the risk somehow?\n> \n> This comes up again and again, and I feel that rather than adding config\n> options one by one, we should either allow aliases for standard commands\n> and/or setting default options depending on the mode (ui use vs.\n> scripting use), so to say a companion to \"git -c n=v\" which allows\n> \n> git config ui.grep \"-E -n\"\n> \n> I.e. just like \"git -c n=v \" sets up pseudo config before running\n> cmd, our wrapper could augment argv from \"ui.\".\n> \n> We could safeguard scripts from this by\n> \n> - checking istty and\n> - checking env for GIT_PLUMBING\n> \n> and setting the latter in git-sh-setup.sh. After a long migration phase,\n> we could skip the first (fragile) check.\n\nI like the idea of a GIT_PLUMBING variable. It is something that has\ncome up as a solution to similar problems many times in the past, but it\nnever ends up getting implemented, because it never solves the problem\nat hand. It is something that we would introduce now, and would solve\nproblems several versions down the road, after people adjusted their\nscripts. So nobody ends up doing it.\n\nAnd probably we would want something like --no-plumbing to switch back\nto porcelain mode for a specific command. Because often scripts do a\nbunch of work, and then want to show the user their output in their\nfavorite format. Something like:\n\n . git-sh-setup ;# or manually GIT_PLUMBING=1\n # get list of \"interesting\" files containing some pattern\n files=`git grep -le \"$1\"`\n # and then show them\n git --no-plumbing log $files\n\nOne shortcoming of such a scheme, though, is that it is an\nall-or-nothing proposal; a script has no way to say \"it's OK to take the\nuser's default for _this_ option, but not for others\". For example, in\nthe script above, it would make sense for the grep call to respect the\nuser's choice of \"-E\". So it is tempting to use --no-plumbing there,\ntoo. But we don't necessarily want to respect whatever cruft the user\nput into ui.grep, because we care what the output looks like (something\nlike \"-O\" would not be helpful).\n\nSo what we really want is to let the script \"allow\" certain options from\nthe user's preferences. This could be done easily with individual config\noptions, like:\n\n git --allow=grep.extended grep ...\n\nwhere the config code would respect a variable if GIT_PLUMBING is unset,\nor if the key is in the allowed list. You could probably also adapt it\nto something like:\n\n git --allow=\"-E --foo\" grep ...\n\nwhich would allow \"-E\" and \"--foo\" in ui.grep, but nothing else.\n\nNow, obviously my script is a toy (it's not a real script I use). In\nparticular, \"-O\" would probably be suppressed by the lack of a tty,\nanyway. But:\n\n 1. I used a toy to have something readable. Look at something more\n complex, like \"git pull\". It calls \"git merge\". Should it be\n respecting \"-n\" and \"--log\" from the user's config? Probably.\n Should it respect \"--no-ff\" or \"-s\"? I'm not sure. And there are\n lots more examples just in the git.git scripts.\n\n 2. It's not just about safeguarding versus the current set of grep\n options. It's about safeguarding the script against any _future_\n options that don't exist yet.\n\n 3. This might be overengineering a little bit. Most invocations\n probably do fall into either the \"run command to get its output\"\n category (where you want pure plumbing) or the \"run command to show\n things to the user\" category (where you will take whatever options\n the user wants). But because the deployment of whatever scheme is\n chosen is going to be so annoying (because it will take many\n versions before we're clear to assume that people are setting\n GIT_PLUMBING), I would rather over-engineer than find out 2 years\n into the process that the flexibility we provide is not sufficient\n and have to start again.\n\n> We could safeguard scripts from this by\n> \n> - checking istty and\n> - checking env for GIT_PLUMBING\n\nI'm not sure isatty is a good check. In the example above, grep's output\nwas not going to a tty, but I did want to respect the user's choice of\n\"-E\".\n\n-Peff\n"},{"id":"164422","messageId":"4D90787A.8010403@drmicha.warpmail.net","threadId":"26864","inReplyTo":"20110328115421.GA9232@sigill.intra.peff.net","subject":"Re: [PATCH] Add two grep config options","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-03-28T12:00:58Z","receivedAt":"2011-03-28T12:00:58Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 28.03.2011 13:54:\n> On Mon, Mar 28, 2011 at 09:24:26AM +0200, Michael J Gruber wrote:\n> \n>>> - This will break scripts people have written, knowing that they can rely\n>>> on \"grep\" they wrote without giving \"-E\" from their command line will\n>>> use BRE, and force them to update the script with --no-extended-regexp\n>>> for no good reason. Worse yet, there isn't even --no-line-numbers\n>>> supported to defeat grep.linenumbers configuration to protect such\n>>> scripts.\n>>>\n>>> I understand that some people would feel that the convenience would\n>>> outweigh the risk of script breakage in this particular case, and I am\n>>> sympathetic to the cause, but I still have to point it out. Is there\n>>> anything we can do to mitigate the risk somehow?\n>>\n>> This comes up again and again, and I feel that rather than adding config\n>> options one by one, we should either allow aliases for standard commands\n>> and/or setting default options depending on the mode (ui use vs.\n>> scripting use), so to say a companion to \"git -c n=v\" which allows\n>>\n>> git config ui.grep \"-E -n\"\n>>\n>> I.e. just like \"git -c n=v \" sets up pseudo config before running\n>> cmd, our wrapper could augment argv from \"ui.\".\n>>\n>> We could safeguard scripts from this by\n>>\n>> - checking istty and\n>> - checking env for GIT_PLUMBING\n>>\n>> and setting the latter in git-sh-setup.sh. After a long migration phase,\n>> we could skip the first (fragile) check.\n> \n> I like the idea of a GIT_PLUMBING variable. It is something that has\n> come up as a solution to similar problems many times in the past, but it\n> never ends up getting implemented, because it never solves the problem\n> at hand. It is something that we would introduce now, and would solve\n> problems several versions down the road, after people adjusted their\n> scripts. So nobody ends up doing it.\n> \n> And probably we would want something like --no-plumbing to switch back\n> to porcelain mode for a specific command. Because often scripts do a\n> bunch of work, and then want to show the user their output in their\n> favorite format. Something like:\n> \n> . git-sh-setup ;# or manually GIT_PLUMBING=1\n> # get list of \"interesting\" files containing some pattern\n> files=`git grep -le \"$1\"`\n> # and then show them\n> git --no-plumbing log $files\n> \n> One shortcoming of such a scheme, though, is that it is an\n> all-or-nothing proposal; a script has no way to say \"it's OK to take the\n> user's default for _this_ option, but not for others\". For example, in\n> the script above, it would make sense for the grep call to respect the\n> user's choice of \"-E\". So it is tempting to use --no-plumbing there,\n> too. But we don't necessarily want to respect whatever cruft the user\n> put into ui.grep, because we care what the output looks like (something\n> like \"-O\" would not be helpful).\n> \n> So what we really want is to let the script \"allow\" certain options from\n> the user's preferences. This could be done easily with individual config\n> options, like:\n> \n> git --allow=grep.extended grep ...\n> \n> where the config code would respect a variable if GIT_PLUMBING is unset,\n> or if the key is in the allowed list. You could probably also adapt it\n> to something like:\n> \n> git --allow=\"-E --foo\" grep ...\n> \n> which would allow \"-E\" and \"--foo\" in ui.grep, but nothing else.\n> \n> Now, obviously my script is a toy (it's not a real script I use). In\n> particular, \"-O\" would probably be suppressed by the lack of a tty,\n> anyway. But:\n> \n> 1. I used a toy to have something readable. Look at something more\n> complex, like \"git pull\". It calls \"git merge\". Should it be\n> respecting \"-n\" and \"--log\" from the user's config? Probably.\n> Should it respect \"--no-ff\" or \"-s\"? I'm not sure. And there are\n> lots more examples just in the git.git scripts.\n> \n> 2. It's not just about safeguarding versus the current set of grep\n> options. It's about safeguarding the script against any _future_\n> options that don't exist yet.\n> \n> 3. This might be overengineering a little bit. Most invocations\n> probably do fall into either the \"run command to get its output\"\n> category (where you want pure plumbing) or the \"run command to show\n> things to the user\" category (where you will take whatever options\n> the user wants). But because the deployment of whatever scheme is\n> chosen is going to be so annoying (because it will take many\n> versions before we're clear to assume that people are setting\n> GIT_PLUMBING), I would rather over-engineer than find out 2 years\n> into the process that the flexibility we provide is not sufficient\n> and have to start again.\n> \n>> We could safeguard scripts from this by\n>>\n>> - checking istty and\n>> - checking env for GIT_PLUMBING\n> \n> I'm not sure isatty is a good check. In the example above, grep's output\n> was not going to a tty, but I did want to respect the user's choice of\n> \"-E\".\n\nI'm not saying it's good either, but it is something that a new git\n(i.e. between the time we introduce ui.* and GIT_PLUMBING/--no-plum and\nthe time we rely on the latter) could do to make use of (and promote)\nthe new ui.* options.\n\nMichael\n"},{"id":"164423","messageId":"20110328120938.GA14198@sigill.intra.peff.net","threadId":"26864","inReplyTo":"4D90787A.8010403@drmicha.warpmail.net","subject":"Re: [PATCH] Add two grep config options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-28T12:09:38Z","receivedAt":"2011-03-28T12:09:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 28, 2011 at 02:00:58PM +0200, Michael J Gruber wrote:\n\n> >> We could safeguard scripts from this by\n> >>\n> >> - checking istty and\n> >> - checking env for GIT_PLUMBING\n> > \n> > I'm not sure isatty is a good check. In the example above, grep's output\n> > was not going to a tty, but I did want to respect the user's choice of\n> > \"-E\".\n> \n> I'm not saying it's good either, but it is something that a new git\n> (i.e. between the time we introduce ui.* and GIT_PLUMBING/--no-plum and\n> the time we rely on the latter) could do to make use of (and promote)\n> the new ui.* options.\n\nThe example I gave was a false negative (we could have used the user's\npreference but the isatty check said no). Which is OK for a transitional\nperiod, because we err on the side of being conservative. But I wonder\nif there are false positives (i.e., cases where the isatty check says\nit's OK, but we are breaking a script). Maybe something where the\nscript prepares a BRE to hand to git-grep, but we want to show the user\nthe output in their usual way.\n\nThat seems pretty contrived, though. Maybe it is a non-issue.\n\n-Peff\n"},{"id":"164424","messageId":"AANLkTin_qeFSqyXHddmEpu=5e4yO8cxyOkbhcXUg3efn@mail.gmail.com","threadId":"26864","inReplyTo":"4D9037AA.9090601@drmicha.warpmail.net","subject":"Re: [PATCH] Add two grep config options","fromName":"Bert Wesarg","fromEmail":"bert.wesarg@googlemail.com","sentAt":"2011-03-28T12:13:49Z","receivedAt":"2011-03-28T12:13:49Z","isPatch":true,"sender":{"key":"bert.wesarg@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/111934?v=4"},"body":"On Mon, Mar 28, 2011 at 09:24, Michael J Gruber\n wrote:\n> We could safeguard scripts from this by\n>\n> - checking istty and\n\nThat should consider running under the pager too.\n\nI had proposed this scheme last year to TopGit as a way to\ndifferentiate between ui and plumbing. But as Jeff pointed out, this\nis probably not enough.\n\nBert\n\n> - checking env for GIT_PLUMBING\n>\n"},{"id":"164439","messageId":"AANLkTin_qRH54C0wKML67gftS05X98sKE+AxHJ4xo77C@mail.gmail.com","threadId":"26864","inReplyTo":"AANLkTin_qeFSqyXHddmEpu=5e4yO8cxyOkbhcXUg3efn@mail.gmail.com","subject":"Re: [PATCH] Add two grep config options","fromName":"Joe Ratterman","fromEmail":"jratt0@gmail.com","sentAt":"2011-03-28T14:48:19Z","receivedAt":"2011-03-28T14:48:19Z","isPatch":true,"sender":{"key":"jratt0@gmail.com","avatar":"https://avatars.githubusercontent.com/u/66082737?v=4"},"body":"On Mon, Mar 28, 2011 at 7:13 AM, Bert Wesarg wrote:\n>\n> On Mon, Mar 28, 2011 at 09:24, Michael J Gruber\n> wrote:\n> > We could safeguard scripts from this by\n> >\n> > - checking istty and\n>\n> That should consider running under the pager too.\n>\n> I had proposed this scheme last year to TopGit as a way to\n> differentiate between ui and plumbing. But as Jeff pointed out, this\n> is probably not enough.\n>\n> Bert\n>\n> > - checking env for GIT_PLUMBING\n> >\n\n\nThanks for looking over this and taking the time to comment. I had\nnot considered the importance of the first line, nor had I noticed the\nlack of hyphens in the other options--I copied the name of\ngrep.extended-regexp from the --extended-regexp grep option. I can\nchange those things if the general idea is accepted. I like the idea\nof a single option config key have takes a list of command-line flags,\nbut I'm not 100% sure I know enough about git code internals to\nimplement it. I'll look.\n\nWith regard to disabling the line numbering, GNU grep supports both -n\nand --line-number. Adding the latter option to git grep allows for\nthe long form --no-line-number. Another option would be to use -N for\nthe negative, like -h and -H. GNU grep doesn't support either of\nthose.\n\n\nThanks,\nJoe Ratterman\n"},{"id":"164465","messageId":"7vvcz3yxs8.fsf@alter.siamese.dyndns.org","threadId":"26864","inReplyTo":"20110328115421.GA9232@sigill.intra.peff.net","subject":"Re: [PATCH] Add two grep config options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-28T17:12:23Z","receivedAt":"2011-03-28T17:12:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> One shortcoming of such a scheme, though, is that it is an\n> all-or-nothing proposal...\n\nI fully agree with this assessment, and I think that was the primary\nreason that we rejected --plumbing / GIT_PLUMB (i.e. as too naive to be\nuseful).\n\n> So what we really want is to let the script \"allow\" certain options from\n> the user's preferences. This could be done easily with individual config\n> options, like:\n>\n> git --allow=grep.extended grep ...\n\nI think this is probably the right thing to do _if_ we wanted to add such\na configuration variable and give a way to let script writers protect\nthemselves.\n\nBut would any user go all that trouble, just not to say \"-nE\" from the\ncommand line (or use an alias that was designed not to crash with\nscripts)?\n"},{"id":"164466","messageId":"7vr59ryxo3.fsf@alter.siamese.dyndns.org","threadId":"26864","inReplyTo":"AANLkTin_qRH54C0wKML67gftS05X98sKE+AxHJ4xo77C@mail.gmail.com","subject":"Re: [PATCH] Add two grep config options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-28T17:14:52Z","receivedAt":"2011-03-28T17:14:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joe Ratterman writes:\n\n> With regard to disabling the line numbering, GNU grep supports both -n\n> and --line-number. Adding the latter option to git grep allows for\n> the long form --no-line-number.\n\nYes, that is something we would probably want regardless of your patch we\nare discussing in this thread, if only for helping people who are used to\nthe GNU way.\n"},{"id":"164468","messageId":"20110328172122.GA20817@sigill.intra.peff.net","threadId":"26864","inReplyTo":"7vvcz3yxs8.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Add two grep config options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-28T17:21:22Z","receivedAt":"2011-03-28T17:21:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 28, 2011 at 10:12:23AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > One shortcoming of such a scheme, though, is that it is an\n> > all-or-nothing proposal...\n> \n> I fully agree with this assessment, and I think that was the primary\n> reason that we rejected --plumbing / GIT_PLUMB (i.e. as too naive to be\n> useful).\n> \n> > So what we really want is to let the script \"allow\" certain options from\n> > the user's preferences. This could be done easily with individual config\n> > options, like:\n> >\n> > git --allow=grep.extended grep ...\n> \n> I think this is probably the right thing to do _if_ we wanted to add such\n> a configuration variable and give a way to let script writers protect\n> themselves.\n\nNote that \"git --allow=grep.extended\" is not useful without\nGIT_PLUMBING=1. Otherwise, users at the command line would have to\nindividually allow each config option, which makes them pointless.\n\nProbably you already figured that out, but I wasn't sure from what you\nwrote.\n\n> But would any user go all that trouble, just not to say \"-nE\" from the\n> command line (or use an alias that was designed not to crash with\n> scripts)?\n\nI dunno. I generally prefer to use extended regexps when I can, but I\ndon't remember ever having been annoyed that they are not the default\nwith git grep. Perhaps because 99% of my greps are for literal symbol\nnames (whereas in my editor, I am often doing substitutions, and I am\ncontinually annoyed at having to use backslash to make my parentheses\nmagical).\n\n-Peff\n"},{"id":"164475","messageId":"AANLkTinNM_tf=rJYXLO8cgaxwpAz2pi8G4nJiwZ9RLR6@mail.gmail.com","threadId":"26864","inReplyTo":"7vvcz3yxs8.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Add two grep config options","fromName":"Joe Ratterman","fromEmail":"jratt0@gmail.com","sentAt":"2011-03-28T18:08:11Z","receivedAt":"2011-03-28T18:08:11Z","isPatch":true,"sender":{"key":"jratt0@gmail.com","avatar":"https://avatars.githubusercontent.com/u/66082737?v=4"},"body":"On Mon, Mar 28, 2011 at 12:12 PM, Junio C Hamano wrote:\n> But would any user go all that trouble, just not to say \"-nE\" from the\n> command line (or use an alias that was designed not to crash with\n> scripts)?\n>\n\nI use the $GREP_OPTIONS environment variable to add -n and -P (pcre)\nto my grep commands by default. Coming from a strong Perl background,\nI don't know the *exact* difference between basic and extended, so I\ndo not usually realize I need -E until it gets a little too\ncomplicated. I generally forget line numbers until I want to start\nediting the file, making me run the grep command a second time. For\nme, it is much easier to have them in the environment/config-file,\nsince I never want to avoid them.\n\nJoe\n"},{"id":"164479","messageId":"7vy63zxf6v.fsf@alter.siamese.dyndns.org","threadId":"26864","inReplyTo":"20110328172122.GA20817@sigill.intra.peff.net","subject":"Re: [PATCH] Add two grep config options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-28T18:39:20Z","receivedAt":"2011-03-28T18:39:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I dunno. I generally prefer to use extended regexps when I can, but I\n> don't remember ever having been annoyed that they are not the default\n> with git grep. Perhaps because 99% of my greps are for literal symbol\n> names (whereas in my editor, I am often doing substitutions, and I am\n> continually annoyed at having to use backslash to make my parentheses\n> magical).\n\nI do not think I've ever been annoyed that \"GNU grep\" needs -E to enable\nextended regexps, either.\n\nAs much as I hate GREP_OPTIONS and GREP_COLORS environment variables that\nforce us to write a \"sane_grep\" wrapper to run \"grep\" while disabling\nthese, I have to admit that the approach to use these environment\nvariables is far easier to disable than having a set of configuration\nvariables that cover many commands we have, which is harder to disable\nselectively.\n\nHaving discussed all this, with an addition of --[no-]line-number synonym\nto the existing -n option, I don't think this particular incompatibility\nis a huge issue. At the worst, we might want to wait until a version bump\nlike 1.8.0, but I personally think a prominent notice in the release notes\nin 1.7.5 would be sufficient.\n"},{"id":"164499","messageId":"7vbp0ux5c9.fsf_-_@alter.siamese.dyndns.org","threadId":"26864","inReplyTo":"7vlj024wal.fsf@alter.siamese.dyndns.org","subject":"[PATCH] grep: allow -E and -n to be turned on by default via configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-28T22:12:06Z","receivedAt":"2011-03-28T22:12:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Joe Ratterman \n\nAdd two configration variables grep.extendedRegexp and grep.lineNumbers to\nallow the user to skip typing -E and -n on the command line, respectively.\n\nScripts that are meant to be used by random users and/or in random\nrepositories now have use -G and/or --no-line-number options as\nappropriately to override the settings in the repository or user's\n~/.gitconfig settings. Just because the script didn't say \"git grep -n\" no\nlonger guarantees that the output from the command will not have line\nnumbers.\n\nSigned-off-by: Joe Ratterman \nSigned-off-by: Junio C Hamano \n---\n\n * Having discussed the b/c issue to death, here is an updated version of\n your patch to be applied on top of your --[no-]line-number patch.\n\n I addressed both the title for shortlog and names of the variables, but\n two more important fixes are not to overwrite the whole regflags and\n not to assume that the value of REG_EXTENDED is 01.\n\n I did not add tests, but we do need one perhaps at the end of t7810 to\n move things forward.\n\n Documentation/config.txt | 6 ++++++\n Documentation/git-grep.txt | 10 ++++++++++\n builtin/grep.c | 10 ++++++++++\n 3 files changed, 26 insertions(+), 0 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 8ea55d4..1ae5d80 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1098,6 +1098,12 @@ All gitcvs variables except for 'gitcvs.usecrlfattr' and\n is one of \"ext\" and \"pserver\") to make them apply only for the given\n access method.\n \n+grep.lineNumbers::\n+\tIf set to true, enable '-n' option by default.\n+\n+grep.extendedRegexp::\n+\tIf set to true, enable '--extended-regexp' option by default.\n+\n gui.commitmsgwidth::\n \tDefines how wide the commit message window is in the\n \tlinkgit:git-gui[1]. \"75\" is the default.\ndiff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\nindex 132505e..d7c2427 100644\n--- a/Documentation/git-grep.txt\n+++ b/Documentation/git-grep.txt\n@@ -31,6 +31,16 @@ Look for specified patterns in the tracked files in the work tree, blobs\n registered in the index file, or blobs in given tree objects.\n \n \n+CONFIGURATION\n+-------------\n+\n+grep.lineNumbers::\n+\tIf set to true, enable '-n' option by default.\n+\n+grep.extendedRegexp::\n+\tIf set to true, enable '--extended-regexp' option by default.\n+\n+\n OPTIONS\n -------\n --cached::\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 85e9583..62d8b11 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -302,6 +302,16 @@ static int grep_config(const char *var, const char *value, void *cb)\n \tdefault: return 0;\n \t}\n \n+\tif (!strcmp(var, \"grep.extendedregexp\")) {\n+\t\topt->regflags |= git_config_bool(var, value) ? REG_EXTENDED : 0;\n+\t\treturn 0;\n+\t}\n+\n+\tif (!strcmp(var, \"grep.linenumbers\")) {\n+\t\topt->linenum = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n \tif (!strcmp(var, \"color.grep\"))\n \t\topt->color = git_config_colorbool(var, value, -1);\n \telse if (!strcmp(var, \"color.grep.context\"))\n-- \n1.7.4.2.412.g61e8a\n"},{"id":"164500","messageId":"7v7hbix587.fsf@alter.siamese.dyndns.org","threadId":"26864","inReplyTo":"7vbp0ux5c9.fsf_-_@alter.siamese.dyndns.org","subject":"Re: [PATCH] grep: allow -E and -n to be turned on by default via configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-28T22:14:32Z","receivedAt":"2011-03-28T22:14:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> +\tif (!strcmp(var, \"grep.extendedregexp\")) {\n> +\t\topt->regflags |= git_config_bool(var, value) ? REG_EXTENDED : 0;\n> +\t\treturn 0;\n> +\t}\n\nHeh, I am an idiot. This should be more like\n\n\tif (git_config_bool(var, value))\n \topt->regflags |= REG_EXTENDED;\n\telse\n \topt->regflags &= ~REG_EXTENDED;\n\treturn 0;\n"},{"id":"164501","messageId":"AANLkTimpp+dyt8cONp_nK72eTXXgsOfkHuWpPTr90h=u@mail.gmail.com","threadId":"26864","inReplyTo":"7vbp0ux5c9.fsf_-_@alter.siamese.dyndns.org","subject":"Re: [PATCH] grep: allow -E and -n to be turned on by default via configuration","fromName":"Joe Ratterman","fromEmail":"jratt0@gmail.com","sentAt":"2011-03-28T22:41:16Z","receivedAt":"2011-03-28T22:41:16Z","isPatch":true,"sender":{"key":"jratt0@gmail.com","avatar":"https://avatars.githubusercontent.com/u/66082737?v=4"},"body":"On Mon, Mar 28, 2011 at 5:12 PM, Junio C Hamano wrote:\n> +grep.lineNumbers::\n> +       If set to true, enable '-n' option by default.\n> +\n> +grep.extendedRegexp::\n> +       If set to true, enable '--extended-regexp' option by default.\n> +\n\nI know my original patch was plural, but I since noticed that the GNU\ngrep --line-number option is singular. I used the same thing in my\npatch to add that option to git grep. Should this one be singular?\n\n\n> +grep.lineNumbers::\n> +       If set to true, enable '-n' option by default.\n> +\n> +grep.extendedRegexp::\n> +       If set to true, enable '--extended-regexp' option by default.\n> +\n> +\n...\n> +       if (!strcmp(var, \"grep.extendedregexp\")) {\n> +               opt->regflags |= git_config_bool(var, value) ? REG_EXTENDED : 0;\n> +               return 0;\n> +       }\n> +\n> +       if (!strcmp(var, \"grep.linenumbers\")) {\n> +               opt->linenum = git_config_bool(var, value);\n\nWe need to match the case between the docs and the code or use\nstrcasecmp(). Most config options I could find used all lower-case.\n\n\n\nJoe\n"},{"id":"164502","messageId":"7vr59qvnt3.fsf@alter.siamese.dyndns.org","threadId":"26864","inReplyTo":"AANLkTimpp+dyt8cONp_nK72eTXXgsOfkHuWpPTr90h=u@mail.gmail.com","subject":"Re: [PATCH] grep: allow -E and -n to be turned on by default via configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-28T23:16:08Z","receivedAt":"2011-03-28T23:16:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joe Ratterman writes:\n\n> On Mon, Mar 28, 2011 at 5:12 PM, Junio C Hamano wrote:\n>> +grep.lineNumbers::\n>> +       If set to true, enable '-n' option by default.\n>> +\n>> +grep.extendedRegexp::\n>> +       If set to true, enable '--extended-regexp' option by default.\n>> +\n>\n> I know my original patch was plural, but I since noticed that the GNU\n> grep --line-number option is singular. I used the same thing in my\n> patch to add that option to git grep. Should this one be singular?\n\nGood eyes.\n\nWe should match what we are trying to mimic, and we need to be internally\nconsistent. So --[no-]line-number should be singular to mimic GNU (which\nis how your othr patch is done---I don't have to amend what I already\nqueued). And we should match the variable by naming it \"grep.lineNumber\"\nto the command line option.\n\n>> +       if (!strcmp(var, \"grep.linenumbers\")) {\n>> +               opt->linenum = git_config_bool(var, value);\n>\n> We need to match the case between the docs and the code or use\n> strcasecmp().\n\nIn git_config() callback functions, the variable name is supposed to be\nalready downcased by the caller, except for the second-level name in\nthree-level names, like \"branch.Foo.merge\" where the second-level name is\ncase sensitive. Feeding var we got from the caller and lowercased\nvariable name we expect to strcmp() is the right way to write them when\nyou are dealing with configuration variable names.\n\nSome places might unnecessarily and incorrectly use strcasecmp() but if\nso, we should be fixing them instead. We shouldn't mimic bad code.\n\nIn documentation we customary write them in camelCase only for\nreadability.\n"},{"id":"164524","messageId":"AANLkTikpjoyPWYomKsi0YA=KrcPU2Qxn0dBgjbUi2B_x@mail.gmail.com","threadId":"26864","inReplyTo":"7vbp0ux5c9.fsf_-_@alter.siamese.dyndns.org","subject":"Re: [PATCH] grep: allow -E and -n to be turned on by default via configuration","fromName":"Joe Ratterman","fromEmail":"jratt0@gmail.com","sentAt":"2011-03-29T03:12:27Z","receivedAt":"2011-03-29T03:12:27Z","isPatch":true,"sender":{"key":"jratt0@gmail.com","avatar":"https://avatars.githubusercontent.com/u/66082737?v=4"},"body":"On Mon, Mar 28, 2011 at 5:12 PM, Junio C Hamano wrote:\n>   I did not add tests, but we do need one perhaps at the end of t7810 to\n>   move things forward.\n\n\nWhat sort of tests would be appropriate? I cannot find anything that\ntests -E/--extended-regexp. I copied the first test that used \"git\ngrep -n\" and changed it to these:\n1) git -c grep.linenumber=false grep -n # Expected to have line numbers\n2) git -c grep.linenumber=true grep # Expected to have line numbers\n3) git -c grep.linenumber=true grep --no-line-number # Expected to\nhave no line numbers\n\n\nJoe\n"},{"id":"164714","messageId":"1301513465-15357-1-git-send-email-jratt0@gmail.com","threadId":"26864","inReplyTo":"AANLkTikpjoyPWYomKsi0YA=KrcPU2Qxn0dBgjbUi2B_x@mail.gmail.com","subject":"[PATCH v3] grep: allow -E and -n to be turned on by default via configuration","fromName":"Joe Ratterman","fromEmail":"jratt0@gmail.com","sentAt":"2011-03-30T19:31:05Z","receivedAt":"2011-03-30T19:31:05Z","isPatch":true,"sender":{"key":"jratt0@gmail.com","avatar":"https://avatars.githubusercontent.com/u/66082737?v=4"},"body":"Add two configration variables grep.extendedRegexp and grep.lineNumbers to\nallow the user to skip typing -E and -n on the command line, respectively.\n\nScripts that are meant to be used by random users and/or in random\nrepositories now have use -G and/or --no-line-number options as\nappropriately to override the settings in the repository or user's\n~/.gitconfig settings. Just because the script didn't say \"git grep -n\" no\nlonger guarantees that the output from the command will not have line\nnumbers.\n\nSigned-off-by: Joe Ratterman \nSigned-off-by: Junio C Hamano \n---\n\nThis adds 3 tests to t7810-grep.sh:\n1) git -c grep.linenumber=false grep -n # Expected to have line numbers\n2) git -c grep.linenumber=true grep # Expected to have line numbers\n3) git -c grep.linenumber=true grep --no-line-number # Expected to\nhave no line numbers\n\n\n Documentation/config.txt | 6 ++++++\n Documentation/git-grep.txt | 10 ++++++++++\n builtin/grep.c | 13 +++++++++++++\n t/t7810-grep.sh | 24 +++++++++++++++++++++++-\n 4 files changed, 52 insertions(+), 1 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 701fba9..c63458d 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1092,6 +1092,12 @@ All gitcvs variables except for 'gitcvs.usecrlfattr' and\n is one of \"ext\" and \"pserver\") to make them apply only for the given\n access method.\n \n+grep.lineNumber::\n+\tIf set to true, enable '-n' option by default.\n+\n+grep.extendedRegexp::\n+\tIf set to true, enable '--extended-regexp' option by default.\n+\n gui.commitmsgwidth::\n \tDefines how wide the commit message window is in the\n \tlinkgit:git-gui[1]. \"75\" is the default.\ndiff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\nindex 791d4d4..16a0466 100644\n--- a/Documentation/git-grep.txt\n+++ b/Documentation/git-grep.txt\n@@ -31,6 +31,16 @@ Look for specified patterns in the tracked files in the work tree, blobs\n registered in the index file, or blobs in given tree objects.\n \n \n+CONFIGURATION\n+-------------\n+\n+grep.lineNumber::\n+\tIf set to true, enable '-n' option by default.\n+\n+grep.extendedRegexp::\n+\tIf set to true, enable '--extended-regexp' option by default.\n+\n+\n OPTIONS\n -------\n --cached::\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex bb0f932..dcc5a76 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -302,6 +302,19 @@ static int grep_config(const char *var, const char *value, void *cb)\n \tdefault: return 0;\n \t}\n \n+\tif (!strcmp(var, \"grep.extendedregexp\")) {\n+\t\tif (git_config_bool(var, value))\n+\t\t\topt->regflags |= REG_EXTENDED;\n+\t\telse\n+\t\t\topt->regflags &= ~REG_EXTENDED;\n+\t\treturn 0;\n+\t}\n+\n+\tif (!strcmp(var, \"grep.linenumber\")) {\n+\t\topt->linenum = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n \tif (!strcmp(var, \"color.grep\"))\n \t\topt->color = git_config_colorbool(var, value, -1);\n \telse if (!strcmp(var, \"color.grep.context\"))\ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex c877758..be95bb2 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -59,7 +59,29 @@ do\n \t\t\techo ${HC}file:4:foo mmap bar_mmap\n \t\t\techo ${HC}file:5:foo_mmap bar mmap baz\n \t\t} >expected &&\n-\t\tgit grep -n -w -e mmap $H >actual &&\n+\t\tgit -c grep.linenumber=false grep -n -w -e mmap $H >actual &&\n+\t\ttest_cmp expected actual\n+\t'\n+\n+\ttest_expect_success \"grep -w $L\" '\n+\t\t{\n+\t\t\techo ${HC}file:1:foo mmap bar\n+\t\t\techo ${HC}file:3:foo_mmap bar mmap\n+\t\t\techo ${HC}file:4:foo mmap bar_mmap\n+\t\t\techo ${HC}file:5:foo_mmap bar mmap baz\n+\t\t} >expected &&\n+\t\tgit -c grep.linenumber=true grep -w -e mmap $H >actual &&\n+\t\ttest_cmp expected actual\n+\t'\n+\n+\ttest_expect_success \"grep -w $L\" '\n+\t\t{\n+\t\t\techo ${HC}file:foo mmap bar\n+\t\t\techo ${HC}file:foo_mmap bar mmap\n+\t\t\techo ${HC}file:foo mmap bar_mmap\n+\t\t\techo ${HC}file:foo_mmap bar mmap baz\n+\t\t} >expected &&\n+\t\tgit -c grep.linenumber=true grep --no-line-number -w -e mmap $H >actual &&\n \t\ttest_cmp expected actual\n \t'\n \n-- \n1.7.4.1.4.g0ea33\n"}]}