threads / patch / 33257

patchAvoid false positives in label detection in cpp diff hunk header regex.

Subject: [PATCH] Avoid false positives in label detection in cpp diff hunk header regex.

## tl;dr

9 messages between Mar 22, 2013 and Mar 23, 2013. Diffs are folded; open one to read it.

replies: 8people: 4as markdown or json

Vadim Zeitlin· Mar 22, 2013, 13:43 UTC · lore
A C++ method start such as
        void
        foo::bar()

wasn't recognized by cpp diff driver as it mistakenly included "foo::bar" as a label. However the colon in a label can't be followed by another colon, so recognize this case specially to correctly detect C++ methods using this style.

Signed-off-by: Vadim Zeitlin <vz-git@zeitlins.org>
---
 userdiff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to userdiff.c +1 −2
diff --git a/userdiff.c b/userdiff.c
index ea43a03..9415586 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -125,7 +125,7 @@ PATTERNS("tex",
"^(\\\\((sub)*section|chapter|part)\\*{0,1}\\{.*)$",
         "\\\\[a-zA-Z@]+|\\\\.|[a-zA-Z0-9\x80-\xff]+"),
 PATTERNS("cpp",
         /* Jump targets or access declarations */
-        "!^[ \t]*[A-Za-z_][A-Za-z_0-9]*:.*$\n"
+        "!^[ \t]*[A-Za-z_][A-Za-z_0-9]*:([^:].*$|$)\n"
         /* C/++ functions/methods at top level */
         "^([A-Za-z_][A-Za-z_0-9]*([ \t*]+[A-Za-z_][A-Za-z_0-9]*([ \t]*::[
\t]*[^[:space:]]+)?){1,}[ \t]*\\([^;]*)$\n"
         /* compound type at top level */
--
1.8.2.135.g7b592fa
Junio C Hamano· Mar 22, 2013, 15:02 UTC · re: Vadim Zeitlin · lore

Re: [PATCH] Avoid false positives in label detection in cpp diff hunk header regex.

Vadim Zeitlin <vz-git@zeitlins.org> writes:
Show 25 quoted lines
> A C++ method start such as
>
>         void
>         foo::bar()
>
> wasn't recognized by cpp diff driver as it mistakenly included "foo::bar" as a
> label. However the colon in a label can't be followed by another colon, so
> recognize this case specially to correctly detect C++ methods using this style.
>
> Signed-off-by: Vadim Zeitlin <vz-git@zeitlins.org>
> ---
>  userdiff.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/userdiff.c b/userdiff.c
> index ea43a03..9415586 100644
> --- a/userdiff.c
> +++ b/userdiff.c
> @@ -125,7 +125,7 @@ PATTERNS("tex",
> "^(\\\\((sub)*section|chapter|part)\\*{0,1}\\{.*)$",
>          "\\\\[a-zA-Z@]+|\\\\.|[a-zA-Z0-9\x80-\xff]+"),
>  PATTERNS("cpp",
>          /* Jump targets or access declarations */
> -        "!^[ \t]*[A-Za-z_][A-Za-z_0-9]*:.*$\n"
> +        "!^[ \t]*[A-Za-z_][A-Za-z_0-9]*:([^:].*$|$)\n"

Hmm. Wouldn't "find a word (possibly after indentation), colon and then either a non-colon or end of line" be sufficient and simpler? iow, something like...

       "!^[ \t]*[A-Za-z_][A-Za-z_0-9]*:([^:]|$)"
Show 6 quoted lines
>          /* C/++ functions/methods at top level */
>          "^([A-Za-z_][A-Za-z_0-9]*([ \t*]+[A-Za-z_][A-Za-z_0-9]*([ \t]*::[
> \t]*[^[:space:]]+)?){1,}[ \t]*\\([^;]*)$\n"
>          /* compound type at top level */
> --
> 1.8.2.135.g7b592fa
Vadim Zeitlin· Mar 22, 2013, 17:27 UTC · re: Junio C Hamano · lore

Re: [PATCH] Avoid false positives in label detection in cpp diff hunk header regex.

