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

From: Jeff King <peff@peff.net>
Date: Mar 13, 2017, 18:48 UTC
Message-ID: <20170313184837.wnluuyflbx63cwlm@sigill.intra.peff.net>
In-Reply-To: <20170313172232.96678-2-lehmacdj@gmail.com>

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. :)

Show 14 quoted lines

> +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