Re: [PATCH] blame-tree: add library and tests via "test-tool blame-tree"

From: Taylor Blau <me@ttaylorr.com>
Date: Mar 8, 2023, 15:49 UTC
Message-ID: <ZAiuojxNifGo3n97@nand.local>
In-Reply-To: <230307.86o7p4zm4s.gmgdl@evledraar.gmail.com>

On Tue, Mar 07, 2023 at 02:56:29PM +0100, Ævar Arnfjörð Bjarmason wrote:

Show 9 quoted lines

> I hear your concern about leaving this open for optimization, and in
> general I'd vehemently agree with it, except for needing to eventually
> feed a command-line to setup_revisions().
>
> Ideally the revision API would make what you're describing easy, but the
> way it's currently implemented (and changing it would be a much larger
> project) someone who'd like to pass structured options in the way you'd
> describe will end up having to re-implement bug-for-bug compatible
> versions of some subset of the option parsing in revision.c.

I get what you are both saying here, but I think I find myself tending to agree with Ævar a little bit more here.

In an ideal world, sure, having the blame-tree API take a single struct called 'blame_tree_options' would be very clean. But the crux is that we have to pass some arguments to setup_revisions(), and that our problems here stem from the leakiness of *that* API, not this one.

I ran into a similar problem when looking at rewriting the bitmap traversal code a year or so ago (which is sadly still on my to-do list).

Without getting into the details, part of that work involved calling limit_list() as a function of setup_revisions() to discover the traversal boundary. And if the caller happened to put --topo-order in their command-line arguments, we wouldn't end up calling limit_list() at all, since (as Stolee well knows ;-)) those two code paths are quite different.

I can't recall if I either detected if '--topo-order' was passed (by looking to see if `revs.topo_order` was set), or grafted an extra `--no-topo-order` argument onto the end of the list. Either way, I think those two problems are more or less equivalent in this context, and that it seemed like a much more expedient solution to work around the fundamental leakiness of the setup_revision() API rather than refactor it.

> Isn't a way to get the best of both worlds to have a small snippet of
> code that inspects the "struct rev_info" before & after
> setup_revisions(), and which would only implement certain optimizations
> if certain known options are provided, but not if any unknown ones are?

Yeah, I think this is basically the same as my "let's check if the caller passed `--topo-order` by checking the `revs.topo_order` bit" above.

> I think those are all good ways forward here, and I'd much prefer those
> to having to re-implement or pull out subsets of the current option
> parsing logic in revision.c. What do you think?

Same :-).

Thanks, Taylor