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

[Paul Tan <pyokagan@gmail.com>]

Hi,

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:
>> Below is the fixed patch. I also decided to return NULL if `filename` is
>> NULL because such an input usually indicated an uncaught error.
>
> Unfortunately, this blurs the line between programmer error (passing
> NULL for filename) and a user/configuration error (XDG_CONFIG_HOME and
> HOME being undefined). If there is indeed no valid interpretation for
> filename==NULL, then it may be better to die() or assert() here to
> flag the programmer error as early as possible, rather than returning
> NULL.
>
> More below.
>
>> ---
>> diff --git a/cache.h b/cache.h
>> index 3d3244b..2db10b8 100644
>> --- a/cache.h
>> +++ b/cache.h
>> @@ -836,6 +836,14 @@ char *strip_path_suffix(const char *path, const char *suffix);
>>  int daemon_avoid_alias(const char *path);
>>  extern int is_ntfs_dotgit(const char *name);
>>
>> +/**
>> + * 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 :-).

> Also, for consistency with other API documentation, say "Return"
> rather than "Returns".

Okay, will fix.

>
> More below.
>
>> +extern char *xdg_config_home(const char *filename);
>> +
>>  /* object replacement */
>>  #define LOOKUP_REPLACE_OBJECT 1
>>  extern void *read_sha1_file_extended(const unsigned char *sha1, enum object_type *type, unsigned long *size, unsigned flag);
>> diff --git a/path.c b/path.c
>> index e608993..8ee7191 100644
>> --- a/path.c
>> +++ b/path.c
>> @@ -856,3 +856,19 @@ int is_ntfs_dotgit(const char *name)
>>                         len = -1;
>>                 }
>>  }
>> +
>> +char *xdg_config_home(const char *filename)
>> +{
>> +       const char *config_home = getenv("XDG_CONFIG_HOME");
>> +
>> +       if (!filename)
>> +               return NULL;
>
> See above regarding conflation of programmer error and user/configuration error.
>
>> +       if (!config_home || !config_home[0]) {
>
> On this project, *config_home is usually favored over 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!

Regards,
Paul