git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Chris Packham <judge.packham@gmail.com>
Date
Sep 29, 2010, 23:02 UTC
Message-ID
<4CA3C569.4020309@gmail.com>
In-Reply-To
<4CA3BBD7.3090006@web.de>
On 29/09/10 15:21, Jens Lehmann wrote:
Show 13 quoted lines
> 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.

Show 40 quoted lines
> 
>> 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.

Show 6 quoted lines
> 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.

Show 21 quoted lines
> 
>> +	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.

Show 23 quoted lines
>> +
>> +	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.
Show 58 quoted lines
>> +		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);
>
Previous: Jens LehmannNext: Jens Lehmann
Message 18 of 22 in “grep: submodule support”
  1. 0/3 grep: submodule supportChris Packham, Sep 29, 2010
  2. 1/3 add test for git grep --recursiveChris Packham, Sep 29, 2010
  3. Ævar Arnfjörð BjarmasonSep 29, 2010
  4. Chris PackhamSep 29, 2010
  5. Kevin BallardSep 29, 2010
  6. 2/3 grep: prepare grep for submodulesChris Packham, Sep 29, 2010
  7. Nguyen Thai Ngoc DuySep 30, 2010
  8. Chris PackhamSep 30, 2010
  9. Nguyen Thai Ngoc DuyOct 1, 2010
  10. Chris PackhamOct 1, 2010
  11. 3/3 grep: add support for grepping in submodulesChris Packham, Sep 29, 2010
  12. Jens LehmannSep 29, 2010
  13. Junio C HamanoSep 29, 2010
  14. Chris PackhamSep 29, 2010
  15. Jens LehmannSep 30, 2010
  16. Johannes SixtSep 30, 2010
  17. Jens LehmannSep 30, 2010
  18. Chris PackhamSep 29, 2010
  19. Jens LehmannSep 30, 2010
  20. Chris PackhamSep 30, 2010
  21. Heiko VoigtSep 30, 2010
  22. Jens LehmannSep 30, 2010

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.