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
