{"thread":{"id":"36060","subject":"[RFC/PATCH] diff: simplify cpp funcname regex","startedAt":"2014-03-05T00:36:40Z","lastAt":"2014-03-25T21:42:33Z","messageCount":30,"participants":["Jeff King","Johannes Sixt","Junio C Hamano","René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"236058","messageId":"20140305003639.GA9474@sigill.intra.peff.net","threadId":"36060","inReplyTo":null,"subject":"[RFC/PATCH] diff: simplify cpp funcname regex","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-05T00:36:40Z","receivedAt":"2014-03-05T00:36:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The current funcname matcher for C files requires one or\nmore words before the function name, like:\n\n static int foo(int arg)\n {\n\nHowever, some coding styles look like this:\n\n static int\n foo(int arg)\n {\n\nand we do not match, even though the default regex would.\n\nThis patch simplifies the regex significantly. Rather than\ntrying to handle all the variants of keywords and return\ntypes, we simply look for an identifier at the start of the\nline that contains a \"(\", meaning it is either a function\ndefinition or a function call, and then not containing \";\"\nwhich would indicate it is a call or declaration.\n\nThis can be fooled by something like:\n\n if (foo)\n\nbut it is unlikely to find that at the very start of a line\nwith no identation (and without having a complete set of\nkeywords, we cannot distinguish that from a function called\n\"if\" taking \"foo\" anyway).\n\nWe had no tests at all for .c files, so this attempts to add\na few obvious cases (including the one we are fixing here).\n\nSigned-off-by: Jeff King \n---\nI tried accommodating this one case in the current regex, but it just\nkept getting more complicated and unreadable. Maybe I am being naive to\nthink that this much simpler version can work. We don't have a lot of\ntests or a known-good data set to check.\n\nI did try \"git log -1000 -p\" before and after on git.git, diff'd the\nresults and manually inspected. The results were a mix of strict\nimprovement (mostly instances of the style I was trying to fix here) and\ncases where there is no good answer. For example, for top-level changes\noutside functions, we might find:\n\n N_(\"some text that is long\"\n\nthat is part of:\n\n const char *foo =\n N_(\"some text that is long\"\n \"and spans multiple lines\");\n\nBefore this change, we would skip past it (using the cpp regex, that is;\nthe default one tends to find the same line) and either report nothing,\nor whatever random function was before us. So it's a behavior change,\nbut the existing behavior is really no better.\n\n t/t4018-diff-funcname.sh | 36 ++++++++++++++++++++++++++++++++++++\n userdiff.c | 2 +-\n 2 files changed, 37 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh\nindex 38a092a..1e80b15 100755\n--- a/t/t4018-diff-funcname.sh\n+++ b/t/t4018-diff-funcname.sh\n@@ -93,6 +93,29 @@ sed -e '\n \ts/song;/song();/\n ' Beer-correct.perl\n \n+cat >Beer.c <<\\EOF\n+static int foo(void)\n+{\n+label:\n+\tint x = old;\n+}\n+\n+struct foo; /* catch failure below */\n+static int\n+gnu(int arg)\n+{\n+\tint x = old;\n+}\n+\n+struct foo; /* catch failure below */\n+int multiline(int arg,\n+\t char *arg2)\n+{\n+\tint x = old;\n+}\n+EOF\n+sed s/old/new/ Beer-correct.c\n+\n test_expect_funcname () {\n \tlang=${2-java}\n \ttest_expect_code 1 git diff --no-index -U1 \\\n@@ -127,6 +150,7 @@ test_expect_success 'set up .gitattributes declaring drivers to test' '\n \tcat >.gitattributes <<-\\EOF\n \t*.java diff=java\n \t*.perl diff=perl\n+\t*.c diff=cpp\n \tEOF\n '\n \n@@ -158,6 +182,18 @@ test_expect_success 'perl pattern is not distracted by forward declaration' '\n \ttest_expect_funcname \"package Beer;\\$\" perl\n '\n \n+test_expect_success 'c pattern skips labels' '\n+\ttest_expect_funcname \"static int foo(void)\" c\n+'\n+\n+test_expect_success 'c pattern matches GNU-style functions' '\n+\ttest_expect_funcname \"gnu(int arg)\\$\" c\n+'\n+\n+test_expect_success 'c pattern matches multiline functions' '\n+\ttest_expect_funcname \"int multiline(int arg,\\$\" c\n+'\n+\n test_expect_success 'custom pattern' '\n \ttest_config diff.java.funcname \"!static\n !String\ndiff --git a/userdiff.c b/userdiff.c\nindex 10b61ec..b9d52b7 100644\n--- a/userdiff.c\n+++ b/userdiff.c\n@@ -127,7 +127,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_].*\\\\([^;]*)$\\n\"\n \t /* compound type at top level */\n \t \"^((struct|class|enum)[^;]*)$\",\n \t /* -- */\n-- \n1.8.5.2.500.g8060133\n"},{"id":"236073","messageId":"5316D922.9010501@viscovery.net","threadId":"36060","inReplyTo":"20140305003639.GA9474@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] diff: simplify cpp funcname regex","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2014-03-05T07:58:26Z","receivedAt":"2014-03-05T07:58:26Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 3/5/2014 1:36, schrieb Jeff King:\n> The current funcname matcher for C files requires one or\n> more words before the function name, like:\n> \n> static int foo(int arg)\n> {\n> \n> However, some coding styles look like this:\n> \n> static int\n> foo(int arg)\n> {\n> \n> and we do not match, even though the default regex would.\n> \n> This patch simplifies the regex significantly. Rather than\n> trying to handle all the variants of keywords and return\n> types, we simply look for an identifier at the start of the\n> line that contains a \"(\", meaning it is either a function\n> definition or a function call, and then not containing \";\"\n> which would indicate it is a call or declaration.\n\nHere is a patch that I'm carrying around since... a while.\nWhat do you think?\n\nThe pattern I chose also catches variable definition, not just\nfunctions. That is what I need, but it hurts grep --function-context\nThat's the reason I didn't forward the patch, yet.\n\n--- 8< ---\nFrom: Johannes Sixt \nDate: Tue, 25 Sep 2012 14:08:02 +0200\nSubject: [PATCH] userdiff: have 'cpp' hunk header pattern catch more C++ anchor points\n\nThe hunk header pattern 'cpp' is intended for C and C++ source code, but\nit is actually not very useful for the latter, and even hurts some\nuse-cases for the former.\n\nThe parts of the pattern have the following flaws:\n\n- The first part matches an identifier followed immediately by a colon and\n arbitrary text and is intended to reject goto labels and C++ access\n specifiers (public, private, protected). But this pattern also rejects\n C++ constructs, which look like this:\n\n MyClass::MyClass()\n MyClass::~MyClass()\n MyClass::Item MyClass::Find(...\n\n- The second part matches an identifier followed by a list of qualified\n names (i.e. identifiers separated by the C++ scope operator '::')\n separated by space or '*' followed by an opening parenthesis (with space\n between the tokens). It matches function declarations like\n\n struct item* get_head(...\n int Outer::Inner::Func(...\n\n Since the pattern requires at least two identifiers, GNU-style function\n definitions are ignored:\n\n void\n func(...\n\n Moreover, since the pattern does not allow punctuation other than '*',\n the following C++ constructs are not recognized:\n\n . template definitions:\n template int func(T arg)\n\n . functions returning references:\n const string& get_message()\n\n . functions returning templated types:\n vector foo()\n\n . operator definitions:\n Value operator+(Value l, Value r)\n\n- The third part of the pattern finally matches compound definitions. But\n it forgets about unions and namespaces, and also skips single-line\n definitions\n\n struct random_iterator_tag {};\n\n because no semicolon can occur on the line.\n\nChange the first pattern to require a colon at the end of the line (except\nfor trailing space and comments), so that it does not reject constructor\nor destructor definitions.\n\nNotice that all interesting anchor points begin with an identifier or\nkeyword. But since there is a large variety of syntactical constructs after\nthe first \"word\", the simplest is to require only this word and accept\neverything else. Therefore, this boils down to a line that begins with a\nletter or underscore (optionally preceded by the C++ scope operator '::'\nto accept functions returning a type anchored at the global namespace).\nReplace the second and third part by a single pattern that picks such a\nline.\n\nThis has the following desirable consequence:\n\n- All constructs mentioned above are recognized.\n\nand the following likely desirable consequences:\n\n- Definitions of global variables and typedefs are recognized:\n\n int num_entries = 0;\n extern const char* help_text;\n typedef basic_string wstring;\n\n- Commonly used marco-ized boilerplate code is recognized:\n\n BEGIN_MESSAGE_MAP(CCanvas,CWnd)\n Q_DECLARE_METATYPE(MyStruct)\n PATTERNS(\"tex\",...)\n\n (The last one is from this very patch.)\n\nbut also the following possibly undesirable consequence:\n\n- When a label is not on a line by itself (except for a comment) it is no\n longer rejected, but can appear as a hunk header if it occurs at the\n beginning of a line:\n\n next:;\n\nIMO, the benefits of the change outweigh the (possible) regressions by a\nlarge margin.\n\nSigned-off-by: Johannes Sixt \n---\n userdiff.c | 8 +++-----\n 13 files changed, 3 insertions(+), 17 deletions(-)\n\ndiff --git a/userdiff.c b/userdiff.c\nindex ed958ef..49b2094 100644\n--- a/userdiff.c\n+++ b/userdiff.c\n@@ -125,11 +125,9 @@ PATTERNS(\"tex\", \"^(\\\\\\\\((sub)*section|chapter|part)\\\\*{0,1}\\\\{.*)$\",\n \t \"\\\\\\\\[a-zA-Z@]+|\\\\\\\\.|[a-zA-Z0-9\\x80-\\xff]+\"),\n PATTERNS(\"cpp\",\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 /* compound type at top level */\n-\t \"^((struct|class|enum)[^;]*)$\",\n+\t \"!^[ \\t]*[A-Za-z_][A-Za-z_0-9]*:[[:space:]]*($|/[/*])\\n\"\n+\t /* functions/methods, variables, and compounds at top level */\n+\t \"^((::[[:space:]]*)?[A-Za-z_].*)$\",\n \t /* -- */\n \t \"[a-zA-Z_][a-zA-Z0-9_]*\"\n \t \"|[-+0-9.e]+[fFlL]?|0[xXbB]?[0-9a-fA-F]+[lL]?\"\n-- \n1.9.0.1398.g59a4f1b\n"},{"id":"236125","messageId":"xmqqzjl4fkuh.fsf@gitster.dls.corp.google.com","threadId":"36060","inReplyTo":"20140305003639.GA9474@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] diff: simplify cpp funcname regex","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-05T20:31:50Z","receivedAt":"2014-03-05T20:31:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> types, we simply look for an identifier at the start of the\n> line that contains a \"(\", meaning it is either a function\n> definition or a function call, and then not containing \";\"\n> which would indicate it is a call or declaration.\n\nIt is not worth worrying about:\n\nfoo(arg,\n another);\n\nthat is not indented, so I think that simplicity is good.\n\n> For example, for top-level changes\n> outside functions, we might find:\n>\n> N_(\"some text that is long\"\n>\n> that is part of:\n>\n> const char *foo =\n> N_(\"some text that is long\"\n> \"and spans multiple lines\");\n\nUnfortunate, but cannot be avoided.\n\n>\n> Before this change, we would skip past it (using the cpp regex, that is;\n> the default one tends to find the same line) and either report nothing,\n> or whatever random function was before us. So it's a behavior change,\n> but the existing behavior is really no better.\n\nTrue.\n"},{"id":"236204","messageId":"20140306212835.GA11743@sigill.intra.peff.net","threadId":"36060","inReplyTo":"5316D922.9010501@viscovery.net","subject":"Re: [RFC/PATCH] diff: simplify cpp funcname regex","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-06T21:28:36Z","receivedAt":"2014-03-06T21:28:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 05, 2014 at 08:58:26AM +0100, Johannes Sixt wrote:\n\n> Here is a patch that I'm carrying around since... a while.\n> What do you think?\n> \n> The pattern I chose also catches variable definition, not just\n> functions. That is what I need, but it hurts grep --function-context\n> That's the reason I didn't forward the patch, yet.\n\nIf by variable definition you mean:\n\n struct foo bar = {\n - old\n + new\n };\n\nI'd think that would be covered by the existing \"struct|class|enum\".\nThough I think we'd want to also allow keywords in front of it, like\n\"static\". I suspect the original was more meant to find:\n\n struct foo {\n -old\n +new\n };\n\n> The parts of the pattern have the following flaws:\n> \n> - The first part matches an identifier followed immediately by a colon and\n> arbitrary text and is intended to reject goto labels and C++ access\n> specifiers (public, private, protected). But this pattern also rejects\n> C++ constructs, which look like this:\n> \n> MyClass::MyClass()\n> MyClass::~MyClass()\n> MyClass::Item MyClass::Find(...\n\nMakes sense. I noticed your fix is to look for end-of-line or comments\nafterwards. Would it be simpler to just check for a non-colon, like:\n\n !^[ \\t]*[A-Za-z_][A-Za-z_0-9]*:($|[^:])\n\n> - The second part matches an identifier followed by a list of qualified\n> names (i.e. identifiers separated by the C++ scope operator '::')\n> [...]\n\nA tried to keep the \"looks like a function definition\" bit in mine, and\nyours loosens this quite a bit more. I think that may be OK. That is, I\ndo not think there is any reason for somebody to do:\n\n void foo() {\n call_to_bar();\n -old\n +new\n }\n\nThat is, nobody would put a function _call_ without indentation. If\nsomething has alphanumerics at the left-most column, then it is probably\ninteresting no matter what.\n\n> - The third part of the pattern finally matches compound definitions. But\n> it forgets about unions and namespaces, and also skips single-line\n> definitions\n> \n> struct random_iterator_tag {};\n> \n> because no semicolon can occur on the line.\n\nI don't see how that is an interesting line. The point is to find a\nblock that is surrounding the changes, but that is not surrounding\nthe lines below.\n\n> Notice that all interesting anchor points begin with an identifier or\n> keyword. But since there is a large variety of syntactical constructs after\n> the first \"word\", the simplest is to require only this word and accept\n> everything else. Therefore, this boils down to a line that begins with a\n> letter or underscore (optionally preceded by the C++ scope operator '::'\n> to accept functions returning a type anchored at the global namespace).\n> Replace the second and third part by a single pattern that picks such a\n> line.\n\nYeah, this bit makes sense to me.\n\nBoth yours and mine will find the first line here in things like:\n\n void foo(void);\n -void bar(void);\n +void bar(int arg);\n\nbut I think that is OK. There _isn't_ any interesting surrounding\ncontext here. The current code will sometimes come up with an empty\nfuncline (which is good), but it may just as easily come up with a\ntotally bogus funcline in a case like:\n\n void unrelated(void)\n {\n }\n\n void foo(void);\n -void bar(void);\n +void bar(int arg);\n\nSo trying to be very restrictive and say \"that doesn't look like a\nfunction\" does not really buy us anything (and it creates tons of false\nnegatives, as you documented, because C++ syntax has all kinds of crazy\nstuff).\n\n_If_ the backwards search learned to terminate (e.g., seeing the \"^}\"\nline and saying \"well, we can't be inside a function\"), then such\nnegative lines might be useful for coming up with an empty funcname\nrather than the bogus \"void foo(void);\". But we do not do that\ncurrently, and I do not think it is that useful (the funcname above is\narguably just as or more useful than an empty one).\n\n-Peff\n"},{"id":"236250","messageId":"531973D9.9070803@viscovery.net","threadId":"36060","inReplyTo":"20140306212835.GA11743@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] diff: simplify cpp funcname regex","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2014-03-07T07:23:05Z","receivedAt":"2014-03-07T07:23:05Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 3/6/2014 22:28, schrieb Jeff King:\n> On Wed, Mar 05, 2014 at 08:58:26AM +0100, Johannes Sixt wrote:\n>> The pattern I chose also catches variable definition, not just\n>> functions. That is what I need, but it hurts grep --function-context\n>> That's the reason I didn't forward the patch, yet.\n> \n> If by variable definition you mean:\n> \n> struct foo bar = {\n> - old\n> + new\n> };\n> \n> I'd think that would be covered by the existing \"struct|class|enum\".\n> Though I think we'd want to also allow keywords in front of it, like\n> \"static\". I suspect the original was more meant to find:\n> \n> struct foo {\n> -old\n> +new\n> };\n\nNo, I meant lines like\n\n static double var;\n -static int old;\n +static int new;\n\nThe motivation is to show hints where in a file the change is located:\nAnything that could be of significance for the author should be picked up.\n\nBut that does not necessarily help grep --function-context. For example,\nwhen there is a longish block of global variable definitions and there is\na match in the middle, then --function-context would provide no context\nbecause the line itself would be regarded as the beginning of a\n\"function\", i.e., the context, and the next line (which also matches the\npattern) would be the beginning of the *next* function, and would not be\nin the context anymore.\n\n> \n>> The parts of the pattern have the following flaws:\n>>\n>> - The first part matches an identifier followed immediately by a colon and\n>> arbitrary text and is intended to reject goto labels and C++ access\n>> specifiers (public, private, protected). But this pattern also rejects\n>> C++ constructs, which look like this:\n>>\n>> MyClass::MyClass()\n>> MyClass::~MyClass()\n>> MyClass::Item MyClass::Find(...\n> \n> Makes sense. I noticed your fix is to look for end-of-line or comments\n> afterwards. Would it be simpler to just check for a non-colon, like:\n> \n> !^[ \\t]*[A-Za-z_][A-Za-z_0-9]*:($|[^:])\n\nI want to match [[:space:]] after the label's colon, because I have lot's\nof C++ files with CRLF, and I need to match the CR. Your more liberal\npattern would fit as well, although it would pick up a bit field as in\n\n struct foo {\n unsigned\n flag: 1;\n -old\n +new\n\nI would not mind ignoring this case (\"garbage in, garbage out\" ;-).\n\n>> - The second part matches an identifier followed by a list of qualified\n>> names (i.e. identifiers separated by the C++ scope operator '::')\n>> [...]\n> \n> A tried to keep the \"looks like a function definition\" bit in mine, and\n> yours loosens this quite a bit more. I think that may be OK. That is, I\n> do not think there is any reason for somebody to do:\n> \n> void foo() {\n> call_to_bar();\n> -old\n> +new\n> }\n> \n> That is, nobody would put a function _call_ without indentation. If\n> something has alphanumerics at the left-most column, then it is probably\n> interesting no matter what.\n> \n>> - The third part of the pattern finally matches compound definitions. But\n>> it forgets about unions and namespaces, and also skips single-line\n>> definitions\n>>\n>> struct random_iterator_tag {};\n>>\n>> because no semicolon can occur on the line.\n> \n> I don't see how that is an interesting line. The point is to find a\n> block that is surrounding the changes, but that is not surrounding\n> the lines below.\n\nI more often than not want to have an answer to the question \"where?\", not\n\"wherein?\" Then anything that helps locate a hunk is useful.\n\n(The particular example, an empty struct, looks strange for C programmers,\nof course, but it's a common idiom in C++ when it comes to template\nmeta-programming.)\n\n>> Notice that all interesting anchor points begin with an identifier or\n>> keyword. But since there is a large variety of syntactical constructs after\n>> the first \"word\", the simplest is to require only this word and accept\n>> everything else. Therefore, this boils down to a line that begins with a\n>> letter or underscore (optionally preceded by the C++ scope operator '::'\n>> to accept functions returning a type anchored at the global namespace).\n>> Replace the second and third part by a single pattern that picks such a\n>> line.\n> \n> Yeah, this bit makes sense to me.\n> \n> Both yours and mine will find the first line here in things like:\n> \n> void foo(void);\n> -void bar(void);\n> +void bar(int arg);\n> \n> but I think that is OK. There _isn't_ any interesting surrounding\n> context here. The current code will sometimes come up with an empty\n> funcline (which is good), but it may just as easily come up with a\n> totally bogus funcline in a case like:\n> \n> void unrelated(void)\n> {\n> }\n> \n> void foo(void);\n> -void bar(void);\n> +void bar(int arg);\n> \n> So trying to be very restrictive and say \"that doesn't look like a\n> function\" does not really buy us anything (and it creates tons of false\n> negatives, as you documented, because C++ syntax has all kinds of crazy\n> stuff).\n> \n> _If_ the backwards search learned to terminate (e.g., seeing the \"^}\"\n> line and saying \"well, we can't be inside a function\"), then such\n> negative lines might be useful for coming up with an empty funcname\n> rather than the bogus \"void foo(void);\". But we do not do that\n> currently, and I do not think it is that useful (the funcname above is\n> arguably just as or more useful than an empty one).\n\nAs I said, my motivation is not so much to find a \"container\", but rather\na clue to help locate a change while reading the patch text. I can speak\nfor myself, but I have no idea what is more important for the majority.\n\n-- Hannes\n"},{"id":"236708","messageId":"20140314035457.GA31906@sigill.intra.peff.net","threadId":"36060","inReplyTo":"531973D9.9070803@viscovery.net","subject":"Re: [RFC/PATCH] diff: simplify cpp funcname regex","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-14T03:54:57Z","receivedAt":"2014-03-14T03:54:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 07, 2014 at 08:23:05AM +0100, Johannes Sixt wrote:\n\n> No, I meant lines like\n> \n> static double var;\n> -static int old;\n> +static int new;\n> \n> The motivation is to show hints where in a file the change is located:\n> Anything that could be of significance for the author should be picked up.\n\nI see. Coupled with what you said below:\n\n> As I said, my motivation is not so much to find a \"container\", but rather\n> a clue to help locate a change while reading the patch text. I can speak\n> for myself, but I have no idea what is more important for the majority.\n\nyour proposal makes a lot more sense to me, and I think that is really\nat the center of our discussion. I do not have a gut feeling for which\nis \"more right\" (i.e., \"container\" versus \"context\").\n\nBut given that most of the cases we are discussing are ones where the\n\"diff -p\" default regex gets it right (or at least better than) compared\nto the C regex, I am tempted to say that we should be erring in the\ndirection of simplicity, and just finding interesting lines without\nworrying about containers (i.e., it argues for your patch).\n\n> > Makes sense. I noticed your fix is to look for end-of-line or comments\n> > afterwards. Would it be simpler to just check for a non-colon, like:\n> > \n> > !^[ \\t]*[A-Za-z_][A-Za-z_0-9]*:($|[^:])\n> \n> I want to match [[:space:]] after the label's colon, because I have lot's\n> of C++ files with CRLF, and I need to match the CR. Your more liberal\n> pattern would fit as well, although it would pick up a bit field as in\n> \n> struct foo {\n> unsigned\n> flag: 1;\n> -old\n> +new\n\nThanks, I was having trouble thinking of another good use of a colon,\nbut bitfields are what I was missing. Your pattern is probably better\nhere.\n\nSo I am leaning towards your patch, but I'm not sure if I understand all\nof the implications for \"git grep\". Can you give some concrete examples?\n\n-Peff\n"},{"id":"236720","messageId":"5322A82E.9060808@viscovery.net","threadId":"36060","inReplyTo":"20140314035457.GA31906@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] diff: simplify cpp funcname regex","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2014-03-14T06:56:46Z","receivedAt":"2014-03-14T06:56:46Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 3/14/2014 4:54, schrieb Jeff King:\n> On Fri, Mar 07, 2014 at 08:23:05AM +0100, Johannes Sixt wrote:\n> \n>> No, I meant lines like\n>>\n>> static double var;\n>> -static int old;\n>> +static int new;\n>>\n>> The motivation is to show hints where in a file the change is located:\n>> Anything that could be of significance for the author should be picked up.\n> \n> I see. Coupled with what you said below:\n> \n>> As I said, my motivation is not so much to find a \"container\", but rather\n>> a clue to help locate a change while reading the patch text. I can speak\n>> for myself, but I have no idea what is more important for the majority.\n> \n> your proposal makes a lot more sense to me, and I think that is really\n> at the center of our discussion. I do not have a gut feeling for which\n> is \"more right\" (i.e., \"container\" versus \"context\").\n> \n> But given that most of the cases we are discussing are ones where the\n> \"diff -p\" default regex gets it right (or at least better than) compared\n> to the C regex, I am tempted to say that we should be erring in the\n> direction of simplicity, and just finding interesting lines without\n> worrying about containers (i.e., it argues for your patch).\n\nWe are in agreement here. So, let's base a decision on the implications on\ngit grep.\n\n> ... but I'm not sure if I understand all\n> of the implications for \"git grep\". Can you give some concrete examples?\n\nConsider this code:\n\n void above()\n {}\n static int Y;\n static int A;\n int bar()\n {\n return X;\n }\n void below()\n {}\n\nWhen you 'git grep --function-context X', then you get this output with\nthe current pattern, you proposal, and my proposal (file name etc omitted\nfor brevity):\n\n int bar()\n {\n return X;\n }\n\nbecause we start the context at the last hunk header pattern above *or\nincluding the matching line* (and write it in the output), and we stop at\nthe next hunk header pattern below the match (and do not write it in the\noutput).\n\nWhen you 'git grep --function-context Y', what do you want to see? With\nthe current pattern, and with your pattern that forbids semicolon we get:\n\n void above()\n {}\n static int Y;\n static int A;\n\nand with my simple pattern, which allows semicolon, we get merely\n\n static int Y;\n\nbecause the line itself is a hunk header (and we do not look back any\nfurther) and the next line is as well. That is not exactly \"function\ncontext\", and that is what I'm a bit worried about.\n\n-- Hannes\n"},{"id":"236963","messageId":"20140318052456.GA17454@sigill.intra.peff.net","threadId":"36060","inReplyTo":"5322A82E.9060808@viscovery.net","subject":"Re: [RFC/PATCH] diff: simplify cpp funcname regex","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-18T05:24:56Z","receivedAt":"2014-03-18T05:24:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 14, 2014 at 07:56:46AM +0100, Johannes Sixt wrote:\n\n> Consider this code:\n> \n> void above()\n> {}\n> static int Y;\n> static int A;\n> int bar()\n> {\n> return X;\n> }\n> void below()\n> {}\n\nThanks, this example is very helpful.\n\n> When you 'git grep --function-context X', then you get this output with\n> the current pattern, you proposal, and my proposal (file name etc omitted\n> for brevity):\n> \n> int bar()\n> {\n> return X;\n> }\n\nRight, that makes sense to me.\n\n> When you 'git grep --function-context Y', what do you want to see? With\n> the current pattern, and with your pattern that forbids semicolon we get:\n> \n> void above()\n> {}\n> static int Y;\n> static int A;\n> \n> and with my simple pattern, which allows semicolon, we get merely\n> \n> static int Y;\n> \n> because the line itself is a hunk header (and we do not look back any\n> further) and the next line is as well. That is not exactly \"function\n> context\", and that is what I'm a bit worried about.\n\nHmm. To be honest, I do not see yours as all that bad. Is \"above()\" or\n\"A\" actually interesting here? I'm not sure that they are. But then I do\nnot use --function-context myself.\n\nI guess it violates the \"show things that are vaguely nearby, rather\nthan a container\" view of context that we discussed earlier. But somehow\nthat seems less important to me with \"--function-context\".\n\nSo I dunno. I kind of like your version.\n\n-Peff\n"},{"id":"236968","messageId":"5327FD81.4040508@viscovery.net","threadId":"36060","inReplyTo":"20140318052456.GA17454@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] diff: simplify cpp funcname regex","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2014-03-18T08:02:09Z","receivedAt":"2014-03-18T08:02:09Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Cc René; do you have any comments regarding grep --function-context?\n\nAm 3/18/2014 6:24, schrieb Jeff King:\n> On Fri, Mar 14, 2014 at 07:56:46AM +0100, Johannes Sixt wrote:\n> \n>> Consider this code:\n>>\n>> void above()\n>> {}\n>> static int Y;\n>> static int A;\n>> int bar()\n>> {\n>> return X;\n>> }\n>> void below()\n>> {}\n> \n> Thanks, this example is very helpful.\n> \n>> When you 'git grep --function-context X', then you get this output with\n>> the current pattern, you proposal, and my proposal (file name etc omitted\n>> for brevity):\n>>\n>> int bar()\n>> {\n>> return X;\n>> }\n> \n> Right, that makes sense to me.\n> \n>> When you 'git grep --function-context Y', what do you want to see? With\n>> the current pattern, and with your pattern that forbids semicolon we get:\n>>\n>> void above()\n>> {}\n>> static int Y;\n>> static int A;\n>>\n>> and with my simple pattern, which allows semicolon, we get merely\n>>\n>> static int Y;\n>>\n>> because the line itself is a hunk header (and we do not look back any\n>> further) and the next line is as well. That is not exactly \"function\n>> context\", and that is what I'm a bit worried about.\n> \n> Hmm. To be honest, I do not see yours as all that bad. Is \"above()\" or\n> \"A\" actually interesting here? I'm not sure that they are. But then I do\n> not use --function-context myself.\n> \n> I guess it violates the \"show things that are vaguely nearby, rather\n> than a container\" view of context that we discussed earlier. But somehow\n> that seems less important to me with \"--function-context\".\n> \n> So I dunno. I kind of like your version.\n\nThen I'll polish my patch series (it also rewrites the test framework) and\npost it.\n\n-- Hannes\n"},{"id":"236980","messageId":"53282741.5010609@web.de","threadId":"36060","inReplyTo":"5327FD81.4040508@viscovery.net","subject":"Re: [RFC/PATCH] diff: simplify cpp funcname regex","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-03-18T11:00:17Z","receivedAt":"2014-03-18T11:00:17Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 18.03.2014 09:02, schrieb Johannes Sixt:\n> Cc René; do you have any comments regarding grep --function-context?\n>\n> Am 3/18/2014 6:24, schrieb Jeff King:\n>> On Fri, Mar 14, 2014 at 07:56:46AM +0100, Johannes Sixt wrote:\n>>\n>>> Consider this code:\n>>>\n>>> void above()\n>>> {}\n>>> static int Y;\n>>> static int A;\n>>> int bar()\n>>> {\n>>> return X;\n>>> }\n>>> void below()\n>>> {}\n>>\n>> Thanks, this example is very helpful.\n>>\n>>> When you 'git grep --function-context X', then you get this output with\n>>> the current pattern, you proposal, and my proposal (file name etc omitted\n>>> for brevity):\n>>>\n>>> int bar()\n>>> {\n>>> return X;\n>>> }\n>>\n>> Right, that makes sense to me.\n>>\n>>> When you 'git grep --function-context Y', what do you want to see? With\n>>> the current pattern, and with your pattern that forbids semicolon we get:\n>>>\n>>> void above()\n>>> {}\n>>> static int Y;\n>>> static int A;\n>>>\n>>> and with my simple pattern, which allows semicolon, we get merely\n>>>\n>>> static int Y;\n>>>\n>>> because the line itself is a hunk header (and we do not look back any\n>>> further) and the next line is as well. That is not exactly \"function\n>>> context\", and that is what I'm a bit worried about.\n\nIn global context there is no \"function context\", of course, so the \nlatter makes sense.\n\n\"grep --function-context\" is about useful context and its implementation \npiggy-backs on the hunk header definitions. If those are useful then \nthe grep output should be fine as well. IAW: No worries, go ahead. :)\n\nHowever, I only use the defaults heuristic (which shows just the Y-line \nas well) and don't know C++, so I my opinion on this matter isn't worth \nthat much.\n\nRené\n"},{"id":"237361","messageId":"cover.1395433874.git.j6t@kdbg.org","threadId":"36060","inReplyTo":"53282741.5010609@web.de","subject":"[PATCH 00/10] userdiff: cpp pattern simplification and test framework","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2014-03-21T21:07:12Z","receivedAt":"2014-03-21T21:07:12Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Here is a series that makes the hunk header pattern for C and C++ even\nsimpler than suggested by Peff in [1] to catch a lot more C++ functions\nand two more C patterns.\n\nAs a preparatory work, the test cases are totally rewritten to make it\na lot simpler to drop in new tests. There was an earlier attempt to\nchange the infrastructure [2], and it is the reason for the widened Cc\nlist.\n\nTwo patches also extend the word regexp.\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/243408\n[2] http://thread.gmane.org/gmane.comp.version-control.git/187269/focus=187497\n\nJohannes Sixt (10):\n userdiff: support C++ ->* and .* operators in the word regexp\n userdiff: support unsigned and long long suffixes of integer constants\n t4018: an infrastructure to test hunk headers\n t4018: convert perl pattern tests to the new infrastructure\n t4018: convert java pattern test to the new infrastructure\n t4018: convert custom pattern test to the new infrastructure\n t4018: reduce test files for pattern compilation tests\n t4018: test cases for the built-in cpp pattern\n t4018: test cases showing that the cpp pattern misses many anchor\n points\n userdiff: have 'cpp' hunk header pattern catch more C++ anchor points\n\n t/t4018-diff-funcname.sh | 230 ++++++++++-------------------\n t/t4018/README | 18 +++\n t/t4018/cpp-c++-function | 4 +\n t/t4018/cpp-class-constructor | 4 +\n t/t4018/cpp-class-constructor-mem-init | 5 +\n t/t4018/cpp-class-definition | 4 +\n t/t4018/cpp-class-definition-derived | 5 +\n t/t4018/cpp-class-destructor | 4 +\n t/t4018/cpp-function-returning-global-type | 4 +\n t/t4018/cpp-function-returning-nested | 5 +\n t/t4018/cpp-function-returning-pointer | 4 +\n t/t4018/cpp-function-returning-reference | 4 +\n t/t4018/cpp-gnu-style-function | 5 +\n t/t4018/cpp-namespace-definition | 4 +\n t/t4018/cpp-operator-definition | 4 +\n t/t4018/cpp-skip-access-specifiers | 8 +\n t/t4018/cpp-skip-comment-block | 9 ++\n t/t4018/cpp-skip-labels | 8 +\n t/t4018/cpp-struct-definition | 9 ++\n t/t4018/cpp-struct-single-line | 7 +\n t/t4018/cpp-template-function-definition | 4 +\n t/t4018/cpp-union-definition | 4 +\n t/t4018/cpp-void-c-function | 4 +\n t/t4018/custom1-pattern | 17 +++\n t/t4018/custom2-match-to-end-of-line | 8 +\n t/t4018/custom3-alternation-in-pattern | 17 +++\n t/t4018/java-class-member-function | 8 +\n t/t4018/perl-skip-end-of-heredoc | 8 +\n t/t4018/perl-skip-forward-decl | 10 ++\n t/t4018/perl-skip-sub-in-pod | 18 +++\n t/t4018/perl-sub-definition | 4 +\n t/t4018/perl-sub-definition-kr-brace | 4 +\n userdiff.c | 12 +-\n 33 files changed, 303 insertions(+), 160 deletions(-)\n create mode 100644 t/t4018/README\n create mode 100644 t/t4018/cpp-c++-function\n create mode 100644 t/t4018/cpp-class-constructor\n create mode 100644 t/t4018/cpp-class-constructor-mem-init\n create mode 100644 t/t4018/cpp-class-definition\n create mode 100644 t/t4018/cpp-class-definition-derived\n create mode 100644 t/t4018/cpp-class-destructor\n create mode 100644 t/t4018/cpp-function-returning-global-type\n create mode 100644 t/t4018/cpp-function-returning-nested\n create mode 100644 t/t4018/cpp-function-returning-pointer\n create mode 100644 t/t4018/cpp-function-returning-reference\n create mode 100644 t/t4018/cpp-gnu-style-function\n create mode 100644 t/t4018/cpp-namespace-definition\n create mode 100644 t/t4018/cpp-operator-definition\n create mode 100644 t/t4018/cpp-skip-access-specifiers\n create mode 100644 t/t4018/cpp-skip-comment-block\n create mode 100644 t/t4018/cpp-skip-labels\n create mode 100644 t/t4018/cpp-struct-definition\n create mode 100644 t/t4018/cpp-struct-single-line\n create mode 100644 t/t4018/cpp-template-function-definition\n create mode 100644 t/t4018/cpp-union-definition\n create mode 100644 t/t4018/cpp-void-c-function\n create mode 100644 t/t4018/custom1-pattern\n create mode 100644 t/t4018/custom2-match-to-end-of-line\n create mode 100644 t/t4018/custom3-alternation-in-pattern\n create mode 100644 t/t4018/java-class-member-function\n create mode 100644 t/t4018/perl-skip-end-of-heredoc\n create mode 100644 t/t4018/perl-skip-forward-decl\n create mode 100644 t/t4018/perl-skip-sub-in-pod\n create mode 100644 t/t4018/perl-sub-definition\n create mode 100644 t/t4018/perl-sub-definition-kr-brace\n\n-- \n1.8.5.2.244.g9fb3fb1\n"},{"id":"237362","messageId":"293a6d890efa25bb6c2887afe75b11a47edd8810.1395433874.git.j6t@kdbg.org","threadId":"36060","inReplyTo":"cover.1395433874.git.j6t@kdbg.org","subject":"[PATCH 01/10] userdiff: support C++ ->* and .* operators in the word regexp","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2014-03-21T21:07:13Z","receivedAt":"2014-03-21T21:07:13Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"The character sequences ->* and .* are valid C++ operators. Keep them\ntogether in --word-diff mode.\n\nSigned-off-by: Johannes Sixt \n---\n userdiff.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/userdiff.c b/userdiff.c\nindex 10b61ec..434535b 100644\n--- a/userdiff.c\n+++ b/userdiff.c\n@@ -133,7 +133,7 @@\n \t /* -- */\n \t \"[a-zA-Z_][a-zA-Z0-9_]*\"\n \t \"|[-+0-9.e]+[fFlL]?|0[xXbB]?[0-9a-fA-F]+[lL]?\"\n-\t \"|[-+*/<>%&^|=!]=|--|\\\\+\\\\+|<<=?|>>=?|&&|\\\\|\\\\||::|->\"),\n+\t \"|[-+*/<>%&^|=!]=|--|\\\\+\\\\+|<<=?|>>=?|&&|\\\\|\\\\||::|->\\\\*?|\\\\.\\\\*\"),\n PATTERNS(\"csharp\",\n \t /* Keywords */\n \t \"!^[ \\t]*(do|while|for|if|else|instanceof|new|return|switch|case|throw|catch|using)\\n\"\n-- \n1.8.5.2.244.g9fb3fb1\n"},{"id":"237363","messageId":"449a75237d45739084efddbcaf545db4b0a0a40a.1395433874.git.j6t@kdbg.org","threadId":"36060","inReplyTo":"cover.1395433874.git.j6t@kdbg.org","subject":"[PATCH 02/10] userdiff: support unsigned and long long suffixes of integer constants","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2014-03-21T21:07:14Z","receivedAt":"2014-03-21T21:07:14Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Do not split constants such as 123U, 456ll, 789UL at the first U or\nsecond L.\n\nSigned-off-by: Johannes Sixt \n---\n userdiff.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/userdiff.c b/userdiff.c\nindex 434535b..8830417 100644\n--- a/userdiff.c\n+++ b/userdiff.c\n@@ -132,7 +132,7 @@\n \t \"^((struct|class|enum)[^;]*)$\",\n \t /* -- */\n \t \"[a-zA-Z_][a-zA-Z0-9_]*\"\n-\t \"|[-+0-9.e]+[fFlL]?|0[xXbB]?[0-9a-fA-F]+[lL]?\"\n+\t \"|[-+0-9.e]+[fFlL]?|0[xXbB]?[0-9a-fA-F]+[lLuU]*\"\n \t \"|[-+*/<>%&^|=!]=|--|\\\\+\\\\+|<<=?|>>=?|&&|\\\\|\\\\||::|->\\\\*?|\\\\.\\\\*\"),\n PATTERNS(\"csharp\",\n \t /* Keywords */\n-- \n1.8.5.2.244.g9fb3fb1\n"},{"id":"237364","messageId":"52505977c20a480941ae1f85f50ffb7a0bbffedb.1395433874.git.j6t@kdbg.org","threadId":"36060","inReplyTo":"cover.1395433874.git.j6t@kdbg.org","subject":"[PATCH 03/10] t4018: an infrastructure to test hunk headers","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2014-03-21T21:07:15Z","receivedAt":"2014-03-21T21:07:15Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Add an infrastructure that simplifies adding new tests of the hunk\nheader regular expressions.\n\nTo add new tests, a file with the syntax to test can be dropped in the\ndirectory t4018. The README file explains how a test file must contain;\nthe README itself tests the default behavior.\n\nSigned-off-by: Johannes Sixt \n---\n t/t4018-diff-funcname.sh | 60 +++++++++++++++++++++++++++++++++++++++++++-----\n t/t4018/README | 18 +++++++++++++++\n 2 files changed, 72 insertions(+), 6 deletions(-)\n create mode 100644 t/t4018/README\n\ndiff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh\nindex 38a092a..b467d9e 100755\n--- a/t/t4018-diff-funcname.sh\n+++ b/t/t4018-diff-funcname.sh\n@@ -100,7 +100,25 @@ test_expect_funcname () {\n \tgrep \"^@@.*@@ $1\" diff\n }\n \n-for p in ada bibtex cpp csharp fortran html java matlab objc pascal perl php python ruby tex\n+diffpatterns=\"\n+\tada\n+\tbibtex\n+\tcpp\n+\tcsharp\n+\tfortran\n+\thtml\n+\tjava\n+\tmatlab\n+\tobjc\n+\tpascal\n+\tperl\n+\tphp\n+\tpython\n+\truby\n+\ttex\n+\"\n+\n+for p in $diffpatterns\n do\n \ttest_expect_success \"builtin $p pattern compiles\" '\n \t\techo \"*.java diff=$p\" >.gitattributes &&\n@@ -118,11 +136,6 @@ do\n \t'\n done\n \n-test_expect_success 'default behaviour' '\n-\trm -f .gitattributes &&\n-\ttest_expect_funcname \"public class Beer\\$\"\n-'\n-\n test_expect_success 'set up .gitattributes declaring drivers to test' '\n \tcat >.gitattributes <<-\\EOF\n \t*.java diff=java\n@@ -182,4 +195,39 @@ test_expect_success 'alternation in pattern' '\n \ttest_expect_funcname \"public static void main(\"\n '\n \n+test_expect_success 'setup hunk header tests' '\n+\tfor i in $diffpatterns\n+\tdo\n+\t\techo \"$i-* diff=$i\"\n+\tdone > .gitattributes &&\n+\n+\t# add all test files to the index\n+\t(\n+\t\tcd \"$TEST_DIRECTORY\"/t4018 &&\n+\t\tgit --git-dir=\"$TRASH_DIRECTORY/.git\" add .\n+\t) &&\n+\n+\t# place modified files in the worktree\n+\tfor i in $(git ls-files)\n+\tdo\n+\t\tsed -e \"s/ChangeMe/IWasChanged/\" <\"$TEST_DIRECTORY/t4018/$i\" >\"$i\" || return 1\n+\tdone\n+'\n+\n+# check each individual file\n+for i in $(git ls-files)\n+do\n+\tif grep broken \"$i\" >/dev/null 2>&1\n+\tthen\n+\t\tresult=failure\n+\telse\n+\t\tresult=success\n+\tfi\n+\ttest_expect_$result \"hunk header: $i\" \"\n+\t\ttest_when_finished 'cat actual' &&\t# for debugging only\n+\t\tgit diff -U1 $i >actual &&\n+\t\tgrep '@@ .* @@.*RIGHT' actual\n+\t\"\n+done\n+\n test_done\ndiff --git a/t/t4018/README b/t/t4018/README\nnew file mode 100644\nindex 0000000..283e01cc\n--- /dev/null\n+++ b/t/t4018/README\n@@ -0,0 +1,18 @@\n+How to write RIGHT test cases\n+=============================\n+\n+Insert the word \"ChangeMe\" (exactly this form) at a distance of\n+at least two lines from the line that must appear in the hunk header.\n+\n+The text that must appear in the hunk header must contain the word\n+\"right\", but in all upper-case, like in the title above.\n+\n+To mark a test case that highlights a malfunction, insert the word\n+BROKEN in all lower-case somewhere in the file.\n+\n+This text is a bit twisted and out of order, but it is itself a\n+test case for the default hunk header pattern. Know what you are doing\n+if you change it.\n+\n+BTW, this tests that the head line goes to the hunk header, not the line\n+of equal signs.\n-- \n1.8.5.2.244.g9fb3fb1\n"},{"id":"237365","messageId":"1953f0c353a2fb5b51db53722d222d548a09fe72.1395433874.git.j6t@kdbg.org","threadId":"36060","inReplyTo":"cover.1395433874.git.j6t@kdbg.org","subject":"[PATCH 04/10] t4018: convert perl pattern tests to the new infrastructure","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2014-03-21T21:07:16Z","receivedAt":"2014-03-21T21:07:16Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"There is one subtlety: The old test case 'perl pattern gets full line of\nPOD header' does not have its own new test case, but the feature is\ntested nevertheless by placing the RIGHT tag at the end of the expected\nhunk header in t4018/perl-skip-sub-in-pod.\n\nSigned-off-by: Johannes Sixt \n---\n t/t4018-diff-funcname.sh | 88 ------------------------------------\n t/t4018/perl-skip-end-of-heredoc | 8 ++++\n t/t4018/perl-skip-forward-decl | 10 ++++\n t/t4018/perl-skip-sub-in-pod | 18 ++++++++\n t/t4018/perl-sub-definition | 4 ++\n t/t4018/perl-sub-definition-kr-brace | 4 ++\n 6 files changed, 44 insertions(+), 88 deletions(-)\n create mode 100644 t/t4018/perl-skip-end-of-heredoc\n create mode 100644 t/t4018/perl-skip-forward-decl\n create mode 100644 t/t4018/perl-skip-sub-in-pod\n create mode 100644 t/t4018/perl-sub-definition\n create mode 100644 t/t4018/perl-sub-definition-kr-brace\n\ndiff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh\nindex b467d9e..c94a5f4 100755\n--- a/t/t4018-diff-funcname.sh\n+++ b/t/t4018-diff-funcname.sh\n@@ -29,69 +29,6 @@ public class Beer\n }\n EOF\n sed 's/beer\\\\/beer,\\\\/' Beer-correct.java\n-cat >Beer.perl <<\\EOT\n-package Beer;\n-\n-use strict;\n-use warnings;\n-use parent qw(Exporter);\n-our @EXPORT_OK = qw(round finalround);\n-\n-sub other; # forward declaration\n-\n-# hello\n-\n-sub round {\n-\tmy ($n) = @_;\n-\tprint \"$n bottles of beer on the wall \";\n-\tprint \"$n bottles of beer\\n\";\n-\tprint \"Take one down, pass it around, \";\n-\t$n = $n - 1;\n-\tprint \"$n bottles of beer on the wall.\\n\";\n-}\n-\n-sub finalround\n-{\n-\tprint \"Go to the store, buy some more\\n\";\n-\tprint \"99 bottles of beer on the wall.\\n\");\n-}\n-\n-sub withheredocument {\n-\tprint <<\"EOF\"\n-decoy here-doc\n-EOF\n-\t# some lines of context\n-\t# to pad it out\n-\tprint \"hello\\n\";\n-}\n-\n-__END__\n-\n-=head1 NAME\n-\n-Beer - subroutine to output fragment of a drinking song\n-\n-=head1 SYNOPSIS\n-\n-\tuse Beer qw(round finalround);\n-\n-\tsub song {\n-\t\tfor (my $i = 99; $i > 0; $i--) {\n-\t\t\tround $i;\n-\t\t}\n-\t\tfinalround;\n-\t}\n-\n-\tsong;\n-\n-=cut\n-EOT\n-sed -e '\n-\ts/hello/goodbye/\n-\ts/beer\\\\/beer,\\\\/\n-\ts/more\\\\/more,\\\\/\n-\ts/song;/song();/\n-' Beer-correct.perl\n \n test_expect_funcname () {\n \tlang=${2-java}\n@@ -139,7 +76,6 @@ done\n test_expect_success 'set up .gitattributes declaring drivers to test' '\n \tcat >.gitattributes <<-\\EOF\n \t*.java diff=java\n-\t*.perl diff=perl\n \tEOF\n '\n \n@@ -147,30 +83,6 @@ test_expect_success 'preset java pattern' '\n \ttest_expect_funcname \"public static void main(\"\n '\n \n-test_expect_success 'preset perl pattern' '\n-\ttest_expect_funcname \"sub round {\\$\" perl\n-'\n-\n-test_expect_success 'perl pattern accepts K&R style brace placement, too' '\n-\ttest_expect_funcname \"sub finalround\\$\" perl\n-'\n-\n-test_expect_success 'but is not distracted by end of < 0; $i--) {\n+\t\t\tround $i;\n+\t\t}\n+\t\tfinalround;\n+\t}\n+\n+\tChangeMe;\n+\n+=cut\ndiff --git a/t/t4018/perl-sub-definition b/t/t4018/perl-sub-definition\nnew file mode 100644\nindex 0000000..a507d1f\n--- /dev/null\n+++ b/t/t4018/perl-sub-definition\n@@ -0,0 +1,4 @@\n+sub RIGHT {\n+\tmy ($n) = @_;\n+\tprint \"ChangeMe\";\n+}\ndiff --git a/t/t4018/perl-sub-definition-kr-brace b/t/t4018/perl-sub-definition-kr-brace\nnew file mode 100644\nindex 0000000..330b3df\n--- /dev/null\n+++ b/t/t4018/perl-sub-definition-kr-brace\n@@ -0,0 +1,4 @@\n+sub RIGHT\n+{\n+\tprint \"ChangeMe\\n\";\n+}\n-- \n1.8.5.2.244.g9fb3fb1\n"},{"id":"237366","messageId":"ebffda22f824ddf3d3fbe420872d43c3d667f799.1395433874.git.j6t@kdbg.org","threadId":"36060","inReplyTo":"cover.1395433874.git.j6t@kdbg.org","subject":"[PATCH 05/10] t4018: convert java pattern test to the new infrastructure","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2014-03-21T21:07:17Z","receivedAt":"2014-03-21T21:07:17Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Signed-off-by: Johannes Sixt \n---\n t/t4018-diff-funcname.sh | 4 ----\n t/t4018/java-class-member-function | 8 ++++++++\n 2 files changed, 8 insertions(+), 4 deletions(-)\n create mode 100644 t/t4018/java-class-member-function\n\ndiff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh\nindex c94a5f4..008325f 100755\n--- a/t/t4018-diff-funcname.sh\n+++ b/t/t4018-diff-funcname.sh\n@@ -79,10 +79,6 @@ test_expect_success 'set up .gitattributes declaring drivers to test' '\n \tEOF\n '\n \n-test_expect_success 'preset java pattern' '\n-\ttest_expect_funcname \"public static void main(\"\n-'\n-\n test_expect_success 'custom pattern' '\n \ttest_config diff.java.funcname \"!static\n !String\ndiff --git a/t/t4018/java-class-member-function b/t/t4018/java-class-member-function\nnew file mode 100644\nindex 0000000..298bc7a\n--- /dev/null\n+++ b/t/t4018/java-class-member-function\n@@ -0,0 +1,8 @@\n+public class Beer\n+{\n+\tint special;\n+\tpublic static void main(String RIGHT[])\n+\t{\n+\t\tSystem.out.print(\"ChangeMe\");\n+\t}\n+}\n-- \n1.8.5.2.244.g9fb3fb1\n"},{"id":"237369","messageId":"55ac94cf04b486da0dc6402557455b148971be01.1395433874.git.j6t@kdbg.org","threadId":"36060","inReplyTo":"cover.1395433874.git.j6t@kdbg.org","subject":"[PATCH 06/10] t4018: convert custom pattern test to the new infrastructure","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2014-03-21T21:07:18Z","receivedAt":"2014-03-21T21:07:18Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"For the test case \"matches to end of line\", extend the pattern by a few\nwildcards so that the pattern captures the \"RIGHT\" token, which is needed\nfor verification, without mentioning it in the pattern.\n\nSigned-off-by: Johannes Sixt \n---\n t/t4018-diff-funcname.sh | 40 ++++++++++++++--------------------\n t/t4018/custom1-pattern | 17 +++++++++++++++\n t/t4018/custom2-match-to-end-of-line | 8 +++++++\n t/t4018/custom3-alternation-in-pattern | 17 +++++++++++++++\n 4 files changed, 58 insertions(+), 24 deletions(-)\n create mode 100644 t/t4018/custom1-pattern\n create mode 100644 t/t4018/custom2-match-to-end-of-line\n create mode 100644 t/t4018/custom3-alternation-in-pattern\n\ndiff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh\nindex 008325f..5ac744f 100755\n--- a/t/t4018-diff-funcname.sh\n+++ b/t/t4018-diff-funcname.sh\n@@ -30,12 +30,19 @@ public class Beer\n EOF\n sed 's/beer\\\\/beer,\\\\/' Beer-correct.java\n \n-test_expect_funcname () {\n-\tlang=${2-java}\n-\ttest_expect_code 1 git diff --no-index -U1 \\\n-\t\t\"Beer.$lang\" \"Beer-correct.$lang\" >diff &&\n-\tgrep \"^@@.*@@ $1\" diff\n-}\n+test_expect_success 'setup' '\n+\t# a non-trivial custom pattern\n+\tgit config diff.custom1.funcname \"!static\n+!String\n+[^ \t].*s.*\" &&\n+\n+\t# a custom pattern which matches to end of line\n+\tgit config diff.custom2.funcname \"......Beer\\$\" &&\n+\n+\t# alternation in pattern\n+\tgit config diff.custom3.funcname \"Beer$\" &&\n+\tgit config diff.custom3.xfuncname \"^[ \t]*((public|static).*)$\"\n+'\n \n diffpatterns=\"\n \tada\n@@ -53,6 +60,9 @@ diffpatterns=\"\n \tpython\n \truby\n \ttex\n+\tcustom1\n+\tcustom2\n+\tcustom3\n \"\n \n for p in $diffpatterns\n@@ -79,30 +89,12 @@ test_expect_success 'set up .gitattributes declaring drivers to test' '\n \tEOF\n '\n \n-test_expect_success 'custom pattern' '\n-\ttest_config diff.java.funcname \"!static\n-!String\n-[^ \t].*s.*\" &&\n-\ttest_expect_funcname \"int special;\\$\"\n-'\n-\n test_expect_success 'last regexp must not be negated' '\n \ttest_config diff.java.funcname \"!static\" &&\n \ttest_expect_code 128 git diff --no-index Beer.java Beer-correct.java 2>msg &&\n \tgrep \": Last expression must not be negated:\" msg\n '\n \n-test_expect_success 'pattern which matches to end of line' '\n-\ttest_config diff.java.funcname \"Beer\\$\" &&\n-\ttest_expect_funcname \"Beer\\$\"\n-'\n-\n-test_expect_success 'alternation in pattern' '\n-\ttest_config diff.java.funcname \"Beer$\" &&\n-\ttest_config diff.java.xfuncname \"^[ \t]*((public|static).*)$\" &&\n-\ttest_expect_funcname \"public static void main(\"\n-'\n-\n test_expect_success 'setup hunk header tests' '\n \tfor i in $diffpatterns\n \tdo\ndiff --git a/t/t4018/custom1-pattern b/t/t4018/custom1-pattern\nnew file mode 100644\nindex 0000000..e8fd59f\n--- /dev/null\n+++ b/t/t4018/custom1-pattern\n@@ -0,0 +1,17 @@\n+public class Beer\n+{\n+\tint special, RIGHT;\n+\tpublic static void main(String args[])\n+\t{\n+\t\tString s=\" \";\n+\t\tfor(int x = 99; x > 0; x--)\n+\t\t{\n+\t\t\tSystem.out.print(x + \" bottles of beer on the wall \"\n+\t\t\t\t+ x + \" bottles of beer\\n\" // ChangeMe\n+\t\t\t\t+ \"Take one down, pass it around, \" + (x - 1)\n+\t\t\t\t+ \" bottles of beer on the wall.\\n\");\n+\t\t}\n+\t\tSystem.out.print(\"Go to the store, buy some more,\\n\"\n+\t\t\t+ \"99 bottles of beer on the wall.\\n\");\n+\t}\n+}\ndiff --git a/t/t4018/custom2-match-to-end-of-line b/t/t4018/custom2-match-to-end-of-line\nnew file mode 100644\nindex 0000000..f88ac31\n--- /dev/null\n+++ b/t/t4018/custom2-match-to-end-of-line\n@@ -0,0 +1,8 @@\n+public class RIGHT_Beer\n+{\n+\tint special;\n+\tpublic static void main(String args[])\n+\t{\n+\t\tSystem.out.print(\"ChangeMe\");\n+\t}\n+}\ndiff --git a/t/t4018/custom3-alternation-in-pattern b/t/t4018/custom3-alternation-in-pattern\nnew file mode 100644\nindex 0000000..5f3769c\n--- /dev/null\n+++ b/t/t4018/custom3-alternation-in-pattern\n@@ -0,0 +1,17 @@\n+public class Beer\n+{\n+\tint special;\n+\tpublic static void main(String RIGHT[])\n+\t{\n+\t\tString s=\" \";\n+\t\tfor(int x = 99; x > 0; x--)\n+\t\t{\n+\t\t\tSystem.out.print(x + \" bottles of beer on the wall \"\n+\t\t\t\t+ x + \" bottles of beer\\n\" // ChangeMe\n+\t\t\t\t+ \"Take one down, pass it around, \" + (x - 1)\n+\t\t\t\t+ \" bottles of beer on the wall.\\n\");\n+\t\t}\n+\t\tSystem.out.print(\"Go to the store, buy some more,\\n\"\n+\t\t\t+ \"99 bottles of beer on the wall.\\n\");\n+\t}\n+}\n-- \n1.8.5.2.244.g9fb3fb1\n"},{"id":"237367","messageId":"92c48a41f3aee778d1a1d21ef9f9abdd54bfdaab.1395433874.git.j6t@kdbg.org","threadId":"36060","inReplyTo":"cover.1395433874.git.j6t@kdbg.org","subject":"[PATCH 07/10] t4018: reduce test files for pattern compilation tests","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2014-03-21T21:07:19Z","receivedAt":"2014-03-21T21:07:19Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"All test cases that need a file with specific text patterns have been\nconverted to utilize texts in the t4018/ directory. The remaining tests\nin the test script deal only with the validity of the regular\nexpressions. These tests do not depend on the contents of files that\n'git diff' is invoked on. Remove the largish here-document and use only\ntiny files.\n\nWhile we are touching these tests, convert grep to test_i18ngrep as the\ntexts checked for may undergo translation in the future.\n\nSigned-off-by: Johannes Sixt \n---\n t/t4018-diff-funcname.sh | 52 +++++++++++++-----------------------------------\n 1 file changed, 14 insertions(+), 38 deletions(-)\n\ndiff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh\nindex 5ac744f..34591c2 100755\n--- a/t/t4018-diff-funcname.sh\n+++ b/t/t4018-diff-funcname.sh\n@@ -7,29 +7,6 @@ test_description='Test custom diff function name patterns'\n \n . ./test-lib.sh\n \n-LF='\n-'\n-cat >Beer.java <<\\EOF\n-public class Beer\n-{\n-\tint special;\n-\tpublic static void main(String args[])\n-\t{\n-\t\tString s=\" \";\n-\t\tfor(int x = 99; x > 0; x--)\n-\t\t{\n-\t\t\tSystem.out.print(x + \" bottles of beer on the wall \"\n-\t\t\t\t+ x + \" bottles of beer\\n\"\n-\t\t\t\t+ \"Take one down, pass it around, \" + (x - 1)\n-\t\t\t\t+ \" bottles of beer on the wall.\\n\");\n-\t\t}\n-\t\tSystem.out.print(\"Go to the store, buy some more,\\n\"\n-\t\t\t+ \"99 bottles of beer on the wall.\\n\");\n-\t}\n-}\n-EOF\n-sed 's/beer\\\\/beer,\\\\/' Beer-correct.java\n-\n test_expect_success 'setup' '\n \t# a non-trivial custom pattern\n \tgit config diff.custom1.funcname \"!static\n@@ -41,7 +18,11 @@ test_expect_success 'setup' '\n \n \t# alternation in pattern\n \tgit config diff.custom3.funcname \"Beer$\" &&\n-\tgit config diff.custom3.xfuncname \"^[ \t]*((public|static).*)$\"\n+\tgit config diff.custom3.xfuncname \"^[ \t]*((public|static).*)$\" &&\n+\n+\t# for regexp compilation tests\n+\techo A >A.java &&\n+\techo B >B.java\n '\n \n diffpatterns=\"\n@@ -70,29 +51,24 @@ do\n \ttest_expect_success \"builtin $p pattern compiles\" '\n \t\techo \"*.java diff=$p\" >.gitattributes &&\n \t\ttest_expect_code 1 git diff --no-index \\\n-\t\t\tBeer.java Beer-correct.java 2>msg &&\n-\t\t! grep fatal msg &&\n-\t\t! grep error msg\n+\t\t\tA.java B.java 2>msg &&\n+\t\t! test_i18ngrep fatal msg &&\n+\t\t! test_i18ngrep error msg\n \t'\n \ttest_expect_success \"builtin $p wordRegex pattern compiles\" '\n \t\techo \"*.java diff=$p\" >.gitattributes &&\n \t\ttest_expect_code 1 git diff --no-index --word-diff \\\n-\t\t\tBeer.java Beer-correct.java 2>msg &&\n-\t\t! grep fatal msg &&\n-\t\t! grep error msg\n+\t\t\tA.java B.java 2>msg &&\n+\t\t! test_i18ngrep fatal msg &&\n+\t\t! test_i18ngrep error msg\n \t'\n done\n \n-test_expect_success 'set up .gitattributes declaring drivers to test' '\n-\tcat >.gitattributes <<-\\EOF\n-\t*.java diff=java\n-\tEOF\n-'\n-\n test_expect_success 'last regexp must not be negated' '\n+\techo \"*.java diff=java\" >.gitattributes &&\n \ttest_config diff.java.funcname \"!static\" &&\n-\ttest_expect_code 128 git diff --no-index Beer.java Beer-correct.java 2>msg &&\n-\tgrep \": Last expression must not be negated:\" msg\n+\ttest_expect_code 128 git diff --no-index A.java B.java 2>msg &&\n+\ttest_i18ngrep \": Last expression must not be negated:\" msg\n '\n \n test_expect_success 'setup hunk header tests' '\n-- \n1.8.5.2.244.g9fb3fb1\n"},{"id":"237368","messageId":"83db40e0bf4cfd107841a5f87275eca53419874c.1395433874.git.j6t@kdbg.org","threadId":"36060","inReplyTo":"cover.1395433874.git.j6t@kdbg.org","subject":"[PATCH 08/10] t4018: test cases for the built-in cpp pattern","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2014-03-21T21:07:20Z","receivedAt":"2014-03-21T21:07:20Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"A later patch changes the built-in cpp pattern. These test cases\ndemonstrate aspects of the pattern that we do not want to change.\n\nSigned-off-by: Johannes Sixt \n---\n t/t4018/cpp-c++-function | 4 ++++\n t/t4018/cpp-class-definition | 4 ++++\n t/t4018/cpp-class-definition-derived | 5 +++++\n t/t4018/cpp-function-returning-pointer | 4 ++++\n t/t4018/cpp-skip-access-specifiers | 8 ++++++++\n t/t4018/cpp-skip-comment-block | 9 +++++++++\n t/t4018/cpp-skip-labels | 8 ++++++++\n t/t4018/cpp-struct-definition | 9 +++++++++\n t/t4018/cpp-void-c-function | 4 ++++\n 9 files changed, 55 insertions(+)\n create mode 100644 t/t4018/cpp-c++-function\n create mode 100644 t/t4018/cpp-class-definition\n create mode 100644 t/t4018/cpp-class-definition-derived\n create mode 100644 t/t4018/cpp-function-returning-pointer\n create mode 100644 t/t4018/cpp-skip-access-specifiers\n create mode 100644 t/t4018/cpp-skip-comment-block\n create mode 100644 t/t4018/cpp-skip-labels\n create mode 100644 t/t4018/cpp-struct-definition\n create mode 100644 t/t4018/cpp-void-c-function\n\ndiff --git a/t/t4018/cpp-c++-function b/t/t4018/cpp-c++-function\nnew file mode 100644\nindex 0000000..9ee6bbe\n--- /dev/null\n+++ b/t/t4018/cpp-c++-function\n@@ -0,0 +1,4 @@\n+Item RIGHT::DoSomething( Args with_spaces )\n+{\n+\tChangeMe;\n+}\ndiff --git a/t/t4018/cpp-class-definition b/t/t4018/cpp-class-definition\nnew file mode 100644\nindex 0000000..11b61da\n--- /dev/null\n+++ b/t/t4018/cpp-class-definition\n@@ -0,0 +1,4 @@\n+class RIGHT\n+{\n+\tint ChangeMe;\n+};\ndiff --git a/t/t4018/cpp-class-definition-derived b/t/t4018/cpp-class-definition-derived\nnew file mode 100644\nindex 0000000..3b98cd0\n--- /dev/null\n+++ b/t/t4018/cpp-class-definition-derived\n@@ -0,0 +1,5 @@\n+class RIGHT :\n+\tpublic Baseclass\n+{\n+\tint ChangeMe;\n+};\ndiff --git a/t/t4018/cpp-function-returning-pointer b/t/t4018/cpp-function-returning-pointer\nnew file mode 100644\nindex 0000000..ef15657\n--- /dev/null\n+++ b/t/t4018/cpp-function-returning-pointer\n@@ -0,0 +1,4 @@\n+const char *get_it_RIGHT(char *ptr)\n+{\n+\tChangeMe;\n+}\ndiff --git a/t/t4018/cpp-skip-access-specifiers b/t/t4018/cpp-skip-access-specifiers\nnew file mode 100644\nindex 0000000..4d4a9db\n--- /dev/null\n+++ b/t/t4018/cpp-skip-access-specifiers\n@@ -0,0 +1,8 @@\n+class RIGHT : public Baseclass\n+{\n+public:\n+protected:\n+private:\n+\tvoid DoSomething();\n+\tint ChangeMe;\n+};\ndiff --git a/t/t4018/cpp-skip-comment-block b/t/t4018/cpp-skip-comment-block\nnew file mode 100644\nindex 0000000..3800b99\n--- /dev/null\n+++ b/t/t4018/cpp-skip-comment-block\n@@ -0,0 +1,9 @@\n+struct item RIGHT(int i)\n+// Do not\n+// pick up\n+/* these\n+** comments.\n+*/\n+{\n+\tChangeMe;\n+}\ndiff --git a/t/t4018/cpp-skip-labels b/t/t4018/cpp-skip-labels\nnew file mode 100644\nindex 0000000..b9c10ab\n--- /dev/null\n+++ b/t/t4018/cpp-skip-labels\n@@ -0,0 +1,8 @@\n+void RIGHT (void)\n+{\n+repeat:\t\t// C++ comment\n+next:\t\t/* C comment */\n+\tdo_something();\n+\n+\tChangeMe;\n+}\ndiff --git a/t/t4018/cpp-struct-definition b/t/t4018/cpp-struct-definition\nnew file mode 100644\nindex 0000000..521c59f\n--- /dev/null\n+++ b/t/t4018/cpp-struct-definition\n@@ -0,0 +1,9 @@\n+struct RIGHT {\n+\tunsigned\n+\t/* this bit field looks like a label and should not be picked up */\n+\t\tdecoy_bitfield: 2,\n+\t\tmore : 1;\n+\tint filler;\n+\n+\tint ChangeMe;\n+};\ndiff --git a/t/t4018/cpp-void-c-function b/t/t4018/cpp-void-c-function\nnew file mode 100644\nindex 0000000..153081e\n--- /dev/null\n+++ b/t/t4018/cpp-void-c-function\n@@ -0,0 +1,4 @@\n+void RIGHT (void)\n+{\n+\tChangeMe;\n+}\n-- \n1.8.5.2.244.g9fb3fb1\n"},{"id":"237370","messageId":"4f5a35463f5edc7274d4dd124aa03f1262887aad.1395433874.git.j6t@kdbg.org","threadId":"36060","inReplyTo":"cover.1395433874.git.j6t@kdbg.org","subject":"[PATCH 09/10] t4018: test cases showing that the cpp pattern misses many anchor points","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2014-03-21T21:07:21Z","receivedAt":"2014-03-21T21:07:21Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Most of the tests show C++ code, but there is also a union definition and\na GNU style function definition that are not recognized.\n\nSigned-off-by: Johannes Sixt \n---\n t/t4018/cpp-class-constructor | 5 +++++\n t/t4018/cpp-class-constructor-mem-init | 6 ++++++\n t/t4018/cpp-class-destructor | 5 +++++\n t/t4018/cpp-function-returning-global-type | 5 +++++\n t/t4018/cpp-function-returning-nested | 6 ++++++\n t/t4018/cpp-function-returning-reference | 5 +++++\n t/t4018/cpp-gnu-style-function | 6 ++++++\n t/t4018/cpp-namespace-definition | 5 +++++\n t/t4018/cpp-operator-definition | 5 +++++\n t/t4018/cpp-struct-single-line | 8 ++++++++\n t/t4018/cpp-template-function-definition | 5 +++++\n t/t4018/cpp-union-definition | 5 +++++\n 12 files changed, 66 insertions(+)\n create mode 100644 t/t4018/cpp-class-constructor\n create mode 100644 t/t4018/cpp-class-constructor-mem-init\n create mode 100644 t/t4018/cpp-class-destructor\n create mode 100644 t/t4018/cpp-function-returning-global-type\n create mode 100644 t/t4018/cpp-function-returning-nested\n create mode 100644 t/t4018/cpp-function-returning-reference\n create mode 100644 t/t4018/cpp-gnu-style-function\n create mode 100644 t/t4018/cpp-namespace-definition\n create mode 100644 t/t4018/cpp-operator-definition\n create mode 100644 t/t4018/cpp-struct-single-line\n create mode 100644 t/t4018/cpp-template-function-definition\n create mode 100644 t/t4018/cpp-union-definition\n\ndiff --git a/t/t4018/cpp-class-constructor b/t/t4018/cpp-class-constructor\nnew file mode 100644\nindex 0000000..4c4925c\n--- /dev/null\n+++ b/t/t4018/cpp-class-constructor\n@@ -0,0 +1,5 @@\n+Item::Item(int RIGHT)\n+{\n+\tChangeMe;\n+\tbroken;\n+}\ndiff --git a/t/t4018/cpp-class-constructor-mem-init b/t/t4018/cpp-class-constructor-mem-init\nnew file mode 100644\nindex 0000000..eec1d7c\n--- /dev/null\n+++ b/t/t4018/cpp-class-constructor-mem-init\n@@ -0,0 +1,6 @@\n+Item::Item(int RIGHT) :\n+\tmember(0)\n+{\n+\tChangeMe;\n+\tbroken;\n+}\ndiff --git a/t/t4018/cpp-class-destructor b/t/t4018/cpp-class-destructor\nnew file mode 100644\nindex 0000000..03aa51c\n--- /dev/null\n+++ b/t/t4018/cpp-class-destructor\n@@ -0,0 +1,5 @@\n+RIGHT::~RIGHT()\n+{\n+\tChangeMe;\n+\tbroken;\n+}\ndiff --git a/t/t4018/cpp-function-returning-global-type b/t/t4018/cpp-function-returning-global-type\nnew file mode 100644\nindex 0000000..bff3e5f\n--- /dev/null\n+++ b/t/t4018/cpp-function-returning-global-type\n@@ -0,0 +1,5 @@\n+::Item get::it::RIGHT()\n+{\n+\tChangeMe;\n+\tbroken;\n+}\ndiff --git a/t/t4018/cpp-function-returning-nested b/t/t4018/cpp-function-returning-nested\nnew file mode 100644\nindex 0000000..41700f2\n--- /dev/null\n+++ b/t/t4018/cpp-function-returning-nested\n@@ -0,0 +1,6 @@\n+get::Item get::it::RIGHT()\n+{\n+\tChangeMe;\n+\tbroken;\n+}\n+\ndiff --git a/t/t4018/cpp-function-returning-reference b/t/t4018/cpp-function-returning-reference\nnew file mode 100644\nindex 0000000..29e2bd4\n--- /dev/null\n+++ b/t/t4018/cpp-function-returning-reference\n@@ -0,0 +1,5 @@\n+string& get::it::RIGHT(char *ptr)\n+{\n+\tChangeMe;\n+\tbroken;\n+}\ndiff --git a/t/t4018/cpp-gnu-style-function b/t/t4018/cpp-gnu-style-function\nnew file mode 100644\nindex 0000000..d65fc74\n--- /dev/null\n+++ b/t/t4018/cpp-gnu-style-function\n@@ -0,0 +1,6 @@\n+const char *\n+RIGHT(int arg)\n+{\n+\tChangeMe;\n+\tbroken;\n+}\ndiff --git a/t/t4018/cpp-namespace-definition b/t/t4018/cpp-namespace-definition\nnew file mode 100644\nindex 0000000..6b88dd9\n--- /dev/null\n+++ b/t/t4018/cpp-namespace-definition\n@@ -0,0 +1,5 @@\n+namespace RIGHT\n+{\n+\tChangeMe;\n+\tbroken;\n+}\ndiff --git a/t/t4018/cpp-operator-definition b/t/t4018/cpp-operator-definition\nnew file mode 100644\nindex 0000000..f2bd167\n--- /dev/null\n+++ b/t/t4018/cpp-operator-definition\n@@ -0,0 +1,5 @@\n+Value operator+(Value LEFT, Value RIGHT)\n+{\n+\tChangeMe;\n+\tbroken;\n+}\ndiff --git a/t/t4018/cpp-struct-single-line b/t/t4018/cpp-struct-single-line\nnew file mode 100644\nindex 0000000..ad6fa8b\n--- /dev/null\n+++ b/t/t4018/cpp-struct-single-line\n@@ -0,0 +1,8 @@\n+void wrong()\n+{\n+}\n+\n+struct RIGHT_iterator_tag {};\n+\n+int ChangeMe;\n+// broken\ndiff --git a/t/t4018/cpp-template-function-definition b/t/t4018/cpp-template-function-definition\nnew file mode 100644\nindex 0000000..a410298\n--- /dev/null\n+++ b/t/t4018/cpp-template-function-definition\n@@ -0,0 +1,5 @@\n+template int RIGHT(T arg)\n+{\n+\tChangeMe;\n+\tbroken;\n+}\ndiff --git a/t/t4018/cpp-union-definition b/t/t4018/cpp-union-definition\nnew file mode 100644\nindex 0000000..133b662\n--- /dev/null\n+++ b/t/t4018/cpp-union-definition\n@@ -0,0 +1,5 @@\n+union RIGHT {\n+\tdouble v;\n+\tint ChangeMe;\n+\tbroken;\n+};\n-- \n1.8.5.2.244.g9fb3fb1\n"},{"id":"237371","messageId":"49db8b0f66c725ef6ff0b8a4d536c43e6397879a.1395433874.git.j6t@kdbg.org","threadId":"36060","inReplyTo":"cover.1395433874.git.j6t@kdbg.org","subject":"[PATCH 10/10] userdiff: have 'cpp' hunk header pattern catch more C++ anchor points","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2014-03-21T21:07:22Z","receivedAt":"2014-03-21T21:07:22Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"The hunk header pattern 'cpp' is intended for C and C++ source code, but\nit is actually not particularly useful for the latter, and even misses\nsome use-cases for the former.\n\nThe parts of the pattern have the following flaws:\n\n- The first part matches an identifier followed immediately by a colon\n and arbitrary text and is intended to reject goto labels and C++\n access specifiers (public, private, protected). But this pattern also\n rejects C++ constructs, which look like this:\n\n MyClass::MyClass()\n MyClass::~MyClass()\n MyClass::Item MyClass::Find(...\n\n- The second part matches an identifier followed by a list of qualified\n names (i.e. identifiers separated by the C++ scope operator '::')\n separated by space or '*' followed by an opening parenthesis (with\n space between the tokens). It matches function declarations like\n\n struct item* get_head(...\n int Outer::Inner::Func(...\n\n Since the pattern requires at least two identifiers, GNU-style\n function definitions are ignored:\n\n void\n func(...\n\n Moreover, since the pattern does not allow punctuation other than '*',\n the following C++ constructs are not recognized:\n\n . template definitions:\n template int func(T arg)\n\n . functions returning references:\n const string& get_message()\n\n . functions returning templated types:\n vector foo()\n\n . operator definitions:\n Value operator+(Value l, Value r)\n\n- The third part of the pattern finally matches compound definitions.\n But it forgets about unions and namespaces, and also skips single-line\n definitions\n\n struct random_iterator_tag {};\n\n because no semicolon can occur on the line.\n\nChange the first pattern to require a colon at the end of the line\n(except for trailing space and comments), so that it does not reject\nconstructor or destructor definitions.\n\nNotice that all interesting anchor points begin with an identifier or\nkeyword. But since there is a large variety of syntactical constructs\nafter the first \"word\", the simplest is to require only this word and\naccept everything else. Therefore, this boils down to a line that begins\nwith a letter or underscore (optionally preceded by the C++ scope\noperator '::' to accept functions returning a type anchored at the\nglobal namespace). Replace the second and third part by a single pattern\nthat picks such a line.\n\nThis has the following desirable consequence:\n\n- All constructs mentioned above are recognized.\n\nand the following likely desirable consequences:\n\n- Definitions of global variables and typedefs are recognized:\n\n int num_entries = 0;\n extern const char* help_text;\n typedef basic_string wstring;\n\n- Commonly used marco-ized boilerplate code is recognized:\n\n BEGIN_MESSAGE_MAP(CCanvas,CWnd)\n Q_DECLARE_METATYPE(MyStruct)\n PATTERNS(\"tex\",...)\n\n (The last one is from this very patch.)\n\nbut also the following possibly undesirable consequence:\n\n- When a label is not on a line by itself (except for a comment) it is\n no longer rejected, but can appear as a hunk header if it occurs at\n the beginning of a line:\n\n next:;\n\nIMO, the benefits of the change outweigh the (possible) regressions by a\nlarge margin.\n\nSigned-off-by: Johannes Sixt \n---\n t/t4018/cpp-class-constructor | 1 -\n t/t4018/cpp-class-constructor-mem-init | 1 -\n t/t4018/cpp-class-destructor | 1 -\n t/t4018/cpp-function-returning-global-type | 1 -\n t/t4018/cpp-function-returning-nested | 1 -\n t/t4018/cpp-function-returning-reference | 1 -\n t/t4018/cpp-gnu-style-function | 1 -\n t/t4018/cpp-namespace-definition | 1 -\n t/t4018/cpp-operator-definition | 1 -\n t/t4018/cpp-struct-single-line | 1 -\n t/t4018/cpp-template-function-definition | 1 -\n t/t4018/cpp-union-definition | 1 -\n userdiff.c | 8 +++-----\n 13 files changed, 3 insertions(+), 17 deletions(-)\n\ndiff --git a/t/t4018/cpp-class-constructor b/t/t4018/cpp-class-constructor\nindex 4c4925c..ec4f115 100644\n--- a/t/t4018/cpp-class-constructor\n+++ b/t/t4018/cpp-class-constructor\n@@ -1,5 +1,4 @@\n Item::Item(int RIGHT)\n {\n \tChangeMe;\n-\tbroken;\n }\ndiff --git a/t/t4018/cpp-class-constructor-mem-init b/t/t4018/cpp-class-constructor-mem-init\nindex eec1d7c..49a69f3 100644\n--- a/t/t4018/cpp-class-constructor-mem-init\n+++ b/t/t4018/cpp-class-constructor-mem-init\n@@ -2,5 +2,4 @@ Item::Item(int RIGHT) :\n \tmember(0)\n {\n \tChangeMe;\n-\tbroken;\n }\ndiff --git a/t/t4018/cpp-class-destructor b/t/t4018/cpp-class-destructor\nindex 03aa51c..5487665 100644\n--- a/t/t4018/cpp-class-destructor\n+++ b/t/t4018/cpp-class-destructor\n@@ -1,5 +1,4 @@\n RIGHT::~RIGHT()\n {\n \tChangeMe;\n-\tbroken;\n }\ndiff --git a/t/t4018/cpp-function-returning-global-type b/t/t4018/cpp-function-returning-global-type\nindex bff3e5f..1084d59 100644\n--- a/t/t4018/cpp-function-returning-global-type\n+++ b/t/t4018/cpp-function-returning-global-type\n@@ -1,5 +1,4 @@\n ::Item get::it::RIGHT()\n {\n \tChangeMe;\n-\tbroken;\n }\ndiff --git a/t/t4018/cpp-function-returning-nested b/t/t4018/cpp-function-returning-nested\nindex 41700f2..d9750aa 100644\n--- a/t/t4018/cpp-function-returning-nested\n+++ b/t/t4018/cpp-function-returning-nested\n@@ -1,6 +1,5 @@\n get::Item get::it::RIGHT()\n {\n \tChangeMe;\n-\tbroken;\n }\n \ndiff --git a/t/t4018/cpp-function-returning-reference b/t/t4018/cpp-function-returning-reference\nindex 29e2bd4..01b051d 100644\n--- a/t/t4018/cpp-function-returning-reference\n+++ b/t/t4018/cpp-function-returning-reference\n@@ -1,5 +1,4 @@\n string& get::it::RIGHT(char *ptr)\n {\n \tChangeMe;\n-\tbroken;\n }\ndiff --git a/t/t4018/cpp-gnu-style-function b/t/t4018/cpp-gnu-style-function\nindex d65fc74..08c7c75 100644\n--- a/t/t4018/cpp-gnu-style-function\n+++ b/t/t4018/cpp-gnu-style-function\n@@ -2,5 +2,4 @@ const char *\n RIGHT(int arg)\n {\n \tChangeMe;\n-\tbroken;\n }\ndiff --git a/t/t4018/cpp-namespace-definition b/t/t4018/cpp-namespace-definition\nindex 6b88dd9..6749980 100644\n--- a/t/t4018/cpp-namespace-definition\n+++ b/t/t4018/cpp-namespace-definition\n@@ -1,5 +1,4 @@\n namespace RIGHT\n {\n \tChangeMe;\n-\tbroken;\n }\ndiff --git a/t/t4018/cpp-operator-definition b/t/t4018/cpp-operator-definition\nindex f2bd167..1acd827 100644\n--- a/t/t4018/cpp-operator-definition\n+++ b/t/t4018/cpp-operator-definition\n@@ -1,5 +1,4 @@\n Value operator+(Value LEFT, Value RIGHT)\n {\n \tChangeMe;\n-\tbroken;\n }\ndiff --git a/t/t4018/cpp-struct-single-line b/t/t4018/cpp-struct-single-line\nindex ad6fa8b..a0de5fb 100644\n--- a/t/t4018/cpp-struct-single-line\n+++ b/t/t4018/cpp-struct-single-line\n@@ -5,4 +5,3 @@ void wrong()\n struct RIGHT_iterator_tag {};\n \n int ChangeMe;\n-// broken\ndiff --git a/t/t4018/cpp-template-function-definition b/t/t4018/cpp-template-function-definition\nindex a410298..0cdf5ba 100644\n--- a/t/t4018/cpp-template-function-definition\n+++ b/t/t4018/cpp-template-function-definition\n@@ -1,5 +1,4 @@\n template int RIGHT(T arg)\n {\n \tChangeMe;\n-\tbroken;\n }\ndiff --git a/t/t4018/cpp-union-definition b/t/t4018/cpp-union-definition\nindex 133b662..7ec94df 100644\n--- a/t/t4018/cpp-union-definition\n+++ b/t/t4018/cpp-union-definition\n@@ -1,5 +1,4 @@\n union RIGHT {\n \tdouble v;\n \tint ChangeMe;\n-\tbroken;\n };\ndiff --git a/userdiff.c b/userdiff.c\nindex 8830417..fad52d6 100644\n--- a/userdiff.c\n+++ b/userdiff.c\n@@ -125,11 +125,9 @@\n \t \"\\\\\\\\[a-zA-Z@]+|\\\\\\\\.|[a-zA-Z0-9\\x80-\\xff]+\"),\n PATTERNS(\"cpp\",\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 /* compound type at top level */\n-\t \"^((struct|class|enum)[^;]*)$\",\n+\t \"!^[ \\t]*[A-Za-z_][A-Za-z_0-9]*:[[:space:]]*($|/[/*])\\n\"\n+\t /* functions/methods, variables, and compounds at top level */\n+\t \"^((::[[:space:]]*)?[A-Za-z_].*)$\",\n \t /* -- */\n \t \"[a-zA-Z_][a-zA-Z0-9_]*\"\n \t \"|[-+0-9.e]+[fFlL]?|0[xXbB]?[0-9a-fA-F]+[lLuU]*\"\n-- \n1.8.5.2.244.g9fb3fb1\n"},{"id":"237380","messageId":"xmqqk3bnrz3m.fsf@gitster.dls.corp.google.com","threadId":"36060","inReplyTo":"52505977c20a480941ae1f85f50ffb7a0bbffedb.1395433874.git.j6t@kdbg.org","subject":"Re: [PATCH 03/10] t4018: an infrastructure to test hunk headers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-21T22:00:45Z","receivedAt":"2014-03-21T22:00:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Add an infrastructure that simplifies adding new tests of the hunk\n> header regular expressions.\n>\n> To add new tests, a file with the syntax to test can be dropped in the\n> directory t4018. The README file explains how a test file must contain;\n\ns/how/what/, or \"how a test file must be written\" you mean?\n\n> the README itself tests the default behavior.\n\nThanks. Looks like a reasonable way to mark what must be found.\n\n> Signed-off-by: Johannes Sixt \n> ---\n> t/t4018-diff-funcname.sh | 60 +++++++++++++++++++++++++++++++++++++++++++-----\n> t/t4018/README | 18 +++++++++++++++\n> 2 files changed, 72 insertions(+), 6 deletions(-)\n> create mode 100644 t/t4018/README\n>\n> diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh\n> index 38a092a..b467d9e 100755\n> --- a/t/t4018-diff-funcname.sh\n> +++ b/t/t4018-diff-funcname.sh\n> @@ -100,7 +100,25 @@ test_expect_funcname () {\n> \tgrep \"^@@.*@@ $1\" diff\n> }\n> \n> -for p in ada bibtex cpp csharp fortran html java matlab objc pascal perl php python ruby tex\n> +diffpatterns=\"\n> +\tada\n> +\tbibtex\n> +\tcpp\n> +\tcsharp\n> +\tfortran\n> +\thtml\n> +\tjava\n> +\tmatlab\n> +\tobjc\n> +\tpascal\n> +\tperl\n> +\tphp\n> +\tpython\n> +\truby\n> +\ttex\n> +\"\n> +\n> +for p in $diffpatterns\n> do\n> \ttest_expect_success \"builtin $p pattern compiles\" '\n> \t\techo \"*.java diff=$p\" >.gitattributes &&\n\nI always found this \"Let's apply rules for language $p to these\n*.java files\" strange. I have wonder if it makes sense to further\nchange the framework to read the name of the rule to be applied from\nthe file in t/t4018/ directory, instead of using filename that is\nthe same as the name of the rule? That way, you can list the files\nin t/t4018/ directory to come up with the above list, without having\nto maintain the list of rules separately like the above.\n\n> @@ -118,11 +136,6 @@ do\n> \t'\n> done\n> \n> -test_expect_success 'default behaviour' '\n> -\trm -f .gitattributes &&\n> -\ttest_expect_funcname \"public class Beer\\$\"\n> -'\n> -\n> test_expect_success 'set up .gitattributes declaring drivers to test' '\n> \tcat >.gitattributes <<-\\EOF\n> \t*.java diff=java\n> @@ -182,4 +195,39 @@ test_expect_success 'alternation in pattern' '\n> \ttest_expect_funcname \"public static void main(\"\n> '\n> \n> +test_expect_success 'setup hunk header tests' '\n> +\tfor i in $diffpatterns\n> +\tdo\n> +\t\techo \"$i-* diff=$i\"\n> +\tdone > .gitattributes &&\n\nI like that you can have more than one test for each language/rule\nthis way, allowing you to test one kind of breakage without getting\naffected by lines prepared for other tests in the same file.\n\n> +\t# add all test files to the index\n> +\t(\n> +\t\tcd \"$TEST_DIRECTORY\"/t4018 &&\n> +\t\tgit --git-dir=\"$TRASH_DIRECTORY/.git\" add .\n> +\t) &&\n> +\n> +\t# place modified files in the worktree\n> +\tfor i in $(git ls-files)\n> +\tdo\n> +\t\tsed -e \"s/ChangeMe/IWasChanged/\" <\"$TEST_DIRECTORY/t4018/$i\" >\"$i\" || return 1\n> +\tdone\n> +'\n> +\n> +# check each individual file\n> +for i in $(git ls-files)\n> +do\n> +\tif grep broken \"$i\" >/dev/null 2>&1\n> +\tthen\n> +\t\tresult=failure\n> +\telse\n> +\t\tresult=success\n> +\tfi\n> +\ttest_expect_$result \"hunk header: $i\" \"\n> +\t\ttest_when_finished 'cat actual' &&\t# for debugging only\n> +\t\tgit diff -U1 $i >actual &&\n> +\t\tgrep '@@ .* @@.*RIGHT' actual\n> +\t\"\n> +done\n> +\n> test_done\n> diff --git a/t/t4018/README b/t/t4018/README\n> new file mode 100644\n> index 0000000..283e01cc\n> --- /dev/null\n> +++ b/t/t4018/README\n> @@ -0,0 +1,18 @@\n> +How to write RIGHT test cases\n> +=============================\n> +\n> +Insert the word \"ChangeMe\" (exactly this form) at a distance of\n> +at least two lines from the line that must appear in the hunk header.\n> +\n> +The text that must appear in the hunk header must contain the word\n> +\"right\", but in all upper-case, like in the title above.\n> +\n> +To mark a test case that highlights a malfunction, insert the word\n> +BROKEN in all lower-case somewhere in the file.\n> +\n> +This text is a bit twisted and out of order, but it is itself a\n> +test case for the default hunk header pattern. Know what you are doing\n> +if you change it.\n> +\n> +BTW, this tests that the head line goes to the hunk header, not the line\n> +of equal signs.\n"},{"id":"237381","messageId":"xmqqbnwzrxye.fsf@gitster.dls.corp.google.com","threadId":"36060","inReplyTo":"cover.1395433874.git.j6t@kdbg.org","subject":"Re: [PATCH 00/10] userdiff: cpp pattern simplification and test framework","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-21T22:25:29Z","receivedAt":"2014-03-21T22:25:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks; will replace jk/diff-funcname-cpp-regex with this series.\n"},{"id":"237385","messageId":"532D3411.2000109@kdbg.org","threadId":"36060","inReplyTo":"xmqqk3bnrz3m.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 03/10] t4018: an infrastructure to test hunk headers","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2014-03-22T06:56:17Z","receivedAt":"2014-03-22T06:56:17Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 21.03.2014 23:00, schrieb Junio C Hamano:\n> Johannes Sixt writes:\n> \n>> Add an infrastructure that simplifies adding new tests of the hunk\n>> header regular expressions.\n>>\n>> To add new tests, a file with the syntax to test can be dropped in the\n>> directory t4018. The README file explains how a test file must contain;\n> \n> s/how/what/, or \"how a test file must be written\" you mean?\n\nI do mean one of those, preferably the latter ;)\n\n>> -for p in ada bibtex cpp csharp fortran html java matlab objc pascal perl php python ruby tex\n>> +diffpatterns=\"\n>> +\tada\n>> +\tbibtex\n>> +\tcpp\n>> +\tcsharp\n>> +\tfortran\n>> +\thtml\n>> +\tjava\n>> +\tmatlab\n>> +\tobjc\n>> +\tpascal\n>> +\tperl\n>> +\tphp\n>> +\tpython\n>> +\truby\n>> +\ttex\n>> +\"\n>> +\n>> +for p in $diffpatterns\n>> do\n>> \ttest_expect_success \"builtin $p pattern compiles\" '\n>> \t\techo \"*.java diff=$p\" >.gitattributes &&\n> \n> I always found this \"Let's apply rules for language $p to these\n> *.java files\" strange. I have wonder if it makes sense to further\n> change the framework to read the name of the rule to be applied from\n> the file in t/t4018/ directory, instead of using filename that is\n> the same as the name of the rule? That way, you can list the files\n> in t/t4018/ directory to come up with the above list, without having\n> to maintain the list of rules separately like the above.\n\nI see what you mean. Notice, however, that we also test whether the\nregular expressions can be compiled successfully, and for this it is\ndesirable to have the complete list of userdiff drivers. Until we have\nat least one test-case for each driver, we wouldn't get the complete\nlist. In summary, I do agree that a bit more automation is also\ndesirable, but we are not there, yet.\n\n-- Hannes\n"},{"id":"237424","messageId":"xmqq38i8so4y.fsf@gitster.dls.corp.google.com","threadId":"36060","inReplyTo":"532D3411.2000109@kdbg.org","subject":"Re: [PATCH 03/10] t4018: an infrastructure to test hunk headers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-23T19:36:45Z","receivedAt":"2014-03-23T19:36:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> I see what you mean. Notice, however, that we also test whether\n> the regular expressions can be compiled successfully, and for this\n> it is desirable to have the complete list of userdiff drivers.\n> Until we have at least one test-case for each driver, we wouldn't\n> get the complete list.\n\nI missed the fact that we lack \"here is a pair of test files in this\nlang $p; go find diff between them and make sure we have the right\nhunk header\" for many $p.\n\nSo the kind of improvements I wondered about are allowed to come\nlater, but shouldn't be done in this series, especiallly not at this\nstep in the series.\n\nThanks.\n"},{"id":"237500","messageId":"20140324213659.GF13728@sigill.intra.peff.net","threadId":"36060","inReplyTo":"52505977c20a480941ae1f85f50ffb7a0bbffedb.1395433874.git.j6t@kdbg.org","subject":"Re: [PATCH 03/10] t4018: an infrastructure to test hunk headers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-24T21:36:59Z","receivedAt":"2014-03-24T21:36:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 21, 2014 at 10:07:15PM +0100, Johannes Sixt wrote:\n\n> Add an infrastructure that simplifies adding new tests of the hunk\n> header regular expressions.\n> \n> To add new tests, a file with the syntax to test can be dropped in the\n> directory t4018. The README file explains how a test file must contain;\n> the README itself tests the default behavior.\n\nI really like the cleanups you've done in t4018. I noticed how messy it\nwas when I modified it recently, but I didn't take the time to clean it.\n\n> diff --git a/t/t4018/README b/t/t4018/README\n> new file mode 100644\n> index 0000000..283e01cc\n> --- /dev/null\n> +++ b/t/t4018/README\n> @@ -0,0 +1,18 @@\n> +How to write RIGHT test cases\n> +=============================\n> +\n> +Insert the word \"ChangeMe\" (exactly this form) at a distance of\n> +at least two lines from the line that must appear in the hunk header.\n\nThe existing tests use -U1 to make writing cases simpler. Is there a\nreason not to continue that (or if you found that porting the existing\ncases was not a chore with -U3, I can buy that argument, too)?\n\n> +The text that must appear in the hunk header must contain the word\n> +\"right\", but in all upper-case, like in the title above.\n> +\n> +To mark a test case that highlights a malfunction, insert the word\n> +BROKEN in all lower-case somewhere in the file.\n\nI wondered why you wouldn't write them in the case you are indicating,\nwhen...\n\n> +This text is a bit twisted and out of order, but it is itself a\n> +test case for the default hunk header pattern. Know what you are doing\n> +if you change it.\n\nAh. Clever. :)\n\n-Peff\n"},{"id":"237501","messageId":"20140324213913.GA14890@sigill.intra.peff.net","threadId":"36060","inReplyTo":"20140324213659.GF13728@sigill.intra.peff.net","subject":"Re: [PATCH 03/10] t4018: an infrastructure to test hunk headers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-24T21:39:13Z","receivedAt":"2014-03-24T21:39:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 24, 2014 at 05:36:59PM -0400, Jeff King wrote:\n\n> > +How to write RIGHT test cases\n> > +=============================\n> > +\n> > +Insert the word \"ChangeMe\" (exactly this form) at a distance of\n> > +at least two lines from the line that must appear in the hunk header.\n> \n> The existing tests use -U1 to make writing cases simpler. Is there a\n> reason not to continue that (or if you found that porting the existing\n> cases was not a chore with -U3, I can buy that argument, too)?\n\nI take it back. You did keep \"-U1\" in the result. Is this \"two lines\"\nrule necessary, then?\n\n-Peff\n"},{"id":"237506","messageId":"20140324214954.GG13728@sigill.intra.peff.net","threadId":"36060","inReplyTo":"cover.1395433874.git.j6t@kdbg.org","subject":"Re: [PATCH 00/10] userdiff: cpp pattern simplification and test framework","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-24T21:49:54Z","receivedAt":"2014-03-24T21:49:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 21, 2014 at 10:07:12PM +0100, Johannes Sixt wrote:\n\n> Here is a series that makes the hunk header pattern for C and C++ even\n> simpler than suggested by Peff in [1] to catch a lot more C++ functions\n> and two more C patterns.\n> \n> As a preparatory work, the test cases are totally rewritten to make it\n> a lot simpler to drop in new tests. There was an earlier attempt to\n> change the infrastructure [2], and it is the reason for the widened Cc\n> list.\n\nThanks. This looks sane overall, and I am especially happy about the\nincreased test coverage. I do not know if your 10/10 misses some cases,\nbut at least now we can collect and reason about a set of examples going\nforward.\n\n-Peff\n"},{"id":"237769","messageId":"5331E1F6.1070506@kdbg.org","threadId":"36060","inReplyTo":"20140324213913.GA14890@sigill.intra.peff.net","subject":"Re: [PATCH 03/10] t4018: an infrastructure to test hunk headers","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2014-03-25T20:07:18Z","receivedAt":"2014-03-25T20:07:18Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 24.03.2014 22:39, schrieb Jeff King:\n> On Mon, Mar 24, 2014 at 05:36:59PM -0400, Jeff King wrote:\n> \n>>> +How to write RIGHT test cases\n>>> +=============================\n>>> +\n>>> +Insert the word \"ChangeMe\" (exactly this form) at a distance of\n>>> +at least two lines from the line that must appear in the hunk header.\n>>\n>> The existing tests use -U1 to make writing cases simpler. Is there a\n>> reason not to continue that (or if you found that porting the existing\n>> cases was not a chore with -U3, I can buy that argument, too)?\n> \n> I take it back. You did keep \"-U1\" in the result. Is this \"two lines\"\n> rule necessary, then?\n\nWhen we have\n\n one\n two\n three\n\nhow would you describe the distance between \"one\" and \"three\"?\n\nOr do you have a wrong expectation how we determine the hunk header? The\nhunk header is searched in the text *before* the pre-context. Therefore,\nwe need at least as many lines between the intended hunk header line\n(with tag RIGHT) and the changed line (ChangeMe) as we request with -U\nfor the context.\n\nSo: yes, we need the rule.\n\n-- Hannes\n"},{"id":"237775","messageId":"20140325214232.GA18217@sigill.intra.peff.net","threadId":"36060","inReplyTo":"5331E1F6.1070506@kdbg.org","subject":"Re: [PATCH 03/10] t4018: an infrastructure to test hunk headers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-25T21:42:33Z","receivedAt":"2014-03-25T21:42:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 25, 2014 at 09:07:18PM +0100, Johannes Sixt wrote:\n\n> Am 24.03.2014 22:39, schrieb Jeff King:\n> > On Mon, Mar 24, 2014 at 05:36:59PM -0400, Jeff King wrote:\n> > \n> >>> +How to write RIGHT test cases\n> >>> +=============================\n> >>> +\n> >>> +Insert the word \"ChangeMe\" (exactly this form) at a distance of\n> >>> +at least two lines from the line that must appear in the hunk header.\n> >>\n> >> The existing tests use -U1 to make writing cases simpler. Is there a\n> >> reason not to continue that (or if you found that porting the existing\n> >> cases was not a chore with -U3, I can buy that argument, too)?\n> > \n> > I take it back. You did keep \"-U1\" in the result. Is this \"two lines\"\n> > rule necessary, then?\n> \n> When we have\n> \n> one\n> two\n> three\n> \n> how would you describe the distance between \"one\" and \"three\"?\n\nI read it as \"two lines between\". Though that would be appropriate for\n-U2, so I think I probably had an off-by-one in my head, then, too.\n\nSo it is probably fine as-is, and this was mostly just me confusing\nmyself. Sorry for the noise.\n\n-Peff\n"}]}