On Thu, Mar 11, 2021 at 7:20 AM Derrick Stolee <stolee@xxxxxxxxx> 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.
