Re: misleading diff-hunk header

Tim Chase [off-list ref] writes:

On 08/21/12 10:22, Thomas Rast wrote:

quoted

Tim Chase [off-list ref] writes:

quoted

diff.{type}.xfuncname seems to start searching backwards in
from the beginning of the hunk, not the first differing line.

[...]

quoted

  @@ -4,4 +4,5 @@ int call_me(int maybe)

   int main()
   {
  +  return 0;
   }

misleadingly suggesting that the change occurred in the call_me()
function, rather than in main()

I think that's intentional, and matches what 'diff -p' does.  It gives
you the context before the hunk.  After all, if a new function starts in
the leading context lines, you can see that in the usual diff data.

Correct.  It is about "give the user _more_ hint/clue on the context
of the hunk", in addition to what the user can see in the
pre-context of the hunk, so it is pointless to hoist "int main()"
there.

...  It just makes it hard for me to gather some stats on the
functions that changed, and requires that I look in more than one
place (both in the header, and in the leading context) rather than
having a single authoritative place to grep.

The right way to answer "which functions were touched?" question is
to ignore what you see on the hunk header "@@ .. @@" lines and only
look at the patch text, running "git diff" with larger number of
context lines as necessary.

If you have a large patch hunk that adds or removes two or more new
functions, you would have to look at the patch text _anyway_ to
learn about these two or more names---they cannot possibly both
appear on the hunk header lines, so looking at the context hint
there is pointless for the purpose for which you are using "diff"
output.

On Tue, Aug 21, 2012 at 10:52:03AM -0700, Junio C Hamano wrote:

quoted

quoted

quoted

diff.{type}.xfuncname seems to start searching backwards in
from the beginning of the hunk, not the first differing line.

[...]

quoted

  @@ -4,4 +4,5 @@ int call_me(int maybe)

   int main()
   {
  +  return 0;
   }

misleadingly suggesting that the change occurred in the call_me()
function, rather than in main()

I think that's intentional, and matches what 'diff -p' does.  It gives
you the context before the hunk.  After all, if a new function starts in
the leading context lines, you can see that in the usual diff data.

Correct.  It is about "give the user _more_ hint/clue on the context
of the hunk", in addition to what the user can see in the
pre-context of the hunk, so it is pointless to hoist "int main()"
there.

I don't think it is pointless. If you are skimming a diff, then the hunk
headers stand out to easily show which functions were touched. Of
course, as you mentioned later in your email, it is not an exhaustive
list, and I think for Tim's use case, he needs to actually read and
parse the whole patch.

But mentioning call_me here _is_ pointless, because it is not relevant
context at all (it was not modified; it just happens to be located near
the code in question).  So I would argue that showing main() is more
useful to a reader.

It gets even more obvious as you increase the context. Imagine I have
code like this:

   int foo(void)
   {
          return 1;
   }

   int bar(void)
   {
          return 2;
   }

   int baz(void)
   {
          return 3;
   }

and I modify "baz" to return "4" instead. With the regular diff
settings, the hunk header would claim that "bar()" is the context in the
hunk header. But if I ask for -U7, then "foo()" is mentioned in the hunk
header. To me, that doesn't make sense; the modification is exactly the
same, so why would the hunk header differ?

I suppose one could argue that the hunk header is not showing the
context of the change, but rather the context of the surrounding context
lines. But that doesn't seem useful to me.

We discussed this a while ago and you did a "how about this" patch:

  http://article.gmane.org/gmane.comp.version-control.git/181385

Gmane seems to be acting up this morning, so here is the patch (and your
comment) for reference:

In the case we were discussing then, the modified function started on
the first line of context. But as Tim's example shows, it doesn't
necessarily have to. I think it would make more sense to start counting
from the first modified line.

-Peff

On 08/24/12 09:29, Jeff King wrote:

On Tue, Aug 21, 2012 at 10:52:03AM -0700, Junio C Hamano wrote:

quoted

quoted

quoted

quoted

diff.{type}.xfuncname seems to start searching backwards in
from the beginning of the hunk, not the first differing line.

[...]

quoted

  @@ -4,4 +4,5 @@ int call_me(int maybe)

   int main()
   {
  +  return 0;
   }

misleadingly suggesting that the change occurred in the call_me()
function, rather than in main()

I think that's intentional, and matches what 'diff -p' does. 

...

quoted

 xdiff/xemit.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index 277e2ee..5f9c0e0 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c

