Re: [PATCH v2 4/4] diff.c: convert builtin funcname patterns to non-GNU extended regex syntax

From: Brandon Casey <casey@nrlssc.navy.mil>
Date: Sep 22, 2008, 16:59 UTC
Message-ID: <i-l9YX2TO45e2OB9LuoxrAN6a2iFYaH_eEGlVmRsP0oa97XuwX4eGQ@cipher.nrlssc.navy.mil>
In-Reply-To: <7vmyi21mf8.fsf@gitster.siamese.dyndns.org>

Junio C Hamano wrote:

Show 16 quoted lines

> Brandon Casey <drafnel@gmail.com> writes:
> 
>> Junio C Hamano <gitster <at> pobox.com> writes:
>>
>>> Here is [1/2] to be applied on top of 45d9414 (diff.*.xfuncname which uses
>>> "extended" regex's for hunk header selection, 2008-09-18).
>>>
>>> Testing appreciated.
>>> +	{ "bibtex", "(@[a-zA-Z]{1,}[ \t]*\{{0,1}[ \t]*[^ \t\"@',\\#}{~%]*).*$",
>>> +	  REG_EXTENDED },
>>> +	{ "tex",
>>> +	  "^(\\\\((sub)*section|chapter|part)\\*{0,1}\{.*)$",
>> I think you need double backslash '\\' before '{' in the two places in these
>> patterns where you only have a single backslash.
> 
> Thanks.  Any other nits?

Just a single minor one on the "fix multiple regexp semantics" patch.

This:

 	for (i = 0; i < regs->nr; i++) {
		...
	}
+	if (regs->nr <= i)

makes me use my brain (I try to avoid that).

I only mention it since I've seen discussions in the past about this sort of ordering, and you seem to have accepted that it can be confusing to native english speakers who would "read" the 'if' statement. The ordering above puts emphasis on the value of regs->nr, when it is actually the value of 'i' that is being tested (since regs->nr is unchanging).

I'll do some testing on non-GNU platforms today.

-brandon