threads / patch / 41727

v2, 4 partspath.c: implement strbuf_mkpath()

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

## tl;dr

9 messages between Mar 17, 2016 and Mar 18, 2016. Diffs are folded; open one to read it.

replies: 8people: 2as markdown or json

Hui Yiqun· Mar 17, 2016, 16:48 UTC · lore

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 <huiyiqun@gmail.com>
---
 cache.h |  2 ++
 path.c  | 21 +++++++++++++++++----
 2 files changed, 19 insertions(+), 4 deletions(-)
Show changes to 2 files +19 −4

cache.h, path.c

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
Hui Yiqun· Mar 17, 2016, 16:48 UTC · re: Hui Yiqun · lore

[PATCH v2/GSoC 2/4] path.c: implement xdg_runtime_dir()

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 <huiyiqun@gmail.com>
---
 cache.h | 23 +++++++++++++++++++++++
 path.c  | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 79 insertions(+)
Show changes to 2 files +79 −0

cache.h, path.c

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
Jeff King· Mar 17, 2016, 18:13 UTC · re: Hui Yiqun · lore

Re: [PATCH v2/GSoC 2/4] path.c: implement xdg_runtime_dir()

On Fri, Mar 18, 2016 at 12:48:44AM +0800, Hui Yiqun wrote:
Show 17 quoted lines
> 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
Hui Yiqun· Mar 17, 2016, 16:48 UTC · re: Hui Yiqun · lore

[PATCH v2/GSoC 3/4] git-credential-cache: put socket to xdg-compatible path

move .git-credential-cache/socket to xdg_runtime_dir("credential-cache.sock")
Signed-off-by: Hui Yiqun <huiyiqun@gmail.com>
---
 credential-cache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to credential-cache.c +1 −1
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
Hui Yiqun· Mar 17, 2016, 16:48 UTC · re: Hui Yiqun · lore

[PATCH v2/GSoC 4/4] t0301: test credential-cache support of XDG_RUNTIME_DIR

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 <huiyiqun@gmail.com>
---
 t/t0301-credential-cache.sh | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)
Show changes to t/t0301-credential-cache.sh +27 −0
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
Jeff King· Mar 17, 2016, 18:08 UTC · re: Hui Yiqun · lore

Re: [PATCH v2/GSoC 4/4] t0301: test credential-cache support of XDG_RUNTIME_DIR

On Fri, Mar 18, 2016 at 12:48:46AM +0800, Hui Yiqun wrote:
Show 31 quoted lines
> 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 <huiyiqun@gmail.com>
> ---
>  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
惠轶群· Mar 18, 2016, 04:38 UTC · re: Jeff King · lore

Re: [PATCH v2/GSoC 4/4] t0301: test credential-cache support of XDG_RUNTIME_DIR

2016-03-18 2:08 GMT+08:00 Jeff King <peff@peff.net>:
Show 38 quoted lines
> On Fri, Mar 18, 2016 at 12:48:46AM +0800, Hui Yiqun wrote:
>
>> 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 <huiyiqun@gmail.com>
>> ---
>>  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.

Jeff King· Mar 18, 2016, 05:01 UTC · re: 惠轶群 · lore

Re: [PATCH v2/GSoC 4/4] t0301: test credential-cache support of XDG_RUNTIME_DIR

On Fri, Mar 18, 2016 at 12:38:16PM +0800, 惠轶群 wrote:
Show 12 quoted lines
> >> +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
惠轶群· Mar 18, 2016, 05:22 UTC · re: Jeff King · lore

Re: [PATCH v2/GSoC 4/4] t0301: test credential-cache support of XDG_RUNTIME_DIR

2016-03-18 13:01 GMT+08:00 Jeff King <peff@peff.net>:
Show 24 quoted lines
> On Fri, Mar 18, 2016 at 12:38:16PM +0800, 惠轶群 wrote:
>
>> >> +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.

← back to recent threads