git rev-list --author/--committer b0rked with -F/--fixed-strings

git rev-list --author/--committer b0rked with -F/--fixed-strings

[Giuseppe Bilotta]

Hello all,

I was recently looking into the reason why the gitweb search box
failed to provide results when using the author and committer
search and NOT using regexp searches.

I managed to track the fault down to the inability for git
rev-list to handle --author and --committer in conjunction with
the --fixed-strings option.

It's not difficult to see that the reason for this is that the
author and committer search patterns are *always* built as regular
expressions. However, I couldn't see any obvious way to do things
in a *different* way ...

So, is this a known bug (I would call it a bug, yes) in rev-list?
Are there any ideas around on how to fix it?

-- 
Giuseppe "Oblomov" Bilotta

Re: git rev-list --author/--committer b0rked with -F/--fixed-strings

[Junio C Hamano]

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:

> So, is this a known bug (I would call it a bug, yes) in rev-list?
> Are there any ideas around on how to fix it?

It depends on what you are trying to do.  An obvious thing to do is to
accept the fact that it has always been a regexp match, and quotemeta your
input string.

Because we allow you to give "-F" and "--author=foo" in any order, looking
at the options we have seen already and adjusting the pattern depending on
the fixed-string option in the code would not be workable.  You would have
to instead queue up all the --grep/--author/--committer options until the
very end and _then_ compile them at the end, after you saw all the other
grep related options such as -i/-F/-E

I am not interested in helping people with attitude very much ;-), but one
somewhat related thing I have been wanting to add was the change in the
attached patch.  It allows you to anchor not just at the beginning but at
the end.

 revision.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git c/revision.c w/revision.c
