Re: [PATCH v6 2/6] worktree: create init_worktree_config()

From: Derrick Stolee <stolee@gmail.com>
Date: Feb 9, 2022, 02:21 UTC
Message-ID: <5f7f10df-f048-a663-a98c-536025be0040@gmail.com>
In-Reply-To: <xmqq1r0dc8om.fsf@gitster.g>

On 2/8/2022 5:09 PM, Junio C Hamano wrote:

Show 19 quoted lines

> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> +static int move_config_setting(const char *key, const char *value,
>> +			       const char *from_file, const char *to_file)
>> +{
>> +	if (git_config_set_in_file_gently(to_file, key, value))
>> +		return error(_("unable to set %s in '%s'"), key, to_file);
>> +	if (git_config_set_in_file_gently(from_file, key, NULL))
>> +		return error(_("unable to unset %s in '%s'"), key, from_file);
>> +	return 0;
>> +}
> 
> Interesting.
> 
> The verb "move" in its name made me expect a "get (and remove)
> whatever value(s) defined out of the old file, and set them
> identically in the new file" sequence, but that is not what is done
> here.  "set to this new single value in the new file and unset from
> the old one".

I think this "copy into the worktree-specific config, then remove from the common file" is an important sequence of events in case a concurrent process comes in and reads the two config files in the intermediate state and does not see the config value anywhere.

But perhaps that's not actually what you are concerned about, because you're saying that the 'value' being provided does not actually guarantee that we are moving the setting.

Show 5 quoted lines

> I can see the need to say "move it only when its value is X",
> so having the caller to extract the value before deciding to call
> the function (hence not "moving from old") does make sense, but then
> the function is misnamed---it is not "moving", it is doing something
> else.

I think the end state is correct for all uses here, since we only run this after checking to see if the config value exists in the 'from_file', so 'value' is correct (and this is a static method, not a generally-useful method for config.h).

Perhaps a "write_in_new_and_remove_from_old()" would be a better, if verbose, name. I struggle to find a less cumbersome name, and "move" seems to match the intent pretty well in the context of its use.

Show 20 quoted lines

>> +	common_config_file = xstrfmt("%s/config", r->commondir);
>> +	main_worktree_file = xstrfmt("%s/config.worktree", r->commondir);
>> +
>> +	git_configset_init(&cs);
>> +	git_configset_add_file(&cs, common_config_file);
>> +
>> +	/*
>> +	 * If core.bare is true in the common config file, then we need to
>> +	 * move it to the main worktree's config file or it will break all
>> +	 * worktrees. If it is false, then leave it in place because it
>> +	 * _could_ be negating a global core.bare=true.
>> +	 */
> 
> Is the assumption that the secondary worktrees are never bare, but
> the primary one could be (iow, adding worktrees to a bare repository
> would leave the original bare repository as the primary "worktree"
> that does not have "working tree")?  I am trying to see what downsides
> it tries to avoid by not moving the core.bare==false setting.  Shouldn't
> core.bare be set to false when "worktree add" creates a new one anyway,
> if the secondaries are never bare?

Secondary worktrees cannot be bare. If Git interprets the worktree config to have core.bare=true in a secondary worktree, it errors out.

You seem to be suggesting that we should explicitly write core.bare=false into each of the worktree-specific config files. Is that right? This move is effectively the same, since 'false' is the default.

Show 16 quoted lines

>> +	if (!git_configset_get_bool(&cs, "core.bare", &bare) && bare) {
>> +		if ((res = move_config_setting("core.bare", "true",
>> +					       common_config_file,
>> +					       main_worktree_file)))
>> +			goto cleanup;
>> +	}
> 
>> +	/*
>> +	 * If core.worktree is set, then the main worktree is located
>> +	 * somewhere different than the parent of the common Git dir.
> 
> OK.  We do not want to share the working tree for the primary worktree
> among secondary worktrees.  For the primary, common and uncommon are
> the same, so it may not matter, but mention of "common Git dir" here
> may confuse readers?  Unless overridden by the config, the parent of
> the git dir is the root of the working tree, no?

Here, the verbal gymnastics are somewhat necessary because secondary worktrees have a .git _file_, not a git directory, so using "common Git dir" is a way to explicitly reference the Git dir. And the strangeness here is exactly that core.worktree can change this working tree to be something other than the parent of the (common) Git dir.

Show 7 quoted lines

>> +	 * Relocate that value to avoid breaking all worktrees with this
>> +	 * upgrade to worktree config.
>> +	 */
> 
> And if it is not set, then working tree of each worktree is the
> parent of the per-worktree Git dir, so they will automatically
> become separate, which makes sense.

Thanks, -Stolee