{"thread":{"id":"3741","subject":"[PATCH] xdiff: Show function names in hunk headers.","startedAt":"2006-03-28T02:23:31Z","lastAt":"2006-03-29T09:57:47Z","messageCount":8,"participants":["Mark Wooding","Junio C Hamano","Andreas Ericsson"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"18053","messageId":"11435126113456-git-send-email-mdw@distorted.org.uk","threadId":"3741","inReplyTo":null,"subject":"[PATCH] xdiff: Show function names in hunk headers.","fromName":"Mark Wooding","fromEmail":"mdw@distorted.org.uk","sentAt":"2006-03-28T02:23:31Z","receivedAt":"2006-03-28T02:23:31Z","isPatch":true,"sender":{"key":"mdw@distorted.org.uk","avatar":null},"body":"The speed of the built-in diff generator is nice; but the function names\nshown by `diff -p' are /really/ nice.  And I hate having to choose.  So,\nwe hack xdiff to find the function names and print them.\n\nxdiff has grown a flag to say whether to dig up the function names.  The\nbuiltin_diff function passes this flag unconditionally.  I suppose it\ncould parse GIT_DIFF_OPTS, but it doesn't at the moment.  I've also\nreintroduced the `function name' into the test suite, from which it was\nremoved in commit 3ce8f089.\n\nThe function names are parsed by a particularly stupid algorithm at the\nmoment: it just tries to find a line in the `old' file, from before the\nstart of the hunk, whose first character looks plausible.  Still, it's\nmost definitely a start.\n\nSigned-off-by: Mark Wooding <mdw@distorted.org.uk>\n\n---\n\n diff.c                 |    1 +\n t/t4001-diff-rename.sh |    2 +-\n xdiff/xdiff.h          |    3 +++\n xdiff/xemit.c          |   41 ++++++++++++++++++++++++++++++++++++++++-\n xdiff/xinclude.h       |    1 +\n xdiff/xutils.c         |   15 ++++++++++++---\n xdiff/xutils.h         |    3 ++-\n 7 files changed, 60 insertions(+), 6 deletions(-)\n\n746418a20769c003886d7f4bbec6563af7aabd4b\ndiff --git a/diff.c b/diff.c\nindex 5eae094..8b37477 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -267,6 +267,7 @@ static void builtin_diff(const char *nam\n \t\tecbdata.label_path = lbl;\n \t\txpp.flags = XDF_NEED_MINIMAL;\n \t\txecfg.ctxlen = 3;\n+\t\txecfg.flags = XDL_EMIT_FUNCNAMES;\n \t\tif (!diffopts)\n \t\t\t;\n \t\telse if (!strncmp(diffopts, \"--unified=\", 10))\ndiff --git a/t/t4001-diff-rename.sh b/t/t4001-diff-rename.sh\nindex 08c1131..2e3c20d 100755\n--- a/t/t4001-diff-rename.sh\n+++ b/t/t4001-diff-rename.sh\n@@ -49,7 +49,7 @@ rename from path0\n rename to path1\n --- a/path0\n +++ b/path1\n-@@ -8,7 +8,7 @@\n+@@ -8,7 +8,7 @@ Line 7\n  Line 8\n  Line 9\n  Line 10\ndiff --git a/xdiff/xdiff.h b/xdiff/xdiff.h\nindex 71cb939..2540e8a 100644\n--- a/xdiff/xdiff.h\n+++ b/xdiff/xdiff.h\n@@ -35,6 +35,8 @@ #define XDL_PATCH_REVERSE '+'\n #define XDL_PATCH_MODEMASK ((1 << 8) - 1)\n #define XDL_PATCH_IGNOREBSPACE (1 << 8)\n \n+#define XDL_EMIT_FUNCNAMES (1 << 0)\n+\n #define XDL_MMB_READONLY (1 << 0)\n \n #define XDL_MMF_ATOMIC (1 << 0)\n@@ -65,6 +67,7 @@ typedef struct s_xdemitcb {\n \n typedef struct s_xdemitconf {\n \tlong ctxlen;\n+\tunsigned long flags;\n } xdemitconf_t;\n \n typedef struct s_bdiffparam {\ndiff --git a/xdiff/xemit.c b/xdiff/xemit.c\nindex 2e5d54c..ad5bfb1 100644\n--- a/xdiff/xemit.c\n+++ b/xdiff/xemit.c\n@@ -69,10 +69,43 @@ static xdchange_t *xdl_get_hunk(xdchange\n }\n \n \n+static void xdl_find_func(xdfile_t *xf, long i, char *buf, long sz, long *ll) {\n+\n+\t/*\n+\t * Be quite stupid about this for now.  Find a line in the old file\n+\t * before the start of the hunk (and context) which starts with a\n+\t * plausible character.\n+\t */\n+\n+\tconst char *rec;\n+\tlong len;\n+\n+\t*ll = 0;\n+\twhile (i-- > 0) {\n+\t\tlen = xdl_get_rec(xf, i, &rec);\n+\t\tif (len > 0 &&\n+\t\t    (isalpha((unsigned char)*rec) || /* identifier? */\n+\t\t     *rec == '_' ||\t/* also identifier? */\n+\t\t     *rec == '(' ||\t/* lisp defun? */\n+\t\t     *rec == '#')) {\t/* #define? */\n+\t\t\tif (len > sz)\n+\t\t\t\tlen = sz;\n+\t\t\tif (len && rec[len - 1] == '\\n')\n+\t\t\t\tlen--;\n+\t\t\tmemcpy(buf, rec, len);\n+\t\t\t*ll = len;\n+\t\t\treturn;\n+\t\t}\n+\t}\n+}\n+\n+\n int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,\n \t\t  xdemitconf_t const *xecfg) {\n \tlong s1, s2, e1, e2, lctx;\n \txdchange_t *xch, *xche;\n+\tchar funcbuf[40];\n+\tlong funclen = 0;\n \n \tfor (xch = xche = xscr; xch; xch = xche->next) {\n \t\txche = xdl_get_hunk(xch, xecfg);\n@@ -90,7 +123,13 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange\n \t\t/*\n \t\t * Emit current hunk header.\n \t\t */\n-\t\tif (xdl_emit_hunk_hdr(s1 + 1, e1 - s1, s2 + 1, e2 - s2, ecb) < 0)\n+\n+\t\tif (xecfg->flags & XDL_EMIT_FUNCNAMES) {\n+\t\t\txdl_find_func(&xe->xdf1, s1, funcbuf,\n+\t\t\t\t      sizeof(funcbuf), &funclen);\n+\t\t}\n+\t\tif (xdl_emit_hunk_hdr(s1 + 1, e1 - s1, s2 + 1, e2 - s2,\n+\t\t\t\t      funcbuf, funclen, ecb) < 0)\n \t\t\treturn -1;\n \n \t\t/*\ndiff --git a/xdiff/xinclude.h b/xdiff/xinclude.h\nindex 9490fc5..04a9da8 100644\n--- a/xdiff/xinclude.h\n+++ b/xdiff/xinclude.h\n@@ -23,6 +23,7 @@\n #if !defined(XINCLUDE_H)\n #define XINCLUDE_H\n \n+#include <ctype.h>\n #include <stdio.h>\n #include <stdlib.h>\n #include <unistd.h>\ndiff --git a/xdiff/xutils.c b/xdiff/xutils.c\nindex 8221806..afaada1 100644\n--- a/xdiff/xutils.c\n+++ b/xdiff/xutils.c\n@@ -235,7 +235,8 @@ long xdl_atol(char const *str, char cons\n }\n \n \n-int xdl_emit_hunk_hdr(long s1, long c1, long s2, long c2, xdemitcb_t *ecb) {\n+int xdl_emit_hunk_hdr(long s1, long c1, long s2, long c2,\n+\t\t      const char *func, long funclen, xdemitcb_t *ecb) {\n \tint nb = 0;\n \tmmbuffer_t mb;\n \tchar buf[128];\n@@ -264,8 +265,16 @@ int xdl_emit_hunk_hdr(long s1, long c1, \n \t\tnb += xdl_num_out(buf + nb, c2);\n \t}\n \n-\tmemcpy(buf + nb, \" @@\\n\", 4);\n-\tnb += 4;\n+\tmemcpy(buf + nb, \" @@\", 3);\n+\tnb += 3;\n+\tif (func && funclen) {\n+\t\tbuf[nb++] = ' ';\n+\t\tif (funclen > sizeof(buf) - nb - 1)\n+\t\t\tfunclen = sizeof(buf) - nb - 1;\n+\t\tmemcpy(buf + nb, func, funclen);\n+\t\tnb += funclen;\n+\t}\n+\tbuf[nb++] = '\\n';\n \n \tmb.ptr = buf;\n \tmb.size = nb;\ndiff --git a/xdiff/xutils.h b/xdiff/xutils.h\nindex 428a4bb..55b0d39 100644\n--- a/xdiff/xutils.h\n+++ b/xdiff/xutils.h\n@@ -36,7 +36,8 @@ unsigned long xdl_hash_record(char const\n unsigned int xdl_hashbits(unsigned int size);\n int xdl_num_out(char *out, long val);\n long xdl_atol(char const *str, char const **next);\n-int xdl_emit_hunk_hdr(long s1, long c1, long s2, long c2, xdemitcb_t *ecb);\n+int xdl_emit_hunk_hdr(long s1, long c1, long s2, long c2,\n+\t\t      const char *func, long funclen, xdemitcb_t *ecb);\n \n \n \n-- \n1.3.0.rc1.g7464\n"},{"id":"18061","messageId":"7vfyl3m7vy.fsf@assigned-by-dhcp.cox.net","threadId":"3741","inReplyTo":"11435126113456-git-send-email-mdw@distorted.org.uk","subject":"Re: [PATCH] xdiff: Show function names in hunk headers.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-03-28T05:54:25Z","receivedAt":"2006-03-28T05:54:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mark Wooding <mdw@distorted.org.uk> writes:\n\n> The function names are parsed by a particularly stupid algorithm at the\n> moment: it just tries to find a line in the `old' file, from before the\n> start of the hunk, whose first character looks plausible.  Still, it's\n> most definitely a start.\n\n> +\t\t    (isalpha((unsigned char)*rec) || /* identifier? */\n> +\t\t     *rec == '_' ||\t/* also identifier? */\n> +\t\t     *rec == '(' ||\t/* lisp defun? */\n> +\t\t     *rec == '#')) {\t/* #define? */\n\nGNU diff -p does \"^[[:alpha:]$_]\"; personally I think any line\nthat does not begin with a whitespace is good enough.  In either\nway, your patch is good.  Thanks.\n"},{"id":"18066","messageId":"7vvetykiel.fsf@assigned-by-dhcp.cox.net","threadId":"3741","inReplyTo":"7vfyl3m7vy.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] xdiff: Show function names in hunk headers.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-03-28T09:50:10Z","receivedAt":"2006-03-28T09:50:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <junkio@cox.net> writes:\n\n> Mark Wooding <mdw@distorted.org.uk> writes:\n>...\n>> +\t\t    (isalpha((unsigned char)*rec) || /* identifier? */\n>> +\t\t     *rec == '_' ||\t/* also identifier? */\n>> +\t\t     *rec == '(' ||\t/* lisp defun? */\n>> +\t\t     *rec == '#')) {\t/* #define? */\n>\n> GNU diff -p does \"^[[:alpha:]$_]\"; personally I think any line\n> that does not begin with a whitespace is good enough.\n\nObviously I was not thinking.  That should at least be \"any line\nthat begins with a non-whitespace and has a few characters\", to\nomit \"{\\n\" and catch \"int main()\\n\" in:\n\n\tint main()\n        {\n        \tprintf(\"Hello, world.\\n\");\n        }\n\n;-).\n"},{"id":"18067","messageId":"slrne2i31p.s3g.mdw@metalzone.distorted.org.uk","threadId":"3741","inReplyTo":"7vvetykiel.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] xdiff: Show function names in hunk headers.","fromName":"Mark Wooding","fromEmail":"mdw@distorted.org.uk","sentAt":"2006-03-28T10:13:13Z","receivedAt":"2006-03-28T10:13:13Z","isPatch":true,"sender":{"key":"mdw@distorted.org.uk","avatar":null},"body":"Junio C Hamano <junkio@cox.net> wrote:\n\n> Obviously I was not thinking.  That should at least be \"any line\n> that begins with a non-whitespace and has a few characters\", to\n> omit \"{\\n\" and catch \"int main()\\n\" in:\n\nHeh!  I already got that one wrong last night.  Hence my more\ncomplicated version. ;-)\n\n-- [mdw]\n"},{"id":"18068","messageId":"442918D4.6070105@op5.se","threadId":"3741","inReplyTo":"11435126113456-git-send-email-mdw@distorted.org.uk","subject":"Re: [PATCH] xdiff: Show function names in hunk headers.","fromName":"Andreas Ericsson","fromEmail":"ae@op5.se","sentAt":"2006-03-28T11:07:00Z","receivedAt":"2006-03-28T11:07:00Z","isPatch":true,"sender":{"key":"ae@op5.se","avatar":"https://gravatar.com/avatar/426e89595c75a8f5252dd0c989e5fabe5bcac616e68557427ad9aef6b0ca342a?d=mp&s=160"},"body":"Mark Wooding wrote:\n> \n> The function names are parsed by a particularly stupid algorithm at the\n> moment: it just tries to find a line in the `old' file, from before the\n> start of the hunk, whose first character looks plausible.  Still, it's\n> most definitely a start.\n> \n\nStupid is sometimes good. I've noticed that the gnu diff algorithm \nsometimes won't notice changes in small structs, though they are large \nenough for the surrounding context in the unified diff not to show it \neither.\n\n-- \nAndreas Ericsson                   andreas.ericsson@op5.se\nOP5 AB                             www.op5.se\nTel: +46 8-230225                  Fax: +46 8-230231\n"},{"id":"18074","messageId":"slrne2ik1i.s3g.mdw@metalzone.distorted.org.uk","threadId":"3741","inReplyTo":"7vfyl3m7vy.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] xdiff: Show function names in hunk headers.","fromName":"Mark Wooding","fromEmail":"mdw@distorted.org.uk","sentAt":"2006-03-28T15:03:14Z","receivedAt":"2006-03-28T15:03:14Z","isPatch":true,"sender":{"key":"mdw@distorted.org.uk","avatar":null},"body":"Junio C Hamano <junkio@cox.net> wrote:\n\n> GNU diff -p does \"^[[:alpha:]$_]\"; personally I think any line\n> that does not begin with a whitespace is good enough.\n\nHmm.  I think my approach is wrong.  I've noticed that targets of the\nform `$(FOO): ...' in Makefiles would make nice hunk headers, but my\ncurrent hack won't notice them.  Without a shift of approach, I think\nI run the risk of deluging the list with little fixes to this bit of\ncode, which sounds like a pile of no fun.\n\nSo, I have two main suggestions.  The first is /very/ stupid, and just\nasks for two non-whitespace characters at the start of a line.\n\ndiff --git a/xdiff/xemit.c b/xdiff/xemit.c\nindex ad5bfb1..822f991 100644\n--- a/xdiff/xemit.c\n+++ b/xdiff/xemit.c\n@@ -83,11 +83,9 @@ static void xdl_find_func(xdfile_t *xf, \n        *ll = 0;\n        while (i-- > 0) {\n                len = xdl_get_rec(xf, i, &rec);\n-               if (len > 0 &&\n-                   (isalpha((unsigned char)*rec) || /* identifier? */\n-                    *rec == '_' ||     /* also identifier? */\n-                    *rec == '(' ||     /* lisp defun? */\n-                    *rec == '#')) {    /* #define? */\n+               if (len >= 2 &&\n+                   !isspace((unsigned char)rec[0]) &&\n+                   !isspace((unsigned char)rec[1])) {\n                        if (len > sz)\n                                len = sz;\n                        if (len && rec[len - 1] == '\\n')\n\nThe second suggestion is slightly refined, but a little more\ncomplicated.  We ask for a line which starts /either/ with two\nnon-whitespace characters, or with an alphanumeric.  Why?  Because text\ndocuments have a tendency to have headings of the form `7 Heading!' and\nI want to catch them.\n\ndiff --git a/xdiff/xemit.c b/xdiff/xemit.c\nindex ad5bfb1..bcb3e47 100644\n--- a/xdiff/xemit.c\n+++ b/xdiff/xemit.c\n@@ -83,11 +83,10 @@ static void xdl_find_func(xdfile_t *xf, \n        *ll = 0;\n        while (i-- > 0) {\n                len = xdl_get_rec(xf, i, &rec);\n-               if (len > 0 &&\n-                   (isalpha((unsigned char)*rec) || /* identifier? */\n-                    *rec == '_' ||     /* also identifier? */\n-                    *rec == '(' ||     /* lisp defun? */\n-                    *rec == '#')) {    /* #define? */\n+               if (len && !isspace((unsigned char)*rec) &&\n+                   ((len >= 2 && !isspace((unsigned char)rec[1])) ||\n+                    isalnum((unsigned char)*rec) ||\n+                    *rec == '_')) {\n                        if (len > sz)\n                                len = sz;\n                        if (len && rec[len - 1] == '\\n')\n\nAnother possibility I just thought of: insist that the line starts with\na non-space, and contains another non-space somewhere.  This will get\ncaught out by `{       /* ... rest of comment */', which I've seen a few\nplaces, though.\n\ndiff --git a/xdiff/xemit.c b/xdiff/xemit.c\nindex ad5bfb1..81b38ce 100644\n--- a/xdiff/xemit.c\n+++ b/xdiff/xemit.c\n@@ -79,15 +79,18 @@ static void xdl_find_func(xdfile_t *xf, \n \n        const char *rec;\n        long len;\n+       long j;\n \n        *ll = 0;\n        while (i-- > 0) {\n                len = xdl_get_rec(xf, i, &rec);\n-               if (len > 0 &&\n-                   (isalpha((unsigned char)*rec) || /* identifier? */\n-                    *rec == '_' ||     /* also identifier? */\n-                    *rec == '(' ||     /* lisp defun? */\n-                    *rec == '#')) {    /* #define? */\n+               if (len && !isspace((unsigned char)*rec)) {\n+                       for (j = 1; j < len; j++) {\n+                               if (!isspace((unsigned char)rec[j]))\n+                                       goto good;\n+                       }\n+                       continue;\n+               good:\n                        if (len > sz)\n                                len = sz;\n                        if (len && rec[len - 1] == '\\n')\n\nI think I like option 2 best, as a nice compromise between stupidity and\nactually working.  Opinions, anyone?\n\n-- [mdw]\n"},{"id":"18098","messageId":"7v4q1ihzio.fsf@assigned-by-dhcp.cox.net","threadId":"3741","inReplyTo":"slrne2ik1i.s3g.mdw@metalzone.distorted.org.uk","subject":"Re: [PATCH] xdiff: Show function names in hunk headers.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-03-29T00:21:03Z","receivedAt":"2006-03-29T00:21:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mark Wooding <mdw@distorted.org.uk> writes:\n\n> Junio C Hamano <junkio@cox.net> wrote:\n>\n>> GNU diff -p does \"^[[:alpha:]$_]\"; personally I think any line\n>> that does not begin with a whitespace is good enough.\n>...\n> The second suggestion is slightly refined, but a little more\n> complicated.  We ask for a line which starts /either/ with two\n> non-whitespace characters, or with an alphanumeric.  Why?  Because text\n> documents have a tendency to have headings of the form `7 Heading!' and\n> I want to catch them.\n\nAsciidoc?\n\n        . enumerated one\n          this is one item\n\n        . enumerated two\n          this is another item\n\n> I think I like option 2 best, as a nice compromise between stupidity and\n> actually working.  Opinions, anyone?\n\nIt's just a heuristic, so there are only two things we could\nsensibly do.  Either we keep it absolutely stupid to save our\ncode and sanity, or we give full configurability via -F regexp\nto the end users.\n\nI suspect feeping creaturism would eventually push us to go the\nlatter route, but for now I'd vote for doing exactly the same as\nwhat default GNU does, by looking at the first letter without\nusing regexps.  When we add regexps later, the users can\ncustomize the pattern to match the languages they use, and we\nmight end up having to have a set of (file-suffix -> default\nregexp) mappings, with full end user configurability via\n.git/config -- gaaah but true X-<.\n\n\t[diff]\n        \tfunctionline = \"^\\w\" for .c\n                functionline = \"^(?i)\\s*(?:function|procedure)\" for .f77\n                functionline = \"^\\(defun \" for .el\n                ...\n"},{"id":"18114","messageId":"slrne2kmgq.s3g.mdw@metalzone.distorted.org.uk","threadId":"3741","inReplyTo":"7v4q1ihzio.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] xdiff: Show function names in hunk headers.","fromName":"Mark Wooding","fromEmail":"mdw@distorted.org.uk","sentAt":"2006-03-29T09:57:47Z","receivedAt":"2006-03-29T09:57:47Z","isPatch":true,"sender":{"key":"mdw@distorted.org.uk","avatar":null},"body":"Junio C Hamano <junkio@cox.net> wrote:\n\n> Asciidoc?\n>\n>         . enumerated one\n>           this is one item\n\nI thought about this, and decided that missing these was a good thing,\nbecause item heads are probably too low-level.  I'd rather go for\nsection or subsection headings.\n\n> It's just a heuristic, so there are only two things we could\n> sensibly do.  Either we keep it absolutely stupid to save our\n> code and sanity, or we give full configurability via -F regexp\n> to the end users.\n\nI'm already thinking about that...\n\n-- [mdw]\n"}]}