[PATCH v2/GSoC 1/4] path.c: implement strbuf_mkpath()

Common logic of mkpath() and mkpathdup() are collected into a
new do_mkpath(). Then, based on do_mkpath(), strbuf_mkpath() is
implemented.

Signed-off-by: Hui Yiqun <redacted>
---
 cache.h |  2 ++
 path.c  | 21 +++++++++++++++++----
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/cache.h b/cache.h
index b829410..ef843c1 100644
--- a/cache.h
+++ b/cache.h

@@ -781,6 +781,8 @@ extern char *git_pathdup(const char *fmt, ...)
 	__attribute__((format (printf, 1, 2)));
 extern char *mkpathdup(const char *fmt, ...)
 	__attribute__((format (printf, 1, 2)));
+extern void strbuf_mkpath(struct strbuf *sb, const char *fmt, ...)
+	__attribute__((format (printf, 2, 3)));
 extern char *git_pathdup_submodule(const char *path, const char *fmt, ...)
 	__attribute__((format (printf, 2, 3)));
 

diff --git a/path.c b/path.c
index 8b7e168..699af68 100644
--- a/path.c
+++ b/path.c

@@ -433,14 +433,19 @@ char *git_pathdup(const char *fmt, ...)
 	return strbuf_detach(&path, NULL);
 }
 
+static void do_mkpath(struct strbuf *buf, const char *fmt, va_list args)
+{
+	strbuf_vaddf(buf, fmt, args);
+	strbuf_cleanup_path(buf);
+}
+
 char *mkpathdup(const char *fmt, ...)
 {
 	struct strbuf sb = STRBUF_INIT;
 	va_list args;
 	va_start(args, fmt);
-	strbuf_vaddf(&sb, fmt, args);
+	do_mkpath(&sb, fmt, args);
 	va_end(args);
-	strbuf_cleanup_path(&sb);
 	return strbuf_detach(&sb, NULL);
 }
 

@@ -449,9 +454,17 @@ const char *mkpath(const char *fmt, ...)
 	va_list args;
 	struct strbuf *pathname = get_pathname();
 	va_start(args, fmt);
