{"thread":{"id":"42572","subject":"[BUG-ish] diff compaction heuristic false positive","startedAt":"2016-06-16T02:19:50Z","lastAt":"2016-06-16T02:19:52Z","messageCount":16,"participants":["Jeff King","Michael Haggerty","Junio C Hamano","Stefan Beller","Jacob Keller"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"288868","messageId":"20160610075043.GA13411@sigill.intra.peff.net","threadId":"42572","inReplyTo":null,"subject":"[BUG-ish] diff compaction heuristic false positive","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-10T07:50:43Z","receivedAt":"2016-06-16T02:19:50Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I found a false positive with the new compaction heuristic in v2.9:\n\n-- >8 --\n\n# start with a simple file\ncat >file.rb <<\\EOF\ndef foo\n  do_foo_stuff()\n\n  common_ending()\nend\nEOF\n\ngit add file.rb\ngit commit -m base\n\n# and then add another function with a similar ending\ncat >>file.rb <<\\EOF\n\ndef bar\n  do_bar_stuff()\n\n  common_ending()\nend\nEOF\n\n-- 8< --\n\nI get this rather unfortunate diff:\n\n    $ git diff\n    diff --git a/file.rb b/file.rb\n    index bd9d1cb..67fbeba 100644\n    --- a/file.rb\n    +++ b/file.rb\n    @@ -1,5 +1,11 @@\n     def foo\n       do_foo_stuff()\n     \n    +  common_ending()\n    +end\n    +\n    +def bar\n    +  do_bar_stuff()\n    +\n       common_ending()\n     end\n\nbut without the compaction heuristic (or with an older git), I get:\n\n    $ git -c diff.compactionHeuristic=false diff\n    diff --git a/file.rb b/file.rb\n    index bd9d1cb..67fbeba 100644\n    --- a/file.rb\n    +++ b/file.rb\n    @@ -3,3 +3,9 @@ def foo\n     \n       common_ending()\n     end\n    +\n    +def bar\n    +  do_bar_stuff()\n    +\n    +  common_ending()\n    +end\n\n:( The problem is that the common bits are separated from the\ninteresting bits by a blank line. This is simplified from a real-world\nexample, but I think you could come up with the same example in C, like:\n\n  if (foo) {\n        do_foo();\n\n        something_else();\n  }\n\n-Peff\n"},{"id":"288878","messageId":"20160610083102.GA14192@sigill.intra.peff.net","threadId":"42572","inReplyTo":"20160610075043.GA13411@sigill.intra.peff.net","subject":"Re: [BUG-ish] diff compaction heuristic false positive","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-10T08:31:02Z","receivedAt":"2016-06-16T02:19:50Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 10, 2016 at 03:50:43AM -0400, Jeff King wrote:\n\n> I found a false positive with the new compaction heuristic in v2.9:\n> [...]\n\nAnd by the way, this is less \"hey neat, I found a case\" and more \"wow,\nthis is a lot worse than I thought\".\n\nI diffed the old and new output for the top 10,000 commits in this\nparticular ruby code base. There were 45 commits with changed diffs.\nSpot-checking them manually, a little over 1/3 of them featured this bad\npattern. The others looked like strict improvements.\n\nThat's a lot worse than the outcomes we saw on other code bases earlier.\n1/3 bad is still a net improvement, so I dunno. Is this worth worrying\nabout? Should we bring back the documentation for the knob to disable\nit? Should we consider making it tunable via gitattributes?\n\nI don't think that last one really helps; the good cases _and_ the bad\nones are both in ruby code (though certainly the C code we looked at\nearlier was all good).\n\nIt may also be possible to make it Just Work by using extra information\nlike indentation. I haven't thought hard enough about that to say.\n\n-Peff\n"},{"id":"288879","messageId":"575A7AD1.50604@alum.mit.edu","threadId":"42572","inReplyTo":"20160610075043.GA13411@sigill.intra.peff.net","subject":"Re: [BUG-ish] diff compaction heuristic false positive","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-06-10T08:31:13Z","receivedAt":"2016-06-16T02:19:50Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 06/10/2016 09:50 AM, Jeff King wrote:\n> I found a false positive with the new compaction heuristic in v2.9:\n> [...]\n> I get this rather unfortunate diff:\n> \n>     $ git diff\n>     diff --git a/file.rb b/file.rb\n>     index bd9d1cb..67fbeba 100644\n>     --- a/file.rb\n>     +++ b/file.rb\n>     @@ -1,5 +1,11 @@\n>      def foo\n>        do_foo_stuff()\n>      \n>     +  common_ending()\n>     +end\n>     +\n>     +def bar\n>     +  do_bar_stuff()\n>     +\n>        common_ending()\n>      end\n\nI've often thought that indentation would be a good, fairly universal\nsignal for diff to use when deciding how to slide hunks around. Most\nsource code is indented in a way that shows its structure.\n\nI propose the following heuristic:\n\n* Prefer to start and end hunks following lines with the least\n  indentation.\n\n* Define the \"indentation\" of a blank line to be the indentation of\n  the previous non-blank line minus epsilon.\n\n* In the case of a tie, prefer to slide the hunk down as far as\n  possible.\n\nFor the case above, the indentations for the candidate \"before-the-hunk\"\nlines and the resulting hunk would be\n\n>      def foo\n> 2      do_foo_stuff()\n> 2-ε\n> 2      common_ending()\n> 0    end\n> 0-ε +\n>     +def bar\n>     +  do_bar_stuff()\n>     +\n>     +  common_ending()\n>     +end\n\nI haven't tried testing this heuristic systematically but I have the\nfeeling that it would be pretty effective and yet quite easy to implement.\n\nMichael\n"},{"id":"288880","messageId":"20160610084149.GA14592@sigill.intra.peff.net","threadId":"42572","inReplyTo":"575A7AD1.50604@alum.mit.edu","subject":"Re: [BUG-ish] diff compaction heuristic false positive","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-10T08:41:50Z","receivedAt":"2016-06-16T02:19:50Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 10, 2016 at 10:31:13AM +0200, Michael Haggerty wrote:\n\n> I've often thought that indentation would be a good, fairly universal\n> signal for diff to use when deciding how to slide hunks around. Most\n> source code is indented in a way that shows its structure.\n> \n> I propose the following heuristic:\n> \n> * Prefer to start and end hunks following lines with the least\n>   indentation.\n> \n> * Define the \"indentation\" of a blank line to be the indentation of\n>   the previous non-blank line minus epsilon.\n> \n> * In the case of a tie, prefer to slide the hunk down as far as\n>   possible.\n\nHmm. That might help this case, but the original motivation for this\nheuristic was something like:\n\n  ##\n  # foo\n  def foo\n    something\n  end\n\n  ##\n  # bar\n  def bar\n    something_else\n  end\n\nwhere we add the first function above the second. We end up with:\n\ndiff --git a/file.rb b/file.rb\nindex 1f9b151..f991c76 100644\n--- a/file.rb\n+++ b/file.rb\n@@ -1,4 +1,10 @@\n ##\n+# foo\n+def foo\n+  something\n+end\n+\n+##\n # bar\n def bar\n   something else\n\nI.e., crediting the \"##\" to the wrong spot (or in C, the \"/*\"). I don't\nthink indentation helps us there (sliding-up would, but like\nsliding-down, it just depends on the order of the hunks).\n\nSo I agree that adding indentation to the mix might help, but I don't\nthink it can replace this heuristic.\n\n-Peff\n"},{"id":"288889","messageId":"575A9DE5.2020401@alum.mit.edu","threadId":"42572","inReplyTo":"20160610084149.GA14592@sigill.intra.peff.net","subject":"Re: [BUG-ish] diff compaction heuristic false positive","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-06-10T11:00:53Z","receivedAt":"2016-06-16T02:19:50Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 06/10/2016 10:41 AM, Jeff King wrote:\n> On Fri, Jun 10, 2016 at 10:31:13AM +0200, Michael Haggerty wrote:\n> \n>> I've often thought that indentation would be a good, fairly universal\n>> signal for diff to use when deciding how to slide hunks around. Most\n>> source code is indented in a way that shows its structure.\n>>\n>> I propose the following heuristic:\n>>\n>> * Prefer to start and end hunks following lines with the least\n>>   indentation.\n>>\n>> * Define the \"indentation\" of a blank line to be the indentation of\n>>   the previous non-blank line minus epsilon.\n>>\n>> * In the case of a tie, prefer to slide the hunk down as far as\n>>   possible.\n> \n> Hmm. That might help this case, but the original motivation for this\n> heuristic was something like:\n> \n>   ##\n>   # foo\n>   def foo\n>     something\n>   end\n> \n>   ##\n>   # bar\n>   def bar\n>     something_else\n>   end\n> \n> where we add the first function above the second. We end up with:\n> \n> diff --git a/file.rb b/file.rb\n> index 1f9b151..f991c76 100644\n> --- a/file.rb\n> +++ b/file.rb\n> @@ -1,4 +1,10 @@\n>  ##\n> +# foo\n> +def foo\n> +  something\n> +end\n> +\n> +##\n>  # bar\n>  def bar\n>    something else\n> \n> I.e., crediting the \"##\" to the wrong spot (or in C, the \"/*\"). I don't\n> think indentation helps us there (sliding-up would, but like\n> sliding-down, it just depends on the order of the hunks).\n> \n> So I agree that adding indentation to the mix might help, but I don't\n> think it can replace this heuristic.\n\nUmmm, I think the indentation heuristic would work with that example,\ntoo, as long as we consider there to be an imaginary line \"0\" of the\nfile (i.e., preceding the first real line) that has an indentation of -ε.\n\nMichael\n"},{"id":"288930","messageId":"xmqqvb1hf35y.fsf@gitster.mtv.corp.google.com","threadId":"42572","inReplyTo":"20160610083102.GA14192@sigill.intra.peff.net","subject":"Re: [BUG-ish] diff compaction heuristic false positive","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-10T15:56:41Z","receivedAt":"2016-06-16T02:19:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Fri, Jun 10, 2016 at 03:50:43AM -0400, Jeff King wrote:\n>\n>> I found a false positive with the new compaction heuristic in v2.9:\n>> [...]\n>\n> And by the way, this is less \"hey neat, I found a case\" and more \"wow,\n> this is a lot worse than I thought\".\n>\n> I diffed the old and new output for the top 10,000 commits in this\n> particular ruby code base. There were 45 commits with changed diffs.\n> Spot-checking them manually, a little over 1/3 of them featured this bad\n> pattern. The others looked like strict improvements.\n>\n> That's a lot worse than the outcomes we saw on other code bases earlier.\n> 1/3 bad is still a net improvement, so I dunno. Is this worth worrying\n> about? Should we bring back the documentation for the knob to disable\n> it? Should we consider making it tunable via gitattributes?\n>\n> I don't think that last one really helps; the good cases _and_ the bad\n> ones are both in ruby code (though certainly the C code we looked at\n> earlier was all good).\n>\n> It may also be possible to make it Just Work by using extra information\n> like indentation. I haven't thought hard enough about that to say.\n>\n> -Peff\n\nI recall saying \"we'd end up being better in some and worse in\nothers\" at the very beginning.  How about toggling the default back\nfor the upcoming release, keeping the experimentation knob in the\ncode, and try different heuristics like the \"indentation\" during the\nnext cycle?\n"},{"id":"288931","messageId":"CAGZ79kZLT8AfmWTrrW+a-v7aXw5sm68P2H=vT7QZr2hj4Z2gDA@mail.gmail.com","threadId":"42572","inReplyTo":"xmqqvb1hf35y.fsf@gitster.mtv.corp.google.com","subject":"Re: [BUG-ish] diff compaction heuristic false positive","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-06-10T16:25:00Z","receivedAt":"2016-06-16T02:19:51Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jun 10, 2016 at 8:56 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jeff King <peff@peff.net> writes:\n>\n>> On Fri, Jun 10, 2016 at 03:50:43AM -0400, Jeff King wrote:\n>>\n>>> I found a false positive with the new compaction heuristic in v2.9:\n>>> [...]\n>>\n>> And by the way, this is less \"hey neat, I found a case\" and more \"wow,\n>> this is a lot worse than I thought\".\n>>\n>> I diffed the old and new output for the top 10,000 commits in this\n>> particular ruby code base. There were 45 commits with changed diffs.\n>> Spot-checking them manually, a little over 1/3 of them featured this bad\n>> pattern. The others looked like strict improvements.\n>>\n>> That's a lot worse than the outcomes we saw on other code bases earlier.\n>> 1/3 bad is still a net improvement, so I dunno. Is this worth worrying\n>> about? Should we bring back the documentation for the knob to disable\n>> it? Should we consider making it tunable via gitattributes?\n>>\n>> I don't think that last one really helps; the good cases _and_ the bad\n>> ones are both in ruby code (though certainly the C code we looked at\n>> earlier was all good).\n>>\n>> It may also be possible to make it Just Work by using extra information\n>> like indentation. I haven't thought hard enough about that to say.\n>>\n>> -Peff\n>\n> I recall saying \"we'd end up being better in some and worse in\n> others\" at the very beginning.  How about toggling the default back\n> for the upcoming release, keeping the experimentation knob in the\n> code, and try different heuristics like the \"indentation\" during the\n> next cycle?\n\nSure. I thought about for a while now and by now I agree with Junio.\nNo matter what kind of heuristic we can come up with it is easy to construct\na counter example.\n\nThat said, let's try the indentation thing, though I suspect\none of the early motivating examples (an excerpt from a  kernel config file)\nwould not do well with it, as it had not an indentation scheme as programming\nlanguages do.\n\nThanks,\nStefan\n"},{"id":"288932","messageId":"CA+P7+xp=bTPiwRRTH=h7v5pV8+=he4+789_3PNz227mv1387MA@mail.gmail.com","threadId":"42572","inReplyTo":"CAGZ79kZLT8AfmWTrrW+a-v7aXw5sm68P2H=vT7QZr2hj4Z2gDA@mail.gmail.com","subject":"Re: [BUG-ish] diff compaction heuristic false positive","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-06-10T16:29:43Z","receivedAt":"2016-06-16T02:19:51Z","isPatch":false,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Fri, Jun 10, 2016 at 9:25 AM, Stefan Beller <sbeller@google.com> wrote:\n> On Fri, Jun 10, 2016 at 8:56 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Jeff King <peff@peff.net> writes:\n>>\n>>> On Fri, Jun 10, 2016 at 03:50:43AM -0400, Jeff King wrote:\n>>>\n>>>> I found a false positive with the new compaction heuristic in v2.9:\n>>>> [...]\n>>>\n>>> And by the way, this is less \"hey neat, I found a case\" and more \"wow,\n>>> this is a lot worse than I thought\".\n>>>\n>>> I diffed the old and new output for the top 10,000 commits in this\n>>> particular ruby code base. There were 45 commits with changed diffs.\n>>> Spot-checking them manually, a little over 1/3 of them featured this bad\n>>> pattern. The others looked like strict improvements.\n>>>\n>>> That's a lot worse than the outcomes we saw on other code bases earlier.\n>>> 1/3 bad is still a net improvement, so I dunno. Is this worth worrying\n>>> about? Should we bring back the documentation for the knob to disable\n>>> it? Should we consider making it tunable via gitattributes?\n>>>\n>>> I don't think that last one really helps; the good cases _and_ the bad\n>>> ones are both in ruby code (though certainly the C code we looked at\n>>> earlier was all good).\n>>>\n>>> It may also be possible to make it Just Work by using extra information\n>>> like indentation. I haven't thought hard enough about that to say.\n>>>\n>>> -Peff\n>>\n>> I recall saying \"we'd end up being better in some and worse in\n>> others\" at the very beginning.  How about toggling the default back\n>> for the upcoming release, keeping the experimentation knob in the\n>> code, and try different heuristics like the \"indentation\" during the\n>> next cycle?\n>\n> Sure. I thought about for a while now and by now I agree with Junio.\n> No matter what kind of heuristic we can come up with it is easy to construct\n> a counter example.\n>\n> That said, let's try the indentation thing, though I suspect\n> one of the early motivating examples (an excerpt from a  kernel config file)\n> would not do well with it, as it had not an indentation scheme as programming\n> languages do.\n>\n> Thanks,\n> Stefan\n\nI think we could use the indentation trick and it might help in this\ncase. I agree, let's disable this for this cycle and experiment in the\nnext one. Good catch, Peff.\n\nAs others have said you will always be able to produce counter\nexamples, that's the nature of heuristics. The idea is to see if we\ncan come up with something simple that mostly improves the output,\neven if sometimes it might have a negative impact on the outputs. But\nI think we should avoid changing behavior unless it's mostly an\nimprovement.\n\nRegards,\nJake\n"},{"id":"288935","messageId":"xmqqeg84gbex.fsf_-_@gitster.mtv.corp.google.com","threadId":"42572","inReplyTo":"CA+P7+xp=bTPiwRRTH=h7v5pV8+=he4+789_3PNz227mv1387MA@mail.gmail.com","subject":"Re* [BUG-ish] diff compaction heuristic false positive","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-10T18:13:10Z","receivedAt":"2016-06-16T02:19:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller <jacob.keller@gmail.com> writes:\n\n> I think we could use the indentation trick and it might help in this\n> case. I agree, let's disable this for this cycle and experiment in the\n> next one. Good catch, Peff.\n>\n> As others have said you will always be able to produce counter\n> examples, that's the nature of heuristics. The idea is to see if we\n> can come up with something simple that mostly improves the output,\n> even if sometimes it might have a negative impact on the outputs. But\n> I think we should avoid changing behavior unless it's mostly an\n> improvement.\n\nOK, let's do this then for the upcoming release for now.  I am\ntempted to flip it back on after the release in 'next', so that\ndevelopers would be exposed to the heuristic by default, though.\n\n-- >8 --\nSubject: [PATCH] diff: disable compaction heuristic for now\n\nhttp://lkml.kernel.org/g/20160610075043.GA13411@sigill.intra.peff.net\nreports that a change to add a new \"function\" with common ending\nwith the existing one at the end of the file is shown like this:\n\n    def foo\n      do_foo_stuff()\n\n   +  common_ending()\n   +end\n   +\n   +def bar\n   +  do_bar_stuff()\n   +\n      common_ending()\n    end\n\nwhen the new heuristic is in use.  In reality, the change is to add\nthe blank line before \"def bar\" and everything below, which is what\nthe code without the new heuristic shows.\n\nDisable the heuristics by default and leave it as an experimental\nfeature for now.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n diff.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex 05ca3ce..9116d9d 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -25,7 +25,7 @@\n #endif\n \n static int diff_detect_rename_default;\n-static int diff_compaction_heuristic = 1;\n+static int diff_compaction_heuristic; /* experimental */\n static int diff_rename_limit_default = 400;\n static int diff_suppress_blank_empty;\n static int diff_use_color_default = -1;\n-- \n2.9.0-rc2-275-g493bdbb\n"},{"id":"288936","messageId":"CAGZ79kZpiPKLS_RFzROznQAE8iT35hV0QXknFMsVcDR7siLOTw@mail.gmail.com","threadId":"42572","inReplyTo":"xmqqeg84gbex.fsf_-_@gitster.mtv.corp.google.com","subject":"Re: Re* [BUG-ish] diff compaction heuristic false positive","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-06-10T18:21:34Z","receivedAt":"2016-06-16T02:19:51Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jun 10, 2016 at 11:13 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Subject: [PATCH] diff: disable compaction heuristic for now\n>\n> http://lkml.kernel.org/g/20160610075043.GA13411@sigill.intra.peff.net\n> reports that a change to add a new \"function\" with common ending\n> with the existing one at the end of the file is shown like this:\n>\n>     def foo\n>       do_foo_stuff()\n>\n>    +  common_ending()\n>    +end\n>    +\n>    +def bar\n>    +  do_bar_stuff()\n>    +\n>       common_ending()\n>     end\n>\n> when the new heuristic is in use.  In reality, the change is to add\n> the blank line before \"def bar\" and everything below, which is what\n> the code without the new heuristic shows.\n>\n> Disable the heuristics by default and leave it as an experimental\n> feature for now.\n>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n\nThanks,\nStefan\n"},{"id":"288989","messageId":"20160610203026.GA21464@sigill.intra.peff.net","threadId":"42572","inReplyTo":"xmqqeg84gbex.fsf_-_@gitster.mtv.corp.google.com","subject":"Re: Re* [BUG-ish] diff compaction heuristic false positive","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-10T20:30:26Z","receivedAt":"2016-06-16T02:19:52Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 10, 2016 at 11:13:10AM -0700, Junio C Hamano wrote:\n\n> Jacob Keller <jacob.keller@gmail.com> writes:\n> \n> > I think we could use the indentation trick and it might help in this\n> > case. I agree, let's disable this for this cycle and experiment in the\n> > next one. Good catch, Peff.\n> >\n> > As others have said you will always be able to produce counter\n> > examples, that's the nature of heuristics. The idea is to see if we\n> > can come up with something simple that mostly improves the output,\n> > even if sometimes it might have a negative impact on the outputs. But\n> > I think we should avoid changing behavior unless it's mostly an\n> > improvement.\n> \n> OK, let's do this then for the upcoming release for now.  I am\n> tempted to flip it back on after the release in 'next', so that\n> developers would be exposed to the heuristic by default, though.\n\nThanks for the patch, and I agree flipping it back on in \"next\" is a\ngood idea.\n\nIt may be that I am making a fuss over nothing. As you say, we always\nknew that it might have false positives. Mostly I was just surprised how\nfrequent they were in this example (I also wondered why the same pattern\ndid not trigger in the C code we looked at).\n\n> -- >8 --\n> Subject: [PATCH] diff: disable compaction heuristic for now\n\nLooks good.\n\nWe probably want a patch to the release notes to note that it's not on\nby default. And we may want to advertise the experimental knob so\nthat people actually try it (otherwise we won't get feedback from the\nmasses).\n\n-Peff\n"},{"id":"288992","messageId":"xmqqoa78epmt.fsf_-_@gitster.mtv.corp.google.com","threadId":"42572","inReplyTo":"20160610203026.GA21464@sigill.intra.peff.net","subject":"[PATCH v2] diff: disable compaction heuristic for now","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-10T20:48:58Z","receivedAt":"2016-06-16T02:19:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"http://lkml.kernel.org/g/20160610075043.GA13411@sigill.intra.peff.net\nreports that a change to add a new \"function\" with common ending\nwith the existing one at the end of the file is shown like this:\n\n    def foo\n      do_foo_stuff()\n\n   +  common_ending()\n   +end\n   +\n   +def bar\n   +  do_bar_stuff()\n   +\n      common_ending()\n    end\n\nwhen the new heuristic is in use.  In reality, the change is to add\nthe blank line before \"def bar\" and everything below, which is what\nthe code without the new heuristic shows.\n\nDisable the heuristics by default, and resurrect the documentation\nfor the option and the configuration variables, while clearly\nmarking the feature as still experimental.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n    Jeff King <peff@peff.net> writes:\n\n    > We probably want a patch to the release notes to note that it's not on\n    > by default. And we may want to advertise the experimental knob so\n    > that people actually try it (otherwise we won't get feedback from the\n    > masses).\n\n    OK, I think that is sensible.  The interdiff is not a strict\n    reversion of 77085a61 (diff: undocument the compaction heuristic\n    knobs for experimentation, 2016-05-02) but stresses that the\n    feature is off by default and is experimental.\n\n Documentation/diff-config.txt  | 5 +++++\n Documentation/diff-options.txt | 7 +++++++\n diff.c                         | 2 +-\n 3 files changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt\nindex 6eaa452..6fb70c5 100644\n--- a/Documentation/diff-config.txt\n+++ b/Documentation/diff-config.txt\n@@ -166,6 +166,11 @@ diff.tool::\n \n include::mergetools-diff.txt[]\n \n+diff.compactionHeuristic::\n+\tSet this option to `true` to enable an experimental heuristic that\n+\tshifts the hunk boundary in an attempt to make the resulting\n+\tpatch easier to read.\n+\n diff.algorithm::\n \tChoose a diff algorithm.  The variants are as follows:\n +\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 3ad6404..9baf1ad 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -63,6 +63,13 @@ ifndef::git-format-patch[]\n \tSynonym for `-p --raw`.\n endif::git-format-patch[]\n \n+--compaction-heuristic::\n+--no-compaction-heuristic::\n+\tThese are to help debugging and tuning an experimental\n+\theuristic (which is off by default) that shifts the hunk\n+\tboundary in an attempt to make the resulting patch easier\n+\tto read.\n+\n --minimal::\n \tSpend extra time to make sure the smallest possible\n \tdiff is produced.\ndiff --git a/diff.c b/diff.c\nindex 05ca3ce..9116d9d 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -25,7 +25,7 @@\n #endif\n \n static int diff_detect_rename_default;\n-static int diff_compaction_heuristic = 1;\n+static int diff_compaction_heuristic; /* experimental */\n static int diff_rename_limit_default = 400;\n static int diff_suppress_blank_empty;\n static int diff_use_color_default = -1;\n-- \n2.9.0-rc2-285-ge226c12\n"},{"id":"288993","messageId":"20160610205357.GA22470@sigill.intra.peff.net","threadId":"42572","inReplyTo":"xmqqoa78epmt.fsf_-_@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] diff: disable compaction heuristic for now","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-10T20:53:57Z","receivedAt":"2016-06-16T02:19:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 10, 2016 at 01:48:58PM -0700, Junio C Hamano wrote:\n\n>     Jeff King <peff@peff.net> writes:\n> \n>     > We probably want a patch to the release notes to note that it's not on\n>     > by default. And we may want to advertise the experimental knob so\n>     > that people actually try it (otherwise we won't get feedback from the\n>     > masses).\n> \n>     OK, I think that is sensible.  The interdiff is not a strict\n>     reversion of 77085a61 (diff: undocument the compaction heuristic\n>     knobs for experimentation, 2016-05-02) but stresses that the\n>     feature is off by default and is experimental.\n\nLooks good to me. Do we want to include in \"experimental\" that the\noption and command-line flag might go away in the future? I think\nprobably not. I do not mind supporting this forever (it _has_ shown its\nusefulness, so I don't think it is any worse to maintain going forward\nthan things like --patience, which people can tweak if their diff looks\nuglier than they would like).\n\n-Peff\n"},{"id":"288994","messageId":"xmqqk2hwepcq.fsf@gitster.mtv.corp.google.com","threadId":"42572","inReplyTo":"xmqqoa78epmt.fsf_-_@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] diff: disable compaction heuristic for now","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-10T20:55:01Z","receivedAt":"2016-06-16T02:19:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>     Jeff King <peff@peff.net> writes:\n>\n>     > We probably want a patch to the release notes to note that it's not on\n>     > by default. And we may want to advertise the experimental knob so\n>     > that people actually try it (otherwise we won't get feedback from the\n>     > masses).\n>\n>     OK, I think that is sensible.  The interdiff is not a strict\n>     reversion of 77085a61 (diff: undocument the compaction heuristic\n>     knobs for experimentation, 2016-05-02) but stresses that the\n>     feature is off by default and is experimental.\n\nThis is for 'master' when the topic is merged (will keep it in stash\nfor now).\n\n Documentation/RelNotes/2.9.0.txt | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/RelNotes/2.9.0.txt b/Documentation/RelNotes/2.9.0.txt\nindex 927cb9b..79b46e1 100644\n--- a/Documentation/RelNotes/2.9.0.txt\n+++ b/Documentation/RelNotes/2.9.0.txt\n@@ -118,9 +118,11 @@ UI, Workflows & Features\n  * HTTP transport clients learned to throw extra HTTP headers at the\n    server, specified via http.extraHeader configuration variable.\n \n- * Patch output from \"git diff\" and friends has been tweaked to be\n-   more readable by using a blank line as a strong hint that the\n-   contents before and after it belong to logically separate units.\n+ * The \"--compaction-heuristic\" option to \"git diff\" family of\n+   commands enables a heuristic to make the patch output more readable\n+   by using a blank line as a strong hint that the contents before and\n+   after it belong to logically separate units.  It is still\n+   experimental.\n \n  * A new configuration variable core.hooksPath allows customizing\n    where the hook directory is.\n"},{"id":"288996","messageId":"20160610210516.GC22470@sigill.intra.peff.net","threadId":"42572","inReplyTo":"xmqqk2hwepcq.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] diff: disable compaction heuristic for now","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-10T21:05:17Z","receivedAt":"2016-06-16T02:19:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 10, 2016 at 01:55:01PM -0700, Junio C Hamano wrote:\n\n> This is for 'master' when the topic is merged (will keep it in stash\n> for now).\n> \n>  Documentation/RelNotes/2.9.0.txt | 8 +++++---\n>  1 file changed, 5 insertions(+), 3 deletions(-)\n> \n> diff --git a/Documentation/RelNotes/2.9.0.txt b/Documentation/RelNotes/2.9.0.txt\n> index 927cb9b..79b46e1 100644\n> --- a/Documentation/RelNotes/2.9.0.txt\n> +++ b/Documentation/RelNotes/2.9.0.txt\n> @@ -118,9 +118,11 @@ UI, Workflows & Features\n>   * HTTP transport clients learned to throw extra HTTP headers at the\n>     server, specified via http.extraHeader configuration variable.\n>  \n> - * Patch output from \"git diff\" and friends has been tweaked to be\n> -   more readable by using a blank line as a strong hint that the\n> -   contents before and after it belong to logically separate units.\n> + * The \"--compaction-heuristic\" option to \"git diff\" family of\n> +   commands enables a heuristic to make the patch output more readable\n> +   by using a blank line as a strong hint that the contents before and\n> +   after it belong to logically separate units.  It is still\n> +   experimental.\n>  \n>   * A new configuration variable core.hooksPath allows customizing\n>     where the hook directory is.\n\nLooks good.\n\nI think your calendar calls for release 2.9.0 on Monday. Are you going\nto bump the schedule for this? I don't think it's very high risk, and\nwouldn't need to.\n\n-Peff\n"},{"id":"288998","messageId":"xmqq8tycemyl.fsf@gitster.mtv.corp.google.com","threadId":"42572","inReplyTo":"20160610210516.GC22470@sigill.intra.peff.net","subject":"Re: [PATCH v2] diff: disable compaction heuristic for now","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-10T21:46:42Z","receivedAt":"2016-06-16T02:19:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Looks good.\n>\n> I think your calendar calls for release 2.9.0 on Monday. Are you going\n> to bump the schedule for this? I don't think it's very high risk, and\n> wouldn't need to.\n\nI didn't plan to.\n"}]}