Re: [GSoC][PATCH 2/3] credential-cache.c: Make git use XDG_CACHE_HOME for credentials

From: Jeff King <peff@peff.net>
Date: Mar 13, 2017, 19:01 UTC
Message-ID: <20170313190111.64vzo7v3b3aty2dt@sigill.intra.peff.net>
In-Reply-To: <20170313172232.96678-3-lehmacdj@gmail.com>

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

> Subject: Re: [GSoC][PATCH 2/3] credential-cache.c: Make git use XDG_CACHE_HOME for credentials

Another subject nitpick. :)

Saying "Make git" is just extraneous. All commits make git do something.

Likewise "for credentials" doesn't say much. Perhaps "for socket" would be more descriptive.

I'm not sure we are entirely consistent here, but I'd probably say just "credential-cache" (not "credential-cache.c"). My rule of thumb is to mention the C file if it's an internal thing: a cleanup, a new function, etc. And use the command-name if it's something user-visible.

So all together:

  Subject: credential-cache: use XDG_CACHE_HOME for socket

is shorter and more descriptive (IMHO).

Show 5 quoted lines

> git-credential-cache will now use the socket
> $XDG_CACHE_HOME/git/credential/socket if there is not already a socket
> at ~/.git-credential-cache/socket. This ensures that if another process
> already created a socket at the old location it will be used over the
> new one if it exists.

This tells us what, but not much about "why". Probably the reason is something simple, like "using standards is better than not". But the commit message should say that.

Show 9 quoted lines

> +static char* get_socket_path(void) {
> +	char *home_socket;
> +
> +	home_socket = expand_user_path("~/.git_credential_cache/socket");
> +	if (home_socket)
> +		if (file_exists(home_socket))
> +			return home_socket;
> +		else
> +			free(home_socket);

Please use braces for nested ifs. I know that C resolves a dangling-else like this as you've indented it (as opposed to attaching the else to the outer "if"), but it is unnecessarily confusing. Just one set of braces:

  if (home_socket) {
	if (file_exists(home_socket);
		return home_socket;
	else
		free(home_socket);
  }

is sufficient.

-Peff