Re: [GSoC][PATCH v2 2/2] credential-cache: use XDG_CACHE_HOME for socket

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 13, 2017, 21:52 UTC
Message-ID: <xmqqa88orgjd.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170313204355.56559-2-lehmacdj@gmail.com>

Devin Lehmacher <lehmacdj@gmail.com> writes:

> git-credential-cache will now use a socket following the XDG base path
> specification by default. This increases consistency with other
> applications and helps keep clutter out of users' home directories.

We tend to write our log messages in imperative mood, as if you are giving an order to the codebase to "be like so" (alternatively, you can read them as if you are giving an order to the maintainer of the code to "make these changes").

	We have been using ~/.git-credential-cache/socket as the
	location to store the UNIX socket to communicate with the
	credential daemon process.  In order to make it more
	consistent with other applications and reduce clutter in the
	home directory, move it to $XDG_CACHE_HOME/git/credential/socket,
	which matches what XDG base path specification suggests.

Similarly for the other two paragraphs. Instead of "We still check the old location ...", just "Check the old location ...", etc.

> If there is not a socket at that location we create a new one at
> $XDG_CACHE_HOME/git/credential/socket. This complies with the XDG
> standard and good for the reasons previously mentioned.

And the second sentence can go; you already said why you think XDG_CACHE_HOME is a good idea.

> We use the
> subdirectory credential/ in case we later want to store other files
> under $XDG_CACHE_HOME/git/ and to make the purpose of the socket clear.

And this probably can disappear (or rolled into the first paragraph, if we really want; personally I think it is obvious why we want the extra "credential" directory under "git" there).

> I also change to documentation to reflect the new default socket
> location.

This probably does not have to be said, as it is obvious from the diffstat.

Missing sign-off.

Show 16 quoted lines

> ---
>  Documentation/git-credential-cache.txt |  3 ++-
>  credential-cache.c                     | 16 +++++++++++++++-
>  2 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-credential-cache.txt b/Documentation/git-credential-cache.txt
> index 96208f822..4b9db3856 100644
> --- a/Documentation/git-credential-cache.txt
> +++ b/Documentation/git-credential-cache.txt
> @@ -34,7 +34,8 @@ OPTIONS
>  
>  	Use `<path>` to contact a running cache daemon (or start a new
>  	cache daemon if one is not started). Defaults to
> -	`~/.git-credential-cache/socket`. If your home directory is on a
> +	`~/.git-credential-cache/socket` if it exists and otherwise
> +    `$XDG_CACHE_HOME/git/credential/socket`. If your home directory is on a

There is a funny indentation here.

Show 17 quoted lines

>  	network-mounted filesystem, you may need to change this to a
>  	local filesystem. You must specify an absolute path.
>  
> diff --git a/credential-cache.c b/credential-cache.c
> index cc8a6ee19..db1343b46 100644
> --- a/credential-cache.c
> +++ b/credential-cache.c
> @@ -83,6 +83,20 @@ static void do_cache(const char *socket, const char *action, int timeout,
>  	strbuf_release(&buf);
>  }
>  
> +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))

Don't we want to make sure that this path _is_ a socket? In general I think file_exists() is a poor choice to use here (the existing use are all about having a regular file there, and its definition may be later tightened from "does lstat() succeed?" to something a bit more sensible, and FIFO may start failing the updated test.

Thanks.