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

[Taylor Blau <me@ttaylorr.com>]

On Tue, Mar 07, 2023 at 02:56:29PM +0100, Ævar Arnfjörð Bjarmason wrote:
> 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