Re: [PATCH v5 3/4] repo-settings: introduce commitgraph.changedPathsVersion
From: Taylor Blau <hidden>
Date: 2023-07-19 18:10:54
On Thu, Jul 13, 2023 at 02:42:10PM -0700, Jonathan Tan wrote:
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
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".
quoted hunk ↗ jump to hunk
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