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

On Fri, 2016-04-29 at 15:35 -0700, Stefan Beller wrote:

On Fri, Apr 29, 2016 at 3:18 PM, Junio C Hamano [off-list ref]
wrote:

quoted

Jacob Keller [off-list ref] writes:

quoted

On Fri, Apr 29, 2016 at 1:29 PM, Junio C Hamano <gitster@pobox.co
m> wrote:

quoted

Jeff King [off-list ref] writes:

quoted

... 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