[PATCH 0/8 REVISION2] work-tree cleanups

[PATCH 0/8 REVISION2] work-tree cleanups

[Johannes Schindelin]

Hi,

the patch series I sent was almost as bogus as the original work-tree 
patch series.  However, thanks to the comments, and a night's sleep, here 
comes a revised patch series which is not only readable (it was readable 
before, too), but which actually works as expected.

Not only does it pass the tests, but it also adds some tests of its own.

And yes, you can "GIT_DIR=/some/where git add <file>..." now.  Even if you 
are not at your working tree's root.

Ciao,
Dscho

[PATCH 1/8] Add is_absolute_path() and make_absolute_path()

[Johannes Schindelin]

This patch adds convenience functions to work with absolute paths.
The function is_absolute_path() should help the efforts to integrate
the MinGW fork.

Note that make_absolute_path() returns a pointer to a static buffer.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	Okay, gone is normalize_path().  In contrast, "make_absolute_path()"
	does the getcwd() && chdir() && getcwd() && chdir(back) mantra, to
	follow symlinks.

	AFAICT, make_absolute_path() would be a Good Thing to use for the
	recent lockfile stuff.

 Makefile             |    2 +-
 cache.h              |    5 ++++
 path.c               |   62 ++++++++++++++++++++++++++++++++++++++++++++++++++
 t/t0000-basic.sh     |   16 +++++++++++++
 test-absolute-path.c |   11 +++++++++
 5 files changed, 95 insertions(+), 1 deletions(-)
 create mode 100644 test-absolute-path.c

diff --git a/Makefile b/Makefile
index d8100ad..546e008 100644
--- a/Makefile
+++ b/Makefile
@@ -936,7 +936,7 @@ endif
 
 ### Testing rules
 
-TEST_PROGRAMS = test-chmtime$X test-genrandom$X test-date$X test-delta$X test-sha1$X test-match-trees$X
+TEST_PROGRAMS = test-chmtime$X test-genrandom$X test-date$X test-delta$X test-sha1$X test-match-trees$X test-absolute-path$X
 
 all:: $(TEST_PROGRAMS)
 
diff --git a/cache.h b/cache.h
index 53801b8..98af530 100644
--- a/cache.h
+++ b/cache.h
@@ -358,6 +358,11 @@ int git_config_perm(const char *var, const char *value);
 int adjust_shared_perm(const char *path);
 int safe_create_leading_directories(char *path);
 char *enter_repo(char *path, int strict);
+static inline int is_absolute_path(const char *path)
+{
+	return path[0] == '/';
+}
+const char *make_absolute_path(const char *path);
 
 /* Read and unpack a sha1 file into memory, write memory to a sha1 file */
 extern int sha1_object_info(const unsigned char *, unsigned long *);
diff --git a/path.c b/path.c
index c4ce962..0f7012f 100644
--- a/path.c
+++ b/path.c
@@ -292,3 +292,65 @@ int adjust_shared_perm(const char *path)
 		return -2;
 	return 0;
 }
+
+/* We allow "recursive" symbolic links. Only within reason, though. */
+#define MAXDEPTH 5
+
+const char *make_absolute_path(const char *path)
+{
+	static char bufs[2][PATH_MAX + 1], *buf = bufs[0], *next_buf = bufs[1];
+	char cwd[1024] = "";
+	int buf_index = 1, len;
+
+	int depth = MAXDEPTH;
+	char *last_elem = NULL;
+	struct stat st;
+
+	if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX)
+		die ("Too long path: %.*s", 60, path);
+
+	while (depth--) {
+		if (stat(buf, &st) || !S_ISDIR(st.st_mode)) {
+			char *last_slash = strrchr(buf, '/');
+			*last_slash = '\0';
+			last_elem = xstrdup(last_slash + 1);
+		}
+
+		if (*buf) {
+			if (!*cwd && getcwd(cwd, sizeof(cwd)) < 0)
+				die ("Could not get current working directory");
+
+			if (chdir(buf))
+				die ("Could not switch to '%s'", buf);
+		}
+		if (getcwd(buf, PATH_MAX) < 0)
+			die ("Could not get current working directory");
+
+		if (last_elem) {
+			int len = strlen(buf);
+			if (len + strlen(last_elem) + 2 > PATH_MAX)
+				die ("Too long path name: '%s/%s'",
+						buf, last_elem);
+			buf[len] = '/';
+			strcpy(buf + len + 1, last_elem);
+			free(last_elem);
+			last_elem = NULL;
+		}
+
+		if (!lstat(buf, &st) && S_ISLNK(st.st_mode)) {
+			len = readlink(buf, next_buf, PATH_MAX);
+			if (len < 0)
+				die ("Invalid symlink: %s", buf);
+			next_buf[len] = '\0';
+			buf = next_buf;
+			buf_index = 1 - buf_index;
+			next_buf = bufs[buf_index];
+		} else
+			break;
+	}
+
+	if (*cwd && chdir(cwd))
+		die ("Could not change back to '%s'", cwd);
+
+	return buf;
+}
diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 4bba9c0..4e49d59 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -281,4 +281,20 @@ test_expect_success 'update-index D/F conflict' '
 	test $numpath0 = 1
 '
 
+test_expect_success 'absolute path works as expected' '
+	mkdir first &&
+	ln -s ../.git first/.git &&
+	mkdir second &&
+	ln -s ../first second/other &&
+	mkdir third &&
+	dir="$(cd .git; pwd -P)" &&
+	dir2=third/../second/other/.git &&
+	test "$dir" = "$(test-absolute-path $dir2)" &&
+	file="$dir"/index &&
+	test "$file" = "$(test-absolute-path $dir2/index)" &&
+	ln -s ../first/file .git/syml &&
+	sym="$(cd first; pwd -P)"/file &&
+	test "$sym" = "$(test-absolute-path $dir2/syml)"
+'
+
 test_done
diff --git a/test-absolute-path.c b/test-absolute-path.c
new file mode 100644
index 0000000..c959ea2
--- /dev/null
+++ b/test-absolute-path.c
@@ -0,0 +1,11 @@
+#include "cache.h"
+
+int main(int argc, char **argv)
+{
+	while (argc > 1) {
+		puts(make_absolute_path(argv[1]));
+		argc--;
+		argv++;
+	}
+	return 0;
+}
-- 
1.5.3.rc3.18.g49a1

[PATCH 2/8] Add functions get_relative_cwd() and is_inside_dir()

[Johannes Schindelin]

The function get_relative_cwd() works just as getcwd(), only that it
takes an absolute path as additional parameter, returning the prefix
of the current working directory relative to the given path.  If the
cwd is no subdirectory of the given path, it returns NULL.

is_inside_dir() is just a trivial wrapper over get_relative_cwd().

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
	This patch should be the same as I sent out earlier.  Just to
	make sure that you get the correct version, I send it again.

 dir.c |   30 ++++++++++++++++++++++++++++++
 dir.h |    3 +++
 2 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/dir.c b/dir.c
index 8d8faf5..c16a004 100644
--- a/dir.c
+++ b/dir.c
@@ -642,3 +642,33 @@ file_exists(const char *f)
   struct stat sb;
   return stat(f, &sb) == 0;
 }
