Re: [PATCH v4 3/4] repo-settings: introduce commitgraph.changedPathsVersion

From: Derrick Stolee <derrickstolee@github.com>
Date: Jun 20, 2023, 13:28 UTC
Message-ID: <ceb8eb58-6cbe-1846-14e4-33c4967e3024@github.com>
In-Reply-To: <6adfa53dafc8c34f94a017fdc08460911f53d310.1686677910.git.jonathantanmy@google.com>

On 6/13/2023 1:39 PM, Jonathan Tan wrote:

Show 10 quoted lines

> A subsequent commit will introduce another version of the changed-path
> filter in the commit graph file. In order to control which version is
> to be accepted when read (and which version to write), a config variable
> is needed.
> 
> Therefore, introduce this config variable. For forwards compatibility,
> teach Git to not read commit graphs when the config variable
> is set to an unsupported version. Because we teach Git this,
> commitgraph.readChangedPaths is now redundant, so deprecate it and
> define its behavior in terms of the config variable we introduce.

I'm late to the party, but I support this change. The safety valve of "I want to turn this off if something goes wrong" is long overdue for deletion (it would help someone in this high-bit situation).

Having a new replacement is a good way to preserve the safety valve
behavior while also promoting the new versioning scheme.

> 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.

Show 6 quoted lines

>  commitGraph.readChangedPaths::
> -	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
> +	changedPathsVersion=0 if false.

This defaulted to true before...

Show 6 quoted lines

> +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
> +	match the version set in this config variable will be ignored.
> ++
> +Defaults to 1.

So this version defaults to 1. Good.

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

Show 5 quoted lines

> -	if (s->commit_graph_read_changed_paths) {
> +	if (s->commit_graph_changed_paths_version == 1) {
>  		pair_chunk(cf, GRAPH_CHUNKID_BLOOMINDEXES,
>  			   &graph->chunk_bloom_indexes);
>  		read_chunk(cf, GRAPH_CHUNKID_BLOOMDATA,

Show 8 quoted lines

>  	/* Commit graph config or default, does not cascade (simple) */
>  	repo_cfg_bool(r, "core.commitgraph", &r->settings.core_commit_graph, 1);
>  	repo_cfg_int(r, "commitgraph.generationversion", &r->settings.commit_graph_generation_version, 2);
> -	repo_cfg_bool(r, "commitgraph.readchangedpaths", &r->settings.commit_graph_read_changed_paths, 1);
> +	repo_cfg_bool(r, "commitgraph.readchangedpaths", &readChangedPaths, 1);
> +	repo_cfg_int(r, "commitgraph.changedpathsversion",
> +		     &r->settings.commit_graph_changed_paths_version,
> +		     readChangedPaths ? 1 : 0);

And here we default to 'true' and '1', respectively. This allows 'commitGraph.readChangedPaths=false' to override 'commitGraph.changedPathsVersion=1'. Should that implication be documented?

Thanks, -Stolee