Junio C Hamano <gitster <at> pobox.com> writes:
> 
> Vadim Zeitlin <vz-git <at> zeitlins.org> writes:
...
Show 17 quoted lines
> > diff --git a/userdiff.c b/userdiff.c
> > index ea43a03..9415586 100644
> > --- a/userdiff.c
> > +++ b/userdiff.c
> > @@ -125,7 +125,7 @@ PATTERNS("tex",
> > "^(\\\\((sub)*section|chapter|part)\\*{0,1}\\{.*)$",
> >          "\\\\[a-zA-Z@]+|\\\\.|[a-zA-Z0-9\x80-\xff]+"),
> >  PATTERNS("cpp",
> >          /* Jump targets or access declarations */
> > -        "!^[ \t]*[A-Za-z_][A-Za-z_0-9]*:.*$\n"
> > +        "!^[ \t]*[A-Za-z_][A-Za-z_0-9]*:([^:].*$|$)\n"
> 
> Hmm.  Wouldn't "find a word (possibly after indentation), colon and
> then either a non-colon or end of line" be sufficient and simpler?
> iow, something like...
> 
>        "!^[ \t]*[A-Za-z_][A-Za-z_0-9]*:([^:]|$)"
 This works too, of course. I didn't know why did the original regex
contain ".*$" part so I decided to keep it but your version is indeed
how I would have written it myself if I were doing it from scratch.
 Should I resubmit an updated patch or could you please just apply
your version?
 TIA!
VZ
Johannes Sixt· Mar 22, 2013, 21:55 UTC · re: Junio C Hamano · lore

Re: [PATCH] Avoid false positives in label detection in cpp diff hunk header regex.

Am 22.03.2013 16:02, schrieb Junio C Hamano:
Show 10 quoted lines
> Vadim Zeitlin <vz-git@zeitlins.org> writes:
> 
>> A C++ method start such as
>>
>>         void
>>         foo::bar()
>>
>> wasn't recognized by cpp diff driver as it mistakenly included "foo::bar" as a
>> label. However the colon in a label can't be followed by another colon, so
>> recognize this case specially to correctly detect C++ methods using this style.
Much appreciated!
Show 10 quoted lines
>>  PATTERNS("cpp",
>>          /* Jump targets or access declarations */
>> -        "!^[ \t]*[A-Za-z_][A-Za-z_0-9]*:.*$\n"
>> +        "!^[ \t]*[A-Za-z_][A-Za-z_0-9]*:([^:].*$|$)\n"
> 
> Hmm.  Wouldn't "find a word (possibly after indentation), colon and
> then either a non-colon or end of line" be sufficient and simpler?
> iow, something like...
> 
>        "!^[ \t]*[A-Za-z_][A-Za-z_0-9]*:([^:]|$)"

Yes, indeed. We don't need to match more than necessary in a negative pattern. The \n must still remain, though.

-- Hannes
Junio C Hamano· Mar 22, 2013, 22:32 UTC · re: Johannes Sixt · lore

Re: [PATCH] Avoid false positives in label detection in cpp diff hunk header regex.

Johannes Sixt <j6t@kdbg.org> writes:
Show 27 quoted lines
> Am 22.03.2013 16:02, schrieb Junio C Hamano:
>> Vadim Zeitlin <vz-git@zeitlins.org> writes:
>> 
>>> A C++ method start such as
>>>
>>>         void
>>>         foo::bar()
>>>
>>> wasn't recognized by cpp diff driver as it mistakenly included "foo::bar" as a
>>> label. However the colon in a label can't be followed by another colon, so
>>> recognize this case specially to correctly detect C++ methods using this style.
>
> Much appreciated!
>
>>>  PATTERNS("cpp",
>>>          /* Jump targets or access declarations */
>>> -        "!^[ \t]*[A-Za-z_][A-Za-z_0-9]*:.*$\n"
>>> +        "!^[ \t]*[A-Za-z_][A-Za-z_0-9]*:([^:].*$|$)\n"
>> 
>> Hmm.  Wouldn't "find a word (possibly after indentation), colon and
>> then either a non-colon or end of line" be sufficient and simpler?
>> iow, something like...
>> 
>>        "!^[ \t]*[A-Za-z_][A-Za-z_0-9]*:([^:]|$)"
>
> Yes, indeed. We don't need to match more than necessary in a negative
> pattern. The \n must still remain, though.

... because \n is not for matching against the text, but merely to separate the regular expressions, right?

I also wonder if 
	label :

should also be caught, or is it too weird format to be worth supporting?

Johannes Sixt· Mar 22, 2013, 23:11 UTC · re: Junio C Hamano · lore

Re: [PATCH] Avoid false positives in label detection in cpp diff hunk header regex.

Am 22.03.2013 23:32, schrieb Junio C Hamano:
Show 32 quoted lines
> Johannes Sixt <j6t@kdbg.org> writes:
> 
>> Am 22.03.2013 16:02, schrieb Junio C Hamano:
>>> Vadim Zeitlin <vz-git@zeitlins.org> writes:
>>>
>>>> A C++ method start such as
>>>>
>>>>         void
>>>>         foo::bar()
>>>>
>>>> wasn't recognized by cpp diff driver as it mistakenly included "foo::bar" as a
>>>> label. However the colon in a label can't be followed by another colon, so
>>>> recognize this case specially to correctly detect C++ methods using this style.
>>
>> Much appreciated!
>>
>>>>  PATTERNS("cpp",
>>>>          /* Jump targets or access declarations */
>>>> -        "!^[ \t]*[A-Za-z_][A-Za-z_0-9]*:.*$\n"
>>>> +        "!^[ \t]*[A-Za-z_][A-Za-z_0-9]*:([^:].*$|$)\n"
>>>
>>> Hmm.  Wouldn't "find a word (possibly after indentation), colon and
>>> then either a non-colon or end of line" be sufficient and simpler?
>>> iow, something like...
>>>
>>>        "!^[ \t]*[A-Za-z_][A-Za-z_0-9]*:([^:]|$)"
>>
>> Yes, indeed. We don't need to match more than necessary in a negative
>> pattern. The \n must still remain, though.
> 
> ... because \n is not for matching against the text, but merely to
> separate the regular expressions, right?
Correct.
Show 6 quoted lines
> I also wonder if 
> 
> 	label :
> 
> should also be caught, or is it too weird format to be worth
> supporting?

It's easy to support, by inserting another [ \t] before the first colon. So, why not?

-- Hannes
Vadim Zeitlin· Mar 23, 2013, 00:38 UTC · re: Johannes Sixt · lore

Re: [PATCH] Avoid false positives in label detection in cpp diff hunk header regex.

Johannes Sixt <j6t <at> kdbg.org> writes:
Show 9 quoted lines
> > I also wonder if 
> > 
> > 	label :
> > 
> > should also be caught, or is it too weird format to be worth
> > supporting?
> 
> It's easy to support, by inserting another [ \t] before the first colon.
> So, why not?
 This is really nitpicking, but if we do it, then it should be "[ \t]*". And the
"*" after the label should actually be a "+". So the full line becomes
  "!^[ \t]*[A-Za-z_][A-Za-z_0-9]+[ \t]*:([^:]|$)\n"
 But then I've never actually seen git putting labels incorrectly into the hunk
headers while I did see the problem this patch tries to fix, with wrong method
appearing in the header because the correct one was skipped due to this ignore
regex, quite a few times in the past.
 Regards,
VZ
Andreas Schwab· Mar 23, 2013, 08:31 UTC · re: Vadim Zeitlin · lore

Re: [PATCH] Avoid false positives in label detection in cpp diff hunk header regex.

Vadim Zeitlin <vz-git@zeitlins.org> writes:
>   "!^[ \t]*[A-Za-z_][A-Za-z_0-9]+[ \t]*:([^:]|$)\n"
That would fail to match single-character identifiers.
Andreas.
-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
Vadim Zeitlin· Mar 23, 2013, 09:48 UTC · re: Andreas Schwab · lore

Re: [PATCH] Avoid false positives in label detection in cpp diff hunk header regex.

Andreas Schwab <schwab <at> linux-m68k.org> writes:
Show 5 quoted lines
> Vadim Zeitlin <vz-git <at> zeitlins.org> writes:
> 
> >   "!^[ \t]*[A-Za-z_][A-Za-z_0-9]+[ \t]*:([^:]|$)\n"
> 
> That would fail to match single-character identifiers.
 Oops, yes, you're right, of course, sorry. I have no idea why did I write
that we needed to change this "*" to "+", the only explanation I see is that
it was simply too late at night when I did it. So the final version of the
exclusion regex is
	"!^[ \t]*[A-Za-z_][A-Za-z_0-9]*[ \t]*:([^:]|$)\n"
 But I feel like I'm still missing something about what is going on here.
Because after looking carefully at the (positive) regex for matching function
and method names, which is
	"^([A-Za-z_][A-Za-z_0-9]*([ \t*]+[A-Za-z_][A-Za-z_0-9]*"
	"([ \t]*::[ \t]*[^[:space:]]+)?){1,}[ \t]*\\([^;]*)$\n"

(split over 2 lines for readability), I actually don't understand how does it manage to match my declaration. Yet match it does, I do get

Show changes to diff +0 −0
@@ -438,6 +438,10 @@ firebird_statement_backend::execute(int number)

in my diff. But how is this possible? The "[ \t*]+" part has nowhere to match
but between "int" and "number" but it can't match there because there must be
only alphanumeric characters before it. Yet, not only it does match but if I
test with GNU grep -E, it matches too (after replacing "\\(" with just "\("
and removing "\n"). However if I test with perl or "sed -r", it does *not*
match. Can anyone see what's going on here?


 FWIW I've started looking into this because I thought that the current
regex wouldn't detect something like

	foo::nested_type foo::method()

as a start of a method. However it does detect this just fine as well which
I can't understand at all. I'm out of lame excuses (it's not too late here
yet...) so I just hope that I'm missing something about the way Git creates
hunk headers and not some obvious problem with the regex itself because
I've been staring at it for half an hour but still can't see how does it
manage to match here. Could anyone who does see it please explain?

 Thanks in advance,
VZ

← back to recent threads