index 36291b6..83478ef 100644
--- c/revision.c
+++ w/revision.c
@@ -956,18 +956,24 @@ static void add_grep(struct rev_info *revs, const char *ptn, enum grep_pat_token
 static void add_header_grep(struct rev_info *revs, const char *field, const char *pattern)
 {
 	char *pat;
-	const char *prefix;
+	const char *prefix, *suffix;
 	int patlen, fldlen;
 
 	fldlen = strlen(field);
 	patlen = strlen(pattern);
-	pat = xmalloc(patlen + fldlen + 10);
-	prefix = ".*";
+	pat = xmalloc(patlen + fldlen + 64);
+	suffix = prefix = ".*";
 	if (*pattern == '^') {
 		prefix = "";
 		pattern++;
+		patlen--;
 	}
-	sprintf(pat, "^%s %s%s", field, prefix, pattern);
+	if (pattern[patlen-1] == '$') {
+		suffix = "";
+		patlen--;
+	}
+	sprintf(pat, "^%s %s%.*s%s [1-9][0-9]* [-+][0-1][0-9][0-9][0-9]$",
+		field, prefix, patlen, pattern, suffix);
 	add_grep(revs, pat, GREP_PATTERN_HEAD);
 }
 

[PATCH] Teach "log -F --author=<match>" to behave better

[Junio C Hamano]

To handle --author=<match> request, the code created a grep instruction
that tried to match a line that begins with 'author ' and <match>
somewhere on the same line.  "begins with 'author '" obviously needs to be
expressed with an regexp '^author '.

When the user specifies --fixed-string, this does not work at all.

This extends the grep machinery so that a match insn can ignore user
specified --fixed-string request, and uses the '( -e A --and -e B )'
construct from the grep machinery in order to express "has to begin with
'^author ', and also the same line must match the given pattern".

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * A variant of this would be to make the --author match always ignore -F,
   which is now possible with the enhancement to the grep machinery.  But
   it is unlikely that you would have regexp metacharacters in author
   field, so it might not be necessary.

   "log -F --author=1220485514" would match the timestamp part of the
   author line.  This will not likely to change but it is not likely to
   cause issues in real world either, so I would not bother to address it
   myself.

 grep.c     |   16 ++++++++++++----
 grep.h     |    3 +++
 revision.c |   44 ++++++++++++++++++++++++++++++++++++++++++++
 revision.h |    7 +++++++
 4 files changed, 66 insertions(+), 4 deletions(-)

diff --git c/grep.c w/grep.c
index f67d671..dd71b78 100644
--- c/grep.c
+++ w/grep.c
@@ -2,19 +2,27 @@
 #include "grep.h"
 #include "xdiff-interface.h"
 
-void append_grep_pattern(struct grep_opt *opt, const char *pat,
-			 const char *origin, int no, enum grep_pat_token t)
+void append_grep_pattern_special(struct grep_opt *opt, const char *pat,
+				const char *origin, int no,
+				enum grep_pat_token t, unsigned flags)
 {
 	struct grep_pat *p = xcalloc(1, sizeof(*p));
 	p->pattern = pat;
 	p->origin = origin;
 	p->no = no;
 	p->token = t;
+	p->flags = flags;
 	*opt->pattern_tail = p;
 	opt->pattern_tail = &p->next;
 	p->next = NULL;
 }
 
+void append_grep_pattern(struct grep_opt *opt, const char *pat,
+			 const char *origin, int no, enum grep_pat_token t)
+{
+	append_grep_pattern_special(opt, pat, origin, no, t, 0);
+}
+
 static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
 {
 	int err = regcomp(&p->regexp, p->pattern, opt->regflags);
@@ -146,7 +154,7 @@ void compile_grep_patterns(struct grep_opt *opt)
 		case GREP_PATTERN: /* atom */
 		case GREP_PATTERN_HEAD:
 		case GREP_PATTERN_BODY:
-			if (!opt->fixed)
+			if (!opt->fixed || (p->flags & GREP_ALWAYS_REGEXP))
 				compile_regexp(p, opt);
 			break;
 		default:
@@ -258,7 +266,7 @@ static int match_one_pattern(struct grep_opt *opt, struct grep_pat *p, char *bol
 		return 0;
 
  again:
-	if (!opt->fixed) {
+	if (!opt->fixed || (p->flags & GREP_ALWAYS_REGEXP)) {
 		regex_t *exp = &p->regexp;
 		hit = !regexec(exp, bol, ARRAY_SIZE(pmatch),
 			       pmatch, 0);
diff --git c/grep.h w/grep.h
index d252dd2..b03d89d 100644
--- c/grep.h
+++ w/grep.h
@@ -21,6 +21,8 @@ struct grep_pat {
 	struct grep_pat *next;
 	const char *origin;
 	int no;
+	unsigned flags;
+#define GREP_ALWAYS_REGEXP 01
 	enum grep_pat_token token;
 	const char *pattern;
 	regex_t regexp;
@@ -73,6 +75,7 @@ struct grep_opt {
 	unsigned post_context;
 };
 
+extern void append_grep_pattern_special(struct grep_opt *opt, const char *pat, const char *origin, int no, enum grep_pat_token t, unsigned);
 extern void append_grep_pattern(struct grep_opt *opt, const char *pat, const char *origin, int no, enum grep_pat_token t);
 extern void compile_grep_patterns(struct grep_opt *opt);
 extern void free_grep_patterns(struct grep_opt *opt);
diff --git c/revision.c w/revision.c
index 83478ef..65d22f5 100644
--- c/revision.c
+++ w/revision.c
@@ -955,6 +955,17 @@ static void add_grep(struct rev_info *revs, const char *ptn, enum grep_pat_token
 
 static void add_header_grep(struct rev_info *revs, const char *field, const char *pattern)
 {
+	struct pending_header_grep *p = xcalloc(1, sizeof(*p));
+	p->next = revs->pending_header_grep;
+	p->field = field;
+	p->pattern = pattern;
+	revs->pending_header_grep = p;;
+}
+
+static void add_header_grep_regexp(struct rev_info *revs, struct pending_header_grep *p)
+{
+	const char *field = p->field;
+	const char *pattern = p->pattern;
 	char *pat;
 	const char *prefix, *suffix;
 	int patlen, fldlen;
@@ -977,6 +988,38 @@ static void add_header_grep(struct rev_info *revs, const char *field, const char
 	add_grep(revs, pat, GREP_PATTERN_HEAD);
 }
 
+static void add_header_grep_fixed(struct rev_info *revs, struct pending_header_grep *p)
+{
+	char *field = xmalloc(strlen(p->field) + 2);
+	strcpy(field + 1, p->field);
+	field[0] = '^';
+	append_grep_pattern(&revs->grep_filter, "(",
+			    "header grep", 0, GREP_OPEN_PAREN);
+	append_grep_pattern_special(&revs->grep_filter, field,
+				    "header grep", 0, GREP_PATTERN_HEAD,
+				    GREP_ALWAYS_REGEXP);
+	append_grep_pattern(&revs->grep_filter, "--and",
+			    "header grep", 0, GREP_AND);
+	add_grep(revs, p->pattern, GREP_PATTERN_HEAD);
+	append_grep_pattern(&revs->grep_filter, ")",
+			    "header grep", 0, GREP_CLOSE_PAREN);
+}
+
+static void finish_header_grep(struct rev_info *revs)
+{
+	struct pending_header_grep *q, *p = revs->pending_header_grep;
+
+	while (p) {
+		if (revs->grep_filter.fixed)
+			add_header_grep_fixed(revs, p);
+		else
+			add_header_grep_regexp(revs, p);
+		q = p->next;
+		free(p);
+		p = q;
+	}
+}
+
 static void add_message_grep(struct rev_info *revs, const char *pattern)
 {
 	add_grep(revs, pattern, GREP_PATTERN_BODY);
@@ -1347,6 +1390,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
 	if (diff_setup_done(&revs->diffopt) < 0)
 		die("diff_setup_done failed");
 
+	finish_header_grep(revs);
 	compile_grep_patterns(&revs->grep_filter);
 
 	if (revs->reverse && revs->reflog_info)
diff --git c/revision.h w/revision.h
index 91f1944..f9cfa26 100644
--- c/revision.h
+++ w/revision.h
@@ -18,6 +18,12 @@
 struct rev_info;
 struct log_info;
 
+struct pending_header_grep {
+	struct pending_header_grep *next;
+	const char *field;
+	const char *pattern;
+};
+
 struct rev_info {
 	/* Starting list */
 	struct commit_list *commits;
@@ -94,6 +100,7 @@ struct rev_info {
 
 	/* Filter by commit log message */
 	struct grep_opt	grep_filter;
+	struct pending_header_grep *pending_header_grep;
 
 	/* Display history graph */
 	struct git_graph *graph;

Re: [PATCH] Teach "log -F --author=<match>" to behave better

[Teemu Likonen]

Junio C Hamano wrote (2008-09-04 01:31 -0700):

> To handle --author=<match> request, the code created a grep
> instruction that tried to match a line that begins with 'author ' and
> <match> somewhere on the same line.  "begins with 'author '" obviously
> needs to be expressed with an regexp '^author '.
> 
> When the user specifies --fixed-string, this does not work at all.

> This extends the grep machinery so that a match insn can ignore user
> specified --fixed-string request, and uses the '( -e A --and -e B )'
> construct from the grep machinery in order to express "has to begin
> with '^author ', and also the same line must match the given pattern".

I want to add a side note here because this _may_ be related to what 
I found almost two months ago:

    "Patterns work unexpectedly with 'git log' commit limiting"
    http://thread.gmane.org/gmane.comp.version-control.git/88813

One of the problems was that with

    git log -E --author=pattern

the pattern is interpreted as basic regexp but with 

    git log --author=pattern -E

as extended rexexp.

Re: [PATCH] Teach "log -F --author=<match>" to behave better

[Junio C Hamano]

Teemu Likonen <tlikonen@iki.fi> writes:

> One of the problems was that with
>
>     git log -E --author=pattern
>
> the pattern is interpreted as basic regexp but with 
>
>     git log --author=pattern -E
>
> as extended rexexp.

I think Jeff fixed that one already on 'maint'.

Re: [PATCH] Teach "log -F --author=<match>" to behave better

[Teemu Likonen]

Junio C Hamano wrote (2008-09-04 12:06 -0700):

> Teemu Likonen <tlikonen@iki.fi> writes:
> 
> > One of the problems was that with
> >
> >     git log -E --author=pattern
> >
> > the pattern is interpreted as basic regexp but with 
> >
> >     git log --author=pattern -E
> >
> > as extended rexexp.
> 
> I think Jeff fixed that one already on 'maint'.

You think right, it's indeed fixed. Great!

Re: [PATCH] Teach "log -F --author=<match>" to behave better

[Junio C Hamano]

Teemu Likonen <tlikonen@iki.fi> writes:

> Junio C Hamano wrote (2008-09-04 12:06 -0700):
>
>> I think Jeff fixed that one already on 'maint'.
>
> You think right, it's indeed fixed. Great!

I went back to your list, and I think all of them now have solutions.

 1. Option order changes the behaviour. "git log -E --author" ...

Jeff fixed this when he did "log -i --author".

 2. ... --author='@iki.fi>$'

$gmane/94900 would be a fix; needs commit log, documentation and tests.

 3. ... -F (--fixed-strings) when combined with --author= ?

We just talked about it; the patch should go to 'maint' with tests.

 4. Logical AND/OR operation. ... combined together do not limit ...

$gmane/94953 look for --all-match.

Re: [PATCH] Teach "log -F --author=<match>" to behave better

[Giuseppe Bilotta]

On Thu, Sep 4, 2008 at 10:31 AM, Junio C Hamano <gitster@pobox.com> wrote:
> This extends the grep machinery so that a match insn can ignore user
> specified --fixed-string request, and uses the '( -e A --and -e B )'

(insn?)

>  * A variant of this would be to make the --author match always ignore -F,
>   which is now possible with the enhancement to the grep machinery.  But
>   it is unlikely that you would have regexp metacharacters in author
>   field, so it might not be necessary.

Well,  if the user greps for a dotted email address, not escaping the
search string might cause some false positives to pop up.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH] Teach "log -F --author=<match>" to behave better

[Petr Baudis]

On Thu, Sep 04, 2008 at 01:31:19AM -0700, Junio C Hamano wrote:
> To handle --author=<match> request, the code created a grep instruction
> that tried to match a line that begins with 'author ' and <match>
> somewhere on the same line.  "begins with 'author '" obviously needs to be
> expressed with an regexp '^author '.
> 
> When the user specifies --fixed-string, this does not work at all.
> 
> This extends the grep machinery so that a match insn can ignore user
> specified --fixed-string request, and uses the '( -e A --and -e B )'
> construct from the grep machinery in order to express "has to begin with
> '^author ', and also the same line must match the given pattern".
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Wow, this is clever solution! FWIW,

Acked-by: Petr Baudis <pasky@suse.cz>

There goes one of my long-standing TODO items. :-)

Re: [PATCH] Teach "log -F --author=<match>" to behave better

[Junio C Hamano]

Petr Baudis <pasky@suse.cz> writes:

> On Thu, Sep 04, 2008 at 01:31:19AM -0700, Junio C Hamano wrote:
>> To handle --author=<match> request, the code created a grep instruction
>> that tried to match a line that begins with 'author ' and <match>
>> somewhere on the same line.  "begins with 'author '" obviously needs to be
>> expressed with an regexp '^author '.
>> 
>> When the user specifies --fixed-string, this does not work at all.
>> 
>> This extends the grep machinery so that a match insn can ignore user
>> specified --fixed-string request, and uses the '( -e A --and -e B )'
>> construct from the grep machinery in order to express "has to begin with
>> '^author ', and also the same line must match the given pattern".
>> 
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>
> Wow, this is clever solution! FWIW,

Actually, it is not clever at all.

It is a more natural solution, than what we originally had, at least from
the view of somebody who wrote our grep superstructure.  We use stock
regexp(3) library for finding individual matches, but the layer on top of
that to implement the default behaviour to --or hits together from
multiple expressions, optionally --and hits to find a line that has both
expressions, parenthesize to form groups of expressions etc. are all our
inventions.

One related thing I suspect many people haven't realized is that "git log"
can use "--all-match".

These two do different things:

	$ git log --author=Linus --grep=revision
	$ git log --all-match --author=Linus --grep=revision

The former finds commits that talk about "revision" by anybody, or commits
by Linus where he may or may not talk about "revision".  The latter finds
only commits by Linus where he talks about a word "revision" in the log
message.

I've been wondering if we want to make --all-match the default for
revision traversal family (i.e. "git log"), as I suspect it would match
people's expectations more naturally, even though it is a backward
incompatible change.  It is backward incompatible only because the
original implementation was stupid ;-).

--all-match

[Teemu Likonen]

Junio C Hamano wrote (2008-09-04 12:34 -0700):

> One related thing I suspect many people haven't realized is that "git
> log" can use "--all-match".

> I've been wondering if we want to make --all-match the default for
> revision traversal family (i.e. "git log"), as I suspect it would
> match people's expectations more naturally, even though it is
> a backward incompatible change.  It is backward incompatible only
> because the original implementation was stupid ;-).

You can count my vote for making --all-match the default, at least for
"git log". My logic says that adding more commit-limiting options to
command line would limit commits more.

Anyway, thanks for hint. I see --all-match is not a new feature (70d0afb
2006-09-27).

Re: [PATCH] Teach "log -F --author=<match>" to behave better

[Junio C Hamano]

Actually, I changed my mind.  If we were to polish the grep machinery to
suit the log matching usage, I think doing it the way this patch does is
much better.

-- >8 --
log --author/--committer: really match only with name part

When we tried to find commits done by AUTHOR, the first implementation
tried to pattern match a line with "^author .*AUTHOR", which later was
enhanced to strip leading caret and look for "^author AUTHOR" when the
search pattern was anchored at the left end (i.e. --author="^AUTHOR").

This had a few problems:

 * When looking for fixed strings (e.g. "git log -F --author=x --grep=y"),
   the regexp internally used "^author .*x" would never match anything;

 * To match at the end (e.g. "git log --author='google.com>$'"), the
   generated regexp has to also match the trailing timestamp part the
   commit header lines have.  Also, in order to determine if the '$' at
   the end means "match at the end of the line" or just a literal dollar
   sign (probably backslash-quoted), we would need to parse the regexp
   ourselves.

An earlier alternative tried to make sure that a line matches "^author "
(to limit by field name) and the user supplied pattern at the same time.
While it solved the -F problem by introducing a special override for
matching the "^author ", it did not solve the trailing timestamp nor tail
match problem.  It also would have matched every commit if --author=author
was asked for, not because the author's email part had this string, but
because every commit header line that talks about the author begins with
that field name, regardleses of who wrote it.  

Instead of piling more hacks on top of hacks, this rethinks the grep
machinery that is used to look for strings in the commit header, and makes
sure that (1) field name matches literally at the beginning of the line,
followed by a SP, and (2) the user supplied pattern is matched against the
remainder of the line, excluding the trailing timestamp data.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 grep.c     |   41 +++++++++++++++++++++++++++++++++++++++++
 grep.h     |    3 +++
 revision.c |   15 +--------------
 3 files changed, 45 insertions(+), 14 deletions(-)

diff --git c/grep.c w/grep.c
index f67d671..44a7dc6 100644
--- c/grep.c
+++ w/grep.c
@@ -2,6 +2,20 @@
 #include "grep.h"
 #include "xdiff-interface.h"
 
+void append_header_grep_pattern(struct grep_opt *opt, const char *field, const char *pat)
+{
+	struct grep_pat *p = xcalloc(1, sizeof(*p));
+	p->pattern = pat;
+	p->origin = "header";
+	p->no = 0;
+	p->token = GREP_PATTERN_HEAD;
+	p->field = field;
+	p->field_len = strlen(field);
+	*opt->pattern_tail = p;
+	opt->pattern_tail = &p->next;
+	p->next = NULL;
+}
+
 void append_grep_pattern(struct grep_opt *opt, const char *pat,
 			 const char *origin, int no, enum grep_pat_token t)
 {
@@ -247,16 +261,41 @@ static int fixmatch(const char *pattern, char *line, regmatch_t *match)
 	}
 }
 
+static int strip_timestamp(char *bol, char **eol_p)
+{
+	char *eol = *eol_p;
+	int ch;
+
+	while (bol < --eol) {
+		if (*eol != '>')
+			continue;
+		*eol_p = ++eol;
+		ch = *eol;
+		*eol = '\0';
+		return ch;
+	}
+	return 0;
+}
+
 static int match_one_pattern(struct grep_opt *opt, struct grep_pat *p, char *bol, char *eol, enum grep_context ctx)
 {
 	int hit = 0;
 	int at_true_bol = 1;
+	int saved_ch = 0;
 	regmatch_t pmatch[10];
 
 	if ((p->token != GREP_PATTERN) &&
 	    ((p->token == GREP_PATTERN_HEAD) != (ctx == GREP_CONTEXT_HEAD)))
 		return 0;
 
+	if (p->token == GREP_PATTERN_HEAD) {
+		/* author/committer fields in a commit object */
+		if (strncmp(bol, p->field, p->field_len) || bol[p->field_len] != ' ')
+			return 0;
+		bol += p->field_len + 1;
+		saved_ch = strip_timestamp(bol, &eol);
+	}
+
  again:
 	if (!opt->fixed) {
 		regex_t *exp = &p->regexp;
@@ -298,6 +337,8 @@ static int match_one_pattern(struct grep_opt *opt, struct grep_pat *p, char *bol
 			goto again;
 		}
 	}
+	if (p->token == GREP_PATTERN_HEAD && saved_ch)
+		*eol = saved_ch;
 	return hit;
 }
 
diff --git c/grep.h w/grep.h
index d252dd2..0454e50 100644
--- c/grep.h
+++ w/grep.h
@@ -23,6 +23,8 @@ struct grep_pat {
 	int no;
 	enum grep_pat_token token;
 	const char *pattern;
+	const char *field;
+	int field_len;
 	regex_t regexp;
 };
 
@@ -74,6 +76,7 @@ struct grep_opt {
 };
 
 extern void append_grep_pattern(struct grep_opt *opt, const char *pat, const char *origin, int no, enum grep_pat_token t);
+extern void append_header_grep_pattern(struct grep_opt *opt, const char *field, const char *pat);
 extern void compile_grep_patterns(struct grep_opt *opt);
 extern void free_grep_patterns(struct grep_opt *opt);
 extern int grep_buffer(struct grep_opt *opt, const char *name, char *buf, unsigned long size);
diff --git c/revision.c w/revision.c
index 36291b6..4092a11 100644
--- c/revision.c
+++ w/revision.c
@@ -955,20 +955,7 @@ static void add_grep(struct rev_info *revs, const char *ptn, enum grep_pat_token
 
 static void add_header_grep(struct rev_info *revs, const char *field, const char *pattern)
 {
-	char *pat;
-	const char *prefix;
-	int patlen, fldlen;
-
-	fldlen = strlen(field);
-	patlen = strlen(pattern);
-	pat = xmalloc(patlen + fldlen + 10);
-	prefix = ".*";
-	if (*pattern == '^') {
-		prefix = "";
-		pattern++;
-	}
-	sprintf(pat, "^%s %s%s", field, prefix, pattern);
-	add_grep(revs, pat, GREP_PATTERN_HEAD);
+	append_header_grep_pattern(&revs->grep_filter, field, pattern);
 }
 
 static void add_message_grep(struct rev_info *revs, const char *pattern)

Re: git rev-list --author/--committer b0rked with -F/--fixed-strings

[Giuseppe Bilotta]

On Thu, Sep 4, 2008 at 9:12 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Because we allow you to give "-F" and "--author=foo" in any order, looking
> at the options we have seen already and adjusting the pattern depending on
> the fixed-string option in the code would not be workable.  You would have
> to instead queue up all the --grep/--author/--committer options until the
> very end and _then_ compile them at the end, after you saw all the other
> grep related options such as -i/-F/-E

I suspected as much ... basically the idea would be to transform
fixed-string searches into auto-quoted regexp matches, right? I wonder
what kind of speed hit this would give.

> I am not interested in helping people with attitude very much ;-)

Ouch, I'm sorry. Did I come through as having an attitude? The reason
why I specified I thought it was a bug was because, as you specified,
regexps are actually necessary with --author etc, so one might
consider it a simple flag incompatibility. Sorry.


-- 
Giuseppe "Oblomov" Bilotta