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

[Eric Sunshine <sunshine@sunshineco.com>]

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.

> ---- >8 ----
> The XDG base dir spec[1] specifies that configuration files be stored in
> a subdirectory in $XDG_CONFIG_HOME. To construct such a configuration
> file path, home_config_paths() can be used. However, home_config_paths()
> combines distinct functionality:
>
> 1. Retrieve the home git config file path ~/.gitconfig
>
> 2. Construct the XDG config path of the file specified by `file`.
>
> This function was introduced in commit 21cf3227 ("read (but not write)
> from $XDG_CONFIG_HOME/git/config file").  While the intention of the
> function was to allow the home directory configuration file path and the
> xdg directory configuration file path to be retrieved with one function
> call, the hard-coding of the path ~/.gitconfig prevents it from being
> used for other configuration files. Furthermore, retrieving a file path
> relative to the user's home directory can be done with
> expand_user_path(). Hence, it can be seen that home_config_paths()
> introduces unnecessary complexity, especially if a user just wants to
> retrieve the xdg config file path.
>
> As such, implement a simpler function xdg_config_home() for constructing
> the XDG base dir spec configuration file path. This function, together
> with expand_user_path(), can replace all uses of home_config_paths().
>
> [1] http://standards.freedesktop.org/basedir-spec/basedir-spec-0.7.html
>
> Signed-off-by: Paul Tan <pyokagan@gmail.com>
> ---
> 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.

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

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;

> +}
> --
> 2.1.4