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

[Jens Lehmann <Jens.Lehmann@web.de>]

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