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

[Junio C Hamano <gitster@pobox.com>]

Jonathan Tan <jonathantanmy@google.com> writes:

> I did not implement the mitigation of not using the Bloom filters when
> a high-bit path is sought because, as Stolee says, this is useful only
> when mixing Git implementations and will slow down operations (without
> any increase in correctness) in the absence of such a mix [1].

Sensible, I guess.

>     @@ Commit message
>          This commit does not change the behavior of writing (Git writes changed
>          path filters when explicitly instructed regardless of any config
>          variable), but a subsequent commit will restrict Git such that it will
>     -    only write when commitgraph.changedPathsVersion is 0, 1, or 2.
>     +    only write when commitgraph.changedPathsVersion is a recognized value.

This is nicer.

>          Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
>          Signed-off-by: Junio C Hamano <gitster@pobox.com>
>     @@ Documentation/config/commitgraph.txt: commitGraph.maxNewFilters::
>      -	If true, then git will use the changed-path Bloom filters in the
>      -	commit-graph file (if it exists, and they are present). Defaults to
>      -	true. See linkgit:git-commit-graph[1] for more information.
>     -+	Deprecated. Equivalent to changedPathsVersion=1 if true, and
>     ++	Deprecated. Equivalent to changedPathsVersion=-1 if true, and
>      +	changedPathsVersion=0 if false.

I forgot to comment on this part earlier, but does the context make
it clear enough that these `changedPathsVersion` references are
about `commitGraph.changedPathsVersion` configuration variable
without fully spelled out?  They sit next to each other right now,
so it may not be too bad.  If they appeared across more distance,
I would be worried, though.

>      +commitGraph.changedPathsVersion::
>      +	Specifies the version of the changed-path Bloom filters that Git will read and
>     -+	write. May be 0 or 1. Any changed-path Bloom filters on disk that do not
>     ++	write. May be -1, 0 or 1. Any changed-path Bloom filters on disk that do not
>      +	match the version set in this config variable will be ignored.

So, any time the user configures this to a different value, we will
start to ignore the existing changed-path-filters data in the
repository, and when we are told to write commit-graph, we will
construct changed-path-filters data using the new version?

>      ++
>     -+Defaults to 1.
>     ++Defaults to -1.
>     +++
>     ++If -1, Git will use the version of the changed-path Bloom filters in the
>     ++repository, defaulting to 1 if there are none.

OK, that was misleading.  The configuration can say "-1" and it does
not mean "I'll ignore anything other than version -1"---it means
"I'll read anything".  The earlier statement should be toned down so
that we do not surprise readers, perhaps

    When set to a positive integer value, any changed-path Bloom
    filters on disk whose version is different from the value are
    ignored.

to signal that 0 and negative are special.  Then the readers can
anticipate that special cases are described next.

    When set to -1, then ...
    When set to 0, then ...
    Defaults to -1.
    
When set to the special value -1, what version will we write?

>      +If 0, git will write version 1 Bloom filters when instructed to write.

And we will only read 0 and refuse to read 1?  Or we will read both
0 and 1?

Thanks.