Re: [PATCH v5 1/1] config: add conditional include

From: Duy Nguyen <pclouds@gmail.com>
Date: Feb 24, 2017, 09:37 UTC
Message-ID: <CACsJy8CerU-=PF6wqzUAM02jkrVVGJ5MA0NgL6z9bHn5KM6jiw@mail.gmail.com>
In-Reply-To: <xmqqwpcg7k6r.fsf@gitster.mtv.corp.google.com>

On Fri, Feb 24, 2017 at 2:59 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 6 quoted lines

> The variable is obviously not treated the same way as include.path ;-)
>
>     When includeIf.<condition>.path variable is set in a
>     configuration file, the configuration file named by that
>     variable is included (in a way similar to how include.path
>     works) only if the <condition> holds true.

Yeah. I was thinking "value" and writing "variable" instead (or perhaps I meant to write "variable value" and accidentally'd the last word again).

Show 9 quoted lines

>> +     /* TODO: maybe support ~user/ too */
>> +     if (pat->buf[0] == '~' && is_dir_sep(pat->buf[1])) {
>> +             const char *home = getenv("HOME");
>> +
>> +             if (!home)
>> +                     return error(_("$HOME is not defined"));
>
> Instead of half-duplicating it here yourself, can't we let
> expand_user_path() do its thing?

This comment came up in previous iterations. I perform explicit expansion to make sure we don't apply wildcards on the expanded "~". But I guess going with expand_user_path() is better (less confusion for future readers). We can come back to it when that "don't apply wildcards" concern becomes real.

Show 21 quoted lines

>> +static int include_condition_is_true(const char *cond, size_t cond_len)
>> +{
>> +     /* no condition (i.e., "include.path") is always true */
>> +     if (!cond)
>> +             return 1;
>> +
>> +     if (skip_prefix_mem(cond, cond_len, "gitdir:", &cond, &cond_len))
>> +             return include_by_gitdir(cond, cond_len, 0);
>> +     else if (skip_prefix_mem(cond, cond_len, "gitdir/i:", &cond, &cond_len))
>> +             return include_by_gitdir(cond, cond_len, 1);
>
> This may be OK for now, but it should be trivial to start from a
> table with two entries, i.e.
>
>         struct include_cond {
>                 const char *keyword;
>                 int (*fn)(const char *, size_t);
>         };
>
> and will show a better way to do things to those who follow your
> footsteps.

Yeah I don't see a third include coming soon and did not go with that. Let's way for it and refactor then.

-- 
Duy