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

From: Taylor Blau <me@ttaylorr.com>
Date: Jul 19, 2023, 18:10 UTC
Message-ID: <ZLgnJdo3o9WA5sNy@nand.local>
In-Reply-To: <131095666d414ff68416526112c8f8deb31ac9e1.1689283789.git.jonathantanmy@google.com>

On Thu, Jul 13, 2023 at 02:42:10PM -0700, Jonathan Tan wrote:

Show 13 quoted lines

> diff --git a/Documentation/config/commitgraph.txt b/Documentation/config/commitgraph.txt
> index 30604e4a4c..07f3799e05 100644
> --- a/Documentation/config/commitgraph.txt
> +++ b/Documentation/config/commitgraph.txt
> @@ -9,6 +9,19 @@ commitGraph.maxNewFilters::
>  	commit-graph write` (c.f., linkgit:git-commit-graph[1]).
>
>  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.

What happens if we have a combination of the two, like:

    [commitGraph]
        readChangedPaths = false
        changedPathsVersion = 1

? From reading the implementation below, I think the answer is that changedPathsVersion would win out. I think that's fine behavior to implement (the more modern configuration option taking precedence over the deprecated one). But I think that we should either (a) note that precedence in the documentation here, or (b) issue a warning() when both are set.

For my $.02, I think that doing just (a) would be sufficient.

Show 13 quoted lines

> diff --git a/commit-graph.c b/commit-graph.c
> index c11b59f28b..9b72319450 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -399,7 +399,7 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,
>  			graph->read_generation_data = 1;
>  	}
>
> -	if (s->commit_graph_read_changed_paths) {
> +	if (s->commit_graph_changed_paths_version != 0) {
>  		pair_chunk(cf, GRAPH_CHUNKID_BLOOMINDEXES,
>  			   &graph->chunk_bloom_indexes);
>  		read_chunk(cf, GRAPH_CHUNKID_BLOOMDATA,

Just a small note, but writing this as

    if (!s->commit_graph_changed_paths_version)

would probably be more in line with our coding guidelines.

Show 9 quoted lines

> diff --git a/repo-settings.c b/repo-settings.c
> index 3dbd3f0e2e..e3b6565ffc 100644
> --- a/repo-settings.c
> +++ b/repo-settings.c
> @@ -24,6 +24,7 @@ void prepare_repo_settings(struct repository *r)
>  	int value;
>  	const char *strval;
>  	int manyfiles;
> +	int readChangedPaths;

Small note: this should be snake-cased like "read_changed_paths".

Show 9 quoted lines

> diff --git a/repository.h b/repository.h
> index e8c67ffe16..1f1c32a6dd 100644
> --- a/repository.h
> +++ b/repository.h
> @@ -32,7 +32,7 @@ struct repo_settings {
>
>  	int core_commit_graph;
>  	int commit_graph_generation_version;
> -	int commit_graph_read_changed_paths;

Nice, I'm glad that we're getting rid of this variable and replacing it with commit_graph_changed_paths_version instead.

> +	int commit_graph_changed_paths_version;

Looking good.

Thanks, Taylor