From: Derrick Stolee Date: Fri, 10 Oct 2025 12:48:23 GMT Subject: Re: [PATCH] commit-graph: add new config for changed-paths & recommend it in scalar Message-ID: <1a88e577-a808-4815-b390-e5d2253e670c@gmail.com> In-Reply-To: On 10/9/2025 6:30 PM, Junio C Hamano wrote: > "Emily Yang via GitGitGadget" writes: > >> From: Emily Yang >> 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.) > 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=' 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. >> @@ -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