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

Derrick Stolee [off-list ref] writes:

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

quoted

"Derrick Stolee via GitGitGadget" [off-list ref] writes:

quoted

+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.

Yes.  "Why are we _ignoring_ what is in the old file when we claim
to be _moving_?" was the question I had upon seeing this function.

quoted

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).

As long as this is used on a single-valued "last one wins" variable,
the callers and this helper taken together will do the right thing.

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.

The name is fine as long as the requirement for the caller is made
clear.  A short comment to help the next reader from having to ask
the same question before the helper may be sufficient.

quoted

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.

Unless there is a lower-precedence configuration file that we have
to override, yes, not writing core.bare=false upon "worktree add" is
fine.  I simply do not know if we need to do something special in
order to defeat /etc/gitconfig or $HOME/.gitconfig with the repository
or the worktree specific configuration file.

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.

OK.  The .git _file_ is our moral equivalent to a symbolic link, and
I forgot about that.

I also wonder if we should do something like what we do for refs
(i.e. the API knows which refs are per-worktree and which are
global, so the callers do not have to care and just can say things
like "update HEAD to this value", and "give me the value of
refs/bisect/good") when repo_set_config*() is called, but that is
outside the scope of this step, which is about one-time migration.

As the code for migration go, I think I am happy with what it wants
to do and how it does it.

Thanks.