[GSoC][PATCH 0/3] Move ~/.git-credential-cache to ~/.cache/git

[GSoC][PATCH 0/3] Move ~/.git-credential-cache to ~/.cache/git

[Devin Lehmacher]

Patches for my Microproject. I took into consideration the feedback I
recieved on an earlier email thread and now look for a socket at the old
default location before creating a new socket in $XDG_CACHE_HOME.

Documentation/git-credential-cache.txt |  3 ++-
cache.h                                |  7 +++++++
credential-cache.c                     | 15 ++++++++++++++-
path.c                                 | 15 +++++++++++++++
4 files changed, 38 insertions(+), 2 deletions(-)

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

[Devin Lehmacher]

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

Signed-off-by: Devin Lehmacher <lehmacdj@gmail.com>
---
 cache.h |  7 +++++++
 path.c  | 15 +++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/cache.h b/cache.h
index 8c0e64420..378a636e1 100644
--- a/cache.h
+++ b/cache.h
@@ -1169,6 +1169,13 @@ extern int is_ntfs_dotgit(const char *name);
  */
 extern char *xdg_config_home(const char *filename);
 
+/**
+ * Return a newly allocated string with the evaluation of
+ * "$XDG_CACHE_HOME/git/$filename" if $XDG_CONFIG_HOME is non-empty, otherwise
+ * "$HOME/.config/git/$filename". Return NULL upon error.
+ */
+extern char *xdg_cache_home(const char *filename);
+
 /* object replacement */
 #define LOOKUP_REPLACE_OBJECT 1
 #define LOOKUP_UNKNOWN_OBJECT 2
diff --git a/path.c b/path.c
index efcedafba..22248436b 100644
--- a/path.c
+++ b/path.c
@@ -1272,6 +1272,21 @@ char *xdg_config_home(const char *filename)
 	return NULL;
 }
 
+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;
+}
+
 GIT_PATH_FUNC(git_path_cherry_pick_head, "CHERRY_PICK_HEAD")
 GIT_PATH_FUNC(git_path_revert_head, "REVERT_HEAD")
 GIT_PATH_FUNC(git_path_squash_msg, "SQUASH_MSG")
-- 
2.11.0

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

[Jeff King]

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

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

[Devin Lehmacher]

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

Right now we check the return value from these two functions and if it
is NULL we instead use some other configuration location.

That said I agree that this could lead to unexpected behavior in some
scenarios.

Devin

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

[Devin Lehmacher]

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.

Signed-off-by: Devin Lehmacher <lehmacdj@gmail.com>
---
 credential-cache.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/credential-cache.c b/credential-cache.c
index cc8a6ee19..75c917de7 100644
--- a/credential-cache.c
+++ b/credential-cache.c
@@ -83,6 +83,19 @@ 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))
+			return home_socket;
+		else
+			free(home_socket);
+
+	return xdg_cache_home("credential/socket");
+}
+
 int cmd_main(int argc, const char **argv)
 {
 	char *socket_path = NULL;
@@ -106,7 +119,7 @@ int cmd_main(int argc, const char **argv)
 	op = argv[0];
 
 	if (!socket_path)
-		socket_path = expand_user_path("~/.git-credential-cache/socket");
+		socket_path = get_socket_path();
 	if (!socket_path)
 		die("unable to find a suitable socket path; use --socket");
 
-- 
2.11.0

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

[Junio C Hamano]

Devin Lehmacher <lehmacdj@gmail.com> writes:

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

The typo on this line guarantees that nobody will retain the current
location and will be migrated forcibly to the new xdg place ;-)

> +	if (home_socket)
> +		if (file_exists(home_socket))
> +			return home_socket;
> +		else
> +			free(home_socket);
> +
> +	return xdg_cache_home("credential/socket");
> +}

I somehow feel that the order of precedence should be the other way
around, though.  

If somebody really wants to use the xdg location and has a socket
already there, using that existing socket would be the right thing
to do.  However, when neither ~/.git-credential-cache/socket nor
~/.cache/git/socket exists, why should we prefer the latter over the
former?

>  int cmd_main(int argc, const char **argv)
>  {
>  	char *socket_path = NULL;
> @@ -106,7 +119,7 @@ int cmd_main(int argc, const char **argv)
>  	op = argv[0];
>  
>  	if (!socket_path)
> -		socket_path = expand_user_path("~/.git-credential-cache/socket");
> +		socket_path = get_socket_path();
>  	if (!socket_path)
>  		die("unable to find a suitable socket path; use --socket");

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

[Jeff King]

On Mon, Mar 13, 2017 at 11:09:11AM -0700, Junio C Hamano wrote:

> > +	if (home_socket)
> > +		if (file_exists(home_socket))
> > +			return home_socket;
> > +		else
> > +			free(home_socket);
> > +
> > +	return xdg_cache_home("credential/socket");
> > +}
> 
> I somehow feel that the order of precedence should be the other way
> around, though.  
> 
> If somebody really wants to use the xdg location and has a socket
> already there, using that existing socket would be the right thing
> to do.  However, when neither ~/.git-credential-cache/socket nor
> ~/.cache/git/socket exists, why should we prefer the latter over the
> former?

How would they get a socket in the xdg location if we never create one?

I think the point of this commit is that we should generally prefer the
xdg locations to ones that were simply made up by Git.

As an aside, I also wondered if stale socket files might cause us to
keep picking the "old" location forever. But it looks like
credential-cache--daemon does use a tempfile handler to make sure it
cleans up the socket afterwards (and we are checking for the actual
socket, not just the presence of the outer directory, which is good).

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Mar 13, 2017 at 11:09:11AM -0700, Junio C Hamano wrote:
>
>> > +	if (home_socket)
>> > +		if (file_exists(home_socket))
>> > +			return home_socket;
>> > +		else
>> > +			free(home_socket);
>> > +
>> > +	return xdg_cache_home("credential/socket");
>> > +}
>> 
>> I somehow feel that the order of precedence should be the other way
>> around, though.  
>> 
>> If somebody really wants to use the xdg location and has a socket
>> already there, using that existing socket would be the right thing
>> to do.  However, when neither ~/.git-credential-cache/socket nor
>> ~/.cache/git/socket exists, why should we prefer the latter over the
>> former?
>
> How would they get a socket in the xdg location if we never create one?

They wouldn't.  It was my oblique way to say "it is unclear from the
patch description and the code why this is a good idea---it needs to
be explained better" ;-).

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

[Devin Lehmacher]

> They wouldn't.  It was my oblique way to say "it is unclear from the
> patch description and the code why this is a good idea---it needs to
> be explained better" ;-).

Ok, I will submit a new patch with a better explanation.

Devin

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

[Devin Lehmacher]

>  I somehow feel that the order of precedence should be the other way
>  around, though.
>
>  If somebody really wants to use the xdg location and has a socket
>  already there, using that existing socket would be the right thing
>  to do.  However, when neither ~/.git-credential-cache/socket nor
>  ~/.cache/git/socket exists, why should we prefer the latter over the
>  former?

At least to me the latter feels superior. It keeps clutter out of the
home directory putting it in a directory with all other cache files from
other programs. If we continue to put the socket at
~/.git-credential-cache/socket by default then checking
~/.cache/git/credential/socket seems useless since the daemon will check
socket locations provided with —-socket anyways and there is no other
way to create the socket at the new location.

To me it would make sense to check the old location and if it is not 
there then use the XDG location to follow that standard. 

I guess that this is not the same behavior that git config —-global 
(it will only put its configuration file there if .gitconfig is unreadable)
has right now though so we should stay consistent and reverse the precedence
here.

Thanks for the feedback,
Devin

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

[Junio C Hamano]

Devin Lehmacher <lehmacdj@gmail.com> writes:

> +static char* get_socket_path(void) {

By the way, in our codebase (which is C not C++), the asterisk does
not stick to the type, i.e. the above should be:

	static char *get_socket_path(void)
	{

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

[Jeff King]

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

[GSoC][PATCH 3/3] Update documentation to reflect new socket location

[Devin Lehmacher]

Signed-off-by: Devin Lehmacher <lehmacdj@gmail.com>
---
 Documentation/git-credential-cache.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

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
 	network-mounted filesystem, you may need to change this to a
 	local filesystem. You must specify an absolute path.
 
-- 
2.11.0

Re: [GSoC][PATCH 3/3] Update documentation to reflect new socket location

[Junio C Hamano]

Devin Lehmacher <lehmacdj@gmail.com> writes:

> Subject: Re: [GSoC][PATCH 3/3] Update documentation to reflect new socket location

Read the above single line and nothing else, while pretending you do
not know what this patch is about at all.  That is what readers of
"git shortlog" will see.

It may be obvious for you, but can you tell whose socket the series
has updated in 6 months after forgetting what you worked on?  The
title does not give any hint that this is about the credential cache
daemon.

> Signed-off-by: Devin Lehmacher <lehmacdj@gmail.com>
> ---
>  Documentation/git-credential-cache.txt | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> 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
>  	network-mounted filesystem, you may need to change this to a
>  	local filesystem. You must specify an absolute path.

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

[Devin Lehmacher]

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 still check the old socket location, ~/.git-credential-cache/socket
first in case the user already has a socket at that location. This
ensures that a socket previously created will be used over forcibly
switching to the new socket location.

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

I also change to documentation to reflect the new default socket
location.
---
 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
 	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))
