Re: [PATCH] xdiff: Show function names in hunk headers.

2 messages, 2 authors, 2016-06-15 · open the first message on its own page

Re: [PATCH] xdiff: Show function names in hunk headers.

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:42:22

Mark Wooding [off-list ref] writes:
The function names are parsed by a particularly stupid algorithm at the
moment: it just tries to find a line in the `old' file, from before the
start of the hunk, whose first character looks plausible.  Still, it's
most definitely a start.
+		    (isalpha((unsigned char)*rec) || /* identifier? */
+		     *rec == '_' ||	/* also identifier? */
+		     *rec == '(' ||	/* lisp defun? */
+		     *rec == '#')) {	/* #define? */
GNU diff -p does "^[[:alpha:]$_]"; personally I think any line
that does not begin with a whitespace is good enough.  In either
way, your patch is good.  Thanks.

Re: [PATCH] xdiff: Show function names in hunk headers.

From: Mark Wooding <hidden>
Date: 2016-06-15 22:42:22

Junio C Hamano [off-list ref] wrote:
GNU diff -p does "^[[:alpha:]$_]"; personally I think any line
that does not begin with a whitespace is good enough.
Hmm.  I think my approach is wrong.  I've noticed that targets of the
form `$(FOO): ...' in Makefiles would make nice hunk headers, but my
current hack won't notice them.  Without a shift of approach, I think
I run the risk of deluging the list with little fixes to this bit of
code, which sounds like a pile of no fun.

So, I have two main suggestions.  The first is /very/ stupid, and just
asks for two non-whitespace characters at the start of a line.
diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index ad5bfb1..822f991 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c
@@ -83,11 +83,9 @@ static void xdl_find_func(xdfile_t *xf, 
        *ll = 0;
        while (i-- > 0) {
                len = xdl_get_rec(xf, i, &rec);
-               if (len > 0 &&
-                   (isalpha((unsigned char)*rec) || /* identifier? */
-                    *rec == '_' ||     /* also identifier? */
-                    *rec == '(' ||     /* lisp defun? */
-                    *rec == '#')) {    /* #define? */
+               if (len >= 2 &&
+                   !isspace((unsigned char)rec[0]) &&
+                   !isspace((unsigned char)rec[1])) {
                        if (len > sz)
                                len = sz;
                        if (len && rec[len - 1] == '\n')
The second suggestion is slightly refined, but a little more
complicated.  We ask for a line which starts /either/ with two
non-whitespace characters, or with an alphanumeric.  Why?  Because text
documents have a tendency to have headings of the form `7 Heading!' and
I want to catch them.
diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index ad5bfb1..bcb3e47 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c
@@ -83,11 +83,10 @@ static void xdl_find_func(xdfile_t *xf, 
        *ll = 0;
        while (i-- > 0) {
                len = xdl_get_rec(xf, i, &rec);
-               if (len > 0 &&
-                   (isalpha((unsigned char)*rec) || /* identifier? */
-                    *rec == '_' ||     /* also identifier? */
-                    *rec == '(' ||     /* lisp defun? */
-                    *rec == '#')) {    /* #define? */
+               if (len && !isspace((unsigned char)*rec) &&
+                   ((len >= 2 && !isspace((unsigned char)rec[1])) ||
+                    isalnum((unsigned char)*rec) ||
+                    *rec == '_')) {
                        if (len > sz)
                                len = sz;
                        if (len && rec[len - 1] == '\n')
Another possibility I just thought of: insist that the line starts with
a non-space, and contains another non-space somewhere.  This will get
caught out by `{       /* ... rest of comment */', which I've seen a few
places, though.
diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index ad5bfb1..81b38ce 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c
@@ -79,15 +79,18 @@ static void xdl_find_func(xdfile_t *xf, 
 
        const char *rec;
        long len;
+       long j;
 
        *ll = 0;
        while (i-- > 0) {
                len = xdl_get_rec(xf, i, &rec);
-               if (len > 0 &&
-                   (isalpha((unsigned char)*rec) || /* identifier? */
-                    *rec == '_' ||     /* also identifier? */
-                    *rec == '(' ||     /* lisp defun? */
-                    *rec == '#')) {    /* #define? */
+               if (len && !isspace((unsigned char)*rec)) {
+                       for (j = 1; j < len; j++) {
+                               if (!isspace((unsigned char)rec[j]))
+                                       goto good;
+                       }
+                       continue;
+               good:
                        if (len > sz)
                                len = sz;
                        if (len && rec[len - 1] == '\n')
I think I like option 2 best, as a nice compromise between stupidity and
actually working.  Opinions, anyone?

-- [mdw]
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help