[PATCH v3 0/10] getcwd without PATH_MAX

[PATCH v3 0/10] getcwd without PATH_MAX

[René Scharfe]

Paths longer than PATH_MAX can be created and used on at least on some
file systems.  Currently we use getcwd() generally with a PATH_MAX-
sized buffer.  This series adds two functions, strbuf_getcwd() and
xgetcwd(), then uses them to reduce the number of fixed-sized buffers
and to allow us to handle longer working directory paths.

Changes in v3:
  * all getcwd() calls are converted
  * the two strbuf_getcwd() examples from last round use xgetcwd()
    now, as suggested by Jeff
  * strbuf_add_absolute_path() is introduced

René Scharfe (10):
  strbuf: add strbuf_getcwd()
  unix-sockets: use strbuf_getcwd()
  setup: convert setup_git_directory_gently_1 et al. to strbuf
  abspath: use strbuf_getcwd() to remember original working directory
  abspath: convert real_path_internal() to strbuf
  wrapper: add xgetcwd()
  use xgetcwd() to get the current directory or die
  use xgetcwd() to set $GIT_DIR
  abspath: convert absolute_path() to strbuf
  use strbuf_add_absolute_path() to add absolute paths

 Documentation/technical/api-strbuf.txt |  10 +++
 abspath.c                              | 124 +++++++++------------------------
 builtin/init-db.c                      |  24 +++----
 builtin/rev-parse.c                    |   6 +-
 dir.c                                  |  12 ++--
 exec_cmd.c                             |   6 +-
 git-compat-util.h                      |   1 +
 git.c                                  |  13 ++--
 setup.c                                |  91 ++++++++++++------------
 sha1_file.c                            |   2 +-
 strbuf.c                               |  46 ++++++++++++
 strbuf.h                               |   3 +
 trace.c                                |   7 +-
 unix-socket.c                          |  14 ++--
 wrapper.c                              |   8 +++
 15 files changed, 190 insertions(+), 177 deletions(-)

-- 
2.0.2

[PATCH v3 01/10] strbuf: add strbuf_getcwd()

[René Scharfe]

Add strbuf_getcwd(), which puts the current working directory into a
strbuf.  Because it doesn't use a fixed-size buffer it supports
arbitrarily long paths, provided the platform's getcwd() does as well.
At least on Linux and FreeBSD it handles paths longer than PATH_MAX
just fine.

Suggested-by: Karsten Blees <karsten.blees@gmail.com>
Helped-by: Duy Nguyen <pclouds@gmail.com>
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 Documentation/technical/api-strbuf.txt |  4 ++++
 strbuf.c                               | 21 +++++++++++++++++++++
 strbuf.h                               |  1 +
 3 files changed, 26 insertions(+)

diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt
index f9c06a7..49e477d 100644
--- a/Documentation/technical/api-strbuf.txt
+++ b/Documentation/technical/api-strbuf.txt
@@ -307,6 +307,10 @@ same behaviour as well.
 	use it unless you need the correct position in the file
 	descriptor.
 
+`strbuf_getcwd`::
+
+	Set the buffer to the path of the current working directory.
+
 `stripspace`::
 
 	Strip whitespace from a buffer. The second parameter controls if
diff --git a/strbuf.c b/strbuf.c
index 33018d8..2bf4dfa 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -406,6 +406,27 @@ int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)
 	return -1;
 }
 
+int strbuf_getcwd(struct strbuf *sb)
+{
+	size_t oldalloc = sb->alloc;
+	size_t guessed_len = 128;
+
+	for (;; guessed_len *= 2) {
+		strbuf_grow(sb, guessed_len);
+		if (getcwd(sb->buf, sb->alloc)) {
+			strbuf_setlen(sb, strlen(sb->buf));
+			return 0;
+		}
+		if (errno != ERANGE)
+			break;
+	}
+	if (oldalloc == 0)
+		strbuf_release(sb);
+	else
+		strbuf_reset(sb);
+	return -1;
+}
+
 int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)
 {
 	int ch;
diff --git a/strbuf.h b/strbuf.h
index a7c0192..bc38bb9 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -174,6 +174,7 @@ extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);
 extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint);
 extern int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint);
 extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);
+extern int strbuf_getcwd(struct strbuf *sb);
 
 extern int strbuf_getwholeline(struct strbuf *, FILE *, int);
 extern int strbuf_getline(struct strbuf *, FILE *, int);
-- 
2.0.2

[PATCH v3 02/10] unix-sockets: use strbuf_getcwd()

[René Scharfe]

Instead of using a PATH_MAX-sized buffer, which can be too small on some
file systems, use strbuf_getcwd(), which handles any path getcwd()
returns.  Also preserve the errno set by strbuf_getcwd() instead of
setting it to ENAMETOOLONG; that way a more appropriate error message
can be shown based on the actual reason for failing.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 unix-socket.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/unix-socket.c b/unix-socket.c
index 91bd6b8..19ed48b 100644
--- a/unix-socket.c
+++ b/unix-socket.c
@@ -18,12 +18,12 @@ static int chdir_len(const char *orig, int len)
 }
 
 struct unix_sockaddr_context {
-	char orig_dir[PATH_MAX];
+	char *orig_dir;
 };
 
 static void unix_sockaddr_cleanup(struct unix_sockaddr_context *ctx)
 {
-	if (!ctx->orig_dir[0])
+	if (!ctx->orig_dir)
 		return;
 	/*
 	 * If we fail, we can't just return an error, since we have
@@ -32,6 +32,7 @@ static void unix_sockaddr_cleanup(struct unix_sockaddr_context *ctx)
 	 */
 	if (chdir(ctx->orig_dir) < 0)
 		die("unable to restore original working directory");
+	free(ctx->orig_dir);
 }
 
 static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path,
@@ -39,10 +40,11 @@ static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path,
 {
 	int size = strlen(path) + 1;
 
-	ctx->orig_dir[0] = '\0';
+	ctx->orig_dir = NULL;
 	if (size > sizeof(sa->sun_path)) {
 		const char *slash = find_last_dir_sep(path);
 		const char *dir;
+		struct strbuf cwd = STRBUF_INIT;
 
 		if (!slash) {
 			errno = ENAMETOOLONG;
@@ -56,11 +58,9 @@ static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path,
 			errno = ENAMETOOLONG;
 			return -1;
 		}
-
-		if (!getcwd(ctx->orig_dir, sizeof(ctx->orig_dir))) {
-			errno = ENAMETOOLONG;
+		if (strbuf_getcwd(&cwd))
 			return -1;
-		}
+		ctx->orig_dir = strbuf_detach(&cwd, NULL);
 		if (chdir_len(dir, slash - dir) < 0)
 			return -1;
 	}
-- 
2.0.2

[PATCH v3 03/10] setup: convert setup_git_directory_gently_1 et al. to strbuf

[René Scharfe]

Convert setup_git_directory_gently_1() and its helper functions
setup_explicit_git_dir(), setup_discovered_git_dir() and
setup_bare_git_dir() to use a struct strbuf to hold the current working
directory.  Replacing the PATH_MAX-sized buffer used before removes a
path length limition on some file systems.  The functions are converted
all in one go because they all read and write the variable cwd.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 setup.c | 85 +++++++++++++++++++++++++++++++++--------------------------------
 1 file changed, 43 insertions(+), 42 deletions(-)

diff --git a/setup.c b/setup.c
index 0a22f8b..c8b8a97 100644
--- a/setup.c
+++ b/setup.c
@@ -387,7 +387,7 @@ const char *read_gitfile(const char *path)
 }
 
 static const char *setup_explicit_git_dir(const char *gitdirenv,
-					  char *cwd, int len,
+					  struct strbuf *cwd,
 					  int *nongit_ok)
 {
 	const char *work_tree_env = getenv(GIT_WORK_TREE_ENVIRONMENT);
@@ -441,7 +441,7 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,
 				die_errno("Could not chdir to '%s'", git_work_tree_cfg);
 			if (!getcwd(core_worktree, PATH_MAX))
 				die_errno("Could not get directory '%s'", git_work_tree_cfg);
-			if (chdir(cwd))
+			if (chdir(cwd->buf))
 				die_errno("Could not come back to cwd");
 			set_git_work_tree(core_worktree);
 		}
@@ -459,21 +459,20 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,
 	worktree = get_git_work_tree();
 
 	/* both get_git_work_tree() and cwd are already normalized */
