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

[Duy Nguyen <pclouds@gmail.com>]

On Sun, Dec 6, 2015 at 8:47 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> 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.

>> +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".

>> @@ -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.

>> +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