Re: [PATCH v3 0/8] pack-objects: Create an alternative name hash algorithm (recreated)

From: Taylor Blau <me@ttaylorr.com>
Date: Jan 22, 2025, 23:28 UTC
Message-ID: <Z5F/JdnSAYqUBJ8s@nand.local>
In-Reply-To: <35026c72-f9b4-40a3-b528-1c28b1238972@gmail.com>

On Tue, Jan 21, 2025 at 03:21:15PM -0500, Derrick Stolee wrote:

Show 6 quoted lines

> This series has been at this version for a while. I'm pretty sure that this
> is the most promising direction we have at the moment for improving delta
> compression for many users.
>
> The only decision point I think remains is whether or not to include the last
> patch (--name-hash-version=3) which I would be happy either way.

Sorry that I punted on reviewing this for way longer than I should have, and thanks for bearing with me.

I took a close look at this latest round of patches, skipping over the parts that I remembered from previous rounds. My memory is far from perfect, so I may have commented on things that we've already discussed, in which case I apologize :-).

I left a handful of comments on the patches themselves, but they are mostly cosmetic. My idle thought before having a chance to review this series is that the --name-hash-version option was handing over too much control to the user without clear instruction on when to use one version over the other.

After reviewing, I think the idea of having a versioned name-hash is a good one, and I agree that it'll make the eventual .bitmap changes much easier to implement.

So I think in that sense exposing a `--name-hash-version` is the right thing to do. My feeling is that we should probably just add Jonathan's "v2", since it appears to be a improvement in nearly all cases against v1, and more often an improvement than not when compared to v3. In that world, just introducing v2 leaves us with less code to maintain and fewer, clearer options presented to users.

If you feel strongly about keeping v3, I am definitely open to changing my mind here, but my feeling on first blush of this most recent round is that I would probably just include v2.

I'm excited about seeing these patches land, and I am glad that someone is working on them!

Thanks, Taylor