threads / patch / 3741

patchxdiff: Show function names in hunk headers.

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

## tl;dr

8 messages between Mar 28, 2006 and Mar 29, 2006. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Mark Wooding· Mar 28, 2006, 02:23 UTC · lore

The speed of the built-in diff generator is nice; but the function names shown by `diff -p' are /really/ nice. And I hate having to choose. So, we hack xdiff to find the function names and print them.

xdiff has grown a flag to say whether to dig up the function names. The builtin_diff function passes this flag unconditionally. I suppose it could parse GIT_DIFF_OPTS, but it doesn't at the moment. I've also reintroduced the `function name' into the test suite, from which it was removed in commit 3ce8f089.

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.

Signed-off-by: Mark Wooding <mdw@distorted.org.uk>
---
 diff.c                 |    1 +
 t/t4001-diff-rename.sh |    2 +-
 xdiff/xdiff.h          |    3 +++
 xdiff/xemit.c          |   41 ++++++++++++++++++++++++++++++++++++++++-
 xdiff/xinclude.h       |    1 +
 xdiff/xutils.c         |   15 ++++++++++++---
 xdiff/xutils.h         |    3 ++-
 7 files changed, 60 insertions(+), 6 deletions(-)
746418a20769c003886d7f4bbec6563af7aabd4b
Show changes to 7 files +60 −6

diff.c, t/t4001-diff-rename.sh, xdiff/xdiff.h, xdiff/xemit.c, xdiff/xinclude.h, xdiff/xutils.c, xdiff/xutils.h

