Re: [PATCH 04/15] scalar: 'register' sets recommended config and starts maintenance

[Derrick Stolee <stolee@gmail.com>]

On 8/31/2021 4:11 AM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Mon, Aug 30 2021, Derrick Stolee via GitGitGadget wrote:
> 
>> [...]
>> +#ifndef WIN32
>> +		{ "core.untrackedCache", "true" },
>> +#else
>> +		/*
>> +		 * Unfortunately, Scalar's Functional Tests demonstrated
>> +		 * that the untracked cache feature is unreliable on Windows
>> +		 * (which is a bummer because that platform would benefit the
>> +		 * most from it). For some reason, freshly created files seem
>> +		 * not to update the directory's `lastModified` time
>> +		 * immediately, but the untracked cache would need to rely on
>> +		 * that.
>> +		 *
>> +		 * Therefore, with a sad heart, we disable this very useful
>> +		 * feature on Windows.
>> +		 */
>> +		{ "core.untrackedCache", "false" },
>> +#endif
>> [...]
> 
> Ok, but why the need to set it to "false" explicitly? Does it need to be
> so opinionated as to overwrite existing user-set config in these cases?

Users can overwrite this local config value, but this is placed to avoid
a global config value from applying specifically within Scalar-created
repos.
 
>> +		{ "core.bare", "false" },
> 
> Shouldn't this be set by "git init" already?

This one is probably a bit _too_ defensive. It can be removed.

>> [...]
>> +		{ "core.logAllRefUpdates", "true" },
> 
> An opinionated thing unrelated to performance?

It's an opinionated thing related to supporting monorepo users. It helps
us diagnose issues they have by recreating a sequence of events.

>> [...]
>> +		{ "feature.manyFiles", "false" },
>> +		{ "feature.experimental", "false" },
> 
> Ditto the question about the need to set this, these are false by
> default, right?

But if a user has them on globally, then we don't want them to apply
locally (in favor of the settings that we set explicitly).

>> [...]
>> +		if (git_config_get_string(config[i].key, &value)) {
>> +			trace2_data_string("scalar", the_repository, config[i].key, "created");
>> +			if (git_config_set_gently(config[i].key,
>> +						  config[i].value) < 0)
>> +				return error(_("could not configure %s=%s"),
>> +					     config[i].key, config[i].value);
>> +		} else {
>> +			trace2_data_string("scalar", the_repository, config[i].key, "exists");
>> +			free(value);
>> +		}
> 
> The commit message doesn't discuss these trace2 additions, these in
> particular seem like they might be useful, but better done as as some
> more general trace2 intergration in config.c, i.e. if the functions
> being called here did the same logging on config set/get.

If we want to do such a tracing change within git_config_set*(), then
that would be an appropriate replacement. The biggest reason to include
them here is to trace that an existing value already exists, for the
case of running 'scalar reconfigure' during an upgrade. That part
doesn't make much sense to put into config.c.

Thanks,
-Stolee