{"thread":{"id":"54284","subject":"[RFC 0/1] Leading whitespace as a function identification heuristic?","startedAt":"2020-09-23T21:59:21Z","lastAt":"2020-09-25T20:36:29Z","messageCount":10,"participants":["Ryan Zoeller","Junio C Hamano","Jeff King","Phillip Wood","Johannes Sixt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"406225","messageId":"20200923215859.102981-1-rtzoeller@rtzoeller.com","threadId":"54284","inReplyTo":null,"subject":"[RFC 0/1] Leading whitespace as a function identification heuristic?","fromName":"Ryan Zoeller","fromEmail":"rtzoeller@rtzoeller.com","sentAt":"2020-09-23T21:59:09Z","receivedAt":"2020-09-23T21:59:21Z","isPatch":false,"sender":{"key":"rtzoeller@rtzoeller.com","avatar":"https://avatars.githubusercontent.com/u/573497?v=4"},"body":"I've recently been annoyed by git's choice of lines when invoking\n`git diff --function-context`. git isn't doing a _bad_ job per se,\ngiven that it's using regular expressions to parse nonregular languages,\nbut in my opinion it could do better.\n\nThe issue I'm seeing is that constructs like nested functions cause\n`git diff --function-context` to stop taking lines prematurely; a line\ncontaining a function declaration has been reached, but it's not the one\nI'm expecting git to stop at. This results in some of the function being\nomitted from the output of `git diff --function-context`, and also\nmanifests itself as the incorrect function being listed in the hunk header\nin general.\n\nA trivial example: in a Python file containing the following contents,\nmodifying the indicated line will result in git picking `bar()` for the\nfunction header, rather than `foo()`. This is obviously worse for longer\nfunctions, or code with multiple inner functions or classes.\n\n```\ndef foo(xs, i):\n    def bar(x):\n        return x + i\n\n    # Intentionally not using a lambda,\n    # because the issue occurs for inner functions.\n    # Making this comment extra long so the function name\n    # doesn't get automatically included in the context.\n\n    return map(bar, xs) # Modify this line\n```\n\nEven without properly parsing the file as code, git could use\nindentation as a heuristic to determine which lines containing functions\nare relevant. I've used Python as an example here, but I think the\nissue and heuristic apply to many languages.\n\nThis patch implements a proof of concept for this heuristic: only\nlines which are indented less than the hunk are considered eligible\nto contain function definitions. It only supports `git diff`,\nignoring things like `git grep` for simplicity (and introducing\n`-1`s to the diff in leiu of handling them).\n\nWhile I welcome feedback on the existing implementation, I'm really\nrequesting commentary on the following:\n\n1. Is this indentation-aware function identification useful, and\n    generally worth pursuing further?\n2. What level of granularity would be desirable for enabling this?\n    Something akin to xfuncname in .gitattributes, where it can be\n    enabled per path?\n3. How do we handle files with a mix of tabs and spaces?\n\nBest,\nRyan\n\nRyan Zoeller (1):\n  xdiff: use leading whitespace in function heuristic\n\n grep.c            |  2 +-\n line-range.c      |  2 +-\n xdiff-interface.c | 14 +++++++++++++-\n xdiff/xdiff.h     |  2 +-\n xdiff/xemit.c     | 23 +++++++++++++++++------\n 5 files changed, 33 insertions(+), 10 deletions(-)\n\n-- \n2.28.0.586.g47c91ef7fe\n\n\n"},{"id":"406226","messageId":"20200923215859.102981-2-rtzoeller@rtzoeller.com","threadId":"54284","inReplyTo":"20200923215859.102981-1-rtzoeller@rtzoeller.com","subject":"[RFC 1/1] xdiff: use leading whitespace in function heuristic","fromName":"Ryan Zoeller","fromEmail":"rtzoeller@rtzoeller.com","sentAt":"2020-09-23T21:59:19Z","receivedAt":"2020-09-23T21:59:35Z","isPatch":false,"sender":{"key":"rtzoeller@rtzoeller.com","avatar":"https://avatars.githubusercontent.com/u/573497?v=4"},"body":"The regular expressions specified in userdiff.c, as well as user-defined\nexpressions, allow git to detect which lines of code which declare functions\n(as well as other notable lines, such as class declarations).\n\nAlthough useful, these regular expressions can't identify which function\na line of code belongs to, only the closest function to it.\nLanguages which allow for nested functions -- or functions inside of\nclasses -- can trip this mechanism up.\n\nSince many languages use indentation to associate lines of code with a\nfunction (either semantically or cosmetically), we can use indentation\nas an additional heuristic for identifying the owning function.\n\nSpecifically, this assumes code belongs to a function which is less\nindented than it.\n\nSigned-off-by: Ryan Zoeller <rtzoeller@rtzoeller.com>\n---\n grep.c            |  2 +-\n line-range.c      |  2 +-\n xdiff-interface.c | 14 +++++++++++++-\n xdiff/xdiff.h     |  2 +-\n xdiff/xemit.c     | 23 +++++++++++++++++------\n 5 files changed, 33 insertions(+), 10 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 54af9f813e..3281f19977 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1555,7 +1555,7 @@ static int match_funcname(struct grep_opt *opt, struct grep_source *gs, char *bo\n \n \tif (xecfg) {\n \t\tchar buf[1];\n-\t\treturn xecfg->find_func(bol, eol - bol, buf, 1,\n+\t\treturn xecfg->find_func(bol, eol - bol, buf, 1, -1,\n \t\t\t\t\txecfg->find_func_priv) >= 0;\n \t}\n \ndiff --git a/line-range.c b/line-range.c\nindex 9b50583dc0..eb9540bc76 100644\n--- a/line-range.c\n+++ b/line-range.c\n@@ -119,7 +119,7 @@ static int match_funcname(xdemitconf_t *xecfg, const char *bol, const char *eol)\n {\n \tif (xecfg) {\n \t\tchar buf[1];\n-\t\treturn xecfg->find_func(bol, eol - bol, buf, 1,\n+\t\treturn xecfg->find_func(bol, eol - bol, buf, 1, -1,\n \t\t\t\t\txecfg->find_func_priv) >= 0;\n \t}\n \ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex 4d20069302..d93cb5c72e 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -201,7 +201,7 @@ struct ff_regs {\n };\n \n static long ff_regexp(const char *line, long len,\n-\t\tchar *buffer, long buffer_size, void *priv)\n+\t\tchar *buffer, long buffer_size, long max_leading_spaces, void *priv)\n {\n \tstruct ff_regs *regs = priv;\n \tregmatch_t pmatch[2];\n@@ -216,6 +216,18 @@ static long ff_regexp(const char *line, long len,\n \t\t\tlen--;\n \t}\n \n+\t// TODO: Is it faster to check whitespace only after matching the regex?\n+\tif (max_leading_spaces >= 0) {\n+\t\tlong leading_spaces;\n+\t\tfor (leading_spaces = 0; leading_spaces < len\n+\t\t\t\t&& leading_spaces <= max_leading_spaces\n+\t\t\t\t&& isspace(line[leading_spaces]); leading_spaces++)\n+\t\t\t;\n+\n+\t\tif (leading_spaces > max_leading_spaces)\n+\t\t\treturn -1;\n+\t}\n+\n \tfor (i = 0; i < regs->nr; i++) {\n \t\tstruct ff_reg *reg = regs->array + i;\n \t\tif (!regexec_buf(&reg->re, line, len, 2, pmatch, 0)) {\ndiff --git a/xdiff/xdiff.h b/xdiff/xdiff.h\nindex 032e3a9f41..f78c30c527 100644\n--- a/xdiff/xdiff.h\n+++ b/xdiff/xdiff.h\n@@ -93,7 +93,7 @@ typedef struct s_xdemitcb {\n \tint (*out_line)(void *, mmbuffer_t *, int);\n } xdemitcb_t;\n \n-typedef long (*find_func_t)(const char *line, long line_len, char *buffer, long buffer_size, void *priv);\n+typedef long (*find_func_t)(const char *line, long line_len, char *buffer, long buffer_size, long max_leading_spaces, void *priv);\n \n typedef int (*xdl_emit_hunk_consume_func_t)(long start_a, long count_a,\n \t\t\t\t\t    long start_b, long count_b,\ndiff --git a/xdiff/xemit.c b/xdiff/xemit.c\nindex 9d7d6c5087..1de68008f9 100644\n--- a/xdiff/xemit.c\n+++ b/xdiff/xemit.c\n@@ -95,7 +95,7 @@ xdchange_t *xdl_get_hunk(xdchange_t **xscr, xdemitconf_t const *xecfg)\n }\n \n \n-static long def_ff(const char *rec, long len, char *buf, long sz, void *priv)\n+static long def_ff(const char *rec, long len, char *buf, long sz, long max_leading_spaces, void *priv)\n {\n \tif (len > 0 &&\n \t\t\t(isalpha((unsigned char)*rec) || /* identifier? */\n@@ -112,19 +112,19 @@ static long def_ff(const char *rec, long len, char *buf, long sz, void *priv)\n }\n \n static long match_func_rec(xdfile_t *xdf, xdemitconf_t const *xecfg, long ri,\n-\t\t\t   char *buf, long sz)\n+\t\t\t   char *buf, long sz, long max_leading_spaces)\n {\n \tconst char *rec;\n \tlong len = xdl_get_rec(xdf, ri, &rec);\n \tif (!xecfg->find_func)\n-\t\treturn def_ff(rec, len, buf, sz, xecfg->find_func_priv);\n-\treturn xecfg->find_func(rec, len, buf, sz, xecfg->find_func_priv);\n+\t\treturn def_ff(rec, len, buf, sz, max_leading_spaces, xecfg->find_func_priv);\n+\treturn xecfg->find_func(rec, len, buf, sz, max_leading_spaces, xecfg->find_func_priv);\n }\n \n static int is_func_rec(xdfile_t *xdf, xdemitconf_t const *xecfg, long ri)\n {\n \tchar dummy[1];\n-\treturn match_func_rec(xdf, xecfg, ri, dummy, sizeof(dummy)) >= 0;\n+\treturn match_func_rec(xdf, xecfg, ri, dummy, -1, sizeof(dummy)) >= 0;\n }\n \n struct func_line {\n@@ -137,12 +137,23 @@ static long get_func_line(xdfenv_t *xe, xdemitconf_t const *xecfg,\n {\n \tlong l, size, step = (start > limit) ? -1 : 1;\n \tchar *buf, dummy[1];\n+\tlong leading_spaces;\n+\n+\tif (start - step >= 0 && start - step < xe->xdf1.nrec) {\n+\t\txrecord_t *first_line = xe->xdf1.recs[start - step];\n+\n+\t\tfor (leading_spaces = 0; first_line->ptr[leading_spaces]\n+\t\t\t\t&& isspace(first_line->ptr[leading_spaces]); leading_spaces++)\n+\t\t\t;\n+\t} else {\n+\t\tleading_spaces = 0;\n+\t}\n \n \tbuf = func_line ? func_line->buf : dummy;\n \tsize = func_line ? sizeof(func_line->buf) : sizeof(dummy);\n \n \tfor (l = start; l != limit && 0 <= l && l < xe->xdf1.nrec; l += step) {\n-\t\tlong len = match_func_rec(&xe->xdf1, xecfg, l, buf, size);\n+\t\tlong len = match_func_rec(&xe->xdf1, xecfg, l, buf, size, leading_spaces - 1);\n \t\tif (len >= 0) {\n \t\t\tif (func_line)\n \t\t\t\tfunc_line->len = len;\n-- \n2.28.0.586.g47c91ef7fe\n\n\n"},{"id":"406238","messageId":"xmqqzh5fhduk.fsf@gitster.c.googlers.com","threadId":"54284","inReplyTo":"20200923215859.102981-1-rtzoeller@rtzoeller.com","subject":"Re: [RFC 0/1] Leading whitespace as a function identification heuristic?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-24T06:45:55Z","receivedAt":"2020-09-24T06:46:00Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ryan Zoeller <rtzoeller@rtzoeller.com> writes:\n\n> 1. Is this indentation-aware function identification useful, and\n>     generally worth pursuing further?\n\nI cannot shake the feeling off that this is being overly generous\nand inviting for misidentification for languages whose usual\nconvention is not to nest and indent the definitions freely.\n\nIOW, why can't the \"we allow leading whitespaces\" a per-language\nthing?  IOW, why do we even need any new code---shouldn't it be just\nthe matter of defining xfuncname patterns for such a language with\nnested and indented definitions?\n\nSo, a mild Meh from me at this point.  I may change my mind in the\nmorning, though ;-)\n\nThanks.\n"},{"id":"406284","messageId":"20200924211725.GA3103003@coredump.intra.peff.net","threadId":"54284","inReplyTo":"xmqqzh5fhduk.fsf@gitster.c.googlers.com","subject":"Re: [RFC 0/1] Leading whitespace as a function identification heuristic?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-24T21:17:25Z","receivedAt":"2020-09-24T21:17:28Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 23, 2020 at 11:45:55PM -0700, Junio C Hamano wrote:\n\n> Ryan Zoeller <rtzoeller@rtzoeller.com> writes:\n> \n> > 1. Is this indentation-aware function identification useful, and\n> >     generally worth pursuing further?\n> \n> I cannot shake the feeling off that this is being overly generous\n> and inviting for misidentification for languages whose usual\n> convention is not to nest and indent the definitions freely.\n> \n> IOW, why can't the \"we allow leading whitespaces\" a per-language\n> thing?  IOW, why do we even need any new code---shouldn't it be just\n> the matter of defining xfuncname patterns for such a language with\n> nested and indented definitions?\n\nIf I understand the problem correctly, I don't think a static regex can\naccomplish this, because you need context from the original line. E.g.,\nconsider something like this:\n\n  void foo() {\n\tvoid bar() {\n\t\t...code 1...\n\t}\n\t...code 2...\n  }\n\nIf we change one of the lines of code, then we find the function header\nby walking backwards up the lines, evaluating a regex for each line. But\nfor \"code 2\", how do we know to keep walking past bar() and up to foo()?\nOr more specifically, what is different when evaluating a change from\n\"code 2\" that is different than when we would evaluate \"code 1\"?\n\nIf the only input to the question of \"is this line a function header\" is\nthe regex from the config, then changes to either line of code must\nproduce the same answer (either bar() if we allow leading whitespace, or\nfoo() if we do not).\n\nSo I think Ryan's proposal is to give that search an extra piece of\ninformation: the indentation of the original changed line. Which is\nenough to differentiate the two cases.\n\nIf I understand the patch correctly, it is always picking the first line\nwhere indentation is lessened (and which matches the funcname pattern).\nThat works out of the box with existing funcname patterns, which is\nnice. Though I wonder if there are cases where the funcname regex could\nuse the information more flexibly (i.e., some marker in the regex that\nmeans \"match less than this many spaces\" or something).\n\nI do agree that this should not be on automatically for all languages.\nSome file formats may want to show a header that's at the same\nindentation as the change. Adding a diff.foo.funcnameIndentation config\noption would be one solution. But requiring the funcname pattern to make\nexplicit use of the information is another (and would allow a format to\nmatch some things at one indentation level and some at another; but\nagain, I'm hand-waving a bit on whether this level of flexibility is\neven useful).\n\n-Peff\n"},{"id":"406287","messageId":"c759b472-c889-ef85-bcf2-6d9cbc588b51@rtzoeller.com","threadId":"54284","inReplyTo":"20200924211725.GA3103003@coredump.intra.peff.net","subject":"Re: [RFC 0/1] Leading whitespace as a function identification heuristic?","fromName":"Ryan Zoeller","fromEmail":"rtzoeller@rtzoeller.com","sentAt":"2020-09-24T22:01:48Z","receivedAt":"2020-09-24T22:01:57Z","isPatch":false,"sender":{"key":"rtzoeller@rtzoeller.com","avatar":"https://avatars.githubusercontent.com/u/573497?v=4"},"body":"On 9/24/20 4:17 PM, Jeff King wrote:\n> \n> \n> If I understand the problem correctly, I don't think a static regex can\n> accomplish this, because you need context from the original line. E.g.,\n> consider something like this:\n> \n>    void foo() {\n> \tvoid bar() {\n> \t\t...code 1...\n> \t}\n> \t...code 2...\n>    }\n> \n> If we change one of the lines of code, then we find the function header\n> by walking backwards up the lines, evaluating a regex for each line. But\n> for \"code 2\", how do we know to keep walking past bar() and up to foo()?\n> Or more specifically, what is different when evaluating a change from\n> \"code 2\" that is different than when we would evaluate \"code 1\"?\n> \n> If the only input to the question of \"is this line a function header\" is\n> the regex from the config, then changes to either line of code must\n> produce the same answer (either bar() if we allow leading whitespace, or\n> foo() if we do not).\n> \n> So I think Ryan's proposal is to give that search an extra piece of\n> information: the indentation of the original changed line. Which is\n> enough to differentiate the two cases.\n\nYou've explained this better than I could have. Thanks.\n\n> If I understand the patch correctly, it is always picking the first line\n> where indentation is lessened (and which matches the funcname pattern).\n> That works out of the box with existing funcname patterns, which is\n> nice. Though I wonder if there are cases where the funcname regex could\n> use the information more flexibly (i.e., some marker in the regex that\n> means \"match less than this many spaces\" or something).\n\nMy original intent was to work with existing funcname expressions \nwithout modifications. Some of the funcname regexes are rather \nimpenetrable at first glance, and not requiring modifications seemed \nlike an easy win.\n\nEspecially for funcname patterns specified by a user, I assumed it\nwould be easier to set an additional configuration option than\nrewrite an existing regex to handle this complexity.\n\n> I do agree that this should not be on automatically for all languages.\n> Some file formats may want to show a header that's at the same\n> indentation as the change. Adding a diff.foo.funcnameIndentation config\n> option would be one solution. But requiring the funcname pattern to make\n> explicit use of the information is another (and would allow a format to\n> match some things at one indentation level and some at another; but\n> again, I'm hand-waving a bit on whether this level of flexibility is\n> even useful)\nIf the configuration option is implemented correctly (i.e. as an enum \nrather than a binary toggle), I think we could leave the door open for\na more flexible approach in the future, without needing to answer how \nuseful that flexibility is now. I couldn't think of any situations \nrequiring this flexibility, but that doesn't mean they don't exist.\n\nRyan\n\n"},{"id":"406318","messageId":"1c03faa0-011c-39c2-acb6-d09a5fcfc818@gmail.com","threadId":"54284","inReplyTo":"c759b472-c889-ef85-bcf2-6d9cbc588b51@rtzoeller.com","subject":"Re: [RFC 0/1] Leading whitespace as a function identification heuristic?","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-09-25T09:11:33Z","receivedAt":"2020-09-25T09:11:39Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 24/09/2020 23:01, Ryan Zoeller wrote:\n> On 9/24/20 4:17 PM, Jeff King wrote:\n>>\n>>\n>> If I understand the problem correctly, I don't think a static regex can\n>> accomplish this, because you need context from the original line. E.g.,\n>> consider something like this:\n>>\n>>     void foo() {\n>> \tvoid bar() {\n>> \t\t...code 1...\n>> \t}\n>> \t...code 2...\n>>     }\n>>\n\nIf I've understood correctly when ...code 2... contains changes that are \nthemselves indented then we'll pick the wrong function header for example\n\n\tvoid foo() {\n\t\tvoid bar() {\n\t\t\t...code 1...\n\t\t}\n\t\tfor (...) {\n\t\t\t// if this line is changed we pick bar rather\n\t\t\t// than foo because it is the first function\n\t\t\t// header with a smaller indentation than the\n\t\t\t// first changed line.\n\t\t}\n\t}\n\nUnfortunately I suspect code like that is not uncommon and the diff \nwould regress with this simple heuristic. It might be possible to \nrecalculate the required indentation as we walk backwards up the file \nthough, so when we hit the \"for\" line we reduce the maximum indentation \nallowed for a match and so skip \"bar\" as a function header.\n\nBest Wishes\n\nPhillip\n\n>> If we change one of the lines of code, then we find the function header\n>> by walking backwards up the lines, evaluating a regex for each line. But\n>> for \"code 2\", how do we know to keep walking past bar() and up to foo()?\n>> Or more specifically, what is different when evaluating a change from\n>> \"code 2\" that is different than when we would evaluate \"code 1\"?\n>>\n>> If the only input to the question of \"is this line a function header\" is\n>> the regex from the config, then changes to either line of code must\n>> produce the same answer (either bar() if we allow leading whitespace, or\n>> foo() if we do not).\n>>\n>> So I think Ryan's proposal is to give that search an extra piece of\n>> information: the indentation of the original changed line. Which is\n>> enough to differentiate the two cases.\n> \n> You've explained this better than I could have. Thanks.\n> \n>> If I understand the patch correctly, it is always picking the first line\n>> where indentation is lessened (and which matches the funcname pattern).\n>> That works out of the box with existing funcname patterns, which is\n>> nice. Though I wonder if there are cases where the funcname regex could\n>> use the information more flexibly (i.e., some marker in the regex that\n>> means \"match less than this many spaces\" or something).\n> \n> My original intent was to work with existing funcname expressions\n> without modifications. Some of the funcname regexes are rather\n> impenetrable at first glance, and not requiring modifications seemed\n> like an easy win.\n> \n> Especially for funcname patterns specified by a user, I assumed it\n> would be easier to set an additional configuration option than\n> rewrite an existing regex to handle this complexity.\n> \n>> I do agree that this should not be on automatically for all languages.\n>> Some file formats may want to show a header that's at the same\n>> indentation as the change. Adding a diff.foo.funcnameIndentation config\n>> option would be one solution. But requiring the funcname pattern to make\n>> explicit use of the information is another (and would allow a format to\n>> match some things at one indentation level and some at another; but\n>> again, I'm hand-waving a bit on whether this level of flexibility is\n>> even useful)\n> If the configuration option is implemented correctly (i.e. as an enum\n> rather than a binary toggle), I think we could leave the door open for\n> a more flexible approach in the future, without needing to answer how\n> useful that flexibility is now. I couldn't think of any situations\n> requiring this flexibility, but that doesn't mean they don't exist.\n> \n> Ryan\n> \n"},{"id":"406373","messageId":"f0fab667-fb72-b15d-258b-aa437235bba4@kdbg.org","threadId":"54284","inReplyTo":"20200923215859.102981-1-rtzoeller@rtzoeller.com","subject":"Re: [RFC 0/1] Leading whitespace as a function identification heuristic?","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2020-09-25T18:12:12Z","receivedAt":"2020-09-25T18:12:16Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 23.09.20 um 23:59 schrieb Ryan Zoeller:\n> I've recently been annoyed by git's choice of lines when invoking\n> `git diff --function-context`. git isn't doing a _bad_ job per se,\n> given that it's using regular expressions to parse nonregular languages,\n> but in my opinion it could do better.\n> \n> The issue I'm seeing is that constructs like nested functions cause\n> `git diff --function-context` to stop taking lines prematurely; a line\n> containing a function declaration has been reached, but it's not the one\n> I'm expecting git to stop at. This results in some of the function being\n> omitted from the output of `git diff --function-context`, and also\n> manifests itself as the incorrect function being listed in the hunk header\n> in general.\n\nThere are two mechanisms at play here:\n\nThe first is that the context above and below a point of interest is\nextended to the next function. This is relevant for both `git diff` and\n`git grep` when --function-context is specified.\n\nThe second is the determination of the hunk header. It is only relevant\nfor `git diff`.\n\nThe purpose of the hunk header is to get an estimation of where the hunk\nis located. It is most helpful in this case when the closest anchor\npoint is listed; it is not important to find where the semantically\nsurrounding nesting context begins.\n\nTherefore, I suggest that you aim only for the --function-context case,\nbecause there it makes sense to find the correctly nested surrounding\ncontext if possible.\n\n-- Hannes\n"},{"id":"406379","messageId":"20200925184319.GA660343@coredump.intra.peff.net","threadId":"54284","inReplyTo":"1c03faa0-011c-39c2-acb6-d09a5fcfc818@gmail.com","subject":"Re: [RFC 0/1] Leading whitespace as a function identification heuristic?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-25T18:43:19Z","receivedAt":"2020-09-25T18:43:22Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 25, 2020 at 10:11:33AM +0100, Phillip Wood wrote:\n\n> If I've understood correctly when ...code 2... contains changes that are\n> themselves indented then we'll pick the wrong function header for example\n> \n> \tvoid foo() {\n> \t\tvoid bar() {\n> \t\t\t...code 1...\n> \t\t}\n> \t\tfor (...) {\n> \t\t\t// if this line is changed we pick bar rather\n> \t\t\t// than foo because it is the first function\n> \t\t\t// header with a smaller indentation than the\n> \t\t\t// first changed line.\n> \t\t}\n> \t}\n> \n> Unfortunately I suspect code like that is not uncommon and the diff would\n> regress with this simple heuristic. It might be possible to recalculate the\n> required indentation as we walk backwards up the file though, so when we hit\n> the \"for\" line we reduce the maximum indentation allowed for a match and so\n> skip \"bar\" as a function header.\n\nThanks, that's a great counter-example I hadn't considered.\n\nYes, I agree that adjusting the desired indentation as we walk backwards\nwould work. That's assuming indentation is hierarchical, but I think\nthat's implied by the existence of this feature at all.\n\nAnother possible corner case: tabs vs spaces. If I have:\n\n  <space><space><space><space><space><space><space><space>foo\n  <tab><tab>bar\n\nwhich is more indented? Counting isspace(), it is the first one. But\nvisually, it would _usually_ be the second one. But of course it would\ndepend on your tabstops.\n\nThe above example is obviously stupid and contrived, but I wonder if\nthere are legitimate confusing cases where people mix tabs and spaces\n(e.g., mixed tabs and spaces to align function parameters, etc).\n\n-Peff\n"},{"id":"406382","messageId":"005de229-ad05-437c-7b83-8f6502b4e27d@gmail.com","threadId":"54284","inReplyTo":"20200925184319.GA660343@coredump.intra.peff.net","subject":"Re: [RFC 0/1] Leading whitespace as a function identification heuristic?","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-09-25T19:01:07Z","receivedAt":"2020-09-25T19:01:14Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Peff\n\nOn 25/09/2020 19:43, Jeff King wrote:\n> On Fri, Sep 25, 2020 at 10:11:33AM +0100, Phillip Wood wrote:\n> \n>> If I've understood correctly when ...code 2... contains changes that are\n>> themselves indented then we'll pick the wrong function header for example\n>>\n>> \tvoid foo() {\n>> \t\tvoid bar() {\n>> \t\t\t...code 1...\n>> \t\t}\n>> \t\tfor (...) {\n>> \t\t\t// if this line is changed we pick bar rather\n>> \t\t\t// than foo because it is the first function\n>> \t\t\t// header with a smaller indentation than the\n>> \t\t\t// first changed line.\n>> \t\t}\n>> \t}\n>>\n>> Unfortunately I suspect code like that is not uncommon and the diff would\n>> regress with this simple heuristic. It might be possible to recalculate the\n>> required indentation as we walk backwards up the file though, so when we hit\n>> the \"for\" line we reduce the maximum indentation allowed for a match and so\n>> skip \"bar\" as a function header.\n> \n> Thanks, that's a great counter-example I hadn't considered.\n> \n> Yes, I agree that adjusting the desired indentation as we walk backwards\n> would work. That's assuming indentation is hierarchical, but I think\n> that's implied by the existence of this feature at all.\n> \n> Another possible corner case: tabs vs spaces. If I have:\n> \n>    <space><space><space><space><space><space><space><space>foo\n>    <tab><tab>bar\n> \n> which is more indented? Counting isspace(), it is the first one. But\n> visually, it would _usually_ be the second one. But of course it would\n> depend on your tabstops.\n> \n> The above example is obviously stupid and contrived, but I wonder if\n> there are legitimate confusing cases where people mix tabs and spaces\n> (e.g., mixed tabs and spaces to align function parameters, etc).\n\nTo calculate the indentation for diff \n--color-moved-ws=allow-indentation-change in fill_es_indent_data() we \nuse the tabwidth whitespace attribute to calculate the width of a tab in \nspaces\n\n\tint tab_width = es->flags & WS_TAB_WIDTH_MASK;\n\t/* calculate the visual width of indentation */\n\twhile(1) {\n\t\tif (s[off] == ' ') {\n\t\t\twidth++;\n\t\t\toff++;\n\t\t} else if (s[off] == '\\t') {\n\t\t\twidth += tab_width - (width % tab_width);\n\t\t\twhile (s[++off] == '\\t')\n\t\t\t\twidth += tab_width;\n\t\t} else {\n\t\t\tbreak;\n\t\t}\n\t}\n\nI think we could probably do something similar here assuming it is the \nvisual width of the indentation that we care about.\n\nBest Wishes\n\nPhillip\n\n> -Peff\n> \n"},{"id":"406388","messageId":"20200925190501.GA668512@coredump.intra.peff.net","threadId":"54284","inReplyTo":"005de229-ad05-437c-7b83-8f6502b4e27d@gmail.com","subject":"Re: [RFC 0/1] Leading whitespace as a function identification heuristic?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-25T19:05:01Z","receivedAt":"2020-09-25T20:36:29Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 25, 2020 at 08:01:07PM +0100, Phillip Wood wrote:\n\n> > Another possible corner case: tabs vs spaces. If I have:\n> > \n> >    <space><space><space><space><space><space><space><space>foo\n> >    <tab><tab>bar\n> > \n> > which is more indented? Counting isspace(), it is the first one. But\n> > visually, it would _usually_ be the second one. But of course it would\n> > depend on your tabstops.\n> > \n> > The above example is obviously stupid and contrived, but I wonder if\n> > there are legitimate confusing cases where people mix tabs and spaces\n> > (e.g., mixed tabs and spaces to align function parameters, etc).\n> \n> To calculate the indentation for diff\n> --color-moved-ws=allow-indentation-change in fill_es_indent_data() we use\n> the tabwidth whitespace attribute to calculate the width of a tab in spaces\n> [...]\n\nAh, right, I forgot we already had \"tabwidth\" config to deal with this.\nI agree we could make use of that same technique here.\n\n-Peff\n"}]}