Re: [GSoC][PATCH 1/3] path.c: Add xdg_cache_home to get paths under XDG_CACHE_HOME

[Jeff King <peff@peff.net>]

On Mon, Mar 13, 2017 at 01:22:30PM -0400, Devin Lehmacher wrote:

> Subject: Re: [GSoC][PATCH 1/3] path.c: Add xdg_cache_home to get paths under XDG_CACHE_HOME

It's nice to keep subject lines succinct, as people will skim through
them in "shortlog" or "log --oneline" output for years to come. Of
course, you should not make them _too_ succinct, but I think there is a
lot of extra detail here.

Probably:

  path.c: add xdg_cache_home

would suffice in this case (not also that we usually do not start
with a capital letter).

> This is necessary to make it posible to use XDG_CACHE_HOME for caches in
> the XDG standard. I modeled this after the very similar xdg_config_home
> function for obtaining paths to functions under XDG_CONFIG_HOME

I think this covers what it needs to, which is good. I'd usually try to
avoid starting the message with "this", as it it can be a bit vague (and
assumes that the body is shown next to the subject, which is not always
the case).

I'd have probably written it like:

  We already have xdg_config_home() to help us format paths in
  XDG_CONFIG_HOME. Let's provide a similar xdg_cache_home() to do the
  same thing for XDG_CACHE_HOME.

or something. I admit this is bikeshedding, but since you're new I feel
like I get to spout off about commit message style. :)

> +char *xdg_cache_home(const char *filename)
> +{
> +	const char *home, *cache_home;
> +
> +	assert(filename);
> +	cache_home = getenv("XDG_CACHE_HOME");
> +	if (cache_home && *cache_home)
> +		return mkpathdup("%s/git/%s", cache_home, filename);
> +
> +	home = getenv("HOME");
> +	if (home)
> +		return mkpathdup("%s/.cache/git/%s", home, filename);
> +	return NULL;
> +}

This looks fine, as it comes from xdg_config_home(). It does make me
wonder if the two should be sharing a common implementation, with a
signature like:

  char *xdg_generic_home(const char *env,
			 const char *fallback,
			 const char *filename);

and then the two functions do:

  return xdg_generic_home("XDG_CACHE_HOME", ".cache", filename);

For two the duplication is not so bad, but I wonder if there are other
xdg paths we'd care about.

And one final note. I notice that we return NULL if the user has no
HOME. But I'm not sure most callers are prepared to handle this. E.g.,
if you have no ident set and no HOME, then we will pass NULL to lstat().
On Linux at least that just gets you EFAULT, but I wouldn't be surprised
if it's a segfault on other systems (probably at least Windows, where we
have an lstat wrapper that calls strlen on the filename).

This is not at all a new thing with your patch, but it might be worth
considering while we are thinking about expanding this interface. I'm
not sure if the callers should be more careful, of it the function
should promise to either die() or return a non-NULL value.

-Peff