+			return home_socket;
+		else
+			free(home_socket);
+	}
+
+	return xdg_cache_home("credential/socket");
+}
+
 int cmd_main(int argc, const char **argv)
 {
 	char *socket_path = NULL;
@@ -106,7 +120,7 @@ int cmd_main(int argc, const char **argv)
 	op = argv[0];
 
 	if (!socket_path)
-		socket_path = expand_user_path("~/.git-credential-cache/socket");
+		socket_path = get_socket_path();
 	if (!socket_path)
 		die("unable to find a suitable socket path; use --socket");
 
-- 
2.11.0

[GSoC][PATCH v2 1/2] path.c: add xdg_cache_home

[Devin Lehmacher]

We already have xdg_config_home to format paths relative to
XDG_CONFIG_HOME. Let's provide a similar function xdg_cache_home to do
the same for paths relative to XDG_CACHE_HOME.

Signed-off-by: Devin Lehmacher <lehmacdj@gmail.com>
---
 cache.h |  7 +++++++
 path.c  | 15 +++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/cache.h b/cache.h
index 8c0e64420..378a636e1 100644
--- a/cache.h
+++ b/cache.h
@@ -1169,6 +1169,13 @@ extern int is_ntfs_dotgit(const char *name);
  */
 extern char *xdg_config_home(const char *filename);
 
+/**
+ * Return a newly allocated string with the evaluation of
+ * "$XDG_CACHE_HOME/git/$filename" if $XDG_CONFIG_HOME is non-empty, otherwise
+ * "$HOME/.config/git/$filename". Return NULL upon error.
+ */
+extern char *xdg_cache_home(const char *filename);
+
 /* object replacement */
 #define LOOKUP_REPLACE_OBJECT 1
 #define LOOKUP_UNKNOWN_OBJECT 2
diff --git a/path.c b/path.c
index efcedafba..22248436b 100644
--- a/path.c
+++ b/path.c
@@ -1272,6 +1272,21 @@ char *xdg_config_home(const char *filename)
 	return NULL;
 }
 
+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;
+}
+
 GIT_PATH_FUNC(git_path_cherry_pick_head, "CHERRY_PICK_HEAD")
 GIT_PATH_FUNC(git_path_revert_head, "REVERT_HEAD")
 GIT_PATH_FUNC(git_path_squash_msg, "SQUASH_MSG")
-- 
2.11.0

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

[Devin Lehmacher]

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 still check the old socket location, ~/.git-credential-cache/socket
first in case the user already has a socket at that location. This
ensures that a socket previously created will be used over forcibly
switching to the new socket location.

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

I also change to documentation to reflect the new default socket
location.
---
 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
 	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))
+			return home_socket;
+		else
+			free(home_socket);
+	}
+
+	return xdg_cache_home("credential/socket");
+}
+
 int cmd_main(int argc, const char **argv)
 {
 	char *socket_path = NULL;
@@ -106,7 +120,7 @@ int cmd_main(int argc, const char **argv)
 	op = argv[0];
 
 	if (!socket_path)
-		socket_path = expand_user_path("~/.git-credential-cache/socket");
+		socket_path = get_socket_path();
 	if (!socket_path)
 		die("unable to find a suitable socket path; use --socket");
 
-- 
2.11.0

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

[Junio C Hamano]

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.

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

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

[GSoC][PATCH/RFC v3 0/3] Fix commit messages, check if socket is socket

[Devin Lehmacher]

I fixed all of the commit messages and the weird indentation.
I also now check that the socket is actually a socket.

What do you think of the function is_socket? Is it general enough to be put
in dir.h or unix_socket.h for use in other files? Or should it be left as is?

- Devin

[GSoC][PATCH/RFC v3 1/3] path.c: add xdg_cache_home

[Devin Lehmacher]

We already have xdg_config_home to format paths relative to
XDG_CONFIG_HOME. Let's provide a similar function xdg_cache_home to do
the same for paths relative to XDG_CACHE_HOME.

Signed-off-by: Devin Lehmacher <lehmacdj@gmail.com>
---
 cache.h |  7 +++++++
 path.c  | 15 +++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/cache.h b/cache.h
index 8c0e64420..5db29a945 100644
--- a/cache.h
+++ b/cache.h
@@ -1169,6 +1169,13 @@ extern int is_ntfs_dotgit(const char *name);
  */
 extern char *xdg_config_home(const char *filename);
 
+/**
+ * Return a newly allocated string with the evaluation of
+ * "$XDG_CACHE_HOME/git/$filename" if $XDG_CACHE_HOME is non-empty, otherwise
+ * "$HOME/.cache/git/$filename". Return NULL upon error.
+ */
+extern char *xdg_cache_home(const char *filename);
+
 /* object replacement */
 #define LOOKUP_REPLACE_OBJECT 1
 #define LOOKUP_UNKNOWN_OBJECT 2
diff --git a/path.c b/path.c
index efcedafba..22248436b 100644
--- a/path.c
+++ b/path.c
@@ -1272,6 +1272,21 @@ char *xdg_config_home(const char *filename)
 	return NULL;
 }
 
+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;
+}
+
 GIT_PATH_FUNC(git_path_cherry_pick_head, "CHERRY_PICK_HEAD")
 GIT_PATH_FUNC(git_path_revert_head, "REVERT_HEAD")
 GIT_PATH_FUNC(git_path_squash_msg, "SQUASH_MSG")
-- 
2.11.0

[GSoC][PATCH/RFC v3 2/3] credential-cache: use XDG_CACHE_HOME for socket

[Devin Lehmacher]

Make git-credential-cache follow the XDG base path specification by
default. This increases consistency with other applications and helps
keep clutter out of users' home directories.

Check the old socket location, ~/.git-credential-cache/socket and use it
instead if there is already a socket at that location rather than
forcibly creating a new socket at the new location.
If there is not a socket at that location create a new one at
$XDG_CACHE_HOME/git/credential/socket following XDG base path
specification. Use the subdirectory credential/ in case other files are
stored under $XDG_CACHE_HOME/git/ in the future and to make the socket's
purpose clear.

Signed-off-by: Devin Lehmacher <lehmacdj@gmail.com>
Reviewed-by: Junio C Hamano, Jeff King
---
 Documentation/git-credential-cache.txt | 10 ++++++----
 credential-cache.c                     | 16 +++++++++++++++-
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-credential-cache.txt b/Documentation/git-credential-cache.txt
index 96208f822..fce6319e8 100644
--- a/Documentation/git-credential-cache.txt
+++ b/Documentation/git-credential-cache.txt
@@ -33,10 +33,12 @@ OPTIONS
 --socket <path>::
 
 	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
