Re: [PATCH v2 3/5] worktree: add upgrade_to_worktree_config()

[Derrick Stolee <stolee@gmail.com>]

On 12/22/2021 12:38 AM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> +int upgrade_to_worktree_config(struct repository *r)
> 
> Not a suggestion to rename the function, but does it mean "we have
> been using a single configuration for all worktrees attached to the
> repository, but we are switching to use per-worktree configuration
> file"?  I am wondering if the phrase "worktree_config" is clear
> enough for our future developers.
> 
>> +{
>> +	int res;
>> +	int bare = 0;
>> +	struct config_set cs = { 0 };
>> +	char *base_config_file = xstrfmt("%s/config", r->commondir);
>> +	char *base_worktree_file = xstrfmt("%s/config.worktree", r->commondir);
>> +
>> +	git_configset_init(&cs);
>> +	git_configset_add_file(&cs, base_config_file);
>> +
>> +	/*
>> +	 * If the base repository is bare, then we need to move core.bare=true
>> +	 * out of the base config file and into the base repository's
>> +	 * config.worktree file.
>> +	 */
>> +	if (!git_configset_get_bool(&cs, "core.bare", &bare) && bare) {
>> +		if ((res = git_config_set_in_file_gently(base_worktree_file,
>> +							"core.bare", "true"))) {
>> +			error(_("unable to set core.bare=true in '%s'"), base_worktree_file);
>> +			goto cleanup;
>> +		}
> 
> Hmph, I would have expected to see
> 
> 		if (git_config_set_in_file_gently(...)) {
> 			res = error(_("..."));
>                         goto cleanup;
> 		}
> 
> instead (obviously with "res" initialized to "0" to assume all will
> be well at the beginning).

Deep down in the git_config_set_... stack, it returns helpful error
codes that I thought would be good to communicate upwards. cmd_config()
uses these error codes, too, but that's a more obvious place where the
user is expecting the error code to be related to the config operation.
 
If this communication is not helpful, then I will use the pattern you
suggest. I will assume this is the case unless told otherwise.

> More importantly, are we prepared to see the repository 'r' that
> already uses per-worktree config layout and refrain from causing any
> damage to it, or are we all perfect developers that any caller that
> calls this on repository 'r' that does not need to be upgraded is a
> BUG()?

I don't think we should add burden to the callers to check that the
repo is not using worktree config. Thinking back to your rename idea
this could be "ensure_using_worktree_config()" to make it clear that
we will use the worktree config after the method, whether or not we
were using it beforehand.

I think this current implementation is non-damaging if someone calls
it after already using worktree config. The change being that a user
can go and add core.bare=false to the common config file and break all
worktrees again, and the current implementation will help heal that.
It's probably better to let the user have that ability to mess things
up and not try to fix something so broken.

Thanks,
-Stolee