-	if (!strcmp(cwd, worktree)) { /* cwd == worktree */
+	if (!strcmp(cwd->buf, worktree)) { /* cwd == worktree */
 		set_git_dir(gitdirenv);
 		free(gitfile);
 		return NULL;
 	}
 
-	offset = dir_inside_of(cwd, worktree);
+	offset = dir_inside_of(cwd->buf, worktree);
 	if (offset >= 0) {	/* cwd inside worktree? */
 		set_git_dir(real_path(gitdirenv));
 		if (chdir(worktree))
 			die_errno("Could not chdir to '%s'", worktree);
-		cwd[len++] = '/';
-		cwd[len] = '\0';
+		strbuf_addch(cwd, '/');
 		free(gitfile);
-		return cwd + offset;
+		return cwd->buf + offset;
 	}
 
 	/* cwd outside worktree */
@@ -483,7 +482,7 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,
 }
 
 static const char *setup_discovered_git_dir(const char *gitdir,
-					    char *cwd, int offset, int len,
+					    struct strbuf *cwd, int offset,
 					    int *nongit_ok)
 {
 	if (check_repository_format_gently(gitdir, nongit_ok))
@@ -491,17 +490,17 @@ static const char *setup_discovered_git_dir(const char *gitdir,
 
 	/* --work-tree is set without --git-dir; use discovered one */
 	if (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) {
-		if (offset != len && !is_absolute_path(gitdir))
+		if (offset != cwd->len && !is_absolute_path(gitdir))
 			gitdir = xstrdup(real_path(gitdir));
-		if (chdir(cwd))
+		if (chdir(cwd->buf))
 			die_errno("Could not come back to cwd");
-		return setup_explicit_git_dir(gitdir, cwd, len, nongit_ok);
+		return setup_explicit_git_dir(gitdir, cwd, nongit_ok);
 	}
 
 	/* #16.2, #17.2, #20.2, #21.2, #24, #25, #28, #29 (see t1510) */
 	if (is_bare_repository_cfg > 0) {
-		set_git_dir(offset == len ? gitdir : real_path(gitdir));
-		if (chdir(cwd))
+		set_git_dir(offset == cwd->len ? gitdir : real_path(gitdir));
+		if (chdir(cwd->buf))
 			die_errno("Could not come back to cwd");
 		return NULL;
 	}
@@ -512,18 +511,18 @@ static const char *setup_discovered_git_dir(const char *gitdir,
 		set_git_dir(gitdir);
 	inside_git_dir = 0;
 	inside_work_tree = 1;
-	if (offset == len)
+	if (offset == cwd->len)
 		return NULL;
 
 	/* Make "offset" point to past the '/', and add a '/' at the end */
 	offset++;
-	cwd[len++] = '/';
-	cwd[len] = 0;
-	return cwd + offset;
+	strbuf_addch(cwd, '/');
+	return cwd->buf + offset;
 }
 
 /* #16.1, #17.1, #20.1, #21.1, #22.1 (see t1510) */
-static const char *setup_bare_git_dir(char *cwd, int offset, int len, int *nongit_ok)
+static const char *setup_bare_git_dir(struct strbuf *cwd, int offset,
+				      int *nongit_ok)
 {
 	int root_len;
 
@@ -536,20 +535,20 @@ static const char *setup_bare_git_dir(char *cwd, int offset, int len, int *nongi
 	if (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) {
 		const char *gitdir;
 
-		gitdir = offset == len ? "." : xmemdupz(cwd, offset);
-		if (chdir(cwd))
+		gitdir = offset == cwd->len ? "." : xmemdupz(cwd->buf, offset);
+		if (chdir(cwd->buf))
 			die_errno("Could not come back to cwd");
-		return setup_explicit_git_dir(gitdir, cwd, len, nongit_ok);
+		return setup_explicit_git_dir(gitdir, cwd, nongit_ok);
 	}
 
 	inside_git_dir = 1;
 	inside_work_tree = 0;
-	if (offset != len) {
-		if (chdir(cwd))
+	if (offset != cwd->len) {
+		if (chdir(cwd->buf))
 			die_errno("Cannot come back to cwd");
-		root_len = offset_1st_component(cwd);
-		cwd[offset > root_len ? offset : root_len] = '\0';
-		set_git_dir(cwd);
+		root_len = offset_1st_component(cwd->buf);
+		strbuf_setlen(cwd, offset > root_len ? offset : root_len);
+		set_git_dir(cwd->buf);
 	}
 	else
 		set_git_dir(".");
@@ -617,10 +616,10 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
 {
 	const char *env_ceiling_dirs = getenv(CEILING_DIRECTORIES_ENVIRONMENT);
 	struct string_list ceiling_dirs = STRING_LIST_INIT_DUP;
-	static char cwd[PATH_MAX + 1];
+	static struct strbuf cwd = STRBUF_INIT;
 	const char *gitdirenv, *ret;
 	char *gitfile;
-	int len, offset, offset_parent, ceil_offset = -1;
+	int offset, offset_parent, ceil_offset = -1;
 	dev_t current_device = 0;
 	int one_filesystem = 1;
 
@@ -632,9 +631,9 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
 	if (nongit_ok)
 		*nongit_ok = 0;
 
-	if (!getcwd(cwd, sizeof(cwd) - 1))
+	if (strbuf_getcwd(&cwd))
 		die_errno("Unable to read current working directory");
-	offset = len = strlen(cwd);
+	offset = cwd.len;
 
 	/*
 	 * If GIT_DIR is set explicitly, we're not going
@@ -643,7 +642,7 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
 	 */
 	gitdirenv = getenv(GIT_DIR_ENVIRONMENT);
 	if (gitdirenv)
-		return setup_explicit_git_dir(gitdirenv, cwd, len, nongit_ok);
+		return setup_explicit_git_dir(gitdirenv, &cwd, nongit_ok);
 
 	if (env_ceiling_dirs) {
 		int empty_entry_found = 0;
@@ -651,7 +650,7 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
 		string_list_split(&ceiling_dirs, env_ceiling_dirs, PATH_SEP, -1);
 		filter_string_list(&ceiling_dirs, 0,
 				   canonicalize_ceiling_entry, &empty_entry_found);
-		ceil_offset = longest_ancestor_length(cwd, &ceiling_dirs);
+		ceil_offset = longest_ancestor_length(cwd.buf, &ceiling_dirs);
 		string_list_clear(&ceiling_dirs, 0);
 	}
 
@@ -683,7 +682,7 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
 
 		if (gitdirenv) {
 			ret = setup_discovered_git_dir(gitdirenv,
-						       cwd, offset, len,
+						       &cwd, offset,
 						       nongit_ok);
 			free(gitfile);
 			return ret;
@@ -691,29 +690,31 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
 		free(gitfile);
 
 		if (is_git_directory("."))
-			return setup_bare_git_dir(cwd, offset, len, nongit_ok);
+			return setup_bare_git_dir(&cwd, offset, nongit_ok);
 
 		offset_parent = offset;
-		while (--offset_parent > ceil_offset && cwd[offset_parent] != '/');
+		while (--offset_parent > ceil_offset && cwd.buf[offset_parent] != '/');
 		if (offset_parent <= ceil_offset)
-			return setup_nongit(cwd, nongit_ok);
+			return setup_nongit(cwd.buf, nongit_ok);
 		if (one_filesystem) {
-			dev_t parent_device = get_device_or_die("..", cwd, offset);
+			dev_t parent_device = get_device_or_die("..", cwd.buf,
+								offset);
 			if (parent_device != current_device) {
 				if (nongit_ok) {
-					if (chdir(cwd))
+					if (chdir(cwd.buf))
 						die_errno("Cannot come back to cwd");
 					*nongit_ok = 1;
 					return NULL;
 				}
-				cwd[offset] = '\0';
+				strbuf_setlen(&cwd, offset);
 				die("Not a git repository (or any parent up to mount point %s)\n"
-				"Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).", cwd);
+				"Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).",
+				    cwd.buf);
 			}
 		}
 		if (chdir("..")) {
-			cwd[offset] = '\0';
-			die_errno("Cannot change to '%s/..'", cwd);
+			strbuf_setlen(&cwd, offset);
+			die_errno("Cannot change to '%s/..'", cwd.buf);
 		}
 		offset = offset_parent;
 	}
-- 
2.0.2

[PATCH 04/10] abspath: use strbuf_getcwd() to remember original working directory

[René Scharfe]

Store the original working directory in a strbuf instead of in a
fixed-sized buffer, in order to be able to handle longer paths.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 abspath.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/abspath.c b/abspath.c
index ca33558..911e931 100644
--- a/abspath.c
+++ b/abspath.c
@@ -41,7 +41,7 @@ static const char *real_path_internal(const char *path, int die_on_error)
 	 * here so that we can chdir() back to it at the end of the
 	 * function:
 	 */
-	char cwd[1024] = "";
+	struct strbuf cwd = STRBUF_INIT;
 
 	int buf_index = 1;
 
@@ -80,7 +80,7 @@ static const char *real_path_internal(const char *path, int die_on_error)
 		}
 
 		if (*buf) {
-			if (!*cwd && !getcwd(cwd, sizeof(cwd))) {
+			if (!cwd.len && strbuf_getcwd(&cwd)) {
 				if (die_on_error)
 					die_errno("Could not get current working directory");
 				else
@@ -142,8 +142,9 @@ static const char *real_path_internal(const char *path, int die_on_error)
 	retval = buf;
 error_out:
 	free(last_elem);
-	if (*cwd && chdir(cwd))
-		die_errno("Could not change back to '%s'", cwd);
+	if (cwd.len && chdir(cwd.buf))
+		die_errno("Could not change back to '%s'", cwd.buf);
+	strbuf_release(&cwd);
 
 	return retval;
 }
-- 
2.0.2

[PATCH v3 05/10] abspath: convert real_path_internal() to strbuf

[René Scharfe]

Use strbuf instead of fixed-sized buffers in real_path() in order to
avoid the size limitations of the latter.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 abspath.c | 69 +++++++++++++++++++++++----------------------------------------
 1 file changed, 25 insertions(+), 44 deletions(-)

diff --git a/abspath.c b/abspath.c
index 911e931..16e7fa2 100644
--- a/abspath.c
+++ b/abspath.c
@@ -33,7 +33,7 @@ int is_directory(const char *path)
  */
 static const char *real_path_internal(const char *path, int die_on_error)
 {
-	static char bufs[2][PATH_MAX + 1], *buf = bufs[0], *next_buf = bufs[1];
+	static struct strbuf sb = STRBUF_INIT;
 	char *retval = NULL;
 
 	/*
@@ -43,14 +43,12 @@ static const char *real_path_internal(const char *path, int die_on_error)
 	 */
 	struct strbuf cwd = STRBUF_INIT;
 
-	int buf_index = 1;
-
 	int depth = MAXDEPTH;
 	char *last_elem = NULL;
 	struct stat st;
 
 	/* We've already done it */
-	if (path == buf || path == next_buf)
+	if (path == sb.buf)
 		return path;
 
 	if (!*path) {
@@ -60,26 +58,22 @@ static const char *real_path_internal(const char *path, int die_on_error)
 			goto error_out;
 	}
 
-	if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX) {
-		if (die_on_error)
-			die("Too long path: %.*s", 60, path);
-		else
-			goto error_out;
-	}
+	strbuf_init(&sb, 0);
+	strbuf_addstr(&sb, path);
 
 	while (depth--) {
-		if (!is_directory(buf)) {
-			char *last_slash = find_last_dir_sep(buf);
+		if (!is_directory(sb.buf)) {
+			char *last_slash = find_last_dir_sep(sb.buf);
 			if (last_slash) {
 				last_elem = xstrdup(last_slash + 1);
-				last_slash[1] = '\0';
+				strbuf_setlen(&sb, last_slash - sb.buf + 1);
 			} else {
-				last_elem = xstrdup(buf);
-				*buf = '\0';
+				last_elem = xmemdupz(sb.buf, sb.len);
+				strbuf_reset(&sb);
 			}
 		}
 
-		if (*buf) {
+		if (sb.len) {
 			if (!cwd.len && strbuf_getcwd(&cwd)) {
 				if (die_on_error)
 					die_errno("Could not get current working directory");
@@ -87,14 +81,15 @@ static const char *real_path_internal(const char *path, int die_on_error)
 					goto error_out;
 			}
 
-			if (chdir(buf)) {
+			if (chdir(sb.buf)) {
 				if (die_on_error)
-					die_errno("Could not switch to '%s'", buf);
+					die_errno("Could not switch to '%s'",
+						  sb.buf);
 				else
 					goto error_out;
 			}
 		}
-		if (!getcwd(buf, PATH_MAX)) {
+		if (strbuf_getcwd(&sb)) {
 			if (die_on_error)
 				die_errno("Could not get current working directory");
 			else
@@ -102,44 +97,30 @@ static const char *real_path_internal(const char *path, int die_on_error)
 		}
 
 		if (last_elem) {
-			size_t len = strlen(buf);
-			if (len + strlen(last_elem) + 2 > PATH_MAX) {
-				if (die_on_error)
-					die("Too long path name: '%s/%s'",
-					    buf, last_elem);
-				else
-					goto error_out;
-			}
-			if (len && !is_dir_sep(buf[len - 1]))
-				buf[len++] = '/';
-			strcpy(buf + len, last_elem);
+			if (sb.len && !is_dir_sep(sb.buf[sb.len - 1]))
+				strbuf_addch(&sb, '/');
+			strbuf_addstr(&sb, last_elem);
 			free(last_elem);
 			last_elem = NULL;
 		}
 
-		if (!lstat(buf, &st) && S_ISLNK(st.st_mode)) {
-			ssize_t len = readlink(buf, next_buf, PATH_MAX);
+		if (!lstat(sb.buf, &st) && S_ISLNK(st.st_mode)) {
+			struct strbuf next_sb = STRBUF_INIT;
+			ssize_t len = strbuf_readlink(&next_sb, sb.buf, 0);
 			if (len < 0) {
 				if (die_on_error)
-					die_errno("Invalid symlink '%s'", buf);
-				else
-					goto error_out;
-			}
-			if (PATH_MAX <= len) {
-				if (die_on_error)
-					die("symbolic link too long: %s", buf);
+					die_errno("Invalid symlink '%s'",
+						  sb.buf);
 				else
 					goto error_out;
 			}
-			next_buf[len] = '\0';
-			buf = next_buf;
-			buf_index = 1 - buf_index;
-			next_buf = bufs[buf_index];
+			strbuf_swap(&sb, &next_sb);
+			strbuf_release(&next_sb);
 		} else
 			break;
 	}
 
-	retval = buf;
+	retval = sb.buf;
 error_out:
 	free(last_elem);
 	if (cwd.len && chdir(cwd.buf))
-- 
2.0.2

[PATCH v3 06/10] wrapper: add xgetcwd()

[René Scharfe]

Add the helper function xgetcwd(), which returns the current directory
or dies.  The returned string has to be free()d after use.

Helped-by: Duy Nguyen <pclouds@gmail.com>
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 git-compat-util.h | 1 +
 wrapper.c         | 8 ++++++++
 2 files changed, 9 insertions(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index 26e92f1..4d6edea 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -607,6 +607,7 @@ extern int xmkstemp(char *template);
 extern int xmkstemp_mode(char *template, int mode);
 extern int odb_mkstemp(char *template, size_t limit, const char *pattern);
 extern int odb_pack_keep(char *name, size_t namesz, const unsigned char *sha1);
+extern char *xgetcwd(void);
 
 static inline size_t xsize_t(off_t len)
 {
diff --git a/wrapper.c b/wrapper.c
index bc1bfb8..bd24cda 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -493,3 +493,11 @@ struct passwd *xgetpwuid_self(void)
 		    errno ? strerror(errno) : _("no such user"));
 	return pw;
 }
+
+char *xgetcwd(void)
+{
+	struct strbuf sb = STRBUF_INIT;
+	if (strbuf_getcwd(&sb))
+		die_errno(_("unable to get current working directory"));
+	return strbuf_detach(&sb, NULL);
+}
-- 
2.0.2

[PATCH v3 07/10] use xgetcwd() to get the current directory or die

[René Scharfe]

Convert several calls of getcwd() and die() to use xgetcwd() instead.
This way we get rid of fixed-size buffers (which can be too small
depending on the used file system) and gain consistent error messages.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 builtin/init-db.c   | 17 ++++++++---------
 builtin/rev-parse.c |  6 +++---
 dir.c               | 12 ++++++++----
 git.c               |  8 ++++----
 setup.c             |  6 +++---
 trace.c             |  7 ++++---
 6 files changed, 30 insertions(+), 26 deletions(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index 56f85e2..f6dd172 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -426,8 +426,9 @@ int init_db(const char *template_dir, unsigned int flags)
 
 static int guess_repository_type(const char *git_dir)
 {
-	char cwd[PATH_MAX];
 	const char *slash;
+	char *cwd;
+	int cwd_is_git_dir;
 
 	/*
 	 * "GIT_DIR=. git init" is always bare.
@@ -435,9 +436,10 @@ static int guess_repository_type(const char *git_dir)
 	 */
 	if (!strcmp(".", git_dir))
 		return 1;
-	if (!getcwd(cwd, sizeof(cwd)))
-		die_errno(_("cannot tell cwd"));
-	if (!strcmp(git_dir, cwd))
+	cwd = xgetcwd();
+	cwd_is_git_dir = !strcmp(git_dir, cwd);
+	free(cwd);
+	if (cwd_is_git_dir)
 		return 1;
 	/*
 	 * "GIT_DIR=.git or GIT_DIR=something/.git is usually not.
@@ -572,11 +574,8 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 			git_work_tree_cfg = xstrdup(real_path(rel));
 			free(rel);
 		}
-		if (!git_work_tree_cfg) {
-			git_work_tree_cfg = xcalloc(PATH_MAX, 1);
-			if (!getcwd(git_work_tree_cfg, PATH_MAX))
-				die_errno (_("Cannot access current working directory"));
-		}
+		if (!git_work_tree_cfg)
+			git_work_tree_cfg = xgetcwd();
 		if (work_tree)
 			set_git_work_tree(real_path(work_tree));
 		else
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 8102aaa..f6bbac7 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -735,7 +735,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 			}
 			if (!strcmp(arg, "--git-dir")) {
 				const char *gitdir = getenv(GIT_DIR_ENVIRONMENT);
-				static char cwd[PATH_MAX];
+				char *cwd;
 				int len;
 				if (gitdir) {
 					puts(gitdir);
@@ -745,10 +745,10 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 					puts(".git");
 					continue;
 				}
-				if (!getcwd(cwd, PATH_MAX))
-					die_errno("unable to get current working directory");
+				cwd = xgetcwd();
 				len = strlen(cwd);
 				printf("%s%s.git\n", cwd, len && cwd[len-1] != '/' ? "/" : "");
+				free(cwd);
 				continue;
 			}
 			if (!strcmp(arg, "--resolve-git-dir")) {
diff --git a/dir.c b/dir.c
index fcb6872..bd274a7 100644
--- a/dir.c
+++ b/dir.c
@@ -1507,12 +1507,16 @@ int dir_inside_of(const char *subdir, const char *dir)
 
 int is_inside_dir(const char *dir)
 {
-	char cwd[PATH_MAX];
+	char *cwd;
+	int rc;
+
 	if (!dir)
 		return 0;
-	if (!getcwd(cwd, sizeof(cwd)))
-		die_errno("can't find the current directory");
-	return dir_inside_of(cwd, dir) >= 0;
+
+	cwd = xgetcwd();
+	rc = (dir_inside_of(cwd, dir) >= 0);
+	free(cwd);
+	return rc;
 }
 
 int is_empty_dir(const char *path)
diff --git a/git.c b/git.c
index 9c49519..47137db 100644
--- a/git.c
+++ b/git.c
@@ -20,7 +20,7 @@ const char git_more_info_string[] =
 
 static struct startup_info git_startup_info;
 static int use_pager = -1;
-static char orig_cwd[PATH_MAX];
+static char *orig_cwd;
 static const char *env_names[] = {
 	GIT_DIR_ENVIRONMENT,
 	GIT_WORK_TREE_ENVIRONMENT,
@@ -36,8 +36,7 @@ static void save_env(void)
 	if (saved_environment)
 		return;
 	saved_environment = 1;
-	if (!getcwd(orig_cwd, sizeof(orig_cwd)))
-		die_errno("cannot getcwd");
+	orig_cwd = xgetcwd();
 	for (i = 0; i < ARRAY_SIZE(env_names); i++) {
 		orig_env[i] = getenv(env_names[i]);
 		if (orig_env[i])
@@ -48,8 +47,9 @@ static void save_env(void)
 static void restore_env(void)
 {
 	int i;
-	if (*orig_cwd && chdir(orig_cwd))
+	if (orig_cwd && chdir(orig_cwd))
 		die_errno("could not move to %s", orig_cwd);
+	free(orig_cwd);
 	for (i = 0; i < ARRAY_SIZE(env_names); i++) {
 		if (orig_env[i])
 			setenv(env_names[i], orig_env[i], 1);
diff --git a/setup.c b/setup.c
index c8b8a97..23b4338 100644
--- a/setup.c
+++ b/setup.c
@@ -434,16 +434,16 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,
 		if (is_absolute_path(git_work_tree_cfg))
 			set_git_work_tree(git_work_tree_cfg);
 		else {
-			char core_worktree[PATH_MAX];
+			char *core_worktree;
 			if (chdir(gitdirenv))
 				die_errno("Could not chdir to '%s'", gitdirenv);
 			if (chdir(git_work_tree_cfg))
 				die_errno("Could not chdir to '%s'", git_work_tree_cfg);
-			if (!getcwd(core_worktree, PATH_MAX))
-				die_errno("Could not get directory '%s'", git_work_tree_cfg);
+			core_worktree = xgetcwd();
 			if (chdir(cwd->buf))
 				die_errno("Could not come back to cwd");
 			set_git_work_tree(core_worktree);
+			free(core_worktree);
 		}
 	}
 	else if (!git_env_bool(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, 1)) {
diff --git a/trace.c b/trace.c
index e583dc6..54aaee5 100644
--- a/trace.c
+++ b/trace.c
@@ -298,13 +298,12 @@ void trace_repo_setup(const char *prefix)
 {
 	static struct trace_key key = TRACE_KEY_INIT(SETUP);
 	const char *git_work_tree;
-	char cwd[PATH_MAX];
+	char *cwd;
 
 	if (!trace_want(&key))
 		return;
 
-	if (!getcwd(cwd, PATH_MAX))
-		die("Unable to get current working directory");
+	cwd = xgetcwd();
 
 	if (!(git_work_tree = get_git_work_tree()))
 		git_work_tree = "(null)";
@@ -316,6 +315,8 @@ void trace_repo_setup(const char *prefix)
 	trace_printf_key(&key, "setup: worktree: %s\n", quote_crnl(git_work_tree));
 	trace_printf_key(&key, "setup: cwd: %s\n", quote_crnl(cwd));
 	trace_printf_key(&key, "setup: prefix: %s\n", quote_crnl(prefix));
+
+	free(cwd);
 }
 
 int trace_want(struct trace_key *key)
-- 
2.0.2

[PATCH v3 08/10] use xgetcwd() to set $GIT_DIR

[René Scharfe]

Instead of dying of a segmentation fault if getcwd() returns NULL, use
xgetcwd() to make sure to write a useful error message and then exit
in an orderly fashion.

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 builtin/init-db.c | 7 +++----
 git.c             | 5 +++--
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index f6dd172..ab0ea02 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -537,10 +537,9 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 		usage(init_db_usage[0]);
 	}
 	if (is_bare_repository_cfg == 1) {
-		static char git_dir[PATH_MAX+1];
-
-		setenv(GIT_DIR_ENVIRONMENT,
-			getcwd(git_dir, sizeof(git_dir)), argc > 0);
+		char *cwd = xgetcwd();
+		setenv(GIT_DIR_ENVIRONMENT, cwd, argc > 0);
+		free(cwd);
 	}
 
 	if (init_shared_repository != -1)
diff --git a/git.c b/git.c
index 47137db..210f1ae 100644
--- a/git.c
+++ b/git.c
@@ -161,9 +161,10 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 			if (envchanged)
 				*envchanged = 1;
 		} else if (!strcmp(cmd, "--bare")) {
-			static char git_dir[PATH_MAX+1];
+			char *cwd = xgetcwd();
 			is_bare_repository_cfg = 1;
-			setenv(GIT_DIR_ENVIRONMENT, getcwd(git_dir, sizeof(git_dir)), 0);
+			setenv(GIT_DIR_ENVIRONMENT, cwd, 0);
+			free(cwd);
 			setenv(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, "0", 1);
 			if (envchanged)
 				*envchanged = 1;
-- 
2.0.2

[PATCH v3 09/10] abspath: convert absolute_path() to strbuf

[René Scharfe]

Move most of the code of absolute_path() into the new function
strbuf_add_absolute_path() and in the process transform it to use
struct strbuf and xgetcwd() instead of a PATH_MAX-sized buffer,
which can be too small on some file systems.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 Documentation/technical/api-strbuf.txt |  6 +++++
 abspath.c                              | 46 +++-------------------------------
 strbuf.c                               | 25 ++++++++++++++++++
 strbuf.h                               |  2 ++
 4 files changed, 37 insertions(+), 42 deletions(-)

diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt
index 49e477d..430302c 100644
--- a/Documentation/technical/api-strbuf.txt
+++ b/Documentation/technical/api-strbuf.txt
@@ -311,6 +311,12 @@ same behaviour as well.
 
 	Set the buffer to the path of the current working directory.
 
+`strbuf_add_absolute_path`
+
+	Add a path to a buffer, converting a relative path to an
+	absolute one in the process.  Symbolic links are not
+	resolved.
+
 `stripspace`::
 
 	Strip whitespace from a buffer. The second parameter controls if
diff --git a/abspath.c b/abspath.c
index 16e7fa2..197af68 100644
--- a/abspath.c
+++ b/abspath.c
@@ -140,54 +140,16 @@ const char *real_path_if_valid(const char *path)
 	return real_path_internal(path, 0);
 }
 
-static const char *get_pwd_cwd(void)
-{
-	static char cwd[PATH_MAX + 1];
-	char *pwd;
-	struct stat cwd_stat, pwd_stat;
-	if (getcwd(cwd, PATH_MAX) == NULL)
-		return NULL;
-	pwd = getenv("PWD");
-	if (pwd && strcmp(pwd, cwd)) {
-		stat(cwd, &cwd_stat);
-		if ((cwd_stat.st_dev || cwd_stat.st_ino) &&
-		    !stat(pwd, &pwd_stat) &&
-		    pwd_stat.st_dev == cwd_stat.st_dev &&
-		    pwd_stat.st_ino == cwd_stat.st_ino) {
-			strlcpy(cwd, pwd, PATH_MAX);
-		}
-	}
-	return cwd;
-}
-
 /*
  * Use this to get an absolute path from a relative one. If you want
  * to resolve links, you should use real_path.
- *
- * If the path is already absolute, then return path. As the user is
- * never meant to free the return value, we're safe.
  */
 const char *absolute_path(const char *path)
 {
-	static char buf[PATH_MAX + 1];
-
-	if (!*path) {
-		die("The empty string is not a valid path");
-	} else if (is_absolute_path(path)) {
-		if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX)
-			die("Too long path: %.*s", 60, path);
-	} else {
-		size_t len;
-		const char *fmt;
-		const char *cwd = get_pwd_cwd();
-		if (!cwd)
-			die_errno("Cannot determine the current working directory");
-		len = strlen(cwd);
-		fmt = (len > 0 && is_dir_sep(cwd[len - 1])) ? "%s%s" : "%s/%s";
-		if (snprintf(buf, PATH_MAX, fmt, cwd, path) >= PATH_MAX)
-			die("Too long path: %.*s", 60, path);
-	}
-	return buf;
+	static struct strbuf sb;
+	strbuf_init(&sb, 0);
+	strbuf_add_absolute_path(&sb, path);
+	return sb.buf;
 }
 
 /*
diff --git a/strbuf.c b/strbuf.c
index 2bf4dfa..4d31443 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -576,6 +576,31 @@ void strbuf_humanise_bytes(struct strbuf *buf, off_t bytes)
 	}
 }
 
+void strbuf_add_absolute_path(struct strbuf *sb, const char *path)
+{
+	if (!*path)
+		die("The empty string is not a valid path");
+	if (!is_absolute_path(path)) {
+		struct stat cwd_stat, pwd_stat;
+		size_t orig_len = sb->len;
+		char *cwd = xgetcwd();
+		char *pwd = getenv("PWD");
+		if (pwd && strcmp(pwd, cwd) &&
+		    !stat(cwd, &cwd_stat) &&
+		    (cwd_stat.st_dev || cwd_stat.st_ino) &&
+		    !stat(pwd, &pwd_stat) &&
+		    pwd_stat.st_dev == cwd_stat.st_dev &&
+		    pwd_stat.st_ino == cwd_stat.st_ino)
+			strbuf_addstr(sb, pwd);
+		else
+			strbuf_addstr(sb, cwd);
+		if (sb->len > orig_len && !is_dir_sep(sb->buf[sb->len - 1]))
+			strbuf_addch(sb, '/');
+		free(cwd);
+	}
+	strbuf_addstr(sb, path);
+}
+
 int printf_ln(const char *fmt, ...)
 {
 	int ret;
diff --git a/strbuf.h b/strbuf.h
index bc38bb9..7bdc1da 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -190,6 +190,8 @@ extern void strbuf_addstr_urlencode(struct strbuf *, const char *,
 				    int reserved);
 extern void strbuf_humanise_bytes(struct strbuf *buf, off_t bytes);
 
+extern void strbuf_add_absolute_path(struct strbuf *sb, const char *path);
+
 __attribute__((format (printf,1,2)))
 extern int printf_ln(const char *fmt, ...);
 __attribute__((format (printf,2,3)))
-- 
2.0.2

[PATCH v3 10/10] use strbuf_add_absolute_path() to add absolute paths

[René Scharfe]

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 exec_cmd.c  | 6 +-----
 sha1_file.c | 2 +-
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/exec_cmd.c b/exec_cmd.c
index 125fa6f..698e752 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -86,11 +86,7 @@ const char *git_exec_path(void)
 static void add_path(struct strbuf *out, const char *path)
 {
 	if (path && *path) {
-		if (is_absolute_path(path))
-			strbuf_addstr(out, path);
-		else
-			strbuf_addstr(out, absolute_path(path));
-
+		strbuf_add_absolute_path(out, path);
 		strbuf_addch(out, PATH_SEP);
 	}
 }
diff --git a/sha1_file.c b/sha1_file.c
index 3f70b1d..95afd20 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -350,7 +350,7 @@ static void link_alt_odb_entries(const char *alt, int len, int sep,
 		return;
 	}
 
-	strbuf_addstr(&objdirbuf, absolute_path(get_object_directory()));
+	strbuf_add_absolute_path(&objdirbuf, get_object_directory());
 	normalize_path_copy(objdirbuf.buf, objdirbuf.buf);
 
 	alt_copy = xmemdupz(alt, len);
-- 
2.0.2

[PATCH v3 04/10] abspath: use strbuf_getcwd() to remember original working directory

[René Scharfe]

Store the original working directory in a strbuf instead of in a
fixed-sized buffer, in order to be able to handle longer paths.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
Resent with corrected subject.

 abspath.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/abspath.c b/abspath.c
index ca33558..911e931 100644
--- a/abspath.c
+++ b/abspath.c
@@ -41,7 +41,7 @@ static const char *real_path_internal(const char *path, int die_on_error)
 	 * here so that we can chdir() back to it at the end of the
 	 * function:
 	 */
-	char cwd[1024] = "";
+	struct strbuf cwd = STRBUF_INIT;
 
 	int buf_index = 1;
 
@@ -80,7 +80,7 @@ static const char *real_path_internal(const char *path, int die_on_error)
 		}
 
 		if (*buf) {
-			if (!*cwd && !getcwd(cwd, sizeof(cwd))) {
+			if (!cwd.len && strbuf_getcwd(&cwd)) {
 				if (die_on_error)
 					die_errno("Could not get current working directory");
 				else
@@ -142,8 +142,9 @@ static const char *real_path_internal(const char *path, int die_on_error)
 	retval = buf;
 error_out:
 	free(last_elem);
-	if (*cwd && chdir(cwd))
-		die_errno("Could not change back to '%s'", cwd);
+	if (cwd.len && chdir(cwd.buf))
+		die_errno("Could not change back to '%s'", cwd.buf);
+	strbuf_release(&cwd);
 
 	return retval;
 }
-- 
2.0.2

Re: [PATCH v3 02/10] unix-sockets: use strbuf_getcwd()

[Jeff King]

On Mon, Jul 28, 2014 at 08:25:40PM +0200, René Scharfe wrote:

> Instead of using a PATH_MAX-sized buffer, which can be too small on some
> file systems, use strbuf_getcwd(), which handles any path getcwd()
> returns.  Also preserve the errno set by strbuf_getcwd() instead of
> setting it to ENAMETOOLONG; that way a more appropriate error message
> can be shown based on the actual reason for failing.
> 
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
>  unix-socket.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/unix-socket.c b/unix-socket.c
> index 91bd6b8..19ed48b 100644
> --- a/unix-socket.c
> +++ b/unix-socket.c
> @@ -18,12 +18,12 @@ static int chdir_len(const char *orig, int len)
>  }
>  
>  struct unix_sockaddr_context {
> -	char orig_dir[PATH_MAX];
> +	char *orig_dir;
>  };

I would have expected this to just convert to a strbuf. I guess, though
that you were making this...

>  
>  static void unix_sockaddr_cleanup(struct unix_sockaddr_context *ctx)
>  {
> -	if (!ctx->orig_dir[0])
> +	if (!ctx->orig_dir)
>  		return;

...a little nicer by using the pointer to check for validity. Looks good
to me.

-Peff

Re: [PATCH v3 05/10] abspath: convert real_path_internal() to strbuf

[Jeff King]

On Mon, Jul 28, 2014 at 08:28:30PM +0200, René Scharfe wrote:

>  static const char *real_path_internal(const char *path, int die_on_error)
>  {
> -	static char bufs[2][PATH_MAX + 1], *buf = bufs[0], *next_buf = bufs[1];
> +	static struct strbuf sb = STRBUF_INIT;

Hrm. I thought at first that this was our usual trick of keeping two
"simultaneous" static buffers, so that we can do:

  printf("paths '%s' and '%s'\n", real_path(foo), real_path(bar));

But it looks like that is not the case, and we only have two for
swapping back and forth as we figure out the answer (but they both need
to be static, because we do not know which one we will return in the
end). Is that right?

-Peff

Re: [PATCH v3 09/10] abspath: convert absolute_path() to strbuf

[Jeff King]

On Mon, Jul 28, 2014 at 08:33:55PM +0200, René Scharfe wrote:

>  const char *absolute_path(const char *path)
>  {
> -	static char buf[PATH_MAX + 1];
> -
> -	if (!*path) {
> -		die("The empty string is not a valid path");
> -	} else if (is_absolute_path(path)) {
> -		if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX)
> -			die("Too long path: %.*s", 60, path);
> -	} else {
> -		size_t len;
> -		const char *fmt;
> -		const char *cwd = get_pwd_cwd();
> -		if (!cwd)
> -			die_errno("Cannot determine the current working directory");
> -		len = strlen(cwd);
> -		fmt = (len > 0 && is_dir_sep(cwd[len - 1])) ? "%s%s" : "%s/%s";
> -		if (snprintf(buf, PATH_MAX, fmt, cwd, path) >= PATH_MAX)
> -			die("Too long path: %.*s", 60, path);
> -	}
> -	return buf;
> +	static struct strbuf sb;
> +	strbuf_init(&sb, 0);
> +	strbuf_add_absolute_path(&sb, path);
> +	return sb.buf;
>  }

Is it right to strbuf_init here? That means that we are throwing away
the old buffer for each call. I would think you want instead:

  static struct strbuf sb = STRBUF_INIT;
  strbuf_reset(&sb);
  ...

-Peff

Re: [PATCH v3 05/10] abspath: convert real_path_internal() to strbuf

[Jeff King]

On Mon, Jul 28, 2014 at 08:28:30PM +0200, René Scharfe wrote:

> @@ -60,26 +58,22 @@ static const char *real_path_internal(const char *path, int die_on_error)
>  			goto error_out;
>  	}
>  
> -	if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX) {
> -		if (die_on_error)
> -			die("Too long path: %.*s", 60, path);
> -		else
> -			goto error_out;
> -	}
> +	strbuf_init(&sb, 0);
> +	strbuf_addstr(&sb, path);

As with the other patch I just mentioned, should this be strbuf_reset,
not strbuf_init? We want to reset the static buffer back to zero-size,
not throw it away and leak whatever was there.

-Peff

Re: [PATCH v3 0/10] getcwd without PATH_MAX

[Jeff King]

On Mon, Jul 28, 2014 at 08:21:22PM +0200, René Scharfe wrote:

> Paths longer than PATH_MAX can be created and used on at least on some
> file systems.  Currently we use getcwd() generally with a PATH_MAX-
> sized buffer.  This series adds two functions, strbuf_getcwd() and
> xgetcwd(), then uses them to reduce the number of fixed-sized buffers
> and to allow us to handle longer working directory paths.

With the exception of the potential strbuf_reset/init I pointed out, all
of these look sane to me.

-Peff

Re: [PATCH v3 05/10] abspath: convert real_path_internal() to strbuf

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Jul 28, 2014 at 08:28:30PM +0200, René Scharfe wrote:
>
>> @@ -60,26 +58,22 @@ static const char *real_path_internal(const char *path, int die_on_error)
>>  			goto error_out;
>>  	}
>>  
>> -	if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX) {
>> -		if (die_on_error)
>> -			die("Too long path: %.*s", 60, path);
>> -		else
>> -			goto error_out;
>> -	}
>> +	strbuf_init(&sb, 0);
>> +	strbuf_addstr(&sb, path);
>
> As with the other patch I just mentioned, should this be strbuf_reset,
> not strbuf_init? We want to reset the static buffer back to zero-size,
> not throw it away and leak whatever was there.
>
> -Peff

Yes, this one seems to be leaking.

"Next call to the function invalidates the return value the last
caller received" feels like playing with fire.  Most existing
callers are safe in that the first thing they do to the returned
string is xstrdup() it, but we would need to check all the other
callers.

I briefly thought it is not OK for set_git_work_tree(), which gets
new_work_tree, calls real_path() to receive the value from the
function, and then calls real_path() again on it.  The "We've
already done it" optimization is the only thing that makes it safe,
which feels overly fragile.

Re: [PATCH v3 05/10] abspath: convert real_path_internal() to strbuf

[René Scharfe]

Am 28.07.2014 um 21:09 schrieb Jeff King:
> On Mon, Jul 28, 2014 at 08:28:30PM +0200, René Scharfe wrote:
>
>>   static const char *real_path_internal(const char *path, int die_on_error)
>>   {
>> -	static char bufs[2][PATH_MAX + 1], *buf = bufs[0], *next_buf = bufs[1];
>> +	static struct strbuf sb = STRBUF_INIT;
>
> Hrm. I thought at first that this was our usual trick of keeping two
> "simultaneous" static buffers, so that we can do:
>
>    printf("paths '%s' and '%s'\n", real_path(foo), real_path(bar));
>
> But it looks like that is not the case, and we only have two for
> swapping back and forth as we figure out the answer (but they both need
> to be static, because we do not know which one we will return in the
> end). Is that right?

AFAICS it's only swapped to avoid copying the results of a readlink() 
call against one buffer into the other.  So, yes, that's how I 
understand it as well.

René

Re: [PATCH v3 09/10] abspath: convert absolute_path() to strbuf

[René Scharfe]

Am 28.07.2014 um 21:15 schrieb Jeff King:
> On Mon, Jul 28, 2014 at 08:33:55PM +0200, René Scharfe wrote:
>
>>   const char *absolute_path(const char *path)
>>   {
>> -	static char buf[PATH_MAX + 1];
>> -
>> -	if (!*path) {
>> -		die("The empty string is not a valid path");
>> -	} else if (is_absolute_path(path)) {
>> -		if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX)
>> -			die("Too long path: %.*s", 60, path);
>> -	} else {
>> -		size_t len;
>> -		const char *fmt;
>> -		const char *cwd = get_pwd_cwd();
>> -		if (!cwd)
>> -			die_errno("Cannot determine the current working directory");
>> -		len = strlen(cwd);
>> -		fmt = (len > 0 && is_dir_sep(cwd[len - 1])) ? "%s%s" : "%s/%s";
>> -		if (snprintf(buf, PATH_MAX, fmt, cwd, path) >= PATH_MAX)
>> -			die("Too long path: %.*s", 60, path);
>> -	}
>> -	return buf;
>> +	static struct strbuf sb;
>> +	strbuf_init(&sb, 0);
>> +	strbuf_add_absolute_path(&sb, path);
>> +	return sb.buf;
>>   }
>
> Is it right to strbuf_init here? That means that we are throwing away
> the old buffer for each call. I would think you want instead:
>
>    static struct strbuf sb = STRBUF_INIT;
>    strbuf_reset(&sb);
>    ...

I changed it from _reset to _init, but I can't remember why. :(  Perhaps 
it's the summer heat.  Your version makes more sense to me now.

René

Re: [PATCH v3 03/10] setup: convert setup_git_directory_gently_1 et al. to strbuf

[Eric Sunshine]

On Mon, Jul 28, 2014 at 2:26 PM, René Scharfe <l.s.r@web.de> wrote:
> Convert setup_git_directory_gently_1() and its helper functions
> setup_explicit_git_dir(), setup_discovered_git_dir() and
> setup_bare_git_dir() to use a struct strbuf to hold the current working
> directory.  Replacing the PATH_MAX-sized buffer used before removes a
> path length limition on some file systems.  The functions are converted

s/limition/limitation/

> all in one go because they all read and write the variable cwd.
>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>

Re: [PATCH v3 05/10] abspath: convert real_path_internal() to strbuf

[René Scharfe]

Am 28.07.2014 um 23:42 schrieb Junio C Hamano:
> Jeff King <peff@peff.net> writes:
>
>> On Mon, Jul 28, 2014 at 08:28:30PM +0200, René Scharfe wrote:
>>
>>> @@ -60,26 +58,22 @@ static const char *real_path_internal(const char *path, int die_on_error)
>>>   			goto error_out;
>>>   	}
>>>
>>> -	if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX) {
>>> -		if (die_on_error)
>>> -			die("Too long path: %.*s", 60, path);
>>> -		else
>>> -			goto error_out;
>>> -	}
>>> +	strbuf_init(&sb, 0);
>>> +	strbuf_addstr(&sb, path);
>>
>> As with the other patch I just mentioned, should this be strbuf_reset,
>> not strbuf_init? We want to reset the static buffer back to zero-size,
>> not throw it away and leak whatever was there.
>>
>> -Peff
>
> Yes, this one seems to be leaking.
>
> "Next call to the function invalidates the return value the last
> caller received" feels like playing with fire.  Most existing
> callers are safe in that the first thing they do to the returned
> string is xstrdup() it, but we would need to check all the other
> callers.

That's the price we pay for using static variables, no?  Callers need to 
consume them as long as they're fresh and multi-threading is not 
allowed.  Before, callers could use wrong buffer contents, after the 
patch they could still have a pointer to freed memory, which should be 
more noticeable in tests.

Getting a strbuf_add_real_path() in order to avoid static variables 
would be nice.  And it would also be nice if it worked without calling 
chdir().  Nice topics for follow-up patches. :)

> I briefly thought it is not OK for set_git_work_tree(), which gets
> new_work_tree, calls real_path() to receive the value from the
> function, and then calls real_path() again on it.  The "We've
> already done it" optimization is the only thing that makes it safe,
> which feels overly fragile.

It wasn't introduced as an optimization, but to silence valgrind 
(1d679de5: make_absolute_path: return the input path if it points to our 
buffer).  set_git_work_tree() calls real_path() only once in each of its 
two branches.  However, one caller (init) hands it a path returned by 
real_path(); we can change that (sent a patch).

René

fixup for 05/10: plug leak

[René Scharfe]

Noticed-by: Jeff King <peff@peff.net>
---
 abspath.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/abspath.c b/abspath.c
index 16e7fa2..6aa328f 100644
--- a/abspath.c
+++ b/abspath.c
@@ -58,7 +58,7 @@ static const char *real_path_internal(const char *path, int die_on_error)
 			goto error_out;
 	}
 
-	strbuf_init(&sb, 0);
+	strbuf_reset(&sb);
 	strbuf_addstr(&sb, path);
 
 	while (depth--) {
-- 
2.0.2

fixup for 09/10: plug leak

[René Scharfe]

Noticed-by: Jeff King <peff@peff.net>
---
 abspath.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/abspath.c b/abspath.c
index cf9b404..5edb4e7 100644
--- a/abspath.c
+++ b/abspath.c
@@ -146,8 +146,8 @@ const char *real_path_if_valid(const char *path)
  */
 const char *absolute_path(const char *path)
 {
-	static struct strbuf sb;
-	strbuf_init(&sb, 0);
+	static struct strbuf sb = STRBUF_INIT;
+	strbuf_reset(&sb);
 	strbuf_add_absolute_path(&sb, path);
 	return sb.buf;
 }
-- 
2.0.2

Re: [PATCH v3 05/10] abspath: convert real_path_internal() to strbuf

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

>> "Next call to the function invalidates the return value the last
>> caller received" feels like playing with fire.  Most existing
>> callers are safe in that the first thing they do to the returned
>> string is xstrdup() it, but we would need to check all the other
>> callers.
>
> That's the price we pay for using static variables, no?  Callers need
> to consume them as long as they're fresh and multi-threading is not
> allowed.

Yes, I didn't mean to say that fixing this leak by a static whose
lifetime rule is "alive until next call" is introducing a new
brittleness.  The existing callers have lived with that lifetime
rule with the callee without the changes in this series, and fixing
the leak by replacing _init() with _reset() will make the callee
give the same old "alive until next call" lifetime rule to its
callers.

> Getting a strbuf_add_real_path() in order to avoid static variables
> would be nice.  And it would also be nice if it worked without calling
> chdir().  Nice topics for follow-up patches. :)

Yup.  Nice, but outside the scope.  Of course it is related and can
be done as a "while we know about the issue" close follow-up.

Thanks.

Re: [PATCH v3 03/10] setup: convert setup_git_directory_gently_1 et al. to strbuf

[Torsten Bögershausen]

On 2014-07-28 20.26, René Scharfe wrote:
> Convert setup_git_directory_gently_1() and its helper functions
> setup_explicit_git_dir(), setup_discovered_git_dir() and
> setup_bare_git_dir() to use a struct strbuf to hold the current working
> directory.  Replacing the PATH_MAX-sized buffer used before removes a
> path length limition on some file systems.  The functions are converted
> all in one go because they all read and write the variable cwd.
> 
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
>  setup.c | 85 +++++++++++++++++++++++++++++++++--------------------------------
>  1 file changed, 43 insertions(+), 42 deletions(-)
> 
> diff --git a/setup.c b/setup.c
> index 0a22f8b..c8b8a97 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -387,7 +387,7 @@ const char *read_gitfile(const char *path)
>  }
>  
>  static const char *setup_explicit_git_dir(const char *gitdirenv,
> -					  char *cwd, int len,
> +					  struct strbuf *cwd,
>  					  int *nongit_ok)
>  {
>  	const char *work_tree_env = getenv(GIT_WORK_TREE_ENVIRONMENT);
> @@ -441,7 +441,7 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,
>  				die_errno("Could not chdir to '%s'", git_work_tree_cfg);
>  			if (!getcwd(core_worktree, PATH_MAX))
>  				die_errno("Could not get directory '%s'", git_work_tree_cfg);
> -			if (chdir(cwd))
> +			if (chdir(cwd->buf))
>  				die_errno("Could not come back to cwd");
>  			set_git_work_tree(core_worktree);
>  		}
> @@ -459,21 +459,20 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,
>  	worktree = get_git_work_tree();
>  
>  	/* both get_git_work_tree() and cwd are already normalized */
> -	if (!strcmp(cwd, worktree)) { /* cwd == worktree */
> +	if (!strcmp(cwd->buf, worktree)) { /* cwd == worktree */
>  		set_git_dir(gitdirenv);
>  		free(gitfile);
>  		return NULL;
>  	}
>  
> -	offset = dir_inside_of(cwd, worktree);
> +	offset = dir_inside_of(cwd->buf, worktree);
>  	if (offset >= 0) {	/* cwd inside worktree? */
>  		set_git_dir(real_path(gitdirenv));
>  		if (chdir(worktree))
>  			die_errno("Could not chdir to '%s'", worktree);
> -		cwd[len++] = '/';
> -		cwd[len] = '\0';
> +		strbuf_addch(cwd, '/');
>  		free(gitfile);
> -		return cwd + offset;
> +		return cwd->buf + offset;
>  	}
>  
>  	/* cwd outside worktree */
> @@ -483,7 +482,7 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,
>  }
>  
>  static const char *setup_discovered_git_dir(const char *gitdir,
> -					    char *cwd, int offset, int len,
> +					    struct strbuf *cwd, int offset,
>  					    int *nongit_ok)
>  {
>  	if (check_repository_format_gently(gitdir, nongit_ok))
> @@ -491,17 +490,17 @@ static const char *setup_discovered_git_dir(const char *gitdir,
>  
>  	/* --work-tree is set without --git-dir; use discovered one */
>  	if (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) {
> -		if (offset != len && !is_absolute_path(gitdir))
> +		if (offset != cwd->len && !is_absolute_path(gitdir))
>  			gitdir = xstrdup(real_path(gitdir));
> -		if (chdir(cwd))
> +		if (chdir(cwd->buf))
>  			die_errno("Could not come back to cwd");
> -		return setup_explicit_git_dir(gitdir, cwd, len, nongit_ok);
> +		return setup_explicit_git_dir(gitdir, cwd, nongit_ok);
>  	}
>  
>  	/* #16.2, #17.2, #20.2, #21.2, #24, #25, #28, #29 (see t1510) */
>  	if (is_bare_repository_cfg > 0) {
> -		set_git_dir(offset == len ? gitdir : real_path(gitdir));
> -		if (chdir(cwd))
> +		set_git_dir(offset == cwd->len ? gitdir : real_path(gitdir));
> +		if (chdir(cwd->buf))
>  			die_errno("Could not come back to cwd");
>  		return NULL;
>  	}
> @@ -512,18 +511,18 @@ static const char *setup_discovered_git_dir(const char *gitdir,
>  		set_git_dir(gitdir);
>  	inside_git_dir = 0;
>  	inside_work_tree = 1;
> -	if (offset == len)
> +	if (offset == cwd->len)
>  		return NULL;
>  
>  	/* Make "offset" point to past the '/', and add a '/' at the end */
>  	offset++;
> -	cwd[len++] = '/';
> -	cwd[len] = 0;
> -	return cwd + offset;
> +	strbuf_addch(cwd, '/');
> +	return cwd->buf + offset;
>  }
>  
>  /* #16.1, #17.1, #20.1, #21.1, #22.1 (see t1510) */
> -static const char *setup_bare_git_dir(char *cwd, int offset, int len, int *nongit_ok)
> +static const char *setup_bare_git_dir(struct strbuf *cwd, int offset,
> +				      int *nongit_ok)
>  {
>  	int root_len;
>  
> @@ -536,20 +535,20 @@ static const char *setup_bare_git_dir(char *cwd, int offset, int len, int *nongi
>  	if (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) {
>  		const char *gitdir;
>  
> -		gitdir = offset == len ? "." : xmemdupz(cwd, offset);
> -		if (chdir(cwd))
> +		gitdir = offset == cwd->len ? "." : xmemdupz(cwd->buf, offset);
> +		if (chdir(cwd->buf))
>  			die_errno("Could not come back to cwd");
> -		return setup_explicit_git_dir(gitdir, cwd, len, nongit_ok);
> +		return setup_explicit_git_dir(gitdir, cwd, nongit_ok);
>  	}
>  
>  	inside_git_dir = 1;
>  	inside_work_tree = 0;
> -	if (offset != len) {
> -		if (chdir(cwd))
> +	if (offset != cwd->len) {
> +		if (chdir(cwd->buf))
>  			die_errno("Cannot come back to cwd");
> -		root_len = offset_1st_component(cwd);
> -		cwd[offset > root_len ? offset : root_len] = '\0';
> -		set_git_dir(cwd);
> +		root_len = offset_1st_component(cwd->buf);
> +		strbuf_setlen(cwd, offset > root_len ? offset : root_len);
> +		set_git_dir(cwd->buf);
>  	}
>  	else
>  		set_git_dir(".");
> @@ -617,10 +616,10 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
>  {
>  	const char *env_ceiling_dirs = getenv(CEILING_DIRECTORIES_ENVIRONMENT);
>  	struct string_list ceiling_dirs = STRING_LIST_INIT_DUP;
> -	static char cwd[PATH_MAX + 1];
> +	static struct strbuf cwd = STRBUF_INIT;


Is there a chance to squueze this in:


$ git diff
diff --git a/setup.c b/setup.c
index 526cdf6..fb61860 100644
--- a/setup.c
+++ b/setup.c
@@ -734,7 +734,7 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
                string_list_clear(&ceiling_dirs, 0);
        }

-       if (ceil_offset < 0 && has_dos_drive_prefix(cwd))
+       if (ceil_offset < 0 && has_dos_drive_prefix(cwd.buf))
                ceil_offset = 1;

Re: [PATCH v3 03/10] setup: convert setup_git_directory_gently_1 et al. to strbuf

[René Scharfe]

> Is there a chance to squueze this in:
> 
> 
> $ git diff
> diff --git a/setup.c b/setup.c
> index 526cdf6..fb61860 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -734,7 +734,7 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
>                  string_list_clear(&ceiling_dirs, 0);
>          }
> 
> -       if (ceil_offset < 0 && has_dos_drive_prefix(cwd))
> +       if (ceil_offset < 0 && has_dos_drive_prefix(cwd.buf))
>                  ceil_offset = 1;
> 
> 

Ouch, thanks for catching this.

Perhaps the following patch should go in as well.

-- >8 --
Subject: [PATCH] turn path macros into inline function

Use static inline functions instead of macros for has_dos_drive_prefix,
offset_1st_component, is_dir_sep and find_last_dir_sep in order to let
the compiler do type checking.

The definitions of offset_1st_component and is_dir_sep are switched
around because the former uses the latter.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 git-compat-util.h | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index f587749..0b6c13a 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -264,19 +264,35 @@ extern char *gitbasename(char *);
 #endif
 
 #ifndef has_dos_drive_prefix
-#define has_dos_drive_prefix(path) 0
+static inline int git_has_dos_drive_prefix(const char *path)
+{
+	return 0;
+}
+#define has_dos_drive_prefix git_has_dos_drive_prefix
 #endif
 
-#ifndef offset_1st_component
-#define offset_1st_component(path) (is_dir_sep((path)[0]))
+#ifndef is_dir_sep
+static inline int git_is_dir_sep(int c)
+{
+	return c == '/';
+}
+#define is_dir_sep git_is_dir_sep
 #endif
 
-#ifndef is_dir_sep
-#define is_dir_sep(c) ((c) == '/')
+#ifndef offset_1st_component
+static inline int git_offset_1st_component(const char *path)
+{
+	return is_dir_sep(path[0]);
+}
+#define offset_1st_component git_offset_1st_component
 #endif
 
 #ifndef find_last_dir_sep
-#define find_last_dir_sep(path) strrchr(path, '/')
+static inline char *git_find_last_dir_sep(const char *path)
+{
+	return strrchr(path, '/');
+}
+#define find_last_dir_sep git_find_last_dir_sep
 #endif
 
 #if defined(__HP_cc) && (__HP_cc >= 61000)
-- 
2.1.0

Re: [PATCH v3 03/10] setup: convert setup_git_directory_gently_1 et al. to strbuf

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

>> Is there a chance to squueze this in:
>> 
>> 
>> $ git diff
>> diff --git a/setup.c b/setup.c
>> index 526cdf6..fb61860 100644
>> --- a/setup.c
>> +++ b/setup.c
>> @@ -734,7 +734,7 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
>>                  string_list_clear(&ceiling_dirs, 0);
>>          }
>> 
>> -       if (ceil_offset < 0 && has_dos_drive_prefix(cwd))
>> +       if (ceil_offset < 0 && has_dos_drive_prefix(cwd.buf))
>>                  ceil_offset = 1;
>> 
>> 
>
> Ouch, thanks for catching this.

Let me squash it in to the original change when rebuilding 'next'
sometime this week, since we have tagged 2.1 now.

> Perhaps the following patch should go in as well.

Nice; it catches the above, and shows there isn't any similar gotcha
at least for me (meaning: parts inside e.g. "#ifdef WINDOWS" that I
do not compile are not covered by "at least for me" test I did).

Thanks.

> -- >8 --
> Subject: [PATCH] turn path macros into inline function
>
> Use static inline functions instead of macros for has_dos_drive_prefix,
> offset_1st_component, is_dir_sep and find_last_dir_sep in order to let
> the compiler do type checking.
>
> The definitions of offset_1st_component and is_dir_sep are switched
> around because the former uses the latter.
>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
>  git-compat-util.h | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index f587749..0b6c13a 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -264,19 +264,35 @@ extern char *gitbasename(char *);
>  #endif
>  
>  #ifndef has_dos_drive_prefix
> -#define has_dos_drive_prefix(path) 0
> +static inline int git_has_dos_drive_prefix(const char *path)
> +{
> +	return 0;
> +}
> +#define has_dos_drive_prefix git_has_dos_drive_prefix
>  #endif
>  
> -#ifndef offset_1st_component
> -#define offset_1st_component(path) (is_dir_sep((path)[0]))
> +#ifndef is_dir_sep
> +static inline int git_is_dir_sep(int c)
> +{
> +	return c == '/';
> +}
> +#define is_dir_sep git_is_dir_sep
>  #endif
>  
> -#ifndef is_dir_sep
> -#define is_dir_sep(c) ((c) == '/')
> +#ifndef offset_1st_component
> +static inline int git_offset_1st_component(const char *path)
> +{
> +	return is_dir_sep(path[0]);
> +}
> +#define offset_1st_component git_offset_1st_component
>  #endif
>  
>  #ifndef find_last_dir_sep
> -#define find_last_dir_sep(path) strrchr(path, '/')
> +static inline char *git_find_last_dir_sep(const char *path)
> +{
> +	return strrchr(path, '/');
> +}
> +#define find_last_dir_sep git_find_last_dir_sep
>  #endif
>  
>  #if defined(__HP_cc) && (__HP_cc >= 61000)