getcwd without PATH_MAX

PATCH In review René Scharfe 5 participants 28 messages Jun 15, 2016 Documentation

Share & tools

René Scharfe

author

2016-06-15 23:02 UTC #0 lore raw

v3 cover getcwd without PATH_MAX

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(-)

Signature · 1 line

2.0.2

Jeff King

2016-06-15 23:02 UTC #1 lore raw

Quoted 5 lines · 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.

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

-Peff

René Scharfe

author

2016-06-15 23:02 UTC #2 lore raw

PATCH v3 1/10 strbuf: add strbuf_getcwd()

In reply to 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.

Helped-by Duy Nguyen <[email protected]>

Signed-off-by Rene Scharfe <[email protected]>

---
 Documentation/technical/api-strbuf.txt |  4 ++++
 strbuf.c                               | 21 +++++++++++++++++++++
 strbuf.h                               |  1 +
 3 files changed, 26 insertions(+)

Documentation/technical/api-strbuf.txt +4 -0

index f9c06a7..49e477d 100644
@@ -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

strbuf.c +21 -0

index 33018d8..2bf4dfa 100644
@@ -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;

strbuf.h +1 -0

index a7c0192..bc38bb9 100644
@@ -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);

Signature · 1 line

2.0.2

René Scharfe

author

2016-06-15 23:02 UTC #3 lore raw

PATCH v3 2/10 unix-sockets: use strbuf_getcwd()

In reply to 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 <[email protected]>

---
 unix-socket.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

unix-socket.c +7 -7

index 91bd6b8..19ed48b 100644
@@ -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;
 	}

Signature · 1 line

2.0.2

Jeff King

2016-06-15 23:02 UTC #4 lore raw

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

Quoted 16 lines · 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 <[email protected]>
---
 unix-socket.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)
unix-socket.c +0 -0
index 91bd6b8..19ed48b 100644

unix-socket.c static int chdir_len(const char *orig, int len) · line 22

Quoted 6 lines

unix-socket.c +1 -1

@@ -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...

unix-socket.c static int chdir_len(const char *orig, int len) · line 27

Quoted 6 lines

unix-socket.c +1 -1

 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

René Scharfe

author

2016-06-15 23:02 UTC #5 lore raw

PATCH v3 3/10 setup: convert setup_git_directory_gently_1 et al. to strbuf

In reply to 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 <[email protected]>

---
 setup.c | 85 +++++++++++++++++++++++++++++++++--------------------------------
 1 file changed, 43 insertions(+), 42 deletions(-)

setup.c +43 ~~-42~~

index 0a22f8b..c8b8a97 100644
@@ -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;
 	}

Signature · 1 line

2.0.2

Eric Sunshine

2016-06-15 23:02 UTC #6 lore raw

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

Quoted 5 lines · 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

s/limition/limitation/

Quoted 3 lines

all in one go because they all read and write the variable cwd.
Signed-off-by Rene Scharfe <[email protected]>

Torsten Bögershausen

2016-06-15 23:02 UTC #7 lore raw

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

In reply to René Scharfe

Quoted 146 lines · René Scharfe

Signed-off-by Rene Scharfe <[email protected]>

---
 setup.c | 85 +++++++++++++++++++++++++++++++++--------------------------------
 1 file changed, 43 insertions(+), 42 deletions(-)

setup.c +25 ~~-26~~

index 0a22f8b..c8b8a97 100644
@@ -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(".");

setup.c static const char *setup_git_directory_gently_1(int *nongit_ok) · line 619

Quoted 5 lines

setup.c +1 -1

@@ -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

setup.c +1 -1

index 526cdf6..fb61860 100644
@@ -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;

René Scharfe

author

2016-06-15 23:02 UTC #8 lore raw

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

Quoted 9 lines

Is there a chance to squueze this in:
$ git diff
setup.c +0 -0
index 526cdf6..fb61860 100644

setup.c static const char *setup_git_directory_gently_1(int *nongit_ok) · line 740

Quoted 8 lines

setup.c +1 -1

@@ -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 <[email protected]>

---
 git-compat-util.h | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

git-compat-util.h +22 -6

index f587749..0b6c13a 100644
@@ -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)

Signature · 1 line

2.1.0

Junio C Hamano

2016-06-15 23:02 UTC #9 lore raw

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

Quoted 12 lines · René Scharfe

Quoted 12 lines

