From: Jonathan Tan <jonathantanmy@google.com>
Date: Fri, 11 Nov 2016 23:09:15 GMT
Subject: Re: [PATCH v2 5/6] grep: enable recurse-submodules to work on <tree> objects
Message-ID: <36dd3a9b-39ae-4c5f-90d6-589820501ed4@google.com>
In-Reply-To: <1477953496-103596-6-git-send-email-bmwill@google.com>

On 10/31/2016 03:38 PM, Brandon Williams wrote:
> diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
> index 17aa1ba..386a868 100644
> --- a/Documentation/git-grep.txt
> +++ b/Documentation/git-grep.txt
> @@ -26,7 +26,7 @@ SYNOPSIS
>  	   [--threads <num>]
>  	   [-f <file>] [-e] <pattern>
>  	   [--and|--or|--not|(|)|-e <pattern>...]
> -	   [--recurse-submodules]
> +	   [--recurse-submodules] [--parent-basename]

Maybe add something after --parent-basename, since it takes an argument 
(like --threads above).

> @@ -91,7 +91,16 @@ OPTIONS
>
>  --recurse-submodules::
>  	Recursively search in each submodule that has been initialized and
> -	checked out in the repository.
> +	checked out in the repository.  When used in combination with the
> +	<tree> option the prefix of all submodule output will be the name of
> +	the parent project's <tree> object.
> +
> +--parent-basename::

Same comment as above.

> diff --git a/builtin/grep.c b/builtin/grep.c
> index cf4f51e..2f10930 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -19,6 +19,7 @@
>  #include "dir.h"
>  #include "pathspec.h"
>  #include "submodule.h"
> +#include "submodule-config.h"
>
>  static char const * const grep_usage[] = {
>  	N_("git grep [<options>] [-e] <pattern> [<rev>...] [[--] <path>...]"),
> @@ -28,6 +29,7 @@ static char const * const grep_usage[] = {
>  static const char *super_prefix;
>  static int recurse_submodules;
>  static struct argv_array submodule_options = ARGV_ARRAY_INIT;
> +static const char *parent_basename;

Can this be passed as an argument to the functions (grep_objects and 
grep_object, it seems) instead of having a file-visible variable?

> @@ -671,12 +707,29 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,
>  	enum interesting match = entry_not_interesting;
>  	struct name_entry entry;
>  	int old_baselen = base->len;
> +	struct strbuf name = STRBUF_INIT;
> +	int name_base_len = 0;
> +	if (super_prefix) {
> +		name_base_len = strlen(super_prefix);
> +		strbuf_addstr(&name, super_prefix);

Better to invoke strbuf_addstr, and then set name_base_len from 
name.len. This makes it clear where strbuf_setlen (subsequently) resets 
the strbuf to, and is also a slight performance improvement.