Subject: Re: [PATCH v2 3/6] grep: add submodules as a grep source type

From: Junio C Hamano <[email-masked]>
Date: Tue, 01 Nov 2016 10:31:24 -0700

Brandon Williams <[email-masked]> writes:

> Add `GREP_SOURCE_SUBMODULE` as a grep_source type and cases for this new
> type in the various switch statements in grep.c.
>
> When initializing a grep_source with type `GREP_SOURCE_SUBMODULE` the
> identifier can either be NULL (to indicate that the working tree will be
> used) or a SHA1 (the REV of the submodule to be grep'd).  If the
> identifier is a SHA1 then we want to fall through to the
> `GREP_SOURCE_SHA1` case to handle the copying of the SHA1.
>
> Signed-off-by: Brandon Williams <[email-masked]>
> ---

Conceptually, it somehow feels strange to have SUBMODULE in this
set.

Source being SHA1 means we are doing a recursive grep in a tree
structure that is stored in the object store, being FILE means we
are reading from the filesystem, being BUF means we are fed in-core
buffer (e.g. to implement the "log --grep='string in message'").  It
is unclear how SUBMODULE fits in that picture, as we do not have a
caller that uses the type at this step yet.  Hopefully it will
become obvious why this new type belongs to that set as the series
progresses ;-)

>  grep.c | 16 +++++++++++++++-
>  grep.h |  1 +
>  2 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/grep.c b/grep.c
> index 1194d35..0dbdc1d 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -1735,12 +1735,23 @@ void grep_source_init(struct grep_source *gs, enum grep_source_type type,
>  	case GREP_SOURCE_FILE:
>  		gs->identifier = xstrdup(identifier);
>  		break;
> +	case GREP_SOURCE_SUBMODULE:
> +		if (!identifier) {
> +			gs->identifier = NULL;
> +			break;
> +		}
> +		/*
> +		 * FALL THROUGH
> +		 * If the identifier is non-NULL (in the submodule case) it
> +		 * will be a SHA1 that needs to be copied.
> +		 */
>  	case GREP_SOURCE_SHA1:
>  		gs->identifier = xmalloc(20);
>  		hashcpy(gs->identifier, identifier);
>  		break;
>  	case GREP_SOURCE_BUF:
>  		gs->identifier = NULL;
> +		break;
>  	}
>  }
>  
> @@ -1760,6 +1771,7 @@ void grep_source_clear_data(struct grep_source *gs)
>  	switch (gs->type) {
>  	case GREP_SOURCE_FILE:
>  	case GREP_SOURCE_SHA1:
> +	case GREP_SOURCE_SUBMODULE:
>  		free(gs->buf);
>  		gs->buf = NULL;
>  		gs->size = 0;
> @@ -1831,8 +1843,10 @@ static int grep_source_load(struct grep_source *gs)
>  		return grep_source_load_sha1(gs);
>  	case GREP_SOURCE_BUF:
>  		return gs->buf ? 0 : -1;
> +	case GREP_SOURCE_SUBMODULE:
> +		break;
>  	}
> -	die("BUG: invalid grep_source type");
> +	die("BUG: invalid grep_source type to load");
>  }
>  
>  void grep_source_load_driver(struct grep_source *gs)
> diff --git a/grep.h b/grep.h
> index 5856a23..267534c 100644
> --- a/grep.h
> +++ b/grep.h
> @@ -161,6 +161,7 @@ struct grep_source {
>  		GREP_SOURCE_SHA1,
>  		GREP_SOURCE_FILE,
>  		GREP_SOURCE_BUF,
> +		GREP_SOURCE_SUBMODULE,
>  	} type;
>  	void *identifier;