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