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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, Apr 18, 2015 at 3:51 AM, Paul Tan <pyokagan@xxxxxxxxx> wrote:
> On Fri, Apr 17, 2015 at 5:41 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>> On Tue, Apr 14, 2015 at 1:28 PM, Paul Tan <pyokagan@xxxxxxxxx> 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.

>>> +       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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]