-	network-mounted filesystem, you may need to change this to a
-	local filesystem. You must specify an absolute path.
+	cache daemon if one is not started).
+	Defaults to `~/.git-credential-cache/socket` if it exists and
+	`$XDG_CACHE_HOME/git/credential/socket` otherwise.
+	If your home directory is on a network-mounted filesystem, you
+	may need to change this to a local filesystem. You must specify
+	an absolute path.
 
 CONTROLLING THE DAEMON
 ----------------------
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))
+			return home_socket;
+		else
+			free(home_socket);
+	}
+
+	return xdg_cache_home("credential/socket");
+}
+
 int cmd_main(int argc, const char **argv)
 {
 	char *socket_path = NULL;
@@ -106,7 +120,7 @@ int cmd_main(int argc, const char **argv)
 	op = argv[0];
 
 	if (!socket_path)
-		socket_path = expand_user_path("~/.git-credential-cache/socket");
+		socket_path = get_socket_path();
 	if (!socket_path)
 		die("unable to find a suitable socket path; use --socket");
 
-- 
2.11.0

Re: [GSoC][PATCH/RFC v3 2/3] credential-cache: use XDG_CACHE_HOME for socket

[Junio C Hamano]

Devin Lehmacher <lehmacdj@gmail.com> writes:

> Signed-off-by: Devin Lehmacher <lehmacdj@gmail.com>
> Reviewed-by: Junio C Hamano, Jeff King

The last line is premature; neither of us reviewed this exact
version and haven't checked if all the issues we mentioned in the
previous review have been addressed.


> +static char *get_socket_path(void) {

Compare ths line with how cmd_main() below is declared.  Notice
any difference?  We begin our functions like this:

	static char *get_socket_path(void)
	{

[GSoC][PATCH/RFC v3 3/3] credential-cache: only use user_socket if a socket

[Devin Lehmacher]

Create function is_socket.
Make get_socket_path return check if ~/.git-credential-cache/socket is a
socket and not just a file. If file_exists behavior could change in an
unexpected way. Additionally a file at ~/.git-credential-cache/socket
could cause false positives which would otherwise lead to crashes.

Signed-off-by: Devin Lehmacher <lehmacdj@gmail.com>
---
 credential-cache.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/credential-cache.c b/credential-cache.c
index db1343b46..63236adc2 100644
--- a/credential-cache.c
+++ b/credential-cache.c
@@ -83,12 +83,18 @@ static void do_cache(const char *socket, const char *action, int timeout,
 	strbuf_release(&buf);
 }
 
+static int is_socket(char *path) {
+	struct stat sb;
+	int ret = lstat(path, &sb);
+	return ret && S_IFSOCK(sb.st_mode);
+}
+
 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))
+		if (is_socket(home_socket))
 			return home_socket;
 		else
 			free(home_socket);
-- 
2.11.0

Re: [GSoC][PATCH/RFC v3 3/3] credential-cache: only use user_socket if a socket

[Devin Lehmacher]

> +static int is_socket(char *path) {
> +	struct stat sb;
> +	int ret = lstat(path, &sb);
> +	return ret && S_IFSOCK(sb.st_mode);
> +}

This patch won’t even compile. S_IFSOCK(sb.st_mode) should have been S_IFSOCK & sb.st_mode.

(I guess I should have compiled first)

After making that change this patch compiles (currently running tests).

-Devin

Re: [GSoC][PATCH/RFC v3 3/3] credential-cache: only use user_socket if a socket

[Brandon Williams]

On 03/13, Devin Lehmacher wrote:
> > +static int is_socket(char *path) {
> > +	struct stat sb;
> > +	int ret = lstat(path, &sb);
> > +	return ret && S_IFSOCK(sb.st_mode);
> > +}
> 
> This patch won’t even compile. S_IFSOCK(sb.st_mode) should have been S_IFSOCK & sb.st_mode.
> 
> (I guess I should have compiled first)
> 
> After making that change this patch compiles (currently running tests).

Best practice for submitting patches would be to ensure that each patch
compiles without errors (with the DEVELOPER=1 flag set) and that the
entire test suite passes with no errors; this is to maintain
bisect-ability.  Only after you've done this should you send your
patches to the mailing list.

-- 
Brandon Williams

Re: [GSoC][PATCH/RFC v3 3/3] credential-cache: only use user_socket if a socket

[Devin Lehmacher]

> Best practice for submitting patches would be to ensure that each patch
> compiles without errors (with the DEVELOPER=1 flag set) and that the
> entire test suite passes with no errors; this is to maintain
> bisect-ability.  Only after you've done this should you send your
> patches to the mailing list.

Thanks for the advice. I will be more careful in the future.

-Devin

Re: [GSoC][PATCH/RFC v3 3/3] credential-cache: only use user_socket if a socket

[Brandon Williams]

On 03/13, Devin Lehmacher wrote:
> > Best practice for submitting patches would be to ensure that each patch
> > compiles without errors (with the DEVELOPER=1 flag set) and that the
> > entire test suite passes with no errors; this is to maintain
> > bisect-ability.  Only after you've done this should you send your
> > patches to the mailing list.
> 
> Thanks for the advice. I will be more careful in the future.

Of course! And no worries, I just wanted you to be aware of the
conventions so that you have an easier time contributing :)

-- 
Brandon Williams

Re: [GSoC][PATCH/RFC v3 3/3] credential-cache: only use user_socket if a socket

[Junio C Hamano]

Devin Lehmacher <lehmacdj@gmail.com> writes:

> diff --git a/credential-cache.c b/credential-cache.c
> index db1343b46..63236adc2 100644
> --- a/credential-cache.c
> +++ b/credential-cache.c
> @@ -83,12 +83,18 @@ static void do_cache(const char *socket, const char *action, int timeout,
>  	strbuf_release(&buf);
>  }
>  
> +static int is_socket(char *path) {
> +	struct stat sb;
> +	int ret = lstat(path, &sb);
> +	return ret && S_IFSOCK(sb.st_mode);
> +}
> +
>  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))
> +		if (is_socket(home_socket))

This should be done as part of 2/3, no?  It does not make sense to
add 2/3 and then immediately say "oops, the check in 2/3 is wrong,
and let's update it like so".

Re: [GSoC][PATCH/RFC v3 3/3] credential-cache: only use user_socket if a socket

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Devin Lehmacher <lehmacdj@gmail.com> writes:
>
>> diff --git a/credential-cache.c b/credential-cache.c
>> index db1343b46..63236adc2 100644
>> --- a/credential-cache.c
>> +++ b/credential-cache.c
>> @@ -83,12 +83,18 @@ static void do_cache(const char *socket, const char *action, int timeout,
>>  	strbuf_release(&buf);
>>  }
>>  
>> +static int is_socket(char *path) {
>> +	struct stat sb;
>> +	int ret = lstat(path, &sb);
>> +	return ret && S_IFSOCK(sb.st_mode);
>> +}
>> +
>>  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))
>> +		if (is_socket(home_socket))
>
> This should be done as part of 2/3, no?  It does not make sense to
> add 2/3 and then immediately say "oops, the check in 2/3 is wrong,
> and let's update it like so".

Also I think you would want to use S_ISFIFO() and/or S_ISSOCK()
macros (I do not offhand recall which one credential cache daemon
uses), not the S_IFxxx constant.

Perhaps 

   1/3 - add xdg_cache similar to xdg_config

   2/3 - add is_socket()

   3/3 - use xdg_cache location for socket if traditional location
         is not in use

would be a better logical ordering of the patches.

Having said that, I do not think ~/.git-credential-cache/socket is
the right thing to test.  If the ~/.git-credential-cache directory
already exists, it is likely that the user has a set-up that works
well with "socket" inside it, and it is safer to keep using that
location.  

On the other hand, if that directory does not exist, we know it is
safe to use whatever new location---after all, the lack of the
directory tells us that the user has never used the traditional
location successfully ;-)

So is_socket() may not even be needed, in which case this will be a
two-patch series:

   1/2 - add xdg_cache similar to xdg_config
   2/2 - use xdg_cache location if ~/.git-credential-cache/ is not there

Re: [GSoC][PATCH/RFC v3 3/3] credential-cache: only use user_socket if a socket

[Devin Lehmacher]

> Also I think you would want to use S_ISFIFO() and/or S_ISSOCK()
> macros (I do not offhand recall which one credential cache daemon
> uses), not the S_IFxxx constant.

Huh. These macros are not on the man page for lstat.

> Having said that, I do not think ~/.git-credential-cache/socket is
> the right thing to test.  If the ~/.git-credential-cache directory
> already exists, it is likely that the user has a set-up that works
> well with "socket" inside it, and it is safer to keep using that
> location.
>
> On the other hand, if that directory does not exist, we know it is
> safe to use whatever new location---after all, the lack of the
> directory tells us that the user has never used the traditional
> location successfully ;-)

That makes sense

-Devin

[GSoC][PATCH v4 0/4] Moving credential-cache socket to xdg path

[Devin Lehmacher]

I implemented all changes from the previous reviews and also wrote a few
tests.

-Devin

[GSoC][PATCH v4 1/4] path.c: add xdg_cache_home

[Devin Lehmacher]

We already have xdg_config_home to format paths relative to
XDG_CONFIG_HOME. Let's provide a similar function xdg_cache_home to do
the same for paths relative to XDG_CACHE_HOME.

Signed-off-by: Devin Lehmacher <lehmacdj@gmail.com>
---
 cache.h |  7 +++++++
 path.c  | 15 +++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/cache.h b/cache.h
index c95826971..66761bc56 100644
--- a/cache.h
+++ b/cache.h
@@ -1169,6 +1169,13 @@ extern int is_ntfs_dotgit(const char *name);
  */
 extern char *xdg_config_home(const char *filename);
 
+/**
+ * Return a newly allocated string with the evaluation of
+ * "$XDG_CACHE_HOME/git/$filename" if $XDG_CACHE_HOME is non-empty, otherwise
+ * "$HOME/.cache/git/$filename". Return NULL upon error.
+ */
+extern char *xdg_cache_home(const char *filename);
+
 /* object replacement */
 #define LOOKUP_REPLACE_OBJECT 1
 #define LOOKUP_UNKNOWN_OBJECT 2
diff --git a/path.c b/path.c
index efcedafba..22248436b 100644
--- a/path.c
+++ b/path.c
@@ -1272,6 +1272,21 @@ char *xdg_config_home(const char *filename)
 	return NULL;
 }
 
+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;
+}
+
 GIT_PATH_FUNC(git_path_cherry_pick_head, "CHERRY_PICK_HEAD")
 GIT_PATH_FUNC(git_path_revert_head, "REVERT_HEAD")
 GIT_PATH_FUNC(git_path_squash_msg, "SQUASH_MSG")
-- 
2.11.0

[GSoC][PATCH v4 2/4] dir: add directory_exists

[Devin Lehmacher]

file_exists currently only does an lstat to see if anything is at the
path. For some purposes it would be better to ensure that a path
is a certain type of file. Add function directory_exists that checks if
a path exists and is a directory.

