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

[Elijah Newren <newren@gmail.com>]

On Thu, Mar 11, 2021 at 7:20 AM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 3/9/2021 1:24 AM, Elijah Newren via GitGitGadget wrote:
> > 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.