{"thread":{"id":"25289","subject":"[RFC PATCH 1/3] add test for git grep --recursive","startedAt":"2010-09-29T20:28:51Z","lastAt":"2010-10-01T16:26:29Z","messageCount":22,"participants":["Chris Packham","Ævar Arnfjörð Bjarmason","Kevin Ballard","Jens Lehmann","Junio C Hamano","Nguyen Thai Ngoc Duy","Johannes Sixt","Heiko Voigt"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"152084","messageId":"1285792134-26339-1-git-send-email-judge.packham@gmail.com","threadId":"25289","inReplyTo":null,"subject":"[RFC PATCH 0/3] grep: submodule support","fromName":"Chris Packham","fromEmail":"judge.packham@gmail.com","sentAt":"2010-09-29T20:28:51Z","receivedAt":"2010-09-29T20:28:51Z","isPatch":true,"sender":{"key":"judge.packham@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155667?v=4"},"body":"This patch series is my initial attempt to add submodule awareness to git grep.\nIt's also the first time I've played with the git C code so expect bugs. The\npatches are based off apply on Jens Lehmann's 'enhance_git_for_submodules'\nbranch in git://github.com/jlehmann/git-submod-enhancements.git\n\nThe first patch adds some basic tests for grep with submodules. There is\nprobably some overlap with other grep tests so I'll have a more in-depth look\nat what is needed later. I have a problem with the tests that when I invoke\n'git grep' using run_command I actually end up using the installed git which\ndoesn't understand my new --submodule-prefix option.\n\nThe 2nd patch just adds a --submodule-prefix option so that I can prepend some\ntext to the output from the sub processes.\n\nThe 3rd patch is the main implementation. Currently I rebuild a command line\nfor the subprocess based on the opts structure and I make use of the modified\nargv[0] from the command. Neither of these are really optimal, it'd be much\neasier if I could just start my subprocess from cmd_grep (or even grep_cache).\nAny pointers to get me moving in this direction are welcome. Even if I retain\nthe rebuilding of the command line I'd like to rebuild the pattern(s) instead\nof relying on the saved_argv[0].\n\nChris Packham (3):\n      add test for git grep --recursive\n      grep: prepare grep for submodules\n      grep: add support for grepping in submodules\n"},{"id":"152083","messageId":"1285792134-26339-2-git-send-email-judge.packham@gmail.com","threadId":"25289","inReplyTo":"1285792134-26339-1-git-send-email-judge.packham@gmail.com","subject":"[RFC PATCH 1/3] add test for git grep --recursive","fromName":"Chris Packham","fromEmail":"judge.packham@gmail.com","sentAt":"2010-09-29T20:28:52Z","receivedAt":"2010-09-29T20:28:52Z","isPatch":true,"sender":{"key":"judge.packham@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155667?v=4"},"body":"Signed-off-by: Chris Packham <judge.packham@gmail.com>\n---\n t/t7820-grep-recursive.sh |  101 +++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 101 insertions(+), 0 deletions(-)\n create mode 100644 t/t7820-grep-recursive.sh\n\ndiff --git a/t/t7820-grep-recursive.sh b/t/t7820-grep-recursive.sh\nnew file mode 100644\nindex 0000000..4bbd109\n--- /dev/null\n+++ b/t/t7820-grep-recursive.sh\n@@ -0,0 +1,101 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2010 Chris Packham\n+#\n+\n+test_description='git grep --recursive test\n+\n+This test checks the ability of git grep to search within submodules when told\n+to do so with the --recursive option'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup - initial commit' '\n+\tprintf \"one two three\\nfour five six\\n\" >t &&\n+\tgit add t &&\n+\tgit commit -m \"initial commit\"\n+'\n+submodurl=$TRASH_DIRECTORY\n+\n+test_expect_success 'setup submodules for test' '\n+\tfor mod in $(seq 1 5 | sed \"s/.*/submodule&/\"); do\n+\t\tgit submodule add \"$submodurl\" $mod &&\n+\t\tgit submodule init $mod\n+\tdone\n+'\n+\n+test_expect_success 'update data in each submodule' '\n+\tfor n in $(seq 1 5); do\n+\t\t(cd submodule$n &&\n+\t\t\tsed -i \"s/^four.*/& #$n/\" t &&\n+\t\t\tgit commit -a -m\"update\")\n+\tdone\n+'\n+\n+cat >expected <<EOF\n+t:four five six\n+EOF\n+test_expect_success 'non-recursive grep in base' '\n+\tgit grep \"five\" >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+cat >expected <<EOF\n+foo/t:four five six\n+EOF\n+test_expect_success 'submodule-prefix option' '\n+\tgit grep --submodule-prefix=foo/ \"five\" >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+cat >submodule1/expected <<EOF\n+t:four five six #1\n+EOF\n+test_expect_success 'non-recursive grep in submodule' '\n+\t(\n+\t\tcd submodule1 &&\n+\t\tgit grep \"five\" >actual &&\n+\t\ttest_cmp expected actual\n+\t)\n+'\n+\n+cat >expected <<EOF\n+t:four five six #1\n+t:four five six #2\n+t:four five six #3\n+t:four five six #4\n+t:four five six #5\n+t:four five six\n+EOF\n+test_expect_success 'recursive grep' '\n+\tgit grep --recurse-submodules \"five\" >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+cat >expected <<EOF\n+t:2:four five six #1\n+t:2:four five six #2\n+t:2:four five six #3\n+t:2:four five six #4\n+t:2:four five six #5\n+t:2:four five six\n+EOF\n+test_expect_success 'recursive grep (with -n)' '\n+\tgit grep --recurse-submodules -n \"five\" >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+cat >expected <<EOF\n+t\n+t\n+t\n+t\n+t\n+t\n+EOF\n+test_expect_success 'recursive grep (with -l)' '\n+\tgit grep --recurse-submodules -l \"five\" >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_done\n-- \n1.7.3.dirty\n"},{"id":"152086","messageId":"1285792134-26339-3-git-send-email-judge.packham@gmail.com","threadId":"25289","inReplyTo":"1285792134-26339-1-git-send-email-judge.packham@gmail.com","subject":"[RFC PATCH 2/3] grep: prepare grep for submodules","fromName":"Chris Packham","fromEmail":"judge.packham@gmail.com","sentAt":"2010-09-29T20:28:53Z","receivedAt":"2010-09-29T20:28:53Z","isPatch":true,"sender":{"key":"judge.packham@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155667?v=4"},"body":"Add --submodule-prefix option to pass to subprocess grep invocations. The\nprefix is then used when outputting the results.\n\nSigned-off-by: Chris Packham <judge.packham@gmail.com>\n---\n builtin/grep.c |    2 ++\n grep.c         |    8 ++++++++\n grep.h         |    1 +\n 3 files changed, 11 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex da32f3d..8315ff0 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -927,6 +927,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t\t    \"allow calling of grep(1) (ignored by this build)\"),\n \t\t{ OPTION_CALLBACK, 0, \"help-all\", &options, NULL, \"show usage\",\n \t\t  PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, help_callback },\n+\t\tOPT_STRING(0, \"submodule-prefix\", &opt.submodule_prefix, \"DIR\",\n+\t\t\t\"prepend this to submodule path output\"),\n \t\tOPT_END()\n \t};\n \ndiff --git a/grep.c b/grep.c\nindex 63c4280..36bec98 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -370,6 +370,10 @@ static void output_sep(struct grep_opt *opt, char sign)\n \n static void show_name(struct grep_opt *opt, const char *name)\n {\n+\tif (opt->submodule_prefix) {\n+\t\toutput_color(opt, opt->submodule_prefix,\n+\t\t\tstrlen(opt->submodule_prefix), opt->color_filename);\n+\t}\n \toutput_color(opt, name, strlen(name), opt->color_filename);\n \topt->output(opt, opt->null_following_name ? \"\\0\" : \"\\n\", 1);\n }\n@@ -644,6 +648,10 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol,\n \t}\n \topt->last_shown = lno;\n \n+\tif (opt->submodule_prefix) {\n+\t\toutput_color(opt, opt->submodule_prefix,\n+\t\t\tstrlen(opt->submodule_prefix), opt->color_filename);\n+\t}\n \tif (opt->pathname) {\n \t\toutput_color(opt, name, strlen(name), opt->color_filename);\n \t\toutput_sep(opt, sign);\ndiff --git a/grep.h b/grep.h\nindex 06621fe..d918da4 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -67,6 +67,7 @@ struct grep_opt {\n \tstruct grep_expr *pattern_expression;\n \tconst char *prefix;\n \tint prefix_length;\n+\tconst char *submodule_prefix;\n \tregex_t regexp;\n \tint linenum;\n \tint invert;\n-- \n1.7.3.dirty\n"},{"id":"152085","messageId":"1285792134-26339-4-git-send-email-judge.packham@gmail.com","threadId":"25289","inReplyTo":"1285792134-26339-1-git-send-email-judge.packham@gmail.com","subject":"[RFC PATCH 3/3] grep: add support for grepping in submodules","fromName":"Chris Packham","fromEmail":"judge.packham@gmail.com","sentAt":"2010-09-29T20:28:54Z","receivedAt":"2010-09-29T20:28:54Z","isPatch":true,"sender":{"key":"judge.packham@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155667?v=4"},"body":"When the --recurse-submodules option is given git grep will search in\nsubmodules as they are encountered.\n\nSigned-off-by: Chris Packham <judge.packham@gmail.com>\n---\n builtin/grep.c |   72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n grep.h         |    1 +\n 2 files changed, 73 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 8315ff0..c9befdc 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -30,6 +30,9 @@ static char const * const grep_usage[] = {\n \n static int use_threads = 1;\n \n+static int saved_argc;\n+static const char **saved_argv;\n+\n #ifndef NO_PTHREADS\n #define THREADS 8\n static pthread_t threads[THREADS];\n@@ -585,6 +588,67 @@ static void run_pager(struct grep_opt *opt, const char *prefix)\n \tfree(argv);\n }\n \n+static const char **create_sub_grep_argv(struct grep_opt *opt, const char *path)\n+{\n+\t#define NUM_ARGS 10\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tconst char **argv;\n+\tint i = 0;\n+\n+\targv = xcalloc(NUM_ARGS, sizeof(const char *));\n+\targv[i++] = \"grep\";\n+\tstrbuf_addf(&buf, \"--submodule-prefix=\\\\\\\"%s\\\\\\\"\", path);\n+\t//argv[i++] = buf.buf;\n+\n+\tif (opt->linenum)\n+\t\targv[i++] = \"-n\";\n+\tif (opt->invert)\n+\t\targv[i++] = \"-v\";\n+\tif (opt->ignore_case)\n+\t\targv[i++] = \"-i\";\n+\tif (opt->count)\n+\t\targv[i++] = \"-c\";\n+\tif (opt->name_only)\n+\t\targv[i++] = \"-l\";\n+\n+\targv[i++] = saved_argv[0];\n+\targv[i++] = NULL;\n+\n+\tstrbuf_release(&buf);\n+\treturn argv;\n+}\n+\n+static int grep_submodule(struct grep_opt *opt, const char *path)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tstruct child_process cp;\t\n+\tconst char **argv = create_sub_grep_argv(opt, path);\n+\tconst char *git_dir;\n+\tint hit = 0;\n+\n+\tstrbuf_addf(&buf, \"%s/.git\", path);\n+\tgit_dir = read_gitfile_gently(buf.buf);\n+\tif (!git_dir)\n+\t\tgit_dir = buf.buf;\n+\tif (!is_directory(git_dir)) {\n+\t\twarning(\"submodule %s has not been initialized\\n\", path);\n+\t\tgoto out_free;\n+\t}\n+\n+\tmemset(&cp, 0, sizeof(cp));\n+\tcp.argv = argv;\n+\tcp.env = local_repo_env;\n+\tcp.git_cmd = 1;\n+\tcp.no_stdin = 1;\n+\tcp.dir = path;\n+\tif (run_command(&cp) == 0)\n+\t\thit = 1;\n+out_free:\n+\tfree(argv);\n+\tstrbuf_release(&buf);\n+\treturn hit;\n+}\n+\n static int grep_cache(struct grep_opt *opt, const char **paths, int cached)\n {\n \tint hit = 0;\n@@ -593,6 +657,10 @@ static int grep_cache(struct grep_opt *opt, const char **paths, int cached)\n \n \tfor (nr = 0; nr < active_nr; nr++) {\n \t\tstruct cache_entry *ce = active_cache[nr];\n+\t\tif (S_ISGITLINK(ce->ce_mode) && opt->recurse_submodules) {\n+\t\t\thit |= grep_submodule(opt, ce->name);\n+\t\t\tcontinue;\n+\t\t}\n \t\tif (!S_ISREG(ce->ce_mode))\n \t\t\tcontinue;\n \t\tif (!pathspec_matches(paths, ce->name, opt->max_depth))\n@@ -929,9 +997,13 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t  PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, help_callback },\n \t\tOPT_STRING(0, \"submodule-prefix\", &opt.submodule_prefix, \"DIR\",\n \t\t\t\"prepend this to submodule path output\"),\n+\t\tOPT_BOOLEAN(0, \"recurse-submodules\", &opt.recurse_submodules,\n+\t\t\t\"recurse into submodules\"),\n \t\tOPT_END()\n \t};\n \n+\tsaved_argc = argc;\n+\tsaved_argv = argv;\n \t/*\n \t * 'git grep -h', unlike 'git grep -h <pattern>', is a request\n \t * to show usage information and exit.\ndiff --git a/grep.h b/grep.h\nindex d918da4..e3199c9 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -102,6 +102,7 @@ struct grep_opt {\n \tunsigned post_context;\n \tunsigned last_shown;\n \tint show_hunk_mark;\n+\tint recurse_submodules;\n \tvoid *priv;\n \n \tvoid (*output)(struct grep_opt *opt, const void *data, size_t size);\n-- \n1.7.3.dirty\n"},{"id":"152088","messageId":"AANLkTikAUe=YWHcgF33aNFHesuxHtgCTXNZmbRoV99c-@mail.gmail.com","threadId":"25289","inReplyTo":"1285792134-26339-2-git-send-email-judge.packham@gmail.com","subject":"Re: [RFC PATCH 1/3] add test for git grep --recursive","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-09-29T20:35:50Z","receivedAt":"2010-09-29T20:35:50Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Wed, Sep 29, 2010 at 20:28, Chris Packham <judge.packham@gmail.com> wrote:\n> Signed-off-by: Chris Packham <judge.packham@gmail.com>\n> ---\n>  t/t7820-grep-recursive.sh |  101 +++++++++++++++++++++++++++++++++++++++++++++\n>  1 files changed, 101 insertions(+), 0 deletions(-)\n>  create mode 100644 t/t7820-grep-recursive.sh\n>\n> diff --git a/t/t7820-grep-recursive.sh b/t/t7820-grep-recursive.sh\n> new file mode 100644\n> index 0000000..4bbd109\n> --- /dev/null\n> +++ b/t/t7820-grep-recursive.sh\n> @@ -0,0 +1,101 @@\n> +#!/bin/sh\n> +#\n> +# Copyright (c) 2010 Chris Packham\n> +#\n> +\n> +test_description='git grep --recursive test\n> +\n> +This test checks the ability of git grep to search within submodules when told\n> +to do so with the --recursive option'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'setup - initial commit' '\n> +       printf \"one two three\\nfour five six\\n\" >t &&\n> +       git add t &&\n> +       git commit -m \"initial commit\"\n> +'\n> +submodurl=$TRASH_DIRECTORY\n> +\n> +test_expect_success 'setup submodules for test' '\n> +       for mod in $(seq 1 5 | sed \"s/.*/submodule&/\"); do\n> +               git submodule add \"$submodurl\" $mod &&\n> +               git submodule init $mod\n> +       done\n> +'\n> +\n> +test_expect_success 'update data in each submodule' '\n> +       for n in $(seq 1 5); do\n\nseq isn't portable to windows, so we usually write out \"1 2 3 4 5\"\ndirectly.\n\n> +               (cd submodule$n &&\n> +                       sed -i \"s/^four.*/& #$n/\" t &&\n> +                       git commit -a -m\"update\")\n> +       done\n> +'\n> +\n> +cat >expected <<EOF\n> +t:four five six\n> +EOF\n> +test_expect_success 'non-recursive grep in base' '\n> +       git grep \"five\" >actual &&\n> +       test_cmp expected actual\n> +'\n\nPut the \"cat >expected <<EOF\" inside the test:\n\n    test_expect_success 'non-recursive grep in base' '\n        cat >expected <<\\EOF &&\n        t:four five six\n        EOF\n        git grep \"five\" >actual &&\n        test_cmp expected actual\n    '\n\nditto for the rest.\n"},{"id":"152090","messageId":"4CA3A619.70507@gmail.com","threadId":"25289","inReplyTo":"AANLkTikAUe=YWHcgF33aNFHesuxHtgCTXNZmbRoV99c-@mail.gmail.com","subject":"Re: [RFC PATCH 1/3] add test for git grep --recursive","fromName":"Chris Packham","fromEmail":"judge.packham@gmail.com","sentAt":"2010-09-29T20:48:25Z","receivedAt":"2010-09-29T20:48:25Z","isPatch":true,"sender":{"key":"judge.packham@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155667?v=4"},"body":"On 29/09/10 13:35, Ævar Arnfjörð Bjarmason wrote:\n> On Wed, Sep 29, 2010 at 20:28, Chris Packham <judge.packham@gmail.com> wrote:\n>> Signed-off-by: Chris Packham <judge.packham@gmail.com>\n>> ---\n>>  t/t7820-grep-recursive.sh |  101 +++++++++++++++++++++++++++++++++++++++++++++\n>>  1 files changed, 101 insertions(+), 0 deletions(-)\n>>  create mode 100644 t/t7820-grep-recursive.sh\n>>\n>> diff --git a/t/t7820-grep-recursive.sh b/t/t7820-grep-recursive.sh\n>> new file mode 100644\n>> index 0000000..4bbd109\n>> --- /dev/null\n>> +++ b/t/t7820-grep-recursive.sh\n>> @@ -0,0 +1,101 @@\n>> +#!/bin/sh\n>> +#\n>> +# Copyright (c) 2010 Chris Packham\n>> +#\n>> +\n>> +test_description='git grep --recursive test\n>> +\n>> +This test checks the ability of git grep to search within submodules when told\n>> +to do so with the --recursive option'\n>> +\n>> +. ./test-lib.sh\n>> +\n>> +test_expect_success 'setup - initial commit' '\n>> +       printf \"one two three\\nfour five six\\n\" >t &&\n>> +       git add t &&\n>> +       git commit -m \"initial commit\"\n>> +'\n>> +submodurl=$TRASH_DIRECTORY\n>> +\n>> +test_expect_success 'setup submodules for test' '\n>> +       for mod in $(seq 1 5 | sed \"s/.*/submodule&/\"); do\n>> +               git submodule add \"$submodurl\" $mod &&\n>> +               git submodule init $mod\n>> +       done\n>> +'\n>> +\n>> +test_expect_success 'update data in each submodule' '\n>> +       for n in $(seq 1 5); do\n> \n> seq isn't portable to windows, so we usually write out \"1 2 3 4 5\"\n> directly.\n> \n>> +               (cd submodule$n &&\n>> +                       sed -i \"s/^four.*/& #$n/\" t &&\n>> +                       git commit -a -m\"update\")\n>> +       done\n>> +'\n>> +\n>> +cat >expected <<EOF\n>> +t:four five six\n>> +EOF\n>> +test_expect_success 'non-recursive grep in base' '\n>> +       git grep \"five\" >actual &&\n>> +       test_cmp expected actual\n>> +'\n> \n> Put the \"cat >expected <<EOF\" inside the test:\n> \n>     test_expect_success 'non-recursive grep in base' '\n>         cat >expected <<\\EOF &&\n>         t:four five six\n>         EOF\n>         git grep \"five\" >actual &&\n>         test_cmp expected actual\n>     '\n> \n> ditto for the rest.\n\nThanks for the review, will be in next re-roll.\n"},{"id":"152092","messageId":"E2FEFF76-77DF-4DA3-AFC1-914417581A81@sb.org","threadId":"25289","inReplyTo":"AANLkTikAUe=YWHcgF33aNFHesuxHtgCTXNZmbRoV99c-@mail.gmail.com","subject":"Re: [RFC PATCH 1/3] add test for git grep --recursive","fromName":"Kevin Ballard","fromEmail":"kevin@sb.org","sentAt":"2010-09-29T21:34:21Z","receivedAt":"2010-09-29T21:34:21Z","isPatch":true,"sender":{"key":"kevin@sb.org","avatar":"https://avatars.githubusercontent.com/u/714?v=4"},"body":"On Sep 29, 2010, at 1:35 PM, Ævar Arnfjörð Bjarmason wrote:\n\n>> +test_expect_success 'update data in each submodule' '\n>> +       for n in $(seq 1 5); do\n> \n> seq isn't portable to windows, so we usually write out \"1 2 3 4 5\"\n> directly.\n\nFWIW, seq isn't even provided by default on OS X.\n\n-Kevin Ballard"},{"id":"152096","messageId":"4CA3BBD7.3090006@web.de","threadId":"25289","inReplyTo":"1285792134-26339-4-git-send-email-judge.packham@gmail.com","subject":"Re: [RFC PATCH 3/3] grep: add support for grepping in submodules","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2010-09-29T22:21:11Z","receivedAt":"2010-09-29T22:21:11Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Nice start!\n\n\nAm 29.09.2010 22:28, schrieb Chris Packham:\n> When the --recurse-submodules option is given git grep will search in\n> submodules as they are encountered.\n\nAs \"git clone\" already introduced a \"--recursive\" option for\nsubmodule recursion IMO \"--recursive\" should be used here too for\nconsistency. (Maybe you took the idea to use \"--recurse-submodules\"\nfrom my \"git-checkout-recurse-submodules\" branch on github? But that\nis only used there because I didn't get around to change it yet like\nI did in the \"fetch-submodules-too\" branch).\n\n\n> Signed-off-by: Chris Packham <judge.packham@gmail.com>\n> ---\n>  builtin/grep.c |   72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n>  grep.h         |    1 +\n>  2 files changed, 73 insertions(+), 0 deletions(-)\n> \n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index 8315ff0..c9befdc 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -30,6 +30,9 @@ static char const * const grep_usage[] = {\n>  \n>  static int use_threads = 1;\n>  \n> +static int saved_argc;\n> +static const char **saved_argv;\n> +\n>  #ifndef NO_PTHREADS\n>  #define THREADS 8\n>  static pthread_t threads[THREADS];\n> @@ -585,6 +588,67 @@ static void run_pager(struct grep_opt *opt, const char *prefix)\n>  \tfree(argv);\n>  }\n>  \n> +static const char **create_sub_grep_argv(struct grep_opt *opt, const char *path)\n> +{\n> +\t#define NUM_ARGS 10\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tconst char **argv;\n> +\tint i = 0;\n> +\n> +\targv = xcalloc(NUM_ARGS, sizeof(const char *));\n> +\targv[i++] = \"grep\";\n> +\tstrbuf_addf(&buf, \"--submodule-prefix=\\\\\\\"%s\\\\\\\"\", path);\n> +\t//argv[i++] = buf.buf;\n\nAs C++ comments are not portable they have to be avoided, but I\nassume this one here (and the unused \"saved_argc\" variable too) is\na hint that this code is not working as intended yet? ;-)\n\nIt seems you want to use strbuf_detach() here so that this argv[]\nstays valid after the strbuf_release() at the end of this function.\nAnd if I'm not missing something this would not work correctly in\nthe second recursion depth, as the new submodule prefix should\nbe the one given to this grep command concatenated with the current\nsubmodule path.\n\n\n> +\tif (opt->linenum)\n> +\t\targv[i++] = \"-n\";\n> +\tif (opt->invert)\n> +\t\targv[i++] = \"-v\";\n> +\tif (opt->ignore_case)\n> +\t\targv[i++] = \"-i\";\n> +\tif (opt->count)\n> +\t\targv[i++] = \"-c\";\n> +\tif (opt->name_only)\n> +\t\targv[i++] = \"-l\";\n> +\n> +\targv[i++] = saved_argv[0];\n> +\targv[i++] = NULL;\n\nHm, at a quick glance it might be much easier to copy argc & argv\nin cmd_grep() before parse_options() starts manipulating it. Then\nyou would only have to change/add the \"--submodule-prefix\" option\nas needed and would not have to deal with all possible grep option\ncombinations (and for example you don't pass the recurse option\nyet, which would stop the recursion pretty soon).\n\n\n> +\n> +\tstrbuf_release(&buf);\n> +\treturn argv;\n> +}\n> +\n> +static int grep_submodule(struct grep_opt *opt, const char *path)\n> +{\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tstruct child_process cp;\t\n> +\tconst char **argv = create_sub_grep_argv(opt, path);\n> +\tconst char *git_dir;\n> +\tint hit = 0;\n> +\n> +\tstrbuf_addf(&buf, \"%s/.git\", path);\n> +\tgit_dir = read_gitfile_gently(buf.buf);\n> +\tif (!git_dir)\n> +\t\tgit_dir = buf.buf;\n> +\tif (!is_directory(git_dir)) {\n> +\t\twarning(\"submodule %s has not been initialized\\n\", path);\n\nHaving a submodule which is not checked out is perfectly fine, so\nI don't think we want to issue a warning here.\n\n\n> +\t\tgoto out_free;\n> +\t}\n> +\n> +\tmemset(&cp, 0, sizeof(cp));\n> +\tcp.argv = argv;\n> +\tcp.env = local_repo_env;\n> +\tcp.git_cmd = 1;\n> +\tcp.no_stdin = 1;\n> +\tcp.dir = path;\n> +\tif (run_command(&cp) == 0)\n> +\t\thit = 1;\n> +out_free:\n> +\tfree(argv);\n> +\tstrbuf_release(&buf);\n> +\treturn hit;\n> +}\n> +\n>  static int grep_cache(struct grep_opt *opt, const char **paths, int cached)\n>  {\n>  \tint hit = 0;\n> @@ -593,6 +657,10 @@ static int grep_cache(struct grep_opt *opt, const char **paths, int cached)\n>  \n>  \tfor (nr = 0; nr < active_nr; nr++) {\n>  \t\tstruct cache_entry *ce = active_cache[nr];\n> +\t\tif (S_ISGITLINK(ce->ce_mode) && opt->recurse_submodules) {\n> +\t\t\thit |= grep_submodule(opt, ce->name);\n> +\t\t\tcontinue;\n> +\t\t}\n>  \t\tif (!S_ISREG(ce->ce_mode))\n>  \t\t\tcontinue;\n>  \t\tif (!pathspec_matches(paths, ce->name, opt->max_depth))\n> @@ -929,9 +997,13 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n>  \t\t  PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, help_callback },\n>  \t\tOPT_STRING(0, \"submodule-prefix\", &opt.submodule_prefix, \"DIR\",\n>  \t\t\t\"prepend this to submodule path output\"),\n> +\t\tOPT_BOOLEAN(0, \"recurse-submodules\", &opt.recurse_submodules,\n> +\t\t\t\"recurse into submodules\"),\n>  \t\tOPT_END()\n>  \t};\n>  \n> +\tsaved_argc = argc;\n> +\tsaved_argv = argv;\n>  \t/*\n>  \t * 'git grep -h', unlike 'git grep -h <pattern>', is a request\n>  \t * to show usage information and exit.\n> diff --git a/grep.h b/grep.h\n> index d918da4..e3199c9 100644\n> --- a/grep.h\n> +++ b/grep.h\n> @@ -102,6 +102,7 @@ struct grep_opt {\n>  \tunsigned post_context;\n>  \tunsigned last_shown;\n>  \tint show_hunk_mark;\n> +\tint recurse_submodules;\n>  \tvoid *priv;\n>  \n>  \tvoid (*output)(struct grep_opt *opt, const void *data, size_t size);\n"},{"id":"152101","messageId":"7v4od8ma0j.fsf@alter.siamese.dyndns.org","threadId":"25289","inReplyTo":"4CA3BBD7.3090006@web.de","subject":"Re: [RFC PATCH 3/3] grep: add support for grepping in submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-09-29T22:59:24Z","receivedAt":"2010-09-29T22:59:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jens Lehmann <Jens.Lehmann@web.de> writes:\n\n> Hm, at a quick glance it might be much easier to copy argc & argv\n> in cmd_grep() before parse_options() starts manipulating it.\n\nYes, I think that is a much saner direction to go.  Otherwise, you would\nneed to unparse grep boolean expressions as well.\n\nA few more things to think about.\n\n1. What does this mean:\n\n    $ git grep --recursive -e frotz master next\n\nIt recurses into the submodule commits recorded in 'master' and 'next'\ncommits in the superproject, right?\n\nHow do the lines output from the above look like?  From the superproject,\nwe will get lines like these:\n\n    master:t/README:  test_description='xxx test (option --frotz)\n    master:t/README:  and tries to run git-ls-files with option --frotz.'\n\nWhat if we have a submodule at git-gui in the superproject, and its README\nhas string frotz in it?  Should we label the submodule commit we find in\n'master' of superproject as 'master' as well, even if it is not at the tip\nof 'master' branch of the submodule?  Or do we get abbreviated hexadecimal\nSHA-1 name?  IOW, would we see:\n\n    master:git-gui/README: git-gui also knows frotz\n\nor\n\n    deadbeef:git-gui/README: git-gui also knows frotz\n\nwhere \"deadbeaf....\" is what \"git rev-parse master:git-gui\" would give us\nin the superproject?\n\nI tend to think the former is preferable, but then most likely you would\nneed to pass not just submodule-prefix but the original ref name\n(i.e. 'master') you started from down to the recursive one.\n\n2. Now how would this work with pathspecs?\n\n    $ git grep --recursive -e frotz -- dir/\n\nThis should look for the string in the named directory in the superproject\nand if there are submodules in that directory, recurse into them as well,\nright?\n\nWhat pathspec, if any, will be in effect when we recurse into the\nsubmodule at dir/sub?  Limiting to dir/ feels wrong, no?\n\n3. Corollary.\n\nIs it reasonable to expect that we will look into all shell scripts, both\nin the superproject and in submodules, with this:\n\n    $ git grep --recursive -e frotz -- '*.sh'\n\nOops?  What happened to the \"we restrict the recursion using pathspec, and\nwe do not pass down the pathspec\" that was suggested in 2.?\n"},{"id":"152102","messageId":"4CA3C569.4020309@gmail.com","threadId":"25289","inReplyTo":"4CA3BBD7.3090006@web.de","subject":"Re: [RFC PATCH 3/3] grep: add support for grepping in submodules","fromName":"Chris Packham","fromEmail":"judge.packham@gmail.com","sentAt":"2010-09-29T23:02:01Z","receivedAt":"2010-09-29T23:02:01Z","isPatch":true,"sender":{"key":"judge.packham@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155667?v=4"},"body":"On 29/09/10 15:21, Jens Lehmann wrote:\n> Nice start!\n> \n> \n> Am 29.09.2010 22:28, schrieb Chris Packham:\n>> When the --recurse-submodules option is given git grep will search in\n>> submodules as they are encountered.\n> \n> As \"git clone\" already introduced a \"--recursive\" option for\n> submodule recursion IMO \"--recursive\" should be used here too for\n> consistency. (Maybe you took the idea to use \"--recurse-submodules\"\n> from my \"git-checkout-recurse-submodules\" branch on github? But that\n> is only used there because I didn't get around to change it yet like\n> I did in the \"fetch-submodules-too\" branch).\n\nI actually started with --recursive and switched to\n--recurse-submodules. One thing with this is the standard grep\n--recursive option which may cause some confusion if people expect git\ngrep to behave like normal grep. I'll switch to using --recursive for\nnow until someone objects to the potential confusion.\n\nOne more thought on this that has been hanging around in my mind. I\nsometimes want to do something on all but one submodule, in this case\nwith grep I'm fairly likely to want to skip a linux repository because I\nalready know the thing I'm looking for is in userland. Maybe in the\nfuture we can make --recursive take an argument that allows us to\nspecify/restrict which submodules get included in the command invocation.\n\n> \n>> Signed-off-by: Chris Packham <judge.packham@gmail.com>\n>> ---\n>>  builtin/grep.c |   72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n>>  grep.h         |    1 +\n>>  2 files changed, 73 insertions(+), 0 deletions(-)\n>>\n>> diff --git a/builtin/grep.c b/builtin/grep.c\n>> index 8315ff0..c9befdc 100644\n>> --- a/builtin/grep.c\n>> +++ b/builtin/grep.c\n>> @@ -30,6 +30,9 @@ static char const * const grep_usage[] = {\n>>  \n>>  static int use_threads = 1;\n>>  \n>> +static int saved_argc;\n>> +static const char **saved_argv;\n>> +\n>>  #ifndef NO_PTHREADS\n>>  #define THREADS 8\n>>  static pthread_t threads[THREADS];\n>> @@ -585,6 +588,67 @@ static void run_pager(struct grep_opt *opt, const char *prefix)\n>>  \tfree(argv);\n>>  }\n>>  \n>> +static const char **create_sub_grep_argv(struct grep_opt *opt, const char *path)\n>> +{\n>> +\t#define NUM_ARGS 10\n>> +\tstruct strbuf buf = STRBUF_INIT;\n>> +\tconst char **argv;\n>> +\tint i = 0;\n>> +\n>> +\targv = xcalloc(NUM_ARGS, sizeof(const char *));\n>> +\targv[i++] = \"grep\";\n>> +\tstrbuf_addf(&buf, \"--submodule-prefix=\\\\\\\"%s\\\\\\\"\", path);\n>> +\t//argv[i++] = buf.buf;\n> \n> As C++ comments are not portable they have to be avoided, but I\n> assume this one here (and the unused \"saved_argc\" variable too) is\n> a hint that this code is not working as intended yet? ;-)\n\nYeah this is due to my test problem I mentioned in the cover email. When\nrun_command gets called it ends up invoking the installed git executable\nwhich doesn't understand my new option.\n\n> It seems you want to use strbuf_detach() here so that this argv[]\n> stays valid after the strbuf_release() at the end of this function.\n> And if I'm not missing something this would not work correctly in\n> the second recursion depth, as the new submodule prefix should\n> be the one given to this grep command concatenated with the current\n> submodule path.\n\nI'll look into strbuf_detatch. The tricky thing will be keeping track of\nwhat to free at the end of grep_submodule. I guess I can assume that it\nwill argv[1] is always going to be the dynamically allocated string.\n\n> \n>> +\tif (opt->linenum)\n>> +\t\targv[i++] = \"-n\";\n>> +\tif (opt->invert)\n>> +\t\targv[i++] = \"-v\";\n>> +\tif (opt->ignore_case)\n>> +\t\targv[i++] = \"-i\";\n>> +\tif (opt->count)\n>> +\t\targv[i++] = \"-c\";\n>> +\tif (opt->name_only)\n>> +\t\targv[i++] = \"-l\";\n>> +\n>> +\targv[i++] = saved_argv[0];\n>> +\targv[i++] = NULL;\n> \n> Hm, at a quick glance it might be much easier to copy argc & argv\n> in cmd_grep() before parse_options() starts manipulating it. Then\n> you would only have to change/add the \"--submodule-prefix\" option\n> as needed and would not have to deal with all possible grep option\n> combinations (and for example you don't pass the recurse option\n> yet, which would stop the recursion pretty soon).\n\nYeah this is the part I was struggling with a little. It would be easy\nto save argv before any option processing but I wondered if that would\nbe frowned upon as an overhead for non-submodule usages.\n\nI was thinking about doing something tricky with max-depth and\nrecursion. But maybe its better to keep it simple.\n\n>> +\n>> +\tstrbuf_release(&buf);\n>> +\treturn argv;\n>> +}\n>> +\n>> +static int grep_submodule(struct grep_opt *opt, const char *path)\n>> +{\n>> +\tstruct strbuf buf = STRBUF_INIT;\n>> +\tstruct child_process cp;\t\n>> +\tconst char **argv = create_sub_grep_argv(opt, path);\n>> +\tconst char *git_dir;\n>> +\tint hit = 0;\n>> +\n>> +\tstrbuf_addf(&buf, \"%s/.git\", path);\n>> +\tgit_dir = read_gitfile_gently(buf.buf);\n>> +\tif (!git_dir)\n>> +\t\tgit_dir = buf.buf;\n>> +\tif (!is_directory(git_dir)) {\n>> +\t\twarning(\"submodule %s has not been initialized\\n\", path);\n> \n> Having a submodule which is not checked out is perfectly fine, so\n> I don't think we want to issue a warning here.\n> \nOK I'll remove it.\n\n>> +\t\tgoto out_free;\n>> +\t}\n>> +\n>> +\tmemset(&cp, 0, sizeof(cp));\n>> +\tcp.argv = argv;\n>> +\tcp.env = local_repo_env;\n>> +\tcp.git_cmd = 1;\n>> +\tcp.no_stdin = 1;\n>> +\tcp.dir = path;\n>> +\tif (run_command(&cp) == 0)\n>> +\t\thit = 1;\n>> +out_free:\n>> +\tfree(argv);\n>> +\tstrbuf_release(&buf);\n>> +\treturn hit;\n>> +}\n>> +\n>>  static int grep_cache(struct grep_opt *opt, const char **paths, int cached)\n>>  {\n>>  \tint hit = 0;\n>> @@ -593,6 +657,10 @@ static int grep_cache(struct grep_opt *opt, const char **paths, int cached)\n>>  \n>>  \tfor (nr = 0; nr < active_nr; nr++) {\n>>  \t\tstruct cache_entry *ce = active_cache[nr];\n>> +\t\tif (S_ISGITLINK(ce->ce_mode) && opt->recurse_submodules) {\n>> +\t\t\thit |= grep_submodule(opt, ce->name);\n>> +\t\t\tcontinue;\n>> +\t\t}\n>>  \t\tif (!S_ISREG(ce->ce_mode))\n>>  \t\t\tcontinue;\n>>  \t\tif (!pathspec_matches(paths, ce->name, opt->max_depth))\n>> @@ -929,9 +997,13 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n>>  \t\t  PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, help_callback },\n>>  \t\tOPT_STRING(0, \"submodule-prefix\", &opt.submodule_prefix, \"DIR\",\n>>  \t\t\t\"prepend this to submodule path output\"),\n>> +\t\tOPT_BOOLEAN(0, \"recurse-submodules\", &opt.recurse_submodules,\n>> +\t\t\t\"recurse into submodules\"),\n>>  \t\tOPT_END()\n>>  \t};\n>>  \n>> +\tsaved_argc = argc;\n>> +\tsaved_argv = argv;\n>>  \t/*\n>>  \t * 'git grep -h', unlike 'git grep -h <pattern>', is a request\n>>  \t * to show usage information and exit.\n>> diff --git a/grep.h b/grep.h\n>> index d918da4..e3199c9 100644\n>> --- a/grep.h\n>> +++ b/grep.h\n>> @@ -102,6 +102,7 @@ struct grep_opt {\n>>  \tunsigned post_context;\n>>  \tunsigned last_shown;\n>>  \tint show_hunk_mark;\n>> +\tint recurse_submodules;\n>>  \tvoid *priv;\n>>  \n>>  \tvoid (*output)(struct grep_opt *opt, const void *data, size_t size);\n> \n"},{"id":"152106","messageId":"4CA3D01B.6060600@gmail.com","threadId":"25289","inReplyTo":"7v4od8ma0j.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC PATCH 3/3] grep: add support for grepping in submodules","fromName":"Chris Packham","fromEmail":"judge.packham@gmail.com","sentAt":"2010-09-29T23:47:39Z","receivedAt":"2010-09-29T23:47:39Z","isPatch":true,"sender":{"key":"judge.packham@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155667?v=4"},"body":"On 29/09/10 15:59, Junio C Hamano wrote:\n> Jens Lehmann <Jens.Lehmann@web.de> writes:\n> \n>> Hm, at a quick glance it might be much easier to copy argc & argv\n>> in cmd_grep() before parse_options() starts manipulating it.\n> \n> Yes, I think that is a much saner direction to go.  Otherwise, you would\n> need to unparse grep boolean expressions as well.\n\nI've got some of that working in my quest to not use saved_argv[0]. If\nwe follow some of your points below about handling ref names then\nrebuilding the args actually starts to make life easier. I guess the\nsame is true for just making these manipulations on the grep_opts. The\nonly thing that is really clear is that saving the original argv is not\ngoing to help us.\n\n> A few more things to think about.\n> \n> 1. What does this mean:\n> \n>     $ git grep --recursive -e frotz master next\n> \n> It recurses into the submodule commits recorded in 'master' and 'next'\n> commits in the superproject, right?\n> \n> How do the lines output from the above look like?  From the superproject,\n> we will get lines like these:\n> \n>     master:t/README:  test_description='xxx test (option --frotz)\n>     master:t/README:  and tries to run git-ls-files with option --frotz.'\n> \n> What if we have a submodule at git-gui in the superproject, and its README\n> has string frotz in it?  Should we label the submodule commit we find in\n> 'master' of superproject as 'master' as well, even if it is not at the tip\n> of 'master' branch of the submodule?  Or do we get abbreviated hexadecimal\n> SHA-1 name?  IOW, would we see:\n> \n>     master:git-gui/README: git-gui also knows frotz\n> \n> or\n> \n>     deadbeef:git-gui/README: git-gui also knows frotz\n> \n> where \"deadbeaf....\" is what \"git rev-parse master:git-gui\" would give us\n> in the superproject?\n> \n> I tend to think the former is preferable, but then most likely you would\n> need to pass not just submodule-prefix but the original ref name\n> (i.e. 'master') you started from down to the recursive one.\n\nPassing the ref name is doable. There is a little potential for\nconfusion between who's \"master\" that actually is (the same confusion is\nin theory possible with an abbreviated SHA-1). Maybe we should color the\nsubmodule ref's differently\n\n> 2. Now how would this work with pathspecs?\n> \n>     $ git grep --recursive -e frotz -- dir/\n> \n> This should look for the string in the named directory in the superproject\n> and if there are submodules in that directory, recurse into them as well,\n> right?\n> \n> What pathspec, if any, will be in effect when we recurse into the\n> submodule at dir/sub?  Limiting to dir/ feels wrong, no?\n> \n> 3. Corollary.\n> \n> Is it reasonable to expect that we will look into all shell scripts, both\n> in the superproject and in submodules, with this:\n> \n>     $ git grep --recursive -e frotz -- '*.sh'\n> \n> Oops?  What happened to the \"we restrict the recursion using pathspec, and\n> we do not pass down the pathspec\" that was suggested in 2.?\n> \n\nThis is a bit of a grey area, I'm not sure what is the sensible thing to do.\n\nMaybe we could pop a directory level per recursion e.g.\n  user enters 'dir/sub/subsub/*.sh'\n  first level recursion is passed 'sub/subsub/*.sh'\n  second level recursion is passed 'subsub/*.sh'\n  subsequent levels of recursion are passed '*.sh'\n\nBut that's not quite what the user thought they asked for (i.e. they\nwill end up with dir/sub/subsub/subsubsub/file.sh).\n\nOr we could alter the behaviour based on whether their original pathspec\nhad an explicit trailing /.\n"},{"id":"152109","messageId":"AANLkTik3bvJneak9gjG8O1A=LZeivPkFc6MF5_T7nbWo@mail.gmail.com","threadId":"25289","inReplyTo":"1285792134-26339-3-git-send-email-judge.packham@gmail.com","subject":"Re: [RFC PATCH 2/3] grep: prepare grep for submodules","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-09-30T01:10:11Z","receivedAt":"2010-09-30T01:10:11Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Sep 30, 2010 at 6:28 AM, Chris Packham <judge.packham@gmail.com> wrote:\n> Add --submodule-prefix option to pass to subprocess grep invocations. The\n> prefix is then used when outputting the results.\n\nI haven't followed the recursive submodule support in Git lately. But\nI think --submodule-prefix is unnecessary. I would imagine you need to\nadd --submodule-prefix to a lot more commands as they support recusive\nsubmodule search. There is a corner case in Git's prefix setup that we\ncan utilize to avoid the new option.\n\nIf you do this at the superproject repo:\n\n$ GIT_DIR=path/to/submodule/.git GIT_WORK_TREE=path/to/submodule git grep blah\n\nI would expect that it shows the result correctly (i.e. all files\nprefixed by \"path/to/submodule\"), but it does not right now. If you\nmake that setup work, then you don't need --submodule-prefix, just set\nGIT_DIR/GIT_WORK_TREE properly and run \"git grep\".\n\nYou can make setup_explicit_git_dir() realize that situation (current\nworking directory outside $GIT_WORK_TREE), then calculate and save the\nsubmodule prefix in startup_info struct. Then \"git grep\" or any\ncommands can just read startup_info to find out the submodule prefix.\n-- \nDuy\n"},{"id":"152122","messageId":"4CA47005.7030102@web.de","threadId":"25289","inReplyTo":"4CA3D01B.6060600@gmail.com","subject":"Re: [RFC PATCH 3/3] grep: add support for grepping in submodules","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2010-09-30T11:09:57Z","receivedAt":"2010-09-30T11:09:57Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 30.09.2010 01:47, schrieb Chris Packham:\n> On 29/09/10 15:59, Junio C Hamano wrote:\n>> A few more things to think about.\n>>\n>> 1. What does this mean:\n>>\n>>     $ git grep --recursive -e frotz master next\n>>\n>> It recurses into the submodule commits recorded in 'master' and 'next'\n>> commits in the superproject, right?\n>>\n>> How do the lines output from the above look like?  From the superproject,\n>> we will get lines like these:\n>>\n>>     master:t/README:  test_description='xxx test (option --frotz)\n>>     master:t/README:  and tries to run git-ls-files with option --frotz.'\n>>\n>> What if we have a submodule at git-gui in the superproject, and its README\n>> has string frotz in it?  Should we label the submodule commit we find in\n>> 'master' of superproject as 'master' as well, even if it is not at the tip\n>> of 'master' branch of the submodule?  Or do we get abbreviated hexadecimal\n>> SHA-1 name?  IOW, would we see:\n>>\n>>     master:git-gui/README: git-gui also knows frotz\n>>\n>> or\n>>\n>>     deadbeef:git-gui/README: git-gui also knows frotz\n>>\n>> where \"deadbeaf....\" is what \"git rev-parse master:git-gui\" would give us\n>> in the superproject?\n>>\n>> I tend to think the former is preferable, but then most likely you would\n>> need to pass not just submodule-prefix but the original ref name\n>> (i.e. 'master') you started from down to the recursive one.\n\nMe too thinks that as you grep from inside the superproject it makes\nmore sense to use the ref name used there and not the SHA-1 from the\nsubmodule.\n\n> Passing the ref name is doable. There is a little potential for\n> confusion between who's \"master\" that actually is (the same confusion is\n> in theory possible with an abbreviated SHA-1). Maybe we should color the\n> submodule ref's differently\n\nHmm, showing somehow that the grep result is from inside a submodule\ncould be helpful. But using something like the following line seems a\nbit like overkill, so coloring might be a good alternative:\n\n     master/deadbeef:git-gui/README: git-gui also knows frotz\n\nBut I don't have a strong opinion here.\n\n\n>> 2. Now how would this work with pathspecs?\n>>\n>>     $ git grep --recursive -e frotz -- dir/\n>>\n>> This should look for the string in the named directory in the superproject\n>> and if there are submodules in that directory, recurse into them as well,\n>> right?\n>>\n>> What pathspec, if any, will be in effect when we recurse into the\n>> submodule at dir/sub?  Limiting to dir/ feels wrong, no?\n>>\n>> 3. Corollary.\n>>\n>> Is it reasonable to expect that we will look into all shell scripts, both\n>> in the superproject and in submodules, with this:\n>>\n>>     $ git grep --recursive -e frotz -- '*.sh'\n>>\n>> Oops?  What happened to the \"we restrict the recursion using pathspec, and\n>> we do not pass down the pathspec\" that was suggested in 2.?\n>>\n> \n> This is a bit of a grey area, I'm not sure what is the sensible thing to do.\n> \n> Maybe we could pop a directory level per recursion e.g.\n>   user enters 'dir/sub/subsub/*.sh'\n>   first level recursion is passed 'sub/subsub/*.sh'\n>   second level recursion is passed 'subsub/*.sh'\n>   subsequent levels of recursion are passed '*.sh'\n> \n> But that's not quite what the user thought they asked for (i.e. they\n> will end up with dir/sub/subsub/subsubsub/file.sh).\n> \n> Or we could alter the behaviour based on whether their original pathspec\n> had an explicit trailing /.\n\nI think we'll have to manipulate the pathspecs so we properly translate\ntheir meaning into the context of the submodule. What about this: If\nthey point outside the submodule, they must be dropped. If they contain\ndirectories, the prefix part has to be stripped from the beginning.\nExamples for submodule 'dir/sub':\n\n  * 'dir/' and 'dir/sub2/' get dropped as they point outside\n  * '*.sh' should just be passed on\n  * 'dir/sub/foo/*.sh' would become 'foo/*.sh'\n  * 'dir/sub/' gets dropped too (as the result of stripping the\n    prefix is '')\n\nThat should lead to the expected behavior.\n"},{"id":"152123","messageId":"4CA4738A.9040503@web.de","threadId":"25289","inReplyTo":"4CA3C569.4020309@gmail.com","subject":"Re: [RFC PATCH 3/3] grep: add support for grepping in submodules","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2010-09-30T11:24:58Z","receivedAt":"2010-09-30T11:24:58Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 30.09.2010 01:02, schrieb Chris Packham:\n> I actually started with --recursive and switched to\n> --recurse-submodules. One thing with this is the standard grep\n> --recursive option which may cause some confusion if people expect git\n> grep to behave like normal grep.\n\nGuess how I came to use \"--recurse-submodules\" for recursive checkout\nin the first place ;-) But the fact that clone already uses it weighs\nstronger here I suppose ...\n\n\n> One more thought on this that has been hanging around in my mind. I\n> sometimes want to do something on all but one submodule, in this case\n> with grep I'm fairly likely to want to skip a linux repository because I\n> already know the thing I'm looking for is in userland. Maybe in the\n> future we can make --recursive take an argument that allows us to\n> specify/restrict which submodules get included in the command invocation.\n\nHmm, maybe adding an option to \"git grep\" to exclude a pathspec would\nmake more sense?\n\n\n>> It seems you want to use strbuf_detach() here so that this argv[]\n>> stays valid after the strbuf_release() at the end of this function.\n\n> I'll look into strbuf_detatch. The tricky thing will be keeping track of\n> what to free at the end of grep_submodule.\n\nRight, but if you push the strbuf operations into one of the calling\nfunctions you can achieve that more easily.\n\n\n> Yeah this is the part I was struggling with a little. It would be easy\n> to save argv before any option processing but I wondered if that would\n> be frowned upon as an overhead for non-submodule usages.\n\nYup, but as you are only copying a pointer array the overhead is very\nsmall. And if the code gets much easier that way (as I would expect)\nthat price is well paid.\n"},{"id":"152124","messageId":"4CA47478.6030207@viscovery.net","threadId":"25289","inReplyTo":"7v4od8ma0j.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC PATCH 3/3] grep: add support for grepping in submodules","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2010-09-30T11:28:56Z","receivedAt":"2010-09-30T11:28:56Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 9/30/2010 0:59, schrieb Junio C Hamano:\n> A few more things to think about.\n> \n> 1. What does this mean:\n> \n>     $ git grep --recursive -e frotz master next\n> \n> It recurses into the submodule commits recorded in 'master' and 'next'\n> commits in the superproject, right?\n\nAnd what does it mean if you add --cached? Does it grep in the index of\nthe submodules, or does it grep in the rev of the submodule that is\nrecorded in the index of the supermodule?\n\n-- Hannes\n"},{"id":"152161","messageId":"4CA4A799.2060302@web.de","threadId":"25289","inReplyTo":"4CA47478.6030207@viscovery.net","subject":"Re: [RFC PATCH 3/3] grep: add support for grepping in submodules","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2010-09-30T15:07:05Z","receivedAt":"2010-09-30T15:07:05Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 30.09.2010 13:28, schrieb Johannes Sixt:\n> Am 9/30/2010 0:59, schrieb Junio C Hamano:\n>> A few more things to think about.\n>>\n>> 1. What does this mean:\n>>\n>>     $ git grep --recursive -e frotz master next\n>>\n>> It recurses into the submodule commits recorded in 'master' and 'next'\n>> commits in the superproject, right?\n> \n> And what does it mean if you add --cached? Does it grep in the index of\n> the submodules, or does it grep in the rev of the submodule that is\n> recorded in the index of the supermodule?\n\nHmm, as you told grep to use the index of the superproject it should\nuse the rev of the submodule that is recorded in the index of the\nsuperproject. Thus the \"--cached\" should be removed from and the\nappropriate rev must be added to the arguments of the grep forked in\nthe submodule.\n"},{"id":"152173","messageId":"4CA4BF62.3030905@gmail.com","threadId":"25289","inReplyTo":"4CA4738A.9040503@web.de","subject":"Re: [RFC PATCH 3/3] grep: add support for grepping in submodules","fromName":"Chris Packham","fromEmail":"judge.packham@gmail.com","sentAt":"2010-09-30T16:48:34Z","receivedAt":"2010-09-30T16:48:34Z","isPatch":true,"sender":{"key":"judge.packham@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155667?v=4"},"body":"On 30/09/10 04:24, Jens Lehmann wrote:\n> Am 30.09.2010 01:02, schrieb Chris Packham:\n>> Yeah this is the part I was struggling with a little. It would be easy\n>> to save argv before any option processing but I wondered if that would\n>> be frowned upon as an overhead for non-submodule usages.\n> \n> Yup, but as you are only copying a pointer array the overhead is very\n> small. And if the code gets much easier that way (as I would expect)\n> that price is well paid.\n\nWith the talk of translating superproject --index into submodule SHA-1,\nre-formatting pathspecs and passing the superproject ref-name. It sounds\nlike making a copy of argv is not going to be that useful. If we did\ncopy it we'd have to scan it for the things we don't want passed to the\nsubmodule grep.\n"},{"id":"152184","messageId":"4CA4D820.5040100@gmail.com","threadId":"25289","inReplyTo":"AANLkTik3bvJneak9gjG8O1A=LZeivPkFc6MF5_T7nbWo@mail.gmail.com","subject":"Re: [RFC PATCH 2/3] grep: prepare grep for submodules","fromName":"Chris Packham","fromEmail":"judge.packham@gmail.com","sentAt":"2010-09-30T18:34:08Z","receivedAt":"2010-09-30T18:34:08Z","isPatch":true,"sender":{"key":"judge.packham@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155667?v=4"},"body":"On 29/09/10 18:10, Nguyen Thai Ngoc Duy wrote:\n> On Thu, Sep 30, 2010 at 6:28 AM, Chris Packham <judge.packham@gmail.com> wrote:\n>> Add --submodule-prefix option to pass to subprocess grep invocations. The\n>> prefix is then used when outputting the results.\n> \n> I haven't followed the recursive submodule support in Git lately. But\n> I think --submodule-prefix is unnecessary. I would imagine you need to\n> add --submodule-prefix to a lot more commands as they support recusive\n> submodule search. There is a corner case in Git's prefix setup that we\n> can utilize to avoid the new option.\n> \n> If you do this at the superproject repo:\n> \n> $ GIT_DIR=path/to/submodule/.git GIT_WORK_TREE=path/to/submodule git grep blah\n> \n> I would expect that it shows the result correctly (i.e. all files\n> prefixed by \"path/to/submodule\"), but it does not right now. If you\n> make that setup work, then you don't need --submodule-prefix, just set\n> GIT_DIR/GIT_WORK_TREE properly and run \"git grep\".\n> \n> You can make setup_explicit_git_dir() realize that situation (current\n> working directory outside $GIT_WORK_TREE), then calculate and save the\n> submodule prefix in startup_info struct. Then \"git grep\" or any\n> commands can just read startup_info to find out the submodule prefix.\n\nHere's my first naive attempt at implementing what you describe. Needs\ntests, more comments, sign-off etc.\n\nOne situation that could be handled better is when the cwd is a\nsubdirectory of the specified worktree. At the moment this ends up\ngiving the full path to the worktree, the output would look much nicer\nif it gave the relative path (e.g. ../../).\n\n---8<---\nFrom: Chris Packham <judge.packham@gmail.com>\nDate: Thu, 30 Sep 2010 11:19:29 -0700\nSubject: [RFC PATCH] save the work tree prefix in startup_info\n\nThis is the relative path between the cwd and the worktree or the\nabsolute path of the worktree if the worktree is not a subdirectory\nof the worktree.\n---\n cache.h |    1 +\n dir.c   |   26 ++++++++++++++++++++++++++\n dir.h   |    1 +\n setup.c |    4 ++++\n 4 files changed, 32 insertions(+), 0 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex e1d3ffd..f320e78 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1111,6 +1111,7 @@ const char *split_cmdline_strerror(int cmdline_errno);\n /* git.c */\n struct startup_info {\n \tint have_repository;\n+\tconst char *prefix;\n };\n extern struct startup_info *startup_info;\n\ndiff --git a/dir.c b/dir.c\nindex 58ec1a1..2148730 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1036,6 +1036,32 @@ char *get_relative_cwd(char *buffer, int size,\nconst char *dir)\n \t}\n }\n\n+char *get_relative_wt(char *buffer, int size, const char *dir)\n+{\n+\tchar *cwd = buffer;\n+\t\n+\tif (!dir)\n+\t\treturn NULL;\n+\tif (!getcwd(buffer, size))\n+\t\tdie_errno(\"can't find the current directory\");\n+\tif (!is_absolute_path(dir))\n+\t\tdir = make_absolute_path(dir);\n+\tif (strstr(dir, cwd)) {\n+\t\tdir += strlen(cwd);\n+\t\tswitch(*dir){\n+\t\tcase '\\0':\n+\t\t\treturn NULL;\n+\t\tcase '/':\n+\t\t\tdir++;\n+\t\t\tbreak;\n+\t\tdefault:\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\tstrncpy(buffer, dir, size);\n+\treturn buffer;\n+}\n+\n int is_inside_dir(const char *dir)\n {\n \tchar buffer[PATH_MAX];\ndiff --git a/dir.h b/dir.h\nindex b3e2104..d3c161f 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -81,6 +81,7 @@ extern void add_exclude(const char *string, const char\n*base,\n extern int file_exists(const char *);\n\n extern char *get_relative_cwd(char *buffer, int size, const char *dir);\n+extern char *get_relative_wt(char *buffer, int size, const char *dir);\n extern int is_inside_dir(const char *dir);\n\n static inline int is_dot_or_dotdot(const char *name)\ndiff --git a/setup.c b/setup.c\nindex a3b76de..bd9d9fd 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -317,6 +317,7 @@ static const char *setup_explicit_git_dir(const char\n*gitdirenv,\n \t\t\t\tconst char *work_tree_env, int *nongit_ok)\n {\n \tstatic char buffer[1024 + 1];\n+\tstatic char wtbuf[1024 + 1];\n \tconst char *retval;\n\n \tif (PATH_MAX - 40 < strlen(gitdirenv))\n@@ -337,6 +338,9 @@ static const char *setup_explicit_git_dir(const char\n*gitdirenv,\n \t}\n \tif (check_repository_format_gently(nongit_ok))\n \t\treturn NULL;\n+\n+\tstartup_info->prefix=get_relative_wt(wtbuf, sizeof(wtbuf) - 1,\n+\t\t\tget_git_work_tree());\n \tretval = get_relative_cwd(buffer, sizeof(buffer) - 1,\n \t\t\tget_git_work_tree());\n \tif (!retval || !*retval)\n-- \n1.7.3.1\n"},{"id":"152186","messageId":"20100930185937.GA15457@book.hvoigt.net","threadId":"25289","inReplyTo":"4CA3C569.4020309@gmail.com","subject":"Re: Re: [RFC PATCH 3/3] grep: add support for grepping in submodules","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2010-09-30T18:59:37Z","receivedAt":"2010-09-30T18:59:37Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Hi,\n\nOn Wed, Sep 29, 2010 at 04:02:01PM -0700, Chris Packham wrote:\n> On 29/09/10 15:21, Jens Lehmann wrote:\n> > Am 29.09.2010 22:28, schrieb Chris Packham:\n> >> When the --recurse-submodules option is given git grep will search in\n> >> submodules as they are encountered.\n> > \n> > As \"git clone\" already introduced a \"--recursive\" option for\n> > submodule recursion IMO \"--recursive\" should be used here too for\n> > consistency. (Maybe you took the idea to use \"--recurse-submodules\"\n> > from my \"git-checkout-recurse-submodules\" branch on github? But that\n> > is only used there because I didn't get around to change it yet like\n> > I did in the \"fetch-submodules-too\" branch).\n> \n> I actually started with --recursive and switched to\n> --recurse-submodules. One thing with this is the standard grep\n> --recursive option which may cause some confusion if people expect git\n> grep to behave like normal grep. I'll switch to using --recursive for\n> now until someone objects to the potential confusion.\n\nHow about dropping the option all together and making grep search all\npopulated submodules by default and maybe have an option to turn it off.\nSince git grep is searching recursive by default this would be what I\nwould expect as a user. Are there other reasons to turn off the search\nin submodules than the potential runtime penalty because of forks? \n\n> One more thought on this that has been hanging around in my mind. I\n> sometimes want to do something on all but one submodule, in this case\n> with grep I'm fairly likely to want to skip a linux repository because I\n> already know the thing I'm looking for is in userland. Maybe in the\n> future we can make --recursive take an argument that allows us to\n> specify/restrict which submodules get included in the command invocation.\n\nThinking about this how about not providing a disable submodule\nrecursion option at all? Just provide an --exclude option and let it be\nused transparently for both normal subfolders and submodules?\n\nCheers Heiko\n"},{"id":"152189","messageId":"4CA4E980.1010100@web.de","threadId":"25289","inReplyTo":"20100930185937.GA15457@book.hvoigt.net","subject":"Re: [RFC PATCH 3/3] grep: add support for grepping in submodules","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2010-09-30T19:48:16Z","receivedAt":"2010-09-30T19:48:16Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 30.09.2010 20:59, schrieb Heiko Voigt:\n> How about dropping the option all together and making grep search all\n> populated submodules by default and maybe have an option to turn it off.\n\nAnd that option might be called \"--no-recursive\"? :-)\n\nBut when we add an config setting to control the default behavior\nlater (which we had to do for all submodule recursion features so far\nwhere we changed the default to recursion) we'll need the \"--recursive\"\noption again anyway to be able to override the config setting. So I\nvote for just leaving the option as it is for now, and we can discuss\nthe proper default as we go along (And in case of grep I have not made\nup my mind as to what a sane default would be, personally I'm fine with\nhaving to use the \"--recursive\" option when I want recursion, but I\nwon't object to making it the default either).\n\n\n> Since git grep is searching recursive by default this would be what I\n> would expect as a user. Are there other reasons to turn off the search\n> in submodules than the potential runtime penalty because of forks? \n\nThe runtime penalty is a *very* important aspect, as we have some\nsubmodule users who have put huge trees into submodules especially to\navoid the performance penalties (see the discussions for recursive\ndiff and status). So if we change the default, we will have to\nprovide an config option for that.\n"},{"id":"152237","messageId":"AANLkTikH+sd2kPAraGSTB-ik-Toz3s2nTLoLVOj86oSm@mail.gmail.com","threadId":"25289","inReplyTo":"4CA4D820.5040100@gmail.com","subject":"Re: [RFC PATCH 2/3] grep: prepare grep for submodules","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-10-01T14:37:55Z","receivedAt":"2010-10-01T14:37:55Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On 10/1/10, Chris Packham <judge.packham@gmail.com> wrote:\n>  > You can make setup_explicit_git_dir() realize that situation (current\n>  > working directory outside $GIT_WORK_TREE), then calculate and save the\n>  > submodule prefix in startup_info struct. Then \"git grep\" or any\n>  > commands can just read startup_info to find out the submodule prefix.\n>\n>\n> Here's my first naive attempt at implementing what you describe. Needs\n>  tests, more comments, sign-off etc.\n\nThanks.\n\n>  One situation that could be handled better is when the cwd is a\n>  subdirectory of the specified worktree. At the moment this ends up\n>  giving the full path to the worktree, the output would look much nicer\n>  if it gave the relative path (e.g. ../../).\n\nHmm.. if cwd is inside a worktree, prefix (the 3rd parameter in\ncmd_grep) should be correctly set and \"git grep\" should also show\nrelative path. Or are you talking about another command?\n\n>\n>  ---8<---\n>  From: Chris Packham <judge.packham@gmail.com>\n>  Date: Thu, 30 Sep 2010 11:19:29 -0700\n>  Subject: [RFC PATCH] save the work tree prefix in startup_info\n>\n>  This is the relative path between the cwd and the worktree or the\n>  absolute path of the worktree if the worktree is not a subdirectory\n>  of the worktree.\n>  ---\n>   cache.h |    1 +\n>   dir.c   |   26 ++++++++++++++++++++++++++\n>   dir.h   |    1 +\n>   setup.c |    4 ++++\n>   4 files changed, 32 insertions(+), 0 deletions(-)\n>\n>  diff --git a/cache.h b/cache.h\n>  index e1d3ffd..f320e78 100644\n>  --- a/cache.h\n>  +++ b/cache.h\n>  @@ -1111,6 +1111,7 @@ const char *split_cmdline_strerror(int cmdline_errno);\n>   /* git.c */\n>   struct startup_info {\n>         int have_repository;\n>  +       const char *prefix;\n\nYou should use another name here to avoid confusion with the current\nprefix, relative to worktree toplevel directory. I'm thinking of\nouter_prefix or cwd_prefix, but I'm usually bad at naming.\n\n>   };\n>   extern struct startup_info *startup_info;\n>\n>  diff --git a/dir.c b/dir.c\n>  index 58ec1a1..2148730 100644\n>  --- a/dir.c\n>  +++ b/dir.c\n>  @@ -1036,6 +1036,32 @@ char *get_relative_cwd(char *buffer, int size,\n>  const char *dir)\n>         }\n>   }\n>\n>  +char *get_relative_wt(char *buffer, int size, const char *dir)\n>  +{\n>  +       char *cwd = buffer;\n>  +\n>  +       if (!dir)\n>  +               return NULL;\n>  +       if (!getcwd(buffer, size))\n>  +               die_errno(\"can't find the current directory\");\n>  +       if (!is_absolute_path(dir))\n>  +               dir = make_absolute_path(dir);\n>  +       if (strstr(dir, cwd)) {\n\nWhy not strncmp?\n\n>  +               dir += strlen(cwd);\n>  +               switch(*dir){\n>  +               case '\\0':\n>  +                       return NULL;\n>  +               case '/':\n>  +                       dir++;\n>  +                       break;\n\nYeah.\n\n>  +               default:\n>  +                       break;\n\nShould we properly handle relative path that includes \"..\" here too?\n\n>  +               }\n>  +       }\n>  +       strncpy(buffer, dir, size);\n\nSo if \"cwd\" is inside \"dir\", an absolute \"dir\" is returned? That does\nnot look like a prefix to me.\n\n>  +       return buffer;\n>  +}\n>  +\n>   int is_inside_dir(const char *dir)\n>   {\n>         char buffer[PATH_MAX];\n>  diff --git a/dir.h b/dir.h\n>  index b3e2104..d3c161f 100644\n>  --- a/dir.h\n>  +++ b/dir.h\n>  @@ -81,6 +81,7 @@ extern void add_exclude(const char *string, const char\n>  *base,\n>   extern int file_exists(const char *);\n>\n>   extern char *get_relative_cwd(char *buffer, int size, const char *dir);\n>  +extern char *get_relative_wt(char *buffer, int size, const char *dir);\n>   extern int is_inside_dir(const char *dir);\n>\n>   static inline int is_dot_or_dotdot(const char *name)\n>  diff --git a/setup.c b/setup.c\n>  index a3b76de..bd9d9fd 100644\n>  --- a/setup.c\n>  +++ b/setup.c\n>  @@ -317,6 +317,7 @@ static const char *setup_explicit_git_dir(const char\n>  *gitdirenv,\n>                                 const char *work_tree_env, int *nongit_ok)\n>   {\n>         static char buffer[1024 + 1];\n>  +       static char wtbuf[1024 + 1];\n>         const char *retval;\n>\n>         if (PATH_MAX - 40 < strlen(gitdirenv))\n>  @@ -337,6 +338,9 @@ static const char *setup_explicit_git_dir(const char\n>  *gitdirenv,\n>         }\n>         if (check_repository_format_gently(nongit_ok))\n>                 return NULL;\n>  +\n>  +       startup_info->prefix=get_relative_wt(wtbuf, sizeof(wtbuf) - 1,\n>  +                       get_git_work_tree());\n>         retval = get_relative_cwd(buffer, sizeof(buffer) - 1,\n>                         get_git_work_tree());\n>         if (!retval || !*retval)\n>\n> --\n>  1.7.3.1\n>\n>\n\n\n-- \nDuy\n"},{"id":"152245","messageId":"4CA60BB5.4080202@gmail.com","threadId":"25289","inReplyTo":"AANLkTikH+sd2kPAraGSTB-ik-Toz3s2nTLoLVOj86oSm@mail.gmail.com","subject":"Re: [RFC PATCH 2/3] grep: prepare grep for submodules","fromName":"Chris Packham","fromEmail":"judge.packham@gmail.com","sentAt":"2010-10-01T16:26:29Z","receivedAt":"2010-10-01T16:26:29Z","isPatch":true,"sender":{"key":"judge.packham@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155667?v=4"},"body":"On 01/10/10 07:37, Nguyen Thai Ngoc Duy wrote:\n> On 10/1/10, Chris Packham <judge.packham@gmail.com> wrote:\n>>  > You can make setup_explicit_git_dir() realize that situation (current\n>>  > working directory outside $GIT_WORK_TREE), then calculate and save the\n>>  > submodule prefix in startup_info struct. Then \"git grep\" or any\n>>  > commands can just read startup_info to find out the submodule prefix.\n>>\n>>\n>> Here's my first naive attempt at implementing what you describe. Needs\n>>  tests, more comments, sign-off etc.\n> \n> Thanks.\n> \n>>  One situation that could be handled better is when the cwd is a\n>>  subdirectory of the specified worktree. At the moment this ends up\n>>  giving the full path to the worktree, the output would look much nicer\n>>  if it gave the relative path (e.g. ../../).\n> \n> Hmm.. if cwd is inside a worktree, prefix (the 3rd parameter in\n> cmd_grep) should be correctly set and \"git grep\" should also show\n> relative path. Or are you talking about another command?\n\nI was testing this with grep but also with my submodule changes. I\nshould probably move this to its own topic branch and get it working\nthen rebase my grep changes on top of it.\n\n>>\n>>  ---8<---\n>>  From: Chris Packham <judge.packham@gmail.com>\n>>  Date: Thu, 30 Sep 2010 11:19:29 -0700\n>>  Subject: [RFC PATCH] save the work tree prefix in startup_info\n>>\n>>  This is the relative path between the cwd and the worktree or the\n>>  absolute path of the worktree if the worktree is not a subdirectory\n>>  of the worktree.\n>>  ---\n>>   cache.h |    1 +\n>>   dir.c   |   26 ++++++++++++++++++++++++++\n>>   dir.h   |    1 +\n>>   setup.c |    4 ++++\n>>   4 files changed, 32 insertions(+), 0 deletions(-)\n>>\n>>  diff --git a/cache.h b/cache.h\n>>  index e1d3ffd..f320e78 100644\n>>  --- a/cache.h\n>>  +++ b/cache.h\n>>  @@ -1111,6 +1111,7 @@ const char *split_cmdline_strerror(int cmdline_errno);\n>>   /* git.c */\n>>   struct startup_info {\n>>         int have_repository;\n>>  +       const char *prefix;\n> \n> You should use another name here to avoid confusion with the current\n> prefix, relative to worktree toplevel directory. I'm thinking of\n> outer_prefix or cwd_prefix, but I'm usually bad at naming.\n\nI couldn't come up with a better name either, that’s why I used\n\"prefix\". \"cwd_prefix\" seems sensible enough to me.\n\n>>   };\n>>   extern struct startup_info *startup_info;\n>>\n>>  diff --git a/dir.c b/dir.c\n>>  index 58ec1a1..2148730 100644\n>>  --- a/dir.c\n>>  +++ b/dir.c\n>>  @@ -1036,6 +1036,32 @@ char *get_relative_cwd(char *buffer, int size,\n>>  const char *dir)\n>>         }\n>>   }\n>>\n>>  +char *get_relative_wt(char *buffer, int size, const char *dir)\n>>  +{\n>>  +       char *cwd = buffer;\n>>  +\n>>  +       if (!dir)\n>>  +               return NULL;\n>>  +       if (!getcwd(buffer, size))\n>>  +               die_errno(\"can't find the current directory\");\n>>  +       if (!is_absolute_path(dir))\n>>  +               dir = make_absolute_path(dir);\n>>  +       if (strstr(dir, cwd)) {\n> \n> Why not strncmp?\n\nI actually tried strcmp first, expecting to get a number that I can\nincrement dir by. What I actually got was -1, maybe I just screwed up\nthe order of dir and cwd. I'll look into it.\n\n> \n>>  +               dir += strlen(cwd);\n>>  +               switch(*dir){\n>>  +               case '\\0':\n>>  +                       return NULL;\n>>  +               case '/':\n>>  +                       dir++;\n>>  +                       break;\n> \n> Yeah.\n> \n>>  +               default:\n>>  +                       break;\n> \n> Should we properly handle relative path that includes \"..\" here too? \n\nBy now dir and cwd are both absolute paths. So I dn't think there is\nanything to handle\n\n>>  +               }\n>>  +       }\n>>  +       strncpy(buffer, dir, size);\n> \n> So if \"cwd\" is inside \"dir\", an absolute \"dir\" is returned? That does\n> not look like a prefix to me.\n\nThat is a problem. Maybe I should be returning NULL in that case and let\nthe existing code handle the cwd inside dir case. I think if I wrote\nsome tests first I could see the various permutations better.\n\n>>  +       return buffer;\n>>  +}\n>>  +\n>>   int is_inside_dir(const char *dir)\n>>   {\n>>         char buffer[PATH_MAX];\n>>  diff --git a/dir.h b/dir.h\n>>  index b3e2104..d3c161f 100644\n>>  --- a/dir.h\n>>  +++ b/dir.h\n>>  @@ -81,6 +81,7 @@ extern void add_exclude(const char *string, const char\n>>  *base,\n>>   extern int file_exists(const char *);\n>>\n>>   extern char *get_relative_cwd(char *buffer, int size, const char *dir);\n>>  +extern char *get_relative_wt(char *buffer, int size, const char *dir);\n>>   extern int is_inside_dir(const char *dir);\n>>\n>>   static inline int is_dot_or_dotdot(const char *name)\n>>  diff --git a/setup.c b/setup.c\n>>  index a3b76de..bd9d9fd 100644\n>>  --- a/setup.c\n>>  +++ b/setup.c\n>>  @@ -317,6 +317,7 @@ static const char *setup_explicit_git_dir(const char\n>>  *gitdirenv,\n>>                                 const char *work_tree_env, int *nongit_ok)\n>>   {\n>>         static char buffer[1024 + 1];\n>>  +       static char wtbuf[1024 + 1];\n>>         const char *retval;\n>>\n>>         if (PATH_MAX - 40 < strlen(gitdirenv))\n>>  @@ -337,6 +338,9 @@ static const char *setup_explicit_git_dir(const char\n>>  *gitdirenv,\n>>         }\n>>         if (check_repository_format_gently(nongit_ok))\n>>                 return NULL;\n>>  +\n>>  +       startup_info->prefix=get_relative_wt(wtbuf, sizeof(wtbuf) - 1,\n>>  +                       get_git_work_tree());\n>>         retval = get_relative_cwd(buffer, sizeof(buffer) - 1,\n>>                         get_git_work_tree());\n>>         if (!retval || !*retval)\n>>\n>> --\n>>  1.7.3.1\n>>\n>>\n> \n> \n"}]}