Subject: Re: [PATCH v4 0/4] Changed path filter hash fix and version bump

From: Derrick Stolee <[email-masked]>
Date: Tue, 20 Jun 2023 09:43:46 -0400

On 6/13/2023 3:21 PM, Junio C Hamano wrote:
> Jonathan Tan <[email-masked]> writes:
> 
>> Thanks Ramsay for spotting the errors and mentioning that I can use
>> octal escapes. Here's an update taking into account their comments.
> 
> The changes look good.  Will queue.
> 
> Stolee, you had comments on an earlier round---how does this one
> look?

I'm sorry I'm so late to this. I've been meaning to get to it, but
it's been a crazy couple of weeks.

This version is not ready. The backwards compatibility story is
incomplete.

When commitGraph.changedPathsVersion is set, it does not allow
reading a previous filter version, leaving us in a poor performance
state until the commit-graph file can be rewritten.

While I was reviewing, it seemed reasonable to deprecate
commitGraph.readChangedPaths, but this use of "also restrict writes
to this version" didn't make sense to me at the time. Instead, it
would be good to have this clarity between the config options:

 commitGraph.readChangedPaths: should we read and use the filters
 that exist on disk? Defaults to 'true'.

 commitGraph.changedPathsVersion: Which version should we _write_
 when writing a new commit-graph? Defaults to '1' but will default
 to '2' in the next major verion, then '1' will no longer be an
 accepted value in the version after that.

The tricky part is that during the commit-graph write, you will
need to check the existing filter value to see if it matches. If
not, the filters will need to be recomputed from scratch. This
will change patch 4 a bit, but it's the right thing to do.

Thanks,
-Stolee