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

From: Taylor Blau <me@ttaylorr.com>
Date: Nov 21, 2024, 22:57 UTC
Message-ID: <Zz+61fat+vGgb+xL@nand.local>
In-Reply-To: <pull.1818.v2.git.1731181272.gitgitgadget@gmail.com>

On Sat, Nov 09, 2024 at 07:41:06PM +0000, Derrick Stolee via GitGitGadget wrote:

Show 7 quoted lines

> 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