@@ -131,7 +131,7 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,
 
 		if (xecfg->flags & XDL_EMIT_FUNCNAMES) {
 			long l;
-			for (l = s1 - 1; l >= 0 && l > funclineprev; l--) {
+			for (l = s1; l >= 0 && l > funclineprev; l--) {
 				const char *rec;
 				long reclen = xdl_get_rec(&xe->xdf1, l, &rec);
 				long newfunclen = ff(rec, reclen, funcbuf,

In the case we were discussing then, the modified function started on
the first line of context. But as Tim's example shows, it doesn't
necessarily have to. I think it would make more sense to start counting
from the first modified line.

Junio mentions that it matches the "diff -p" output, though I'd
consider that a bug in diff as well, since the diff(1) man/info
pages state "-p  Show which C function each change is in."  In the
above (both with "diff -p" and with git), the change was clearly in
main() but it's not showing main().  Documented behavior and
implemented behavior conflict.

Starting at the first differing line rather than the first line of
context in the hunk would ameliorate this.  It doesn't address what
happens if multiple functions were changed in the same hunk, but at
least it becomes correct for the first one.  More complex code might
be doable to split hunks if an xfuncname match occurs between two
disjoint changes in the same hunk.  But for my purposes here, the
above should suffice.

-tkc

On Fri, Aug 24, 2012 at 10:29:09AM -0400, Jeff King wrote:

quoted

Would this be sufficient?  Instead of looking for the first line that
matches the "beginning" pattern going backwards starting from one line
before the displayed context, we start our examination at the first line
shown in the context.

 xdiff/xemit.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index 277e2ee..5f9c0e0 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c

@@ -131,7 +131,7 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,
 
 		if (xecfg->flags & XDL_EMIT_FUNCNAMES) {
 			long l;
-			for (l = s1 - 1; l >= 0 && l > funclineprev; l--) {
+			for (l = s1; l >= 0 && l > funclineprev; l--) {
 				const char *rec;
 				long reclen = xdl_get_rec(&xe->xdf1, l, &rec);
 				long newfunclen = ff(rec, reclen, funcbuf,

In the case we were discussing then, the modified function started on
the first line of context. But as Tim's example shows, it doesn't
necessarily have to. I think it would make more sense to start counting
from the first modified line.

That patch would look something like this:

diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index d11dbf9..441ecf5 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c

@@ -194,7 +194,7 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,
 
 		if (xecfg->flags & XDL_EMIT_FUNCNAMES) {
 			get_func_line(xe, xecfg, &func_line,
-				      s1 - 1, funclineprev);
+				      xche->i1 - 1, funclineprev);
 			funclineprev = s1 - 1;
 		}
 		if (xdl_emit_hunk_hdr(s1 + 1, e1 - s1, s2 + 1, e2 - s2,

Note that this breaks a ton of tests. Some of them are just noise (e.g.,
t4042 changes line 2, so line 1 is the top of the context; before, we
would show no hunk header, since we were at the top of the file, but now
we will show line 1). Some of them are improved in the way that this
patch intends (e.g., t4051).

But some I'm not sure of. For instance, the failure in t4018.38 is odd.
I think it's because the pattern it is looking for is a somewhat odd toy
example (it's looking for a line with "s" in it, so naturally when we
shift the start-point of our search, we are likely to find some other
false positive). But it raises an interesting point: what if the pattern
is just looking for lines in a list, and not an enclosing function?

For example, imagine you have a file a list of items, one per line.
With the old code, you'd get:

	diff --git a/old b/new
	index f384549..1066a25 100644
	--- a/old
	+++ b/new
	@@ -2,3 +2,3 @@ one
	 two
	-three
	+three -- modified
	 four

So the hunk header is showing you something useful; the element just
above your context. But with my patch, you'd see:

	diff --git a/old b/new
	index f384549..1066a25 100644
	--- a/old
	+++ b/new
	@@ -2,3 +2,3 @@ two
	 two
	-three
	+three -- modified
	 four

I.e., it shows the element just before the change, which is already in
the context anyway. So it's actually less useful. Although note that the
current behavior is not all that useful, either; it is not really giving
you any information about the change, but rather just showing one extra
line of context.

So I would say that which you would prefer might depend on exactly what
you are diffing. But I would also argue that in any case where the new
code produces a worse result, the hunk header was not all that useful to
begin with.

-Peff

On 08/24/12 11:44, Jeff King wrote:

With the old code, you'd get:

	diff --git a/old b/new
	index f384549..1066a25 100644
	--- a/old
	+++ b/new
	@@ -2,3 +2,3 @@ one
	 two
	-three
	+three -- modified
	 four

So the hunk header is showing you something useful; the element just
above your context. But with my patch, you'd see:

	diff --git a/old b/new
	index f384549..1066a25 100644
	--- a/old
	+++ b/new
	@@ -2,3 +2,3 @@ two
	 two
	-three
	+three -- modified
	 four

I.e., it shows the element just before the change, which is already in
the context anyway. So it's actually less useful. Although note that the
current behavior is not all that useful, either; it is not really giving
you any information about the change, but rather just showing one extra
line of context.

So I would say that which you would prefer might depend on exactly what
you are diffing. But I would also argue that in any case where the new
code produces a worse result, the hunk header was not all that useful to
begin with.

If the documented purpose of "diff -p" (and by proxy
diff.{type}.xfuncname) is to show the name of the *function*
containing the changed lines, and all you have is a list of lines
with no function names, it's pretty arbitrary to call either
behavior "worse". :-)

-tkc