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

[Derrick Stolee <stolee@gmail.com>]

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:
>> [...]
>> 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?
> 
>> 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`).

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.

>> +       /*
>> +        * 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`...

Right, or move the 'if' to before the configset is initialized. The
goto is simple enough.
 
>> +       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?
> 
>> +cleanup:
>> +       git_configset_clear(&cs);
>> +       free(common_config_file);
>> +       free(main_worktree_file);
>> +       return res;

Thanks,
-Stolee