{"thread":{"id":"10822","subject":"git diff woes","startedAt":"2007-11-12T09:44:45Z","lastAt":"2007-11-13T10:07:31Z","messageCount":13,"participants":["Andreas Ericsson","Johannes Schindelin","Junio C Hamano","Miles Bader","Jakub Narebski"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"59447","messageId":"4738208D.1080003@op5.se","threadId":"10822","inReplyTo":null,"subject":"git diff woes","fromName":"Andreas Ericsson","fromEmail":"ae@op5.se","sentAt":"2007-11-12T09:44:45Z","receivedAt":"2007-11-12T09:44:45Z","isPatch":false,"sender":{"key":"ae@op5.se","avatar":"https://gravatar.com/avatar/426e89595c75a8f5252dd0c989e5fabe5bcac616e68557427ad9aef6b0ca342a?d=mp&s=160"},"body":"I recently ran into an oddity with the excellent git diff output\nformat. When a function declaration changes in the same patch as\nsomething else in a function, the old declaration is used with the\ndiff hunk-headers.\n\nConsider this hunk:\n---%<---%<---%<---\n@@ -583,75 +346,100 @@ double jitter_request(const char *host, int *status){\n        if(verbose) printf(\"%d candiate peers available\\n\", num_candidates);\n        if(verbose && syncsource_found) printf(\"synchronization source found\\n\")\n        if(! syncsource_found){\n-               *status = STATE_UNKNOWN;\n+               status = STATE_WARNING;\n                if(verbose) printf(\"warning: no synchronization source found\\n\")\n        }\n---%<---%<---%<---\n\nIt definitely looks like a bug, but really isn't, since an earlier hunk\n(pasted below) changes the declaration. There were several hunks between\nthese two, so it was far from obvious when I saw it first.\n\n---%<---%<---%<---\n@@ -517,19 +276,22 @@ setup_control_request(ntp_control_message *p, uint8_t opco\n }\n \n /* XXX handle responses with the error bit set */\n-double jitter_request(const char *host, int *status){\n-       int conn=-1, i, npeers=0, num_candidates=0, syncsource_found=0;\n-       int run=0, min_peer_sel=PEER_INCLUDED, num_selected=0, num_valid=0;\n+int ntp_request(const char *host, double *offset, int *offset_result, double *j\n+       int conn=-1, i, npeers=0, num_candidates=0;\n+       int min_peer_sel=PEER_INCLUDED;\n        int peers_size=0, peer_offset=0;\n+       int status;\n---%<---%<---%<--- \n \nThis makes it impossible to trust the hunk-header info if the declaration\nchanges. It might be better to not write it out when the header-line is\nalso part of the patch. That would at least force one to go back and find\nthe real declaration. Best would probably be to write the new declaration,\nbut I'm unsure if that could cause some other confusion.\n\nI haven't started looking into it yet, and as I'm sure there are others\nwho are much more familiar with the xdiff code I'm shamelessly hoping\nsomeone will beat me to a fix.\n\n-- \nAndreas Ericsson                   andreas.ericsson@op5.se\nOP5 AB                             www.op5.se\nTel: +46 8-230225                  Fax: +46 8-230231\n"},{"id":"59452","messageId":"Pine.LNX.4.64.0711120958500.4362@racer.site","threadId":"10822","inReplyTo":"4738208D.1080003@op5.se","subject":"Re: git diff woes","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-11-12T10:01:51Z","receivedAt":"2007-11-12T10:01:51Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 12 Nov 2007, Andreas Ericsson wrote:\n\n> I recently ran into an oddity with the excellent git diff output\n> format. When a function declaration changes in the same patch as\n> something else in a function, the old declaration is used with the\n> diff hunk-headers.\n> \n> [...]\n> \n> It definitely looks like a bug, but really isn't, since an earlier hunk\n> (pasted below) changes the declaration.\n>\n> [...]\n>\n> This makes it impossible to trust the hunk-header info if the declaration\n> changes.\n\nHuh?  You admit yourself that it is not a bug.  And sure you can trust the \nhunk header.  Like most of the things, the relate to the _original_ \nversion, since the diff is meant to be applied as a forward patch.\n\nSo for all practical matters, the diff shows the correct thing: \"in this \nhunk, which (still) belongs to that function, change this and this.\"\n\nOf course, that is only the case if you accept that the diff should be \napplied _in total_, not piecewise.  IOW if you are a fan of GNU patch \nwhich happily clobbers your file until it fails with the last hunk, you \nwill not be happy.\n\nCiao,\nDscho\n"},{"id":"59459","messageId":"47382C84.50408@op5.se","threadId":"10822","inReplyTo":"Pine.LNX.4.64.0711120958500.4362@racer.site","subject":"Re: git diff woes","fromName":"Andreas Ericsson","fromEmail":"ae@op5.se","sentAt":"2007-11-12T10:35:48Z","receivedAt":"2007-11-12T10:35:48Z","isPatch":false,"sender":{"key":"ae@op5.se","avatar":"https://gravatar.com/avatar/426e89595c75a8f5252dd0c989e5fabe5bcac616e68557427ad9aef6b0ca342a?d=mp&s=160"},"body":"Johannes Schindelin wrote:\n> Hi,\n> \n> On Mon, 12 Nov 2007, Andreas Ericsson wrote:\n> \n>> I recently ran into an oddity with the excellent git diff output\n>> format. When a function declaration changes in the same patch as\n>> something else in a function, the old declaration is used with the\n>> diff hunk-headers.\n>>\n>> [...]\n>>\n>> It definitely looks like a bug, but really isn't, since an earlier hunk\n>> (pasted below) changes the declaration.\n>>\n>> [...]\n>>\n>> This makes it impossible to trust the hunk-header info if the declaration\n>> changes.\n> \n> Huh?  You admit yourself that it is not a bug.\n\n\nIn the check_ntpd.c program, there is no bug. I found the git diff output\nsurprising, so I reported it.\n\n>  And sure you can trust the \n> hunk header.  Like most of the things, the relate to the _original_ \n> version, since the diff is meant to be applied as a forward patch.\n> \n> So for all practical matters, the diff shows the correct thing: \"in this \n> hunk, which (still) belongs to that function, change this and this.\"\n> \n> Of course, that is only the case if you accept that the diff should be \n> applied _in total_, not piecewise.  IOW if you are a fan of GNU patch \n> which happily clobbers your file until it fails with the last hunk, you \n> will not be happy.\n> \n\nYou're right. GNU patch will apply one hunk and then happily churn on even\nif it fails. git-apply will apply all hunks or none, so all hunks can assume\nthat all previous hunks were successfully applied. So what was your point\nagain?\n\n-- \nAndreas Ericsson                   andreas.ericsson@op5.se\nOP5 AB                             www.op5.se\nTel: +46 8-230225                  Fax: +46 8-230231\n"},{"id":"59461","messageId":"Pine.LNX.4.64.0711121047590.4362@racer.site","threadId":"10822","inReplyTo":"47382C84.50408@op5.se","subject":"Re: git diff woes","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-11-12T10:50:34Z","receivedAt":"2007-11-12T10:50:34Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 12 Nov 2007, Andreas Ericsson wrote:\n\n> Johannes Schindelin wrote:\n> \n> >  And sure you can trust the hunk header.  Like most of the things, the \n> > relate to the _original_ version, since the diff is meant to be \n> > applied as a forward patch.\n> > \n> > So for all practical matters, the diff shows the correct thing: \"in \n> > this hunk, which (still) belongs to that function, change this and \n> > this.\"\n> > \n> > Of course, that is only the case if you accept that the diff should be \n> > applied _in total_, not piecewise.  IOW if you are a fan of GNU patch \n> > which happily clobbers your file until it fails with the last hunk, \n> > you will not be happy.\n> > \n> \n> You're right. GNU patch will apply one hunk and then happily churn on \n> even if it fails. git-apply will apply all hunks or none, so all hunks \n> can assume that all previous hunks were successfully applied. So what \n> was your point again?\n\nMy point was that this diff is not to be read as if the previous hunks had \nbeen applied.  Just look at the context: it is also the original file.\n\nIt seems I am singularly unable to explain plain concepts as this: a diff \nassumes that the file is yet unchanged.\n\nSo I'll stop.\n\nCiao,\nDscho\n"},{"id":"59466","messageId":"473836AC.6090802@op5.se","threadId":"10822","inReplyTo":"Pine.LNX.4.64.0711121047590.4362@racer.site","subject":"Re: git diff woes","fromName":"Andreas Ericsson","fromEmail":"ae@op5.se","sentAt":"2007-11-12T11:19:08Z","receivedAt":"2007-11-12T11:19:08Z","isPatch":false,"sender":{"key":"ae@op5.se","avatar":"https://gravatar.com/avatar/426e89595c75a8f5252dd0c989e5fabe5bcac616e68557427ad9aef6b0ca342a?d=mp&s=160"},"body":"Johannes Schindelin wrote:\n> Hi,\n> \n> On Mon, 12 Nov 2007, Andreas Ericsson wrote:\n> \n>> Johannes Schindelin wrote:\n>>\n>>>  And sure you can trust the hunk header.  Like most of the things, the \n>>> relate to the _original_ version, since the diff is meant to be \n>>> applied as a forward patch.\n>>>\n>>> So for all practical matters, the diff shows the correct thing: \"in \n>>> this hunk, which (still) belongs to that function, change this and \n>>> this.\"\n>>>\n>>> Of course, that is only the case if you accept that the diff should be \n>>> applied _in total_, not piecewise.  IOW if you are a fan of GNU patch \n>>> which happily clobbers your file until it fails with the last hunk, \n>>> you will not be happy.\n>>>\n>> You're right. GNU patch will apply one hunk and then happily churn on \n>> even if it fails. git-apply will apply all hunks or none, so all hunks \n>> can assume that all previous hunks were successfully applied. So what \n>> was your point again?\n> \n> My point was that this diff is not to be read as if the previous hunks had \n> been applied.  Just look at the context: it is also the original file.\n> \n\nThe context is ambiguous, as it must be present in both the new and the\nold file for it to actually *be* context. Otherwise it would be part of\nthe +- diff text.\n\n> It seems I am singularly unable to explain plain concepts as this: a diff \n> assumes that the file is yet unchanged.\n> \n\nSure, but the useraid with writing the apparent function declaration in\nthe hunk header *will* be confusing if the function declaration changes\nin the same patch as other things in the function.\n\n> So I'll stop.\n> \n\nGive me something valuable instead, such as your opinion on whether it\nwould be better to not print the function declaration at all if it will\nbe changed by applying the same patch, or if one should pick one of the\ndeclarations from old or new and, if so, which one to pick.\n\nI simply refuse to believe that you wouldn't immediately think the hunk\nbelow holds an obvious bug. I thought so because of the helpful function\ncontext git diff prints (which is a helper for human reviewers, and not\nsomething git-apply or GNU patch needs to work), and now I want to do\nsomething about it so others won't have to suffer the same confusion.\n\n@@ -583,75 +346,100 @@ double jitter_request(const char *host, int *status){\n       if(verbose) printf(\"%d candiate peers available\\n\", num_candidates);\n       if(verbose && syncsource_found) printf(\"synchronization source found\\n\")\n       if(! syncsource_found){\n-               *status = STATE_UNKNOWN;\n+               status = STATE_WARNING;\n               if(verbose) printf(\"warning: no synchronization source found\\n\")\n       }\n\n-- \nAndreas Ericsson                   andreas.ericsson@op5.se\nOP5 AB                             www.op5.se\nTel: +46 8-230225                  Fax: +46 8-230231\n"},{"id":"59576","messageId":"7vhcjr2lte.fsf@gitster.siamese.dyndns.org","threadId":"10822","inReplyTo":"47382C84.50408@op5.se","subject":"Re: git diff woes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-12T21:30:53Z","receivedAt":"2007-11-12T21:30:53Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andreas Ericsson <ae@op5.se> writes:\n\n> In the check_ntpd.c program, there is no bug. I found the git diff output\n> surprising, so I reported it.\n\nThis is what I get from \"GNU diff -pu\" which makes me surpried\nthat anybody finds \"git diff\" hunk header surprising.  Notice\nthat hunk at line 84.\n\n--- read-cache.c\t2007-11-12 12:08:00.000000000 -0800\n+++ read-cache.c+\t2007-11-12 12:07:54.000000000 -0800\n@@ -60,7 +60,7 @@ static int ce_compare_data(struct cache_\n \treturn match;\n }\n \n-static int ce_compare_link(struct cache_entry *ce, size_t expected_size)\n+static int ce_compare_lonk(struct cache_entry *ce, size_t expected_size)\n {\n \tint match = -1;\n \tchar *target;\n@@ -84,7 +84,7 @@ static int ce_compare_link(struct cache_\n \t\tmatch = memcmp(buffer, target, size);\n \tfree(buffer);\n \tfree(target);\n-\treturn match;\n+\treturn match + 0;\n }\n \n static int ce_compare_gitlink(struct cache_entry *ce)\n"},{"id":"59600","messageId":"4738E9E6.2040001@op5.se","threadId":"10822","inReplyTo":"7vhcjr2lte.fsf@gitster.siamese.dyndns.org","subject":"Re: git diff woes","fromName":"Andreas Ericsson","fromEmail":"ae@op5.se","sentAt":"2007-11-13T00:03:50Z","receivedAt":"2007-11-13T00:03:50Z","isPatch":false,"sender":{"key":"ae@op5.se","avatar":"https://gravatar.com/avatar/426e89595c75a8f5252dd0c989e5fabe5bcac616e68557427ad9aef6b0ca342a?d=mp&s=160"},"body":"Junio C Hamano wrote:\n> Andreas Ericsson <ae@op5.se> writes:\n> \n>> In the check_ntpd.c program, there is no bug. I found the git diff output\n>> surprising, so I reported it.\n> \n> This is what I get from \"GNU diff -pu\" which makes me surpried\n> that anybody finds \"git diff\" hunk header surprising.  Notice\n> that hunk at line 84.\n> \n> --- read-cache.c\t2007-11-12 12:08:00.000000000 -0800\n> +++ read-cache.c+\t2007-11-12 12:07:54.000000000 -0800\n> @@ -60,7 +60,7 @@ static int ce_compare_data(struct cache_\n>  \treturn match;\n>  }\n>  \n> -static int ce_compare_link(struct cache_entry *ce, size_t expected_size)\n> +static int ce_compare_lonk(struct cache_entry *ce, size_t expected_size)\n>  {\n>  \tint match = -1;\n>  \tchar *target;\n> @@ -84,7 +84,7 @@ static int ce_compare_link(struct cache_\n>  \t\tmatch = memcmp(buffer, target, size);\n>  \tfree(buffer);\n>  \tfree(target);\n> -\treturn match;\n> +\treturn match + 0;\n>  }\n>  \n>  static int ce_compare_gitlink(struct cache_entry *ce)\n\n\nI notice it, and I don't like it. I guess I'm just used to git being\nsmarter than their GNU tool equivalents, especially since it only ever\napplies patches in full.\n\nI have a patch ready to make it configurable but it lacks doc updates\nand tests, so I'll send it tomorrow morning when I've had time to\nfiddle a bit with that.\n\n-- \nAndreas Ericsson                   andreas.ericsson@op5.se\nOP5 AB                             www.op5.se\nTel: +46 8-230225                  Fax: +46 8-230231\n"},{"id":"59604","messageId":"Pine.LNX.4.64.0711130053090.4362@racer.site","threadId":"10822","inReplyTo":"4738E9E6.2040001@op5.se","subject":"Re: git diff woes","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-11-13T00:59:41Z","receivedAt":"2007-11-13T00:59:41Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 13 Nov 2007, Andreas Ericsson wrote:\n\n> Junio C Hamano wrote:\n> > Andreas Ericsson <ae@op5.se> writes:\n> > \n> > > In the check_ntpd.c program, there is no bug. I found the git diff \n> > > output surprising, so I reported it.\n> > \n> > This is what I get from \"GNU diff -pu\" which makes me surpried\n> > that anybody finds \"git diff\" hunk header surprising.  Notice\n> > that hunk at line 84.\n> > \n> > --- read-cache.c\t2007-11-12 12:08:00.000000000 -0800\n> > +++ read-cache.c+\t2007-11-12 12:07:54.000000000 -0800\n> > @@ -60,7 +60,7 @@ static int ce_compare_data(struct cache_\n> >  \treturn match;\n> >  }\n> >  -static int ce_compare_link(struct cache_entry *ce, size_t expected_size)\n> > +static int ce_compare_lonk(struct cache_entry *ce, size_t expected_size)\n> >  {\n> >  \tint match = -1;\n> >  \tchar *target;\n> > @@ -84,7 +84,7 @@ static int ce_compare_link(struct cache_\n> >  \t\tmatch = memcmp(buffer, target, size);\n> >  \tfree(buffer);\n> >  \tfree(target);\n> > -\treturn match;\n> > +\treturn match + 0;\n> >  }\n> >   static int ce_compare_gitlink(struct cache_entry *ce)\n> \n> \n> I notice it, and I don't like it. I guess I'm just used to git being\n> smarter than their GNU tool equivalents, especially since it only ever\n> applies patches in full.\n\nI still think the existing behaviour is reasonable.  When I read a diff \n(and remember, the hunk headers are _only_ there for the reviewer's \npleasure), the function names are a hint for _me_ where to look, and which \nis the context, in my existing, _original_ file.\n\nThat is, unless I have already applied the patch, and am looking for the \nreverse patch.  And, lo and behold, the reverse patch generated by \ngit-diff really shows the now-current function name!\n\nSo IMO \"fixing\" this behaviour would be a regression.\n\nCiao,\nDscho\n"},{"id":"59607","messageId":"buomytin9dz.fsf@dhapc248.dev.necel.com","threadId":"10822","inReplyTo":"4738E9E6.2040001@op5.se","subject":"Re: git diff woes","fromName":"Miles Bader","fromEmail":"miles.bader@necel.com","sentAt":"2007-11-13T02:53:44Z","receivedAt":"2007-11-13T02:53:44Z","isPatch":false,"sender":{"key":"miles.bader@necel.com","avatar":"https://gravatar.com/avatar/be062d4050eb88e04229cbdb60f803e1bd647923a015996c2439e76f23e336a7?d=mp&s=160"},"body":"Andreas Ericsson <ae@op5.se> writes:\n> I notice it, and I don't like it. I guess I'm just used to git being\n> smarter than their GNU tool equivalents, especially since it only ever\n> applies patches in full.\n\nIt's not at all obvious that this behavior is actually wrong -- it seems\nperfectly reasonable to use either old or new text for the hunk headers.\n\nIt hardly matters really, since that particular output is just \"useful\nnoise\" to provide a bit of helpful context for human readers, and humans\n(unlike programs) are notoriously good at not being bothered by such\nthings.  Er, well most humans anyway.\n\n-Miles\n\n-- \nAmericans are broad-minded people.  They'll accept the fact that a person can\nbe an alcoholic, a dope fiend, a wife beater, and even a newspaperman, but if a\nman doesn't drive, there is something wrong with him.  -- Art Buchwald\n"},{"id":"59625","messageId":"473954F8.8070908@op5.se","threadId":"10822","inReplyTo":"buomytin9dz.fsf@dhapc248.dev.necel.com","subject":"Re: git diff woes","fromName":"Andreas Ericsson","fromEmail":"ae@op5.se","sentAt":"2007-11-13T07:40:40Z","receivedAt":"2007-11-13T07:40:40Z","isPatch":false,"sender":{"key":"ae@op5.se","avatar":"https://gravatar.com/avatar/426e89595c75a8f5252dd0c989e5fabe5bcac616e68557427ad9aef6b0ca342a?d=mp&s=160"},"body":"Miles Bader wrote:\n> Andreas Ericsson <ae@op5.se> writes:\n>> I notice it, and I don't like it. I guess I'm just used to git being\n>> smarter than their GNU tool equivalents, especially since it only ever\n>> applies patches in full.\n> \n> It's not at all obvious that this behavior is actually wrong -- it seems\n> perfectly reasonable to use either old or new text for the hunk headers.\n> \n\nRight, which is why I've made it configurable.\n\n> It hardly matters really, since that particular output is just \"useful\n> noise\" to provide a bit of helpful context for human readers, and humans\n> (unlike programs) are notoriously good at not being bothered by such\n> things.  Er, well most humans anyway.\n> \n\nI wouldn't have reacted either, except that this time someone asked me to\nreview a branch early in the morning because he had introduced a bug in the\nprocess, and the hunk header information made me assume the wrong hunk of\nthe patch was the culprit.\n\nOn the one hand, it wouldn't have been so much of a problem if the developer\nin question would have followed my suggestion of committing small and making\nsure the commit message describes everything that's done. On the other hand,\na tool fooling a human isn't a good thing either, even if said human is not\nreally in shape for using said tool.\n\nGranted, the new form can still fool people, but for archeology excursions\nI think it's definitely right to use the \"new\" funcname in the hunk header.\n\n-- \nAndreas Ericsson                   andreas.ericsson@op5.se\nOP5 AB                             www.op5.se\nTel: +46 8-230225                  Fax: +46 8-230231\n"},{"id":"59636","messageId":"47396B4C.6070406@op5.se","threadId":"10822","inReplyTo":"473954F8.8070908@op5.se","subject":"[PATCH] diffcore: Allow users to decide what funcname to use","fromName":"Andreas Ericsson","fromEmail":"ae@op5.se","sentAt":"2007-11-13T09:15:56Z","receivedAt":"2007-11-13T09:15:56Z","isPatch":true,"sender":{"key":"ae@op5.se","avatar":"https://gravatar.com/avatar/426e89595c75a8f5252dd0c989e5fabe5bcac616e68557427ad9aef6b0ca342a?d=mp&s=160"},"body":"Andreas Ericsson wrote:\n> Miles Bader wrote:\n>> Andreas Ericsson <ae@op5.se> writes:\n>>> I notice it, and I don't like it. I guess I'm just used to git being\n>>> smarter than their GNU tool equivalents, especially since it only ever\n>>> applies patches in full.\n>>\n>> It's not at all obvious that this behavior is actually wrong -- it seems\n>> perfectly reasonable to use either old or new text for the hunk headers.\n>>\n> \n> Right, which is why I've made it configurable.\n> \n\nMy git hacking has been stalled at the office for now, and I'm swanked at\nhome since my girlfriend just moved in and brought temporary pandemonium\nwith her. Here's what I've got now. It passes all tests, but there are no\nnew ones added. Documentation also needs updating.\n\nExtract with\n\tsed -n -e /^#CUTSTART/,/^#CUTEND/p -e /^#/d\n\n#CUTSTART---%<---%<---%<---\nFrom: Andreas Ericsson <ae@op5.se>\nDate: Tue, 13 Nov 2007 09:47:43 +0100\nSubject: [PATCH] diffcore: Allow users to decide what funcname to use\n\nThe function name being printed with the header of each\nhunk is fetched from the \"old\" file today. Since git by\ndefault applies patches either in full or not at all it's\narguably more correct to use the function from the \"new\"\nfile, at least when manually reviewing commits.\n\nI stumbled upon this hunk when reviewing a series of\ncommits which caused the resulting code to segfault\nunder certain circumstances. Several hunks before, the\nfunction declaration was changed and \"status\" was now\ndeclared as an auto variable of type \"int\". The hunk\nlooks obviously bogus, and since I wasn't properly\nawake, I reported this hunk to be the bogus one.\n\n  @@ -583,75 +346,100 @@ double jitter_request(int *status){\n     context\n     context\n     if(!syncsource_found){\n  -    *status = STATE_UNKNOWN;\n  +    status = STATE_WARNING;\n       if(verbose) printf(\"warning: no sync source found\\n\")\n     }\n\nThis is what GNU \"diff -p\" would have reported under the\nsame circumstances, but GNU diff has no notion of version\ncontrol, and as such will not know if it's being used on\ncontent where the patch by definition will apply in full.\n\nGit can be smarter than that, and imo it should. This\npatch lets the diffcore grok a new configuration variable,\n\"diff.funcnames\", which can be set to \"new\", \"old\", or a\nboolean value, which will cause it to be \"old\" (for 'true')\nand 'none' (for 'false').\n\nSigned-off-by: Andreas Ericsson <ae@op5.se>\n---\n diff.c           |   20 ++++++++++++++++++--\n diffcore-break.c |    4 ++--\n xdiff/xdiff.h    |    3 ++-\n xdiff/xemit.c    |    7 ++++++-\n 4 files changed, 28 insertions(+), 6 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 6bb902f..057bba8 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -20,6 +20,7 @@\n static int diff_detect_rename_default;\n static int diff_rename_limit_default = 100;\n static int diff_use_color_default;\n+static unsigned long xdl_emit_flags = XDL_EMIT_FUNCNAMES;\n int diff_auto_refresh_index = 1;\n \n static char diff_colors[][COLOR_MAXLEN] = {\n@@ -178,6 +179,20 @@ int git_diff_ui_config(const char *var, const char *value)\n                color_parse(value, var, diff_colors[slot]);\n                return 0;\n        }\n+       if (!prefixcmp(var, \"diff.funcnames\")) {\n+               if (!value)\n+                       xdl_emit_flags = XDL_EMIT_COMMON;\n+               else if (!strcasecmp(value, \"new\"))\n+                       xdl_emit_flags = XDL_EMIT_FUNCNAMES_NEW;\n+               else if (!strcasecmp(value, \"old\") || !strcasecmp(value, \"default\"))\n+                       xdl_emit_flags = XDL_EMIT_FUNCNAMES;\n+               else {\n+                       if (git_config_bool(var, value))\n+                               xdl_emit_flags = XDL_EMIT_FUNCNAMES;\n+                       else\n+                               xdl_emit_flags = XDL_EMIT_COMMON;\n+               }\n+       }\n \n        return git_default_config(var, value);\n }\n@@ -1332,7 +1347,8 @@ static void builtin_diff(const char *name_a,\n                ecbdata.found_changesp = &o->found_changes;\n                xpp.flags = XDF_NEED_MINIMAL | o->xdl_opts;\n                xecfg.ctxlen = o->context;\n-               xecfg.flags = XDL_EMIT_FUNCNAMES;\n+               xecfg.flags = xdl_emit_flags;\n+\n                if (funcname_pattern)\n                        xdiff_set_find_func(&xecfg, funcname_pattern);\n                if (!diffopts)\n@@ -2844,7 +2860,7 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1)\n \n                xpp.flags = XDF_NEED_MINIMAL;\n                xecfg.ctxlen = 3;\n-               xecfg.flags = XDL_EMIT_FUNCNAMES;\n+               xecfg.flags = xdl_emit_flags;\n                ecb.outf = xdiff_outf;\n                ecb.priv = &data;\n                xdl_diff(&mf1, &mf2, &xpp, &xecfg, &ecb);\ndiff --git a/diffcore-break.c b/diffcore-break.c\nindex c71a226..048ec25 100644\n--- a/diffcore-break.c\n+++ b/diffcore-break.c\n@@ -257,8 +257,8 @@ void diffcore_merge_broken(void)\n                if (!p)\n                        /* we already merged this with its peer */\n                        continue;\n-               else if (p->broken_pair &&\n-                        !strcmp(p->one->path, p->two->path)) {\n+\n+               if (p->broken_pair && !strcmp(p->one->path, p->two->path)) {\n                        /* If the peer also survived rename/copy, then\n                         * we merge them back together.\n                         */\ndiff --git a/xdiff/xdiff.h b/xdiff/xdiff.h\nindex c00ddaa..326e1df 100644\n--- a/xdiff/xdiff.h\n+++ b/xdiff/xdiff.h\n@@ -41,7 +41,8 @@ extern \"C\" {\n \n #define XDL_EMIT_FUNCNAMES (1 << 0)\n #define XDL_EMIT_COMMON (1 << 1)\n-\n+#define XDL_EMIT_FUNCNAMES_NEW (1 << 2)\n+\n #define XDL_MMB_READONLY (1 << 0)\n \n #define XDL_MMF_ATOMIC (1 << 0)\ndiff --git a/xdiff/xemit.c b/xdiff/xemit.c\nindex d3d9c84..51dd085 100644\n--- a/xdiff/xemit.c\n+++ b/xdiff/xemit.c\n@@ -149,7 +149,12 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,\n                 * Emit current hunk header.\n                 */\n \n-               if (xecfg->flags & XDL_EMIT_FUNCNAMES) {\n+               if (xecfg->flags & XDL_EMIT_FUNCNAMES_NEW) {\n+                       xdl_find_func(&xe->xdf2, s2, funcbuf,\n+                                     sizeof(funcbuf), &funclen,\n+                                     ff, xecfg->find_func_priv);\n+               }\n+               else if (xecfg->flags & XDL_EMIT_FUNCNAMES) {\n                        xdl_find_func(&xe->xdf1, s1, funcbuf,\n                                      sizeof(funcbuf), &funclen,\n                                      ff, xecfg->find_func_priv);\n-- \n1.5.3.5.1527.g6161\n#CUTEND---%<---%<---%<---\n\n-- \nAndreas Ericsson                   andreas.ericsson@op5.se\nOP5 AB                             www.op5.se\nTel: +46 8-230225                  Fax: +46 8-230231\n"},{"id":"59644","messageId":"fhbsor$ebf$1@ger.gmane.org","threadId":"10822","inReplyTo":"47396B4C.6070406@op5.se","subject":"Re: [PATCH] diffcore: Allow users to decide what funcname to use","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2007-11-13T10:03:07Z","receivedAt":"2007-11-13T10:03:07Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Andreas Ericsson wrote:\n\n> Git can be smarter than that, and imo it should. This\n> patch lets the diffcore grok a new configuration variable,\n> \"diff.funcnames\", which can be set to \"new\", \"old\", or a\n> boolean value, which will cause it to be \"old\" (for 'true')\n> and 'none' (for 'false').\n\nWouldn't it be better to use existing 'diff driver' infrastructure\nfor this? See \"Defining a custom hunk-header\" section in\ngitattributes(5).\n\nOn the other hand... no.\n\n-- \nJakub Narebski\nWarsaw, Poland\nShadeHawk on #git\n"},{"id":"59645","messageId":"47397763.6080307@op5.se","threadId":"10822","inReplyTo":"fhbsor$ebf$1@ger.gmane.org","subject":"Re: [PATCH] diffcore: Allow users to decide what funcname to use","fromName":"Andreas Ericsson","fromEmail":"ae@op5.se","sentAt":"2007-11-13T10:07:31Z","receivedAt":"2007-11-13T10:07:31Z","isPatch":true,"sender":{"key":"ae@op5.se","avatar":"https://gravatar.com/avatar/426e89595c75a8f5252dd0c989e5fabe5bcac616e68557427ad9aef6b0ca342a?d=mp&s=160"},"body":"Jakub Narebski wrote:\n> Andreas Ericsson wrote:\n> \n>> Git can be smarter than that, and imo it should. This\n>> patch lets the diffcore grok a new configuration variable,\n>> \"diff.funcnames\", which can be set to \"new\", \"old\", or a\n>> boolean value, which will cause it to be \"old\" (for 'true')\n>> and 'none' (for 'false').\n> \n> Wouldn't it be better to use existing 'diff driver' infrastructure\n> for this? See \"Defining a custom hunk-header\" section in\n> gitattributes(5).\n> \n> On the other hand... no.\n> \n\nIt's impossible to do that, since that driver will only ever be\nfed the \"old\" file with the old code. I'm guessing you noticed\nthat yourself, so just explaining in case anyone else wonders.\n\n-- \nAndreas Ericsson                   andreas.ericsson@op5.se\nOP5 AB                             www.op5.se\nTel: +46 8-230225                  Fax: +46 8-230231\n"}]}