Re: [PATCH 2/5] config.c: move worktree-specific variables to .git/worktrees/...

From: Duy Nguyen <pclouds@gmail.com>
Date: Dec 6, 2015, 10:22 UTC
Message-ID: <CACsJy8BDCGxkLuFTwoyNYZqThe9mZYh1OpL8R1R4-BFvB_1coA@mail.gmail.com>
In-Reply-To: <CAPig+cST=UGG8VAG94qb9M8vcOXfmWAhLoP65yWJGywR9R84LA@mail.gmail.com>

On Sun, Dec 6, 2015 at 8:47 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 21 quoted lines

> On Wed, Dec 2, 2015 at 2:13 PM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
>> .git/info/config.worktree is a pattern list that splits .git/config in
>> to sets: the worktree set matches the patterns, the commmon set does
>> not.
>>
>> In normal worktrees, both sets are stored in .git/config. The
>> config.worktree has no effect. Nothing is changed.
>>
>> In linked worktrees, the common and worktree sets are read from and
>> saved to .git/config and .git/config.worktree respectively. Config
>> keys in .git/config that belong to the worktree set is ignored. Those
>> are for the main worktree only. Similarly, keys not matching the
>> patterns come from .git/config, duplicate keys from
>> .git/config.worktree are ignored.
>>
>> The effect is similar to the $GIT_DIR/$GIT_COMMON_DIR split, we can
>> define that some vars can be shared and some cannot. And as a result
>> of the $GIT_DIR/$GIT_COMMON_DIR split, config.worktree is actually
>> found at .git/worktrees/<id>/config.worktree.
>
> Why does this worktree-specific file need/have a .worktree suffix?

I think in the beginning it was supposed to support git-new-workdir as well. With a separate name, you can symlink .git/config back to original repo and create a new .git/config.worktree. The actual code in this patch does not support this though. I guess as 'git worktree' is maturing, we probably don't have to worry about git-new-workdir and could drop .worktree suffix.

Show 13 quoted lines

>> +static int is_config_local(const char *key_)
>> +{
>> +       static struct strbuf key = STRBUF_INIT;
>> +       int i, dtype;
>> +
>> +       if (!config_local.nr)
>> +               return 0;
>> +
>> +       strbuf_reset(&key);
>> +       strbuf_addstr(&key, key_);
>
> Why does 'key' need to be static considering that it is overwritten on
> each call and its value is never accessed after the function returns?

Mostly to avoid re-allocation because this function will be called for every configuration variable. But this may be premature optimization. On top of that, if we go with builtin per-worktree list only as being discussed, then we can drop exclude machinery, we don't have to preprocess "key" and we can finally kill this "strbuf key".

Show 5 quoted lines

>> @@ -198,4 +198,30 @@ test_expect_success 'local clone from linked checkout' '
>> +test_expect_success 'setting worktree.foo goes to config.worktree' '
>> +       echo worKtree.Foo >> .git/info/config.worktree &&
>
> Perhaps? s/>> />/

Yeah. In the previous iteration, config.worktree would contain the default list (core.worktree and stuff) so > may force following tests to re-initialize config.worktree again. But that's now gone and > makes more sense.

Show 8 quoted lines

>> +test_expect_success 'shared config still goes to config' '
>> +       git config random.key randomValue &&
>> +       git --git-dir=wt.foo/.git config random.key >actual &&
>
> What about also testing the opposite scenario?
>
>     git --git-dir=wt.foo/.git  config random.key randomValue &&
>     git config random.key >actual &&

Yep. Will do.

-- 
Duy