On Tue, Apr 14, 2015 at 1:28 PM, Paul Tan <pyoka...@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 <pyoka...@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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
