Re: [PATCH v2 0/6] PATH WALK I: The path-walk API

[Taylor Blau <me@ttaylorr.com>]

On Sat, Nov 09, 2024 at 07:41:06PM +0000, Derrick Stolee via GitGitGadget wrote:
> Derrick Stolee (6):
>   path-walk: introduce an object walk by path
>   test-lib-functions: add test_cmp_sorted
>   t6601: add helper for testing path-walk API
>   path-walk: allow consumer to specify object types
>   path-walk: visit tags and cached objects
>   path-walk: mark trees and blobs as UNINTERESTING

My apologies for taking so long to review this. Having rad through the
patches in detail, a couple of thoughts:

  - First, I like the structure that you decided on for this series. It
    nicely demonstrates a minimal caller for this new API instead of
    implementing a bunch of untested code. I think that's a great way to
    lay out things up until this point.

  - Second, I read through the existing API and only had minor comments.
    I read through the implementation in detail and found it to match my
    expectation of how each step should function.

So my take-away from spending a few hours with this series is that
everything seems on track so far, and I think this is in a good spot to
build on for more path-walk features.

That all said, I am still not totally sold on the idea that we need a
separate path-based traversal given the significant benefits of the
full-name hash approach that I reviewed earlier today.

To be clear, I am totally willing to believe that there are some
benefits to doing the path-walk approach on top, but I think we should
consider those benefits relative to the large amount of highly
non-trivial code that we're adding in order to power it.

So I'm not strongly opposed or in favor of the approach pursued starting
in this series, I just think that it's worth spending time as a group
(beyond just you and I) considering the benefits and costs associated
with it.

As for the patches themselves, I think that cooking them for a long time
in 'next' makes most sense. We will want to land this patch series if
and only if we decide that the traversal powered by this API is the
right approach. IOW, I don't think it makes sense to have the path-walk
stuff in the tree if it has no callers outside of the test helper
provided by this series.

OK. I think that's a good stopping point for me on the list today, and I
look forward to your responses :-).

Thanks,
Taylor