{"thread":{"id":"41683","subject":"[GSOC] Microproject \"Move ~/.git-credential-cache to ~/.config/git\"","startedAt":"2016-03-14T14:47:53Z","lastAt":"2016-03-18T06:12:01Z","messageCount":30,"participants":["惠轶群","Junio C Hamano","Your friend","Jeff King","Ivan Tham","Hui Yiqun","谭俊浩"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"280710","messageId":"CAKqreux4aYhXTE9kUHKoKCJ2-4KDWyi58ioCm-CWqXhUYCtEEw@mail.gmail.com","threadId":"41683","inReplyTo":null,"subject":"[GSOC] Microproject \"Move ~/.git-credential-cache to ~/.config/git\"","fromName":"惠轶群","fromEmail":"huiyiqun@gmail.com","sentAt":"2016-03-14T14:47:53Z","receivedAt":"2016-03-14T14:47:53Z","isPatch":false,"sender":{"key":"huiyiqun@gmail.com","avatar":"https://gravatar.com/avatar/44aa01fbc2fb098682386b82a44aac03bd778275249228c55d09c5b6fe2fe0e0?d=mp&s=160"},"body":"I'm Hui Yiqun, a master degress candidate of Tsinghua University. I'd\nlike to participate GSOC 2016 as an developer of git.\n\nI have basic knowledge about several programming languages (namely C,\npython, go and etc.), compilers and cmake which may help my\ndevelopment.\n\nMy github page is at [here](https://github.com/huiyiqun/). There are\nsome tiny programs.\n\nI took part in a CDN project last year and learnt much about TCP/IP,\nweb service and, most importantly, cooperation. However, I really want\nto learn how members of an opensource project work together.\n\nI have covered most of the available materials, such as list of ideas\nand micro-projects, `README.md`, `INSTALL` and\n`Documentation/CodingGuidelines`. So I decide to start my contribution\nwith the microproject \"Move ~/.git-credential-cache to ~/.config/git\"\nfound [here](http://git.github.io/SoC-2016-Microprojects/), which\nseems easy for me.\n\nI greped the source code and found out that there are two places where\n\"git-credential-cache\" are hard-coded:\n\n1. credential-cache.c\n2. contrib/persistent-https/socket.go\n\nAt first sight, there are following tasks to do:\n\n1. implement a function `xdg_cache_home` similar to `xdg_config_home`\nin `path.c`.\n2. replace the hard-coded path with an call to `xdg_cache_home`.\n\nI'm still confused about following:\n\n1. should `~/.git-credential-cache` been moved to\n`~/.cache/git/credential`(as the descreption of the micropject says)\nor `~/.config/git/credential`(as the title of the microproject says)?\n2. If `~/.cache/git/credential` is the desired target, there seems\nnothing to do with `XDG_CONFIG_HOME`.\n3. Does \"without breaking compatibility with the old behavior.\" mean\nthat I should still try to connect to the unix socket placed at the\nold place? If yes, which order is prefered?\n\nThanks for your patience. I hope that my English didn't affect the\ncommunication.\n"},{"id":"280713","messageId":"xmqqshztawwd.fsf@gitster.mtv.corp.google.com","threadId":"41683","inReplyTo":"CAKqreux4aYhXTE9kUHKoKCJ2-4KDWyi58ioCm-CWqXhUYCtEEw@mail.gmail.com","subject":"Re: [GSOC] Microproject \"Move ~/.git-credential-cache to ~/.config/git\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-14T15:42:42Z","receivedAt":"2016-03-14T15:42:42Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"惠轶群 <huiyiqun@gmail.com> writes:\n\n> I'm still confused about following:\n>\n> 1. should `~/.git-credential-cache` been moved to\n> `~/.cache/git/credential`(as the descreption of the micropject says)\n> or `~/.config/git/credential`(as the title of the microproject says)?\n\nThe latter, I'd think, as you noticed in 2. the former does not make\nmuch sense.\n\n> 2. If `~/.cache/git/credential` is the desired target, there seems\n> nothing to do with `XDG_CONFIG_HOME`.\n\nYes, I think the intent of the problem specification is \"if it is\nfound in the XDG_CONFIG_HOME use $XDG_CONFIG_HOME/.config/git/* a\nthe location to place it; by the way if the user does not have\nanything to explicitly say she wants to use a specific location for\nXDG_CONFIG_HOME, it defaults to \"~/.config, hence the Git specific\nlocation becomes ~/.config/git/*\".\n\n> 3. Does \"without breaking compatibility with the old behavior.\" mean\n> that I should still try to connect to the unix socket placed at the\n> old place? If yes, which order is prefered?\n\nLet me be vague and oblique on purpose ;-)\n\nWe used to have only ~/.gitconfig as the per-user location, and at\nsome point in the history we added XDG_CONFIG_HOME support.  Study\nhow that was done (hint: \"git log\" and \"git blame\" may be useful\ntools for this purpose), and imitate it.\n"},{"id":"280732","messageId":"CAKqreuwgxzVTAUdZPf9+ivXCdW8F2Ksafw7V0pbLAaRPwGX9Uw@mail.gmail.com","threadId":"41683","inReplyTo":"xmqqshztawwd.fsf@gitster.mtv.corp.google.com","subject":"Re: [GSOC] Microproject \"Move ~/.git-credential-cache to ~/.config/git\"","fromName":"惠轶群","fromEmail":"huiyiqun@gmail.com","sentAt":"2016-03-14T19:53:52Z","receivedAt":"2016-03-14T19:53:52Z","isPatch":false,"sender":{"key":"huiyiqun@gmail.com","avatar":"https://gravatar.com/avatar/44aa01fbc2fb098682386b82a44aac03bd778275249228c55d09c5b6fe2fe0e0?d=mp&s=160"},"body":"2016-03-14 23:42 GMT+08:00 Junio C Hamano <gitster@pobox.com>:\n> 惠轶群 <huiyiqun@gmail.com> writes:\n>\n>> I'm still confused about following:\n>>\n>> 1. should `~/.git-credential-cache` been moved to\n>> `~/.cache/git/credential`(as the descreption of the micropject says)\n>> or `~/.config/git/credential`(as the title of the microproject says)?\n>\n> The latter, I'd think, as you noticed in 2. the former does not make\n> much sense.\n\nI'm not sure. There is only a unix socket under ~/.git-credential-cache.\nIt's not a config.\n\nAfter reading the\n[spec](https://specifications.freedesktop.org/basedir-spec/basedir-spec-0.7.html)\nmore carefully, I think that $XDG_RUNTIME_DIR may be a better choice than\nthe above two. For ~/.git-credential-cache/socket is a unix socket and should\nbe used only runtime.\n\nOn my computer, this variable is well defined and pointed to /run/user/$UID.\n\nThe problem is that there is no suitable default to it if $XDG_RUNTIME_DIR is\nnot set. I guess /tmp/git-$USER is a good choice.\n\n>> 3. Does \"without breaking compatibility with the old behavior.\" mean\n>> that I should still try to connect to the unix socket placed at the\n>> old place? If yes, which order is prefered?\n>\n> Let me be vague and oblique on purpose ;-)\n>\n> We used to have only ~/.gitconfig as the per-user location, and at\n> some point in the history we added XDG_CONFIG_HOME support.  Study\n> how that was done (hint: \"git log\" and \"git blame\" may be useful\n> tools for this purpose), and imitate it.\n>\n\nI have read almost every commit which contains \"XDG\", as well as their diffs.\n\nI think this issue I'm currently dealing with is a little different\nfrom formers,\nFor this file is just an unix socket and never persistent. I could not just\nkeep in step with old behavior unless that user manually create the file\nat new-style path.\n\nOf course I could check the existence of `~/.git-credential-cache` and\n`$XDG_CONFIG_HOME/git/credential-cache` to decide where to put\nthe socket, but it's not elegant enough. Because, as mentioned above,\nthe socket file itself is not persistent.\n"},{"id":"280737","messageId":"xmqqwpp494vd.fsf@gitster.mtv.corp.google.com","threadId":"41683","inReplyTo":"CAKqreuwgxzVTAUdZPf9+ivXCdW8F2Ksafw7V0pbLAaRPwGX9Uw@mail.gmail.com","subject":"Re: [GSOC] Microproject \"Move ~/.git-credential-cache to ~/.config/git\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-14T20:33:26Z","receivedAt":"2016-03-14T20:33:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"惠轶群 <huiyiqun@gmail.com> writes:\n\n> After reading the\n> [spec](https://specifications.freedesktop.org/basedir-spec/basedir-spec-0.7.html)\n> more carefully, I think that $XDG_RUNTIME_DIR may be a better choice than\n> the above two. For ~/.git-credential-cache/socket is a unix socket and should\n> be used only runtime.\n\nNice.\n\n> Of course I could check the existence of `~/.git-credential-cache` and\n> `$XDG_CONFIG_HOME/git/credential-cache` to decide where to put\n> the socket,\n\nI think that is the most sensible.  They are directories that are\nexpected to be persistent, aren't they?\n\nYou sound like you think it is better to check the location of the\nexisting socket, but I am having a hard time understanding and/or\nguessing why you might think so, especially when you already know\nthat the socket itself may not be persistent.  Puzzled...\n"},{"id":"280754","messageId":"CAKqreuw0hafi-GwWD-UaGKwmG8xVK6ZJDcw2jpQXAAuK+XBZvQ@mail.gmail.com","threadId":"41683","inReplyTo":"xmqqwpp494vd.fsf@gitster.mtv.corp.google.com","subject":"Re: [GSOC] Microproject \"Move ~/.git-credential-cache to ~/.config/git\"","fromName":"惠轶群","fromEmail":"huiyiqun@gmail.com","sentAt":"2016-03-15T01:32:21Z","receivedAt":"2016-03-15T01:32:21Z","isPatch":false,"sender":{"key":"huiyiqun@gmail.com","avatar":"https://gravatar.com/avatar/44aa01fbc2fb098682386b82a44aac03bd778275249228c55d09c5b6fe2fe0e0?d=mp&s=160"},"body":"> You sound like you think it is better to check the location of the\n> existing socket,\n\nYes, for the purpose of compatibility, it's the only choice, as I can see.\n\nTo sum up, I'd like to implement:\n\n1. <path> is configured by --socket, then put it here.\n2. else if `~/.git-credential-cache` exists, put the socket under here.\n3. else, put the socket under\n`$XDG_RUNTIME_DIR/git/credential-cache.sock`,\nif $XDG_RUNTIME_DIR does not exist, default to\n`/tmp/git-$UID/credential-cache.sock`.\n\nAs a result, new user will use xdg-compatible path while old user will not\nbe surprised.\n"},{"id":"280757","messageId":"c74296a0db1cac9daf5da00756ab0313@riseup.net","threadId":"41683","inReplyTo":"CAKqreuw0hafi-GwWD-UaGKwmG8xVK6ZJDcw2jpQXAAuK+XBZvQ@mail.gmail.com","subject":"Re: [GSOC] Microproject \"Move ~/.git-credential-cache to ~/.config/git\"","fromName":"Your friend","fromEmail":"pickfire@riseup.net","sentAt":"2016-03-15T02:14:15Z","receivedAt":"2016-03-15T02:14:15Z","isPatch":false,"sender":{"key":"pickfire@riseup.net","avatar":null},"body":"On 15/03/2016 09:32, 惠轶群 wrote:\n>> You sound like you think it is better to check the location of the\n>> existing socket,\n> \n> Yes, for the purpose of compatibility, it's the only choice, as I can \n> see.\n> \n> To sum up, I'd like to implement:\n> \n> 1. <path> is configured by --socket, then put it here.\n> 2. else if `~/.git-credential-cache` exists, put the socket under here.\n> 3. else, put the socket under\n> `$XDG_RUNTIME_DIR/git/credential-cache.sock`,\n> if $XDG_RUNTIME_DIR does not exist, default to\n> `/tmp/git-$UID/credential-cache.sock`.\n> \n> As a result, new user will use xdg-compatible path while old user will \n> not\n> be surprised.\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n\nHi, I am Ivan Tham 谭俊浩, I know a bit of C and more in python, I am\ncurrently taking foundation in a Asia Pacific University.\n\nI would really like to work on this microproject for gsoc too but too \nbad\n惠轶群 had taken it. :(\n\nI think it should be in ~/$XDG_CACHE_HOME/git/credential or\n~/.cache/git/credential instead of ~/.config/git/credential.\n"},{"id":"280758","messageId":"20160315031259.GA20508@sigill.intra.peff.net","threadId":"41683","inReplyTo":"CAKqreuw0hafi-GwWD-UaGKwmG8xVK6ZJDcw2jpQXAAuK+XBZvQ@mail.gmail.com","subject":"Re: [GSOC] Microproject \"Move ~/.git-credential-cache to ~/.config/git\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-15T03:13:00Z","receivedAt":"2016-03-15T03:13:00Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 15, 2016 at 09:32:21AM +0800, 惠轶群 wrote:\n\n> > You sound like you think it is better to check the location of the\n> > existing socket,\n> \n> Yes, for the purpose of compatibility, it's the only choice, as I can see.\n\nI'm not sure the existing socket location matters, in the way that it\ndoes for config. The socket is inherently ephemeral, and designed to go\naway after a few minutes (and the program designed to run sanely when it\ndoes not exist).\n\nSo yes, when you switch from older git to newer git, you might\ntechnically have a cache-daemon running that you _could_ contact, but\ndon't find it. But I don't think it's a big deal in practice, and not\nworth designing around.\n\nSo I think it would be OK to just switch the default path to wherever\nXDG recommends ephemeral stuff to go (which sounds like\n$XDG_RUNTIME_DIR) and be done.\n\n-Peff\n"},{"id":"280761","messageId":"bd599933c7609c72bf405371b43bf080@riseup.net","threadId":"41683","inReplyTo":"CAKqreux-m3yHVsEQXdf+8vMNZwC0UCMBWnzbaqYJbdEEM14qiQ@mail.gmail.com","subject":"Re: [GSOC] Microproject \"Move ~/.git-credential-cache to ~/.config/git\"","fromName":"Ivan Tham","fromEmail":"pickfire@riseup.net","sentAt":"2016-03-15T05:56:44Z","receivedAt":"2016-03-15T05:56:44Z","isPatch":false,"sender":{"key":"pickfire@riseup.net","avatar":null},"body":"On 15/03/2016 10:59, 惠轶群 wrote:\n> Well, In my opinion:\n> \n> 1. $XDG_CACHE_HOME is more meaningful than $XDG_CONFIG_HOME,\n> while $XDG_RUNTIME_DIR is more meaningful than $XDG_CACHE_HOME.\n> However, I feel that I should value suggestions of who have more \n> experience\n> about this project than me.\n> \n> 2. $XDG_CACHE_HOME is absolute, so it's $XDG_CACHE_HOME/git/credential\n> instead of ~/$XDG_CACHE_HOME/git/credential.\n\nOh, I guess now $XDG_RUNTIME_DIR is really most suitable for this after\nreading more about the XDG Standard.\n"},{"id":"280823","messageId":"20160315192154.GA30693@sigill.intra.peff.net","threadId":"41683","inReplyTo":"CAKqreuwv+RRziS-NcaLYZYUN0_KrfgZSe6wp0wGBza4q3_x8RA@mail.gmail.com","subject":"Re: [GSOC] Microproject \"Move ~/.git-credential-cache to ~/.config/git\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-15T19:21:54Z","receivedAt":"2016-03-15T19:21:54Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 15, 2016 at 05:48:18AM +0000, 惠轶群 wrote:\n\n> On Tue, Mar 15, 2016, 11:13 AM Jeff King <peff@peff.net> wrote:\n> > The socket is inherently ephemeral, and designed to go\n> > away after a few minutes (and the program designed to run sanely when it\n> > does not exist).\n> \n> I agree.\n> \n> > So yes, when you switch from older git to newer git, you might\n> > technically have a cache-daemon running that you _could_ contact, but\n> > don't find it. But I don't think it's a big deal in practice, and not\n> > worth designing around\n> \n> Yes, it's OK with git itself. What I worry about is that this change break\n> some third-party tools. Does it matter?\n\nI don't think so. I suppose one could have a script that tests for the\nexistence of the socket or something. But then, I don't think \"use the\nold directory if it exists\" really solves that. Such a script would work\non people who had already run the old version of credential-cache, and\nbreak on new ones.\n\n-Peff\n"},{"id":"280879","messageId":"1458122865-29447-1-git-send-email-huiyiqun@gmail.com","threadId":"41683","inReplyTo":"CAKqreux4aYhXTE9kUHKoKCJ2-4KDWyi58ioCm-CWqXhUYCtEEw@mail.gmail.com","subject":"[PATCH/RFC/GSoC 1/3] path.c: implement xdg_runtime_dir()","fromName":"Hui Yiqun","fromEmail":"huiyiqun@gmail.com","sentAt":"2016-03-16T10:07:43Z","receivedAt":"2016-03-16T10:07:43Z","isPatch":true,"sender":{"key":"huiyiqun@gmail.com","avatar":"https://gravatar.com/avatar/44aa01fbc2fb098682386b82a44aac03bd778275249228c55d09c5b6fe2fe0e0?d=mp&s=160"},"body":"this function does the following:\n\n1. if $XDG_RUNTIME_DIR is non-empty, `$XDG_RUNTIME_DIR/git` is used in next\nstep, otherwise `/tmp/git-$uid` is taken.\n2. ensure that above directory does exist. what's more, it must has correct\npermission and ownership.\n3. a newly allocated string consisting of the path of above directory and\n$filename is returned.\n\nUnder following situation, NULL will be returned:\n+ the directory mentioned in step 1 exists but have wrong permission or\nownership.\n+ the directory or its parent cannot be created.\n\nNotice:\n\n+ the caller is responsible for deallocating the returned string.\n\nSigned-off-by: Hui Yiqun <huiyiqun@gmail.com>\n---\n cache.h | 23 +++++++++++++++++++++++\n path.c  | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 82 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex b829410..e640a54 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -999,6 +999,29 @@ extern int is_ntfs_dotgit(const char *name);\n  */\n extern char *xdg_config_home(const char *filename);\n \n+/**\n+ * this function does the following:\n+ *\n+ * 1. if $XDG_RUNTIME_DIR is non-empty, `$XDG_RUNTIME_DIR/git` is used in next\n+ * step, otherwise `/tmp/git-$uid` is taken.\n+ * 2. ensure that above directory does exist. what's more, it must has correct\n+ * permission and ownership.\n+ * 3. a newly allocated string consisting of the path of above directory and\n+ * $filename is returned.\n+ *\n+ * Under following situation, NULL will be returned:\n+ *\n+ * + the directory mentioned in step 1 exists but have wrong permission or\n+ * ownership.\n+ * + the directory or its parent cannot be created.\n+ *\n+ * Notice:\n+ *\n+ * + the caller is responsible for deallocating the returned string.\n+ *\n+ */\n+extern char *xdg_runtime_dir(const char *filename);\n+\n /* object replacement */\n #define LOOKUP_REPLACE_OBJECT 1\n #define LOOKUP_UNKNOWN_OBJECT 2\ndiff --git a/path.c b/path.c\nindex 8b7e168..2deecb3 100644\n--- a/path.c\n+++ b/path.c\n@@ -5,6 +5,7 @@\n #include \"strbuf.h\"\n #include \"string-list.h\"\n #include \"dir.h\"\n+#include \"git-compat-util.h\"\n \n static int get_st_mode_bits(const char *path, int *mode)\n {\n@@ -1193,6 +1194,64 @@ char *xdg_config_home(const char *filename)\n \treturn NULL;\n }\n \n+char *xdg_runtime_dir(const char *filename)\n+{\n+\tchar *runtime_dir, *git_runtime_dir;\n+\tstruct stat st;\n+\tuid_t uid = getuid();\n+\n+\tassert(filename);\n+\truntime_dir = getenv(\"XDG_RUNTIME_DIR\");\n+\tif (runtime_dir && *runtime_dir)\n+\t\tgit_runtime_dir = mkpathdup(\"%s/git/\", runtime_dir);\n+\telse\n+\t\tgit_runtime_dir = mkpathdup(\"/tmp/git-%d\", uid);\n+\n+\tif (!lstat(git_runtime_dir, &st)) {\n+\t\t/*\n+\t\t * As described in XDG base dir spec[1], the subdirectory\n+\t\t * under $XDG_RUNTIME_DIR or its fallback MUST be owned by\n+\t\t * the user, and its unix access mode MUST be 0700.\n+\t\t *\n+\t\t * Calling chmod or chown silently may cause security\n+\t\t * problem if somebody chdir to it, sleep, and then, try\n+\t\t * to open our protected runtime cache or socket.\n+\t\t * So we just put warning and left it to user to solve.\n+\t\t *\n+\t\t * [1]https://specifications.freedesktop.org/basedir-spec/\n+\t\t * basedir-spec-latest.html\n+\t\t */\n+\t\tif ((st.st_mode & 0777) != S_IRWXU) {\n+\t\t\tfprintf(stderr,\n+\t\t\t\t\t\"permission of runtime directory '%s' \"\n+\t\t\t\t\t\"MUST be 0700 instead of 0%o\\n\",\n+\t\t\t\t\tgit_runtime_dir, (st.st_mode & 0777));\n+\t\t\treturn NULL;\n+\t\t} else if (st.st_uid != uid) {\n+\t\t\tfprintf(stderr,\n+\t\t\t\t\t\"owner of runtime directory '%s' \"\n+\t\t\t\t\t\"MUST be %d instead of %d\\n\",\n+\t\t\t\t\tgit_runtime_dir, uid, st.st_uid);\n+\t\t\treturn NULL;\n+\t\t}\n+\t\t/* TODO: check whether git_runtime_dir is an directory */\n+\t} else {\n+\t\tif (safe_create_leading_directories_const(git_runtime_dir) < 0) {\n+\t\t\tfprintf(stderr,\n+\t\t\t\t\t\"unable to create directories for '%s'\\n\",\n+\t\t\t\t\tgit_runtime_dir);\n+\t\t\treturn NULL;\n+\t\t}\n+\t\tif (mkdir(git_runtime_dir, 0700) < 0) {\n+\t\t\tfprintf(stderr,\n+\t\t\t\t\t\"unable to mkdir '%s'\\n\", git_runtime_dir);\n+\t\t\treturn NULL;\n+\t\t}\n+\t}\n+\tfree(git_runtime_dir);\n+\treturn mkpathdup(\"%s/%s\", git_runtime_dir, filename);\n+}\n+\n GIT_PATH_FUNC(git_path_cherry_pick_head, \"CHERRY_PICK_HEAD\")\n GIT_PATH_FUNC(git_path_revert_head, \"REVERT_HEAD\")\n GIT_PATH_FUNC(git_path_squash_msg, \"SQUASH_MSG\")\n-- \n2.7.2\n"},{"id":"280880","messageId":"1458122865-29447-2-git-send-email-huiyiqun@gmail.com","threadId":"41683","inReplyTo":"1458122865-29447-1-git-send-email-huiyiqun@gmail.com","subject":"[PATCH/RFC/GSoC 2/3] git-credential-cache: put socket to xdg-compatible path","fromName":"Hui Yiqun","fromEmail":"huiyiqun@gmail.com","sentAt":"2016-03-16T10:07:44Z","receivedAt":"2016-03-16T10:07:44Z","isPatch":true,"sender":{"key":"huiyiqun@gmail.com","avatar":"https://gravatar.com/avatar/44aa01fbc2fb098682386b82a44aac03bd778275249228c55d09c5b6fe2fe0e0?d=mp&s=160"},"body":"move .git-credential-cache/socket to xdg_runtime_dir(\"credential-cache.sock\")\n\nSigned-off-by: Hui Yiqun <huiyiqun@gmail.com>\n---\n credential-cache.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/credential-cache.c b/credential-cache.c\nindex f4afdc6..40d838b 100644\n--- a/credential-cache.c\n+++ b/credential-cache.c\n@@ -105,7 +105,7 @@ int main(int argc, const char **argv)\n \top = argv[0];\n \n \tif (!socket_path)\n-\t\tsocket_path = expand_user_path(\"~/.git-credential-cache/socket\");\n+\t\tsocket_path = xdg_runtime_dir(\"credential-cache.sock\");\n \tif (!socket_path)\n \t\tdie(\"unable to find a suitable socket path; use --socket\");\n \n-- \n2.7.2\n"},{"id":"280881","messageId":"1458122865-29447-3-git-send-email-huiyiqun@gmail.com","threadId":"41683","inReplyTo":"1458122865-29447-1-git-send-email-huiyiqun@gmail.com","subject":"[PATCH/RFC/GSoC 3/3] t0301: test credential-cache support of XDG_RUNTIME_DIR","fromName":"Hui Yiqun","fromEmail":"huiyiqun@gmail.com","sentAt":"2016-03-16T10:07:45Z","receivedAt":"2016-03-16T10:07:45Z","isPatch":true,"sender":{"key":"huiyiqun@gmail.com","avatar":"https://gravatar.com/avatar/44aa01fbc2fb098682386b82a44aac03bd778275249228c55d09c5b6fe2fe0e0?d=mp&s=160"},"body":"t0301 now tests git-credential-cache support for XDG user-specific\nruntime file $XDG_RUNTIME_DIR/git/credential.sock. Specifically:\n\n* if $XDG_RUNTIME_DIR exists, use socket at\n  `$XDG_RUNTIME_DIR/git/credential-cache.sock`.\n\n* otherwise, `/tmp/git-$uid/credential-cache.sock` is taken.\n\nSigned-off-by: Hui Yiqun <huiyiqun@gmail.com>\n---\n t/t0301-credential-cache.sh | 25 +++++++++++++++++++++++++\n 1 file changed, 25 insertions(+)\n\ndiff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh\nindex 82c8411..0718bb0 100755\n--- a/t/t0301-credential-cache.sh\n+++ b/t/t0301-credential-cache.sh\n@@ -12,7 +12,32 @@ test -z \"$NO_UNIX_SOCKETS\" || {\n # don't leave a stale daemon running\n trap 'code=$?; git credential-cache exit; (exit $code); die' EXIT\n \n+test_expect_success 'set $XDG_RUNTIME_DIR' '\n+\tXDG_RUNTIME_DIR=$HOME/xdg_runtime/\n+'\n+\n+helper_test cache\n+\n+test_expect_success 'when $XDG_RUNTIME_DIR is set, `$XDG_RUNTIME_DIR/git` are used' '\n+\ttest_path_is_missing \"/tmp/git-$(id -u)/git/credential-cache.sock\" &&\n+\ttest -S \"$XDG_RUNTIME_DIR/git/credential-cache.sock\"\n+'\n+\n+test_expect_success 'force git-credential-cache to exit so that socket disappear' '\n+\tgit credential-cache exit &&\n+\ttest_path_is_missing \"$XDG_RUNTIME_DIR/git/credential-cache.sock\" &&\n+\tunset XDG_RUNTIME_DIR\n+'\n+\n helper_test cache\n+\n+test_expect_success 'when $XDG_RUNTIME_DIR is not set, `/tmp/git-$(id -u) is used' '\n+\ttest -S \"/tmp/git-$(id -u)/credential-cache.sock\"\n+'\n+\n+# TODO: if $XDG_RUNTIME_DIR/git/ exists, but has wrong permission and ownership,\n+# `helper_test cache` must fail.\n+\n helper_test_timeout cache --timeout=1\n \n # we can't rely on our \"trap\" above working after test_done,\n-- \n2.7.2\n"},{"id":"280882","messageId":"CAKqreuxn7upMwOQH54rdA9nv1wCLNSSPjrWH3xejHP-ib+Ynuw@mail.gmail.com","threadId":"41683","inReplyTo":"20160315192154.GA30693@sigill.intra.peff.net","subject":"Re: [GSOC] Microproject \"Move ~/.git-credential-cache to ~/.config/git\"","fromName":"惠轶群","fromEmail":"huiyiqun@gmail.com","sentAt":"2016-03-16T10:45:42Z","receivedAt":"2016-03-16T10:45:42Z","isPatch":false,"sender":{"key":"huiyiqun@gmail.com","avatar":"https://gravatar.com/avatar/44aa01fbc2fb098682386b82a44aac03bd778275249228c55d09c5b6fe2fe0e0?d=mp&s=160"},"body":"2016-03-16 3:21 GMT+08:00 Jeff King <peff@peff.net>:\n> I don't think so. I suppose one could have a script that tests for the\n> existence of the socket or something.\n\nI agree, that is what I meant by \"third-party tools\".\n\nBut for user who write script like that, it's easy to figure out the problem\nand either update his/her scripts or explicitly declare the path of the socket\nin his gitconfig.\n\nAnyway, most users will not be affected.\n\n> But then, I don't think \"use the\n> old directory if it exists\" really solves that. Such a script would work\n> on people who had already run the old version of credential-cache, and\n> break on new ones.\n\nIf back-compatibility must be ensured, the only graceful solution I can\nimage is a new option entry like `xdg-style-path` being added to gitconfig\nand default it to false.\n\nI doubt whether the increasing complexity is worthy just for the\nback-compatibilty\nof a socket path.\n"},{"id":"280912","messageId":"xmqqr3fa5rdi.fsf@gitster.mtv.corp.google.com","threadId":"41683","inReplyTo":"1458122865-29447-3-git-send-email-huiyiqun@gmail.com","subject":"Re: [PATCH/RFC/GSoC 3/3] t0301: test credential-cache support of XDG_RUNTIME_DIR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-16T16:17:45Z","receivedAt":"2016-03-16T16:17:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Hui Yiqun <huiyiqun@gmail.com> writes:\n\n> t0301 now tests git-credential-cache support for XDG user-specific\n> runtime file $XDG_RUNTIME_DIR/git/credential.sock. Specifically:\n>\n> * if $XDG_RUNTIME_DIR exists, use socket at\n>   `$XDG_RUNTIME_DIR/git/credential-cache.sock`.\n>\n> * otherwise, `/tmp/git-$uid/credential-cache.sock` is taken.\n\nIs it better to have the fallback in /tmp, and not in\n~/.git-credential-cache/, and why?\n\nIs it because the wish is to always use /tmp/git-$uid/ as a fallback\nfor $XDG_RUNTIME_DIR (as opposed to ~/.git-credential-cache/, which\nis specific to the credential-cache and would look strange if we\nused it for other \"runtime\" things)?\n\nJust being curious, and wanting to see the reasoning behind the\ndesign decision the patch series makes in the log message of one of\nthese patches.\n\nThanks.\n"},{"id":"280917","messageId":"CAKqreuwRpS3uP6=afm-0pBkPW0-bqoJconnKO5q3qTgZwdU_xQ@mail.gmail.com","threadId":"41683","inReplyTo":"xmqqr3fa5rdi.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH/RFC/GSoC 3/3] t0301: test credential-cache support of XDG_RUNTIME_DIR","fromName":"惠轶群","fromEmail":"huiyiqun@gmail.com","sentAt":"2016-03-16T16:40:59Z","receivedAt":"2016-03-16T16:40:59Z","isPatch":true,"sender":{"key":"huiyiqun@gmail.com","avatar":"https://gravatar.com/avatar/44aa01fbc2fb098682386b82a44aac03bd778275249228c55d09c5b6fe2fe0e0?d=mp&s=160"},"body":"2016-03-17 0:17 GMT+08:00 Junio C Hamano <gitster@pobox.com>:\n> Hui Yiqun <huiyiqun@gmail.com> writes:\n>\n>> t0301 now tests git-credential-cache support for XDG user-specific\n>> runtime file $XDG_RUNTIME_DIR/git/credential.sock. Specifically:\n>>\n>> * if $XDG_RUNTIME_DIR exists, use socket at\n>>   `$XDG_RUNTIME_DIR/git/credential-cache.sock`.\n>>\n>> * otherwise, `/tmp/git-$uid/credential-cache.sock` is taken.\n>\n> Is it better to have the fallback in /tmp, and not in\n> ~/.git-credential-cache/, and why?\n>\n> Is it because the wish is to always use /tmp/git-$uid/ as a fallback\n> for $XDG_RUNTIME_DIR (as opposed to ~/.git-credential-cache/, which\n> is specific to the credential-cache and would look strange if we\n> used it for other \"runtime\" things)?\n\nYes, I mean to use it as a general fallback for git.\n\nxdg base dir spec does not specify where to fallback when\n$XDG_RUNTIME_DIR is not defined. It just says:\n\nIf $XDG_RUNTIME_DIR is not set applications should fall back to\na replacement directory with similar capabilities and print a warning\nmessage. Applications should use this directory for communication\nand synchronization purposes and should not place larger files in it,\nsince it might reside in runtime memory and cannot necessarily be\nswapped out to disk.\n\ntmpfs is just like what it describes. And many other applications\nput socket under which, such as tmux.\n\nOn the other hand, I think, falling back to $HOME/.git-credential-cache/socket\ndoesn't make any sense for back-compability cannot be ensured.\n\n>\n> Just being curious, and wanting to see the reasoning behind the\n> design decision the patch series makes in the log message of one of\n> these patches.\n>\n> Thanks.\n>\n"},{"id":"280920","messageId":"20160316165527.GA4039@sigill.intra.peff.net","threadId":"41683","inReplyTo":"CAKqreuwRpS3uP6=afm-0pBkPW0-bqoJconnKO5q3qTgZwdU_xQ@mail.gmail.com","subject":"Re: [PATCH/RFC/GSoC 3/3] t0301: test credential-cache support of XDG_RUNTIME_DIR","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-16T16:55:28Z","receivedAt":"2016-03-16T16:55:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 17, 2016 at 12:40:59AM +0800, 惠轶群 wrote:\n\n> > Is it better to have the fallback in /tmp, and not in\n> > ~/.git-credential-cache/, and why?\n> >\n> > Is it because the wish is to always use /tmp/git-$uid/ as a fallback\n> > for $XDG_RUNTIME_DIR (as opposed to ~/.git-credential-cache/, which\n> > is specific to the credential-cache and would look strange if we\n> > used it for other \"runtime\" things)?\n> \n> Yes, I mean to use it as a general fallback for git.\n> \n> xdg base dir spec does not specify where to fallback when\n> $XDG_RUNTIME_DIR is not defined. It just says:\n> \n> If $XDG_RUNTIME_DIR is not set applications should fall back to\n> a replacement directory with similar capabilities and print a warning\n> message. Applications should use this directory for communication\n> and synchronization purposes and should not place larger files in it,\n> since it might reside in runtime memory and cannot necessarily be\n> swapped out to disk.\n> \n> tmpfs is just like what it describes. And many other applications\n> put socket under which, such as tmux.\n> \n> On the other hand, I think, falling back to $HOME/.git-credential-cache/socket\n> doesn't make any sense for back-compability cannot be ensured.\n\nIf we are going to use a publicly accessible directory like /tmp, I\nthink we need to start worrying about tmp-races with malicious users.\n\nRight now we make sure that an existing socket directory is mode 0700.\nThat's just a courtesy check that the user didn't create it themselves\nwith a permissive mode. But we don't check the owner of the directory,\nand our check is racy with accessing the directory.\n\nSo if we blindly use an existing /tmp/git-$uid, I think an attacker can\nrace with:\n\n    dir=/tmp/git-$victimuid\n    mkdir $dir\n    while true; do\n        chmod 0700 $dir\n\tchmod 0777 $dir\n    done\n\nIf the victim does their mode check while the 0700 is in effect, but\nthen creates the socket during the 0777 moment, they won't notice\nanything amiss. And the attacker will have access to their credential\nsocket.\n\nThis is a classic /tmp race.  I imagine it's less of an issue in this\nday and age when people mostly have their own machines and their own\n/tmp, but we still should not recreate the mistakes of the past.\n\n-Peff\n"},{"id":"280921","messageId":"20160316170623.GB4039@sigill.intra.peff.net","threadId":"41683","inReplyTo":"1458122865-29447-1-git-send-email-huiyiqun@gmail.com","subject":"Re: [PATCH/RFC/GSoC 1/3] path.c: implement xdg_runtime_dir()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-16T17:06:23Z","receivedAt":"2016-03-16T17:06:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 16, 2016 at 06:07:43PM +0800, Hui Yiqun wrote:\n\n> +\tif (runtime_dir && *runtime_dir)\n> +\t\tgit_runtime_dir = mkpathdup(\"%s/git/\", runtime_dir);\n> +\telse\n> +\t\tgit_runtime_dir = mkpathdup(\"/tmp/git-%d\", uid);\n\nHere we allocate the string, but later we may return NULL on error,\nleaking the allocated memory.\n\n> +\tif (!lstat(git_runtime_dir, &st)) {\n> +\t\t/*\n> +\t\t * As described in XDG base dir spec[1], the subdirectory\n> +\t\t * under $XDG_RUNTIME_DIR or its fallback MUST be owned by\n> +\t\t * the user, and its unix access mode MUST be 0700.\n> +\t\t *\n> +\t\t * Calling chmod or chown silently may cause security\n> +\t\t * problem if somebody chdir to it, sleep, and then, try\n> +\t\t * to open our protected runtime cache or socket.\n> +\t\t * So we just put warning and left it to user to solve.\n> +\t\t *\n> +\t\t * [1]https://specifications.freedesktop.org/basedir-spec/\n> +\t\t * basedir-spec-latest.html\n> +\t\t */\n\nOK. I think these checks should be sufficient to deal with the /tmp race\nI mentioned elsewhere in the thread (assuming that an attacker cannot\nflip the uid back and forth in the same way, but that should be true on\nUnix systems).\n\n> +\t\tif ((st.st_mode & 0777) != S_IRWXU) {\n> +\t\t\tfprintf(stderr,\n> +\t\t\t\t\t\"permission of runtime directory '%s' \"\n> +\t\t\t\t\t\"MUST be 0700 instead of 0%o\\n\",\n> +\t\t\t\t\tgit_runtime_dir, (st.st_mode & 0777));\n> +\t\t\treturn NULL;\n> +\t\t} else if (st.st_uid != uid) {\n> +\t\t\tfprintf(stderr,\n> +\t\t\t\t\t\"owner of runtime directory '%s' \"\n> +\t\t\t\t\t\"MUST be %d instead of %d\\n\",\n> +\t\t\t\t\tgit_runtime_dir, uid, st.st_uid);\n> +\t\t\treturn NULL;\n> +\t\t}\n\nShould these be using warning(), rather than a raw fprintf?\n\n> +\t} else {\n> +\t\tif (safe_create_leading_directories_const(git_runtime_dir) < 0) {\n> +\t\t\tfprintf(stderr,\n> +\t\t\t\t\t\"unable to create directories for '%s'\\n\",\n> +\t\t\t\t\tgit_runtime_dir);\n> +\t\t\treturn NULL;\n> +\t\t}\n> +\t\tif (mkdir(git_runtime_dir, 0700) < 0) {\n> +\t\t\tfprintf(stderr,\n> +\t\t\t\t\t\"unable to mkdir '%s'\\n\", git_runtime_dir);\n> +\t\t\treturn NULL;\n> +\t\t}\n> +\t}\n\nAnd this retains the un-racy mkdir(). Good.\n\n> +\tfree(git_runtime_dir);\n> +\treturn mkpathdup(\"%s/%s\", git_runtime_dir, filename);\n\nThis mkpathdup accesses the string we just freed?\n\nIt might be easier to just use a strbuf here, and then you can append to\nit at the end.\n\n-Peff\n"},{"id":"280925","messageId":"20160316171535.GC4039@sigill.intra.peff.net","threadId":"41683","inReplyTo":"1458122865-29447-3-git-send-email-huiyiqun@gmail.com","subject":"Re: [PATCH/RFC/GSoC 3/3] t0301: test credential-cache support of XDG_RUNTIME_DIR","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-16T17:15:36Z","receivedAt":"2016-03-16T17:15:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 16, 2016 at 06:07:45PM +0800, Hui Yiqun wrote:\n\n> diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh\n> index 82c8411..0718bb0 100755\n> --- a/t/t0301-credential-cache.sh\n> +++ b/t/t0301-credential-cache.sh\n> @@ -12,7 +12,32 @@ test -z \"$NO_UNIX_SOCKETS\" || {\n>  # don't leave a stale daemon running\n>  trap 'code=$?; git credential-cache exit; (exit $code); die' EXIT\n>  \n> +test_expect_success 'set $XDG_RUNTIME_DIR' '\n> +\tXDG_RUNTIME_DIR=$HOME/xdg_runtime/\n> +'\n\nDoesn't this need to export the variable so that credential-cache can\nsee it?\n\n> +\n> +helper_test cache\n> +\n\nThis runs the full suite of tests twice (once here, and once for the\noriginal helper_test invocation you left below). Shouldn't we just do it\nonce (making sure that $XDG_RUNTIME_DIR is respected)?\n\n> +test_expect_success 'force git-credential-cache to exit so that socket disappear' '\n> +\tgit credential-cache exit &&\n> +\ttest_path_is_missing \"$XDG_RUNTIME_DIR/git/credential-cache.sock\" &&\n> +\tunset XDG_RUNTIME_DIR\n> +'\n\nI wondered if this might be racy. credential-cache tells the daemon\n\"exit\", then waits for a response or EOF. The daemon sees \"exit\" and\ncalls exit(0) immediately. We clean up the socket in an atexit()\nhandler. So I think we are OK (the pipe will get closed when the process\nexits, and the atexit handler must have run by then).\n\nBut that definitely was not designed, and is just how it happens to\nwork. I'm not sure if it's worth commenting on that (here, or perhaps in\nthe daemon code).\n\n-Peff\n"},{"id":"280927","messageId":"xmqqbn6e5o9v.fsf@gitster.mtv.corp.google.com","threadId":"41683","inReplyTo":"CAKqreuwRpS3uP6=afm-0pBkPW0-bqoJconnKO5q3qTgZwdU_xQ@mail.gmail.com","subject":"Re: [PATCH/RFC/GSoC 3/3] t0301: test credential-cache support of XDG_RUNTIME_DIR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-16T17:24:44Z","receivedAt":"2016-03-16T17:24:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"惠轶群 <huiyiqun@gmail.com> writes:\n\n>> Is it because the wish is to always use /tmp/git-$uid/ as a fallback\n>> for $XDG_RUNTIME_DIR (as opposed to ~/.git-credential-cache/, which\n>> is specific to the credential-cache and would look strange if we\n>> used it for other \"runtime\" things)?\n>\n> Yes, I mean to use it as a general fallback for git.\n\nIf that is the case the code probably needs to be a bit more careful\nbefore deciding to use /tmp/git-$uid/ directory (is it really $uid's?\ncan anybody else write to it to race with the real user? etc.)\n\n> On the other hand, I think, falling back to $HOME/.git-credential-cache/socket\n> doesn't make any sense for back-compability cannot be ensured.\n\nWhat do you mean by that?\n\nUsing ~/.git-credential-cache/credential-cache.sock would not help\nat all for existing users, but ~/.git-credential-cache/socket would\ninteroperate well with users with existing versions of Git, no?\n\n>> Just being curious, and wanting to see the reasoning behind the\n>> design decision the patch series makes in the log message of one of\n>> these patches.\n"},{"id":"280998","messageId":"729c6af810e3000e789f4afd537b77a8@riseup.net","threadId":"41683","inReplyTo":"xmqqbn6e5o9v.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH/RFC/GSoC 3/3] t0301: test credential-cache support of XDG_RUNTIME_DIR","fromName":"谭俊浩","fromEmail":"pickfire@riseup.net","sentAt":"2016-03-17T03:59:14Z","receivedAt":"2016-03-17T03:59:14Z","isPatch":true,"sender":{"key":"pickfire@riseup.net","avatar":null},"body":"On 17/03/2016 01:24, Junio C Hamano wrote:\n> 惠轶群 <huiyiqun@gmail.com> writes:\n> \n>>> Is it because the wish is to always use /tmp/git-$uid/ as a fallback\n>>> for $XDG_RUNTIME_DIR (as opposed to ~/.git-credential-cache/, which\n>>> is specific to the credential-cache and would look strange if we\n>>> used it for other \"runtime\" things)?\n>> \n>> Yes, I mean to use it as a general fallback for git.\n> \n> If that is the case the code probably needs to be a bit more careful\n> before deciding to use /tmp/git-$uid/ directory (is it really $uid's?\n> can anybody else write to it to race with the real user? etc.)\n> \n>> On the other hand, I think, falling back to \n>> $HOME/.git-credential-cache/socket\n>> doesn't make any sense for back-compability cannot be ensured.\n> \n> What do you mean by that?\n> \n> Using ~/.git-credential-cache/credential-cache.sock would not help\n> at all for existing users, but ~/.git-credential-cache/socket would\n> interoperate well with users with existing versions of Git, no?\n> \n>>> Just being curious, and wanting to see the reasoning behind the\n>>> design decision the patch series makes in the log message of one of\n>>> these patches.\n\nI guess it is better to use /tmp or such instead of $HOME/.* so that\nthe users home directory won't be flooded by sockets.\n"},{"id":"281012","messageId":"xmqqmvpx1q25.fsf@gitster.mtv.corp.google.com","threadId":"41683","inReplyTo":"729c6af810e3000e789f4afd537b77a8@riseup.net","subject":"Re: [PATCH/RFC/GSoC 3/3] t0301: test credential-cache support of XDG_RUNTIME_DIR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-17T08:12:02Z","receivedAt":"2016-03-17T08:12:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"谭俊浩 <pickfire@riseup.net> writes:\n\n> On 17/03/2016 01:24, Junio C Hamano wrote:\n>\n>> Using ~/.git-credential-cache/credential-cache.sock would not help\n>> at all for existing users, but ~/.git-credential-cache/socket would\n>> interoperate well with users with existing versions of Git, no?\n>>\n>>>> Just being curious, and wanting to see the reasoning behind the\n>>>> design decision the patch series makes in the log message of one of\n>>>> these patches.\n>\n> I guess it is better to use /tmp or such instead of $HOME/.* so that\n> the users home directory won't be flooded by sockets.\n\nThe \"fallback\" being discussed is to see if $XDG can be used (and\nuse it if so), otherwise see if ~/.git-credential-cache/socket can\nbe used (and use it if so), otherwise die with a message (see\ncredential-cache.c).  The order of the falling back may want to be\nthe other way around, but in either case, the definition of \"can be\nused\" includes \"is there already a directory in which we can create\na socket?\".\n\nThe existing versions have used ~/.git-credential-cache/socket as\nthe default socket path, so it is reasonable to expect that users\nthat are already using the feature already have the directory there.\n\nSo I do not think there is any \"flooded\" involved; if the directory\nis already there, we can use it to create and use a single socket.\nIt's not like we'd be creating many random new directories in ~/.\n"},{"id":"281018","messageId":"CAKqreux4Z4a6qXGewEWLxDZFE+PCXFx3d4d_jrrLvQqX9HKy-Q@mail.gmail.com","threadId":"41683","inReplyTo":"xmqqbn6e5o9v.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH/RFC/GSoC 3/3] t0301: test credential-cache support of XDG_RUNTIME_DIR","fromName":"惠轶群","fromEmail":"huiyiqun@gmail.com","sentAt":"2016-03-17T09:45:05Z","receivedAt":"2016-03-17T09:45:05Z","isPatch":true,"sender":{"key":"huiyiqun@gmail.com","avatar":"https://gravatar.com/avatar/44aa01fbc2fb098682386b82a44aac03bd778275249228c55d09c5b6fe2fe0e0?d=mp&s=160"},"body":"2016-03-17 1:24 GMT+08:00 Junio C Hamano <gitster@pobox.com>:\n> 惠轶群 <huiyiqun@gmail.com> writes:\n>\n>>> Is it because the wish is to always use /tmp/git-$uid/ as a fallback\n>>> for $XDG_RUNTIME_DIR (as opposed to ~/.git-credential-cache/, which\n>>> is specific to the credential-cache and would look strange if we\n>>> used it for other \"runtime\" things)?\n>>\n>> Yes, I mean to use it as a general fallback for git.\n>\n> If that is the case the code probably needs to be a bit more careful\n> before deciding to use /tmp/git-$uid/ directory (is it really $uid's?\n> can anybody else write to it to race with the real user? etc.)\n>\n>> On the other hand, I think, falling back to $HOME/.git-credential-cache/socket\n>> doesn't make any sense for back-compability cannot be ensured.\n>\n> What do you mean by that?\n>\n> Using ~/.git-credential-cache/credential-cache.sock would not help\n> at all for existing users, but ~/.git-credential-cache/socket would\n> interoperate well with users with existing versions of Git, no?\n>\n\nI meant that FALLBACK instead of DEFAULT is useless for back-compatibility.\n\nMost users with existing versions of Git will be broken even if there is a\nfallback.\n"},{"id":"281019","messageId":"CAKqreuy2-QVxuKuC707MdgxMafx31R00NiY_tBP3-mGqHtbohw@mail.gmail.com","threadId":"41683","inReplyTo":"xmqqmvpx1q25.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH/RFC/GSoC 3/3] t0301: test credential-cache support of XDG_RUNTIME_DIR","fromName":"惠轶群","fromEmail":"huiyiqun@gmail.com","sentAt":"2016-03-17T10:10:33Z","receivedAt":"2016-03-17T10:10:33Z","isPatch":true,"sender":{"key":"huiyiqun@gmail.com","avatar":"https://gravatar.com/avatar/44aa01fbc2fb098682386b82a44aac03bd778275249228c55d09c5b6fe2fe0e0?d=mp&s=160"},"body":"2016-03-17 16:12 GMT+08:00 Junio C Hamano <gitster@pobox.com>:\n> 谭俊浩 <pickfire@riseup.net> writes:\n>\n>> On 17/03/2016 01:24, Junio C Hamano wrote:\n>>\n>>> Using ~/.git-credential-cache/credential-cache.sock would not help\n>>> at all for existing users, but ~/.git-credential-cache/socket would\n>>> interoperate well with users with existing versions of Git, no?\n>>>\n>>>>> Just being curious, and wanting to see the reasoning behind the\n>>>>> design decision the patch series makes in the log message of one of\n>>>>> these patches.\n>>\n>> I guess it is better to use /tmp or such instead of $HOME/.* so that\n>> the users home directory won't be flooded by sockets.\n>\n> The \"fallback\" being discussed is to see if $XDG can be used (and\n> use it if so), otherwise see if ~/.git-credential-cache/socket can\n> be used (and use it if so), otherwise die with a message (see\n> credential-cache.c).  The order of the falling back may want to be\n> the other way around, but in either case, the definition of \"can be\n> used\" includes \"is there already a directory in which we can create\n> a socket?\".\n>\n> The existing versions have used ~/.git-credential-cache/socket as\n> the default socket path, so it is reasonable to expect that users\n> that are already using the feature already have the directory there.\n>\n> So I do not think there is any \"flooded\" involved; if the directory\n> is already there, we can use it to create and use a single socket.\n> It's not like we'd be creating many random new directories in ~/.\n\nAs mentioned above, The purpose I try \"$XDG_RUNTIME_DIR\" and then\n\"/tmp/git-$uid\" is that I'd like to build an identical mechanism for searching\nfor path to store runtime files such as socket and so on.\n\nI guess you would not second an additional configuration to let git\nstore runtime\nfiles under XDG-compatible path.\n\nThen is there any better solution to keep compatibility than checking the\nexistence of ~/.git-credential-cache? I'm not sure.\n\nI think we could left following message in documentation:\n\n    From version xxx, we put the socket under a XDG-compatible path instead of\n    a directory under $HOME. If that incompatibility disturbs you,\nplease consider\n    adding `--socket $HOME/.git-credential-cache/socket` to your configuration.\n"},{"id":"281021","messageId":"CAKqreuxBtu3hXVxY27_u-9ZxSP_AfSqeoB=d_UJdUsSTphgf=g@mail.gmail.com","threadId":"41683","inReplyTo":"20160316170623.GB4039@sigill.intra.peff.net","subject":"Re: [PATCH/RFC/GSoC 1/3] path.c: implement xdg_runtime_dir()","fromName":"惠轶群","fromEmail":"huiyiqun@gmail.com","sentAt":"2016-03-17T10:20:14Z","receivedAt":"2016-03-17T10:20:14Z","isPatch":true,"sender":{"key":"huiyiqun@gmail.com","avatar":"https://gravatar.com/avatar/44aa01fbc2fb098682386b82a44aac03bd778275249228c55d09c5b6fe2fe0e0?d=mp&s=160"},"body":"2016-03-17 1:06 GMT+08:00 Jeff King <peff@peff.net>:\n> On Wed, Mar 16, 2016 at 06:07:43PM +0800, Hui Yiqun wrote:\n>\n>> +     if (runtime_dir && *runtime_dir)\n>> +             git_runtime_dir = mkpathdup(\"%s/git/\", runtime_dir);\n>> +     else\n>> +             git_runtime_dir = mkpathdup(\"/tmp/git-%d\", uid);\n>\n> Here we allocate the string, but later we may return NULL on error,\n> leaking the allocated memory.\n\nYes, do you think goto is a good solution for clearup?\n\n>\n>> +     if (!lstat(git_runtime_dir, &st)) {\n>> +             /*\n>> +              * As described in XDG base dir spec[1], the subdirectory\n>> +              * under $XDG_RUNTIME_DIR or its fallback MUST be owned by\n>> +              * the user, and its unix access mode MUST be 0700.\n>> +              *\n>> +              * Calling chmod or chown silently may cause security\n>> +              * problem if somebody chdir to it, sleep, and then, try\n>> +              * to open our protected runtime cache or socket.\n>> +              * So we just put warning and left it to user to solve.\n>> +              *\n>> +              * [1]https://specifications.freedesktop.org/basedir-spec/\n>> +              * basedir-spec-latest.html\n>> +              */\n>\n> OK. I think these checks should be sufficient to deal with the /tmp race\n> I mentioned elsewhere in the thread (assuming that an attacker cannot\n> flip the uid back and forth in the same way, but that should be true on\n> Unix systems).\n>\n>> +             if ((st.st_mode & 0777) != S_IRWXU) {\n>> +                     fprintf(stderr,\n>> +                                     \"permission of runtime directory '%s' \"\n>> +                                     \"MUST be 0700 instead of 0%o\\n\",\n>> +                                     git_runtime_dir, (st.st_mode & 0777));\n>> +                     return NULL;\n>> +             } else if (st.st_uid != uid) {\n>> +                     fprintf(stderr,\n>> +                                     \"owner of runtime directory '%s' \"\n>> +                                     \"MUST be %d instead of %d\\n\",\n>> +                                     git_runtime_dir, uid, st.st_uid);\n>> +                     return NULL;\n>> +             }\n>\n> Should these be using warning(), rather than a raw fprintf?\n\nWell, I will replace it.\n\nDuring the greping. I found that I should also wrap my warning strings\nwith _() for i18n.\n\n>\n>> +     } else {\n>> +             if (safe_create_leading_directories_const(git_runtime_dir) < 0) {\n>> +                     fprintf(stderr,\n>> +                                     \"unable to create directories for '%s'\\n\",\n>> +                                     git_runtime_dir);\n>> +                     return NULL;\n>> +             }\n>> +             if (mkdir(git_runtime_dir, 0700) < 0) {\n>> +                     fprintf(stderr,\n>> +                                     \"unable to mkdir '%s'\\n\", git_runtime_dir);\n>> +                     return NULL;\n>> +             }\n>> +     }\n>\n> And this retains the un-racy mkdir(). Good.\n>\n>> +     free(git_runtime_dir);\n>> +     return mkpathdup(\"%s/%s\", git_runtime_dir, filename);\n>\n> This mkpathdup accesses the string we just freed?\n>\n> It might be easier to just use a strbuf here, and then you can append to\n> it at the end.\n\nI think so. Thanks.\n\n>\n> -Peff\n"},{"id":"281022","messageId":"CAKqreuyOpNOGmWCwLnKY7_ax7szfsFX8R8gpYRym9BYe+3zdDA@mail.gmail.com","threadId":"41683","inReplyTo":"1458122865-29447-2-git-send-email-huiyiqun@gmail.com","subject":"Re: [PATCH/RFC/GSoC 2/3] git-credential-cache: put socket to xdg-compatible path","fromName":"惠轶群","fromEmail":"huiyiqun@gmail.com","sentAt":"2016-03-17T10:26:00Z","receivedAt":"2016-03-17T10:26:00Z","isPatch":true,"sender":{"key":"huiyiqun@gmail.com","avatar":"https://gravatar.com/avatar/44aa01fbc2fb098682386b82a44aac03bd778275249228c55d09c5b6fe2fe0e0?d=mp&s=160"},"body":"2016-03-16 18:07 GMT+08:00 Hui Yiqun <huiyiqun@gmail.com>:\n> move .git-credential-cache/socket to xdg_runtime_dir(\"credential-cache.sock\")\n>\n> Signed-off-by: Hui Yiqun <huiyiqun@gmail.com>\n> ---\n>  credential-cache.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/credential-cache.c b/credential-cache.c\n> index f4afdc6..40d838b 100644\n> --- a/credential-cache.c\n> +++ b/credential-cache.c\n> @@ -105,7 +105,7 @@ int main(int argc, const char **argv)\n>         op = argv[0];\n>\n>         if (!socket_path)\n> -               socket_path = expand_user_path(\"~/.git-credential-cache/socket\");\n> +               socket_path = xdg_runtime_dir(\"credential-cache.sock\");\n>         if (!socket_path)\n>                 die(\"unable to find a suitable socket path; use --socket\");\n>\n> --\n> 2.7.2\n>\nI'm sure but if user set up git-credential-cache with following command:\n\n    git config --global credential.helper \"cache --socket\n~/.git-credential-cache/socket\"\n\nwill the ~ be expanded?\n"},{"id":"281108","messageId":"CAKqreuyirQdzjh3Un=ieJMKLicyG7onyy=YTjZ4zOTqfV06eew@mail.gmail.com","threadId":"41683","inReplyTo":"20160316171535.GC4039@sigill.intra.peff.net","subject":"Re: [PATCH/RFC/GSoC 3/3] t0301: test credential-cache support of XDG_RUNTIME_DIR","fromName":"惠轶群","fromEmail":"huiyiqun@gmail.com","sentAt":"2016-03-18T04:35:14Z","receivedAt":"2016-03-18T04:35:14Z","isPatch":true,"sender":{"key":"huiyiqun@gmail.com","avatar":"https://gravatar.com/avatar/44aa01fbc2fb098682386b82a44aac03bd778275249228c55d09c5b6fe2fe0e0?d=mp&s=160"},"body":"2016-03-17 1:15 GMT+08:00 Jeff King <peff@peff.net>:\n> On Wed, Mar 16, 2016 at 06:07:45PM +0800, Hui Yiqun wrote:\n>\n>> diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh\n>> index 82c8411..0718bb0 100755\n>> --- a/t/t0301-credential-cache.sh\n>> +++ b/t/t0301-credential-cache.sh\n>> @@ -12,7 +12,32 @@ test -z \"$NO_UNIX_SOCKETS\" || {\n>>  # don't leave a stale daemon running\n>>  trap 'code=$?; git credential-cache exit; (exit $code); die' EXIT\n>>\n>> +test_expect_success 'set $XDG_RUNTIME_DIR' '\n>> +     XDG_RUNTIME_DIR=$HOME/xdg_runtime/\n>> +'\n>\n> Doesn't this need to export the variable so that credential-cache can\n> see it?\n\nI'm not sure, but it seems that a little clean up code added before send-email\nmake the test fail. At that time, I run test without building. I've\nsend PATCH v2\nwhich runs well on my computer. However, $XDG_RUNTIME_DIR is still not\nexported, but that just works.\n\nI will try to dig deeper into the bash script to see why.\n\n>\n>> +\n>> +helper_test cache\n>> +\n>\n> This runs the full suite of tests twice (once here, and once for the\n> original helper_test invocation you left below). Shouldn't we just do it\n> once (making sure that $XDG_RUNTIME_DIR is respected)?\n\nI'd like to test the behavior of git-credential-cache when\n$XDG_RUNTIME_DIR is unset.\n\nIn `t/t0302-credential-store.sh`, helper_test is also run multiple\ntimes. That's why I\ndo so.\n\n>> +test_expect_success 'force git-credential-cache to exit so that socket disappear' '\n>> +     git credential-cache exit &&\n>> +     test_path_is_missing \"$XDG_RUNTIME_DIR/git/credential-cache.sock\" &&\n>> +     unset XDG_RUNTIME_DIR\n>> +'\n>\n> I wondered if this might be racy. credential-cache tells the daemon\n> \"exit\", then waits for a response or EOF. The daemon sees \"exit\" and\n> calls exit(0) immediately. We clean up the socket in an atexit()\n> handler. So I think we are OK (the pipe will get closed when the process\n> exits, and the atexit handler must have run by then).\n>\n> But that definitely was not designed, and is just how it happens to\n> work. I'm not sure if it's worth commenting on that (here, or perhaps in\n> the daemon code).\n\nI'm still confused.\n\nWhat do you mean by \"pipe\"? should it be \"socket\" instead?\n\nWhat is not designed? cleanup being done, my tests passing or the\nsynchronization?\n\n>\n> -Pef\n"},{"id":"281112","messageId":"20160318050017.GA22327@sigill.intra.peff.net","threadId":"41683","inReplyTo":"CAKqreuw7Am_wZQjYYjvsxx0Ccr4OOwoF=EnLvMTK9jxeBUFv5Q@mail.gmail.com","subject":"Re: [PATCH/RFC/GSoC 3/3] t0301: test credential-cache support of XDG_RUNTIME_DIR","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-18T05:00:18Z","receivedAt":"2016-03-18T05:00:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 18, 2016 at 12:34:04PM +0800, 惠轶群 wrote:\n\n> >> +test_expect_success 'set $XDG_RUNTIME_DIR' '\n> >> +     XDG_RUNTIME_DIR=$HOME/xdg_runtime/\n> >> +'\n> >\n> > Doesn't this need to export the variable so that credential-cache can\n> > see it?\n> \n> I'm not sure, but it seems that a little clean up code added before\n> send-email\n> make the test fail. At that time, I run test without building. I've send\n> PATCH v2\n> which runs well on my computer. However, $XDG_RUNTIME_DIR is still not\n> exported, but that just works.\n> \n> I will try to dig deeper into the bash script to see why.\n\nI suspect it is because you have $XDG_RUNTIME_DIR defined in your\nenvironment, which causes the shell to automatically export it. I don't,\nso an explicit \"export\" is required to for the variable to make it to\nits children.\n\nI think we should actually be unsetting it in test-lib.sh for all tests,\nas we do for XDG_CONFIG_HOME. That makes sure the tests are running with\na known state.\n\nFor the non-XDG_RUNTIME_DIR tests, does this mean we are creating the\nsocket in /tmp? I'm not entirely happy with that, as we usually try to\nhave the test suite avoid touching anything outside of its trash\ndirectories.\n\n> > This runs the full suite of tests twice (once here, and once for the\n> > original helper_test invocation you left below). Shouldn't we just do it\n> > once (making sure that $XDG_RUNTIME_DIR is respected)?\n> \n> I'd like to test the behavior of git-credential-cache when $XDG_RUNTIME_DIR\n> is unset.\n> \n> In `t/t0302-credential-store.sh`, helper_test is also run multiple times.\n> That's why I do so.\n\nOK. My main concern was just that the tests would take too long, but the\nslow one is the cache test at the end, which is not repeated. So I think\nthis is fine.\n\n> > I wondered if this might be racy. credential-cache tells the daemon\n> > \"exit\", then waits for a response or EOF. The daemon sees \"exit\" and\n> > calls exit(0) immediately. We clean up the socket in an atexit()\n> > handler. So I think we are OK (the pipe will get closed when the process\n> > exits, and the atexit handler must have run by then).\n> >\n> > But that definitely was not designed, and is just how it happens to\n> > work. I'm not sure if it's worth commenting on that (here, or perhaps in\n> > the daemon code).\n> \n> I'm still confused.\n> \n> What do you mean by \"pipe\"? should it be \"socket\" instead?\n\nSorry, yes, I used \"pipe\" and \"socket\" interchangeably there.\n\n> What is not designed? cleanup being done, my tests passing or the\n> synchronization?\n\nThe synchronization. If the daemon were implemented as:\n\n  if (!strcmp(action.buf, \"exit\")) {\n\t/* acknowledge that we got command */\n\tfclose(out);\n\texit(0);\n  }\n\nfor example, then the client would exit at the same that the daemon is\ncleaning up the socket, and we may or may not call test_path_is_missing\nbefore the cleanup is done.\n\nI think it's OK to rely on that, but we may want to put a comment to\nthat effect in the daemon code so that it doesn't get changed.\n\n-Peff\n"},{"id":"281115","messageId":"CAKqreuywzU2i42WzFCOOZWYfgeLXwQA_Ok1E4rZ94RcEaH_abA@mail.gmail.com","threadId":"41683","inReplyTo":"20160318050017.GA22327@sigill.intra.peff.net","subject":"Re: [PATCH/RFC/GSoC 3/3] t0301: test credential-cache support of XDG_RUNTIME_DIR","fromName":"惠轶群","fromEmail":"huiyiqun@gmail.com","sentAt":"2016-03-18T05:11:55Z","receivedAt":"2016-03-18T05:11:55Z","isPatch":true,"sender":{"key":"huiyiqun@gmail.com","avatar":"https://gravatar.com/avatar/44aa01fbc2fb098682386b82a44aac03bd778275249228c55d09c5b6fe2fe0e0?d=mp&s=160"},"body":"2016-03-18 13:00 GMT+08:00 Jeff King <peff@peff.net>:\n> On Fri, Mar 18, 2016 at 12:34:04PM +0800, 惠轶群 wrote:\n>\n>> >> +test_expect_success 'set $XDG_RUNTIME_DIR' '\n>> >> +     XDG_RUNTIME_DIR=$HOME/xdg_runtime/\n>> >> +'\n>> >\n>> > Doesn't this need to export the variable so that credential-cache can\n>> > see it?\n>>\n>> I'm not sure, but it seems that a little clean up code added before\n>> send-email\n>> make the test fail. At that time, I run test without building. I've send\n>> PATCH v2\n>> which runs well on my computer. However, $XDG_RUNTIME_DIR is still not\n>> exported, but that just works.\n>>\n>> I will try to dig deeper into the bash script to see why.\n>\n> I suspect it is because you have $XDG_RUNTIME_DIR defined in your\n> environment, which causes the shell to automatically export it. I don't,\n> so an explicit \"export\" is required to for the variable to make it to\n> its children.\n\nYes, that's the problem. the explicit \"export\" is new knowledge for me, thanks.\n\n> I think we should actually be unsetting it in test-lib.sh for all tests,\n> as we do for XDG_CONFIG_HOME. That makes sure the tests are running with\n> a known state.\n\nWell, I seems a good choice.\n\n> For the non-XDG_RUNTIME_DIR tests, does this mean we are creating the\n> socket in /tmp? I'm not entirely happy with that, as we usually try to\n> have the test suite avoid touching anything outside of its trash\n> directories.\n>\n>> > This runs the full suite of tests twice (once here, and once for the\n>> > original helper_test invocation you left below). Shouldn't we just do it\n>> > once (making sure that $XDG_RUNTIME_DIR is respected)?\n>>\n>> I'd like to test the behavior of git-credential-cache when $XDG_RUNTIME_DIR\n>> is unset.\n>>\n>> In `t/t0302-credential-store.sh`, helper_test is also run multiple times.\n>> That's why I do so.\n>\n> OK. My main concern was just that the tests would take too long, but the\n> slow one is the cache test at the end, which is not repeated. So I think\n> this is fine.\n>\n>> > I wondered if this might be racy. credential-cache tells the daemon\n>> > \"exit\", then waits for a response or EOF. The daemon sees \"exit\" and\n>> > calls exit(0) immediately. We clean up the socket in an atexit()\n>> > handler. So I think we are OK (the pipe will get closed when the process\n>> > exits, and the atexit handler must have run by then).\n>> >\n>> > But that definitely was not designed, and is just how it happens to\n>> > work. I'm not sure if it's worth commenting on that (here, or perhaps in\n>> > the daemon code).\n>>\n>> I'm still confused.\n>>\n>> What do you mean by \"pipe\"? should it be \"socket\" instead?\n>\n> Sorry, yes, I used \"pipe\" and \"socket\" interchangeably there.\n>\n>> What is not designed? cleanup being done, my tests passing or the\n>> synchronization?\n>\n> The synchronization. If the daemon were implemented as:\n>\n>   if (!strcmp(action.buf, \"exit\")) {\n>         /* acknowledge that we got command */\n>         fclose(out);\n>         exit(0);\n>   }\n>\n> for example, then the client would exit at the same that the daemon is\n> cleaning up the socket, and we may or may not call test_path_is_missing\n> before the cleanup is done.\n>\n> I think it's OK to rely on that, but we may want to put a comment to\n> that effect in the daemon code so that it doesn't get changed.\n\nThe current implementation is natural for me. But having additional comment\nis better.\n\n>\n> -Peff\n"},{"id":"281125","messageId":"CAKqreuwpjhLnttP4Z_KmYwjiRYxBVC6gLShKLAcjox1VVqfm7w@mail.gmail.com","threadId":"41683","inReplyTo":"CAKqreuywzU2i42WzFCOOZWYfgeLXwQA_Ok1E4rZ94RcEaH_abA@mail.gmail.com","subject":"Re: [PATCH/RFC/GSoC 3/3] t0301: test credential-cache support of XDG_RUNTIME_DIR","fromName":"惠轶群","fromEmail":"huiyiqun@gmail.com","sentAt":"2016-03-18T06:02:08Z","receivedAt":"2016-03-18T06:02:08Z","isPatch":true,"sender":{"key":"huiyiqun@gmail.com","avatar":"https://gravatar.com/avatar/44aa01fbc2fb098682386b82a44aac03bd778275249228c55d09c5b6fe2fe0e0?d=mp&s=160"},"body":"2016-03-18 13:11 GMT+08:00 惠轶群 <huiyiqun@gmail.com>:\n> 2016-03-18 13:00 GMT+08:00 Jeff King <peff@peff.net>:\n>> On Fri, Mar 18, 2016 at 12:34:04PM +0800, 惠轶群 wrote:\n>>\n>>> >> +test_expect_success 'set $XDG_RUNTIME_DIR' '\n>>> >> +     XDG_RUNTIME_DIR=$HOME/xdg_runtime/\n>>> >> +'\n>>> >\n>>> > Doesn't this need to export the variable so that credential-cache can\n>>> > see it?\n>>>\n>>> I'm not sure, but it seems that a little clean up code added before\n>>> send-email\n>>> make the test fail. At that time, I run test without building. I've send\n>>> PATCH v2\n>>> which runs well on my computer. However, $XDG_RUNTIME_DIR is still not\n>>> exported, but that just works.\n>>>\n>>> I will try to dig deeper into the bash script to see why.\n>>\n>> I suspect it is because you have $XDG_RUNTIME_DIR defined in your\n>> environment, which causes the shell to automatically export it. I don't,\n>> so an explicit \"export\" is required to for the variable to make it to\n>> its children.\n>\n> Yes, that's the problem. the explicit \"export\" is new knowledge for me, thanks.\n>\n>> I think we should actually be unsetting it in test-lib.sh for all tests,\n>> as we do for XDG_CONFIG_HOME. That makes sure the tests are running with\n>> a known state.\n>\n> Well, I seems a good choice.\n>\n>> For the non-XDG_RUNTIME_DIR tests, does this mean we are creating the\n>> socket in /tmp? I'm not entirely happy with that, as we usually try to\n>> have the test suite avoid touching anything outside of its trash\n>> directories.\n>>\n>>> > This runs the full suite of tests twice (once here, and once for the\n>>> > original helper_test invocation you left below). Shouldn't we just do it\n>>> > once (making sure that $XDG_RUNTIME_DIR is respected)?\n>>>\n>>> I'd like to test the behavior of git-credential-cache when $XDG_RUNTIME_DIR\n>>> is unset.\n>>>\n>>> In `t/t0302-credential-store.sh`, helper_test is also run multiple times.\n>>> That's why I do so.\n>>\n>> OK. My main concern was just that the tests would take too long, but the\n>> slow one is the cache test at the end, which is not repeated. So I think\n>> this is fine.\n>>\n>>> > I wondered if this might be racy. credential-cache tells the daemon\n>>> > \"exit\", then waits for a response or EOF. The daemon sees \"exit\" and\n>>> > calls exit(0) immediately. We clean up the socket in an atexit()\n>>> > handler. So I think we are OK (the pipe will get closed when the process\n>>> > exits, and the atexit handler must have run by then).\n>>> >\n>>> > But that definitely was not designed, and is just how it happens to\n>>> > work. I'm not sure if it's worth commenting on that (here, or perhaps in\n>>> > the daemon code).\n>>>\n>>> I'm still confused.\n>>>\n>>> What do you mean by \"pipe\"? should it be \"socket\" instead?\n>>\n>> Sorry, yes, I used \"pipe\" and \"socket\" interchangeably there.\n>>\n>>> What is not designed? cleanup being done, my tests passing or the\n>>> synchronization?\n>>\n>> The synchronization. If the daemon were implemented as:\n>>\n>>   if (!strcmp(action.buf, \"exit\")) {\n>>         /* acknowledge that we got command */\n>>         fclose(out);\n>>         exit(0);\n>>   }\n>>\n>> for example, then the client would exit at the same that the daemon is\n>> cleaning up the socket, and we may or may not call test_path_is_missing\n>> before the cleanup is done.\n>>\n>> I think it's OK to rely on that, but we may want to put a comment to\n>> that effect in the daemon code so that it doesn't get changed.\n>\n> The current implementation is natural for me. But having additional comment\n> is better.\n\nI believe git-credential--daemon is a better place to comment on, but\nI'm not sure\nwhether the comment should be included in this patch set. Above all, they are\nnot quite related.\n\n>\n>>\n>> -Peff\n"},{"id":"281127","messageId":"20160318061201.GA28102@sigill.intra.peff.net","threadId":"41683","inReplyTo":"CAKqreuwpjhLnttP4Z_KmYwjiRYxBVC6gLShKLAcjox1VVqfm7w@mail.gmail.com","subject":"[PATCH] credential-cache--daemon: clarify \"exit\" action semantics","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-18T06:12:01Z","receivedAt":"2016-03-18T06:12:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 18, 2016 at 02:02:08PM +0800, 惠轶群 wrote:\n\n> I believe git-credential--daemon is a better place to comment on, but\n> I'm not sure whether the comment should be included in this patch set.\n> Above all, they are not quite related.\n\nYes, it could be completely separate from your series.\n\nHere is what I think we should do:\n\n-- >8 --\nSubject: credential-cache--daemon: clarify \"exit\" action semantics\n\nWhen this code was originally written, there wasn't much\nthought given to the timing between a client asking for\n\"exit\", the daemon signaling that the action is done (with\nEOF), and the actual cleanup of the socket.\n\nHowever, we need to care about this so that our test scripts\ndo not end up racy (e.g., by asking for an exit and checking\nthat the socket was cleaned up). The code that is already\nthere happens to behave very reasonably; let's add a comment\nto make it clear that any changes should retain the same\nbehavior.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n credential-cache--daemon.c | 11 ++++++++++-\n 1 file changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/credential-cache--daemon.c b/credential-cache--daemon.c\nindex caef21e..291c0fd 100644\n--- a/credential-cache--daemon.c\n+++ b/credential-cache--daemon.c\n@@ -126,8 +126,17 @@ static void serve_one_client(FILE *in, FILE *out)\n \t\t\tfprintf(out, \"password=%s\\n\", e->item.password);\n \t\t}\n \t}\n-\telse if (!strcmp(action.buf, \"exit\"))\n+\telse if (!strcmp(action.buf, \"exit\")) {\n+\t\t/*\n+\t\t * It's important that we clean up our socket first, and then\n+\t\t * signal the client only once we have finished the cleanup.\n+\t\t * Calling exit() directly does this, because we clean up in\n+\t\t * our atexit() handler, and then signal the client when our\n+\t\t * process actually ends, which closes the socket and gives\n+\t\t * them EOF.\n+\t\t */\n \t\texit(0);\n+\t}\n \telse if (!strcmp(action.buf, \"erase\"))\n \t\tremove_credential(&c);\n \telse if (!strcmp(action.buf, \"store\")) {\n-- \n2.8.0.rc3.378.gf2f7872\n"}]}