Re: [PATCH v2 1/7] path.c: implement xdg_config_home()

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Apr 20, 2015, 00:39 UTC
Message-ID: <CAPig+cQD9BH8kib98fCsiUHnsSAAMkwxAnFVxD8Gq5JMwXVjog@mail.gmail.com>
In-Reply-To: <CACRoPnR7MQsP_dAE0RV_80JDbU9h6tfu30yL2OOK+JGYLzVLNQ@mail.gmail.com>

On Sat, Apr 18, 2015 at 3:51 AM, Paul Tan <pyokagan@gmail.com> wrote:

Show 29 quoted lines

> On Fri, Apr 17, 2015 at 5:41 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Tue, Apr 14, 2015 at 1:28 PM, Paul Tan <pyokagan@gmail.com> wrote:
>>> + * Returns the newly allocated string "$XDG_CONFIG_HOME/git/{filename}".  If
>>> + * $XDG_CONFIG_HOME is unset or empty, returns the newly allocated string
>>> + * "$HOME/.config/git/{filename}". Returns NULL if filename is NULL or an error
>>> + * occurred.
>> This is better than the original which said "$XDG_CONFIG_HOME/git/%s",
>> but is still potentially confusing. When I read the earlier iteration,
>> I was left with the impression that it was returning that literal
>> string, with '$' and '%s' embedded, and that the caller would have to
>> process it further to have '$' and '%s' expanded. Perhaps rephrasing
>> it something like this will help?
>>
>>     Return a newly allocated string with value xdg+"/git/"+filename
>>     where xdg is the interpolated value of $XDG_CONFIG_HOME if
>>     defined and non-empty, otherwise "$HOME/.config". Return NULL
>>     upon error.
>
> Personally I think interpolated strings are easier to read than
> concatenated strings. $VARIABLE (all upper case) in shell scripting is
> understood to be an environment variable, and $variable (all lower
> case) to be a local variable.
> Thinking about it again, I should not be using python-style format
> strings either ;-). So I would write it as
> "$XDG_CONFIG_HOME/git/$filename".
>
> But anyway, I don't have strong opinions on documentation, so I will
> leave this to majority opinion. I will change it if you strongly
> disagree :-).

Other than being enuinely confused by the original, and having to check the actual implementation for clarification, I don't feel strongly about it either. Perhaps mentioning "evaluation" like this might help?

    Return a newly allocated string with the evaluation of
    "$XDG_CONFIG_HOME/git/$filename" if $XDG_CONFIG_HOME is
    non-empty, otherwise "$HOME/.config/git/$filename". Return
    NULL upon error.

More below.

Show 25 quoted lines

>>> +       if (!config_home || !config_home[0]) {
>>> +               const char *home = getenv("HOME");
>>> +
>>> +               if (!home)
>>> +                       return NULL;
>>> +               return mkpathdup("%s/.config/git/%s", home, filename);
>>> +       } else
>>> +               return mkpathdup("%s/git/%s", config_home, filename);
>>
>> This logic is more difficult to follow than it ought to be due to the
>> use of 'config_home' so distant from the 'if' which checked it, and
>> due to the nested 'if'. It could be expressed more straight-forwardly
>> as:
>>
>>     if (config_home && *config_home)
>>         return mkpathdup("%s/git/%s", config_home, filename);
>>
>>     home = getenv("HOME");
>>     if (home)
>>         return mkpathdup("%s/.config/git/%s", home, filename);
>>
>>     return NULL;
>
> Ah, flipping the conditionals definitely makes it look nicer. I guess
> I will need your sign off to use your code? Thanks!

My sign-off is probably overkill. I merely re-arranged some lines of code which you wrote. A simple Helped-by: is sufficient if you want to mention my name.