Signed-off-by: Devin Lehmacher <lehmacdj@gmail.com>
---
 dir.c | 7 +++++++
 dir.h | 1 +
 2 files changed, 8 insertions(+)

diff --git a/dir.c b/dir.c
index aeeb5ce10..f6c23ecd2 100644
--- a/dir.c
+++ b/dir.c
@@ -2055,6 +2055,13 @@ int file_exists(const char *f)
 	return lstat(f, &sb) == 0;
 }
 
+int directory_exists(const char *path)
+{
+	struct stat sb;
+	int ret = lstat(path, &sb);
+	return ret == 0 && S_ISDIR(sb.st_mode);
+}
+
 static int cmp_icase(char a, char b)
 {
 	if (a == b)
diff --git a/dir.h b/dir.h
index bf23a470a..9f8278795 100644
--- a/dir.h
+++ b/dir.h
@@ -247,6 +247,7 @@ extern void add_exclude(const char *string, const char *base,
 extern void clear_exclude_list(struct exclude_list *el);
 extern void clear_directory(struct dir_struct *dir);
 extern int file_exists(const char *);
+extern int directory_exists(const char *);
 
 extern int is_inside_dir(const char *dir);
 extern int dir_inside_of(const char *subdir, const char *dir);
-- 
2.11.0

Re: [GSoC][PATCH v4 2/4] dir: add directory_exists

[Junio C Hamano]

Devin Lehmacher <lehmacdj@gmail.com> writes:

> +int directory_exists(const char *path)
> +{
> +	struct stat sb;
> +	int ret = lstat(path, &sb);
> +	return ret == 0 && S_ISDIR(sb.st_mode);
> +}

I am not a great fan of using file_exists() [*1*] on anything other
than paths in the working tree (i.e. in preparation for checking
things out of or into the index), as paths inside .git/ and paths
outside have different requirements.  One of the difference is if it
makes sense to use stat(2) or lstat(2) for a check like this
function does.  For working tree entities, which file_exists() and
friends are designed for, we absolutely should use lstat(2).  The
"RelNotes" symbolic link at the top level of the project must be
known as a symbolic link and a question "is this a directory?" on it
must be answered "no", for example.

But there is no fundamental reason ~/.git-credential-cache (or
anything outside the working tree) must be S_ISDIR().  If the user
wanted to have the real directory elsewhere and point at it with
a symbolic link ~/.git-credential-cache, we should allow it.

If we need a helper function to see if a path in the working tree is
a directory, adding this new helper function to dir.c (which is
about the paths in the working tree) and use lstat(2) in it is the
right thing to do.  But I do not think it should be used to check if
~/.git-credential-cache directory, which is not a filesystem entity
in a working tree, exists.

IOW, I do not think this patch helps the topic of this series.  Drop
this patch from the series and have a similar code (but use stat(2)
instead of lstat(2)) directly inside get_socket_path()'s in the next
patch, perhaps?


[Footnote]

*1* ... and friends, like safe_create_leading_directories().

Re: [GSoC][PATCH v4 2/4] dir: add directory_exists

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Devin Lehmacher <lehmacdj@gmail.com> writes:
>
>> +int directory_exists(const char *path)
>> +{
>> +	struct stat sb;
>> +	int ret = lstat(path, &sb);
>> +	return ret == 0 && S_ISDIR(sb.st_mode);
>> +}
>
> I am not a great fan of using file_exists() [*1*] on anything other
> than paths in the working tree (i.e. in preparation for checking
> things out of or into the index), as ...
> ...
> [Footnote]
>
> *1* ... and friends, like safe_create_leading_directories().

I actually got things mixed up.  safe-create-leading-directories is
to be used only for paths inside .git/ (i.e. not for working tree
files, but for things like ".git/refs/heads/foo" when creating a
branch called foo/bar).

[GSoC][PATCH v4 3/4] credential-cache: use XDG_CACHE_HOME for socket

[Devin Lehmacher]

Make git-credential-cache follow the XDG base path specification by
default. This increases consistency with other applications and helps
keep clutter out of users' home directories.

Check the old socket location, ~/.git-credential-cache/, and use
~/.git-credential-cache/socket if that directory exists rather than
forcing users who have used `git credential-cache` before to migrate to
the new XDG compliant location.
Otherwise use the socket $XDG_CACHE_HOME/git/credential/socket following
XDG base path specification. Use the subdirectory credential/ in case
other files are cached under $XDG_CACHE_HOME/git/ in the future and to
make the socket's purpose clear.

Signed-off-by: Devin Lehmacher <lehmacdj@gmail.com>
---
 Documentation/git-credential-cache.txt | 11 +++++++----
 credential-cache.c                     | 15 ++++++++++++++-
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-credential-cache.txt b/Documentation/git-credential-cache.txt
index 96208f822..2b8582639 100644
--- a/Documentation/git-credential-cache.txt
+++ b/Documentation/git-credential-cache.txt
@@ -33,10 +33,13 @@ OPTIONS
 --socket <path>::
 
 	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
-	network-mounted filesystem, you may need to change this to a
-	local filesystem. You must specify an absolute path.
+	cache daemon if one is not started).
+	Defaults to `$XDG_CACHE_HOME/git/credential/socket` unless
+	`~/.git-credential-cache/` exists in which case
+	`~/.git-credential-cache/socket` is used instead.
+	If your home directory is on a network-mounted filesystem, you
+	may need to change this to a local filesystem. You must specify
+	an absolute path.
 
 CONTROLLING THE DAEMON
 ----------------------
diff --git a/credential-cache.c b/credential-cache.c
index cc8a6ee19..1fd800775 100644
--- a/credential-cache.c
+++ b/credential-cache.c
@@ -1,5 +1,6 @@
 #include "cache.h"
 #include "credential.h"
+#include "dir.h"
 #include "string-list.h"
 #include "parse-options.h"
 #include "unix-socket.h"
@@ -83,6 +84,18 @@ static void do_cache(const char *socket, const char *action, int timeout,
 	strbuf_release(&buf);
 }
 
+static char *get_socket_path(void)
+{
+	char *old_credential_dir, *socket;
+	old_credential_dir = expand_user_path("~/.git-credential-cache");
+	if (old_credential_dir && directory_exists(old_credential_dir))
+		socket = expand_user_path("~/.git-credential-cache/socket");
+	else
+		socket = xdg_cache_home("credential/socket");
+	free(old_credential_dir);
+	return socket;
+}
+
 int cmd_main(int argc, const char **argv)
 {
 	char *socket_path = NULL;
@@ -106,7 +119,7 @@ int cmd_main(int argc, const char **argv)
 	op = argv[0];
 
 	if (!socket_path)
-		socket_path = expand_user_path("~/.git-credential-cache/socket");
+		socket_path = get_socket_path();
 	if (!socket_path)
 		die("unable to find a suitable socket path; use --socket");
 
-- 
2.11.0

Re: [GSoC][PATCH v4 3/4] credential-cache: use XDG_CACHE_HOME for socket

[Junio C Hamano]

Devin Lehmacher <lehmacdj@gmail.com> writes:

> +	If your home directory is on a network-mounted filesystem, you
> +	may need to change this to a local filesystem. You must specify
> +	an absolute path.

Nicely explained. 

If a socket cannot be created in "~/.git-credential-cache", existing
users would have created a symbolic link that points at a local
directory.  With XDG_CACHE_HOME mechanism, they would just set the
variable to directly point at a local directory and there is no need
for symlink trick, on the other hand.

Which points us back to the observation I made in my review on the
previous step.

> +static char *get_socket_path(void)
> +{
> +	char *old_credential_dir, *socket;
> +	old_credential_dir = expand_user_path("~/.git-credential-cache");
> +	if (old_credential_dir && directory_exists(old_credential_dir))
> +		socket = expand_user_path("~/.git-credential-cache/socket");
> +	else
> +		socket = xdg_cache_home("credential/socket");
> +	free(old_credential_dir);
> +	return socket;
> +}

As we do not want to use the dir.c::directory_exists(), which is
meant to be used for working tree files, we can do something like
this instead:

static char *get_socket_path(void)
{
	struct stat st;
	char *path;

	path = expand_user_path("~/.git-credential-cache");
	if (path && !stat(path, &st) && S_ISDIR(st.st_mode))) {
		free(path);
		path = expand_user_path("~/.git-credential-cache/socket");
	} else {
		path = xdg_cache_home("credential/socket");
	}                
	return path;
}

The duplication of "~/.git-credential-cache" bothers me somewhat and
perhaps people can suggest better ways to get rid of the dup.

Other than that, makes sense to me.

Thanks.

Re: [GSoC][PATCH v4 3/4] credential-cache: use XDG_CACHE_HOME for socket

[Jeff King]

On Thu, Mar 16, 2017 at 09:20:29AM -0700, Junio C Hamano wrote:

> static char *get_socket_path(void)
> {
> 	struct stat st;
> 	char *path;
> 
> 	path = expand_user_path("~/.git-credential-cache");
> 	if (path && !stat(path, &st) && S_ISDIR(st.st_mode))) {
> 		free(path);
> 		path = expand_user_path("~/.git-credential-cache/socket");
> 	} else {
> 		path = xdg_cache_home("credential/socket");
> 	}                
> 	return path;
> }
> 
> The duplication of "~/.git-credential-cache" bothers me somewhat and
> perhaps people can suggest better ways to get rid of the dup.

