[PATCH v2 0/4] getcwd without PATH_MAX

[PATCH v2 0/4] 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.

Not all getcwd() calls are replaced, however.  I hope that at least
some of the remaining ones can be avoided altogether.  If that turns
out to be too optimistic then we can use the added function to convert
the rest.

Changes in v2:
  * strbuf_getcwd() instead of strbuf_add_cwd(), because the former is
    simpler and sufficient for now; based on a suggestion by Duy
  * added patch 2 as an example for strbuf_getcwd() usage, suggested
    by Duy
  * made sure strbuf_getcwd() leaves the strbuf intact, no matter
    what getcwd() does
  * converted an easy getcwd() call in setup.c

René Scharfe (4):
  strbuf: add strbuf_getcwd()
  use strbuf_getcwd() to get the current working directory without
    fixed-sized buffers
  wrapper: add xgetcwd()
  use xgetcwd() get the current directory or die

 Documentation/technical/api-strbuf.txt |  4 ++++
 builtin/init-db.c                      | 25 ++++++++++++-------------
 builtin/rev-parse.c                    |  6 +++---
 dir.c                                  | 12 ++++++++----
 git-compat-util.h                      |  1 +
 git.c                                  |  6 ++++--
 setup.c                                |  6 +++---
 strbuf.c                               | 21 +++++++++++++++++++++
 strbuf.h                               |  1 +
 trace.c                                |  7 ++++---
 wrapper.c                              |  8 ++++++++
 11 files changed, 69 insertions(+), 28 deletions(-)

-- 
2.0.2

[PATCH v2 1/4] strbuf: add strbuf_getcwd()

[René Scharfe]

Add strbuf_getcwd(), which puts the current working directory intto
a strbuf.  Because it doesn't use a fixed-size buffer it supports
arbitrarily long paths, as long as 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: 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 v2 2/4] use strbuf_getcwd() to get the current working directory without fixed-sized buffers

[René Scharfe]

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

diff --git a/builtin/init-db.c b/builtin/init-db.c
index 56f85e2..c4958b6 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -535,10 +535,10 @@ 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);
+		struct strbuf cwd = STRBUF_INIT;
+		strbuf_getcwd(&cwd);
+		setenv(GIT_DIR_ENVIRONMENT, cwd.buf, argc > 0);
+		strbuf_release(&cwd);
 	}
 
 	if (init_shared_repository != -1)
diff --git a/git.c b/git.c
index 5b6c761..3f52c43 100644
--- a/git.c
+++ b/git.c
@@ -161,9 +161,11 @@ 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];
+			struct strbuf cwd = STRBUF_INIT;
 			is_bare_repository_cfg = 1;
-			setenv(GIT_DIR_ENVIRONMENT, getcwd(git_dir, sizeof(git_dir)), 0);
+			strbuf_getcwd(&cwd);
+			setenv(GIT_DIR_ENVIRONMENT, cwd.buf, 0);
+			strbuf_release(&cwd);
 			setenv(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, "0", 1);
 			if (envchanged)
 				*envchanged = 1;
-- 
2.0.2

Re: [PATCH v2 2/4] use strbuf_getcwd() to get the current working directory without fixed-sized buffers

[Jeff King]

On Sun, Jul 20, 2014 at 06:49:54PM +0200, René Scharfe wrote:

> diff --git a/builtin/init-db.c b/builtin/init-db.c
> index 56f85e2..c4958b6 100644
> --- a/builtin/init-db.c
> +++ b/builtin/init-db.c
> @@ -535,10 +535,10 @@ 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);
> +		struct strbuf cwd = STRBUF_INIT;
> +		strbuf_getcwd(&cwd);
> +		setenv(GIT_DIR_ENVIRONMENT, cwd.buf, argc > 0);
> +		strbuf_release(&cwd);

Hmm. You are not making anything worse here, as we already do not check
the return value of getcwd. But what happens if it fails? Looks like we
currently get a segfault, and the new code will silently set the
variable to the empty string. Neither is particularly helpful.

Should we be using the xgetcwd helper that you add in the next patch?

> -			setenv(GIT_DIR_ENVIRONMENT, getcwd(git_dir, sizeof(git_dir)), 0);
> +			strbuf_getcwd(&cwd);
> +			setenv(GIT_DIR_ENVIRONMENT, cwd.buf, 0);
> +			strbuf_release(&cwd);

Ditto here.

-Peff

Re: [PATCH v2 2/4] use strbuf_getcwd() to get the current working directory without fixed-sized buffers

[René Scharfe]

