Re: [PATCH v5 3/4] repo-settings: introduce commitgraph.changedPathsVersion
Taylor Blau <me@ttaylorr.com> writes:
Show 14 quoted lines
> 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.
Thanks. I added a note to the documentation.
Show 11 quoted lines
> > - 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.Ah, yes. Changed it (without the exclamation mark).
Show 11 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".
Whoops...thanks for the catch.