git diff woes

git diff woes

[Andreas Ericsson]

I recently ran into an oddity with the excellent git diff output
format. When a function declaration changes in the same patch as
something else in a function, the old declaration is used with the
diff hunk-headers.

Consider this hunk:
---%<---%<---%<---
@@ -583,75 +346,100 @@ double jitter_request(const char *host, int *status){
        if(verbose) printf("%d candiate peers available\n", num_candidates);
        if(verbose && syncsource_found) printf("synchronization source found\n")
        if(! syncsource_found){
-               *status = STATE_UNKNOWN;
+               status = STATE_WARNING;
                if(verbose) printf("warning: no synchronization source found\n")
        }
---%<---%<---%<---

It definitely looks like a bug, but really isn't, since an earlier hunk
(pasted below) changes the declaration. There were several hunks between
these two, so it was far from obvious when I saw it first.

---%<---%<---%<---
@@ -517,19 +276,22 @@ setup_control_request(ntp_control_message *p, uint8_t opco
 }
 
 /* XXX handle responses with the error bit set */
-double jitter_request(const char *host, int *status){
-       int conn=-1, i, npeers=0, num_candidates=0, syncsource_found=0;
-       int run=0, min_peer_sel=PEER_INCLUDED, num_selected=0, num_valid=0;
+int ntp_request(const char *host, double *offset, int *offset_result, double *j
+       int conn=-1, i, npeers=0, num_candidates=0;
+       int min_peer_sel=PEER_INCLUDED;
        int peers_size=0, peer_offset=0;
+       int status;
---%<---%<---%<--- 
 
This makes it impossible to trust the hunk-header info if the declaration
changes. It might be better to not write it out when the header-line is
also part of the patch. That would at least force one to go back and find
the real declaration. Best would probably be to write the new declaration,
but I'm unsure if that could cause some other confusion.

I haven't started looking into it yet, and as I'm sure there are others
who are much more familiar with the xdiff code I'm shamelessly hoping
someone will beat me to a fix.

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

Re: git diff woes

[Johannes Schindelin]

Hi,

On Mon, 12 Nov 2007, Andreas Ericsson wrote:

> I recently ran into an oddity with the excellent git diff output
> format. When a function declaration changes in the same patch as
> something else in a function, the old declaration is used with the
> diff hunk-headers.
> 
> [...]
> 
> It definitely looks like a bug, but really isn't, since an earlier hunk
> (pasted below) changes the declaration.
>
> [...]
>
> This makes it impossible to trust the hunk-header info if the declaration
> changes.

Huh?  You admit yourself that it is not a bug.  And sure you can trust the 
hunk header.  Like most of the things, the relate to the _original_ 
version, since the diff is meant to be applied as a forward patch.

So for all practical matters, the diff shows the correct thing: "in this 
hunk, which (still) belongs to that function, change this and this."

Of course, that is only the case if you accept that the diff should be 
applied _in total_, not piecewise.  IOW if you are a fan of GNU patch 
which happily clobbers your file until it fails with the last hunk, you 
will not be happy.

Ciao,
Dscho

Re: git diff woes

[Andreas Ericsson]

Johannes Schindelin wrote:
> Hi,
> 
> On Mon, 12 Nov 2007, Andreas Ericsson wrote:
> 
>> I recently ran into an oddity with the excellent git diff output
>> format. When a function declaration changes in the same patch as
>> something else in a function, the old declaration is used with the
>> diff hunk-headers.
>>
>> [...]
>>
>> It definitely looks like a bug, but really isn't, since an earlier hunk
>> (pasted below) changes the declaration.
>>
>> [...]
>>
>> This makes it impossible to trust the hunk-header info if the declaration
>> changes.
> 
> Huh?  You admit yourself that it is not a bug.


In the check_ntpd.c program, there is no bug. I found the git diff output
surprising, so I reported it.

>  And sure you can trust the 
> hunk header.  Like most of the things, the relate to the _original_ 
> version, since the diff is meant to be applied as a forward patch.
> 
> So for all practical matters, the diff shows the correct thing: "in this 
> hunk, which (still) belongs to that function, change this and this."
> 
> Of course, that is only the case if you accept that the diff should be 
> applied _in total_, not piecewise.  IOW if you are a fan of GNU patch 
> which happily clobbers your file until it fails with the last hunk, you 
> will not be happy.
> 

You're right. GNU patch will apply one hunk and then happily churn on even
if it fails. git-apply will apply all hunks or none, so all hunks can assume
that all previous hunks were successfully applied. So what was your point
again?

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

Re: git diff woes

[Johannes Schindelin]

Hi,

On Mon, 12 Nov 2007, Andreas Ericsson wrote:

> Johannes Schindelin wrote:
> 
> >  And sure you can trust the hunk header.  Like most of the things, the 
> > relate to the _original_ version, since the diff is meant to be 
> > applied as a forward patch.
> > 
> > So for all practical matters, the diff shows the correct thing: "in 
> > this hunk, which (still) belongs to that function, change this and 
> > this."
> > 
> > Of course, that is only the case if you accept that the diff should be 
> > applied _in total_, not piecewise.  IOW if you are a fan of GNU patch 
> > which happily clobbers your file until it fails with the last hunk, 
> > you will not be happy.
> > 
> 
> You're right. GNU patch will apply one hunk and then happily churn on 
> even if it fails. git-apply will apply all hunks or none, so all hunks 
> can assume that all previous hunks were successfully applied. So what 
> was your point again?

My point was that this diff is not to be read as if the previous hunks had 
been applied.  Just look at the context: it is also the original file.

It seems I am singularly unable to explain plain concepts as this: a diff 
assumes that the file is yet unchanged.

So I'll stop.

Ciao,
Dscho

Re: git diff woes

[Andreas Ericsson]

Johannes Schindelin wrote:
> Hi,
> 
> On Mon, 12 Nov 2007, Andreas Ericsson wrote:
> 
>> Johannes Schindelin wrote:
>>
>>>  And sure you can trust the hunk header.  Like most of the things, the 
>>> relate to the _original_ version, since the diff is meant to be 
>>> applied as a forward patch.
>>>
>>> So for all practical matters, the diff shows the correct thing: "in 
>>> this hunk, which (still) belongs to that function, change this and 
>>> this."
>>>
>>> Of course, that is only the case if you accept that the diff should be 
>>> applied _in total_, not piecewise.  IOW if you are a fan of GNU patch 
>>> which happily clobbers your file until it fails with the last hunk, 
>>> you will not be happy.
>>>
>> You're right. GNU patch will apply one hunk and then happily churn on 
>> even if it fails. git-apply will apply all hunks or none, so all hunks 
>> can assume that all previous hunks were successfully applied. So what 
>> was your point again?
> 
> My point was that this diff is not to be read as if the previous hunks had 
> been applied.  Just look at the context: it is also the original file.
> 

The context is ambiguous, as it must be present in both the new and the
old file for it to actually *be* context. Otherwise it would be part of
the +- diff text.

> It seems I am singularly unable to explain plain concepts as this: a diff 
> assumes that the file is yet unchanged.
> 

Sure, but the useraid with writing the apparent function declaration in
the hunk header *will* be confusing if the function declaration changes
in the same patch as other things in the function.

> So I'll stop.
> 

Give me something valuable instead, such as your opinion on whether it
would be better to not print the function declaration at all if it will
be changed by applying the same patch, or if one should pick one of the
declarations from old or new and, if so, which one to pick.

I simply refuse to believe that you wouldn't immediately think the hunk
below holds an obvious bug. I thought so because of the helpful function
context git diff prints (which is a helper for human reviewers, and not
something git-apply or GNU patch needs to work), and now I want to do
something about it so others won't have to suffer the same confusion.

@@ -583,75 +346,100 @@ double jitter_request(const char *host, int *status){
       if(verbose) printf("%d candiate peers available\n", num_candidates);
       if(verbose && syncsource_found) printf("synchronization source found\n")
       if(! syncsource_found){
-               *status = STATE_UNKNOWN;
+               status = STATE_WARNING;
               if(verbose) printf("warning: no synchronization source found\n")
       }

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

Re: git diff woes

[Junio C Hamano]

Andreas Ericsson <ae@op5.se> writes:

> In the check_ntpd.c program, there is no bug. I found the git diff output
> surprising, so I reported it.

This is what I get from "GNU diff -pu" which makes me surpried
that anybody finds "git diff" hunk header surprising.  Notice
that hunk at line 84.

--- read-cache.c	2007-11-12 12:08:00.000000000 -0800
+++ read-cache.c+	2007-11-12 12:07:54.000000000 -0800
@@ -60,7 +60,7 @@ static int ce_compare_data(struct cache_
 	return match;
 }
 
-static int ce_compare_link(struct cache_entry *ce, size_t expected_size)
+static int ce_compare_lonk(struct cache_entry *ce, size_t expected_size)
 {
 	int match = -1;
 	char *target;
@@ -84,7 +84,7 @@ static int ce_compare_link(struct cache_
 		match = memcmp(buffer, target, size);
 	free(buffer);
 	free(target);
-	return match;
+	return match + 0;
 }
 
 static int ce_compare_gitlink(struct cache_entry *ce)

Re: git diff woes

[Andreas Ericsson]

Junio C Hamano wrote:
> Andreas Ericsson <ae@op5.se> writes:
> 
>> In the check_ntpd.c program, there is no bug. I found the git diff output
>> surprising, so I reported it.
> 
> This is what I get from "GNU diff -pu" which makes me surpried
> that anybody finds "git diff" hunk header surprising.  Notice
> that hunk at line 84.
> 
> --- read-cache.c	2007-11-12 12:08:00.000000000 -0800
> +++ read-cache.c+	2007-11-12 12:07:54.000000000 -0800
> @@ -60,7 +60,7 @@ static int ce_compare_data(struct cache_
>  	return match;
>  }
>  
> -static int ce_compare_link(struct cache_entry *ce, size_t expected_size)
> +static int ce_compare_lonk(struct cache_entry *ce, size_t expected_size)
>  {
>  	int match = -1;
>  	char *target;
> @@ -84,7 +84,7 @@ static int ce_compare_link(struct cache_
>  		match = memcmp(buffer, target, size);
>  	free(buffer);
>  	free(target);
> -	return match;
> +	return match + 0;
>  }
>  
>  static int ce_compare_gitlink(struct cache_entry *ce)


I notice it, and I don't like it. I guess I'm just used to git being
smarter than their GNU tool equivalents, especially since it only ever
applies patches in full.

I have a patch ready to make it configurable but it lacks doc updates
and tests, so I'll send it tomorrow morning when I've had time to
fiddle a bit with that.

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

Re: git diff woes

[Johannes Schindelin]

Hi,

On Tue, 13 Nov 2007, Andreas Ericsson wrote:

> Junio C Hamano wrote:
> > Andreas Ericsson <ae@op5.se> writes:
> > 
> > > In the check_ntpd.c program, there is no bug. I found the git diff 
> > > output surprising, so I reported it.
> > 
> > This is what I get from "GNU diff -pu" which makes me surpried
> > that anybody finds "git diff" hunk header surprising.  Notice
> > that hunk at line 84.
> > 
> > --- read-cache.c	2007-11-12 12:08:00.000000000 -0800
> > +++ read-cache.c+	2007-11-12 12:07:54.000000000 -0800
> > @@ -60,7 +60,7 @@ static int ce_compare_data(struct cache_
> >  	return match;
> >  }
> >  -static int ce_compare_link(struct cache_entry *ce, size_t expected_size)
> > +static int ce_compare_lonk(struct cache_entry *ce, size_t expected_size)
> >  {
> >  	int match = -1;
> >  	char *target;
> > @@ -84,7 +84,7 @@ static int ce_compare_link(struct cache_
> >  		match = memcmp(buffer, target, size);
> >  	free(buffer);
> >  	free(target);
> > -	return match;
> > +	return match + 0;
> >  }
> >   static int ce_compare_gitlink(struct cache_entry *ce)
> 
> 
> I notice it, and I don't like it. I guess I'm just used to git being
> smarter than their GNU tool equivalents, especially since it only ever
> applies patches in full.

I still think the existing behaviour is reasonable.  When I read a diff 
(and remember, the hunk headers are _only_ there for the reviewer's 
pleasure), the function names are a hint for _me_ where to look, and which 
is the context, in my existing, _original_ file.

That is, unless I have already applied the patch, and am looking for the 
reverse patch.  And, lo and behold, the reverse patch generated by 
git-diff really shows the now-current function name!

So IMO "fixing" this behaviour would be a regression.

Ciao,
Dscho

Re: git diff woes

[Miles Bader]

Andreas Ericsson <ae@op5.se> writes:
> I notice it, and I don't like it. I guess I'm just used to git being
> smarter than their GNU tool equivalents, especially since it only ever
> applies patches in full.

It's not at all obvious that this behavior is actually wrong -- it seems
perfectly reasonable to use either old or new text for the hunk headers.

It hardly matters really, since that particular output is just "useful
noise" to provide a bit of helpful context for human readers, and humans
(unlike programs) are notoriously good at not being bothered by such
things.  Er, well most humans anyway.

-Miles

-- 
Americans are broad-minded people.  They'll accept the fact that a person can
be an alcoholic, a dope fiend, a wife beater, and even a newspaperman, but if a
man doesn't drive, there is something wrong with him.  -- Art Buchwald

Re: git diff woes

[Andreas Ericsson]

Miles Bader wrote:
> Andreas Ericsson <ae@op5.se> writes:
>> I notice it, and I don't like it. I guess I'm just used to git being
>> smarter than their GNU tool equivalents, especially since it only ever
>> applies patches in full.
> 
> It's not at all obvious that this behavior is actually wrong -- it seems
> perfectly reasonable to use either old or new text for the hunk headers.
> 

Right, which is why I've made it configurable.

> It hardly matters really, since that particular output is just "useful
> noise" to provide a bit of helpful context for human readers, and humans
> (unlike programs) are notoriously good at not being bothered by such
> things.  Er, well most humans anyway.
> 

I wouldn't have reacted either, except that this time someone asked me to
review a branch early in the morning because he had introduced a bug in the
process, and the hunk header information made me assume the wrong hunk of
the patch was the culprit.

On the one hand, it wouldn't have been so much of a problem if the developer
in question would have followed my suggestion of committing small and making
sure the commit message describes everything that's done. On the other hand,
a tool fooling a human isn't a good thing either, even if said human is not
really in shape for using said tool.

Granted, the new form can still fool people, but for archeology excursions
I think it's definitely right to use the "new" funcname in the hunk header.

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

[PATCH] diffcore: Allow users to decide what funcname to use

[Andreas Ericsson]

Andreas Ericsson wrote:
> Miles Bader wrote:
>> Andreas Ericsson <ae@op5.se> writes:
>>> I notice it, and I don't like it. I guess I'm just used to git being
>>> smarter than their GNU tool equivalents, especially since it only ever
>>> applies patches in full.
>>
>> It's not at all obvious that this behavior is actually wrong -- it seems
>> perfectly reasonable to use either old or new text for the hunk headers.
>>
> 
> Right, which is why I've made it configurable.
> 

My git hacking has been stalled at the office for now, and I'm swanked at
home since my girlfriend just moved in and brought temporary pandemonium
with her. Here's what I've got now. It passes all tests, but there are no
new ones added. Documentation also needs updating.

Extract with
	sed -n -e /^#CUTSTART/,/^#CUTEND/p -e /^#/d

#CUTSTART---%<---%<---%<---
From: Andreas Ericsson <ae@op5.se>
Date: Tue, 13 Nov 2007 09:47:43 +0100
Subject: [PATCH] diffcore: Allow users to decide what funcname to use

The function name being printed with the header of each
hunk is fetched from the "old" file today. Since git by
default applies patches either in full or not at all it's
arguably more correct to use the function from the "new"
file, at least when manually reviewing commits.

I stumbled upon this hunk when reviewing a series of
commits which caused the resulting code to segfault
under certain circumstances. Several hunks before, the
function declaration was changed and "status" was now
declared as an auto variable of type "int". The hunk
looks obviously bogus, and since I wasn't properly
awake, I reported this hunk to be the bogus one.

  @@ -583,75 +346,100 @@ double jitter_request(int *status){
     context
     context
     if(!syncsource_found){
  -    *status = STATE_UNKNOWN;
  +    status = STATE_WARNING;
       if(verbose) printf("warning: no sync source found\n")
     }

This is what GNU "diff -p" would have reported under the
same circumstances, but GNU diff has no notion of version
control, and as such will not know if it's being used on
content where the patch by definition will apply in full.

Git can be smarter than that, and imo it should. This
patch lets the diffcore grok a new configuration variable,
"diff.funcnames", which can be set to "new", "old", or a
boolean value, which will cause it to be "old" (for 'true')
and 'none' (for 'false').

Signed-off-by: Andreas Ericsson <ae@op5.se>
---
 diff.c           |   20 ++++++++++++++++++--
 diffcore-break.c |    4 ++--
 xdiff/xdiff.h    |    3 ++-
 xdiff/xemit.c    |    7 ++++++-
 4 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/diff.c b/diff.c
index 6bb902f..057bba8 100644
--- a/diff.c
+++ b/diff.c
@@ -20,6 +20,7 @@
 static int diff_detect_rename_default;
 static int diff_rename_limit_default = 100;
 static int diff_use_color_default;
+static unsigned long xdl_emit_flags = XDL_EMIT_FUNCNAMES;
 int diff_auto_refresh_index = 1;
 
 static char diff_colors[][COLOR_MAXLEN] = {
@@ -178,6 +179,20 @@ int git_diff_ui_config(const char *var, const char *value)
                color_parse(value, var, diff_colors[slot]);
                return 0;
        }
+       if (!prefixcmp(var, "diff.funcnames")) {
+               if (!value)
+                       xdl_emit_flags = XDL_EMIT_COMMON;
+               else if (!strcasecmp(value, "new"))
+                       xdl_emit_flags = XDL_EMIT_FUNCNAMES_NEW;
+               else if (!strcasecmp(value, "old") || !strcasecmp(value, "default"))
+                       xdl_emit_flags = XDL_EMIT_FUNCNAMES;
+               else {
+                       if (git_config_bool(var, value))
+                               xdl_emit_flags = XDL_EMIT_FUNCNAMES;
+                       else
+                               xdl_emit_flags = XDL_EMIT_COMMON;
+               }
+       }
 
        return git_default_config(var, value);
 }
@@ -1332,7 +1347,8 @@ static void builtin_diff(const char *name_a,
                ecbdata.found_changesp = &o->found_changes;
                xpp.flags = XDF_NEED_MINIMAL | o->xdl_opts;
                xecfg.ctxlen = o->context;
-               xecfg.flags = XDL_EMIT_FUNCNAMES;
+               xecfg.flags = xdl_emit_flags;
+
                if (funcname_pattern)
                        xdiff_set_find_func(&xecfg, funcname_pattern);
                if (!diffopts)
@@ -2844,7 +2860,7 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1)
 
                xpp.flags = XDF_NEED_MINIMAL;
                xecfg.ctxlen = 3;
-               xecfg.flags = XDL_EMIT_FUNCNAMES;
+               xecfg.flags = xdl_emit_flags;
                ecb.outf = xdiff_outf;
                ecb.priv = &data;
                xdl_diff(&mf1, &mf2, &xpp, &xecfg, &ecb);
diff --git a/diffcore-break.c b/diffcore-break.c
index c71a226..048ec25 100644
--- a/diffcore-break.c
+++ b/diffcore-break.c
@@ -257,8 +257,8 @@ void diffcore_merge_broken(void)
                if (!p)
                        /* we already merged this with its peer */
                        continue;
-               else if (p->broken_pair &&
-                        !strcmp(p->one->path, p->two->path)) {
+
+               if (p->broken_pair && !strcmp(p->one->path, p->two->path)) {
                        /* If the peer also survived rename/copy, then
                         * we merge them back together.
                         */
diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
index c00ddaa..326e1df 100644
--- a/xdiff/xdiff.h
+++ b/xdiff/xdiff.h
@@ -41,7 +41,8 @@ extern "C" {
 
 #define XDL_EMIT_FUNCNAMES (1 << 0)
 #define XDL_EMIT_COMMON (1 << 1)
-
+#define XDL_EMIT_FUNCNAMES_NEW (1 << 2)
+
 #define XDL_MMB_READONLY (1 << 0)
 
 #define XDL_MMF_ATOMIC (1 << 0)
diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index d3d9c84..51dd085 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c
@@ -149,7 +149,12 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,
                 * Emit current hunk header.
                 */
 
-               if (xecfg->flags & XDL_EMIT_FUNCNAMES) {
+               if (xecfg->flags & XDL_EMIT_FUNCNAMES_NEW) {
+                       xdl_find_func(&xe->xdf2, s2, funcbuf,
+                                     sizeof(funcbuf), &funclen,
+                                     ff, xecfg->find_func_priv);
+               }
+               else if (xecfg->flags & XDL_EMIT_FUNCNAMES) {
                        xdl_find_func(&xe->xdf1, s1, funcbuf,
                                      sizeof(funcbuf), &funclen,
                                      ff, xecfg->find_func_priv);
-- 
1.5.3.5.1527.g6161
#CUTEND---%<---%<---%<---

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

Re: [PATCH] diffcore: Allow users to decide what funcname to use

[Jakub Narebski]

Andreas Ericsson wrote:

> Git can be smarter than that, and imo it should. This
> patch lets the diffcore grok a new configuration variable,
> "diff.funcnames", which can be set to "new", "old", or a
> boolean value, which will cause it to be "old" (for 'true')
> and 'none' (for 'false').

Wouldn't it be better to use existing 'diff driver' infrastructure
for this? See "Defining a custom hunk-header" section in
gitattributes(5).

On the other hand... no.

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [PATCH] diffcore: Allow users to decide what funcname to use

[Andreas Ericsson]

Jakub Narebski wrote:
> Andreas Ericsson wrote:
> 
>> Git can be smarter than that, and imo it should. This
>> patch lets the diffcore grok a new configuration variable,
>> "diff.funcnames", which can be set to "new", "old", or a
>> boolean value, which will cause it to be "old" (for 'true')
>> and 'none' (for 'false').
> 
> Wouldn't it be better to use existing 'diff driver' infrastructure
> for this? See "Defining a custom hunk-header" section in
> gitattributes(5).
> 
> On the other hand... no.
> 

It's impossible to do that, since that driver will only ever be
fed the "old" file with the old code. I'm guessing you noticed
that yourself, so just explaining in case anyone else wonders.

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