Maybe:

  path = expand_user_path("~/.git-credential-cache");
  if (path && !stat(path, &st) && S_ISDIR(st.st_mode))) {
	char *new_path = xstrfmt("%s/socket", path);
	free(path);
	path = new_path;
  } else ...

-Peff

[GSoC][PATCH v4 4/4] credential-cache: add tests for XDG functionality

[Devin Lehmacher]

Signed-off-by: Devin Lehmacher <lehmacdj@gmail.com>
---
 t/t0301-credential-cache.sh | 64 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh
index 82c841121..664c6dda1 100755
--- a/t/t0301-credential-cache.sh
+++ b/t/t0301-credential-cache.sh
@@ -12,6 +12,7 @@ test -z "$NO_UNIX_SOCKETS" || {
 # don't leave a stale daemon running
 trap 'code=$?; git credential-cache exit; (exit $code); die' EXIT
 
+# test that the daemon works with no special setup
 helper_test cache
 helper_test_timeout cache --timeout=1
 
@@ -20,4 +21,67 @@ helper_test_timeout cache --timeout=1
 # our socket, leaving us with no way to access the daemon.
 git credential-cache exit
 
+# we need to use rm -rf here since sometimes the daemon hasn't finished
+# cleaning up after itself and rmdir fails
+test_expect_success 'credential-cache --socket option overrides default location' '
+	test_when_finished "rm -rf \"$HOME\"/dir/" &&
+	check approve "cache --socket \"$HOME/dir/socket\"" <<-\EOF &&
+	protocol=https
+	host=example.com
+	username=store-user
+	password=store-pass
+	EOF
+	test -S "$HOME/dir/socket" &&
+	git credential-cache exit
+'
+
+XDG_CACHE_HOME="$HOME/xdg"
+export XDG_CACHE_HOME
+# test behavior when XDG_CACHE_HOME is set
+helper_test cache
+
+test_expect_success "use custom XDG_CACHE_HOME if set and default sockets are not created" '
+	test -S "$XDG_CACHE_HOME/git/credential/socket" &&
+	test_path_is_missing "$HOME/.git-credential-cache/socket" &&
+	test_path_is_missing "$HOME/.cache/git/credential/socket" &&
+	git credential-cache exit
+'
+unset XDG_CACHE_HOME
+
+test_expect_success "use custom XDG_CACHE_HOME even if xdg socket exists" '
+	check approve cache <<-\EOF &&
+	protocol=https
+	host=example.com
+	username=store-user
+	password=store-pass
+	EOF
+	test -S "$HOME/.cache/git/credential/socket" &&
+	XDG_CACHE_HOME="$HOME/xdg" &&
+	export XDG_CACHE_HOME &&
+	check approve cache <<-\EOF &&
+	protocol=https
+	host=example.com
+	username=store-user
+	password=store-pass
+	EOF
+	test -S "$HOME/xdg/git/credential/socket" &&
+	git credential-cache exit &&
+	unset XDG_CACHE_HOME
+'
+
+# we need to use rm -rf here since sometimes the daemon hasn't finished
+# cleaning up after itself and rmdir fails
+test_expect_success 'use user socket if user directory exists' '
+	test_when_finished "rm -rf \"$HOME/.git-credential-cache/\"" &&
+	mkdir -p -m 700 "$HOME/.git-credential-cache/" &&
+	check approve cache <<-\EOF &&
+	protocol=https
+	host=example.com
+	username=store-user
+	password=store-pass
+	EOF
+	test -S "$HOME/.git-credential-cache/socket" &&
+	git credential-cache exit
+'
+
 test_done
-- 
2.11.0

Re: [GSoC][PATCH v4 4/4] credential-cache: add tests for XDG functionality

[Junio C Hamano]

Devin Lehmacher <lehmacdj@gmail.com> writes:

> @@ -20,4 +21,67 @@ helper_test_timeout cache --timeout=1
>  # our socket, leaving us with no way to access the daemon.
>  git credential-cache exit
>  
> +# we need to use rm -rf here since sometimes the daemon hasn't finished
> +# cleaning up after itself and rmdir fails

Hmmmm.  Peff, do you have ideas on better ways to do this (or
explanation why this is the best we could do)?

> +test_expect_success 'credential-cache --socket option overrides default location' '
> +	test_when_finished "rm -rf \"$HOME\"/dir/" &&
> +	check approve "cache --socket \"$HOME/dir/socket\"" <<-\EOF &&
> +	protocol=https
> +	host=example.com
> +	username=store-user
> +	password=store-pass
> +	EOF
> +	test -S "$HOME/dir/socket" &&
> +	git credential-cache exit
> +'
> +
> +XDG_CACHE_HOME="$HOME/xdg"
> +export XDG_CACHE_HOME
> +# test behavior when XDG_CACHE_HOME is set
> +helper_test cache
> +
> +test_expect_success "use custom XDG_CACHE_HOME if set and default sockets are not created" '
> +	test -S "$XDG_CACHE_HOME/git/credential/socket" &&
> +	test_path_is_missing "$HOME/.git-credential-cache/socket" &&
> +	test_path_is_missing "$HOME/.cache/git/credential/socket" &&
> +	git credential-cache exit
> +'
> +unset XDG_CACHE_HOME
> +
> +test_expect_success "use custom XDG_CACHE_HOME even if xdg socket exists" '
> +	check approve cache <<-\EOF &&
> +	protocol=https
> +	host=example.com
> +	username=store-user
> +	password=store-pass
> +	EOF
> +	test -S "$HOME/.cache/git/credential/socket" &&
> +	XDG_CACHE_HOME="$HOME/xdg" &&
> +	export XDG_CACHE_HOME &&
> +	check approve cache <<-\EOF &&
> +	protocol=https
> +	host=example.com
> +	username=store-user
> +	password=store-pass
> +	EOF
> +	test -S "$HOME/xdg/git/credential/socket" &&
> +	git credential-cache exit &&
> +	unset XDG_CACHE_HOME

This unset will not run if any of the above steps since it was set
and exported fails.  It probably should be in test_when_finished and
should use safe_unset shell function instead.

> +'
> +
> +# we need to use rm -rf here since sometimes the daemon hasn't finished
> +# cleaning up after itself and rmdir fails
> +test_expect_success 'use user socket if user directory exists' '
> +	test_when_finished "rm -rf \"$HOME/.git-credential-cache/\"" &&
> +	mkdir -p -m 700 "$HOME/.git-credential-cache/" &&
> +	check approve cache <<-\EOF &&
> +	protocol=https
> +	host=example.com
> +	username=store-user
> +	password=store-pass
> +	EOF
> +	test -S "$HOME/.git-credential-cache/socket" &&

We should also test that the XDG location is not touched at the same
time that the traditional directory was used for the socket.

> +	git credential-cache exit
> +'

This last test should be replicated to also check the case where we
have a symbolic link at ~/.git-credential-cache that points at a
directory.

>  test_done

Thanks.

Re: [GSoC][PATCH v4 4/4] credential-cache: add tests for XDG functionality

[Jeff King]

On Thu, Mar 16, 2017 at 09:29:58AM -0700, Junio C Hamano wrote:

> Devin Lehmacher <lehmacdj@gmail.com> writes:
> 
> > @@ -20,4 +21,67 @@ helper_test_timeout cache --timeout=1
> >  # our socket, leaving us with no way to access the daemon.
> >  git credential-cache exit
> >  
> > +# we need to use rm -rf here since sometimes the daemon hasn't finished
> > +# cleaning up after itself and rmdir fails
> 
> Hmmmm.  Peff, do you have ideas on better ways to do this (or
> explanation why this is the best we could do)?

If you call "git credential-cache exit", that should be deterministic.
The client program won't exit until the other side closes the
descriptor, which it won't do until it has cleaned up the socket. See
the comment at line 130 of credential-cache--daemon.c.

So here:

> > +test_expect_success 'credential-cache --socket option overrides default location' '
> > +	test_when_finished "rm -rf \"$HOME\"/dir/" &&
> > +	check approve "cache --socket \"$HOME/dir/socket\"" <<-\EOF &&
> > +	protocol=https
> > +	host=example.com
> > +	username=store-user
> > +	password=store-pass
> > +	EOF
> > +	test -S "$HOME/dir/socket" &&
> > +	git credential-cache exit
> > +'

This is almost right, except:

  - the "exit" needs to be told which socket to use

  - we should do the "exit" even when the test fails early (so in
    test_when_finished)

  - the test_when_finished block will interpolate $HOME when setting up
    the block, which will break if it contains double-quotes or other
    special characters. It should use \$HOME.

    I suspect the "check" invocation needs to do so as well, though it
    is even trickier (we shove it into a single-quoted "-c" argument).

    I think you could get by in both cases with relative paths.

So all together, probably:

  test_when_finished "
	git credential-cache --socket dir/socket &&
	rmdir dir
  " &&
  check approve "cache --socket dir/socket" <<-\EOF &&
  ...
  EOF
  test -S dir/socket

The final "test -S" should be OK in practice. We're assuming that the
cache-daemon is still running, but it has a 900 second timeout by
default (and besides, all the other tests have exact same race).

-Peff

[GSoC][PATCH v5 1/3] path.c: add xdg_cache_home

[Devin Lehmacher]

We already have xdg_config_home to format paths relative to
XDG_CONFIG_HOME. Let's provide a similar function xdg_cache_home to do
the same for paths relative to XDG_CACHE_HOME.

Signed-off-by: Devin Lehmacher <lehmacdj@gmail.com>
---
 cache.h |  7 +++++++
 path.c  | 15 +++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/cache.h b/cache.h
index c95826971..66761bc56 100644
--- a/cache.h
+++ b/cache.h
@@ -1169,6 +1169,13 @@ extern int is_ntfs_dotgit(const char *name);
  */
 extern char *xdg_config_home(const char *filename);
 
+/**
+ * Return a newly allocated string with the evaluation of
+ * "$XDG_CACHE_HOME/git/$filename" if $XDG_CACHE_HOME is non-empty, otherwise
+ * "$HOME/.cache/git/$filename". Return NULL upon error.
+ */
+extern char *xdg_cache_home(const char *filename);
+
 /* object replacement */
 #define LOOKUP_REPLACE_OBJECT 1
 #define LOOKUP_UNKNOWN_OBJECT 2
diff --git a/path.c b/path.c
index efcedafba..22248436b 100644
--- a/path.c
+++ b/path.c
@@ -1272,6 +1272,21 @@ char *xdg_config_home(const char *filename)
 	return NULL;
 }
 
