Re: [PATCH] Interpret :/<pattern> as a regular expression

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

On Tue, 12 Jun 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > diff --git a/sha1_name.c b/sha1_name.c
> > index d9188ed..f1ba194 100644
> > --- a/sha1_name.c
> > +++ b/sha1_name.c
> > @@ -611,6 +611,8 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1)
> >  {
> >  	struct commit_list *list = NULL, *backup = NULL, *l;
> >  	int retval = -1;
> > +	regex_t regexp;
> > +	regmatch_t regmatch[1];
> >  
> >  	if (prefix[0] == '!') {
> >  		if (prefix[1] != '!')
> 
> Because you are not extracting any match substring, I do not
> think you would need regmatch[] there.

I explicitely want to anchor the match at the beginning of the message 
(otherwise, most of the interesting patterns would match a merge pulling 
that commit in), and therefore I have to check where the match was. Alas, 
that's only possible with regmatch.

> > @@ -622,6 +624,8 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1)
> >  	for_each_ref(handle_one_ref, &list);
> >  	for (l = list; l; l = l->next)
> >  		commit_list_insert(l->item, &backup);
> > +	if (regcomp(&regexp, prefix, REG_EXTENDED))
> > +		return error("invalid regexp: %s", prefix);
> >  	while (list) {
> >  		char *p;
> >  		struct commit *commit;
> 
> Why EXTENDED?

Sorry. Old habit. Will fix.

> Also you probably want REG_NEWLINE.

No. If I look for something in the body of the message (like I showed in 
the two examples), I'd like to say ':/.*Blub'. Of course, you could always 
say

	git diff ':/[ -~
]*Blub'

with REG_NEWLINE, to match newlines also. But frankly, it is much more 
often that I want to match something in the whole message than just in the 
oneline. And if I _do_ want the match only in the oneline, I can still go 
and do

	git diff ':/[^
]*Blub'

when REG_NEWLINE is disabled. If you can teach me a better way to do both 
things, matching in the oneline _or_ matching the whole message, _with_ 
REG_NEWLINE, I'll gladly change it, and provide an example in the commit 
message as well as the documentation for equally clueless subjects as me.

> > @@ -630,7 +634,9 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1)
> >  		parse_object(commit->object.sha1);
> >  		if (!commit->buffer || !(p = strstr(commit->buffer, "\n\n")))
> >  			continue;
> > -		if (!prefixcmp(p + 2, prefix)) {
> > +		if (!regexec(&regexp, p + 2, 1, regmatch, 0) &&
> > +				printf("match: %d\n", regmatch[0].rm_so) &&
> > +				regmatch[0].rm_so == 0) {
> >  			hashcpy(sha1, commit->object.sha1);
> >  			retval = 0;
> >  			break;
> 
> Do we want to detect return value other than REG_NOMATCH from
> regexec() when it does not return zero?

I am not well versed in the multitude of POSIX and other standards we have 
on this planet, therefore I just read my man page. And it says, quote:

	regexec() returns zero for a successful match or REG_NOMATCH for 
	failure.

Tertium non datur. At least according to my man page here. Am I mistaken 
in my assumption (which seems to be somewhat supported from my limited 
reading of the man page) that all errors should be caught at regcomp() 
time?

> Please lose the debugging printf() before submitting.

Ouch. Sorry.

> > @@ -639,6 +645,7 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1)
> >  	free_commit_list(list);
> >  	for (l = backup; l; l = l->next)
> >  		clear_commit_marks(l->item, ONELINE_SEEN);
> > +	regfree(&regexp);
> >  	return retval;
> >  }
> >  
> 
> Also I think you would want to fix get_sha1_oneline() so that it
> not to refuse to work without save_commit_buffer.

You're right, will fix.

I'll resubmit shortly. However, feel free to enlighten me with insights 
into working _with_ REG_NEWLINE, and I'll gladly submit another version of 
the patch with REG_NEWLINE enabled, along with the additions to the 
documentation.

Thanks,
Dscho