Re: misleading diff-hunk header

[Jeff King <peff@peff.net>]

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

> >>> diff.{type}.xfuncname seems to start searching backwards in
> >>> from the beginning of the hunk, not the first differing line.
> >> [...]
> >>>   @@ -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:

> 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.

-Peff