Am 21.07.2014 04:33, schrieb Jeff King:
> On Sun, Jul 20, 2014 at 06:49:54PM +0200, René Scharfe wrote:
>
>> diff --git a/builtin/init-db.c b/builtin/init-db.c
>> index 56f85e2..c4958b6 100644
>> --- a/builtin/init-db.c
>> +++ b/builtin/init-db.c
>> @@ -535,10 +535,10 @@ 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);
>> +		struct strbuf cwd = STRBUF_INIT;
>> +		strbuf_getcwd(&cwd);
>> +		setenv(GIT_DIR_ENVIRONMENT, cwd.buf, argc > 0);
>> +		strbuf_release(&cwd);
>
> Hmm. You are not making anything worse here, as we already do not check
> the return value of getcwd. But what happens if it fails? Looks like we
> currently get a segfault, and the new code will silently set the
> variable to the empty string. Neither is particularly helpful.
>
> Should we be using the xgetcwd helper that you add in the next patch?

Probably.  And I was so glad to have found an example case for getcwd 
without dying and without touching the get-there-and-back cases. :) 
Guess I'll have to look closer at setup.c and perhaps unix-socket.c for 
a replacement.

By the way: Simply setting $GIT_DIR to "." probably won't work in the 
two cases, I guess?

>
>> -			setenv(GIT_DIR_ENVIRONMENT, getcwd(git_dir, sizeof(git_dir)), 0);
>> +			strbuf_getcwd(&cwd);
>> +			setenv(GIT_DIR_ENVIRONMENT, cwd.buf, 0);
>> +			strbuf_release(&cwd);
>
> Ditto here.
>
> -Peff
>

Re: [PATCH v2 2/4] use strbuf_getcwd() to get the current working directory without fixed-sized buffers

[Jeff King]

On Mon, Jul 21, 2014 at 07:10:13PM +0200, René Scharfe wrote:

> Probably.  And I was so glad to have found an example case for getcwd
> without dying and without touching the get-there-and-back cases. :) Guess
> I'll have to look closer at setup.c and perhaps unix-socket.c for a
> replacement.

I think just:

  const char *x = xgetcwd();
  setenv(GIT_DIR_ENVIRONMENT, x, 0);
  free(x);

would be enough?

> By the way: Simply setting $GIT_DIR to "." probably won't work in the two
> cases, I guess?

It might, but I'd be a little wary. For example, for the call in
init_db, would we later then chdir to the working tree in order to do a
checkout (since init_db is part of a clone)? Even if it works now, it
seems like a bit of an accident waiting to happen.

-Peff

Re: [PATCH v2 2/4] use strbuf_getcwd() to get the current working directory without fixed-sized buffers

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sun, Jul 20, 2014 at 06:49:54PM +0200, René Scharfe wrote:
>
>> diff --git a/builtin/init-db.c b/builtin/init-db.c
>> index 56f85e2..c4958b6 100644
>> --- a/builtin/init-db.c
>> +++ b/builtin/init-db.c
>> @@ -535,10 +535,10 @@ 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);
>> +		struct strbuf cwd = STRBUF_INIT;
>> +		strbuf_getcwd(&cwd);
>> +		setenv(GIT_DIR_ENVIRONMENT, cwd.buf, argc > 0);
>> +		strbuf_release(&cwd);
>
> Hmm. You are not making anything worse here, as we already do not check
> the return value of getcwd. But what happens if it fails? Looks like we
> currently get a segfault, and the new code will silently set the
> variable to the empty string. Neither is particularly helpful.
>
> Should we be using the xgetcwd helper that you add in the next patch?
>
>> -			setenv(GIT_DIR_ENVIRONMENT, getcwd(git_dir, sizeof(git_dir)), 0);
>> +			strbuf_getcwd(&cwd);
>> +			setenv(GIT_DIR_ENVIRONMENT, cwd.buf, 0);
>> +			strbuf_release(&cwd);
>
> Ditto here.

Good eyes.

[PATCH v2 3/4] 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 0b53c9a..d64d012 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -605,6 +605,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 v2 4/4] use xgetcwd() 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 ++++++++----
 setup.c             |  6 +++---
 trace.c             |  7 ++++---
 5 files changed, 26 insertions(+), 22 deletions(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index c4958b6..dc226d1 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 e65888d..7b994d4 100644
--- a/dir.c
+++ b/dir.c
@@ -1499,12 +1499,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/setup.c b/setup.c
index 0a22f8b..dc92d63 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))
 				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 08180a9..3523667 100644
--- a/trace.c
+++ b/trace.c
@@ -158,13 +158,12 @@ void trace_repo_setup(const char *prefix)
 {
 	static const char *key = "GIT_TRACE_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)";
@@ -176,6 +175,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(const char *key)
-- 
2.0.2