-	strbuf_vaddf(pathname, fmt, args);
+	do_mkpath(pathname, fmt, args);
+	va_end(args);
+	return pathname->buf;
+}
+
+void strbuf_mkpath(struct strbuf *buf, const char *fmt, ...)
+{
+	va_list args;
+	va_start(args, fmt);
+	do_mkpath(buf, fmt, args);
 	va_end(args);
-	return cleanup_path(pathname->buf);
 }
 
 static void do_submodule_path(struct strbuf *buf, const char *path,

-- 
2.7.3

this function does the following:

1. if $XDG_RUNTIME_DIR is non-empty, `$XDG_RUNTIME_DIR/git` is used in next
step, otherwise `/tmp/git-$uid` is taken.
2. ensure that above directory does exist. what's more, it must has correct
permission and ownership.
3. a newly allocated string consisting of the path of above directory and
$filename is returned.

Under following situation, NULL will be returned:
+ the directory mentioned in step 1 exists but have wrong permission or
ownership.
+ the directory or its parent cannot be created.

Notice:

+ the caller is responsible for deallocating the returned string.

Signed-off-by: Hui Yiqun <redacted>
---
 cache.h | 23 +++++++++++++++++++++++
 path.c  | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 79 insertions(+)

diff --git a/cache.h b/cache.h
index ef843c1..f8b649b 100644
--- a/cache.h
+++ b/cache.h

@@ -1001,6 +1001,29 @@ extern int is_ntfs_dotgit(const char *name);
  */
 extern char *xdg_config_home(const char *filename);
 
+/**
+ * this function does the following:
+ *
+ * 1. if $XDG_RUNTIME_DIR is non-empty, `$XDG_RUNTIME_DIR/git` is used in next
+ * step, otherwise `/tmp/git-$uid` is taken.
+ * 2. ensure that above directory does exist. what's more, it must has correct
+ * permission and ownership.
+ * 3. a newly allocated string consisting of the path of above directory and
+ * $filename is returned.
+ *
+ * Under following situation, NULL will be returned:
+ *
+ * + the directory mentioned in step 1 exists but have wrong permission or
+ * ownership.
+ * + the directory or its parent cannot be created.
+ *
+ * Notice:
+ *
+ * + the caller is responsible for deallocating the returned string.
+ *
+ */
+extern char *xdg_runtime_dir(const char *filename);
+
 /* object replacement */
 #define LOOKUP_REPLACE_OBJECT 1
 #define LOOKUP_UNKNOWN_OBJECT 2

diff --git a/path.c b/path.c
index 699af68..2886e59 100644
--- a/path.c
+++ b/path.c

@@ -5,6 +5,7 @@
 #include "strbuf.h"
 #include "string-list.h"
 #include "dir.h"
+#include "git-compat-util.h"
 
 static int get_st_mode_bits(const char *path, int *mode)
 {

@@ -1206,6 +1207,61 @@ char *xdg_config_home(const char *filename)
 	return NULL;
 }
 
+char *xdg_runtime_dir(const char *filename)
+{
+	struct strbuf sb = STRBUF_INIT;
+	char *runtime_dir;
+	struct stat st;
+	uid_t uid = getuid();
+
+	assert(filename);
+	runtime_dir = getenv("XDG_RUNTIME_DIR");
+	if (runtime_dir && *runtime_dir)
+		strbuf_mkpath(&sb, "%s/git/", runtime_dir);
+	else
+		strbuf_mkpath(&sb, "/tmp/git-%d", uid);
+
+	if (!lstat(sb.buf, &st)) {
+		/*
+		 * As described in XDG base dir spec[1], the subdirectory
+		 * under $XDG_RUNTIME_DIR or its fallback MUST be owned by
+		 * the user, and its unix access mode MUST be 0700.
+		 *
+		 * Calling chmod or chown silently may cause security
+		 * problem if somebody chdir to it, sleep, and then, try
+		 * to open our protected runtime cache or socket.
+		 * So we just put warning and left it to user to solve.
+		 *
+		 * [1]https://specifications.freedesktop.org/basedir-spec/
+		 * basedir-spec-latest.html
+		 */
+		if ((st.st_mode & 0777) != S_IRWXU) {
+			warning("permission of runtime directory '%s' "
+					"MUST be 0700 instead of 0%o\n",
+					sb.buf, (st.st_mode & 0777));
+			return NULL;
+		} else if (st.st_uid != uid) {
+			warning("owner of runtime directory '%s' "
+					"MUST be %d instead of %d\n",
+					sb.buf, uid, st.st_uid);
+			return NULL;
+		}
+		/* TODO: check whether st.buf is an directory */
+	} else {
+		if (safe_create_leading_directories_const(sb.buf) < 0) {
+			warning("unable to create directories for '%s'\n",
+					sb.buf);
+			return NULL;
+		}
+		if (mkdir(sb.buf, 0700) < 0) {
+			warning("unable to mkdir '%s'\n", sb.buf);
+			return NULL;
+		}
+	}
+	strbuf_addf(&sb, "/%s", filename);
+	return strbuf_detach(&sb, 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.7.3

move .git-credential-cache/socket to xdg_runtime_dir("credential-cache.sock")

Signed-off-by: Hui Yiqun <redacted>
---
 credential-cache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/credential-cache.c b/credential-cache.c
index f4afdc6..40d838b 100644
--- a/credential-cache.c
+++ b/credential-cache.c

@@ -105,7 +105,7 @@ int main(int argc, const char **argv)
 	op = argv[0];
 
 	if (!socket_path)
-		socket_path = expand_user_path("~/.git-credential-cache/socket");
+		socket_path = xdg_runtime_dir("credential-cache.sock");
 	if (!socket_path)
 		die("unable to find a suitable socket path; use --socket");
 

-- 
2.7.3

t0301 now tests git-credential-cache support for XDG user-specific
runtime file $XDG_RUNTIME_DIR/git/credential.sock. Specifically:

* if $XDG_RUNTIME_DIR exists, use socket at
  `$XDG_RUNTIME_DIR/git/credential-cache.sock`.

* otherwise, `/tmp/git-$uid/credential-cache.sock` is taken.

Signed-off-by: Hui Yiqun <redacted>
---
 t/t0301-credential-cache.sh | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh
index 82c8411..892d1ba 100755
--- a/t/t0301-credential-cache.sh
+++ b/t/t0301-credential-cache.sh

@@ -12,7 +12,34 @@ test -z "$NO_UNIX_SOCKETS" || {
 # don't leave a stale daemon running
 trap 'code=$?; git credential-cache exit; (exit $code); die' EXIT
 
+test_expect_success 'set $XDG_RUNTIME_DIR' '
+	XDG_RUNTIME_DIR=$HOME/xdg_runtime/
+'
+
+helper_test cache
+
+test_expect_success 'when $XDG_RUNTIME_DIR is set, `$XDG_RUNTIME_DIR/git` are used' '
+	test_path_is_missing "/tmp/git-$(id -u)/git/credential-cache.sock" &&
+	test -S "$HOME/xdg_runtime/git/credential-cache.sock"
+'
+
+test_expect_success 'force git-credential-cache to exit so that socket disappear' '
+	git credential-cache exit &&
+	test_path_is_missing "$XDG_RUNTIME_DIR/git/credential-cache.sock" &&
+	unset XDG_RUNTIME_DIR
+'
+
 helper_test cache
+
+test_expect_success 'when $XDG_RUNTIME_DIR is not set, `/tmp/git-$(id -u) is used' '
+	test -S "/tmp/git-$(id -u)/credential-cache.sock"
+'
+
+# TODO: if $XDG_RUNTIME_DIR/git/ exists, but has wrong permission and ownership,
+# `helper_test cache` must fail.
+
+# TODO: check whether `--socket` works
+
 helper_test_timeout cache --timeout=1
 
 # we can't rely on our "trap" above working after test_done,

-- 
2.7.3

On Fri, Mar 18, 2016 at 12:48:46AM +0800, Hui Yiqun wrote:

This test fails for me, probably because XDG_RUNTIME_DIR is not
exported.

-Peff

On Fri, Mar 18, 2016 at 12:48:44AM +0800, Hui Yiqun wrote:

this function does the following:

1. if $XDG_RUNTIME_DIR is non-empty, `$XDG_RUNTIME_DIR/git` is used in next
step, otherwise `/tmp/git-$uid` is taken.
2. ensure that above directory does exist. what's more, it must has correct
permission and ownership.
3. a newly allocated string consisting of the path of above directory and
$filename is returned.

Under following situation, NULL will be returned:
+ the directory mentioned in step 1 exists but have wrong permission or
ownership.
+ the directory or its parent cannot be created.

Notice:

+ the caller is responsible for deallocating the returned string.

I see a lot of "what" in your commit message (and in the other ones in
this series), but not a lot of "why".

We can see the "what" from the diff already (though it is certainly OK
to point out tricky parts). But you probably want to explain the
motivation for things, alternatives considered, etc, like:

  - why is using $XDG_RUNTIME_DIR a good thing?

  - why did you choose to fall back to /tmp (as opposed to, say,
    returning NULL and letting the caller handle it)

  - what is the purpose of the ownership/permission rules? You link to
    the XDG spec in an in-code comment, but IMHO that kind of motivation
    probably makes more sense in the commit message.

-Peff

2016-03-18 2:08 GMT+08:00 Jeff King [off-list ref]:

On Fri, Mar 18, 2016 at 12:48:46AM +0800, Hui Yiqun wrote:

quoted

t0301 now tests git-credential-cache support for XDG user-specific
runtime file $XDG_RUNTIME_DIR/git/credential.sock. Specifically:

* if $XDG_RUNTIME_DIR exists, use socket at
  `$XDG_RUNTIME_DIR/git/credential-cache.sock`.

* otherwise, `/tmp/git-$uid/credential-cache.sock` is taken.

Signed-off-by: Hui Yiqun <redacted>
---
 t/t0301-credential-cache.sh | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh
index 82c8411..892d1ba 100755
--- a/t/t0301-credential-cache.sh
+++ b/t/t0301-credential-cache.sh

@@ -12,7 +12,34 @@ test -z "$NO_UNIX_SOCKETS" || {
 # don't leave a stale daemon running
 trap 'code=$?; git credential-cache exit; (exit $code); die' EXIT

+test_expect_success 'set $XDG_RUNTIME_DIR' '
+     XDG_RUNTIME_DIR=$HOME/xdg_runtime/
+'
+
+helper_test cache
+
+test_expect_success 'when $XDG_RUNTIME_DIR is set, `$XDG_RUNTIME_DIR/git` are used' '
+     test_path_is_missing "/tmp/git-$(id -u)/git/credential-cache.sock" &&
+     test -S "$HOME/xdg_runtime/git/credential-cache.sock"
+'

This test fails for me, probably because XDG_RUNTIME_DIR is not
exported.

-Peff

Could you please give a try to the patch set v2, test of which is
definitely passed on my computer.

On Fri, Mar 18, 2016 at 12:38:16PM +0800, 惠轶群 wrote:

quoted

quoted

+test_expect_success 'when $XDG_RUNTIME_DIR is set, `$XDG_RUNTIME_DIR/git` are used' '
+     test_path_is_missing "/tmp/git-$(id -u)/git/credential-cache.sock" &&
+     test -S "$HOME/xdg_runtime/git/credential-cache.sock"
+'

This test fails for me, probably because XDG_RUNTIME_DIR is not
exported.

-Peff

Could you please give a try to the patch set v2, test of which is
definitely passed on my computer.

Yes, that was what I tried earlier (and I just re-tested to make sure).
It still fails. I suspect it has to do with whether XDG_RUNTIME_DIR is
set in our environments (outside of the test suite). If I run:

  XDG_RUNTIME_DIR=/tmp/foo ./t0301-credential-cache.sh

it passes.

-Peff

2016-03-18 13:01 GMT+08:00 Jeff King [off-list ref]:

On Fri, Mar 18, 2016 at 12:38:16PM +0800, 惠轶群 wrote:

quoted

quoted

quoted

+test_expect_success 'when $XDG_RUNTIME_DIR is set, `$XDG_RUNTIME_DIR/git` are used' '
+     test_path_is_missing "/tmp/git-$(id -u)/git/credential-cache.sock" &&
+     test -S "$HOME/xdg_runtime/git/credential-cache.sock"
+'

This test fails for me, probably because XDG_RUNTIME_DIR is not
exported.

-Peff

Could you please give a try to the patch set v2, test of which is
definitely passed on my computer.

Yes, that was what I tried earlier (and I just re-tested to make sure).
It still fails. I suspect it has to do with whether XDG_RUNTIME_DIR is
set in our environments (outside of the test suite). If I run:

  XDG_RUNTIME_DIR=/tmp/foo ./t0301-credential-cache.sh

it passes.

-Peff

Sorry, I wrongly considered your comment is for my last commit. That
was partly because my thread is
disturbed by my commits in my mailbox(gmail). I hope that did not also
disturb yours.

I sent my commits with `--thread` and `--no-chain-reply-to` in hope
that I would send an summary as
[PATCH 0/x] and all my commits would be sent in reply to it. But I failed.