Re: Re* [BUG-ish] diff compaction heuristic false positive

From: Jeff King <peff@peff.net>
Date: Jun 10, 2016, 20:30 UTC
Message-ID: <20160610203026.GA21464@sigill.intra.peff.net>
In-Reply-To: <xmqqeg84gbex.fsf_-_@gitster.mtv.corp.google.com>

On Fri, Jun 10, 2016 at 11:13:10AM -0700, Junio C Hamano wrote:

Show 16 quoted lines

> Jacob Keller <jacob.keller@gmail.com> writes:
> 
> > I think we could use the indentation trick and it might help in this
> > case. I agree, let's disable this for this cycle and experiment in the
> > next one. Good catch, Peff.
> >
> > As others have said you will always be able to produce counter
> > examples, that's the nature of heuristics. The idea is to see if we
> > can come up with something simple that mostly improves the output,
> > even if sometimes it might have a negative impact on the outputs. But
> > I think we should avoid changing behavior unless it's mostly an
> > improvement.
> 
> OK, let's do this then for the upcoming release for now.  I am
> tempted to flip it back on after the release in 'next', so that
> developers would be exposed to the heuristic by default, though.

Thanks for the patch, and I agree flipping it back on in "next" is a good idea.

It may be that I am making a fuss over nothing. As you say, we always knew that it might have false positives. Mostly I was just surprised how frequent they were in this example (I also wondered why the same pattern did not trigger in the C code we looked at).

> -- >8 --
> Subject: [PATCH] diff: disable compaction heuristic for now

Looks good.

We probably want a patch to the release notes to note that it's not on by default. And we may want to advertise the experimental knob so that people actually try it (otherwise we won't get feedback from the masses).

-Peff