{"thread":{"id":"33257","subject":"[PATCH] Avoid false positives in label detection in cpp diff hunk header regex.","startedAt":"2013-03-22T13:43:52Z","lastAt":"2013-03-23T09:48:30Z","messageCount":9,"participants":["Vadim Zeitlin","Junio C Hamano","Johannes Sixt","Andreas Schwab"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"211938","messageId":"loom.20130322T144107-601@post.gmane.org","threadId":"33257","inReplyTo":null,"subject":"[PATCH] Avoid false positives in label detection in cpp diff hunk header regex.","fromName":"Vadim Zeitlin","fromEmail":"vz-git@zeitlins.org","sentAt":"2013-03-22T13:43:52Z","receivedAt":"2013-03-22T13:43:52Z","isPatch":true,"sender":{"key":"vz-git@zeitlins.org","avatar":null},"body":"A C++ method start such as\n\n        void\n        foo::bar()\n\nwasn't recognized by cpp diff driver as it mistakenly included \"foo::bar\" as a\nlabel. However the colon in a label can't be followed by another colon, so\nrecognize this case specially to correctly detect C++ methods using this style.\n\nSigned-off-by: Vadim Zeitlin <vz-git@zeitlins.org>\n---\n userdiff.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/userdiff.c b/userdiff.c\nindex ea43a03..9415586 100644\n--- a/userdiff.c\n+++ b/userdiff.c\n@@ -125,7 +125,7 @@ PATTERNS(\"tex\",\n\"^(\\\\\\\\((sub)*section|chapter|part)\\\\*{0,1}\\\\{.*)$\",\n         \"\\\\\\\\[a-zA-Z@]+|\\\\\\\\.|[a-zA-Z0-9\\x80-\\xff]+\"),\n PATTERNS(\"cpp\",\n         /* Jump targets or access declarations */\n-        \"!^[ \\t]*[A-Za-z_][A-Za-z_0-9]*:.*$\\n\"\n+        \"!^[ \\t]*[A-Za-z_][A-Za-z_0-9]*:([^:].*$|$)\\n\"\n         /* C/++ functions/methods at top level */\n         \"^([A-Za-z_][A-Za-z_0-9]*([ \\t*]+[A-Za-z_][A-Za-z_0-9]*([ \\t]*::[\n\\t]*[^[:space:]]+)?){1,}[ \\t]*\\\\([^;]*)$\\n\"\n         /* compound type at top level */\n--\n1.8.2.135.g7b592fa\n"},{"id":"211948","messageId":"7vehf78olw.fsf@alter.siamese.dyndns.org","threadId":"33257","inReplyTo":"loom.20130322T144107-601@post.gmane.org","subject":"Re: [PATCH] Avoid false positives in label detection in cpp diff hunk header regex.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-22T15:02:03Z","receivedAt":"2013-03-22T15:02:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Vadim Zeitlin <vz-git@zeitlins.org> writes:\n\n> A C++ method start such as\n>\n>         void\n>         foo::bar()\n>\n> wasn't recognized by cpp diff driver as it mistakenly included \"foo::bar\" as a\n> label. However the colon in a label can't be followed by another colon, so\n> recognize this case specially to correctly detect C++ methods using this style.\n>\n> Signed-off-by: Vadim Zeitlin <vz-git@zeitlins.org>\n> ---\n>  userdiff.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/userdiff.c b/userdiff.c\n> index ea43a03..9415586 100644\n> --- a/userdiff.c\n> +++ b/userdiff.c\n> @@ -125,7 +125,7 @@ PATTERNS(\"tex\",\n> \"^(\\\\\\\\((sub)*section|chapter|part)\\\\*{0,1}\\\\{.*)$\",\n>          \"\\\\\\\\[a-zA-Z@]+|\\\\\\\\.|[a-zA-Z0-9\\x80-\\xff]+\"),\n>  PATTERNS(\"cpp\",\n>          /* Jump targets or access declarations */\n> -        \"!^[ \\t]*[A-Za-z_][A-Za-z_0-9]*:.*$\\n\"\n> +        \"!^[ \\t]*[A-Za-z_][A-Za-z_0-9]*:([^:].*$|$)\\n\"\n\nHmm.  Wouldn't \"find a word (possibly after indentation), colon and\nthen either a non-colon or end of line\" be sufficient and simpler?\niow, something like...\n\n       \"!^[ \\t]*[A-Za-z_][A-Za-z_0-9]*:([^:]|$)\"\n\n>          /* C/++ functions/methods at top level */\n>          \"^([A-Za-z_][A-Za-z_0-9]*([ \\t*]+[A-Za-z_][A-Za-z_0-9]*([ \\t]*::[\n> \\t]*[^[:space:]]+)?){1,}[ \\t]*\\\\([^;]*)$\\n\"\n>          /* compound type at top level */\n> --\n> 1.8.2.135.g7b592fa\n"},{"id":"211961","messageId":"loom.20130322T182517-342@post.gmane.org","threadId":"33257","inReplyTo":"7vehf78olw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Avoid false positives in label detection in cpp diff hunk header regex.","fromName":"Vadim Zeitlin","fromEmail":"vz-git@zeitlins.org","sentAt":"2013-03-22T17:27:41Z","receivedAt":"2013-03-22T17:27:41Z","isPatch":true,"sender":{"key":"vz-git@zeitlins.org","avatar":null},"body":"Junio C Hamano <gitster <at> pobox.com> writes:\n\n> \n> Vadim Zeitlin <vz-git <at> zeitlins.org> writes:\n... \n> > diff --git a/userdiff.c b/userdiff.c\n> > index ea43a03..9415586 100644\n> > --- a/userdiff.c\n> > +++ b/userdiff.c\n> > @@ -125,7 +125,7 @@ PATTERNS(\"tex\",\n> > \"^(\\\\\\\\((sub)*section|chapter|part)\\\\*{0,1}\\\\{.*)$\",\n> >          \"\\\\\\\\[a-zA-Z@]+|\\\\\\\\.|[a-zA-Z0-9\\x80-\\xff]+\"),\n> >  PATTERNS(\"cpp\",\n> >          /* Jump targets or access declarations */\n> > -        \"!^[ \\t]*[A-Za-z_][A-Za-z_0-9]*:.*$\\n\"\n> > +        \"!^[ \\t]*[A-Za-z_][A-Za-z_0-9]*:([^:].*$|$)\\n\"\n> \n> Hmm.  Wouldn't \"find a word (possibly after indentation), colon and\n> then either a non-colon or end of line\" be sufficient and simpler?\n> iow, something like...\n> \n>        \"!^[ \\t]*[A-Za-z_][A-Za-z_0-9]*:([^:]|$)\"\n\n This works too, of course. I didn't know why did the original regex\ncontain \".*$\" part so I decided to keep it but your version is indeed\nhow I would have written it myself if I were doing it from scratch.\n\n Should I resubmit an updated patch or could you please just apply\nyour version?\n\n TIA!\nVZ\n"},{"id":"211990","messageId":"514CD34F.70107@kdbg.org","threadId":"33257","inReplyTo":"7vehf78olw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Avoid false positives in label detection in cpp diff hunk header regex.","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2013-03-22T21:55:27Z","receivedAt":"2013-03-22T21:55:27Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 22.03.2013 16:02, schrieb Junio C Hamano:\n> Vadim Zeitlin <vz-git@zeitlins.org> writes:\n> \n>> A C++ method start such as\n>>\n>>         void\n>>         foo::bar()\n>>\n>> wasn't recognized by cpp diff driver as it mistakenly included \"foo::bar\" as a\n>> label. However the colon in a label can't be followed by another colon, so\n>> recognize this case specially to correctly detect C++ methods using this style.\n\nMuch appreciated!\n\n>>  PATTERNS(\"cpp\",\n>>          /* Jump targets or access declarations */\n>> -        \"!^[ \\t]*[A-Za-z_][A-Za-z_0-9]*:.*$\\n\"\n>> +        \"!^[ \\t]*[A-Za-z_][A-Za-z_0-9]*:([^:].*$|$)\\n\"\n> \n> Hmm.  Wouldn't \"find a word (possibly after indentation), colon and\n> then either a non-colon or end of line\" be sufficient and simpler?\n> iow, something like...\n> \n>        \"!^[ \\t]*[A-Za-z_][A-Za-z_0-9]*:([^:]|$)\"\n\nYes, indeed. We don't need to match more than necessary in a negative\npattern. The \\n must still remain, though.\n\n-- Hannes\n"},{"id":"211997","messageId":"7vhak35ami.fsf@alter.siamese.dyndns.org","threadId":"33257","inReplyTo":"514CD34F.70107@kdbg.org","subject":"Re: [PATCH] Avoid false positives in label detection in cpp diff hunk header regex.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-22T22:32:21Z","receivedAt":"2013-03-22T22:32:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> Am 22.03.2013 16:02, schrieb Junio C Hamano:\n>> Vadim Zeitlin <vz-git@zeitlins.org> writes:\n>> \n>>> A C++ method start such as\n>>>\n>>>         void\n>>>         foo::bar()\n>>>\n>>> wasn't recognized by cpp diff driver as it mistakenly included \"foo::bar\" as a\n>>> label. However the colon in a label can't be followed by another colon, so\n>>> recognize this case specially to correctly detect C++ methods using this style.\n>\n> Much appreciated!\n>\n>>>  PATTERNS(\"cpp\",\n>>>          /* Jump targets or access declarations */\n>>> -        \"!^[ \\t]*[A-Za-z_][A-Za-z_0-9]*:.*$\\n\"\n>>> +        \"!^[ \\t]*[A-Za-z_][A-Za-z_0-9]*:([^:].*$|$)\\n\"\n>> \n>> Hmm.  Wouldn't \"find a word (possibly after indentation), colon and\n>> then either a non-colon or end of line\" be sufficient and simpler?\n>> iow, something like...\n>> \n>>        \"!^[ \\t]*[A-Za-z_][A-Za-z_0-9]*:([^:]|$)\"\n>\n> Yes, indeed. We don't need to match more than necessary in a negative\n> pattern. The \\n must still remain, though.\n\n... because \\n is not for matching against the text, but merely to\nseparate the regular expressions, right?\n\nI also wonder if \n\n\tlabel :\n\nshould also be caught, or is it too weird format to be worth\nsupporting?\n"},{"id":"212004","messageId":"514CE53F.3080308@kdbg.org","threadId":"33257","inReplyTo":"7vhak35ami.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Avoid false positives in label detection in cpp diff hunk header regex.","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2013-03-22T23:11:59Z","receivedAt":"2013-03-22T23:11:59Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 22.03.2013 23:32, schrieb Junio C Hamano:\n> Johannes Sixt <j6t@kdbg.org> writes:\n> \n>> Am 22.03.2013 16:02, schrieb Junio C Hamano:\n>>> Vadim Zeitlin <vz-git@zeitlins.org> writes:\n>>>\n>>>> A C++ method start such as\n>>>>\n>>>>         void\n>>>>         foo::bar()\n>>>>\n>>>> wasn't recognized by cpp diff driver as it mistakenly included \"foo::bar\" as a\n>>>> label. However the colon in a label can't be followed by another colon, so\n>>>> recognize this case specially to correctly detect C++ methods using this style.\n>>\n>> Much appreciated!\n>>\n>>>>  PATTERNS(\"cpp\",\n>>>>          /* Jump targets or access declarations */\n>>>> -        \"!^[ \\t]*[A-Za-z_][A-Za-z_0-9]*:.*$\\n\"\n>>>> +        \"!^[ \\t]*[A-Za-z_][A-Za-z_0-9]*:([^:].*$|$)\\n\"\n>>>\n>>> Hmm.  Wouldn't \"find a word (possibly after indentation), colon and\n>>> then either a non-colon or end of line\" be sufficient and simpler?\n>>> iow, something like...\n>>>\n>>>        \"!^[ \\t]*[A-Za-z_][A-Za-z_0-9]*:([^:]|$)\"\n>>\n>> Yes, indeed. We don't need to match more than necessary in a negative\n>> pattern. The \\n must still remain, though.\n> \n> ... because \\n is not for matching against the text, but merely to\n> separate the regular expressions, right?\n\nCorrect.\n\n> I also wonder if \n> \n> \tlabel :\n> \n> should also be caught, or is it too weird format to be worth\n> supporting?\n\nIt's easy to support, by inserting another [ \\t] before the first colon.\nSo, why not?\n\n-- Hannes\n"},{"id":"212008","messageId":"loom.20130323T011153-345@post.gmane.org","threadId":"33257","inReplyTo":"514CE53F.3080308@kdbg.org","subject":"Re: [PATCH] Avoid false positives in label detection in cpp diff hunk header regex.","fromName":"Vadim Zeitlin","fromEmail":"vz-git@zeitlins.org","sentAt":"2013-03-23T00:38:37Z","receivedAt":"2013-03-23T00:38:37Z","isPatch":true,"sender":{"key":"vz-git@zeitlins.org","avatar":null},"body":"Johannes Sixt <j6t <at> kdbg.org> writes:\n\n> > I also wonder if \n> > \n> > \tlabel :\n> > \n> > should also be caught, or is it too weird format to be worth\n> > supporting?\n> \n> It's easy to support, by inserting another [ \\t] before the first colon.\n> So, why not?\n\n This is really nitpicking, but if we do it, then it should be \"[ \\t]*\". And the\n\"*\" after the label should actually be a \"+\". So the full line becomes\n\n\n  \"!^[ \\t]*[A-Za-z_][A-Za-z_0-9]+[ \\t]*:([^:]|$)\\n\"\n\n\n But then I've never actually seen git putting labels incorrectly into the hunk\nheaders while I did see the problem this patch tries to fix, with wrong method\nappearing in the header because the correct one was skipped due to this ignore\nregex, quite a few times in the past.\n\n Regards,\nVZ\n"},{"id":"212036","messageId":"m2y5de34bz.fsf@linux-m68k.org","threadId":"33257","inReplyTo":"loom.20130323T011153-345@post.gmane.org","subject":"Re: [PATCH] Avoid false positives in label detection in cpp diff hunk header regex.","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2013-03-23T08:31:12Z","receivedAt":"2013-03-23T08:31:12Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"Vadim Zeitlin <vz-git@zeitlins.org> writes:\n\n>   \"!^[ \\t]*[A-Za-z_][A-Za-z_0-9]+[ \\t]*:([^:]|$)\\n\"\n\nThat would fail to match single-character identifiers.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"212040","messageId":"loom.20130323T101131-456@post.gmane.org","threadId":"33257","inReplyTo":"m2y5de34bz.fsf@linux-m68k.org","subject":"Re: [PATCH] Avoid false positives in label detection in cpp diff hunk header regex.","fromName":"Vadim Zeitlin","fromEmail":"vz-git@zeitlins.org","sentAt":"2013-03-23T09:48:30Z","receivedAt":"2013-03-23T09:48:30Z","isPatch":true,"sender":{"key":"vz-git@zeitlins.org","avatar":null},"body":"Andreas Schwab <schwab <at> linux-m68k.org> writes:\n\n> Vadim Zeitlin <vz-git <at> zeitlins.org> writes:\n> \n> >   \"!^[ \\t]*[A-Za-z_][A-Za-z_0-9]+[ \\t]*:([^:]|$)\\n\"\n> \n> That would fail to match single-character identifiers.\n\n Oops, yes, you're right, of course, sorry. I have no idea why did I write\nthat we needed to change this \"*\" to \"+\", the only explanation I see is that\nit was simply too late at night when I did it. So the final version of the\nexclusion regex is\n\n\t\"!^[ \\t]*[A-Za-z_][A-Za-z_0-9]*[ \\t]*:([^:]|$)\\n\"\n\n\n But I feel like I'm still missing something about what is going on here.\nBecause after looking carefully at the (positive) regex for matching function\nand method names, which is\n\n\t\"^([A-Za-z_][A-Za-z_0-9]*([ \\t*]+[A-Za-z_][A-Za-z_0-9]*\"\n\t\"([ \\t]*::[ \\t]*[^[:space:]]+)?){1,}[ \\t]*\\\\([^;]*)$\\n\"\n\n(split over 2 lines for readability), I actually don't understand how does it\nmanage to match my declaration. Yet match it does, I do get\n\n@@ -438,6 +438,10 @@ firebird_statement_backend::execute(int number)\n\nin my diff. But how is this possible? The \"[ \\t*]+\" part has nowhere to match\nbut between \"int\" and \"number\" but it can't match there because there must be\nonly alphanumeric characters before it. Yet, not only it does match but if I\ntest with GNU grep -E, it matches too (after replacing \"\\\\(\" with just \"\\(\"\nand removing \"\\n\"). However if I test with perl or \"sed -r\", it does *not*\nmatch. Can anyone see what's going on here?\n\n\n FWIW I've started looking into this because I thought that the current\nregex wouldn't detect something like\n\n\tfoo::nested_type foo::method()\n\nas a start of a method. However it does detect this just fine as well which\nI can't understand at all. I'm out of lame excuses (it's not too late here\nyet...) so I just hope that I'm missing something about the way Git creates\nhunk headers and not some obvious problem with the regex itself because\nI've been staring at it for half an hour but still can't see how does it\nmanage to match here. Could anyone who does see it please explain?\n\n Thanks in advance,\nVZ\n"}]}