On Tue, Feb 8, 2022 at 2:09 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> "Derrick Stolee via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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 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.
>
> > +int init_worktree_config(struct repository *r)
> > +{
> > + int res = 0;
> > + int bare = 0;
> > + struct config_set cs = { { 0 } };
> > + const char *core_worktree;
> > + char *common_config_file;
> > + char *main_worktree_file;
> > +
> > + /*
> > + * If the extension is already enabled, then we can skip the
> > + * upgrade process.
> > + */
> > + if (repository_format_worktree_config)
> > + return 0;
>
> OK.
>
> > + if ((res = git_config_set_gently("extensions.worktreeConfig", "true")))
> > + return error(_("failed to set extensions.worktreeConfig setting"));
>
> OK.
>
> > + 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")?
Yes, and in fact that was the case which generated the original bug
report -- a bare clone where the affected individual started using
`git worktree add` to create some non-primary worktrees (and then also
used sparse-checkout in some of them).
> 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?
Moving the core.bare==false setting might make sense. In the previous
discussions, we tried to hypothesize about usage of old git clients
and non-git clients (jgit, etc.) on the same repos, and didn't know if
some of those would break if they couldn't find a `core.bare` setting
anywhere (since they wouldn't know to look in config.worktree). We
needed to migrate core.bare=true to avoid an incorrect value affecting
all worktrees (and thus we figured it was worth the risk of breaking
older git/non-git clients because having older clients be broken is
better than having all clients including current git be broken), but
the same wasn't true for core.bare=false. That said, we don't
actively know of any such clients that would be hurt by such a
migration.