Is there a chance to squueze this in:

$ git diff

setup.c +0 -0

index 526cdf6..fb61860 100644
@@ -734,7 +734,7 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
                 string_list_clear(&ceiling_dirs, 0);
         }

Quoted 7 lines

setup.c static const char *setup_git_directory_gently_1(int *nongit_ok) · line 740
Quoted 5 lines
setup.c +1 -1
-       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.

Quoted 1 line

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.

Quoted 61 lines

-- >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 <[email protected]>

---
 git-compat-util.h | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

git-compat-util.h +22 -6

index f587749..0b6c13a 100644
@@ -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)

René Scharfe

author

2016-06-15 23:02 UTC #10 lore raw

PATCH v3 4/10 abspath: use strbuf_getcwd() to remember original working directory

In reply to 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 <[email protected]>

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

abspath.c +5 -4

index ca33558..911e931 100644
@@ -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;
 }

Signature · 1 line

2.0.2

René Scharfe

author

2016-06-15 23:02 UTC #11 lore raw

PATCH v3 4/10 abspath: use strbuf_getcwd() to remember original working directory

In reply to 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 <[email protected]>

---
Resent with corrected subject.

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

abspath.c +5 -4

index ca33558..911e931 100644
@@ -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;
 }

Signature · 1 line

2.0.2

René Scharfe

author

2016-06-15 23:02 UTC #12 lore raw

PATCH v3 5/10 abspath: convert real_path_internal() to strbuf

In reply to 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 <[email protected]>

---
 abspath.c | 69 +++++++++++++++++++++++----------------------------------------
 1 file changed, 25 insertions(+), 44 deletions(-)

abspath.c +25 ~~-44~~

index 911e931..16e7fa2 100644
@@ -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))

Signature · 1 line

2.0.2

Jeff King

2016-06-15 23:02 UTC #13 lore raw

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

abspath.c int is_directory(const char *path) · line 36

Quoted 4 lines · René Scharfe

static const char *real_path_internal(const char *path, int die_on_error) {
abspath.c +1 -1
-	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

René Scharfe

author

2016-06-15 23:02 UTC #14 lore raw

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

Am 28.07.2014 um 21:09 schrieb Jeff King:

Quoted 10 lines · René Scharfe

abspath.c int is_directory(const char *path) · line 36
Quoted 5 lines
static const char *real_path_internal(const char *path, int die_on_error) {
abspath.c +1 -1
-	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:

Quoted 6 lines

   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é

Jeff King

2016-06-15 23:02 UTC #15 lore raw

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

In reply to René Scharfe

abspath.c static const char *real_path_internal(const char *path, int die_on_error) · line 62

Quoted 12 lines · René Scharfe

abspath.c +2 -6

@@ -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

Junio C Hamano

2016-06-15 23:02 UTC #16 lore raw

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

Quoted 19 lines · Jeff King

abspath.c static const char *real_path_internal(const char *path, int die_on_error) · line 62
Quoted 12 lines · René Scharfe
abspath.c +2 -6
@@ -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.

Quoted 1 line

-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.

René Scharfe

author

2016-06-15 23:02 UTC #17 lore raw

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

Am 28.07.2014 um 23:42 schrieb Junio C Hamano:

Quoted 23 lines

Quoted 19 lines · Jeff King
abspath.c static const char *real_path_internal(const char *path, int die_on_error) · line 62
Quoted 12 lines · René Scharfe
abspath.c +2 -6
@@ -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.
Quoted 1 line
-Peff

Quoted 7 lines

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. :)

Quoted 5 lines

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é

Junio C Hamano

2016-06-15 23:02 UTC #18 lore raw

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

Quoted 9 lines · René Scharfe

Quoted 5 lines
"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.

Quoted 3 lines

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.

René Scharfe

author

2016-06-15 23:02 UTC #19 lore raw

fixup for 05/10: plug leak

In reply to René Scharfe

Noticed-by Jeff King <[email protected]>

---
 abspath.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

abspath.c +1 -1

index 16e7fa2..6aa328f 100644
@@ -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--) {

Signature · 1 line

2.0.2

René Scharfe

author

2016-06-15 23:02 UTC #20 lore raw

PATCH v3 6/10 wrapper: add xgetcwd()

In reply to 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 <[email protected]>

Signed-off-by Rene Scharfe <[email protected]>

---
 git-compat-util.h | 1 +
 wrapper.c         | 8 ++++++++
 2 files changed, 9 insertions(+)

git-compat-util.h +1 -0

index 26e92f1..4d6edea 100644
@@ -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)
 {

wrapper.c +8 -0

index bc1bfb8..bd24cda 100644
@@ -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);
+}

