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