Re: [PATCH 1/3] grep: prepare for new header field filter
On Sat, Sep 29, 2012 at 11:41:27AM +0700, Nguyen Thai Ngoc Duy wrote: > 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] grep: prepare for new header field filter
grep supports only author and committer headers, which have the same special treatment that later headers may or may not have. Check for field type and only strip_timestamp() when the field is either author or committer. GREP_HEADER_FIELD_MAX is put in the grep_header_field enum to be calculated automatically, correctly, as long as it's at the end of the enum. Signed-off-by: Nguyễn Thái Ngọc Duy --- grep.c | 9 - grep.h | 6 -- t/t7810-grep.sh | 12 3 files changed, 24 insertions(+), 3 deletions(-) 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; + } } again: diff --git a/grep.h b/grep.h index 8a28a67..d54adbe 100644 --- a/grep.h +++ b/grep.h @@ -29,9 +29,11 @@ enum grep_context { enum grep_header_field { GREP_HEADER_AUTHOR = 0, - GREP_HEADER_COMMITTER + GREP_HEADER_COMMITTER, + + /* Must be at the end of the enum */ + GREP_HEADER_FIELD_MAX }; -#define GREP_HEADER_FIELD_MAX (GREP_HEADER_COMMITTER + 1) struct grep_pat { struct grep_pat *next; diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh index 91db352..30eaa9a 100755 --- a/t/t7810-grep.sh +++ b/t/t7810-grep.sh @@ -628,6 +628,18 @@ test_expect_success 'log --all-match --grep --grep --author takes intersection' test_cmp expect actual ' +test_expect_success 'log --author does not search in timestamp' ' + : >expect && + git log --author="$GIT_AUTHOR_DATE" >actual && + test_cmp expect actual +' + +test_expect_success 'log --committer does not search in timestamp' ' + : >expect && + git log --committer="$GIT_COMMITTER_DATE" >actual && + test_cmp expect actual +' + test_expect_success 'grep with CE_VALID file' ' git update-index --assume-unchanged t/t && rm t/t && -- 1.7.12.1.406.g6ab07c4 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html