Re: [PATCH 2/2] xdiff: implement empty line chunk heuristic

["Keller, Jacob E" <jacob.e.keller@intel.com>]

On Fri, 2016-04-29 at 15:35 -0700, Stefan Beller wrote:
> On Fri, Apr 29, 2016 at 3:18 PM, Junio C Hamano <gitster@pobox.com>
> wrote:
> > 
> > Jacob Keller <jacob.keller@gmail.com> writes:
> > 
> > > 
> > > On Fri, Apr 29, 2016 at 1:29 PM, Junio C Hamano <gitster@pobox.co
> > > m> wrote:
> > > > 
> > > > Jeff King <peff@peff.net> writes:
> > > > 
> > > > > 
> > > > > ... Having the two directly next to each other reads
> > > > > better to me. This is a pretty unusual diff, though, in that
> > > > > it did
> > > > > change the surrounding whitespace (and if you look further in
> > > > > the diff,
> > > > > the identical change is made elsewhere _without_ touching the
> > > > > whitespace). So this is kind of an anomaly. And IMHO the
> > > > > weirdness here
> > > > > is outweighed by the vast number of improvements elsewhere.
> > > > So... is everybody happy with the result and now we can drop
> > > > the
> > > > tweaking knob added to help experimentation before merging the
> > > > result to 'master'?
> > > > 
> > > > I am pretty happy with the end result myself.
> > > I am very happy with it. I haven't had any issues, and I think
> > > we'll
> > > find better traction by enabling it at this point and seeing
> > > when/if
> > > someone complains.
> > > 
> > > I think for most it won't be noticed and for those that do it
> > > will
> > > likely be positive.
> > I am doing this only to prepare in case we have a concensus,
> > i.e. this is not to declare that I do not care what other people
> > say.  Here is a patch to remove the experimentation knob.
> > 
> > Let's say we keep this patch out of tree for now and keep the topic
> > in 'next' so that people can further play with it for several more
> > weeks, and then apply this on top and merge the result to 'master'
> > early in the next cycle.
> > 
> > -- >8 --
> > diff: enable "compaction heuristics" and lose experimentation knob
> > 
> > It seems that the new "find a good hunk boundary by locating a
> > blank
> > line" heuristics gives much more pleasant result without much
> > noticeable downsides.  Let's make it the new algorithm for real,
> > without the opt-out knob we added while experimenting with it.
> I would remove the opt-out knob much later in the game, i.e.
> 
>     1) make a patch that removes the documentation only
>        before the next release (i.e. before 2.9)
>     2) make a patch to remove the actual (unlabeled) knobs,
>         merge into master before 2.10 (i.e. just after the 2.9
> release)
> 
> Then we get the most of the community to test it with the 2.9 release
> and still have an emergency knob in case some major headaches
> show up. After one release cycle we'll be much more confident
> about its usage and its short comings and do not need the
> emergency turn off. If the community doesn't like it for some reason
> we can document it and debate the default setting?
> 
> I agree we want the knob gone eventually.
> Making it an undocumented feature is as good as that from
> a users point of view?
> 

Currently it's an "opt in" knob, so this doesn't make sense to me. If
we remove the entire knob as is, we can always (fairly easily) add it
back. I would keep the code inside xdiff as a knob, but set it to
enable default so that the user config has no knob at the top level but
the xdiff machinery does (this making a "disable" be relatively small
patch).

Thanks,
Jake