Re: [PATCH v3 3/6] worktree: add 'init-worktree-config' subcommand

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Dec 30, 2021, 08:41 UTC
Message-ID: <CAPig+cTn1049oZQwcLk82LPoOZ9N3mYQifSDObejdcvZwo6ZJg@mail.gmail.com>
In-Reply-To: <e2a0a458115a26cfb855f7040f15e5198072b3a5.1640727143.git.gitgitgadget@gmail.com>

On Tue, Dec 28, 2021 at 4:32 PM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 5 quoted lines

> [...]
> To gain access to the core repository's config and config.worktree file,
> we reference a repository struct's 'commondir' member. If the repository
> was a submodule instead of a worktree, then this still applies
> correctly.

In [1], I suggested that you should be using `repository->gitdir` rather than `repository->commondir` to access the `.git/config` file. Is the above paragraph saying that my suggestion was incorrect? Or is it incorrect only in the case of submodules? Or what is it saying?

Show 7 quoted lines

> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -1031,6 +1032,85 @@ static int repair(int ac, const char **av, const char *prefix)
> +static int init_worktree_config(int ac, const char **av, const char *prefix)
> +{
> +       struct repository *r = the_repository;
> +       char *common_config_file = xstrfmt("%s/config", r->commondir);
> +       char *main_worktree_file = xstrfmt("%s/config.worktree", r->commondir);

Specifically, in [1], I said that `common_config_file` should be using `r->gitdir` (and that the use of `r->commondir` was correct for `main_worktree_file`).

[1]: https://lore.kernel.org/git/CAPig+cQrJ9yWjkc8VMu=uyx_qtrXdL3cNnxLVafoxOo6e-r9kw@mail.gmail.com/

Show 9 quoted lines

> +       git_configset_init(&cs);
> +       git_configset_add_file(&cs, common_config_file);
> +
> +       /*
> +        * If the format and extension are already enabled, then we can
> +        * skip the upgrade process.
> +        */
> +       if (repository_format_worktree_config)
> +               return 0;

Rather than `return 0`, should this be `goto cleanup`...

Show 8 quoted lines

> +       if (upgrade_repository_format(r, 1) < 0) {
> +               res = error(_("unable to upgrade repository format to enable worktreeConfig"));
> +               goto cleanup;
> +       }
> +       if ((res = git_config_set_gently("extensions.worktreeConfig", "true"))) {
> +               error(_("failed to set extensions.worktreeConfig setting"));
> +               goto cleanup;
> +       }

... as is done for these two cases?

Show 6 quoted lines

> +cleanup:
> +       git_configset_clear(&cs);
> +       free(common_config_file);
> +       free(main_worktree_file);
> +       return res;
> +}