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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jan 3, 2022, 06:38 UTC
Message-ID: <CAPig+cSZ_F7KAZcCs-zOq5knGzqnTkeCvnktm3QLz6-kBSjNng@mail.gmail.com>
In-Reply-To: <940635e7-ff92-d87d-5dfb-a61b6320c30b@gmail.com>

On Thu, Dec 30, 2021 at 12:29 PM Derrick Stolee <stolee@gmail.com> wrote:

Show 24 quoted lines

> On 12/30/2021 3:41 AM, Eric Sunshine wrote:
> > On Tue, Dec 28, 2021 at 4:32 PM Derrick Stolee via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> >> +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`).
>
> Since you agree that "{r->commondir}/config.worktree" is the main
> worktree's config file, then "{r->commondir}/config" should be the
> repo-wide config file. If r->commondir and r->gitdir are different,
> then using r->gitdir would be wrong, as far as I understand it.
>
> Indeed, tracing these two values when run in a worktree, I see:
>
>   gitdir: /home/stolee/_git/git/.git/worktrees/git-upstream
>   commondir: /home/stolee/_git/git/.git
>
> So we definitely want commondir here.

Okay, it looks like I was misinterpreting how path.c:strbuf_worktree_gitdir() worked and how it was called, and was perhaps a bit confused by the minimal `struct repository` comments.

Show 7 quoted lines

> >> +       if (repository_format_worktree_config)
> >> +               return 0;
> >
> > Rather than `return 0`, should this be `goto cleanup`...
>
> Right, or move the 'if' to before the configset is initialized. The
> goto is simple enough.

Moving the `if` -- in fact, all three `if`s -- earlier is enticing, but you need to be careful not to leak `common_config_file` and `main_worktree_file`, as well. Something like this should work, I think:

   if (repository_format_worktree_config)
       return 0;
   if (upgrade_repository_format(r, 1) < 0)
       return error(...);
   if (git_config_set_gently(...))
       return error(...);

   common_config_file = xstrfmt(...);
   main_worktree_file = xstrfmt(...);
   git_configset_init(&cs);
   git_configset_add_file(&cs, common_config_file);