+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;
+}
+
 GIT_PATH_FUNC(git_path_cherry_pick_head, "CHERRY_PICK_HEAD")
 GIT_PATH_FUNC(git_path_revert_head, "REVERT_HEAD")
 GIT_PATH_FUNC(git_path_squash_msg, "SQUASH_MSG")
-- 
2.11.0

[GSoC][PATCH v5 2/3] credential-cache: use XDG_CACHE_HOME for socket

[Devin Lehmacher]

Make git-credential-cache follow the XDG base path specification by
default. This increases consistency with other applications and helps
keep clutter out of users' home directories.

Check the old socket location, ~/.git-credential-cache/, and use
~/.git-credential-cache/socket if that directory exists rather than
forcing users who have used `git credential-cache` before to migrate to
the new XDG compliant location.
Otherwise use the socket $XDG_CACHE_HOME/git/credential/socket following
XDG base path specification. Use the subdirectory credential/ in case
other files are cached under $XDG_CACHE_HOME/git/ in the future and to
make the socket's purpose clear.

Signed-off-by: Devin Lehmacher <lehmacdj@gmail.com>
---
 Documentation/git-credential-cache.txt | 11 +++++++----
 credential-cache.c                     | 15 ++++++++++++++-
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-credential-cache.txt b/Documentation/git-credential-cache.txt
index 96208f822..2b8582639 100644
--- a/Documentation/git-credential-cache.txt
+++ b/Documentation/git-credential-cache.txt
@@ -33,10 +33,13 @@ OPTIONS
 --socket <path>::
 
 	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
-	network-mounted filesystem, you may need to change this to a
-	local filesystem. You must specify an absolute path.
+	cache daemon if one is not started).
+	Defaults to `$XDG_CACHE_HOME/git/credential/socket` unless
+	`~/.git-credential-cache/` exists in which case
+	`~/.git-credential-cache/socket` is used instead.
+	If your home directory is on a network-mounted filesystem, you
+	may need to change this to a local filesystem. You must specify
+	an absolute path.
 
 CONTROLLING THE DAEMON
 ----------------------
diff --git a/credential-cache.c b/credential-cache.c
index cc8a6ee19..3cbd42001 100644
--- a/credential-cache.c
+++ b/credential-cache.c
@@ -83,6 +83,19 @@ static void do_cache(const char *socket, const char *action, int timeout,
 	strbuf_release(&buf);
 }
 
+static char *get_socket_path(void)
+{
+	struct stat sb;
+	char *old_dir, *socket;
+	old_dir = expand_user_path("~/.git-credential-cache");
+	if (old_dir && !stat(old_dir, &sb) && S_ISDIR(sb.st_mode))
+		socket = xstrfmt("%s/socket", old_dir);
+	else
+		socket = xdg_cache_home("credential/socket");
+	free(old_dir);
+	return socket;
+}
+
 int cmd_main(int argc, const char **argv)
 {
 	char *socket_path = NULL;
@@ -106,7 +119,7 @@ int cmd_main(int argc, const char **argv)
 	op = argv[0];
 
 	if (!socket_path)
-		socket_path = expand_user_path("~/.git-credential-cache/socket");
+		socket_path = get_socket_path();
 	if (!socket_path)
 		die("unable to find a suitable socket path; use --socket");
 
-- 
2.11.0

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

[Ramsay Jones]

On 17/03/17 02:53, Devin Lehmacher wrote:
> Make git-credential-cache follow the XDG base path specification by
> default. This increases consistency with other applications and helps
> keep clutter out of users' home directories.
> 
> Check the old socket location, ~/.git-credential-cache/, and use
> ~/.git-credential-cache/socket if that directory exists rather than
> forcing users who have used `git credential-cache` before to migrate to
> the new XDG compliant location.
> Otherwise use the socket $XDG_CACHE_HOME/git/credential/socket following
> XDG base path specification. Use the subdirectory credential/ in case
> other files are cached under $XDG_CACHE_HOME/git/ in the future and to
> make the socket's purpose clear.
> 
> Signed-off-by: Devin Lehmacher <lehmacdj@gmail.com>
> ---
>  Documentation/git-credential-cache.txt | 11 +++++++----
>  credential-cache.c                     | 15 ++++++++++++++-
>  2 files changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/git-credential-cache.txt b/Documentation/git-credential-cache.txt
> index 96208f822..2b8582639 100644
> --- a/Documentation/git-credential-cache.txt
> +++ b/Documentation/git-credential-cache.txt
> @@ -33,10 +33,13 @@ OPTIONS
>  --socket <path>::
>  
>  	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
> -	network-mounted filesystem, you may need to change this to a
> -	local filesystem. You must specify an absolute path.
> +	cache daemon if one is not started).
> +	Defaults to `$XDG_CACHE_HOME/git/credential/socket` unless

or $HOME/.cache/git/credential/socket if $XDG_CACHE_HOME is not set.
I don't have a good suggestion to re-word this paragraph. (I just
spent ten minutes trying!).

Hmm, git-credential-store doesn't mention the $HOME/.config/git/credentials
outside of the files section. (In particular, not as part of the --file
option description).

> +	`~/.git-credential-cache/` exists in which case
> +	`~/.git-credential-cache/socket` is used instead.
> +	If your home directory is on a network-mounted filesystem, you
> +	may need to change this to a local filesystem. You must specify
> +	an absolute path.

ATB,
Ramsay Jones

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

[Devin Lehmacher]

> or $HOME/.cache/git/credential/socket if $XDG_CACHE_HOME is not set.
> I don't have a good suggestion to re-word this paragraph. (I just
> spent ten minutes trying!).

I think it would not be too bad to not include this though because
`$HOME/.cache/` is what XDG specifies as the default if
`$XDG_CACHE_HOME` is unset.

Devin

[GSoC][PATCH v5 3/3] credential-cache: add tests for XDG functionality

[Devin Lehmacher]

Signed-off-by: Devin Lehmacher <lehmacdj@gmail.com>
---
 t/t0301-credential-cache.sh | 94 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 94 insertions(+)

diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh
index 82c841121..82c4dfa07 100755
--- a/t/t0301-credential-cache.sh
+++ b/t/t0301-credential-cache.sh
@@ -12,7 +12,101 @@ test -z "$NO_UNIX_SOCKETS" || {
 # don't leave a stale daemon running
 trap 'code=$?; git credential-cache exit; (exit $code); die' EXIT
 
+# test that the daemon works with no special setup
 helper_test cache
+
+test_expect_success 'socket defaults to ~/.cache/git/credential/socket' '
+	test_when_finished "
+		git credential-cache exit &&
+		rmdir -p .cache/git/credential/
+	" &&
+	test_path_is_missing "$HOME/.git-credential-cache" &&
+	test -S "$HOME/.cache/git/credential/socket"
+'
+
+XDG_CACHE_HOME="$HOME/xdg"
+export XDG_CACHE_HOME
+# test behavior when XDG_CACHE_HOME is set
+helper_test cache
+
+test_expect_success "use custom XDG_CACHE_HOME if set and default sockets are not created" '
+	test_when_finished "git credential-cache exit" &&
+	test -S "$XDG_CACHE_HOME/git/credential/socket" &&
+	test_path_is_missing "$HOME/.git-credential-cache/socket" &&
+	test_path_is_missing "$HOME/.cache/git/credential/socket"
+'
+unset XDG_CACHE_HOME
+
+test_expect_success 'credential-cache --socket option overrides default location' '
+	test_when_finished "
+		git credential-cache exit --socket \"\$HOME/dir/socket\" &&
+		rmdir \"\$HOME/dir\"
+	" &&
+	check approve "cache --socket \"\$HOME/dir/socket\"" <<-\EOF &&
+	protocol=https
+	host=example.com
+	username=store-user
+	password=store-pass
+	EOF
+	test -S "$HOME/dir/socket"
+'
+
+test_expect_success "use custom XDG_CACHE_HOME even if xdg socket exists" '
+	test_when_finished "
+		git credential-cache exit &&
+		sane_unset XDG_CACHE_HOME
+	" &&
+	check approve cache <<-\EOF &&
+	protocol=https
+	host=example.com
+	username=store-user
+	password=store-pass
+	EOF
+	find . &&
+	test -S "$HOME/.cache/git/credential/socket" &&
+	XDG_CACHE_HOME="$HOME/xdg" &&
+	export XDG_CACHE_HOME &&
+	check approve cache <<-\EOF &&
+	protocol=https
+	host=example.com
+	username=store-user
+	password=store-pass
+	EOF
+	test -S "$XDG_CACHE_HOME/git/credential/socket"
+'
+
+test_expect_success 'use user socket if user directory exists' '
+	test_when_finished "
+		git credential-cache exit &&
+		rmdir \"\$HOME/.git-credential-cache/\"
+	" &&
+	mkdir -p -m 700 "$HOME/.git-credential-cache/" &&
+	check approve cache <<-\EOF &&
+	protocol=https
+	host=example.com
+	username=store-user
+	password=store-pass
+	EOF
+	test -S "$HOME/.git-credential-cache/socket"
+'
+
+test_expect_success 'use user socket if user directory is a symlink to a directory' '
+	test_when_finished "
+		git credential-cache exit &&
+		rmdir \"\$HOME/dir/\" &&
+		rm \"\$HOME/.git-credential-cache\"
+	" &&
+	mkdir -p -m 700 "$HOME/dir/" &&
+	ln -s "$HOME/dir" "$HOME/.git-credential-cache" &&
+	check approve cache <<-\EOF &&
+	protocol=https
+	host=example.com
+	username=store-user
+	password=store-pass
+	EOF
+	test -S "$HOME/.git-credential-cache/socket"
+'
+
 helper_test_timeout cache --timeout=1
 
 # we can't rely on our "trap" above working after test_done,
-- 
2.11.0

Re: [GSoC][PATCH v5 3/3] credential-cache: add tests for XDG functionality

[Junio C Hamano]

Devin Lehmacher <lehmacdj@gmail.com> writes:

> +# test that the daemon works with no special setup
>  helper_test cache
> +
> +test_expect_success 'socket defaults to ~/.cache/git/credential/socket' '
> +	test_when_finished "
> +		git credential-cache exit &&
> +		rmdir -p .cache/git/credential/
> +	" &&
> +	test_path_is_missing "$HOME/.git-credential-cache" &&
> +	test -S "$HOME/.cache/git/credential/socket"
> +'

OK, the clean-up looks quite sensible.

> +test_expect_success "use custom XDG_CACHE_HOME even if xdg socket exists" '
> +...
> +	password=store-pass
> +	EOF
> +	find . &&

A leftover from a debugging session?  If so remove the "find" line.

> +	test -S "$HOME/.cache/git/credential/socket" &&
> +	XDG_CACHE_HOME="$HOME/xdg" &&
> ...
> +'

> +test_expect_success 'use user socket if user directory is a symlink to a directory' '

I think anybody without NO_UNIX_SOCKETS is practically capable of
doing a symbolic link, but still it is a good idea to protect this
one behind the SYMLINKS prerequisite, i.e.

	test_expect_success SYMLINKS 'use ~/.git-credential-cache that is a symlink' '
		...

> +	test_when_finished "
> +...
> +'
> +
>  helper_test_timeout cache --timeout=1
>  
>  # we can't rely on our "trap" above working after test_done,

Thanks.

[GSoC][PATCH v6 1/3] path.c: add xdg_cache_home

[Devin Lehmacher]

We already have xdg_config_home to format paths relative to
XDG_CONFIG_HOME. Let's provide a similar function xdg_cache_home to do
the same for paths relative to XDG_CACHE_HOME.

Signed-off-by: Devin Lehmacher <lehmacdj@gmail.com>
---
 cache.h |  7 +++++++
 path.c  | 15 +++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/cache.h b/cache.h
index c95826971..66761bc56 100644
--- a/cache.h
+++ b/cache.h
@@ -1169,6 +1169,13 @@ extern int is_ntfs_dotgit(const char *name);
  */
 extern char *xdg_config_home(const char *filename);
 
+/**
+ * Return a newly allocated string with the evaluation of
+ * "$XDG_CACHE_HOME/git/$filename" if $XDG_CACHE_HOME is non-empty, otherwise
+ * "$HOME/.cache/git/$filename". Return NULL upon error.
+ */
+extern char *xdg_cache_home(const char *filename);
+
 /* object replacement */
 #define LOOKUP_REPLACE_OBJECT 1
 #define LOOKUP_UNKNOWN_OBJECT 2
diff --git a/path.c b/path.c
index efcedafba..22248436b 100644
--- a/path.c
+++ b/path.c
@@ -1272,6 +1272,21 @@ char *xdg_config_home(const char *filename)
 	return NULL;
 }
 
+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;
+}
+
 GIT_PATH_FUNC(git_path_cherry_pick_head, "CHERRY_PICK_HEAD")
 GIT_PATH_FUNC(git_path_revert_head, "REVERT_HEAD")
 GIT_PATH_FUNC(git_path_squash_msg, "SQUASH_MSG")
-- 
2.11.0

[GSoC][PATCH v6 2/3] credential-cache: use XDG_CACHE_HOME for socket

[Devin Lehmacher]

Make git-credential-cache follow the XDG base path specification by
default. This increases consistency with other applications and helps
keep clutter out of users' home directories.

Check the old socket location, ~/.git-credential-cache/, and use
~/.git-credential-cache/socket if that directory exists rather than
forcing users who have used `git credential-cache` before to migrate to
the new XDG compliant location.
Otherwise use the socket $XDG_CACHE_HOME/git/credential/socket following
XDG base path specification. Use the subdirectory credential/ in case
other files are cached under $XDG_CACHE_HOME/git/ in the future and to
make the socket's purpose clear.

Signed-off-by: Devin Lehmacher <lehmacdj@gmail.com>
---
 Documentation/git-credential-cache.txt | 11 +++++++----
 credential-cache.c                     | 15 ++++++++++++++-
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-credential-cache.txt b/Documentation/git-credential-cache.txt
index 96208f822..2b8582639 100644
--- a/Documentation/git-credential-cache.txt
+++ b/Documentation/git-credential-cache.txt
@@ -33,10 +33,13 @@ OPTIONS
 --socket <path>::
 
 	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
-	network-mounted filesystem, you may need to change this to a
-	local filesystem. You must specify an absolute path.
+	cache daemon if one is not started).
+	Defaults to `$XDG_CACHE_HOME/git/credential/socket` unless
+	`~/.git-credential-cache/` exists in which case
+	`~/.git-credential-cache/socket` is used instead.
+	If your home directory is on a network-mounted filesystem, you
+	may need to change this to a local filesystem. You must specify
+	an absolute path.
 
 CONTROLLING THE DAEMON
 ----------------------
diff --git a/credential-cache.c b/credential-cache.c
index cc8a6ee19..3cbd42001 100644
--- a/credential-cache.c
+++ b/credential-cache.c
@@ -83,6 +83,19 @@ static void do_cache(const char *socket, const char *action, int timeout,
 	strbuf_release(&buf);
 }
 
