{"thread":{"id":"29095","subject":"[PATCH] userdiff: allow * between cpp funcname words","startedAt":"2011-12-06T16:35:08Z","lastAt":"2011-12-07T21:13:10Z","messageCount":9,"participants":["Thomas Rast","Jeff King","Johannes Sixt","René Scharfe","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"180380","messageId":"a639d328e15bce3057de9238ee31097d15850de1.1323189110.git.trast@student.ethz.ch","threadId":"29095","inReplyTo":null,"subject":"[PATCH] userdiff: allow * between cpp funcname words","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-12-06T16:35:08Z","receivedAt":"2011-12-06T16:35:08Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"The cpp pattern, used for C and C++, would not match the start of a\ndeclaration such as\n\n static char *prepare_index(int argc,\n\nbecause it did not allow for * anywhere between the various words that\nconstitute the modifiers, type and function name. Fix it.\n\nSigned-off-by: Thomas Rast \n---\n\nThis is a really sneaky one-character bug that I cannot believe went\nunnoticed for so long, seeing as there are plenty of instances within\ngit itself where it matters.\n\n\n userdiff.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/userdiff.c b/userdiff.c\nindex 7c983c1..76109da 100644\n--- a/userdiff.c\n+++ b/userdiff.c\n@@ -118,7 +118,7 @@\n \t /* Jump targets or access declarations */\n \t \"!^[ \\t]*[A-Za-z_][A-Za-z_0-9]*:.*$\\n\"\n \t /* C/++ functions/methods at top level */\n-\t \"^([A-Za-z_][A-Za-z_0-9]*([ \\t]+[A-Za-z_][A-Za-z_0-9]*([ \\t]*::[ \\t]*[^[:space:]]+)?){1,}[ \\t]*\\\\([^;]*)$\\n\"\n+\t \"^([A-Za-z_][A-Za-z_0-9]*([ \\t*]+[A-Za-z_][A-Za-z_0-9]*([ \\t]*::[ \\t]*[^[:space:]]+)?){1,}[ \\t]*\\\\([^;]*)$\\n\"\n \t /* compound type at top level */\n \t \"^((struct|class|enum)[^;]*)$\",\n \t /* -- */\n-- \n1.7.8.424.gcb840\n"},{"id":"180395","messageId":"20111206190217.GD9492@sigill.intra.peff.net","threadId":"29095","inReplyTo":"a639d328e15bce3057de9238ee31097d15850de1.1323189110.git.trast@student.ethz.ch","subject":"Re: [PATCH] userdiff: allow * between cpp funcname words","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-12-06T19:02:17Z","receivedAt":"2011-12-06T19:02:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 06, 2011 at 05:35:08PM +0100, Thomas Rast wrote:\n\n> The cpp pattern, used for C and C++, would not match the start of a\n> declaration such as\n> \n> static char *prepare_index(int argc,\n> \n> because it did not allow for * anywhere between the various words that\n> constitute the modifiers, type and function name. Fix it.\n> \n> Signed-off-by: Thomas Rast \n> ---\n> \n> This is a really sneaky one-character bug that I cannot believe went\n> unnoticed for so long, seeing as there are plenty of instances within\n> git itself where it matters.\n\nLooks reasonable to me. You can see the difference, for instance, with:\n\n git show -U1 3c73a1d\n\n(The -U1 is because of the annoying \"we will start looking for the\nheader at the top of context, not the top of changes\" behavior I\nmentioned last week).\n\n-Peff\n"},{"id":"180402","messageId":"201112062117.57690.trast@student.ethz.ch","threadId":"29095","inReplyTo":"20111206190217.GD9492@sigill.intra.peff.net","subject":"Re: [PATCH] userdiff: allow * between cpp funcname words","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-12-06T20:17:56Z","receivedAt":"2011-12-06T20:17:56Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Jeff King wrote:\n> On Tue, Dec 06, 2011 at 05:35:08PM +0100, Thomas Rast wrote:\n> \n> > The cpp pattern, used for C and C++, would not match the start of a\n> > declaration such as\n> > \n> > static char *prepare_index(int argc,\n> > \n> > because it did not allow for * anywhere between the various words that\n> > constitute the modifiers, type and function name. Fix it.\n> > \n> > Signed-off-by: Thomas Rast \n> > ---\n> > \n> > This is a really sneaky one-character bug that I cannot believe went\n> > unnoticed for so long, seeing as there are plenty of instances within\n> > git itself where it matters.\n> \n> Looks reasonable to me. You can see the difference, for instance, with:\n> \n> git show -U1 3c73a1d\n> \n> (The -U1 is because of the annoying \"we will start looking for the\n> header at the top of context, not the top of changes\" behavior I\n> mentioned last week).\n\nActually (sadly) I'll have to revise it. It doesn't match much of C++\neither, and I haven't yet come up with a reasonable regex that\nmatches, say,\n\n foo::Bar::t& Baz::operator<<(\n\nwhich I would call ludicrous, but it's valid C++.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"180403","messageId":"20111206201944.GB27930@sigill.intra.peff.net","threadId":"29095","inReplyTo":"201112062117.57690.trast@student.ethz.ch","subject":"Re: [PATCH] userdiff: allow * between cpp funcname words","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-12-06T20:19:44Z","receivedAt":"2011-12-06T20:19:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 06, 2011 at 09:17:56PM +0100, Thomas Rast wrote:\n\n> > Looks reasonable to me. You can see the difference, for instance, with:\n> > \n> > git show -U1 3c73a1d\n> > \n> > (The -U1 is because of the annoying \"we will start looking for the\n> > header at the top of context, not the top of changes\" behavior I\n> > mentioned last week).\n> \n> Actually (sadly) I'll have to revise it. It doesn't match much of C++\n> either, and I haven't yet come up with a reasonable regex that\n> matches, say,\n> \n> foo::Bar::t& Baz::operator<<(\n> \n> which I would call ludicrous, but it's valid C++.\n\nIck, yeah. Maybe it is worth doing the \"*\" thing for now, and then\nworrying about advanced C++ stuff on top as another patch. AFAICT, your\noriginal patch is a strict improvement.\n\n-Peff\n"},{"id":"180406","messageId":"4EDE8086.9080303@kdbg.org","threadId":"29095","inReplyTo":"20111206201944.GB27930@sigill.intra.peff.net","subject":"Re: [PATCH] userdiff: allow * between cpp funcname words","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2011-12-06T20:52:22Z","receivedAt":"2011-12-06T20:52:22Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 06.12.2011 21:19, schrieb Jeff King:\n> On Tue, Dec 06, 2011 at 09:17:56PM +0100, Thomas Rast wrote:\n> \n>>> Looks reasonable to me. You can see the difference, for instance, with:\n>>>\n>>> git show -U1 3c73a1d\n>>>\n>>> (The -U1 is because of the annoying \"we will start looking for the\n>>> header at the top of context, not the top of changes\" behavior I\n>>> mentioned last week).\n>>\n>> Actually (sadly) I'll have to revise it. It doesn't match much of C++\n>> either, and I haven't yet come up with a reasonable regex that\n>> matches, say,\n>>\n>> foo::Bar::t& Baz::operator<<(\n>>\n>> which I would call ludicrous, but it's valid C++.\n> \n> Ick, yeah. Maybe it is worth doing the \"*\" thing for now, and then\n> worrying about advanced C++ stuff on top as another patch. AFAICT, your\n> original patch is a strict improvement.\n\nExcuse me, where's the problem? The above example shows this\n\n@@ -105,8 +105,8 @@ char *url_decode(const char *url)\n struct strbuf out = STRBUF_INIT;\n- const char *slash = strchr(url, '/');\n+ const char *colon = strchr(url, ':');\n...\n\nwith current 4cb5d10b. This looks quite correct, no?\n\n-- Hannes\n"},{"id":"180409","messageId":"4EDE83F7.6030708@lsrfire.ath.cx","threadId":"29095","inReplyTo":"4EDE8086.9080303@kdbg.org","subject":"Re: [PATCH] userdiff: allow * between cpp funcname words","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2011-12-06T21:07:03Z","receivedAt":"2011-12-06T21:07:03Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 06.12.2011 21:52, schrieb Johannes Sixt:\n> Am 06.12.2011 21:19, schrieb Jeff King:\n>> On Tue, Dec 06, 2011 at 09:17:56PM +0100, Thomas Rast wrote:\n>>\n>>>> Looks reasonable to me. You can see the difference, for instance, with:\n>>>>\n>>>> git show -U1 3c73a1d\n>>>>\n>>>> (The -U1 is because of the annoying \"we will start looking for the\n>>>> header at the top of context, not the top of changes\" behavior I\n>>>> mentioned last week).\n>>>\n>>> Actually (sadly) I'll have to revise it. It doesn't match much of C++\n>>> either, and I haven't yet come up with a reasonable regex that\n>>> matches, say,\n>>>\n>>> foo::Bar::t& Baz::operator<<(\n>>>\n>>> which I would call ludicrous, but it's valid C++.\n>>\n>> Ick, yeah. Maybe it is worth doing the \"*\" thing for now, and then\n>> worrying about advanced C++ stuff on top as another patch. AFAICT, your\n>> original patch is a strict improvement.\n>\n> Excuse me, where's the problem? The above example shows this\n>\n> @@ -105,8 +105,8 @@ char *url_decode(const char *url)\n> struct strbuf out = STRBUF_INIT;\n> - const char *slash = strchr(url, '/');\n> + const char *colon = strchr(url, ':');\n> ...\n>\n> with current 4cb5d10b. This looks quite correct, no?\n\nThat's with the default heuristic; try something like this first to turn \non userdiff:\n\n\t$ echo url.c diff=cpp >>.gitattributes\n\nRené\n"},{"id":"180411","messageId":"7vfwgxflkv.fsf@alter.siamese.dyndns.org","threadId":"29095","inReplyTo":"201112062117.57690.trast@student.ethz.ch","subject":"Re: [PATCH] userdiff: allow * between cpp funcname words","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-06T21:15:44Z","receivedAt":"2011-12-06T21:15:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n> Actually (sadly) I'll have to revise it. It doesn't match much of C++\n> either, and I haven't yet come up with a reasonable regex that\n> matches, say,\n>\n> foo::Bar::t& Baz::operator<<(\n>\n> which I would call ludicrous, but it's valid C++.\n\nHeh, I'd rather not see us go that route, which would either end up\nimplementing a C++ parser or reverting the heuristics back to \"non-blank\nat the beginning of the line\" that was already reasonably useful.\n"},{"id":"180463","messageId":"201112070904.28212.trast@student.ethz.ch","threadId":"29095","inReplyTo":"7vfwgxflkv.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] userdiff: allow * between cpp funcname words","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-12-07T08:04:27Z","receivedAt":"2011-12-07T08:04:27Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Junio C Hamano wrote:\n> Thomas Rast writes:\n> \n> > Actually (sadly) I'll have to revise it. It doesn't match much of C++\n> > either, and I haven't yet come up with a reasonable regex that\n> > matches, say,\n> >\n> > foo::Bar::t& Baz::operator<<(\n> >\n> > which I would call ludicrous, but it's valid C++.\n> \n> Heh, I'd rather not see us go that route, which would either end up\n> implementing a C++ parser or reverting the heuristics back to \"non-blank\n> at the beginning of the line\" that was already reasonably useful.\n\nWell, there are many things that we deliberately do not match right\nnow and for which that's a good thing:\n\n label:\n public:\n void declaration_only(...);\n int global_variable;\n\nAt some point I was wondering whether it would be better to just\ndeclare a non-match for '.*;' and '^[A-Za-z_][A-Za-z_0-9]+:', and\notherwise match all '^[A-Za-z].*\\(' but I may be missing something.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"180524","messageId":"4EDFD6E6.5050301@kdbg.org","threadId":"29095","inReplyTo":"201112070904.28212.trast@student.ethz.ch","subject":"Re: [PATCH] userdiff: allow * between cpp funcname words","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2011-12-07T21:13:10Z","receivedAt":"2011-12-07T21:13:10Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 07.12.2011 09:04, schrieb Thomas Rast:\n> Junio C Hamano wrote:\n>> Thomas Rast writes:\n>>\n>>> Actually (sadly) I'll have to revise it. It doesn't match much of C++\n>>> either, and I haven't yet come up with a reasonable regex that\n>>> matches, say,\n>>>\n>>> foo::Bar::t& Baz::operator<<(\n>>>\n>>> which I would call ludicrous, but it's valid C++.\n>>\n>> Heh, I'd rather not see us go that route, which would either end up\n>> implementing a C++ parser or reverting the heuristics back to \"non-blank\n>> at the beginning of the line\" that was already reasonably useful.\n> \n> Well, there are many things that we deliberately do not match right\n> now and for which that's a good thing:\n> \n> label:\n> public:\n> void declaration_only(...);\n> int global_variable;\n> \n> At some point I was wondering whether it would be better to just\n> declare a non-match for '.*;' and '^[A-Za-z_][A-Za-z_0-9]+:', and\n> otherwise match all '^[A-Za-z].*\\(' but I may be missing something.\n\nThe current cpp pattern doesn't work that well with C++. Since it\nrequires a blank before a name before the opening parentheses, it\ndoesn't catch constructors:\n\nFoo::Foo()\n\nand it should fail for GNU style C function definitions as well (I\ndidn't test):\n\nvoid\ndo_the_foo()\n\nI'll run this pattern for a while:\n\ndiff.cpp.xfuncname=!^[a-zA-Z_][a-zA-Z_0-9]*[[:space:]]*:[[:space:]]*$\n^[a-zA-Z_].*\n\nBTW, your match pattern requires an opening parenthesis; it would not\ncatch class definitions.\n\n-- Hannes\n"}]}