Re: [PATCH v2 00/10] Complete merge-ort implementation...almost

On Thu, Mar 11, 2021 at 7:20 AM Derrick Stolee [off-list ref] wrote:

On 3/9/2021 1:24 AM, Elijah Newren via GitGitGadget wrote:

quoted

This series (nearly) completes the merge-ort implementation, cleaning up
testsuite failures. The exceptions are some t6423 failures being addressed
independently[1].

I like that most of the patches here are small and straight-forward, and
are backed up by tests.

I'm concerned about the coupling of the "index with one entry" mechanism.
I'd like to see that built differently by creating methods that can
operate on an attributes file directly. If this method extraction is truly
too difficult, then I'm willing to concede this point. It is worth some
effort to try, though.

Oh, I agree, I hate the "index with one entry" mechanism to activate
the renormalization code.  It feels like a capitulation on merge-ort's
design and is such a hack.  I tried to figure out how to do it a
different way, and failed pretty hard.  I was close to the point of
throwing my hands in the air and leaving the renormalization tests
failing ("who uses that stuff anyway?"), but it was the _only_ test
that merge-recursive passed that merge-ort didn't, so I kept trying
anyway.  At some point I realized that a hack along these lines might
work.  Although, to be honest, I still don't understand this code or
why my patches work -- in particular, the get_stream_filter() call has
me flummoxed.  It returns a value, but I throw it away -- yet the code
fails unless I call it.  What is it actually doing?  Clearly it has
some kind of side-effect somewhere, but reading the code didn't seem
to help me.  I discovered the function and others via various
debugging and trial-and-error attempts, and ended up finding out that
just calling that function is enough.  I know it works, but I don't
know why.  I understand everything else in merge-ort; I hate having a
magic corner here.  I'm not happy with it.

I'm certain that with enough effort the renormalization stuff could be
fixed and made callable without an istate.  Perhaps there's even a
simple way to extract the relevant logic and I'm just being dense, but
I got very lost in that code and I'm a bit afraid of it now.