Re: [PATCH 1/3] grep: prepare for new header field filter

From: Jeff King <peff@peff.net>
Date: Sep 29, 2012, 05:22 UTC
Message-ID: <20120929052202.GA3330@sigill.intra.peff.net>
In-Reply-To: <1348893689-20240-2-git-send-email-pclouds@gmail.com>

On Sat, Sep 29, 2012 at 11:41:27AM +0700, Nguyen Thai Ngoc Duy wrote:

Show 17 quoted lines

> diff --git a/grep.c b/grep.c
> index 898be6e..8d73995 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -720,7 +720,14 @@ static int match_one_pattern(struct grep_pat *p, char *bol, char *eol,
>  		if (strncmp(bol, field, len))
>  			return 0;
>  		bol += len;
> -		saved_ch = strip_timestamp(bol, &eol);
> +		switch (p->field) {
> +		case GREP_HEADER_AUTHOR:
> +		case GREP_HEADER_COMMITTER:
> +			saved_ch = strip_timestamp(bol, &eol);
> +			break;
> +		default:
> +			break;
> +		}

Reading this hunk, I wondered what happens to saved_ch if we do not set it here. Fortunately it is initialized to 0, as we already have to handle the non-header case. Then later we do this, which does introduce a new condition (saved_ch was not set, but we trigger the first half of the conditional):

      if (p->token == GREP_PATTERN_HEAD && saved_ch)
                *eol = saved_ch;

However, the second half of that conditional (which previously was only triggered when we tried to split the timestamp, but there was a bogus line with no ">" on it) prevents us from overwriting *eol.

So I think it is good, but it was non-obvious enough that I wanted to save other reviewers from investigating it. The rest of the patch looks good to me, as well.

-Peff