{"thread":{"id":"44876","subject":"[PATCH 2/3] xdiff: -W: include immediately preceding non-empty lines in context","startedAt":"2017-01-13T16:15:26Z","lastAt":"2017-01-15T23:28:12Z","messageCount":15,"participants":["Vegard Nossum","René Scharfe","Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"309361","messageId":"1484324112-17773-2-git-send-email-vegard.nossum@oracle.com","threadId":"44876","inReplyTo":"1484324112-17773-1-git-send-email-vegard.nossum@oracle.com","subject":"[PATCH 2/3] xdiff: -W: include immediately preceding non-empty lines in context","fromName":"Vegard Nossum","fromEmail":"vegard.nossum@oracle.com","sentAt":"2017-01-13T16:15:11Z","receivedAt":"2017-01-13T16:15:26Z","isPatch":true,"sender":{"key":"vegard.nossum@oracle.com","avatar":"https://avatars.githubusercontent.com/u/24173?v=4"},"body":"When using -W to include the whole function in the diff context, you\nare typically doing this to be able to review the change in its entirety\nwithin the context of the function. It is therefore almost always\ndesirable to include any comments that immediately precede the function.\n\nThis also the fixes the case for C where the declaration is split across\nmultiple lines (where the first line of the declaration would not be\nincluded in the output), e.g.:\n\n\tvoid\n\tdummy(void)\n\t{\n\t\t...\n\t}\n\nWe can include these lines by simply scanning upwards from the place of\nthe detected function start until we hit the first non-blank line.\n\nCc: René Scharfe \nSigned-off-by: Vegard Nossum \n---\n xdiff/xemit.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/xdiff/xemit.c b/xdiff/xemit.c\nindex 8c88dbd..3a3060d 100644\n--- a/xdiff/xemit.c\n+++ b/xdiff/xemit.c\n@@ -200,6 +200,8 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,\n \t\t\t}\n \n \t\t\tfs1 = get_func_line(xe, xecfg, NULL, i1, -1);\n+\t\t\twhile (fs1 > 0 && !is_empty_rec(&xe->xdf1, fs1 - 1))\n+\t\t\t\tfs1--;\n \t\t\tif (fs1 < 0)\n \t\t\t\tfs1 = 0;\n \t\t\tif (fs1 < s1) {\n@@ -220,6 +222,8 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,\n \t\t\tlong fe1 = get_func_line(xe, xecfg, NULL,\n \t\t\t\t\t\t xche->i1 + xche->chg1,\n \t\t\t\t\t\t xe->xdf1.nrec);\n+\t\t\twhile (fe1 > 0 && !is_empty_rec(&xe->xdf1, fe1 - 1))\n+\t\t\t\tfe1--;\n \t\t\twhile (fe1 > 0 && is_empty_rec(&xe->xdf1, fe1 - 1))\n \t\t\t\tfe1--;\n \t\t\tif (fe1 < 0)\n-- \n2.7.4\n\n"},{"id":"309362","messageId":"1484324112-17773-1-git-send-email-vegard.nossum@oracle.com","threadId":"44876","inReplyTo":null,"subject":"[PATCH 1/3] xdiff: -W: relax end-of-file function detection","fromName":"Vegard Nossum","fromEmail":"vegard.nossum@oracle.com","sentAt":"2017-01-13T16:15:10Z","receivedAt":"2017-01-13T16:15:34Z","isPatch":true,"sender":{"key":"vegard.nossum@oracle.com","avatar":"https://avatars.githubusercontent.com/u/24173?v=4"},"body":"When adding a new function to the end of a file, it's enough to know\nthat 1) the addition is at the end of the file; and 2) there is a\nfunction _somewhere_ in there.\n\nIf we had simply been changing the end of an existing function, then we\nwould also be deleting something from the old version.\n\nThis fixes the case where we add e.g.\n\n\t// Begin of dummy\n\tstatic int dummy(void)\n\t{\n\t}\n\nto the end of the file.\n\nCc: René Scharfe \nSigned-off-by: Vegard Nossum \n---\n xdiff/xemit.c | 14 ++++++--------\n 1 file changed, 6 insertions(+), 8 deletions(-)\n\ndiff --git a/xdiff/xemit.c b/xdiff/xemit.c\nindex 7389ce4..8c88dbd 100644\n--- a/xdiff/xemit.c\n+++ b/xdiff/xemit.c\n@@ -183,16 +183,14 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,\n \n \t\t\t\t/*\n \t\t\t\t * We don't need additional context if\n-\t\t\t\t * a whole function was added, possibly\n-\t\t\t\t * starting with empty lines.\n+\t\t\t\t * a whole function was added.\n \t\t\t\t */\n-\t\t\t\twhile (i2 < xe->xdf2.nrec &&\n-\t\t\t\t is_empty_rec(&xe->xdf2, i2))\n+\t\t\t\twhile (i2 < xe->xdf2.nrec) {\n+\t\t\t\t\tif (match_func_rec(&xe->xdf2, xecfg, i2,\n+\t\t\t\t\t\tdummy, sizeof(dummy)) >= 0)\n+\t\t\t\t\t\tgoto post_context_calculation;\n \t\t\t\t\ti2++;\n-\t\t\t\tif (i2 < xe->xdf2.nrec &&\n-\t\t\t\t match_func_rec(&xe->xdf2, xecfg, i2,\n-\t\t\t\t\t\t dummy, sizeof(dummy)) >= 0)\n-\t\t\t\t\tgoto post_context_calculation;\n+\t\t\t\t}\n \n \t\t\t\t/*\n \t\t\t\t * Otherwise get more context from the\n-- \n2.7.4\n\n"},{"id":"309363","messageId":"1484324112-17773-3-git-send-email-vegard.nossum@oracle.com","threadId":"44876","inReplyTo":"1484324112-17773-1-git-send-email-vegard.nossum@oracle.com","subject":"[PATCH 3/3] t/t4051-diff-function-context: improve tests for new diff -W behaviour","fromName":"Vegard Nossum","fromEmail":"vegard.nossum@oracle.com","sentAt":"2017-01-13T16:15:12Z","receivedAt":"2017-01-13T16:15:41Z","isPatch":true,"sender":{"key":"vegard.nossum@oracle.com","avatar":"https://avatars.githubusercontent.com/u/24173?v=4"},"body":"We now include non-empty lines immediately before (and after) a function\nas belonging to the function.\n\nWe can test this new functionality by moving the \"// Begin\" markers on\neach function to the previous line.\n\nThis commit is intentionally not part of the previous commits in order\nto show that the tests do not break even when changing the behaviour of\n'diff -W' in the previous commits.\n\nCc: René Scharfe \nSigned-off-by: Vegard Nossum \n---\n t/t4051-diff-function-context.sh | 2 +-\n t/t4051/appended1.c | 3 ++-\n t/t4051/dummy.c | 3 ++-\n t/t4051/hello.c | 3 ++-\n 4 files changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t4051-diff-function-context.sh b/t/t4051-diff-function-context.sh\nindex 6154acb..d1a646f 100755\n--- a/t/t4051-diff-function-context.sh\n+++ b/t/t4051-diff-function-context.sh\n@@ -72,7 +72,7 @@ test_expect_success 'setup' '\n \n \t# overlap function context of 1st change and -u context of 2nd change\n \tgrep -v \"delete me from hello\" <\"$dir/hello.c\" >file.c &&\n-\tsed 2p <\"$dir/dummy.c\" >>file.c &&\n+\tsed 3p <\"$dir/dummy.c\" >>file.c &&\n \tcommit_and_tag changed_hello_dummy file.c &&\n \n \tgit checkout initial &&\ndiff --git a/t/t4051/appended1.c b/t/t4051/appended1.c\nindex a9f56f1..8683983 100644\n--- a/t/t4051/appended1.c\n+++ b/t/t4051/appended1.c\n@@ -1,5 +1,6 @@\n \n-int appended(void) // Begin of first part\n+// Begin of first part\n+int appended(void)\n {\n \tint i;\n \tchar *s = \"a string\";\ndiff --git a/t/t4051/dummy.c b/t/t4051/dummy.c\nindex a43016e..db227a8 100644\n--- a/t/t4051/dummy.c\n+++ b/t/t4051/dummy.c\n@@ -1,5 +1,6 @@\n \n-static int dummy(void)\t// Begin of dummy\n+// Begin of dummy\n+static int dummy(void)\n {\n \tint rc = 0;\n \ndiff --git a/t/t4051/hello.c b/t/t4051/hello.c\nindex 63b1a1e..75eac1d 100644\n--- a/t/t4051/hello.c\n+++ b/t/t4051/hello.c\n@@ -1,5 +1,6 @@\n \n-static void hello(void)\t// Begin of hello\n+// Begin of hello\n+static void hello(void)\n {\n \t/*\n \t * Classic.\n-- \n2.7.4\n\n"},{"id":"309371","messageId":"23ca90e5-90a4-a03f-e51e-c82ebc75c16f@web.de","threadId":"44876","inReplyTo":"1484324112-17773-1-git-send-email-vegard.nossum@oracle.com","subject":"Re: [PATCH 1/3] xdiff: -W: relax end-of-file function detection","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-01-13T17:49:28Z","receivedAt":"2017-01-13T17:57:04Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 13.01.2017 um 17:15 schrieb Vegard Nossum:\n> When adding a new function to the end of a file, it's enough to know\n> that 1) the addition is at the end of the file; and 2) there is a\n> function _somewhere_ in there.\n>\n> If we had simply been changing the end of an existing function, then we\n> would also be deleting something from the old version.\n\nThat makes sense, thanks.\n\n> This fixes the case where we add e.g.\n>\n> \t// Begin of dummy\n> \tstatic int dummy(void)\n> \t{\n> \t}\n>\n> to the end of the file.\n\nWithout this patch the unchanged function before the added lines is \nshown in its entirety as (uncalled for) context.\n\n>\n> Cc: René Scharfe \n> Signed-off-by: Vegard Nossum \n> ---\n> xdiff/xemit.c | 14 ++++++--------\n> 1 file changed, 6 insertions(+), 8 deletions(-)\n>\n> diff --git a/xdiff/xemit.c b/xdiff/xemit.c\n> index 7389ce4..8c88dbd 100644\n> --- a/xdiff/xemit.c\n> +++ b/xdiff/xemit.c\n> @@ -183,16 +183,14 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,\n>\n> \t\t\t\t/*\n> \t\t\t\t * We don't need additional context if\n> -\t\t\t\t * a whole function was added, possibly\n> -\t\t\t\t * starting with empty lines.\n> +\t\t\t\t * a whole function was added.\n> \t\t\t\t */\n> -\t\t\t\twhile (i2 < xe->xdf2.nrec &&\n> -\t\t\t\t is_empty_rec(&xe->xdf2, i2))\n> +\t\t\t\twhile (i2 < xe->xdf2.nrec) {\n> +\t\t\t\t\tif (match_func_rec(&xe->xdf2, xecfg, i2,\n> +\t\t\t\t\t\tdummy, sizeof(dummy)) >= 0)\n\nNit: I don't like the indentation here. Giving \"dummy\" its own line is \nalso not exactly pretty, but at least would allow the parameters to be \naligned on the opening parenthesis.\n\n> +\t\t\t\t\t\tgoto post_context_calculation;\n> \t\t\t\t\ti2++;\n> -\t\t\t\tif (i2 < xe->xdf2.nrec &&\n> -\t\t\t\t match_func_rec(&xe->xdf2, xecfg, i2,\n> -\t\t\t\t\t\t dummy, sizeof(dummy)) >= 0)\n> -\t\t\t\t\tgoto post_context_calculation;\n> +\t\t\t\t}\n>\n> \t\t\t\t/*\n> \t\t\t\t * Otherwise get more context from the\n>\n"},{"id":"309377","messageId":"e55dc4dd-768b-8c9b-e3b2-e850d5d521f5@web.de","threadId":"44876","inReplyTo":"1484324112-17773-2-git-send-email-vegard.nossum@oracle.com","subject":"Re: [PATCH 2/3] xdiff: -W: include immediately preceding non-empty lines in context","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-01-13T18:19:48Z","receivedAt":"2017-01-13T18:20:14Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 13.01.2017 um 17:15 schrieb Vegard Nossum:\n> When using -W to include the whole function in the diff context, you\n> are typically doing this to be able to review the change in its entirety\n> within the context of the function. It is therefore almost always\n> desirable to include any comments that immediately precede the function.\n>\n> This also the fixes the case for C where the declaration is split across\n> multiple lines (where the first line of the declaration would not be\n> included in the output), e.g.:\n>\n> \tvoid\n> \tdummy(void)\n> \t{\n> \t\t...\n> \t}\n>\n\nThat's true, but I'm not sure \"non-empty line before function line\" is \ngood enough a definition for desirable lines. It wouldn't work for \npeople who don't believe in empty lines. Or for those that put a blank \nline between comment and function. (I have an opinion on such habits, \nbut git diff should probably stay neutral.) And that's just for C code; \nI have no idea how this heuristic would hold up for other file types \nlike HTML.\n\nWe can identify function lines with arbitrary precision (with a \nxfuncname regex, if needed), but there is no accurate way to classify \nlines as comments, or as the end of functions. Adding optional regexes \nfor single- and multi-line comments would help, at least for C.\n\nRené\n"},{"id":"309383","messageId":"CAGZ79kamTgefk9RxUwFk7hgAQK7kEJJKS6RjNMbnv2=Vk=BTSQ@mail.gmail.com","threadId":"44876","inReplyTo":"e55dc4dd-768b-8c9b-e3b2-e850d5d521f5@web.de","subject":"Re: [PATCH 2/3] xdiff: -W: include immediately preceding non-empty lines in context","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-01-13T18:44:02Z","receivedAt":"2017-01-13T18:44:08Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jan 13, 2017 at 10:19 AM, René Scharfe wrote:\n> Am 13.01.2017 um 17:15 schrieb Vegard Nossum:\n>>\n>> When using -W to include the whole function in the diff context, you\n>> are typically doing this to be able to review the change in its entirety\n>> within the context of the function. It is therefore almost always\n>> desirable to include any comments that immediately precede the function.\n\nDo we need a small comment in the actual code to hint at why we count\nupwards there?\n\n>>\n>> This also the fixes the case for C where the declaration is split across\n>> multiple lines (where the first line of the declaration would not be\n>> included in the output), e.g.:\n>>\n>> void\n>> dummy(void)\n>> {\n>> ...\n>> }\n>>\n>\n> That's true, but I'm not sure \"non-empty line before function line\" is good\n> enough a definition for desirable lines. It wouldn't work for people who\n> don't believe in empty lines. Or for those that put a blank line between\n> comment and function. (I have an opinion on such habits, but git diff\n> should probably stay neutral.) And that's just for C code; I have no idea\n> how this heuristic would hold up for other file types like HTML.\n\nI think empty lines are \"good as a first approach\", see e.g.\n433860f3d0beb0c6 the \"compaction\" heuristic for a similar\nthing (the compaction was introduced at d634d61ed), and then\nwe can build a more elaborate thing on top.\n\n>\n> We can identify function lines with arbitrary precision (with a xfuncname\n> regex, if needed), but there is no accurate way to classify lines as\n> comments, or as the end of functions. Adding optional regexes for single-\n> and multi-line comments would help, at least for C.\n\nThat would cover Java and whole lot of other C like languages. So a good\nstart as well IMHO.\n\n>\n> René\n"},{"id":"309389","messageId":"xmqqeg06hh6z.fsf@gitster.mtv.corp.google.com","threadId":"44876","inReplyTo":"e55dc4dd-768b-8c9b-e3b2-e850d5d521f5@web.de","subject":"Re: [PATCH 2/3] xdiff: -W: include immediately preceding non-empty lines in context","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-13T19:51:16Z","receivedAt":"2017-01-13T19:51:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Am 13.01.2017 um 17:15 schrieb Vegard Nossum:\n>> When using -W to include the whole function in the diff context, you\n>> are typically doing this to be able to review the change in its entirety\n>> within the context of the function. It is therefore almost always\n>> desirable to include any comments that immediately precede the function.\n>>\n>> This also the fixes the case for C where the declaration is split across\n>> multiple lines (where the first line of the declaration would not be\n>> included in the output), e.g.:\n>>\n>> \tvoid\n>> \tdummy(void)\n>> \t{\n>> \t\t...\n>> \t}\n>>\n>\n> That's true, but I'm not sure \"non-empty line before function line\" is\n> good enough a definition for desirable lines. It wouldn't work for\n> people who don't believe in empty lines. Or for those that put a\n> blank line between comment and function. (I have an opinion on such\n> habits, but git diff should probably stay neutral.) And that's just\n> for C code; I have no idea how this heuristic would hold up for other\n> file types like HTML.\n\nAs you are, I am fairly negative on the heuristic based on the\n\"non-blank\" thing. We tried once with compaction-heuristics already\nand it did not quite perform well. Let's not hardcode another one.\n\n> We can identify function lines with arbitrary precision (with a\n> xfuncname regex, if needed), but there is no accurate way to classify\n> lines as comments, or as the end of functions. Adding optional\n> regexes for single- and multi-line comments would help, at least for\n> C.\n\nThe funcline regexp is used for two related but different purposes.\nIt identifies a single line to be placed on @@ ... @@ line before a\ndiff hunk. This line however does not have to be at the beginning\nof a function. It has to be the line that conveys the most\nsignificant information (e.g. the name of the function).\n\nThe way \"diff -W\" codepath used it as if it were always the very\nfirst line of a function was bound to invite a patch like this, and\nif we want to be extra elaborate, I agree that an extra mechanism to\nsay \"the line the funcline regexp matches is not the beginning of a\nfunction, but the beginning is a line that matches this other regexp\nbefore that line\" may help.\n\nDo we really want to be that elaborate, though? I dunno.\n\nI wonder if it would be sufficient to make -W take an optional\nnumber, e.g. \"git show -W4\", to add extre context lines before the\nfuncline.\n"},{"id":"309395","messageId":"c74c260d-1a4d-39f6-a644-4f90a67d6d82@oracle.com","threadId":"44876","inReplyTo":"xmqqeg06hh6z.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/3] xdiff: -W: include immediately preceding non-empty lines in context","fromName":"Vegard Nossum","fromEmail":"vegard.nossum@oracle.com","sentAt":"2017-01-13T20:20:09Z","receivedAt":"2017-01-13T20:20:22Z","isPatch":true,"sender":{"key":"vegard.nossum@oracle.com","avatar":"https://avatars.githubusercontent.com/u/24173?v=4"},"body":"On 13/01/2017 20:51, Junio C Hamano wrote:\n> René Scharfe writes:\n>> That's true, but I'm not sure \"non-empty line before function line\" is\n>> good enough a definition for desirable lines. It wouldn't work for\n>> people who don't believe in empty lines. Or for those that put a\n>> blank line between comment and function. (I have an opinion on such\n>> habits, but git diff should probably stay neutral.) And that's just\n>> for C code; I have no idea how this heuristic would hold up for other\n>> file types like HTML.\n>\n> As you are, I am fairly negative on the heuristic based on the\n> \"non-blank\" thing. We tried once with compaction-heuristics already\n> and it did not quite perform well. Let's not hardcode another one.\n\nThe patch will work as intended and as expected for 95% of the users out\nthere (javadoc, Doxygen, kerneldoc, etc. all have the comment\nimmediately preceding the function) and fixes a very real problem for me\n(and I expect many others) _today_; for the remaining 5% (who put a\nblank line between their comment and the start of the function) it will\nrevert back to the current behaviour, so there should be no regression\nfor them.\n\nFor the 0% who don't put even a single blank line between their\nfunctions, it will probably not work as expected, but then again I have\nnever seen such a coding style in any language, so I am doubtful if this\nis something that needs to be taken into account in the first place.\n\n>> We can identify function lines with arbitrary precision (with a\n>> xfuncname regex, if needed), but there is no accurate way to classify\n>> lines as comments, or as the end of functions. Adding optional\n>> regexes for single- and multi-line comments would help, at least for\n>> C.\n>\n> The funcline regexp is used for two related but different purposes.\n> It identifies a single line to be placed on @@ ... @@ line before a\n> diff hunk. This line however does not have to be at the beginning\n> of a function. It has to be the line that conveys the most\n> significant information (e.g. the name of the function).\n>\n> The way \"diff -W\" codepath used it as if it were always the very\n> first line of a function was bound to invite a patch like this, and\n> if we want to be extra elaborate, I agree that an extra mechanism to\n> say \"the line the funcline regexp matches is not the beginning of a\n> function, but the beginning is a line that matches this other regexp\n> before that line\" may help.\n>\n> Do we really want to be that elaborate, though? I dunno.\n\nAdding a regex instead of the simple \"blank line\" test doesn't seem very\ndifficult to do, but I am doubtful that it will make any difference in\npractice. But that can be done incrementally as well by the people who\nwould actually need it (who I strongly suspect do not exist in the first\nplace).\n\n> I wonder if it would be sufficient to make -W take an optional\n> number, e.g. \"git show -W4\", to add extre context lines before the\n> funcline.\n>\n\nI don't like specifying a fixed number, that negates almost all the\nreason for using -W in the first place; I would much prefer adding\na config variable to control the -W behaviour (or a new diff flag).\n\n\nVegard\n"},{"id":"309406","messageId":"xmqqbmvaecpl.fsf@gitster.mtv.corp.google.com","threadId":"44876","inReplyTo":"c74c260d-1a4d-39f6-a644-4f90a67d6d82@oracle.com","subject":"Re: [PATCH 2/3] xdiff: -W: include immediately preceding non-empty lines in context","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-13T23:56:22Z","receivedAt":"2017-01-13T23:56:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Vegard Nossum writes:\n\n> The patch will work as intended and as expected for 95% of the users out\n> there (javadoc, Doxygen, kerneldoc, etc. all have the comment\n> immediately preceding the function) and fixes a very real problem for me\n> (and I expect many others) _today_; for the remaining 5% (who put a\n> blank line between their comment and the start of the function) it will\n> revert back to the current behaviour, so there should be no regression\n> for them.\n\nI notice your 95% are all programming languages, but I am more\nworried about the contents written in non programming languages\n(René gave HTML an an example--there may be other types of contents\nthat we programmer types do not deal with every day, but Git users\ndepend on). \n\nI am also more focused on keeping the codebase maintainable in good\nhealth by making sure that we made an effort to find a solution that\nis general-enough before solving a single specific problem you have\ntoday. We may end up deciding that a blank-line heuristics gives us\ngood enough tradeoff, but I do not want us to make a decision before\nthinking.\n\n>> The way \"diff -W\" codepath used it as if it were always the very\n>> first line of a function was bound to invite a patch like this, and\n>> if we want to be extra elaborate, I agree that an extra mechanism to\n>> say \"the line the funcline regexp matches is not the beginning of a\n>> function, but the beginning is a line that matches this other regexp\n>> before that line\" may help.\n>>\n>> Do we really want to be that elaborate, though? I dunno.\n>\n> Adding a regex instead of the simple \"blank line\" test doesn't seem very\n> difficult to do, but I am doubtful that it will make any difference in\n> practice. But that can be done incrementally as well by the people who\n> would actually need it (who I strongly suspect do not exist in the first\n> place).\n\nAt least, the damage can be limited to the cases we know would work\nwell if we go that way.\n"},{"id":"309418","messageId":"48bdfd94-2fd4-bd55-d78b-2877e195fb82@web.de","threadId":"44876","inReplyTo":"xmqqbmvaecpl.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/3] xdiff: -W: include immediately preceding non-empty lines in context","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-01-14T14:58:25Z","receivedAt":"2017-01-14T14:58:56Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 14.01.2017 um 00:56 schrieb Junio C Hamano:\n> Vegard Nossum writes:\n>\n>> The patch will work as intended and as expected for 95% of the users out\n>> there (javadoc, Doxygen, kerneldoc, etc. all have the comment\n>> immediately preceding the function) and fixes a very real problem for me\n>> (and I expect many others) _today_; for the remaining 5% (who put a\n>> blank line between their comment and the start of the function) it will\n>> revert back to the current behaviour, so there should be no regression\n>> for them.\n>\n> I notice your 95% are all programming languages, but I am more\n> worried about the contents written in non programming languages\n> (René gave HTML an an example--there may be other types of contents\n> that we programmer types do not deal with every day, but Git users\n> depend on).\n>\n> I am also more focused on keeping the codebase maintainable in good\n> health by making sure that we made an effort to find a solution that\n> is general-enough before solving a single specific problem you have\n> today. We may end up deciding that a blank-line heuristics gives us\n> good enough tradeoff, but I do not want us to make a decision before\n> thinking.\n\nHow about extending the context upward only up to and excluding a line \nthat is either empty *or* a function line? That would limit the extra \ncontext to a single function in the worst case.\n\nReducing context at the bottom with the aim to remove comments for the \nnext section is more tricky as it could remove part of the function that \nwe'd like to show if we get the boundary wrong. How bad would it be to \nkeep the southern border unchanged?\n\nRené\n\n"},{"id":"309429","messageId":"xmqqy3ydcaia.fsf@gitster.mtv.corp.google.com","threadId":"44876","inReplyTo":"48bdfd94-2fd4-bd55-d78b-2877e195fb82@web.de","subject":"Re: [PATCH 2/3] xdiff: -W: include immediately preceding non-empty lines in context","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-15T02:39:09Z","receivedAt":"2017-01-15T02:39:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n>> I am also more focused on keeping the codebase maintainable in good\n>> health by making sure that we made an effort to find a solution that\n>> is general-enough before solving a single specific problem you have\n>> today. We may end up deciding that a blank-line heuristics gives us\n>> good enough tradeoff, but I do not want us to make a decision before\n>> thinking.\n>\n> How about extending the context upward only up to and excluding a line\n> that is either empty *or* a function line? That would limit the extra\n> context to a single function in the worst case.\n>\n> Reducing context at the bottom with the aim to remove comments for the\n> next section is more tricky as it could remove part of the function\n> that we'd like to show if we get the boundary wrong. How bad would it\n> be to keep the southern border unchanged?\n\nI personally do not think there is any robust heuristic other than\nVegard's \"a blank line may be a signal enough that lines before that\nare not part of the beginning of the function\", and I think your\n\"hence we look for a blank line but if there is a line that matches\nthe function header, stop there as we know we came too far back\"\nwill be a good-enough safety measure.\n\nI also agree with you that we probably do not want to futz with the\nsouthern border.\n\nThanks.\n"},{"id":"309436","messageId":"0c761135-2696-4b3d-0a4f-3d90edf5da2e@oracle.com","threadId":"44876","inReplyTo":"xmqqy3ydcaia.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/3] xdiff: -W: include immediately preceding non-empty lines in context","fromName":"Vegard Nossum","fromEmail":"vegard.nossum@oracle.com","sentAt":"2017-01-15T10:06:08Z","receivedAt":"2017-01-15T10:06:28Z","isPatch":true,"sender":{"key":"vegard.nossum@oracle.com","avatar":"https://avatars.githubusercontent.com/u/24173?v=4"},"body":"On 15/01/2017 03:39, Junio C Hamano wrote:\n> René Scharfe writes:\n>\n>>> I am also more focused on keeping the codebase maintainable in good\n>>> health by making sure that we made an effort to find a solution that\n>>> is general-enough before solving a single specific problem you have\n>>> today. We may end up deciding that a blank-line heuristics gives us\n>>> good enough tradeoff, but I do not want us to make a decision before\n>>> thinking.\n\nYou are right; I appreciate this approach.\n\n>> How about extending the context upward only up to and excluding a line\n>> that is either empty *or* a function line? That would limit the extra\n>> context to a single function in the worst case.\n>>\n>> Reducing context at the bottom with the aim to remove comments for the\n>> next section is more tricky as it could remove part of the function\n>> that we'd like to show if we get the boundary wrong. How bad would it\n>> be to keep the southern border unchanged?\n>\n> I personally do not think there is any robust heuristic other than\n> Vegard's \"a blank line may be a signal enough that lines before that\n> are not part of the beginning of the function\", and I think your\n> \"hence we look for a blank line but if there is a line that matches\n> the function header, stop there as we know we came too far back\"\n> will be a good-enough safety measure.\n>\n> I also agree with you that we probably do not want to futz with the\n> southern border.\n\nYou are right, trying to change the southern border in this way is not\nquite reliable if there are no empty lines whatsoever and can\nerroneously cause the function context to not include the bottom of the\nfunction being changed.\n\nI'm splitting the function boundary detection logic into separate\nfunctions and trying to solve the above case without breaking the tests\n(and adding a new test for the above case too).\n\nI'll see if I can additionally provide some toggles (flags or config\nvariables) to control the new behaviour, what I had in mind was:\n\n -W[=preamble,=no-preamble]\n --function-context[=preamble,=no-preamble]\n diff.functionContextPreamble = \n\n(where the new logic is controlled by the new config variable and\noverridden by the presence of =preamble or =no-preamble).\n\nThen it also shouldn't be too difficult to add\n\n diff..preamble = \n diff..xpreamble = \n\nto override the heuristic used for function border detection in\nexceptional cases.\n\nYou can argue about the naming now ;-) But I will use this for a start,\nrenaming/reworking it (or throwing it away) afterwards should be easy\nonce the code has been written.\n\n\nVegard\n"},{"id":"309441","messageId":"57bba5fa-015d-5629-73c2-5052ffb39d2a@web.de","threadId":"44876","inReplyTo":"xmqqy3ydcaia.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/3] xdiff: -W: include immediately preceding non-empty lines in context","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-01-15T16:57:38Z","receivedAt":"2017-01-15T16:58:02Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 15.01.2017 um 03:39 schrieb Junio C Hamano:\n> René Scharfe writes:\n> \n>>> I am also more focused on keeping the codebase maintainable in good\n>>> health by making sure that we made an effort to find a solution that\n>>> is general-enough before solving a single specific problem you have\n>>> today. We may end up deciding that a blank-line heuristics gives us\n>>> good enough tradeoff, but I do not want us to make a decision before\n>>> thinking.\n>>\n>> How about extending the context upward only up to and excluding a line\n>> that is either empty *or* a function line? That would limit the extra\n>> context to a single function in the worst case.\n>>\n>> Reducing context at the bottom with the aim to remove comments for the\n>> next section is more tricky as it could remove part of the function\n>> that we'd like to show if we get the boundary wrong. How bad would it\n>> be to keep the southern border unchanged?\n> \n> I personally do not think there is any robust heuristic other than\n> Vegard's \"a blank line may be a signal enough that lines before that\n> are not part of the beginning of the function\", and I think your\n> \"hence we look for a blank line but if there is a line that matches\n> the function header, stop there as we know we came too far back\"\n> will be a good-enough safety measure.\n> \n> I also agree with you that we probably do not want to futz with the\n> southern border.\n\nA replacement patch for 2/3 with these changes would look like this:\n\ndiff --git a/xdiff/xemit.c b/xdiff/xemit.c\nindex 8c88dbde38..9ed54cd318 100644\n--- a/xdiff/xemit.c\n+++ b/xdiff/xemit.c\n@@ -174,11 +174,11 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,\n \t\ts2 = XDL_MAX(xch->i2 - xecfg->ctxlen, 0);\n \n \t\tif (xecfg->flags & XDL_EMIT_FUNCCONTEXT) {\n+\t\t\tchar dummy[1];\n \t\t\tlong fs1, i1 = xch->i1;\n \n \t\t\t/* Appended chunk? */\n \t\t\tif (i1 >= xe->xdf1.nrec) {\n-\t\t\t\tchar dummy[1];\n \t\t\t\tlong i2 = xch->i2;\n \n \t\t\t\t/*\n@@ -200,6 +200,10 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,\n \t\t\t}\n \n \t\t\tfs1 = get_func_line(xe, xecfg, NULL, i1, -1);\n+\t\t\twhile (fs1 > 0 && !is_empty_rec(&xe->xdf1, fs1 - 1) &&\n+\t\t\t match_func_rec(&xe->xdf1, xecfg, fs1 - 1,\n+\t\t\t\t\t dummy, sizeof(dummy)) < 0)\n+\t\t\t\tfs1--;\n \t\t\tif (fs1 < 0)\n \t\t\t\tfs1 = 0;\n \t\t\tif (fs1 < s1) {\n\n"},{"id":"309442","messageId":"2668771d-249b-659d-3a2c-a788d7d5ebd6@web.de","threadId":"44876","inReplyTo":"0c761135-2696-4b3d-0a4f-3d90edf5da2e@oracle.com","subject":"Re: [PATCH 2/3] xdiff: -W: include immediately preceding non-empty lines in context","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-01-15T16:57:45Z","receivedAt":"2017-01-15T16:58:06Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 15.01.2017 um 11:06 schrieb Vegard Nossum:\n> On 15/01/2017 03:39, Junio C Hamano wrote:\n>> René Scharfe writes:\n>>> How about extending the context upward only up to and excluding a line\n>>> that is either empty *or* a function line? That would limit the extra\n>>> context to a single function in the worst case.\n>>>\n>>> Reducing context at the bottom with the aim to remove comments for the\n>>> next section is more tricky as it could remove part of the function\n>>> that we'd like to show if we get the boundary wrong. How bad would it\n>>> be to keep the southern border unchanged?\n>>\n>> I personally do not think there is any robust heuristic other than\n>> Vegard's \"a blank line may be a signal enough that lines before that\n>> are not part of the beginning of the function\", and I think your\n>> \"hence we look for a blank line but if there is a line that matches\n>> the function header, stop there as we know we came too far back\"\n>> will be a good-enough safety measure.\n>>\n>> I also agree with you that we probably do not want to futz with the\n>> southern border.\n>\n> You are right, trying to change the southern border in this way is not\n> quite reliable if there are no empty lines whatsoever and can\n> erroneously cause the function context to not include the bottom of the\n> function being changed.\n>\n> I'm splitting the function boundary detection logic into separate\n> functions and trying to solve the above case without breaking the tests\n> (and adding a new test for the above case too).\n>\n> I'll see if I can additionally provide some toggles (flags or config\n> variables) to control the new behaviour, what I had in mind was:\n>\n> -W[=preamble,=no-preamble]\n> --function-context[=preamble,=no-preamble]\n> diff.functionContextPreamble = \n>\n> (where the new logic is controlled by the new config variable and\n> overridden by the presence of =preamble or =no-preamble).\n\nAdding comments before a function is useful, removing comments after a \nfunction sounds to me as only nice to have (under the assumption that \nthey belong to the next function[*]). How bad would it be to only \nimplement the first part (as in the patch I just sent) without adding \nnew config settings or parameters?\n\nThanks,\nRené\n\n\n[*] Silly counter-example (the #endif line):\n#ifdef SOMETHING\nint f(...) {\n\t// implementation for SOMETHING\n}\n#else\ninf f(...) {\n\t// implementation without SOMETHING\n}\n#endif /* SOMETHING */\n\n"},{"id":"309457","messageId":"xmqqeg03dhtn.fsf@gitster.mtv.corp.google.com","threadId":"44876","inReplyTo":"2668771d-249b-659d-3a2c-a788d7d5ebd6@web.de","subject":"Re: [PATCH 2/3] xdiff: -W: include immediately preceding non-empty lines in context","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-15T23:28:04Z","receivedAt":"2017-01-15T23:28:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> ... How bad would it be to only\n> implement the first part (as in the patch I just sent) without adding\n> new config settings or parameters?\n\nThat probably is a good place to stop ;-)\n"}]}