+static char *get_socket_path(void)
+{
+	struct stat sb;
+	char *old_dir, *socket;
+	old_dir = expand_user_path("~/.git-credential-cache");
+	if (old_dir && !stat(old_dir, &sb) && S_ISDIR(sb.st_mode))
+		socket = xstrfmt("%s/socket", old_dir);
+	else
+		socket = xdg_cache_home("credential/socket");
+	free(old_dir);
+	return socket;
+}
+
 int cmd_main(int argc, const char **argv)
 {
 	char *socket_path = NULL;
@@ -106,7 +119,7 @@ int cmd_main(int argc, const char **argv)
 	op = argv[0];
 
 	if (!socket_path)
-		socket_path = expand_user_path("~/.git-credential-cache/socket");
+		socket_path = get_socket_path();
 	if (!socket_path)
 		die("unable to find a suitable socket path; use --socket");
 
-- 
2.11.0

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

[Jeff King]

On Fri, Mar 17, 2017 at 08:36:33AM -0400, Devin Lehmacher wrote:

> Make git-credential-cache follow the XDG base path specification by
> default. This increases consistency with other applications and helps
> keep clutter out of users' home directories.
> 
> Check the old socket location, ~/.git-credential-cache/, and use
> ~/.git-credential-cache/socket if that directory exists rather than
> forcing users who have used `git credential-cache` before to migrate to
> the new XDG compliant location.
> Otherwise use the socket $XDG_CACHE_HOME/git/credential/socket following
> XDG base path specification. Use the subdirectory credential/ in case
> other files are cached under $XDG_CACHE_HOME/git/ in the future and to
> make the socket's purpose clear.

Makes sense.

> +static char *get_socket_path(void)
> +{
> +	struct stat sb;
> +	char *old_dir, *socket;
> +	old_dir = expand_user_path("~/.git-credential-cache");
> +	if (old_dir && !stat(old_dir, &sb) && S_ISDIR(sb.st_mode))
> +		socket = xstrfmt("%s/socket", old_dir);
> +	else
> +		socket = xdg_cache_home("credential/socket");
> +	free(old_dir);
> +	return socket;
> +}

The implementation looks nice and clean.

-Peff

[GSoC][PATCH v6 3/3] credential-cache: add tests for XDG functionality

[Devin Lehmacher]

Signed-off-by: Devin Lehmacher <lehmacdj@gmail.com>
---
 t/t0301-credential-cache.sh | 93 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 93 insertions(+)

diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh
index 82c841121..fd92533ac 100755
--- a/t/t0301-credential-cache.sh
+++ b/t/t0301-credential-cache.sh
@@ -12,7 +12,100 @@ test -z "$NO_UNIX_SOCKETS" || {
 # don't leave a stale daemon running
 trap 'code=$?; git credential-cache exit; (exit $code); die' EXIT
 
+# test that the daemon works with no special setup
 helper_test cache
+
+test_expect_success 'socket defaults to ~/.cache/git/credential/socket' '
+	test_when_finished "
+		git credential-cache exit &&
+		rmdir -p .cache/git/credential/
+	" &&
+	test_path_is_missing "$HOME/.git-credential-cache" &&
+	test -S "$HOME/.cache/git/credential/socket"
+'
+
+XDG_CACHE_HOME="$HOME/xdg"
+export XDG_CACHE_HOME
+# test behavior when XDG_CACHE_HOME is set
+helper_test cache
+
+test_expect_success "use custom XDG_CACHE_HOME if set and default sockets are not created" '
+	test_when_finished "git credential-cache exit" &&
+	test -S "$XDG_CACHE_HOME/git/credential/socket" &&
+	test_path_is_missing "$HOME/.git-credential-cache/socket" &&
+	test_path_is_missing "$HOME/.cache/git/credential/socket"
+'
+unset XDG_CACHE_HOME
+
+test_expect_success 'credential-cache --socket option overrides default location' '
+	test_when_finished "
+		git credential-cache exit --socket \"\$HOME/dir/socket\" &&
+		rmdir \"\$HOME/dir\"
+	" &&
+	check approve "cache --socket \"\$HOME/dir/socket\"" <<-\EOF &&
+	protocol=https
+	host=example.com
+	username=store-user
+	password=store-pass
+	EOF
+	test -S "$HOME/dir/socket"
+'
+
+test_expect_success "use custom XDG_CACHE_HOME even if xdg socket exists" '
+	test_when_finished "
+		git credential-cache exit &&
+		sane_unset XDG_CACHE_HOME
+	" &&
+	check approve cache <<-\EOF &&
+	protocol=https
+	host=example.com
+	username=store-user
+	password=store-pass
+	EOF
+	test -S "$HOME/.cache/git/credential/socket" &&
+	XDG_CACHE_HOME="$HOME/xdg" &&
+	export XDG_CACHE_HOME &&
+	check approve cache <<-\EOF &&
+	protocol=https
+	host=example.com
+	username=store-user
+	password=store-pass
+	EOF
+	test -S "$XDG_CACHE_HOME/git/credential/socket"
+'
+
+test_expect_success 'use user socket if user directory exists' '
+	test_when_finished "
+		git credential-cache exit &&
+		rmdir \"\$HOME/.git-credential-cache/\"
+	" &&
+	mkdir -p -m 700 "$HOME/.git-credential-cache/" &&
+	check approve cache <<-\EOF &&
+	protocol=https
+	host=example.com
+	username=store-user
+	password=store-pass
+	EOF
+	test -S "$HOME/.git-credential-cache/socket"
+'
+
+test_expect_success SYMLINKS 'use user socket if user directory is a symlink to a directory' '
+	test_when_finished "
+		git credential-cache exit &&
+		rmdir \"\$HOME/dir/\" &&
+		rm \"\$HOME/.git-credential-cache\"
+	" &&
+	mkdir -p -m 700 "$HOME/dir/" &&
+	ln -s "$HOME/dir" "$HOME/.git-credential-cache" &&
+	check approve cache <<-\EOF &&
+	protocol=https
+	host=example.com
+	username=store-user
+	password=store-pass
+	EOF
+	test -S "$HOME/.git-credential-cache/socket"
+'
+
 helper_test_timeout cache --timeout=1
 
 # we can't rely on our "trap" above working after test_done,
-- 
2.11.0

Re: [GSoC][PATCH v6 3/3] credential-cache: add tests for XDG functionality

[Jeff King]

On Fri, Mar 17, 2017 at 08:36:34AM -0400, Devin Lehmacher wrote:

> +test_expect_success 'credential-cache --socket option overrides default location' '
> +	test_when_finished "
> +		git credential-cache exit --socket \"\$HOME/dir/socket\" &&
> +		rmdir \"\$HOME/dir\"
> +	" &&
> +	check approve "cache --socket \"\$HOME/dir/socket\"" <<-\EOF &&

I wondered how this one got through the quoting/eval mess of the
"check()" function. But it works because we pass a raw unexpanded $HOME
into the config variable, and the credential subsystem then runs "cache
--socket ..." using a shell, which is where it gets expanded.

> +	mkdir -p -m 700 "$HOME/.git-credential-cache/" &&

This is our first use of "mkdir -m". It's in POSIX, so it should
probably be fine. I'm not sure if platforms without POSIXPERM would
complain, but I think in practice anything with unix sockets will handle
it just fine.

The rest of the tests look OK to me.

-Peff

Re: [GSoC][PATCH v6 1/3] path.c: add xdg_cache_home

[Jeff King]

On Fri, Mar 17, 2017 at 08:36:32AM -0400, Devin Lehmacher wrote:

> We already have xdg_config_home to format paths relative to
> XDG_CONFIG_HOME. Let's provide a similar function xdg_cache_home to do
> the same for paths relative to XDG_CACHE_HOME.
> 
> Signed-off-by: Devin Lehmacher <lehmacdj@gmail.com>

Looks obviously correct. The whole series looks good to me.

-Peff

Re: [GSoC][PATCH v2 1/2] path.c: add xdg_cache_home

[Junio C Hamano]

Devin Lehmacher <lehmacdj@gmail.com> writes:

> We already have xdg_config_home to format paths relative to
> XDG_CONFIG_HOME. Let's provide a similar function xdg_cache_home to do
> the same for paths relative to XDG_CACHE_HOME.

Nicely explained.

> +/**
> + * Return a newly allocated string with the evaluation of
> + * "$XDG_CACHE_HOME/git/$filename" if $XDG_CONFIG_HOME is non-empty, otherwise
> + * "$HOME/.config/git/$filename". Return NULL upon error.

s|CONFIG|CACHE| and s|.config|.cache| are needed, methinks.

Will fix while queuing, so no need to resend only to fix this typo.

Re: [GSoC][PATCH v4 4/4] credential-cache: add tests for XDG functionality

[Devin Lehmacher]

> > > +test_expect_success 'credential-cache --socket option overrides default location' '
> > > + test_when_finished "rm -rf \"$HOME\"/dir/" &&
> > > + check approve "cache --socket \"$HOME/dir/socket\"" <<-\EOF &&
> > > + protocol=https
> > > + host=example.com
> > > + username=store-user
> > > + password=store-pass
> > > + EOF
> > > + test -S "$HOME/dir/socket" &&
> > > + git credential-cache exit
> > > +'
>
> This is almost right, except:
>
>   - the "exit" needs to be told which socket to use
>
>   - we should do the "exit" even when the test fails early (so in
>     test_when_finished)
>
>   - the test_when_finished block will interpolate $HOME when setting up
>     the block, which will break if it contains double-quotes or other
>     special characters. It should use \$HOME.
>
>     I suspect the "check" invocation needs to do so as well, though it
>     is even trickier (we shove it into a single-quoted "-c" argument).
>
>     I think you could get by in both cases with relative paths.

Using a relative path won't work since `git credential-cache --socket`
only accepts absolute paths. It shouldn't be too hard to escape the
string though.

-Devin