threads / patch / 45367
Subject: [GSoC][PATCH 0/3] Move ~/.git-credential-cache to ~/.cache/git
Devin LehmacherPatches 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(-)
Devin Lehmacher[GSoC][PATCH 1/3] path.c: Add xdg_cache_home to get paths under XDG_CACHE_HOME
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(+)
cache.h, path.c
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
Jeff KingRe: [GSoC][PATCH 1/3] path.c: Add xdg_cache_home to get paths under XDG_CACHE_HOME
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
Devin LehmacherRe: [GSoC][PATCH 1/3] path.c: Add xdg_cache_home to get paths under XDG_CACHE_HOME
> 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
Devin Lehmacher[GSoC][PATCH 3/3] Update documentation to reflect new socket location
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
Junio C HamanoRe: [GSoC][PATCH 3/3] Update documentation to reflect new socket location
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.
Devin Lehmacher[GSoC][PATCH v2 2/2] credential-cache: use XDG_CACHE_HOME for socket
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(-)
Documentation/git-credential-cache.txt, credential-cache.c
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
Devin Lehmacher[GSoC][PATCH v2 1/2] path.c: add xdg_cache_home
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(+)
cache.h, path.c
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
Devin Lehmacher[GSoC][PATCH v2 2/2] credential-cache: use XDG_CACHE_HOME for socket
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(-)
Documentation/git-credential-cache.txt, credential-cache.c
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
Junio C HamanoRe: [GSoC][PATCH v2 2/2] credential-cache: use XDG_CACHE_HOME for socket
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.
Devin Lehmacher[GSoC][PATCH/RFC v3 0/3] Fix commit messages, check if socket is socket
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
Devin Lehmacher[GSoC][PATCH/RFC v3 3/3] credential-cache: only use user_socket if a socket
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
Devin LehmacherRe: [GSoC][PATCH/RFC v3 3/3] credential-cache: only use user_socket if a socket
> +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
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
Devin LehmacherRe: [GSoC][PATCH/RFC v3 3/3] credential-cache: only use user_socket if a socket
> 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
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
Junio C HamanoRe: [GSoC][PATCH/RFC v3 3/3] credential-cache: only use user_socket if a socket
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".
Junio C HamanoRe: [GSoC][PATCH/RFC v3 3/3] credential-cache: only use user_socket if a socket
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 usewould 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
Devin LehmacherRe: [GSoC][PATCH/RFC v3 3/3] credential-cache: only use user_socket if a socket
> 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
Devin Lehmacher[GSoC][PATCH v4 0/4] Moving credential-cache socket to xdg path
I implemented all changes from the previous reviews and also wrote a few tests.
-Devin
Devin Lehmacher[GSoC][PATCH v4 1/4] path.c: add xdg_cache_home
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(+)
cache.h, path.c
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
Devin Lehmacher[GSoC][PATCH v4 4/4] credential-cache: add tests for XDG functionality
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
Junio C HamanoRe: [GSoC][PATCH v4 4/4] credential-cache: add tests for XDG functionality
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.
Jeff KingRe: [GSoC][PATCH v4 4/4] credential-cache: add tests for XDG functionality
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
Devin Lehmacher[GSoC][PATCH v5 1/3] path.c: add xdg_cache_home
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(+)
cache.h, path.c
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
Devin Lehmacher[GSoC][PATCH v5 2/3] credential-cache: use XDG_CACHE_HOME for socket
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(-)
Documentation/git-credential-cache.txt, credential-cache.c
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
Ramsay JonesRe: [GSoC][PATCH v5 2/3] credential-cache: use XDG_CACHE_HOME for socket
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
Devin LehmacherRe: [GSoC][PATCH v5 2/3] credential-cache: use XDG_CACHE_HOME for socket
> 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
Devin Lehmacher[GSoC][PATCH v5 3/3] credential-cache: add tests for XDG functionality
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
Junio C HamanoRe: [GSoC][PATCH v5 3/3] credential-cache: add tests for XDG functionality
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.
Devin Lehmacher[GSoC][PATCH v6 1/3] path.c: add xdg_cache_home
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(+)
cache.h, path.c
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
Devin Lehmacher[GSoC][PATCH v6 2/3] credential-cache: use XDG_CACHE_HOME for socket
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(-)
Documentation/git-credential-cache.txt, credential-cache.c
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
Jeff KingRe: [GSoC][PATCH v6 2/3] credential-cache: use XDG_CACHE_HOME for socket
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
Devin Lehmacher[GSoC][PATCH v6 3/3] credential-cache: add tests for XDG functionality
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
Jeff KingRe: [GSoC][PATCH v6 3/3] credential-cache: add tests for XDG functionality
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
Jeff KingRe: [GSoC][PATCH v6 1/3] path.c: add xdg_cache_home
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
Devin Lehmacher[GSoC][PATCH v4 2/4] dir: add directory_exists
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(+)
dir.c, dir.h
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
Junio C HamanoRe: [GSoC][PATCH v4 2/4] dir: add directory_exists
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().
Junio C HamanoRe: [GSoC][PATCH v4 2/4] dir: add directory_exists
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).
Devin Lehmacher[GSoC][PATCH v4 3/4] credential-cache: use XDG_CACHE_HOME for socket
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(-)
Documentation/git-credential-cache.txt, credential-cache.c
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
Junio C HamanoRe: [GSoC][PATCH v4 3/4] credential-cache: use XDG_CACHE_HOME for socket
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.
Jeff KingRe: [GSoC][PATCH v4 3/4] credential-cache: use XDG_CACHE_HOME for socket
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
Devin Lehmacher[GSoC][PATCH/RFC v3 2/3] credential-cache: use XDG_CACHE_HOME for socket
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(-)
Documentation/git-credential-cache.txt, credential-cache.c
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
Junio C HamanoRe: [GSoC][PATCH/RFC v3 2/3] credential-cache: use XDG_CACHE_HOME for socket
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)
{Devin Lehmacher[GSoC][PATCH/RFC v3 1/3] path.c: add xdg_cache_home
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(+)
cache.h, path.c
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
Junio C HamanoRe: [GSoC][PATCH v2 1/2] path.c: add xdg_cache_home
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.
Devin Lehmacher[GSoC][PATCH 2/3] credential-cache.c: Make git use XDG_CACHE_HOME for credentials
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
Junio C HamanoRe: [GSoC][PATCH 2/3] credential-cache.c: Make git use XDG_CACHE_HOME for credentials
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");Jeff KingRe: [GSoC][PATCH 2/3] credential-cache.c: Make git use XDG_CACHE_HOME for credentials
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
Junio C HamanoRe: [GSoC][PATCH 2/3] credential-cache.c: Make git use XDG_CACHE_HOME for credentials
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" ;-).
Devin LehmacherRe: [GSoC][PATCH 2/3] credential-cache.c: Make git use XDG_CACHE_HOME for credentials
> 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
Devin LehmacherRe: [GSoC][PATCH 2/3] credential-cache.c: Make git use XDG_CACHE_HOME for credentials
> 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
Junio C HamanoRe: [GSoC][PATCH 2/3] credential-cache.c: Make git use XDG_CACHE_HOME for credentials
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)
{Jeff KingRe: [GSoC][PATCH 2/3] credential-cache.c: Make git use XDG_CACHE_HOME for credentials
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