+
+char *get_relative_cwd(char *buffer, int size, const char *dir)
+{
+	char *cwd = buffer;
+
+	if (!dir)
+		return 0;
+
+	if (!getcwd(buffer, PATH_MAX))
+		return 0;
+
+	if (!is_absolute_path(dir))
+		dir = make_absolute_path(dir);
+
+	while (*dir && *dir == *cwd) {
+		dir++;
+		cwd++;
+	}
+	if (*dir)
+		return NULL;
+	if (*cwd == '/')
+		return cwd + 1;
+	return cwd;
+}
+
+int is_inside_dir(const char *dir)
+{
+	char buffer[PATH_MAX];
+	return get_relative_cwd(buffer, sizeof(buffer), dir) != NULL;
+}
diff --git a/dir.h b/dir.h
index ec0e8ab..f55a87b 100644
--- a/dir.h
+++ b/dir.h
@@ -61,4 +61,7 @@ extern void add_exclude(const char *string, const char *base,
 extern int file_exists(const char *);
 extern struct dir_entry *dir_add_name(struct dir_struct *dir, const char *pathname, int len);
 
+extern char *get_relative_cwd(char *buffer, int size, const char *dir);
+extern int is_inside_dir(const char *dir);
+
 #endif
-- 
1.5.3.rc3.18.g49a1

[PATCH 3/8] Clean up work-tree handling

[Johannes Schindelin]

The old version of work-tree support was an unholy mess, barely readable,
and not to the point.

For example, why do you have to provide a worktree, when it is not used?
As in "git status".  Now it works.

Another riddle was: if you can have work trees inside the git dir, why
are some programs complaining that they need a work tree?

IOW when inside repo.git/work, where GIT_DIR points to repo.git
and GIT_WORK_TREE to work, and cwd is work, --is-inside-git-dir _must_
return true, because it is _in the git dir_, but scripts _must_ test
for the right thing.

Also, GIT_DIR=../.git should behave the same as if no GIT_DIR was
specified, unless there is a repository in the current working directory.
It does now.

In related news, a long standing bug was fixed: when in .git/bla/x.git/,
which is a bare repository, git formerly assumed ../.. to be the
appropriate git dir.  This problem was reported by Shawn Pearce to have
caused much pain, where a colleague mistakenly ran "git init" in "/" a
long time ago, and bare repositories just would not work.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
	Not only because of ohloh am I proud that in spite of removing
	more lines than I added, there were more comments added than
	removed...

 builtin-rev-parse.c |   12 +--
 cache.h             |    2 +
 environment.c       |   32 +++++--
 setup.c             |  270 ++++++++++++++++++++++-----------------------------
 4 files changed, 143 insertions(+), 173 deletions(-)

diff --git a/builtin-rev-parse.c b/builtin-rev-parse.c
index 497903a..3f787a8 100644
--- a/builtin-rev-parse.c
+++ b/builtin-rev-parse.c
@@ -320,15 +320,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 				continue;
 			}
 			if (!strcmp(arg, "--show-cdup")) {
-				const char *pfx = prefix;
-				while (pfx) {
-					pfx = strchr(pfx, '/');
-					if (pfx) {
-						pfx++;
-						printf("../");
-					}
-				}
-				putchar('\n');
+				const char *work_tree = get_git_work_tree();
+				if (work_tree)
+					printf("%s\n", work_tree);
 				continue;
 			}
 			if (!strcmp(arg, "--git-dir")) {
diff --git a/cache.h b/cache.h
index 98af530..c0cab34 100644
--- a/cache.h
+++ b/cache.h
@@ -208,12 +208,14 @@ enum object_type {
 extern int is_bare_repository_cfg;
 extern int is_bare_repository(void);
 extern int is_inside_git_dir(void);
+extern char *git_work_tree_cfg;
 extern int is_inside_work_tree(void);
 extern const char *get_git_dir(void);
 extern char *get_object_directory(void);
 extern char *get_refs_directory(void);
 extern char *get_index_file(void);
 extern char *get_graft_file(void);
+extern const char *get_git_work_tree(void);
 
 #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
 
diff --git a/environment.c b/environment.c
index f83fb9e..cb112a4 100644
--- a/environment.c
+++ b/environment.c
@@ -35,6 +35,10 @@ int pager_in_use;
 int pager_use_color = 1;
 int auto_crlf = 0;	/* 1: both ways, -1: only when adding git objects */
 
+/* This is set by setup_git_dir_gently() and/or git_default_config() */
+char *git_work_tree_cfg;
+static const char *work_tree;
+
 static const char *git_dir;
 static char *git_object_dir, *git_index_file, *git_refs_dir, *git_graft_file;
 
@@ -62,15 +66,8 @@ static void setup_git_env(void)
 
 int is_bare_repository(void)
 {
-	const char *dir, *s;
-	if (0 <= is_bare_repository_cfg)
-		return is_bare_repository_cfg;
-
-	dir = get_git_dir();
-	if (!strcmp(dir, DEFAULT_GIT_DIR_ENVIRONMENT))
-		return 0;
-	s = strrchr(dir, '/');
-	return !s || strcmp(s + 1, DEFAULT_GIT_DIR_ENVIRONMENT);
+	/* if core.bare is not 'false', let's see if there is a work tree */
+	return is_bare_repository_cfg && !get_git_work_tree();
 }
 
 const char *get_git_dir(void)
@@ -80,6 +77,23 @@ const char *get_git_dir(void)
 	return git_dir;
 }
 
+const char *get_git_work_tree(void)
+{
+	static int initialized = 0;
+	if (!initialized) {
+		work_tree = getenv(GIT_WORK_TREE_ENVIRONMENT);
+		if (!work_tree) {
+			work_tree = git_work_tree_cfg;
+			if (work_tree && !is_absolute_path(work_tree))
+				work_tree = xstrdup(git_path(work_tree));
+		}
+		if (work_tree && !is_absolute_path(work_tree))
+			work_tree = xstrdup(make_absolute_path(work_tree));
+		initialized = 1;
+	}
+	return work_tree;
+}
+
 char *get_object_directory(void)
 {
 	if (!git_object_dir)
diff --git a/setup.c b/setup.c
index 7b07144..0323d25 100644
--- a/setup.c
+++ b/setup.c
@@ -1,4 +1,8 @@
 #include "cache.h"
+#include "dir.h"
+
+static int inside_git_dir = -1;
+static int inside_work_tree = -1;
 
 const char *prefix_path(const char *prefix, int len, const char *path)
 {
@@ -170,100 +174,80 @@ static int is_git_directory(const char *suspect)
 	return 1;
 }
 
-static int inside_git_dir = -1;
-
 int is_inside_git_dir(void)
 {
-	if (inside_git_dir >= 0)
-		return inside_git_dir;
-	die("BUG: is_inside_git_dir called before setup_git_directory");
+	if (inside_git_dir < 0)
+		inside_git_dir = is_inside_dir(get_git_dir());
+	return inside_git_dir;
 }
 
-static int inside_work_tree = -1;
-
 int is_inside_work_tree(void)
 {
-	if (inside_git_dir >= 0)
-		return inside_work_tree;
-	die("BUG: is_inside_work_tree called before setup_git_directory");
+	if (inside_work_tree < 0)
+		inside_work_tree = is_inside_dir(get_git_work_tree());
+	return inside_work_tree;
 }
 
-static char *gitworktree_config;
-
-static int git_setup_config(const char *var, const char *value)
+/*
+ * If no worktree was given, and we are outside of a default work tree,
+ * now is the time to set it.
+ *
+ * In other words, if the user calls git with something like
+ *
+ *	git --git-dir=/some/where/else bla
+ *
+ * default to the current working directory for the work-tree root.
+ */
+const char* set_work_tree(const char *dir)
 {
-	if (!strcmp(var, "core.worktree")) {
-		if (gitworktree_config)
-			strlcpy(gitworktree_config, value, PATH_MAX);
-		return 0;
+	char dir_buffer[PATH_MAX];
+	static char buffer[PATH_MAX + 1], *rel;
+	int len, postfix_len = strlen(DEFAULT_GIT_DIR_ENVIRONMENT);
+
+	len = strlen(dir);
+	if (len > postfix_len + 1 && dir[len - postfix_len - 1] == '/' &&
+			!strcmp(dir + len - postfix_len,
+				DEFAULT_GIT_DIR_ENVIRONMENT))
+		dir = strncpy(dir_buffer, dir, len - postfix_len - 1);
+
+	rel = get_relative_cwd(buffer, sizeof(buffer), dir);
+	if (rel && *rel)
+		strcat(rel, "/");
+	else {
+		rel = NULL;
+		dir = getcwd(buffer, sizeof(buffer));
 	}
-	return git_default_config(var, value);
+	git_work_tree_cfg = xstrdup(dir);
+	inside_work_tree = 1;
+
+	return rel;
 }
 
+/*
+ * We cannot decide in this function whether we are in the work tree or
+ * not, since the config can only be read _after_ this function was called.
+ */
 const char *setup_git_directory_gently(int *nongit_ok)
 {
+	const char *work_tree_env = getenv(GIT_WORK_TREE_ENVIRONMENT);
 	static char cwd[PATH_MAX+1];
-	char worktree[PATH_MAX+1], gitdir[PATH_MAX+1];
-	const char *gitdirenv, *gitworktree;
-	int wt_rel_gitdir = 0;
+	const char *gitdirenv;
+	int len, offset;
 
+	/*
+	 * If GIT_DIR is set explicitly, we're not going
+	 * to do any discovery, but we still do repository
+	 * validation.
+	 */
 	gitdirenv = getenv(GIT_DIR_ENVIRONMENT);
-	if (!gitdirenv) {
-		int len, offset;
-
-		if (!getcwd(cwd, sizeof(cwd)-1))
-			die("Unable to read current working directory");
-
-		offset = len = strlen(cwd);
-		for (;;) {
-			if (is_git_directory(".git"))
-				break;
-			if (offset == 0) {
-				offset = -1;
-				break;
-			}
-			chdir("..");
-			while (cwd[--offset] != '/')
-				; /* do nothing */
-		}
-
-		if (offset >= 0) {
-			inside_work_tree = 1;
-			git_config(git_default_config);
-			if (offset == len) {
-				inside_git_dir = 0;
-				return NULL;
-			}
-
-			cwd[len++] = '/';
-			cwd[len] = '\0';
-			inside_git_dir = !prefixcmp(cwd + offset + 1, ".git/");
-			return cwd + offset + 1;
-		}
-
-		if (chdir(cwd))
-			die("Cannot come back to cwd");
-		if (!is_git_directory(".")) {
-			if (nongit_ok) {
-				*nongit_ok = 1;
-				return NULL;
-			}
-			die("Not a git repository");
-		}
-		setenv(GIT_DIR_ENVIRONMENT, cwd, 1);
-		gitdirenv = getenv(GIT_DIR_ENVIRONMENT);
-		if (!gitdirenv)
-			die("getenv after setenv failed");
-	}
-
-	if (PATH_MAX - 40 < strlen(gitdirenv)) {
-		if (nongit_ok) {
-			*nongit_ok = 1;
+	if (gitdirenv) {
+		if (PATH_MAX - 40 < strlen(gitdirenv))
+			die("'$%s' too big", GIT_DIR_ENVIRONMENT);
+		if (is_git_directory(gitdirenv)) {
+			if (!work_tree_env)
+				return set_work_tree(gitdirenv);
 			return NULL;
 		}
-		die("$%s too big", GIT_DIR_ENVIRONMENT);
-	}
-	if (!is_git_directory(gitdirenv)) {
 		if (nongit_ok) {
 			*nongit_ok = 1;
 			return NULL;
@@ -273,92 +257,53 @@ const char *setup_git_directory_gently(int *nongit_ok)
 
 	if (!getcwd(cwd, sizeof(cwd)-1))
 		die("Unable to read current working directory");
-	if (chdir(gitdirenv)) {
-		if (nongit_ok) {
-			*nongit_ok = 1;
-			return NULL;
-		}
-		die("Cannot change directory to $%s '%s'",
-			GIT_DIR_ENVIRONMENT, gitdirenv);
-	}
-	if (!getcwd(gitdir, sizeof(gitdir)-1))
-		die("Unable to read current working directory");
-	if (chdir(cwd))
-		die("Cannot come back to cwd");
 
 	/*
-	 * In case there is a work tree we may change the directory,
-	 * therefore make GIT_DIR an absolute path.
+	 * Test in the following order (relative to the cwd):
+	 * - .git/
+	 * - ./ (bare)
+	 * - ../.git/
+	 * - ../ (bare)
+	 * - ../../.git/
+	 *   etc.
 	 */
-	if (gitdirenv[0] != '/') {
-		setenv(GIT_DIR_ENVIRONMENT, gitdir, 1);
-		gitdirenv = getenv(GIT_DIR_ENVIRONMENT);
-		if (!gitdirenv)
-			die("getenv after setenv failed");
-		if (PATH_MAX - 40 < strlen(gitdirenv)) {
-			if (nongit_ok) {
-				*nongit_ok = 1;
-				return NULL;
-			}
-			die("$%s too big after expansion to absolute path",
-				GIT_DIR_ENVIRONMENT);
-		}
-	}
-
-	strcat(cwd, "/");
-	strcat(gitdir, "/");
-	inside_git_dir = !prefixcmp(cwd, gitdir);
-
-	gitworktree = getenv(GIT_WORK_TREE_ENVIRONMENT);
-	if (!gitworktree) {
-		gitworktree_config = worktree;
-		worktree[0] = '\0';
-	}
-	git_config(git_setup_config);
-	if (!gitworktree) {
-		gitworktree_config = NULL;
-		if (worktree[0])
-			gitworktree = worktree;
-		if (gitworktree && gitworktree[0] != '/')
-			wt_rel_gitdir = 1;
-	}
-
-	if (wt_rel_gitdir && chdir(gitdirenv))
-		die("Cannot change directory to $%s '%s'",
-			GIT_DIR_ENVIRONMENT, gitdirenv);
-	if (gitworktree && chdir(gitworktree)) {
-		if (nongit_ok) {
-			if (wt_rel_gitdir && chdir(cwd))
-				die("Cannot come back to cwd");
-			*nongit_ok = 1;
+	offset = len = strlen(cwd);
+	for (;;) {
+		if (is_git_directory(DEFAULT_GIT_DIR_ENVIRONMENT))
+			break;
+		if (is_git_directory(".")) {
+			inside_git_dir = 1;
+			if (!work_tree_env)
+				inside_work_tree = 0;
+			setenv(GIT_DIR_ENVIRONMENT, ".", 1);
 			return NULL;
 		}
-		if (wt_rel_gitdir)
-			die("Cannot change directory to working tree '%s'"
-				" from $%s", gitworktree, GIT_DIR_ENVIRONMENT);
-		else
-			die("Cannot change directory to working tree '%s'",
-				gitworktree);
-	}
-	if (!getcwd(worktree, sizeof(worktree)-1))
-		die("Unable to read current working directory");
-	strcat(worktree, "/");
-	inside_work_tree = !prefixcmp(cwd, worktree);
-
-	if (gitworktree && inside_work_tree && !prefixcmp(worktree, gitdir) &&
-	    strcmp(worktree, gitdir)) {
-		inside_git_dir = 0;
+		chdir("..");
+		do {
+			if (!offset) {
+				if (nongit_ok) {
+					if (chdir(cwd))
+						die("Cannot come back to cwd");
+					*nongit_ok = 1;
+					return NULL;
+				}
+				die("Not a git repository");
+			}
+		} while (cwd[--offset] != '/');
 	}
 
-	if (!inside_work_tree) {
-		if (chdir(cwd))
-			die("Cannot come back to cwd");
+	inside_git_dir = 0;
+	if (!work_tree_env)
+		inside_work_tree = 1;
+	git_work_tree_cfg = xstrndup(cwd, offset);
+	if (offset == len)
 		return NULL;
-	}
 
-	if (!strcmp(cwd, worktree))
-		return NULL;
-	return cwd+strlen(worktree);
+	/* Make "offset" point to past the '/', and add a '/' at the end */
+	offset++;
+	cwd[len++] = '/';
+	cwd[len] = 0;
+	return cwd + offset;
 }
 
 int git_config_perm(const char *var, const char *value)
@@ -382,11 +327,17 @@ int git_config_perm(const char *var, const char *value)
 
 int check_repository_format_version(const char *var, const char *value)
 {
-       if (strcmp(var, "core.repositoryformatversion") == 0)
-               repository_format_version = git_config_int(var, value);
+	if (strcmp(var, "core.repositoryformatversion") == 0)
+		repository_format_version = git_config_int(var, value);
 	else if (strcmp(var, "core.sharedrepository") == 0)
 		shared_repository = git_config_perm(var, value);
-       return 0;
+	else if (strcmp(var, "core.worktree") == 0) {
+		if (git_work_tree_cfg)
+			free(git_work_tree_cfg);
+		git_work_tree_cfg = xstrdup(value);
+		inside_work_tree = -1;
+	}
+	return 0;
 }
 
 int check_repository_format(void)
@@ -402,5 +353,14 @@ const char *setup_git_directory(void)
 {
 	const char *retval = setup_git_directory_gently(NULL);
 	check_repository_format();
+
+	/* If the work tree is not the default one, recompute prefix */
+	if (inside_work_tree < 0) {
+		static char buffer[PATH_MAX + 1];
+		char *rel = get_relative_cwd(buffer, PATH_MAX,
+				get_git_work_tree());
+		return rel && *rel ? strcat(rel, "/") : NULL;
+	}
+
 	return retval;
 }
-- 
1.5.3.rc3.18.g49a1

[PATCH 4/8] Add set_git_dir() function

[Johannes Schindelin]

With the function set_git_dir() you can reset the path that will
be used for git_path(), git_dir() and friends.

The responsibility to close files and throw away information from the
old git_dir lies with the caller.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	This is just ripped out from the branch--new-workdir topic
	I was working on, just that it has a shorter name now.

	Ah, and just to be sure, it xstrdup()s the path.

 cache.h       |    1 +
 environment.c |    8 ++++++++
 2 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/cache.h b/cache.h
index c0cab34..36963f2 100644
--- a/cache.h
+++ b/cache.h
@@ -216,6 +216,7 @@ extern char *get_refs_directory(void);
 extern char *get_index_file(void);
 extern char *get_graft_file(void);
 extern const char *get_git_work_tree(void);
+extern int set_git_dir(const char *path);
 
 #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
 
diff --git a/environment.c b/environment.c
index cb112a4..33c0f90 100644
--- a/environment.c
+++ b/environment.c
@@ -121,3 +121,11 @@ char *get_graft_file(void)
 		setup_git_env();
 	return git_graft_file;
 }
+
+int set_git_dir(const char *path)
+{
+	if (setenv(GIT_DIR_ENVIRONMENT, xstrdup(path), 1))
+		return error("Could not set GIT_DIR to '%s'", path);
+	setup_git_env();
+	return 0;
+}
-- 
1.5.3.rc3.18.g49a1

[PATCH 5/8] work-trees are allowed inside a git-dir

[Johannes Schindelin]

It is allowed to call

	$ git --git-dir=../ --work-tree=. bla

when you really want to.  In this case, you are both in the git directory
and in the working tree.

The earlier handling of this situation was seriously bogus.  For regular
working tree operations, it checked if inside git dir.  That makes no
sense, of course, since the check should be for a work tree, and nothing
else.

Fix that.

Coincidently, you can omit the "--work-tree" now, if your repository knows
about the location already.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin-ls-files.c |    8 +++++---
 git-sh-setup.sh    |    3 +--
 git.c              |   11 ++++++++---
 3 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/builtin-ls-files.c b/builtin-ls-files.c
index 61577ea..d36181a 100644
--- a/builtin-ls-files.c
+++ b/builtin-ls-files.c
@@ -469,9 +469,11 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)
 		break;
 	}
 
-	if (require_work_tree &&
-			(!is_inside_work_tree() || is_inside_git_dir()))
-		die("This operation must be run in a work tree");
+	if (require_work_tree && !is_inside_work_tree()) {
+		const char *work_tree = get_git_work_tree();
+		if (!work_tree || chdir(work_tree))
+			die("This operation must be run in a work tree");
+	}
 
 	pathspec = get_pathspec(prefix, argv + i);
 
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index c51985e..7bef43f 100755
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -59,8 +59,7 @@ cd_to_toplevel () {
 }
 
 require_work_tree () {
-	test $(git rev-parse --is-inside-work-tree) = true &&
-	test $(git rev-parse --is-inside-git-dir) = false ||
+	test $(git rev-parse --is-inside-work-tree) = true ||
 	die "fatal: $0 cannot be used without a working tree."
 }
 
diff --git a/git.c b/git.c
index a647f9c..2433355 100644
--- a/git.c
+++ b/git.c
@@ -272,9 +272,14 @@ static int run_command(struct cmd_struct *p, int argc, const char **argv)
 		prefix = setup_git_directory();
 	if (p->option & USE_PAGER)
 		setup_pager();
-	if ((p->option & NEED_WORK_TREE) &&
-	    (!is_inside_work_tree() || is_inside_git_dir()))
-		die("%s must be run in a work tree", p->cmd);
+	if (p->option & NEED_WORK_TREE) {
+		const char *work_tree = get_git_work_tree();
+		const char *git_dir = get_git_dir();
+		if (!is_absolute_path(git_dir))
+			set_git_dir(make_absolute_path(git_dir));
+		if (!work_tree || chdir(work_tree))
+			die("%s must be run in a work tree", p->cmd);
+	}
 	trace_argv_printf(argv, argc, "trace: built-in: git");
 
 	status = p->fn(argc, argv, prefix);
-- 
1.5.3.rc3.18.g49a1

[PATCH 6/8] init: use get_git_work_tree() instead of rolling our own

[Johannes Schindelin]

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	This is no change in function, but in readability.

 builtin-init-db.c |   48 +++++++++++-------------------------------------
 1 files changed, 11 insertions(+), 37 deletions(-)

diff --git a/builtin-init-db.c b/builtin-init-db.c
index 66ddaeb..0d9b1e0 100644
--- a/builtin-init-db.c
+++ b/builtin-init-db.c
@@ -174,36 +174,7 @@ static void copy_templates(const char *git_dir, int len, const char *template_di
 	closedir(dir);
 }
 
-/*
- * Get the full path to the working tree specified in $GIT_WORK_TREE
- * or NULL if no working tree is specified.
- */
-static const char *get_work_tree(void)
-{
-	const char *git_work_tree;
-	char cwd[PATH_MAX];
-	static char worktree[PATH_MAX];
-
-	git_work_tree = getenv(GIT_WORK_TREE_ENVIRONMENT);
-	if (!git_work_tree)
-		return NULL;
-	if (!getcwd(cwd, sizeof(cwd)))
-		die("Unable to read current working directory");
-	if (chdir(git_work_tree))
-		die("Cannot change directory to specified working tree '%s'",
-			git_work_tree);
-	if (git_work_tree[0] != '/') {
-		if (!getcwd(worktree, sizeof(worktree)))
-			die("Unable to read current working directory");
-		git_work_tree = worktree;
-	}
-	if (chdir(cwd))
-		die("Cannot come back to cwd");
-	return git_work_tree;
-}
-
-static int create_default_files(const char *git_dir, const char *git_work_tree,
-	const char *template_path)
+static int create_default_files(const char *git_dir, const char *template_path)
 {
 	unsigned len = strlen(git_dir);
 	static char path[PATH_MAX];
@@ -282,16 +253,16 @@ static int create_default_files(const char *git_dir, const char *git_work_tree,
 	}
 	git_config_set("core.filemode", filemode ? "true" : "false");
 
-	if (is_bare_repository() && !git_work_tree) {
+	if (is_bare_repository())
 		git_config_set("core.bare", "true");
-	}
 	else {
+		const char *work_tree = get_git_work_tree();
 		git_config_set("core.bare", "false");
 		/* allow template config file to override the default */
 		if (log_all_ref_updates == -1)
 		    git_config_set("core.logallrefupdates", "true");
-		if (git_work_tree)
-			git_config_set("core.worktree", git_work_tree);
+		if (work_tree != git_work_tree_cfg)
+			git_config_set("core.worktree", work_tree);
 	}
 	return reinit;
 }
@@ -308,7 +279,6 @@ static const char init_db_usage[] =
 int cmd_init_db(int argc, const char **argv, const char *prefix)
 {
 	const char *git_dir;
-	const char *git_work_tree;
 	const char *sha1_dir;
 	const char *template_dir = NULL;
 	char *path;
@@ -329,7 +299,11 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 			usage(init_db_usage);
 	}
 
-	git_work_tree = get_work_tree();
+	git_work_tree_cfg = xcalloc(PATH_MAX, 1);
+	if (!getcwd(git_work_tree_cfg, PATH_MAX))
+		die ("Cannot access current working directory.");
+	if (access(get_git_work_tree(), X_OK))
+		die ("Cannot access work tree '%s'", get_git_work_tree());
 
 	/*
 	 * Set up the default .git directory contents
@@ -346,7 +320,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 	 */
 	check_repository_format();
 
-	reinit = create_default_files(git_dir, git_work_tree, template_dir);
+	reinit = create_default_files(git_dir, template_dir);
 
 	/*
 	 * And set up the object store.
-- 
1.5.3.rc3.18.g49a1

[PATCH 7/8] Make t1501 a little saner, and fix it

[Johannes Schindelin]

t1501 is still a PITA to debug, but less so than before.  This patch
fixes also the tests which tested for the wrong (old) work-tree behavior.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t1501-worktree.sh |  161 +++++++++++++++++++++++++++++----------------------
 1 files changed, 92 insertions(+), 69 deletions(-)

diff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh
index aadeeab..fad9a9e 100755
--- a/t/t1501-worktree.sh
+++ b/t/t1501-worktree.sh
@@ -4,89 +4,112 @@ test_description='test separate work tree'
 . ./test-lib.sh
 
 test_rev_parse() {
-	name=$1
-	shift
-
-	test_expect_success "$name: is-bare-repository" \
-	"test '$1' = \"\$(git rev-parse --is-bare-repository)\""
-	shift
-	[ $# -eq 0 ] && return
-
-	test_expect_success "$name: is-inside-git-dir" \
-	"test '$1' = \"\$(git rev-parse --is-inside-git-dir)\""
-	shift
-	[ $# -eq 0 ] && return
-
-	test_expect_success "$name: is-inside-work-tree" \
-	"test '$1' = \"\$(git rev-parse --is-inside-work-tree)\""
-	shift
-	[ $# -eq 0 ] && return
-
-	test_expect_success "$name: prefix" \
-	"test '$1' = \"\$(git rev-parse --show-prefix)\""
-	shift
-	[ $# -eq 0 ] && return
+	name="$1"
+	for option in --is-bare-repository --is-inside-git-dir \
+		--is-inside-work-tree --show-prefix
+	do
+		shift
+		test_expect_success "$name: $option" \
+			"test '$1' = \"\$(git rev-parse $option)\""
+		test $# -eq 1 && return
+	done
 }
 
-mkdir -p work/sub/dir || exit 1
-mv .git repo.git || exit 1
+# usage: set_repo <working directory> [<git dir> [<work tree> [env]]]
+set_repo () {
+	cd "$1"
+	say "switching to $(pwd) with GIT_DIR $2"
+
+	test -z "$2" || {
+		GIT_DIR="$2"
+		GIT_CONFIG="$2"/config
+		export GIT_DIR GIT_CONFIG
+	}
+
+	test -z "$3" ||
+	case "$4" in
+	env)
+		GIT_WORK_TREE="$3"
+		export GIT_WORK_TREE
+		git config core.workTree non-existent
+	;;
+	*)
+		git config core.workTree "$3"
+	esac
+}
+
+test_expect_success 'setup' '
+	mkdir -p work/sub/dir &&
+	mv .git repo.git
+'
 
 say "core.worktree = relative path"
-export GIT_DIR=repo.git
-export GIT_CONFIG=$GIT_DIR/config
-unset GIT_WORK_TREE
-git config core.worktree ../work
-test_rev_parse 'outside'      false false false
-cd work || exit 1
-export GIT_DIR=../repo.git
-export GIT_CONFIG=$GIT_DIR/config
-test_rev_parse 'inside'       false false true ''
-cd sub/dir || exit 1
-export GIT_DIR=../../../repo.git
-export GIT_CONFIG=$GIT_DIR/config
+
+set_repo . repo.git ../work
+test_rev_parse 'outside git dir' false false false
+
+set_repo work ../repo.git
+test_rev_parse 'inside git dir' false false true ''
+
+set_repo sub/dir ../../../repo.git
 test_rev_parse 'subdirectory' false false true sub/dir/
-cd ../../.. || exit 1
+cd ../../..
 
 say "core.worktree = absolute path"
-export GIT_DIR=$(pwd)/repo.git
-export GIT_CONFIG=$GIT_DIR/config
-git config core.worktree "$(pwd)/work"
-test_rev_parse 'outside'      false false false
-cd work || exit 1
-test_rev_parse 'inside'       false false true ''
-cd sub/dir || exit 1
+set_repo . $(pwd)/repo.git $(pwd)/work
+test_rev_parse 'outside git dir' false false false
+
+set_repo work
+test_rev_parse 'inside git dir' false false true ''
+
+set_repo sub/dir
 test_rev_parse 'subdirectory' false false true sub/dir/
-cd ../../.. || exit 1
+cd ../../..
 
 say "GIT_WORK_TREE=relative path (override core.worktree)"
-export GIT_DIR=$(pwd)/repo.git
-export GIT_CONFIG=$GIT_DIR/config
-git config core.worktree non-existent
-export GIT_WORK_TREE=work
-test_rev_parse 'outside'      false false false
-cd work || exit 1
-export GIT_WORK_TREE=.
-test_rev_parse 'inside'       false false true ''
-cd sub/dir || exit 1
-export GIT_WORK_TREE=../..
+
+set_repo . $(pwd)/repo.git work env
+test_rev_parse 'outside git dir' false false false
+
+set_repo work "" . env
+test_rev_parse 'inside git dir' false false true ''
+
+set_repo sub/dir "" ../../ env
 test_rev_parse 'subdirectory' false false true sub/dir/
 cd ../../.. || exit 1
 
+say "GIT_WORK_TREE=absolute path, work tree below git dir"
+
 mv work repo.git/work
 
-say "GIT_WORK_TREE=absolute path, work tree below git dir"
-export GIT_DIR=$(pwd)/repo.git
-export GIT_CONFIG=$GIT_DIR/config
-export GIT_WORK_TREE=$(pwd)/repo.git/work
-test_rev_parse 'outside'              false false false
-cd repo.git || exit 1
-test_rev_parse 'in repo.git'              false true  false
-cd objects || exit 1
-test_rev_parse 'in repo.git/objects'      false true  false
-cd ../work || exit 1
-test_rev_parse 'in repo.git/work'         false false true ''
-cd sub/dir || exit 1
-test_rev_parse 'in repo.git/sub/dir' false false true sub/dir/
-cd ../../../.. || exit 1
+set_repo . $(pwd)/repo.git $(pwd)/repo.git/work env
+test_rev_parse 'outside git dir' false false false
+
+set_repo repo.git
+test_rev_parse 'in repo.git' false true false
+
+set_repo objects
+test_rev_parse 'in repo.git/objects' false true  false
+
+set_repo ../work
+test_rev_parse 'in repo.git/work' false true true ''
+
+set_repo sub/dir
+test_rev_parse 'in repo.git/sub/dir' false true true sub/dir/
+cd ../../../..
+
+test_expect_success 'repo finds its work tree' '
+	(cd repo.git &&
+	 : > work/sub/dir/untracked &&
+	 test sub/dir/untracked = "$(git ls-files --others)")
+'
+
+test_expect_success 'repo finds its work tree from work tree, too' '
+	(cd repo.git/work/sub/dir &&
+	 : > tracked &&
+	 git --git-dir=../../.. add tracked &&
+	 cd ../../.. &&
+	 test sub/dir/tracked = "$(git ls-files)")
+'
 
 test_done
-- 
1.5.3.rc3.18.g49a1

[PATCH 8/8] Fix t1500 for sane work-tree behavior

[Johannes Schindelin]

When GIT_DIR=../.git, and no worktree is specified, it is reasonable
to assume that the repository is not bare, that the work tree is ".."
and that the prefix is the basename of the current directory.

This is the sane behavior.

t1500 tested for the old behavior, which was plain wrong.  And this
patch fixes it minimally.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
	Yeah, I broke down.  But fixing t1501's style was painful enough.

 t/t1500-rev-parse.sh |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index ec49966..6e792b6 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -31,9 +31,9 @@ test_rev_parse() {
 test_rev_parse toplevel false false true ''
 
 cd .git || exit 1
-test_rev_parse .git/ false true true .git/
+test_rev_parse .git/ true true false ''
 cd objects || exit 1
-test_rev_parse .git/objects/ false true true .git/objects/
+test_rev_parse .git/objects/ true true false ''
 cd ../.. || exit 1
 
 mkdir -p sub/dir || exit 1
@@ -42,7 +42,7 @@ test_rev_parse subdirectory false false true sub/dir/
 cd ../.. || exit 1
 
 git config core.bare true
-test_rev_parse 'core.bare = true' true false true
+test_rev_parse 'core.bare = true' false false true
 
 git config --unset core.bare
 test_rev_parse 'core.bare undefined' false false true
@@ -53,13 +53,13 @@ export GIT_DIR=../.git
 export GIT_CONFIG="$GIT_DIR"/config
 
 git config core.bare false
-test_rev_parse 'GIT_DIR=../.git, core.bare = false' false false true ''
+test_rev_parse 'GIT_DIR=../.git, core.bare = false' false false true work/
 
 git config core.bare true
-test_rev_parse 'GIT_DIR=../.git, core.bare = true' true false true ''
+test_rev_parse 'GIT_DIR=../.git, core.bare = true' false false true work/
 
 git config --unset core.bare
-test_rev_parse 'GIT_DIR=../.git, core.bare undefined' false false true ''
+test_rev_parse 'GIT_DIR=../.git, core.bare undefined' false false true work/
 
 mv ../.git ../repo.git || exit 1
 export GIT_DIR=../repo.git
@@ -69,9 +69,9 @@ git config core.bare false
 test_rev_parse 'GIT_DIR=../repo.git, core.bare = false' false false true ''
 
 git config core.bare true
-test_rev_parse 'GIT_DIR=../repo.git, core.bare = true' true false true ''
+test_rev_parse 'GIT_DIR=../repo.git, core.bare = true' false false true ''
 
 git config --unset core.bare
-test_rev_parse 'GIT_DIR=../repo.git, core.bare undefined' true false true ''
+test_rev_parse 'GIT_DIR=../repo.git, core.bare undefined' false false true ''
 
 test_done
-- 
1.5.3.rc3.18.g49a1

Re: [PATCH 1/8] Add is_absolute_path() and make_absolute_path()

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> diff --git a/path.c b/path.c
> index c4ce962..0f7012f 100644
> --- a/path.c
> +++ b/path.c
> @@ -292,3 +292,65 @@ int adjust_shared_perm(const char *path)
>  		return -2;
>  	return 0;
>  }
> +
> +/* We allow "recursive" symbolic links. Only within reason, though. */
> +#define MAXDEPTH 5
> +
> +const char *make_absolute_path(const char *path)
> +{
> +	static char bufs[2][PATH_MAX + 1], *buf = bufs[0], *next_buf = bufs[1];
> +	char cwd[1024] = "";
> +	int buf_index = 1, len;
> +
> +	int depth = MAXDEPTH;
> +	char *last_elem = NULL;
> +	struct stat st;
> +
> +	if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX)
> +		die ("Too long path: %.*s", 60, path);
> +
> +	while (depth--) {
> +		if (stat(buf, &st) || !S_ISDIR(st.st_mode)) {
> +			char *last_slash = strrchr(buf, '/');
> +			*last_slash = '\0';
> +			last_elem = xstrdup(last_slash + 1);

What happens when incoming path is just "abc"?  Does your test
script checks that case?

Re: [PATCH 2/8] Add functions get_relative_cwd() and is_inside_dir()

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> +char *get_relative_cwd(char *buffer, int size, const char *dir)
> +{
> +	char *cwd = buffer;
> +
> +	if (!dir)
> +		return 0;
> +
> +	if (!getcwd(buffer, PATH_MAX))
> +		return 0;

Can size be less than PATH_MAX?

Re: [PATCH 3/8] Clean up work-tree handling

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> 	Not only because of ohloh am I proud that in spite of removing
> 	more lines than I added, there were more comments added than
> 	removed...

> diff --git a/builtin-rev-parse.c b/builtin-rev-parse.c
> index 497903a..3f787a8 100644
> --- a/builtin-rev-parse.c
> +++ b/builtin-rev-parse.c
> @@ -320,15 +320,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
>  				continue;
>  			}
>  			if (!strcmp(arg, "--show-cdup")) {
> -				const char *pfx = prefix;
> -				while (pfx) {
> -					pfx = strchr(pfx, '/');
> -					if (pfx) {
> -						pfx++;
> -						printf("../");
> -					}
> -				}
> -				putchar('\n');
> +				const char *work_tree = get_git_work_tree();
> +				if (work_tree)
> +					printf("%s\n", work_tree);
>  				continue;

This changes semantics, I think.

It used to be relative "up" path when no funny work-tree stuff
is used, but get_git_work_tree() now seems to return absolute,
hence this option as well.  If it introduces regression to
existing callers is up to what the caller does to the resulting
path, though.  If it only is used to prefix other things
(i.e. path="$(git rev-parse --show-cdup)$1"), the caller would
be safe, but if the caller counted number of ../ in the return
value to see how deep it is, or if the caller expected to see
empty string in order to see if the process is at the toplevel,
this change would become a regression.

> @@ -62,15 +66,8 @@ static void setup_git_env(void)
>  
>  int is_bare_repository(void)
>  {
> +	/* if core.bare is not 'false', let's see if there is a work tree */
> +	return is_bare_repository_cfg && !get_git_work_tree();
>  }

I thought about making core.bare a tertiary, true/false/depends,
but I think this makes more sense.

Re: [PATCH 5/8] work-trees are allowed inside a git-dir

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> It is allowed to call
>
> 	$ git --git-dir=../ --work-tree=. bla
>
> when you really want to.  In this case, you are both in the git directory
> and in the working tree.
>
> The earlier handling of this situation was seriously bogus.  For regular
> working tree operations, it checked if inside git dir.  That makes no
> sense, of course, since the check should be for a work tree, and nothing
> else.
>
> Fix that.

I do not doubt this patch makes the above command line to work
better, but I have to wonder how that layout is useful.  Care to
give a use case or two in the commit log message?

Re: [PATCH 8/8] Fix t1500 for sane work-tree behavior

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> When GIT_DIR=../.git, and no worktree is specified, it is reasonable
> to assume that the repository is not bare, that the work tree is ".."
> and that the prefix is the basename of the current directory.
>
> This is the sane behavior.

That is a bit too strong blanket statement, while being weak on
exact conditions, giving only one example.

It makes me wonder...

  * When GIT_DIR=../../.git, and no worktree is specified, the
    same holds true, with worktree is "../.."? (probably yes)

  * "GIT_DIR=../../foo/.git"? (I dunno)

  * "GIT_DIR=../foo.git"? (probably not)

I am assuming that you meant something like this:

    When no worktree is specified, and GIT_DIR (or --git-dir=) is
    zero or more "../" followed by ".git" after stripping trailing
    and/or redundant slashes, it is reasonable to assume that the
    repository is not bare, and the work tree is the parent
    directory of the GIT_DIR directory.

but that requires guesswork if you give only one example and let
the readers to guess.

Re: [PATCH 3/8] Clean up work-tree handling

[Johannes Schindelin]

Hi,

On Fri, 27 Jul 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > 	Not only because of ohloh am I proud that in spite of removing
> > 	more lines than I added, there were more comments added than
> > 	removed...
> 
> > diff --git a/builtin-rev-parse.c b/builtin-rev-parse.c
> > index 497903a..3f787a8 100644
> > --- a/builtin-rev-parse.c
> > +++ b/builtin-rev-parse.c
> > @@ -320,15 +320,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> >  				continue;
> >  			}
> >  			if (!strcmp(arg, "--show-cdup")) {
> > -				const char *pfx = prefix;
> > -				while (pfx) {
> > -					pfx = strchr(pfx, '/');
> > -					if (pfx) {
> > -						pfx++;
> > -						printf("../");
> > -					}
> > -				}
> > -				putchar('\n');
> > +				const char *work_tree = get_git_work_tree();
> > +				if (work_tree)
> > +					printf("%s\n", work_tree);
> >  				continue;
> 
> This changes semantics, I think.
> 
> It used to be relative "up" path when no funny work-tree stuff
> is used, but get_git_work_tree() now seems to return absolute,
> hence this option as well.  If it introduces regression to
> existing callers is up to what the caller does to the resulting
> path, though.  If it only is used to prefix other things
> (i.e. path="$(git rev-parse --show-cdup)$1"), the caller would
> be safe, but if the caller counted number of ../ in the return
> value to see how deep it is, or if the caller expected to see
> empty string in order to see if the process is at the toplevel,
> this change would become a regression.

I am somewhat negative on keeping _that_ much backwards compatibility.  
Scripts which depend on show-cdup being a relative path _will_ be broken 
by work-tree.  Is it worth it to detect those errors late?

> > @@ -62,15 +66,8 @@ static void setup_git_env(void)
> >  
> >  int is_bare_repository(void)
> >  {
> > +	/* if core.bare is not 'false', let's see if there is a work tree */
> > +	return is_bare_repository_cfg && !get_git_work_tree();
> >  }
> 
> I thought about making core.bare a tertiary, true/false/depends,
> but I think this makes more sense.

Actually, you made me think again, and I am more along those lines now:

	return is_bare_repository_cfg >= 0 ?
		is_bare_repository_cfg : !get_git_work_tree();

and according patch to get_git_work_tree to return NULL if 
is_bare_repository_cfg == 1.

Ciao,
Dscho

Re: [PATCH 5/8] work-trees are allowed inside a git-dir

[Johannes Schindelin]

Hi,

On Fri, 27 Jul 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > It is allowed to call
> >
> > 	$ git --git-dir=../ --work-tree=. bla
> >
> > when you really want to.  In this case, you are both in the git directory
> > and in the working tree.
> >
> > The earlier handling of this situation was seriously bogus.  For regular
> > working tree operations, it checked if inside git dir.  That makes no
> > sense, of course, since the check should be for a work tree, and nothing
> > else.
> >
> > Fix that.
> 
> I do not doubt this patch makes the above command line to work
> better, but I have to wonder how that layout is useful.  Care to
> give a use case or two in the commit log message?

In the commit log message?  Better somewhere else.  Only git developers 
read the commit message.

But yes, I can point to a use case.  AFAIR Martin Krafft brought up the 
issue to track different components of the home directory in different 
repositories.

I have a similar scenario here, which does not involve a home directory, 
but rather a directory where I should not put anything into (I could, but 
if the admin was anything akin to competent, I could not).

There are files in that directory (and all of its subdirectories) of a 
certain type, which are the only ones which are human generated, and 
therefore precious.  I like to add them, and inspect them, with

	git --git-dir=$HOME/x.git add

and

	git --git-dir=$HOME/x.git diff

Another similar scenario is a network drive on Losedows, where the locking 
always fails.  So I do not _want_ a repo there, even if I _could_.

Ciao,
Dscho

Re: [PATCH 3/8] Clean up work-tree handling

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> This changes semantics, I think.
>> 
>> It used to be relative "up" path when no funny work-tree stuff
>> is used, but get_git_work_tree() now seems to return absolute,
>> hence this option as well.  If it introduces regression to
>> existing callers is up to what the caller does to the resulting
>> path, though.  If it only is used to prefix other things
>> (i.e. path="$(git rev-parse --show-cdup)$1"), the caller would
>> be safe, but if the caller counted number of ../ in the return
>> value to see how deep it is, or if the caller expected to see
>> empty string in order to see if the process is at the toplevel,
>> this change would become a regression.
>
> I am somewhat negative on keeping _that_ much backwards compatibility.  
> Scripts which depend on show-cdup being a relative path _will_ be broken 
> by work-tree.  Is it worth it to detect those errors late?

Well, one of the conditions to accept the worktree stuff was not
to break anybody who never ever uses worktree.  So if we can
keep the UP-ness of cdup, it would be much better.

Re: [PATCH 8/8] Fix t1500 for sane work-tree behavior

[Johannes Schindelin]

Hi,

On Fri, 27 Jul 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > When GIT_DIR=../.git, and no worktree is specified, it is reasonable
> > to assume that the repository is not bare, that the work tree is ".."
> > and that the prefix is the basename of the current directory.
> >
> > This is the sane behavior.
> 
> That is a bit too strong blanket statement, while being weak on
> exact conditions, giving only one example.

Okay, let me defend it.

> It makes me wonder...
> 
>   * When GIT_DIR=../../.git, and no worktree is specified, the
>     same holds true, with worktree is "../.."? (probably yes)

You meant with "GIT_DIR=../.."? No.  In that case, I'd assume a bare 
repository, and we're inside the git directory, and unless the user 
specified a working tree, assume that we have none.

>   * "GIT_DIR=../../foo/.git"? (I dunno)

Unless ../../foo == .. no.  When we're outside, we're outside.

>   * "GIT_DIR=../foo.git"? (probably not)

Unless "$(basename "$(pwd)")" == foo.git, no.

> I am assuming that you meant something like this:
> 
>     When no worktree is specified, and GIT_DIR (or --git-dir=) is
>     zero or more "../" followed by ".git" after stripping trailing
>     and/or redundant slashes, it is reasonable to assume that the
>     repository is not bare, and the work tree is the parent
>     directory of the GIT_DIR directory.
> 
> but that requires guesswork if you give only one example and let
> the readers to guess.

Your explanation is really much more coherent than mine.  Please replace 
mine.

Ciao,
Dscho

Re: [PATCH 5/8] work-trees are allowed inside a git-dir

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> 
>> > It is allowed to call
>> >
>> > 	$ git --git-dir=../ --work-tree=. bla
>> >
>> > when you really want to.  In this case, you are both in the git directory
>> > and in the working tree.
> ...
> There are files in that directory (and all of its subdirectories) of a 
> certain type, which are the only ones which are human generated, and 
> therefore precious.  I like to add them, and inspect them, with
>
> 	git --git-dir=$HOME/x.git add
>
> and
>
> 	git --git-dir=$HOME/x.git diff

I understand the --git-dir=$HOME/x.git to keep track of
something in $HOME/foo/bar example.

But that is not the issue you described in the original message.
I was asking about this (which is the way I read your original
message):

    $ GIT_DIR=$HOME/x.git git init
    $ mkdir $HOME/x.git/workroot
    $ cd $HOME/x.git/workroot
    $ git --git-dir=../ --work-tree=. 

That is, $HOME/x.git/ is the GIT_DIR that has HEAD, index and
refs/, and you are keeping track of contents whose rootlevel is
at $HOME/x.git/workroot

Re: [PATCH 3/8] Clean up work-tree handling

[Johannes Schindelin]

Hi,

On Fri, 27 Jul 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> This changes semantics, I think.
> >> 
> >> It used to be relative "up" path when no funny work-tree stuff
> >> is used, but get_git_work_tree() now seems to return absolute,
> >> hence this option as well.  If it introduces regression to
> >> existing callers is up to what the caller does to the resulting
> >> path, though.  If it only is used to prefix other things
> >> (i.e. path="$(git rev-parse --show-cdup)$1"), the caller would
> >> be safe, but if the caller counted number of ../ in the return
> >> value to see how deep it is, or if the caller expected to see
> >> empty string in order to see if the process is at the toplevel,
> >> this change would become a regression.
> >
> > I am somewhat negative on keeping _that_ much backwards compatibility.  
> > Scripts which depend on show-cdup being a relative path _will_ be broken 
> > by work-tree.  Is it worth it to detect those errors late?
> 
> Well, one of the conditions to accept the worktree stuff was not
> to break anybody who never ever uses worktree.  So if we can
> keep the UP-ness of cdup, it would be much better.

One could record if the work tree was changed from the default one, and 
do the old thing in that case.

But I really have to wonder what other use people concocted for 
"--show-cdup"?  Potentially some directory-counting?  But --show-prefix 
would be much better at that.

I'll try to flange something into the code to detect unchanged working 
tree, but that is rather ugly, so I'd prefer not to.

Ciao,
Dscho

Re: [PATCH 5/8] work-trees are allowed inside a git-dir

[Johannes Schindelin]

Hi,

On Fri, 27 Jul 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >> 
> >> > It is allowed to call
> >> >
> >> > 	$ git --git-dir=../ --work-tree=. bla
> >> >
> >> > when you really want to.  In this case, you are both in the git directory
> >> > and in the working tree.
> > ...
> > There are files in that directory (and all of its subdirectories) of a 
> > certain type, which are the only ones which are human generated, and 
> > therefore precious.  I like to add them, and inspect them, with
> >
> > 	git --git-dir=$HOME/x.git add
> >
> > and
> >
> > 	git --git-dir=$HOME/x.git diff
> 
> I understand the --git-dir=$HOME/x.git to keep track of
> something in $HOME/foo/bar example.
> 
> But that is not the issue you described in the original message.
> I was asking about this (which is the way I read your original
> message):
> 
>     $ GIT_DIR=$HOME/x.git git init
>     $ mkdir $HOME/x.git/workroot
>     $ cd $HOME/x.git/workroot
>     $ git --git-dir=../ --work-tree=. 
> 
> That is, $HOME/x.git/ is the GIT_DIR that has HEAD, index and
> refs/, and you are keeping track of contents whose rootlevel is
> at $HOME/x.git/workroot

Ah!  But I have a really nice use case for that, too.  I track a 
.git/refs/exclude in one of my branches, because I do not want anybody 
else to have those excludes.  They only apply to me.

Another example would be a temporary checkout+change+checkin to some 
branch that is not currently checked out in the default working tree.  (I 
do that, too, and had to work around that by cloning with "-l -n -s")  
What I do there is to keep a checkout of some often-rsync'ed (actually 
wget'ed) state in the current working tree, automatically committing when 
upstream changes, and tracking upstream _releases_ in a different branch.

Ciao,
Dscho

Re: [PATCH 1/8] Add is_absolute_path() and make_absolute_path()

[Johannes Schindelin]

Hi,

On Fri, 27 Jul 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > diff --git a/path.c b/path.c
> > index c4ce962..0f7012f 100644
> > --- a/path.c
> > +++ b/path.c
> > @@ -292,3 +292,65 @@ int adjust_shared_perm(const char *path)
> >  		return -2;
> >  	return 0;
> >  }
> > +
> > +/* We allow "recursive" symbolic links. Only within reason, though. */
> > +#define MAXDEPTH 5
> > +
> > +const char *make_absolute_path(const char *path)
> > +{
> > +	static char bufs[2][PATH_MAX + 1], *buf = bufs[0], *next_buf = bufs[1];
> > +	char cwd[1024] = "";
> > +	int buf_index = 1, len;
> > +
> > +	int depth = MAXDEPTH;
> > +	char *last_elem = NULL;
> > +	struct stat st;
> > +
> > +	if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX)
> > +		die ("Too long path: %.*s", 60, path);
> > +
> > +	while (depth--) {
> > +		if (stat(buf, &st) || !S_ISDIR(st.st_mode)) {
> > +			char *last_slash = strrchr(buf, '/');
> > +			*last_slash = '\0';
> > +			last_elem = xstrdup(last_slash + 1);
> 
> What happens when incoming path is just "abc"?  Does your test
> script checks that case?

No, and you already guessed it: there will be a segmentation fault.

Will fix.

Ciao,
Dscho

Re: [PATCH 2/8] Add functions get_relative_cwd() and is_inside_dir()

[Johannes Schindelin]

Hi,

On Fri, 27 Jul 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > +char *get_relative_cwd(char *buffer, int size, const char *dir)
> > +{
> > +	char *cwd = buffer;
> > +
> > +	if (!dir)
> > +		return 0;
> > +
> > +	if (!getcwd(buffer, PATH_MAX))
> > +		return 0;
> 
> Can size be less than PATH_MAX?

Thanks.  Will fix.

Ciao,
Dscho

Re: [PATCH 3/8] Clean up work-tree handling

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> Well, one of the conditions to accept the worktree stuff was not
>> to break anybody who never ever uses worktree.  So if we can
>> keep the UP-ness of cdup, it would be much better.
>
> One could record if the work tree was changed from the default one, and 
> do the old thing in that case.

This really should not be something you have to bend over with
special code to support.  One thing I think people's scripts
expect is that cdup is what the name suggests, "how much do I go
*up* in order to go to the top of the working tree", and if the
answer is "nothing", they can tell that they are already at the
top of the tree.

Now, I think it is fair to say that if your worktree is
somewhere totally unrelated to your cwd, no amount of going up
will take you to the top.  IOW, you have to come down after
going up some levels.  In such a case, it is easier to code the
implementation of --show-cdup to give an absolute path.

But in that case you are not even in the working tree to begin
with, aren't you?  Does git need to support that?  What should
"prefix = setup_git_dir()" do and return, in order to come up
with the string to prepend to paths relative to the current
directory the users give us?  I would say if your cwd is not
inside your working tree, then --show-cdup and --show-prefix
should error out, saying "Not in the working tree".  These
options are about translating paths relative to your cwd to
relative to the top of the working tree, so if you are outside a
working tree, they do not make much sense.

Also I think the implementation of --show-prefix needs to work
in terms of relative relationship between cwd and the top
already, so I am not sure where your aversion of keeping the
current cdup behaviour is coming from.

I was hoping that --show-cdup is rarely used, but unfortunately
cd_to_toplevel() in git-sh-setup.sh is not even the only in-tree
user of --show-cdup; it checks if it is empty to avoid chdir()
unnecessarily.  There are others:

 - emacs/git.el uses it as part of expand-file-name, so I think
   giving an absolute path back is Ok.

 - contrib/fast-import/git-p4 gives it to os.chdir() after
   checking if the length is positive; this should be Ok
   although it would involve an unnecessary chdir() if the
   process is already at the top with this change.

 - contrib/p4import/git-p4import.py: the same story as git-p4.

 - git-svn.perl: the same story as above.

Re: [PATCH 3/8] Clean up work-tree handling

[Johannes Schindelin]

Hi,

On Fri, 27 Jul 2007, Junio C Hamano wrote:

> Now, I think it is fair to say that if your worktree is somewhere 
> totally unrelated to your cwd, no amount of going up will take you to 
> the top.  IOW, you have to come down after going up some levels.  In 
> such a case, it is easier to code the implementation of --show-cdup to 
> give an absolute path.
> 
> But in that case you are not even in the working tree to begin
> with, aren't you?  Does git need to support that?

I'd say yes.

It is utterly _inconvenient_ to have to cd to the working tree when you 
just want to check the status, for example.  And git already knows about 
the work-tree!

But you got me convinced about the relative path: it is true that nobody 
who has not set core.worktree should be affected.

So I will do something like

	if (!inside_work_tree()) {
		puts(get_git_work_tree());
		continue;
	}
	[do the old thing of outputting ../../[...]]

In fact, I had this in an unpublished version of the patch, and decided 
that I could remove more lines without breaking the test suite.

Heck, I'll even add a test case to make sure that behavior is maintained.  
Okay?

Ciao,
Dscho

P.S.: I'll be offline for a few hours, but then come back to finish it up.

Re: [PATCH 3/8] Clean up work-tree handling

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> Now, I think it is fair to say that if your worktree is somewhere 
>> totally unrelated to your cwd, no amount of going up will take you to 
>> the top.  IOW, you have to come down after going up some levels.  In 
>> such a case, it is easier to code the implementation of --show-cdup to 
>> give an absolute path.
>> 
>> But in that case you are not even in the working tree to begin
>> with, aren't you?  Does git need to support that?
>
> I'd say yes.
>
> It is utterly _inconvenient_ to have to cd to the working tree when you 
> just want to check the status, for example.  And git already knows about 
> the work-tree!

That is a bit different issue. In such a case, I would agree it
would still be sensible for "git diff" to show the whole tree
diff, for example.  In other words, my "does git need to support
that" is not about "even when the location of a worktree is
known, operations that require a work tree should fail when
invoked from outside of it".

Think about what the sensible thing to do is for "git diff ."
Within a work tree, it instructs git to behave as if the user
issued the command from the toplevel with prefix as the
argument.  And scripts that want to mimick that behaviour would
use --show-prefix to obtain the prefix string, and --show-cdup
to obtain how to move to the toplevel.

Outside of a work tree, I think the only two semi-sensible
behaviours exist.  Either tell the user that we cannot
understand what "." should mean in that context and error out
(IOW, "Not inside a work tree"), or assume that the user meant
"from the top".  So in such a case, if we do not want to error
out to make things more "convenient", one possibility would be:

 * give empty as prefix;

 * give absolute or cwd relative path to the work tree for cdup
   (it would not be a sequence of ../ anyway in this case).

I think this is in line with the traditional behaviour when
GIT_DIR is explicitly given.  We assume the cwd is the toplevel,
and return empty prefix and empty cdup.

Having said that, I am not convinced that "assume toplevel
outside of a work tree" is a win for the end users.  If the
command errors out with a message telling the user that relative
path does not make sense from outside a work tree, the user
would understand.  If the command does not error out but always
works relative to the toplevel without explanation, it might
confuse the user more until the he realizes "assume toplevel
outside of a work tree" is the rule that is applying to his
case.

Re: [PATCH 3/8] Clean up work-tree handling

[Johannes Schindelin]

Hi,

On Sat, 28 Jul 2007, Junio C Hamano wrote:

> Outside of a work tree, I think the only two semi-sensible behaviours 
> exist.  Either tell the user that we cannot understand what "." should 
> mean in that context and error out (IOW, "Not inside a work tree"), or 
> assume that the user meant "from the top".  So in such a case, if we do 
> not want to error out to make things more "convenient", one possibility 
> would be:
> 
>  * give empty as prefix;
> 
>  * give absolute or cwd relative path to the work tree for cdup
>    (it would not be a sequence of ../ anyway in this case).

I am happy that you agree with the way I implemented it...

> I think this is in line with the traditional behaviour when GIT_DIR is 
> explicitly given.  We assume the cwd is the toplevel, and return empty 
> prefix and empty cdup.

Yes, I want that behaviour.

> Having said that, I am not convinced that "assume toplevel outside of a 
> work tree" is a win for the end users.  If the command errors out with a 
> message telling the user that relative path does not make sense from 
> outside a work tree, the user would understand.  If the command does not 
> error out but always works relative to the toplevel without explanation, 
> it might confuse the user more until the he realizes "assume toplevel 
> outside of a work tree" is the rule that is applying to his case.

There are two reasons against that...

- it is really convenient to be able to say "git add .vimrc" (think Martin 
  Krafft's desired setup).  Let's just put some uppercase warning at the 
  side of --work-tree in git-init.txt to tell the user about the 
  behaviour.

- the implementation of "you are not in the worktree" would not be 
  elegant: "git log" should succeed, "git log dir/file" should not.

Ciao,
Dscho

Re: [PATCH 3/8] Clean up work-tree handling

[Johannes Schindelin]

Hi,

I still have a problem with what should happen if both "core.bare == true" 
and "core.worktree = /some/where/over/the/rainbow".  Should it be bare, or 
should it have a working tree?

So here is what I plan to support so far:

--work-tree= overrides GIT_WORK_TREE, which overrides core.worktree, which 
overrides core.bare, which overrides GIT_DIR/.. when GIT_DIR ends in 
/.git, which overrides the directory in which .git/ was found.

Does that look okay?

Ciao,
Dscho

Re: [PATCH 3/8] Clean up work-tree handling

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> I still have a problem with what should happen if both "core.bare == true" 
> and "core.worktree = /some/where/over/the/rainbow".  Should it be bare, or 
> should it have a working tree?

They sound contradicting with each other to me.  Isn't that a
clear configuration error?

Re: [PATCH 3/8] Clean up work-tree handling

[Johannes Schindelin]

Hi,

On Sun, 29 Jul 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > I still have a problem with what should happen if both "core.bare == true" 
> > and "core.worktree = /some/where/over/the/rainbow".  Should it be bare, or 
> > should it have a working tree?
> 
> They sound contradicting with each other to me.  Isn't that a
> clear configuration error?

Yes.  But why not play nice?

Okay, the real reason I do not want to catch this error is because it 
complicates my code.

But really, why not say "worktree takes precedence"?

BTW I realised that callers of setup_git_directory_gently() might forget 
the check for the repository format version...

Ciao,
Dscho