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

[Elijah Newren <newren@gmail.com>]

On Tue, Feb 8, 2022 at 2:09 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "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 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.