[RFC PATCH 0/3] grep: submodule support

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

Signed-off-by: Chris Packham <redacted>
---
 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

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

Signed-off-by: Chris Packham <redacted>
---
 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

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

Signed-off-by: Chris Packham <redacted>
---
 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

On Wed, Sep 29, 2010 at 20:28, Chris Packham [off-list ref] wrote:

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.

On 29/09/10 13:35, Ævar Arnfjörð Bjarmason wrote:

On Wed, Sep 29, 2010 at 20:28, Chris Packham [off-list ref] wrote:

quoted

Signed-off-by: Chris Packham <redacted>
---
 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.

quoted

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

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

quoted

+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

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

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.

On 29/09/10 15:21, Jens Lehmann wrote:

Nice start!


Am 29.09.2010 22:28, schrieb Chris Packham:

quoted

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.

quoted

Signed-off-by: Chris Packham <redacted>
---
 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.

quoted

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

quoted

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

quoted

+		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);

On Thu, Sep 30, 2010 at 6:28 AM, Chris Packham [off-list ref] 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

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?

quoted

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.

On 30/09/10 04:24, Jens Lehmann wrote:

Am 30.09.2010 01:02, schrieb Chris Packham:

quoted

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.

On 29/09/10 18:10, Nguyen Thai Ngoc Duy wrote:

On Thu, Sep 30, 2010 at 6:28 AM, Chris Packham [off-list ref] wrote:

quoted

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

Hi,

On Wed, Sep 29, 2010 at 04:02:01PM -0700, Chris Packham wrote:

On 29/09/10 15:21, Jens Lehmann wrote:

quoted

Am 29.09.2010 22:28, schrieb Chris Packham:

quoted

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

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.

On 10/1/10, Chris Packham [off-list ref] 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 [off-list ref]
 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

On 01/10/10 07:37, Nguyen Thai Ngoc Duy wrote:

On 10/1/10, Chris Packham [off-list ref] wrote:

quoted

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

quoted

 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.

quoted

 ---8<---
 From: Chris Packham [off-list ref]
 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.

quoted

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

quoted

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

Yeah.

quoted

 +               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

quoted

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

quoted

 +       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