diff --git a/diff.c b/diff.c
index 5eae094..8b37477 100644
--- a/diff.c
+++ b/diff.c
@@ -267,6 +267,7 @@ static void builtin_diff(const char *nam
 		ecbdata.label_path = lbl;
 		xpp.flags = XDF_NEED_MINIMAL;
 		xecfg.ctxlen = 3;
+		xecfg.flags = XDL_EMIT_FUNCNAMES;
 		if (!diffopts)
 			;
 		else if (!strncmp(diffopts, "--unified=", 10))
diff --git a/t/t4001-diff-rename.sh b/t/t4001-diff-rename.sh
index 08c1131..2e3c20d 100755
--- a/t/t4001-diff-rename.sh
+++ b/t/t4001-diff-rename.sh
@@ -49,7 +49,7 @@ rename from path0
 rename to path1
 --- a/path0
 +++ b/path1
-@@ -8,7 +8,7 @@
+@@ -8,7 +8,7 @@ Line 7
  Line 8
  Line 9
  Line 10
diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
index 71cb939..2540e8a 100644
--- a/xdiff/xdiff.h
+++ b/xdiff/xdiff.h
@@ -35,6 +35,8 @@ #define XDL_PATCH_REVERSE '+'
 #define XDL_PATCH_MODEMASK ((1 << 8) - 1)
 #define XDL_PATCH_IGNOREBSPACE (1 << 8)
 
+#define XDL_EMIT_FUNCNAMES (1 << 0)
+
 #define XDL_MMB_READONLY (1 << 0)
 
 #define XDL_MMF_ATOMIC (1 << 0)
@@ -65,6 +67,7 @@ typedef struct s_xdemitcb {
 
 typedef struct s_xdemitconf {
 	long ctxlen;
+	unsigned long flags;
 } xdemitconf_t;
 
 typedef struct s_bdiffparam {
diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index 2e5d54c..ad5bfb1 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c
@@ -69,10 +69,43 @@ static xdchange_t *xdl_get_hunk(xdchange
 }
 
 
+static void xdl_find_func(xdfile_t *xf, long i, char *buf, long sz, long *ll) {
+
+	/*
+	 * Be quite stupid about this for now.  Find a line in the old file
+	 * before the start of the hunk (and context) which starts with a
+	 * plausible character.
+	 */
+
+	const char *rec;
+	long len;
+
+	*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 > sz)
+				len = sz;
+			if (len && rec[len - 1] == '\n')
+				len--;
+			memcpy(buf, rec, len);
+			*ll = len;
+			return;
+		}
+	}
+}
+
+
 int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,
 		  xdemitconf_t const *xecfg) {
 	long s1, s2, e1, e2, lctx;
 	xdchange_t *xch, *xche;
+	char funcbuf[40];
+	long funclen = 0;
 
 	for (xch = xche = xscr; xch; xch = xche->next) {
 		xche = xdl_get_hunk(xch, xecfg);
@@ -90,7 +123,13 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange
 		/*
 		 * Emit current hunk header.
 		 */
-		if (xdl_emit_hunk_hdr(s1 + 1, e1 - s1, s2 + 1, e2 - s2, ecb) < 0)
+
+		if (xecfg->flags & XDL_EMIT_FUNCNAMES) {
+			xdl_find_func(&xe->xdf1, s1, funcbuf,
+				      sizeof(funcbuf), &funclen);
+		}
+		if (xdl_emit_hunk_hdr(s1 + 1, e1 - s1, s2 + 1, e2 - s2,
+				      funcbuf, funclen, ecb) < 0)
 			return -1;
 
 		/*
diff --git a/xdiff/xinclude.h b/xdiff/xinclude.h
index 9490fc5..04a9da8 100644
--- a/xdiff/xinclude.h
+++ b/xdiff/xinclude.h
@@ -23,6 +23,7 @@
 #if !defined(XINCLUDE_H)
 #define XINCLUDE_H
 
+#include <ctype.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <unistd.h>
diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index 8221806..afaada1 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c
@@ -235,7 +235,8 @@ long xdl_atol(char const *str, char cons
 }
 
 
-int xdl_emit_hunk_hdr(long s1, long c1, long s2, long c2, xdemitcb_t *ecb) {
+int xdl_emit_hunk_hdr(long s1, long c1, long s2, long c2,
+		      const char *func, long funclen, xdemitcb_t *ecb) {
 	int nb = 0;
 	mmbuffer_t mb;
 	char buf[128];
@@ -264,8 +265,16 @@ int xdl_emit_hunk_hdr(long s1, long c1, 
 		nb += xdl_num_out(buf + nb, c2);
 	}
 
-	memcpy(buf + nb, " @@\n", 4);
-	nb += 4;
+	memcpy(buf + nb, " @@", 3);
+	nb += 3;
+	if (func && funclen) {
+		buf[nb++] = ' ';
+		if (funclen > sizeof(buf) - nb - 1)
+			funclen = sizeof(buf) - nb - 1;
+		memcpy(buf + nb, func, funclen);
+		nb += funclen;
+	}
+	buf[nb++] = '\n';
 
 	mb.ptr = buf;
 	mb.size = nb;
diff --git a/xdiff/xutils.h b/xdiff/xutils.h
index 428a4bb..55b0d39 100644
--- a/xdiff/xutils.h
+++ b/xdiff/xutils.h
@@ -36,7 +36,8 @@ unsigned long xdl_hash_record(char const
 unsigned int xdl_hashbits(unsigned int size);
 int xdl_num_out(char *out, long val);
 long xdl_atol(char const *str, char const **next);
-int xdl_emit_hunk_hdr(long s1, long c1, long s2, long c2, xdemitcb_t *ecb);
+int xdl_emit_hunk_hdr(long s1, long c1, long s2, long c2,
+		      const char *func, long funclen, xdemitcb_t *ecb);
 
 
 
-- 
1.3.0.rc1.g7464
Junio C Hamano· Mar 28, 2006, 05:54 UTC · re: Mark Wooding · lore

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

Mark Wooding <mdw@distorted.org.uk> 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.

Junio C Hamano· Mar 28, 2006, 09:50 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <junkio@cox.net> writes:
Show 9 quoted lines
> Mark Wooding <mdw@distorted.org.uk> writes:
>...
>> +		    (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.

Obviously I was not thinking. That should at least be "any line that begins with a non-whitespace and has a few characters", to omit "{\n" and catch "int main()\n" in:

	int main()
        {
        	printf("Hello, world.\n");
        }
;-).
Mark Wooding· Mar 28, 2006, 10:13 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <junkio@cox.net> wrote:
> Obviously I was not thinking.  That should at least be "any line
> that begins with a non-whitespace and has a few characters", to
> omit "{\n" and catch "int main()\n" in:

Heh! I already got that one wrong last night. Hence my more complicated version. ;-)

-- [mdw]
Mark Wooding· Mar 28, 2006, 15:03 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <junkio@cox.net> 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.

Show changes to xdiff/xemit.c +15 −16
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]
Junio C Hamano· Mar 29, 2006, 00:21 UTC · re: Mark Wooding · lore

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

Mark Wooding <mdw@distorted.org.uk> writes:
Show 10 quoted lines
> Junio C Hamano <junkio@cox.net> wrote:
>
>> GNU diff -p does "^[[:alpha:]$_]"; personally I think any line
>> that does not begin with a whitespace is good enough.
>...
> 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.
Asciidoc?
        . enumerated one
          this is one item
        . enumerated two
          this is another item
> I think I like option 2 best, as a nice compromise between stupidity and
> actually working.  Opinions, anyone?

It's just a heuristic, so there are only two things we could sensibly do. Either we keep it absolutely stupid to save our code and sanity, or we give full configurability via -F regexp to the end users.

I suspect feeping creaturism would eventually push us to go the latter route, but for now I'd vote for doing exactly the same as what default GNU does, by looking at the first letter without using regexps. When we add regexps later, the users can customize the pattern to match the languages they use, and we might end up having to have a set of (file-suffix -> default regexp) mappings, with full end user configurability via .git/config -- gaaah but true X-<.

	[diff]
        	functionline = "^\w" for .c
                functionline = "^(?i)\s*(?:function|procedure)" for .f77
                functionline = "^\(defun " for .el
                ...
Mark Wooding· Mar 29, 2006, 09:57 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <junkio@cox.net> wrote:
> Asciidoc?
>
>         . enumerated one
>           this is one item

I thought about this, and decided that missing these was a good thing, because item heads are probably too low-level. I'd rather go for section or subsection headings.

> It's just a heuristic, so there are only two things we could
> sensibly do.  Either we keep it absolutely stupid to save our
> code and sanity, or we give full configurability via -F regexp
> to the end users.
I'm already thinking about that...
-- [mdw]
Andreas Ericsson· Mar 28, 2006, 11:07 UTC · re: Mark Wooding · lore

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

Mark Wooding wrote:
Show 6 quoted lines
> 
> 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.
> 

Stupid is sometimes good. I've noticed that the gnu diff algorithm sometimes won't notice changes in small structs, though they are large enough for the surrounding context in the unified diff not to show it either.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

← back to recent threads