Re: [PATCH] commit-graph: add new config for changed-paths & recommend it in scalar

From: Derrick Stolee <stolee@gmail.com>
Date: Oct 10, 2025, 12:48 UTC
Message-ID: <1a88e577-a808-4815-b390-e5d2253e670c@gmail.com>
In-Reply-To: <xmqqecrbd7yh.fsf@gitster.g>

On 10/9/2025 6:30 PM, Junio C Hamano wrote:

> "Emily Yang via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Emily Yang <emilyyang.git@gmail.com>

Show 11 quoted lines

>> In this commit, we're proposing a new
>> config option "commitGraph.changedPaths" - "true" value acts like
>> "--changed-paths"; "false" disables a previous "true" config value but
>> doesn't imply "--no-changed-paths".
> 
> The way the above is phrased is so unusual that I am afraid it would
> confuse readers.
> 
> When a configuration variable gives an opportunity for the users to
> override the hardcoded default (in this case, --no-changed-paths has
> been the traditional default,

(I'm pointing out this statement and how it's not quite right. I'll explain more fully lower in this reply.)

Show 40 quoted lines

> and graph.changedPaths=true would make
> us pretend as if --changed-paths were given from the command line).
> So if we were to have this configuration variable, setting it false
> MUST make it pretend as if --no-changed-paths were given from the
> command line, and MUST continue to do so even in some future we
> changed the hardcoded default to be "true" (i.e., unless the user
> says graph.changedPath=false in the configuration and/or declines
> with "--no-changed-paths" from the command line, we will record the
> changed paths filter by default).
> 
> Setting commitGraph.changedPaths to true should mean that the
> "git commit-graph write" command behaves as if --changed-paths
> were given immediately after that "write", so that an end-user
> commmand
> 
>     $ git commit-graph write
> 
> should behave as if it was written like this
> 
>     $ git commit-graph write --changed-paths
> 
> and
> 
>     $ git commit-graph write --no-changed-paths
> 
> should behave as if it was written like this
> 
>     $ git commit-graph write --changed-paths --no-changed-paths
> 
> i.e. allowing the command line --no-changed-paths to override it.
> 
> Setting commitGraph.changedPaths to false should similarly mean that
> "--no-changed-paths" implicitly is added immediately after "write",
> meaning that 
> 
>     $ git commit-graph write
> 
> should behave as if it was written like this
> 
>     $ git commit-graph write --no-changed-paths

One thing that is tricky about --[no-]changed-paths is that it is a "tri-state" argument due to 0087a87ba8 (commit-graph: persist existence of changed-paths, 2020-07-01):

 * --changed-paths : Definitely write the data, even if it didn't
   exist already.

 * --no-changed-paths : Definitely _don't_ write the data, even if
   it exists already.

 * (not present) : Update filters that do exist, but don't write them
   if they don't exist.

This is reflected in how opts.enable_changed_paths is initialized to -1 in the existing version. Then, the config is loaded before the arguments are parsed (this is already enforcing the precedence of '--max-new-filters=<N>' over the 'commitGraph.maxNewFilters' config).

Later, opts.enable_changed_paths is converted into COMMIT_GRAPH_WRITE_BLOOM_FILTERS or COMMIT_GRAPH_NO_WRITE_BLOOM_FILTERS flags for the underlying commit-graph API, with the default of -1 passing neither flag (which will use any existing commit-graph to persist and extend filters that already exist).

The big reason for this is so users can use a foreground process to initialize filters, then background maintenance will respect and persist that behavior. The big change here is that the config allows a user to enable the filters and have them be computed entirely in the background.

So I think this is the root of your concerns here.

Show 14 quoted lines

>> @@ -210,6 +210,8 @@ static int git_commit_graph_write_config(const char *var, const char *value,
>>  {
>>  	if (!strcmp(var, "commitgraph.maxnewfilters"))
>>  		write_opts.max_new_filters = git_config_int(var, value, ctx->kvi);
>> +	else if (!strcmp(var, "commitgraph.changedpaths"))
>> +		opts.enable_changed_paths = git_config_bool(var, value) ? 1 : -1;
> 
> This is iffy.
> 
> Unless the way existing command line parser figures out if the user
> wants or does not want to use the feature is so screwed up, you
> shouldn't have to do any such thing.
> 
> Why do you need to special case 'false' this way?

The config now has this implication:

 * true : turn '(not present)' into '--changed-paths'.
 * false/unset : Continue to assume '(not present)'.

And the typical case is that we would have 'false' imply '--no-changed-paths' which _removes_ filters that may exist. I could see a case for this.

The situation that I wanted to think about was this:

 * A user sets the config to 'true' in global config.
 * They then set the config to 'false' in a specific repo.

In this case, the 'false' _disables the config_ but doesn't cause any existing filters to be deleted.

I hope this helps. I could see a case for 'false' implying '--no-changed-filters' but as Emily was investigating this and noting this discrepancy, we leaned in the direction of being non- destructive with the config.

Thanks, -Stolee