[RFC PATCH 0/3] grep: submodule support

[RFC PATCH 0/3] grep: submodule support

[Chris Packham]

This patch series is my initial attempt to add submodule awareness to git grep.
It's also the first time I've played with the git C code so expect bugs. The
patches are based off apply on Jens Lehmann's 'enhance_git_for_submodules'
branch in git://github.com/jlehmann/git-submod-enhancements.git

The first patch adds some basic tests for grep with submodules. There is
probably some overlap with other grep tests so I'll have a more in-depth look
at what is needed later. I have a problem with the tests that when I invoke
'git grep' using run_command I actually end up using the installed git which
doesn't understand my new --submodule-prefix option.

The 2nd patch just adds a --submodule-prefix option so that I can prepend some
text to the output from the sub processes.

The 3rd patch is the main implementation. Currently I rebuild a command line
for the subprocess based on the opts structure and I make use of the modified
argv[0] from the command. Neither of these are really optimal, it'd be much
easier if I could just start my subprocess from cmd_grep (or even grep_cache).
Any pointers to get me moving in this direction are welcome. Even if I retain
the rebuilding of the command line I'd like to rebuild the pattern(s) instead
of relying on the saved_argv[0].

Chris Packham (3):
      add test for git grep --recursive
      grep: prepare grep for submodules
      grep: add support for grepping in submodules

[RFC PATCH 1/3] add test for git grep --recursive

[Chris Packham]

Signed-off-by: Chris Packham <judge.packham@gmail.com>
---
 t/t7820-grep-recursive.sh |  101 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 101 insertions(+), 0 deletions(-)
 create mode 100644 t/t7820-grep-recursive.sh

diff --git a/t/t7820-grep-recursive.sh b/t/t7820-grep-recursive.sh
new file mode 100644
index 0000000..4bbd109
--- /dev/null
+++ b/t/t7820-grep-recursive.sh
@@ -0,0 +1,101 @@
+#!/bin/sh
+#
+# Copyright (c) 2010 Chris Packham
+#
+
+test_description='git grep --recursive test
+
+This test checks the ability of git grep to search within submodules when told
+to do so with the --recursive option'
+
+. ./test-lib.sh
+
+test_expect_success 'setup - initial commit' '
+	printf "one two three\nfour five six\n" >t &&
+	git add t &&
+	git commit -m "initial commit"
+'
+submodurl=$TRASH_DIRECTORY
+
+test_expect_success 'setup submodules for test' '
+	for mod in $(seq 1 5 | sed "s/.*/submodule&/"); do
+		git submodule add "$submodurl" $mod &&
+		git submodule init $mod
+	done
+'
+
+test_expect_success 'update data in each submodule' '
+	for n in $(seq 1 5); do
+		(cd submodule$n &&
+			sed -i "s/^four.*/& #$n/" t &&
+			git commit -a -m"update")
+	done
+'
+
+cat >expected <<EOF
+t:four five six
+EOF
+test_expect_success 'non-recursive grep in base' '
+	git grep "five" >actual &&
+	test_cmp expected actual
+'
+
+cat >expected <<EOF
+foo/t:four five six
+EOF
+test_expect_success 'submodule-prefix option' '
+	git grep --submodule-prefix=foo/ "five" >actual &&
+	test_cmp expected actual
+'
+
+cat >submodule1/expected <<EOF
+t:four five six #1
+EOF
+test_expect_success 'non-recursive grep in submodule' '
+	(
+		cd submodule1 &&
+		git grep "five" >actual &&
+		test_cmp expected actual
+	)
+'
+
+cat >expected <<EOF
+t:four five six #1
+t:four five six #2
+t:four five six #3
+t:four five six #4
+t:four five six #5
+t:four five six
+EOF
+test_expect_success 'recursive grep' '
+	git grep --recurse-submodules "five" >actual &&
+	test_cmp expected actual
+'
+
+cat >expected <<EOF
+t:2:four five six #1
+t:2:four five six #2
+t:2:four five six #3
+t:2:four five six #4
+t:2:four five six #5
+t:2:four five six
+EOF
+test_expect_success 'recursive grep (with -n)' '
+	git grep --recurse-submodules -n "five" >actual &&
+	test_cmp expected actual
+'
+
+cat >expected <<EOF
+t
+t
+t
+t
+t
+t
+EOF
+test_expect_success 'recursive grep (with -l)' '
+	git grep --recurse-submodules -l "five" >actual &&
+	test_cmp expected actual
+'
+
+test_done
-- 
1.7.3.dirty

[RFC PATCH 2/3] grep: prepare grep for submodules

[Chris Packham]

Add --submodule-prefix option to pass to subprocess grep invocations. The
prefix is then used when outputting the results.

Signed-off-by: Chris Packham <judge.packham@gmail.com>
---
 builtin/grep.c |    2 ++
 grep.c         |    8 ++++++++
 grep.h         |    1 +
 3 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index da32f3d..8315ff0 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -927,6 +927,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 			    "allow calling of grep(1) (ignored by this build)"),
 		{ OPTION_CALLBACK, 0, "help-all", &options, NULL, "show usage",
 		  PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, help_callback },
+		OPT_STRING(0, "submodule-prefix", &opt.submodule_prefix, "DIR",
+			"prepend this to submodule path output"),
 		OPT_END()
 	};
 
diff --git a/grep.c b/grep.c
index 63c4280..36bec98 100644
--- a/grep.c
+++ b/grep.c
@@ -370,6 +370,10 @@ static void output_sep(struct grep_opt *opt, char sign)
 
 static void show_name(struct grep_opt *opt, const char *name)
 {
+	if (opt->submodule_prefix) {
+		output_color(opt, opt->submodule_prefix,
+			strlen(opt->submodule_prefix), opt->color_filename);
+	}
 	output_color(opt, name, strlen(name), opt->color_filename);
 	opt->output(opt, opt->null_following_name ? "\0" : "\n", 1);
 }
@@ -644,6 +648,10 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol,
 	}
 	opt->last_shown = lno;
 
