{"thread":{"id":"29431","subject":"[PATCH/RFC] Fix the result of \"git grep -l -C \"","startedAt":"2012-01-23T16:01:24Z","lastAt":"2012-01-29T23:49:12Z","messageCount":6,"participants":["Albert Yale","Thomas Rast","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"182967","messageId":"1327334484-35196-1-git-send-email-surfingalbert@gmail.com","threadId":"29431","inReplyTo":null,"subject":"[PATCH/RFC] Fix the result of \"git grep -l -C \"","fromName":"Albert Yale","fromEmail":"surfingalbert@gmail.com","sentAt":"2012-01-23T16:01:24Z","receivedAt":"2012-01-23T16:01:24Z","isPatch":true,"sender":{"key":"surfingalbert@gmail.com","avatar":null},"body":"When combining \"git grep -l\" with \"-C \",\nthe first result is omitted.\n\nSigned-off-by: Albert Yale \n---\nFor example, if the following command should output a list of 3\ndifferent files (a.txt, b.txt, c.txt):\n\n$ git grep -l -C 1 albert_yale\nb.txt\nc.txt\n\nThe first result (a.txt) will be missing.\n\nUnderstandably, you wouldn't normally use \"-C\" with \"-l\",\nbut the output should still be correct.\n\nMy solution is to take \"opt.name_only\" into account before setting\n\"skip_first_line\" in grep.c.\n\nI've reproduced this bug with git version 1.7.8.3\nand git version 1.7.9.rc2, both under Mac OS X 10.7.2.\n\nAlbert Yale\n\n builtin/grep.c | 5 ++++-\n 1 files changed, 4 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 9ce064a..076de21 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -1036,7 +1036,10 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \tif (use_threads) {\n \t\tif (opt.pre_context || opt.post_context || opt.file_break ||\n \t\t opt.funcbody)\n-\t\t\tskip_first_line = 1;\n+\t\t{\n+\t\t\tif( ! opt.name_only )\n+\t\t\t\tskip_first_line = 1;\n+\t\t}\n \t\tstart_threads(&opt);\n \t}\n #endif\n-- \n1.7.8.3\n"},{"id":"182968","messageId":"8762g2ieq0.fsf@thomas.inf.ethz.ch","threadId":"29431","inReplyTo":"1327334484-35196-1-git-send-email-surfingalbert@gmail.com","subject":"Re: [PATCH/RFC] Fix the result of \"git grep -l -C \"","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2012-01-23T16:08:23Z","receivedAt":"2012-01-23T16:08:23Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Albert Yale writes:\n\n> When combining \"git grep -l\" with \"-C \",\n> the first result is omitted.\n[...]\n> For example, if the following command should output a list of 3\n> different files (a.txt, b.txt, c.txt):\n>\n> $ git grep -l -C 1 albert_yale\n> b.txt\n> c.txt\n\nNice catch. Can you add a test?\n\nIs -l the only option affected? Why isn't, for example, -L?\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"182972","messageId":"CALEc4zGV6Oo-WR0vPE6=oEmm=fo4dd=nyBWFuK1oU7rmF9K41A@mail.gmail.com","threadId":"29431","inReplyTo":"8762g2ieq0.fsf@thomas.inf.ethz.ch","subject":"Re: [PATCH/RFC] Fix the result of \"git grep -l -C \"","fromName":"Albert Yale","fromEmail":"surfingalbert@gmail.com","sentAt":"2012-01-23T17:10:39Z","receivedAt":"2012-01-23T17:10:39Z","isPatch":true,"sender":{"key":"surfingalbert@gmail.com","avatar":null},"body":"You're right. This solution is incomplete. It needs to take into\naccount \"opt.unmatch_name_only\", and possibly other states. I'm open\nfor suggestions.\n\nI've just posted Version 2 of this patch.\n\nAs for creating a test, I'm unfamiliar with the testing procedure for\ngit-core. A \"how to\" in the \"Documentation\" folder would be very\nuseful in that regard.\n\nAlbert\n\nOn Mon, Jan 23, 2012 at 11:08 AM, Thomas Rast wrote:\n> Albert Yale writes:\n>\n>> When combining \"git grep -l\" with \"-C \",\n>> the first result is omitted.\n> [...]\n>> For example, if the following command should output a list of 3\n>> different files (a.txt, b.txt, c.txt):\n>>\n>> $ git grep -l -C 1 albert_yale\n>> b.txt\n>> c.txt\n>\n> Nice catch.  Can you add a test?\n>\n> Is -l the only option affected?  Why isn't, for example, -L?\n>\n> --\n> Thomas Rast\n> trast@{inf,student}.ethz.ch\n"},{"id":"182973","messageId":"74777e0e8633d980fee9a1a680a63535be042fdc.1327340917.git.trast@student.ethz.ch","threadId":"29431","inReplyTo":"CALEc4zGV6Oo-WR0vPE6=oEmm=fo4dd=nyBWFuK1oU7rmF9K41A@mail.gmail.com","subject":"[PATCH] grep: fix -l/-L interaction with decoration lines","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2012-01-23T17:52:44Z","receivedAt":"2012-01-23T17:52:44Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"From: Albert Yale \n\nIn threaded mode, git-grep emits file breaks (enabled with context, -W\nand --break) into the accumulation buffers even if they are not\nrequired. The output collection thread then uses skip_first_line to\nskip the first such line in the output, which would otherwise be at\nthe very top.\n\nThis is wrong when the user also specified -l/-L/-c, in which case\nevery line is relevant. While arguably giving these options together\ndoesn't make any sense, git-grep has always quietly accepted it. So\ndo not skip anything in these cases.\n\nSigned-off-by: Albert Yale \nSigned-off-by: Thomas Rast \n---\n\n> Reviewed-by: Thomas Rast \n\nPlease don't. I didn't actually read the patch or look at the code,\nor say so, and you're claiming I did. I was working purely from the\ncommit message.\n\n> As for creating a test, I'm unfamiliar with the testing procedure for\n> git-core. A \"how to\" in the \"Documentation\" folder would be very\n> useful in that regard.\n\nWell, there's t/README.\n\n\nHere's a patch that also does -c and has tests. Placing them was more\nfinicky than I hoped; the list of files in the repo varies wildly\nacross the test set. It also exploits knowledge that git-ls-files\norder is the same as 'git grep -l' order, which might not be\nappropriate.\n\n\n builtin/grep.c | 6 ++++--\n t/t7810-grep.sh | 22 ++++++++++++++++++++++\n 2 files changed, 26 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 9ce064a..1120b9f 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -1034,8 +1034,10 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \n #ifndef NO_PTHREADS\n \tif (use_threads) {\n-\t\tif (opt.pre_context || opt.post_context || opt.file_break ||\n-\t\t opt.funcbody)\n+\t\tif (!(opt.name_only || opt.unmatch_name_only ||\n+\t\t opt.count)\n+\t\t && (opt.pre_context || opt.post_context ||\n+\t\t\topt.file_break || opt.funcbody))\n \t\t\tskip_first_line = 1;\n \t\tstart_threads(&opt);\n \t}\ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex 7ba5b16..75f4716 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -246,6 +246,28 @@ do\n done\n \n cat >expected <actual &&\n+\ttest_cmp expected actual\n+'\n+\n+cat >expected <actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'grep -L -C' '\n+\tgit ls-files >expected &&\n+\tgit grep -L -C1 nonexistent_string >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+cat >expected < wrote:\n> From: Albert Yale \n>\n> In threaded mode, git-grep emits file breaks (enabled with context, -W\n> and --break) into the accumulation buffers even if they are not\n> required.  The output collection thread then uses skip_first_line to\n> skip the first such line in the output, which would otherwise be at\n> the very top.\n>\n> This is wrong when the user also specified -l/-L/-c, in which case\n> every line is relevant.  While arguably giving these options together\n> doesn't make any sense, git-grep has always quietly accepted it.  So\n> do not skip anything in these cases.\n>\n> Signed-off-by: Albert Yale \n> Signed-off-by: Thomas Rast \n> ---\n>\n>> Reviewed-by: Thomas Rast \n>\n> Please don't.  I didn't actually read the patch or look at the code,\n> or say so, and you're claiming I did.  I was working purely from the\n> commit message.\n>\n>> As for creating a test, I'm unfamiliar with the testing procedure for\n>> git-core. A \"how to\" in the \"Documentation\" folder would be very\n>> useful in that regard.\n>\n> Well, there's t/README.\n>\n>\n> Here's a patch that also does -c and has tests.  Placing them was more\n> finicky than I hoped; the list of files in the repo varies wildly\n> across the test set.  It also exploits knowledge that git-ls-files\n> order is the same as 'git grep -l' order, which might not be\n> appropriate.\n>\n>\n>  builtin/grep.c  |    6 ++++--\n>  t/t7810-grep.sh |   22 ++++++++++++++++++++++\n>  2 files changed, 26 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index 9ce064a..1120b9f 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -1034,8 +1034,10 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n>\n>  #ifndef NO_PTHREADS\n>        if (use_threads) {\n> -               if (opt.pre_context || opt.post_context || opt.file_break ||\n> -                   opt.funcbody)\n> +               if (!(opt.name_only || opt.unmatch_name_only ||\n> +                     opt.count)\n> +                   && (opt.pre_context || opt.post_context ||\n> +                       opt.file_break || opt.funcbody))\n>                        skip_first_line = 1;\n>                start_threads(&opt);\n>        }\n> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\n> index 7ba5b16..75f4716 100755\n> --- a/t/t7810-grep.sh\n> +++ b/t/t7810-grep.sh\n> @@ -246,6 +246,28 @@ do\n>  done\n>\n>  cat >expected < +file\n> +EOF\n> +test_expect_success 'grep -l -C' '\n> +       git grep -l -C1 foo >actual &&\n> +       test_cmp expected actual\n> +'\n> +\n> +cat >expected < +file:5\n> +EOF\n> +test_expect_success 'grep -l -C' '\n> +       git grep -c -C1 foo >actual &&\n> +       test_cmp expected actual\n> +'\n> +\n> +test_expect_success 'grep -L -C' '\n> +       git ls-files >expected &&\n> +       git grep -L -C1 nonexistent_string >actual &&\n> +       test_cmp expected actual\n> +'\n> +\n> +cat >expected <  file:foo mmap bar_mmap\n>  EOF\n>\n> --\n> 1.7.9.rc2.215.gd9e83\n>\n"},{"id":"183271","messageId":"7vhazedq87.fsf@alter.siamese.dyndns.org","threadId":"29431","inReplyTo":"74777e0e8633d980fee9a1a680a63535be042fdc.1327340917.git.trast@student.ethz.ch","subject":"Re: [PATCH] grep: fix -l/-L interaction with decoration lines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-29T23:49:12Z","receivedAt":"2012-01-29T23:49:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks, makes sense.\n"}]}