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

From: Elijah Newren <newren@gmail.com>
Date: Mar 11, 2021, 16:42 UTC
Message-ID: <CABPp-BE1TvFJ1eOa8Ci5JTMET+dzZh3m3NxppqqWPyEp1UeAVg@mail.gmail.com>
In-Reply-To: <cbf2716a-afbf-0b36-2f45-4c13ade38768@gmail.com>

On Thu, Mar 11, 2021 at 7:20 AM Derrick Stolee <stolee@gmail.com> wrote:

Show 14 quoted lines

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