+	if (opt->submodule_prefix) {
+		output_color(opt, opt->submodule_prefix,
+			strlen(opt->submodule_prefix), opt->color_filename);
+	}
 	if (opt->pathname) {
 		output_color(opt, name, strlen(name), opt->color_filename);
 		output_sep(opt, sign);
diff --git a/grep.h b/grep.h
index 06621fe..d918da4 100644
--- a/grep.h
+++ b/grep.h
@@ -67,6 +67,7 @@ struct grep_opt {
 	struct grep_expr *pattern_expression;
 	const char *prefix;
 	int prefix_length;
+	const char *submodule_prefix;
 	regex_t regexp;
 	int linenum;
 	int invert;
-- 
1.7.3.dirty

[RFC PATCH 3/3] grep: add support for grepping in submodules

[Chris Packham]

When the --recurse-submodules option is given git grep will search in
submodules as they are encountered.

Signed-off-by: Chris Packham <judge.packham@gmail.com>
---
 builtin/grep.c |   72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 grep.h         |    1 +
 2 files changed, 73 insertions(+), 0 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 8315ff0..c9befdc 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -30,6 +30,9 @@ static char const * const grep_usage[] = {
 
 static int use_threads = 1;
 
+static int saved_argc;
+static const char **saved_argv;
+
 #ifndef NO_PTHREADS
 #define THREADS 8
 static pthread_t threads[THREADS];
@@ -585,6 +588,67 @@ static void run_pager(struct grep_opt *opt, const char *prefix)
 	free(argv);
 }
 
+static const char **create_sub_grep_argv(struct grep_opt *opt, const char *path)
+{
+	#define NUM_ARGS 10
+	struct strbuf buf = STRBUF_INIT;
+	const char **argv;
+	int i = 0;
+
+	argv = xcalloc(NUM_ARGS, sizeof(const char *));
+	argv[i++] = "grep";
+	strbuf_addf(&buf, "--submodule-prefix=\\\"%s\\\"", path);
+	//argv[i++] = buf.buf;
+
+	if (opt->linenum)
+		argv[i++] = "-n";
+	if (opt->invert)
+		argv[i++] = "-v";
+	if (opt->ignore_case)
+		argv[i++] = "-i";
+	if (opt->count)
+		argv[i++] = "-c";
+	if (opt->name_only)
+		argv[i++] = "-l";
+
+	argv[i++] = saved_argv[0];
+	argv[i++] = NULL;
+
+	strbuf_release(&buf);
+	return argv;
+}
+
+static int grep_submodule(struct grep_opt *opt, const char *path)
+{
+	struct strbuf buf = STRBUF_INIT;
+	struct child_process cp;	
+	const char **argv = create_sub_grep_argv(opt, path);
+	const char *git_dir;
+	int hit = 0;
+
+	strbuf_addf(&buf, "%s/.git", path);
+	git_dir = read_gitfile_gently(buf.buf);
+	if (!git_dir)
+		git_dir = buf.buf;
+	if (!is_directory(git_dir)) {
+		warning("submodule %s has not been initialized\n", path);
+		goto out_free;
+	}
+
+	memset(&cp, 0, sizeof(cp));
+	cp.argv = argv;
+	cp.env = local_repo_env;
+	cp.git_cmd = 1;
+	cp.no_stdin = 1;
+	cp.dir = path;
+	if (run_command(&cp) == 0)
+		hit = 1;
+out_free:
+	free(argv);
+	strbuf_release(&buf);
+	return hit;
+}
+
 static int grep_cache(struct grep_opt *opt, const char **paths, int cached)
 {
 	int hit = 0;
@@ -593,6 +657,10 @@ static int grep_cache(struct grep_opt *opt, const char **paths, int cached)
 
 	for (nr = 0; nr < active_nr; nr++) {
 		struct cache_entry *ce = active_cache[nr];
+		if (S_ISGITLINK(ce->ce_mode) && opt->recurse_submodules) {
+			hit |= grep_submodule(opt, ce->name);
+			continue;
+		}
 		if (!S_ISREG(ce->ce_mode))
 			continue;
 		if (!pathspec_matches(paths, ce->name, opt->max_depth))
@@ -929,9 +997,13 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		  PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, help_callback },
 		OPT_STRING(0, "submodule-prefix", &opt.submodule_prefix, "DIR",
 			"prepend this to submodule path output"),
+		OPT_BOOLEAN(0, "recurse-submodules", &opt.recurse_submodules,
+			"recurse into submodules"),
 		OPT_END()
 	};
 
+	saved_argc = argc;
+	saved_argv = argv;
 	/*
 	 * 'git grep -h', unlike 'git grep -h <pattern>', is a request
 	 * to show usage information and exit.
diff --git a/grep.h b/grep.h
index d918da4..e3199c9 100644
--- a/grep.h
+++ b/grep.h
@@ -102,6 +102,7 @@ struct grep_opt {
 	unsigned post_context;
 	unsigned last_shown;
 	int show_hunk_mark;
+	int recurse_submodules;
 	void *priv;
 
 	void (*output)(struct grep_opt *opt, const void *data, size_t size);
-- 
1.7.3.dirty

Re: [RFC PATCH 1/3] add test for git grep --recursive

[Ævar Arnfjörð Bjarmason]

On Wed, Sep 29, 2010 at 20:28, Chris Packham <judge.packham@gmail.com> wrote:
> Signed-off-by: Chris Packham <judge.packham@gmail.com>
> ---
>  t/t7820-grep-recursive.sh |  101 +++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 101 insertions(+), 0 deletions(-)
>  create mode 100644 t/t7820-grep-recursive.sh
>
> diff --git a/t/t7820-grep-recursive.sh b/t/t7820-grep-recursive.sh
> new file mode 100644
> index 0000000..4bbd109
> --- /dev/null
> +++ b/t/t7820-grep-recursive.sh
> @@ -0,0 +1,101 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2010 Chris Packham
> +#
> +
> +test_description='git grep --recursive test
> +
> +This test checks the ability of git grep to search within submodules when told
> +to do so with the --recursive option'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'setup - initial commit' '
> +       printf "one two three\nfour five six\n" >t &&
> +       git add t &&
> +       git commit -m "initial commit"
> +'
> +submodurl=$TRASH_DIRECTORY
> +
> +test_expect_success 'setup submodules for test' '
> +       for mod in $(seq 1 5 | sed "s/.*/submodule&/"); do
> +               git submodule add "$submodurl" $mod &&
> +               git submodule init $mod
> +       done
> +'
> +
> +test_expect_success 'update data in each submodule' '
> +       for n in $(seq 1 5); do

seq isn't portable to windows, so we usually write out "1 2 3 4 5"
directly.

> +               (cd submodule$n &&
> +                       sed -i "s/^four.*/& #$n/" t &&
> +                       git commit -a -m"update")
> +       done
> +'
> +
> +cat >expected <<EOF
> +t:four five six
> +EOF
> +test_expect_success 'non-recursive grep in base' '
> +       git grep "five" >actual &&
> +       test_cmp expected actual
> +'

Put the "cat >expected <<EOF" inside the test:

    test_expect_success 'non-recursive grep in base' '
        cat >expected <<\EOF &&
        t:four five six
        EOF
        git grep "five" >actual &&
        test_cmp expected actual
    '

ditto for the rest.

Re: [RFC PATCH 1/3] add test for git grep --recursive

[Chris Packham]

On 29/09/10 13:35, Ævar Arnfjörð Bjarmason wrote:
> On Wed, Sep 29, 2010 at 20:28, Chris Packham <judge.packham@gmail.com> wrote:
>> Signed-off-by: Chris Packham <judge.packham@gmail.com>
>> ---
>>  t/t7820-grep-recursive.sh |  101 +++++++++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 101 insertions(+), 0 deletions(-)
>>  create mode 100644 t/t7820-grep-recursive.sh
>>
>> diff --git a/t/t7820-grep-recursive.sh b/t/t7820-grep-recursive.sh
>> new file mode 100644
>> index 0000000..4bbd109
>> --- /dev/null
>> +++ b/t/t7820-grep-recursive.sh
>> @@ -0,0 +1,101 @@
>> +#!/bin/sh
>> +#
>> +# Copyright (c) 2010 Chris Packham
>> +#
>> +
>> +test_description='git grep --recursive test
>> +
>> +This test checks the ability of git grep to search within submodules when told
>> +to do so with the --recursive option'
>> +
>> +. ./test-lib.sh
>> +
>> +test_expect_success 'setup - initial commit' '
>> +       printf "one two three\nfour five six\n" >t &&
>> +       git add t &&
>> +       git commit -m "initial commit"
>> +'
>> +submodurl=$TRASH_DIRECTORY
>> +
>> +test_expect_success 'setup submodules for test' '
>> +       for mod in $(seq 1 5 | sed "s/.*/submodule&/"); do
>> +               git submodule add "$submodurl" $mod &&
>> +               git submodule init $mod
>> +       done
>> +'
>> +
>> +test_expect_success 'update data in each submodule' '
>> +       for n in $(seq 1 5); do
> 
> seq isn't portable to windows, so we usually write out "1 2 3 4 5"
> directly.
> 
>> +               (cd submodule$n &&
>> +                       sed -i "s/^four.*/& #$n/" t &&
>> +                       git commit -a -m"update")
>> +       done
>> +'
>> +
>> +cat >expected <<EOF
>> +t:four five six
>> +EOF
>> +test_expect_success 'non-recursive grep in base' '
>> +       git grep "five" >actual &&
>> +       test_cmp expected actual
>> +'
> 
> Put the "cat >expected <<EOF" inside the test:
> 
>     test_expect_success 'non-recursive grep in base' '
>         cat >expected <<\EOF &&
>         t:four five six
>         EOF
>         git grep "five" >actual &&
>         test_cmp expected actual
>     '
> 
> ditto for the rest.

Thanks for the review, will be in next re-roll.

Re: [RFC PATCH 1/3] add test for git grep --recursive

[Kevin Ballard]

On Sep 29, 2010, at 1:35 PM, Ævar Arnfjörð Bjarmason wrote:

>> +test_expect_success 'update data in each submodule' '
>> +       for n in $(seq 1 5); do
> 
> seq isn't portable to windows, so we usually write out "1 2 3 4 5"
> directly.

FWIW, seq isn't even provided by default on OS X.

-Kevin Ballard

Re: [RFC PATCH 3/3] grep: add support for grepping in submodules

[Jens Lehmann]

Nice start!


Am 29.09.2010 22:28, schrieb Chris Packham:
> When the --recurse-submodules option is given git grep will search in
> submodules as they are encountered.

As "git clone" already introduced a "--recursive" option for
submodule recursion IMO "--recursive" should be used here too for
consistency. (Maybe you took the idea to use "--recurse-submodules"
from my "git-checkout-recurse-submodules" branch on github? But that
is only used there because I didn't get around to change it yet like
I did in the "fetch-submodules-too" branch).


> Signed-off-by: Chris Packham <judge.packham@gmail.com>
> ---
>  builtin/grep.c |   72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  grep.h         |    1 +
>  2 files changed, 73 insertions(+), 0 deletions(-)
> 
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 8315ff0..c9befdc 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -30,6 +30,9 @@ static char const * const grep_usage[] = {
>  
>  static int use_threads = 1;
>  
> +static int saved_argc;
> +static const char **saved_argv;
> +
>  #ifndef NO_PTHREADS
>  #define THREADS 8
>  static pthread_t threads[THREADS];
> @@ -585,6 +588,67 @@ static void run_pager(struct grep_opt *opt, const char *prefix)
>  	free(argv);
>  }
>  
> +static const char **create_sub_grep_argv(struct grep_opt *opt, const char *path)
> +{
> +	#define NUM_ARGS 10
> +	struct strbuf buf = STRBUF_INIT;
> +	const char **argv;
> +	int i = 0;
> +
> +	argv = xcalloc(NUM_ARGS, sizeof(const char *));
> +	argv[i++] = "grep";
> +	strbuf_addf(&buf, "--submodule-prefix=\\\"%s\\\"", path);
> +	//argv[i++] = buf.buf;

As C++ comments are not portable they have to be avoided, but I
assume this one here (and the unused "saved_argc" variable too) is
a hint that this code is not working as intended yet? ;-)

It seems you want to use strbuf_detach() here so that this argv[]
stays valid after the strbuf_release() at the end of this function.
And if I'm not missing something this would not work correctly in
the second recursion depth, as the new submodule prefix should
be the one given to this grep command concatenated with the current
submodule path.


> +	if (opt->linenum)
> +		argv[i++] = "-n";
> +	if (opt->invert)
> +		argv[i++] = "-v";
> +	if (opt->ignore_case)
> +		argv[i++] = "-i";
> +	if (opt->count)
> +		argv[i++] = "-c";
> +	if (opt->name_only)
> +		argv[i++] = "-l";
> +
> +	argv[i++] = saved_argv[0];
> +	argv[i++] = NULL;

Hm, at a quick glance it might be much easier to copy argc & argv
in cmd_grep() before parse_options() starts manipulating it. Then
you would only have to change/add the "--submodule-prefix" option
as needed and would not have to deal with all possible grep option
combinations (and for example you don't pass the recurse option
yet, which would stop the recursion pretty soon).


> +
> +	strbuf_release(&buf);
> +	return argv;
> +}
> +
> +static int grep_submodule(struct grep_opt *opt, const char *path)
> +{
> +	struct strbuf buf = STRBUF_INIT;
> +	struct child_process cp;	
> +	const char **argv = create_sub_grep_argv(opt, path);
> +	const char *git_dir;
> +	int hit = 0;
> +
> +	strbuf_addf(&buf, "%s/.git", path);
> +	git_dir = read_gitfile_gently(buf.buf);
> +	if (!git_dir)
> +		git_dir = buf.buf;
> +	if (!is_directory(git_dir)) {
> +		warning("submodule %s has not been initialized\n", path);

Having a submodule which is not checked out is perfectly fine, so
I don't think we want to issue a warning here.


> +		goto out_free;
> +	}
> +
> +	memset(&cp, 0, sizeof(cp));
> +	cp.argv = argv;
> +	cp.env = local_repo_env;
> +	cp.git_cmd = 1;
> +	cp.no_stdin = 1;
> +	cp.dir = path;
> +	if (run_command(&cp) == 0)
> +		hit = 1;
> +out_free:
> +	free(argv);
> +	strbuf_release(&buf);
> +	return hit;
> +}
> +
>  static int grep_cache(struct grep_opt *opt, const char **paths, int cached)
>  {
>  	int hit = 0;
> @@ -593,6 +657,10 @@ static int grep_cache(struct grep_opt *opt, const char **paths, int cached)
>  
>  	for (nr = 0; nr < active_nr; nr++) {
>  		struct cache_entry *ce = active_cache[nr];
> +		if (S_ISGITLINK(ce->ce_mode) && opt->recurse_submodules) {
> +			hit |= grep_submodule(opt, ce->name);
> +			continue;
> +		}
>  		if (!S_ISREG(ce->ce_mode))
>  			continue;
>  		if (!pathspec_matches(paths, ce->name, opt->max_depth))
> @@ -929,9 +997,13 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>  		  PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, help_callback },
>  		OPT_STRING(0, "submodule-prefix", &opt.submodule_prefix, "DIR",
>  			"prepend this to submodule path output"),
> +		OPT_BOOLEAN(0, "recurse-submodules", &opt.recurse_submodules,
> +			"recurse into submodules"),
>  		OPT_END()
>  	};
>  
> +	saved_argc = argc;
> +	saved_argv = argv;
>  	/*
>  	 * 'git grep -h', unlike 'git grep -h <pattern>', is a request
>  	 * to show usage information and exit.
> diff --git a/grep.h b/grep.h
> index d918da4..e3199c9 100644
> --- a/grep.h
> +++ b/grep.h
> @@ -102,6 +102,7 @@ struct grep_opt {
>  	unsigned post_context;
>  	unsigned last_shown;
>  	int show_hunk_mark;
> +	int recurse_submodules;
>  	void *priv;
>  
>  	void (*output)(struct grep_opt *opt, const void *data, size_t size);

Re: [RFC PATCH 3/3] grep: add support for grepping in submodules

[Junio C Hamano]

Jens Lehmann <Jens.Lehmann@web.de> writes:

> Hm, at a quick glance it might be much easier to copy argc & argv
> in cmd_grep() before parse_options() starts manipulating it.

Yes, I think that is a much saner direction to go.  Otherwise, you would
need to unparse grep boolean expressions as well.

A few more things to think about.

1. What does this mean:

    $ git grep --recursive -e frotz master next

It recurses into the submodule commits recorded in 'master' and 'next'
commits in the superproject, right?

How do the lines output from the above look like?  From the superproject,
we will get lines like these:

    master:t/README:  test_description='xxx test (option --frotz)
    master:t/README:  and tries to run git-ls-files with option --frotz.'

What if we have a submodule at git-gui in the superproject, and its README
has string frotz in it?  Should we label the submodule commit we find in
'master' of superproject as 'master' as well, even if it is not at the tip
of 'master' branch of the submodule?  Or do we get abbreviated hexadecimal
SHA-1 name?  IOW, would we see:

    master:git-gui/README: git-gui also knows frotz

or

    deadbeef:git-gui/README: git-gui also knows frotz

where "deadbeaf...." is what "git rev-parse master:git-gui" would give us
in the superproject?

I tend to think the former is preferable, but then most likely you would
need to pass not just submodule-prefix but the original ref name
(i.e. 'master') you started from down to the recursive one.

2. Now how would this work with pathspecs?

    $ git grep --recursive -e frotz -- dir/

This should look for the string in the named directory in the superproject
and if there are submodules in that directory, recurse into them as well,
right?

What pathspec, if any, will be in effect when we recurse into the
submodule at dir/sub?  Limiting to dir/ feels wrong, no?

3. Corollary.

Is it reasonable to expect that we will look into all shell scripts, both
in the superproject and in submodules, with this:

    $ git grep --recursive -e frotz -- '*.sh'

Oops?  What happened to the "we restrict the recursion using pathspec, and
we do not pass down the pathspec" that was suggested in 2.?

Re: [RFC PATCH 3/3] grep: add support for grepping in submodules

[Chris Packham]

On 29/09/10 15:21, Jens Lehmann wrote:
> Nice start!
> 
> 
> Am 29.09.2010 22:28, schrieb Chris Packham:
>> When the --recurse-submodules option is given git grep will search in
>> submodules as they are encountered.
> 
> As "git clone" already introduced a "--recursive" option for
> submodule recursion IMO "--recursive" should be used here too for
> consistency. (Maybe you took the idea to use "--recurse-submodules"
> from my "git-checkout-recurse-submodules" branch on github? But that
> is only used there because I didn't get around to change it yet like
> I did in the "fetch-submodules-too" branch).

I actually started with --recursive and switched to
--recurse-submodules. One thing with this is the standard grep
--recursive option which may cause some confusion if people expect git
grep to behave like normal grep. I'll switch to using --recursive for
now until someone objects to the potential confusion.

One more thought on this that has been hanging around in my mind. I
sometimes want to do something on all but one submodule, in this case
with grep I'm fairly likely to want to skip a linux repository because I
already know the thing I'm looking for is in userland. Maybe in the
future we can make --recursive take an argument that allows us to
specify/restrict which submodules get included in the command invocation.

> 
>> Signed-off-by: Chris Packham <judge.packham@gmail.com>
>> ---
>>  builtin/grep.c |   72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  grep.h         |    1 +
>>  2 files changed, 73 insertions(+), 0 deletions(-)
>>
>> diff --git a/builtin/grep.c b/builtin/grep.c
>> index 8315ff0..c9befdc 100644
>> --- a/builtin/grep.c
>> +++ b/builtin/grep.c
>> @@ -30,6 +30,9 @@ static char const * const grep_usage[] = {
>>  
>>  static int use_threads = 1;
>>  
>> +static int saved_argc;
>> +static const char **saved_argv;
>> +
>>  #ifndef NO_PTHREADS
>>  #define THREADS 8
>>  static pthread_t threads[THREADS];
>> @@ -585,6 +588,67 @@ static void run_pager(struct grep_opt *opt, const char *prefix)
>>  	free(argv);
>>  }
>>  
>> +static const char **create_sub_grep_argv(struct grep_opt *opt, const char *path)
>> +{
>> +	#define NUM_ARGS 10
>> +	struct strbuf buf = STRBUF_INIT;
>> +	const char **argv;
>> +	int i = 0;
>> +
>> +	argv = xcalloc(NUM_ARGS, sizeof(const char *));
>> +	argv[i++] = "grep";
>> +	strbuf_addf(&buf, "--submodule-prefix=\\\"%s\\\"", path);
>> +	//argv[i++] = buf.buf;
> 
> As C++ comments are not portable they have to be avoided, but I
> assume this one here (and the unused "saved_argc" variable too) is
> a hint that this code is not working as intended yet? ;-)

Yeah this is due to my test problem I mentioned in the cover email. When
run_command gets called it ends up invoking the installed git executable
which doesn't understand my new option.

> It seems you want to use strbuf_detach() here so that this argv[]
> stays valid after the strbuf_release() at the end of this function.
> And if I'm not missing something this would not work correctly in
> the second recursion depth, as the new submodule prefix should
> be the one given to this grep command concatenated with the current
> submodule path.

I'll look into strbuf_detatch. The tricky thing will be keeping track of
what to free at the end of grep_submodule. I guess I can assume that it
will argv[1] is always going to be the dynamically allocated string.

> 
>> +	if (opt->linenum)
>> +		argv[i++] = "-n";
>> +	if (opt->invert)
>> +		argv[i++] = "-v";
>> +	if (opt->ignore_case)
>> +		argv[i++] = "-i";
>> +	if (opt->count)
>> +		argv[i++] = "-c";
>> +	if (opt->name_only)
>> +		argv[i++] = "-l";
>> +
>> +	argv[i++] = saved_argv[0];
>> +	argv[i++] = NULL;
> 
> Hm, at a quick glance it might be much easier to copy argc & argv
> in cmd_grep() before parse_options() starts manipulating it. Then
> you would only have to change/add the "--submodule-prefix" option
> as needed and would not have to deal with all possible grep option
> combinations (and for example you don't pass the recurse option
> yet, which would stop the recursion pretty soon).

Yeah this is the part I was struggling with a little. It would be easy
to save argv before any option processing but I wondered if that would
be frowned upon as an overhead for non-submodule usages.

I was thinking about doing something tricky with max-depth and
recursion. But maybe its better to keep it simple.

>> +
>> +	strbuf_release(&buf);
>> +	return argv;
>> +}
>> +
>> +static int grep_submodule(struct grep_opt *opt, const char *path)
>> +{
>> +	struct strbuf buf = STRBUF_INIT;
>> +	struct child_process cp;	
>> +	const char **argv = create_sub_grep_argv(opt, path);
>> +	const char *git_dir;
>> +	int hit = 0;
>> +
>> +	strbuf_addf(&buf, "%s/.git", path);
>> +	git_dir = read_gitfile_gently(buf.buf);
>> +	if (!git_dir)
>> +		git_dir = buf.buf;
>> +	if (!is_directory(git_dir)) {
>> +		warning("submodule %s has not been initialized\n", path);
> 
> Having a submodule which is not checked out is perfectly fine, so
> I don't think we want to issue a warning here.
> 
OK I'll remove it.

>> +		goto out_free;
>> +	}
>> +
>> +	memset(&cp, 0, sizeof(cp));
>> +	cp.argv = argv;
>> +	cp.env = local_repo_env;
>> +	cp.git_cmd = 1;
>> +	cp.no_stdin = 1;
>> +	cp.dir = path;
>> +	if (run_command(&cp) == 0)
>> +		hit = 1;
>> +out_free:
>> +	free(argv);
>> +	strbuf_release(&buf);
>> +	return hit;
>> +}
>> +
>>  static int grep_cache(struct grep_opt *opt, const char **paths, int cached)
>>  {
>>  	int hit = 0;
>> @@ -593,6 +657,10 @@ static int grep_cache(struct grep_opt *opt, const char **paths, int cached)
>>  
>>  	for (nr = 0; nr < active_nr; nr++) {
>>  		struct cache_entry *ce = active_cache[nr];
>> +		if (S_ISGITLINK(ce->ce_mode) && opt->recurse_submodules) {
>> +			hit |= grep_submodule(opt, ce->name);
>> +			continue;
>> +		}
>>  		if (!S_ISREG(ce->ce_mode))
>>  			continue;
>>  		if (!pathspec_matches(paths, ce->name, opt->max_depth))
>> @@ -929,9 +997,13 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>>  		  PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, help_callback },
>>  		OPT_STRING(0, "submodule-prefix", &opt.submodule_prefix, "DIR",
>>  			"prepend this to submodule path output"),
>> +		OPT_BOOLEAN(0, "recurse-submodules", &opt.recurse_submodules,
>> +			"recurse into submodules"),
>>  		OPT_END()
>>  	};
>>  
>> +	saved_argc = argc;
>> +	saved_argv = argv;
>>  	/*
>>  	 * 'git grep -h', unlike 'git grep -h <pattern>', is a request
>>  	 * to show usage information and exit.
>> diff --git a/grep.h b/grep.h
>> index d918da4..e3199c9 100644
>> --- a/grep.h
>> +++ b/grep.h
>> @@ -102,6 +102,7 @@ struct grep_opt {
>>  	unsigned post_context;
>>  	unsigned last_shown;
>>  	int show_hunk_mark;
>> +	int recurse_submodules;
>>  	void *priv;
>>  
>>  	void (*output)(struct grep_opt *opt, const void *data, size_t size);
> 

Re: [RFC PATCH 3/3] grep: add support for grepping in submodules

[Chris Packham]

On 29/09/10 15:59, Junio C Hamano wrote:
> Jens Lehmann <Jens.Lehmann@web.de> writes:
> 
>> Hm, at a quick glance it might be much easier to copy argc & argv
>> in cmd_grep() before parse_options() starts manipulating it.
> 
> Yes, I think that is a much saner direction to go.  Otherwise, you would
> need to unparse grep boolean expressions as well.

I've got some of that working in my quest to not use saved_argv[0]. If
we follow some of your points below about handling ref names then
rebuilding the args actually starts to make life easier. I guess the
same is true for just making these manipulations on the grep_opts. The
only thing that is really clear is that saving the original argv is not
going to help us.

> A few more things to think about.
> 
> 1. What does this mean:
> 
>     $ git grep --recursive -e frotz master next
> 
> It recurses into the submodule commits recorded in 'master' and 'next'
> commits in the superproject, right?
> 
> How do the lines output from the above look like?  From the superproject,
> we will get lines like these:
> 
>     master:t/README:  test_description='xxx test (option --frotz)
>     master:t/README:  and tries to run git-ls-files with option --frotz.'
> 
> What if we have a submodule at git-gui in the superproject, and its README
> has string frotz in it?  Should we label the submodule commit we find in
> 'master' of superproject as 'master' as well, even if it is not at the tip
> of 'master' branch of the submodule?  Or do we get abbreviated hexadecimal
> SHA-1 name?  IOW, would we see:
> 
>     master:git-gui/README: git-gui also knows frotz
> 
> or
> 
>     deadbeef:git-gui/README: git-gui also knows frotz
> 
> where "deadbeaf...." is what "git rev-parse master:git-gui" would give us
> in the superproject?
> 
> I tend to think the former is preferable, but then most likely you would
> need to pass not just submodule-prefix but the original ref name
> (i.e. 'master') you started from down to the recursive one.

Passing the ref name is doable. There is a little potential for
confusion between who's "master" that actually is (the same confusion is
in theory possible with an abbreviated SHA-1). Maybe we should color the
submodule ref's differently

> 2. Now how would this work with pathspecs?
> 
>     $ git grep --recursive -e frotz -- dir/
> 
> This should look for the string in the named directory in the superproject
> and if there are submodules in that directory, recurse into them as well,
> right?
> 
> What pathspec, if any, will be in effect when we recurse into the
> submodule at dir/sub?  Limiting to dir/ feels wrong, no?
> 
> 3. Corollary.
> 
> Is it reasonable to expect that we will look into all shell scripts, both
> in the superproject and in submodules, with this:
> 
>     $ git grep --recursive -e frotz -- '*.sh'
> 
> Oops?  What happened to the "we restrict the recursion using pathspec, and
> we do not pass down the pathspec" that was suggested in 2.?
> 

This is a bit of a grey area, I'm not sure what is the sensible thing to do.

Maybe we could pop a directory level per recursion e.g.
  user enters 'dir/sub/subsub/*.sh'
  first level recursion is passed 'sub/subsub/*.sh'
  second level recursion is passed 'subsub/*.sh'
  subsequent levels of recursion are passed '*.sh'

But that's not quite what the user thought they asked for (i.e. they
will end up with dir/sub/subsub/subsubsub/file.sh).

Or we could alter the behaviour based on whether their original pathspec
had an explicit trailing /.

Re: [RFC PATCH 2/3] grep: prepare grep for submodules

[Nguyen Thai Ngoc Duy]

On Thu, Sep 30, 2010 at 6:28 AM, Chris Packham <judge.packham@gmail.com> wrote:
> Add --submodule-prefix option to pass to subprocess grep invocations. The
> prefix is then used when outputting the results.

I haven't followed the recursive submodule support in Git lately. But
I think --submodule-prefix is unnecessary. I would imagine you need to
add --submodule-prefix to a lot more commands as they support recusive
submodule search. There is a corner case in Git's prefix setup that we
can utilize to avoid the new option.

If you do this at the superproject repo:

$ GIT_DIR=path/to/submodule/.git GIT_WORK_TREE=path/to/submodule git grep blah

I would expect that it shows the result correctly (i.e. all files
prefixed by "path/to/submodule"), but it does not right now. If you
make that setup work, then you don't need --submodule-prefix, just set
GIT_DIR/GIT_WORK_TREE properly and run "git grep".

You can make setup_explicit_git_dir() realize that situation (current
working directory outside $GIT_WORK_TREE), then calculate and save the
submodule prefix in startup_info struct. Then "git grep" or any
commands can just read startup_info to find out the submodule prefix.
-- 
Duy

Re: [RFC PATCH 3/3] grep: add support for grepping in submodules

[Jens Lehmann]

Am 30.09.2010 01:47, schrieb Chris Packham:
> On 29/09/10 15:59, Junio C Hamano wrote:
>> A few more things to think about.
>>
>> 1. What does this mean:
>>
>>     $ git grep --recursive -e frotz master next
>>
>> It recurses into the submodule commits recorded in 'master' and 'next'
>> commits in the superproject, right?
>>
>> How do the lines output from the above look like?  From the superproject,
>> we will get lines like these:
>>
>>     master:t/README:  test_description='xxx test (option --frotz)
>>     master:t/README:  and tries to run git-ls-files with option --frotz.'
>>
>> What if we have a submodule at git-gui in the superproject, and its README
>> has string frotz in it?  Should we label the submodule commit we find in
>> 'master' of superproject as 'master' as well, even if it is not at the tip
>> of 'master' branch of the submodule?  Or do we get abbreviated hexadecimal
>> SHA-1 name?  IOW, would we see:
>>
>>     master:git-gui/README: git-gui also knows frotz
>>
>> or
>>
>>     deadbeef:git-gui/README: git-gui also knows frotz
>>
>> where "deadbeaf...." is what "git rev-parse master:git-gui" would give us
>> in the superproject?
>>
>> I tend to think the former is preferable, but then most likely you would
>> need to pass not just submodule-prefix but the original ref name
>> (i.e. 'master') you started from down to the recursive one.

Me too thinks that as you grep from inside the superproject it makes
more sense to use the ref name used there and not the SHA-1 from the
submodule.

> Passing the ref name is doable. There is a little potential for
> confusion between who's "master" that actually is (the same confusion is
> in theory possible with an abbreviated SHA-1). Maybe we should color the
> submodule ref's differently

Hmm, showing somehow that the grep result is from inside a submodule
could be helpful. But using something like the following line seems a
bit like overkill, so coloring might be a good alternative:

     master/deadbeef:git-gui/README: git-gui also knows frotz

But I don't have a strong opinion here.


>> 2. Now how would this work with pathspecs?
>>
>>     $ git grep --recursive -e frotz -- dir/
>>
>> This should look for the string in the named directory in the superproject
>> and if there are submodules in that directory, recurse into them as well,
>> right?
>>
>> What pathspec, if any, will be in effect when we recurse into the
>> submodule at dir/sub?  Limiting to dir/ feels wrong, no?
>>
>> 3. Corollary.
>>
>> Is it reasonable to expect that we will look into all shell scripts, both
>> in the superproject and in submodules, with this:
>>
>>     $ git grep --recursive -e frotz -- '*.sh'
>>
>> Oops?  What happened to the "we restrict the recursion using pathspec, and
>> we do not pass down the pathspec" that was suggested in 2.?
>>
> 
> This is a bit of a grey area, I'm not sure what is the sensible thing to do.
> 
> Maybe we could pop a directory level per recursion e.g.
>   user enters 'dir/sub/subsub/*.sh'
>   first level recursion is passed 'sub/subsub/*.sh'
>   second level recursion is passed 'subsub/*.sh'
>   subsequent levels of recursion are passed '*.sh'
> 
> But that's not quite what the user thought they asked for (i.e. they
> will end up with dir/sub/subsub/subsubsub/file.sh).
> 
> Or we could alter the behaviour based on whether their original pathspec
> had an explicit trailing /.

I think we'll have to manipulate the pathspecs so we properly translate
their meaning into the context of the submodule. What about this: If
they point outside the submodule, they must be dropped. If they contain
directories, the prefix part has to be stripped from the beginning.
Examples for submodule 'dir/sub':

  * 'dir/' and 'dir/sub2/' get dropped as they point outside
  * '*.sh' should just be passed on
  * 'dir/sub/foo/*.sh' would become 'foo/*.sh'
  * 'dir/sub/' gets dropped too (as the result of stripping the
    prefix is '')

That should lead to the expected behavior.

Re: [RFC PATCH 3/3] grep: add support for grepping in submodules

[Jens Lehmann]

Am 30.09.2010 01:02, schrieb Chris Packham:
> I actually started with --recursive and switched to
> --recurse-submodules. One thing with this is the standard grep
> --recursive option which may cause some confusion if people expect git
> grep to behave like normal grep.

Guess how I came to use "--recurse-submodules" for recursive checkout
in the first place ;-) But the fact that clone already uses it weighs
stronger here I suppose ...


> One more thought on this that has been hanging around in my mind. I
> sometimes want to do something on all but one submodule, in this case
> with grep I'm fairly likely to want to skip a linux repository because I
> already know the thing I'm looking for is in userland. Maybe in the
> future we can make --recursive take an argument that allows us to
> specify/restrict which submodules get included in the command invocation.

Hmm, maybe adding an option to "git grep" to exclude a pathspec would
make more sense?


>> It seems you want to use strbuf_detach() here so that this argv[]
>> stays valid after the strbuf_release() at the end of this function.

> I'll look into strbuf_detatch. The tricky thing will be keeping track of
> what to free at the end of grep_submodule.

Right, but if you push the strbuf operations into one of the calling
functions you can achieve that more easily.


> Yeah this is the part I was struggling with a little. It would be easy
> to save argv before any option processing but I wondered if that would
> be frowned upon as an overhead for non-submodule usages.

Yup, but as you are only copying a pointer array the overhead is very
small. And if the code gets much easier that way (as I would expect)
that price is well paid.

Re: [RFC PATCH 3/3] grep: add support for grepping in submodules

[Johannes Sixt]

Am 9/30/2010 0:59, schrieb Junio C Hamano:
> A few more things to think about.
> 
> 1. What does this mean:
> 
>     $ git grep --recursive -e frotz master next
> 
> It recurses into the submodule commits recorded in 'master' and 'next'
> commits in the superproject, right?

And what does it mean if you add --cached? Does it grep in the index of
the submodules, or does it grep in the rev of the submodule that is
recorded in the index of the supermodule?

-- Hannes

Re: [RFC PATCH 3/3] grep: add support for grepping in submodules

[Jens Lehmann]

Am 30.09.2010 13:28, schrieb Johannes Sixt:
> Am 9/30/2010 0:59, schrieb Junio C Hamano:
>> A few more things to think about.
>>
>> 1. What does this mean:
>>
>>     $ git grep --recursive -e frotz master next
>>
>> It recurses into the submodule commits recorded in 'master' and 'next'
>> commits in the superproject, right?
> 
> And what does it mean if you add --cached? Does it grep in the index of
> the submodules, or does it grep in the rev of the submodule that is
> recorded in the index of the supermodule?

Hmm, as you told grep to use the index of the superproject it should
use the rev of the submodule that is recorded in the index of the
superproject. Thus the "--cached" should be removed from and the
appropriate rev must be added to the arguments of the grep forked in
the submodule.

Re: [RFC PATCH 3/3] grep: add support for grepping in submodules

[Chris Packham]

On 30/09/10 04:24, Jens Lehmann wrote:
> Am 30.09.2010 01:02, schrieb Chris Packham:
>> Yeah this is the part I was struggling with a little. It would be easy
>> to save argv before any option processing but I wondered if that would
>> be frowned upon as an overhead for non-submodule usages.
> 
> Yup, but as you are only copying a pointer array the overhead is very
> small. And if the code gets much easier that way (as I would expect)
> that price is well paid.

With the talk of translating superproject --index into submodule SHA-1,
re-formatting pathspecs and passing the superproject ref-name. It sounds
like making a copy of argv is not going to be that useful. If we did
copy it we'd have to scan it for the things we don't want passed to the
submodule grep.

Re: [RFC PATCH 2/3] grep: prepare grep for submodules

[Chris Packham]

On 29/09/10 18:10, Nguyen Thai Ngoc Duy wrote:
> On Thu, Sep 30, 2010 at 6:28 AM, Chris Packham <judge.packham@gmail.com> wrote:
>> Add --submodule-prefix option to pass to subprocess grep invocations. The
>> prefix is then used when outputting the results.
> 
> I haven't followed the recursive submodule support in Git lately. But
> I think --submodule-prefix is unnecessary. I would imagine you need to
> add --submodule-prefix to a lot more commands as they support recusive
> submodule search. There is a corner case in Git's prefix setup that we
> can utilize to avoid the new option.
> 
> If you do this at the superproject repo:
> 
> $ GIT_DIR=path/to/submodule/.git GIT_WORK_TREE=path/to/submodule git grep blah
> 
> I would expect that it shows the result correctly (i.e. all files
> prefixed by "path/to/submodule"), but it does not right now. If you
> make that setup work, then you don't need --submodule-prefix, just set
> GIT_DIR/GIT_WORK_TREE properly and run "git grep".
> 
> You can make setup_explicit_git_dir() realize that situation (current
> working directory outside $GIT_WORK_TREE), then calculate and save the
> submodule prefix in startup_info struct. Then "git grep" or any
> commands can just read startup_info to find out the submodule prefix.

Here's my first naive attempt at implementing what you describe. Needs
tests, more comments, sign-off etc.

One situation that could be handled better is when the cwd is a
subdirectory of the specified worktree. At the moment this ends up
giving the full path to the worktree, the output would look much nicer
if it gave the relative path (e.g. ../../).

---8<---
From: Chris Packham <judge.packham@gmail.com>
Date: Thu, 30 Sep 2010 11:19:29 -0700
Subject: [RFC PATCH] save the work tree prefix in startup_info

This is the relative path between the cwd and the worktree or the
absolute path of the worktree if the worktree is not a subdirectory
of the worktree.
---
 cache.h |    1 +
 dir.c   |   26 ++++++++++++++++++++++++++
 dir.h   |    1 +
 setup.c |    4 ++++
 4 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/cache.h b/cache.h
index e1d3ffd..f320e78 100644
--- a/cache.h
+++ b/cache.h
@@ -1111,6 +1111,7 @@ const char *split_cmdline_strerror(int cmdline_errno);
 /* git.c */
 struct startup_info {
 	int have_repository;
+	const char *prefix;
 };
 extern struct startup_info *startup_info;

diff --git a/dir.c b/dir.c
index 58ec1a1..2148730 100644
--- a/dir.c
+++ b/dir.c
@@ -1036,6 +1036,32 @@ char *get_relative_cwd(char *buffer, int size,
const char *dir)
 	}
 }