Signature · 1 line

2.0.2

René Scharfe

author

2016-06-15 23:02 UTC #21 lore raw

PATCH v3 7/10 use xgetcwd() to get the current directory or die

In reply to 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 <[email protected]>

---
 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(-)

builtin/init-db.c +8 -9

index 56f85e2..f6dd172 100644
@@ -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

builtin/rev-parse.c +3 -3

index 8102aaa..f6bbac7 100644
@@ -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")) {

dir.c +8 -4

index fcb6872..bd274a7 100644
@@ -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)

git.c +4 -4

index 9c49519..47137db 100644
@@ -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);

setup.c +3 -3

index c8b8a97..23b4338 100644
@@ -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)) {

trace.c +4 -3

index e583dc6..54aaee5 100644
@@ -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)

Signature · 1 line

2.0.2

René Scharfe

author

2016-06-15 23:02 UTC #22 lore raw

PATCH v3 8/10 use xgetcwd() to set $GIT_DIR

In reply to 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 <[email protected]>

Signed-off-by Rene Scharfe <[email protected]>

---
 builtin/init-db.c | 7 +++----
 git.c             | 5 +++--
 2 files changed, 6 insertions(+), 6 deletions(-)

builtin/init-db.c +3 -4

index f6dd172..ab0ea02 100644
@@ -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)

git.c +3 -2

index 47137db..210f1ae 100644
@@ -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;

Signature · 1 line

2.0.2

René Scharfe

author

2016-06-15 23:02 UTC #23 lore raw

PATCH v3 9/10 abspath: convert absolute_path() to strbuf

In reply to 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 <[email protected]>

---
 Documentation/technical/api-strbuf.txt |  6 +++++
 abspath.c                              | 46 +++-------------------------------
 strbuf.c                               | 25 ++++++++++++++++++
 strbuf.h                               |  2 ++
 4 files changed, 37 insertions(+), 42 deletions(-)

Documentation/technical/api-strbuf.txt +6 -0

index 49e477d..430302c 100644
@@ -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

abspath.c +4 ~~-42~~

58 lines not on this page · raw message

strbuf.c +25 -0

index 2bf4dfa..4d31443 100644
@@ -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;

strbuf.h +2 -0

index bc38bb9..7bdc1da 100644
@@ -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)))

Signature · 1 line

2.0.2

Jeff King

2016-06-15 23:02 UTC #24 lore raw

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

Quoted 16 lines · René Scharfe

const char *absolute_path(const char *path)
{

abspath.c +0 ~~-14~~

-	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);

abspath.c const char *real_path_if_valid(const char *path) · line 153

Quoted 10 lines

abspath.c +4 -5

-		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

René Scharfe

author

2016-06-15 23:02 UTC #25 lore raw

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

Am 28.07.2014 um 21:15 schrieb Jeff King:

Quoted 32 lines · René Scharfe

Quoted 17 lines

  const char *absolute_path(const char *path)
  {

abspath.c +0 ~~-14~~

-	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);

abspath.c const char *real_path_if_valid(const char *path) · line 153

Quoted 10 lines

abspath.c +4 -5

-		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:

Quoted 3 lines

   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é

René Scharfe

author

2016-06-15 23:02 UTC #26 lore raw

fixup for 09/10: plug leak

In reply to René Scharfe

Noticed-by Jeff King <[email protected]>

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

abspath.c +2 -2

index cf9b404..5edb4e7 100644
@@ -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;
 }

Signature · 1 line

2.0.2

René Scharfe

author

2016-06-15 23:02 UTC #27 lore raw

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

In reply to René Scharfe

Signed-off-by Rene Scharfe <[email protected]>

---
 exec_cmd.c  | 6 +-----
 sha1_file.c | 2 +-
 2 files changed, 2 insertions(+), 6 deletions(-)

exec_cmd.c +1 -5

index 125fa6f..698e752 100644
@@ -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);
 	}
 }

sha1_file.c +1 -1

index 3f70b1d..95afd20 100644
@@ -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);

Signature · 1 line

2.0.2