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

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