+char *get_relative_wt(char *buffer, int size, const char *dir)
+{
+	char *cwd = buffer;
+	
+	if (!dir)
+		return NULL;
+	if (!getcwd(buffer, size))
+		die_errno("can't find the current directory");
+	if (!is_absolute_path(dir))
+		dir = make_absolute_path(dir);
+	if (strstr(dir, cwd)) {
+		dir += strlen(cwd);
+		switch(*dir){
+		case '\0':
+			return NULL;
+		case '/':
+			dir++;
+			break;
+		default:
+			break;
+		}
+	}
+	strncpy(buffer, dir, size);
+	return buffer;
+}
+
 int is_inside_dir(const char *dir)
 {
 	char buffer[PATH_MAX];
diff --git a/dir.h b/dir.h
index b3e2104..d3c161f 100644
--- a/dir.h
+++ b/dir.h
@@ -81,6 +81,7 @@ extern void add_exclude(const char *string, const char
*base,
 extern int file_exists(const char *);

 extern char *get_relative_cwd(char *buffer, int size, const char *dir);
+extern char *get_relative_wt(char *buffer, int size, const char *dir);
 extern int is_inside_dir(const char *dir);

 static inline int is_dot_or_dotdot(const char *name)
diff --git a/setup.c b/setup.c
index a3b76de..bd9d9fd 100644
--- a/setup.c
+++ b/setup.c
@@ -317,6 +317,7 @@ static const char *setup_explicit_git_dir(const char
*gitdirenv,
 				const char *work_tree_env, int *nongit_ok)
 {
 	static char buffer[1024 + 1];
+	static char wtbuf[1024 + 1];
 	const char *retval;

 	if (PATH_MAX - 40 < strlen(gitdirenv))
@@ -337,6 +338,9 @@ static const char *setup_explicit_git_dir(const char
*gitdirenv,
 	}
 	if (check_repository_format_gently(nongit_ok))
 		return NULL;
+
+	startup_info->prefix=get_relative_wt(wtbuf, sizeof(wtbuf) - 1,
+			get_git_work_tree());
 	retval = get_relative_cwd(buffer, sizeof(buffer) - 1,
 			get_git_work_tree());
 	if (!retval || !*retval)
-- 
1.7.3.1

Re: Re: [RFC PATCH 3/3] grep: add support for grepping in submodules

[Heiko Voigt]

Hi,

On Wed, Sep 29, 2010 at 04:02:01PM -0700, Chris Packham wrote:
> On 29/09/10 15:21, Jens Lehmann wrote:
> > Am 29.09.2010 22:28, schrieb Chris Packham:
> >> When the --recurse-submodules option is given git grep will search in
> >> submodules as they are encountered.
> > 
> > As "git clone" already introduced a "--recursive" option for
> > submodule recursion IMO "--recursive" should be used here too for
> > consistency. (Maybe you took the idea to use "--recurse-submodules"
> > from my "git-checkout-recurse-submodules" branch on github? But that
> > is only used there because I didn't get around to change it yet like
> > I did in the "fetch-submodules-too" branch).
> 
> I actually started with --recursive and switched to
> --recurse-submodules. One thing with this is the standard grep
> --recursive option which may cause some confusion if people expect git
> grep to behave like normal grep. I'll switch to using --recursive for
> now until someone objects to the potential confusion.

How about dropping the option all together and making grep search all
populated submodules by default and maybe have an option to turn it off.
Since git grep is searching recursive by default this would be what I
would expect as a user. Are there other reasons to turn off the search
in submodules than the potential runtime penalty because of forks? 

> One more thought on this that has been hanging around in my mind. I
> sometimes want to do something on all but one submodule, in this case
> with grep I'm fairly likely to want to skip a linux repository because I
> already know the thing I'm looking for is in userland. Maybe in the
> future we can make --recursive take an argument that allows us to
> specify/restrict which submodules get included in the command invocation.

Thinking about this how about not providing a disable submodule
recursion option at all? Just provide an --exclude option and let it be
used transparently for both normal subfolders and submodules?

Cheers Heiko

Re: [RFC PATCH 3/3] grep: add support for grepping in submodules

[Jens Lehmann]

Am 30.09.2010 20:59, schrieb Heiko Voigt:
> How about dropping the option all together and making grep search all
> populated submodules by default and maybe have an option to turn it off.

And that option might be called "--no-recursive"? :-)

But when we add an config setting to control the default behavior
later (which we had to do for all submodule recursion features so far
where we changed the default to recursion) we'll need the "--recursive"
option again anyway to be able to override the config setting. So I
vote for just leaving the option as it is for now, and we can discuss
the proper default as we go along (And in case of grep I have not made
up my mind as to what a sane default would be, personally I'm fine with
having to use the "--recursive" option when I want recursion, but I
won't object to making it the default either).


> Since git grep is searching recursive by default this would be what I
> would expect as a user. Are there other reasons to turn off the search
> in submodules than the potential runtime penalty because of forks? 

The runtime penalty is a *very* important aspect, as we have some
submodule users who have put huge trees into submodules especially to
avoid the performance penalties (see the discussions for recursive
diff and status). So if we change the default, we will have to
provide an config option for that.

Re: [RFC PATCH 2/3] grep: prepare grep for submodules

[Nguyen Thai Ngoc Duy]

On 10/1/10, Chris Packham <judge.packham@gmail.com> wrote:
>  > You can make setup_explicit_git_dir() realize that situation (current
>  > working directory outside $GIT_WORK_TREE), then calculate and save the
>  > submodule prefix in startup_info struct. Then "git grep" or any
>  > commands can just read startup_info to find out the submodule prefix.
>
>
> Here's my first naive attempt at implementing what you describe. Needs
>  tests, more comments, sign-off etc.

Thanks.

>  One situation that could be handled better is when the cwd is a
>  subdirectory of the specified worktree. At the moment this ends up
>  giving the full path to the worktree, the output would look much nicer
>  if it gave the relative path (e.g. ../../).

Hmm.. if cwd is inside a worktree, prefix (the 3rd parameter in
cmd_grep) should be correctly set and "git grep" should also show
relative path. Or are you talking about another command?

>
>  ---8<---
>  From: Chris Packham <judge.packham@gmail.com>
>  Date: Thu, 30 Sep 2010 11:19:29 -0700
>  Subject: [RFC PATCH] save the work tree prefix in startup_info
>
>  This is the relative path between the cwd and the worktree or the
>  absolute path of the worktree if the worktree is not a subdirectory
>  of the worktree.
>  ---
>   cache.h |    1 +
>   dir.c   |   26 ++++++++++++++++++++++++++
>   dir.h   |    1 +
>   setup.c |    4 ++++
>   4 files changed, 32 insertions(+), 0 deletions(-)
>
>  diff --git a/cache.h b/cache.h
>  index e1d3ffd..f320e78 100644
>  --- a/cache.h
>  +++ b/cache.h
>  @@ -1111,6 +1111,7 @@ const char *split_cmdline_strerror(int cmdline_errno);
>   /* git.c */
>   struct startup_info {
>         int have_repository;
>  +       const char *prefix;

You should use another name here to avoid confusion with the current
prefix, relative to worktree toplevel directory. I'm thinking of
outer_prefix or cwd_prefix, but I'm usually bad at naming.

>   };
>   extern struct startup_info *startup_info;
>
>  diff --git a/dir.c b/dir.c
>  index 58ec1a1..2148730 100644
>  --- a/dir.c
>  +++ b/dir.c
>  @@ -1036,6 +1036,32 @@ char *get_relative_cwd(char *buffer, int size,
>  const char *dir)
>         }
>   }
>
>  +char *get_relative_wt(char *buffer, int size, const char *dir)
>  +{
>  +       char *cwd = buffer;
>  +
>  +       if (!dir)
>  +               return NULL;
>  +       if (!getcwd(buffer, size))
>  +               die_errno("can't find the current directory");
>  +       if (!is_absolute_path(dir))
>  +               dir = make_absolute_path(dir);
>  +       if (strstr(dir, cwd)) {

Why not strncmp?

>  +               dir += strlen(cwd);
>  +               switch(*dir){
>  +               case '\0':
>  +                       return NULL;
>  +               case '/':
>  +                       dir++;
>  +                       break;

Yeah.

>  +               default:
>  +                       break;

Should we properly handle relative path that includes ".." here too?

>  +               }
>  +       }
>  +       strncpy(buffer, dir, size);

So if "cwd" is inside "dir", an absolute "dir" is returned? That does
not look like a prefix to me.

>  +       return buffer;
>  +}
>  +
>   int is_inside_dir(const char *dir)
>   {
>         char buffer[PATH_MAX];
>  diff --git a/dir.h b/dir.h
>  index b3e2104..d3c161f 100644
>  --- a/dir.h
>  +++ b/dir.h
>  @@ -81,6 +81,7 @@ extern void add_exclude(const char *string, const char
>  *base,
>   extern int file_exists(const char *);
>
>   extern char *get_relative_cwd(char *buffer, int size, const char *dir);
>  +extern char *get_relative_wt(char *buffer, int size, const char *dir);
>   extern int is_inside_dir(const char *dir);
>
>   static inline int is_dot_or_dotdot(const char *name)
>  diff --git a/setup.c b/setup.c
>  index a3b76de..bd9d9fd 100644
>  --- a/setup.c
>  +++ b/setup.c
>  @@ -317,6 +317,7 @@ static const char *setup_explicit_git_dir(const char
>  *gitdirenv,
>                                 const char *work_tree_env, int *nongit_ok)
>   {
>         static char buffer[1024 + 1];
>  +       static char wtbuf[1024 + 1];
>         const char *retval;
>
>         if (PATH_MAX - 40 < strlen(gitdirenv))
>  @@ -337,6 +338,9 @@ static const char *setup_explicit_git_dir(const char
>  *gitdirenv,
>         }
>         if (check_repository_format_gently(nongit_ok))
>                 return NULL;
>  +
>  +       startup_info->prefix=get_relative_wt(wtbuf, sizeof(wtbuf) - 1,
>  +                       get_git_work_tree());
>         retval = get_relative_cwd(buffer, sizeof(buffer) - 1,
>                         get_git_work_tree());
>         if (!retval || !*retval)
>
> --
>  1.7.3.1
>
>


-- 
Duy

Re: [RFC PATCH 2/3] grep: prepare grep for submodules

[Chris Packham]

On 01/10/10 07:37, Nguyen Thai Ngoc Duy wrote:
> On 10/1/10, Chris Packham <judge.packham@gmail.com> wrote:
>>  > You can make setup_explicit_git_dir() realize that situation (current
>>  > working directory outside $GIT_WORK_TREE), then calculate and save the
>>  > submodule prefix in startup_info struct. Then "git grep" or any
>>  > commands can just read startup_info to find out the submodule prefix.
>>
>>
>> Here's my first naive attempt at implementing what you describe. Needs
>>  tests, more comments, sign-off etc.
> 
> Thanks.
> 
>>  One situation that could be handled better is when the cwd is a
>>  subdirectory of the specified worktree. At the moment this ends up
>>  giving the full path to the worktree, the output would look much nicer
>>  if it gave the relative path (e.g. ../../).
> 
> Hmm.. if cwd is inside a worktree, prefix (the 3rd parameter in
> cmd_grep) should be correctly set and "git grep" should also show
> relative path. Or are you talking about another command?

I was testing this with grep but also with my submodule changes. I
should probably move this to its own topic branch and get it working
then rebase my grep changes on top of it.

>>
>>  ---8<---
>>  From: Chris Packham <judge.packham@gmail.com>
>>  Date: Thu, 30 Sep 2010 11:19:29 -0700
>>  Subject: [RFC PATCH] save the work tree prefix in startup_info
>>
>>  This is the relative path between the cwd and the worktree or the
>>  absolute path of the worktree if the worktree is not a subdirectory
>>  of the worktree.
>>  ---
>>   cache.h |    1 +
>>   dir.c   |   26 ++++++++++++++++++++++++++
>>   dir.h   |    1 +
>>   setup.c |    4 ++++
>>   4 files changed, 32 insertions(+), 0 deletions(-)
>>
>>  diff --git a/cache.h b/cache.h
>>  index e1d3ffd..f320e78 100644
>>  --- a/cache.h
>>  +++ b/cache.h
>>  @@ -1111,6 +1111,7 @@ const char *split_cmdline_strerror(int cmdline_errno);
>>   /* git.c */
>>   struct startup_info {
>>         int have_repository;
>>  +       const char *prefix;
> 
> You should use another name here to avoid confusion with the current
> prefix, relative to worktree toplevel directory. I'm thinking of
> outer_prefix or cwd_prefix, but I'm usually bad at naming.

I couldn't come up with a better name either, that’s why I used
"prefix". "cwd_prefix" seems sensible enough to me.

>>   };
>>   extern struct startup_info *startup_info;
>>
>>  diff --git a/dir.c b/dir.c
>>  index 58ec1a1..2148730 100644
>>  --- a/dir.c
>>  +++ b/dir.c
>>  @@ -1036,6 +1036,32 @@ char *get_relative_cwd(char *buffer, int size,
>>  const char *dir)
>>         }
>>   }
>>
>>  +char *get_relative_wt(char *buffer, int size, const char *dir)
>>  +{
>>  +       char *cwd = buffer;
>>  +
>>  +       if (!dir)
>>  +               return NULL;
>>  +       if (!getcwd(buffer, size))
>>  +               die_errno("can't find the current directory");
>>  +       if (!is_absolute_path(dir))
>>  +               dir = make_absolute_path(dir);
>>  +       if (strstr(dir, cwd)) {
> 
> Why not strncmp?

I actually tried strcmp first, expecting to get a number that I can
increment dir by. What I actually got was -1, maybe I just screwed up
the order of dir and cwd. I'll look into it.

> 
>>  +               dir += strlen(cwd);
>>  +               switch(*dir){
>>  +               case '\0':
>>  +                       return NULL;
>>  +               case '/':
>>  +                       dir++;
>>  +                       break;
> 
> Yeah.
> 
>>  +               default:
>>  +                       break;
> 
> Should we properly handle relative path that includes ".." here too? 

By now dir and cwd are both absolute paths. So I dn't think there is
anything to handle

>>  +               }
>>  +       }
>>  +       strncpy(buffer, dir, size);
> 
> So if "cwd" is inside "dir", an absolute "dir" is returned? That does
> not look like a prefix to me.

That is a problem. Maybe I should be returning NULL in that case and let
the existing code handle the cwd inside dir case. I think if I wrote
some tests first I could see the various permutations better.

>>  +       return buffer;
>>  +}
>>  +
>>   int is_inside_dir(const char *dir)
>>   {
>>         char buffer[PATH_MAX];
>>  diff --git a/dir.h b/dir.h
>>  index b3e2104..d3c161f 100644
>>  --- a/dir.h
>>  +++ b/dir.h
>>  @@ -81,6 +81,7 @@ extern void add_exclude(const char *string, const char
>>  *base,
>>   extern int file_exists(const char *);
>>
>>   extern char *get_relative_cwd(char *buffer, int size, const char *dir);
>>  +extern char *get_relative_wt(char *buffer, int size, const char *dir);
>>   extern int is_inside_dir(const char *dir);
>>
>>   static inline int is_dot_or_dotdot(const char *name)
>>  diff --git a/setup.c b/setup.c
>>  index a3b76de..bd9d9fd 100644
>>  --- a/setup.c
>>  +++ b/setup.c
>>  @@ -317,6 +317,7 @@ static const char *setup_explicit_git_dir(const char
>>  *gitdirenv,
>>                                 const char *work_tree_env, int *nongit_ok)
>>   {
>>         static char buffer[1024 + 1];
>>  +       static char wtbuf[1024 + 1];
>>         const char *retval;
>>
>>         if (PATH_MAX - 40 < strlen(gitdirenv))
>>  @@ -337,6 +338,9 @@ static const char *setup_explicit_git_dir(const char
>>  *gitdirenv,
>>         }
>>         if (check_repository_format_gently(nongit_ok))
>>                 return NULL;
>>  +
>>  +       startup_info->prefix=get_relative_wt(wtbuf, sizeof(wtbuf) - 1,
>>  +                       get_git_work_tree());
>>         retval = get_relative_cwd(buffer, sizeof(buffer) - 1,
>>                         get_git_work_tree());
>>         if (!retval || !*retval)
>>
>> --
>>  1.7.3.1
>>
>>
> 
>