[PATCH v2] Add support for GIT_CEILING_DIRS

[PATCH v2] Add support for GIT_CEILING_DIRS

[David Reiss]

Make git recognize a new environment variable that prevents it from
chdir'ing up into specified directories when looking for a GIT_DIR.
Useful for avoiding slow network directories.

For example, I use git in an environment where homedirs are automounted
and "ls /home/nonexistent" takes about 9 seconds.  Setting
GIT_CEILING_DIRS="/home" allows "git help -a" (for bash completion) and
"git symbolic-ref" (for my shell prompt) to run in a reasonable time.

This also moves the chdir call to after computing the new cwd.
This should be a no-op because the cwd is not read in the interim
and any nonlocal exits either chdir to an absolute path or die.

Signed-off-by: David Reiss <dreiss@facebook.com>
---
Regarding initializing min_offset to -1, it is necessary, given that
code to have the root directory be a git repository.  Setting min_offset,
to 0 would prevent "/.git" and "/objects" from being checked.

Removed all C99-style comments.  Old habits die hard. :)

I moved the prefix-calculating code into a separate function, as you
suggested, and removed the allocation (which actually wasn't really
necessary to begin with).  This also has the nice effect that it is
much easier to test it in isolation.  I added a test for it, #if 0'ed
out.  I wasn't able to find a convention for this sort of test.

Regarding the "big, ugly change", your suggestion has an off-by-one error.
With that code, as soon as "cwd[--offset] != '/'" is false, the loop ends,
and we would not detect that offset is now equal to min_offset.  Getting
this loop right was really tricky because it is basically checking three
things at once (beginning of string, ceiling, directory boundary).  The
key to getting it to look like you suggested was to add 1 to min_offset
in all cases.  I renamed it ceil_offset to reflect the fact that it is
one less than the miinimum offset we are willing to test and added a
comment explaining exactly what it means (because it is a little
counter-intuitive).  I also added a bunch of tests to make sure that this
behavior doesn't break down with single-character directories.

For my tests, you said "I think it would make more sense to write these out."
Could you explain what you mean by this?  Also, I removed the "&& return".
I copied these from t1500, and I obviously wasn't paying close attention.

 Documentation/git.txt   |    8 +++
 cache.h                 |    1 +
 setup.c                 |  116 ++++++++++++++++++++++++++++++++++-
 t/t1504-ceiling-dirs.sh |  156 +++++++++++++++++++++++++++++++++++++++++++++++
 t/test-lib.sh           |    1 +
 5 files changed, 279 insertions(+), 3 deletions(-)
 create mode 100755 t/t1504-ceiling-dirs.sh

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 6f445b1..8aea331 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -415,6 +415,14 @@ git so take care if using Cogito etc.
 	This can also be controlled by the '--work-tree' command line
 	option and the core.worktree configuration variable.
 
+'GIT_CEILING_DIRS'::
+	This should be a colon-separated list of absolute paths.
+	If set, it is a list of directories that git should not chdir
+	up into while looking for a repository directory.
+	It will not exclude the current working directory or
+	a GIT_DIR set on the command line or in the environment.
+	(Useful for excluding slow-loading network directories.)
+
 git Commits
 ~~~~~~~~~~~
 'GIT_AUTHOR_NAME'::
diff --git a/cache.h b/cache.h
index 9cee9a5..8300acc 100644
--- a/cache.h
+++ b/cache.h
@@ -300,6 +300,7 @@ static inline enum object_type object_type(unsigned int mode)
 #define CONFIG_ENVIRONMENT "GIT_CONFIG"
 #define CONFIG_LOCAL_ENVIRONMENT "GIT_CONFIG_LOCAL"
 #define EXEC_PATH_ENVIRONMENT "GIT_EXEC_PATH"
+#define CEILING_DIRS_ENVIRONMENT "GIT_CEILING_DIRS"
 #define GITATTRIBUTES_FILE ".gitattributes"
 #define INFOATTRIBUTES_FILE "info/attributes"
 #define ATTRIBUTE_MACRO_PREFIX "[attr]"
diff --git a/setup.c b/setup.c
index b8fd476..de9e7f1 100644
--- a/setup.c
+++ b/setup.c
@@ -353,16 +353,118 @@ const char *read_gitfile_gently(const char *path)
 }
 
 /*
+ * path = Canonical absolute path
+ * prefix_list = Colon-separated list of canonical absolute paths
+ *
+ * Determines, for each path in parent_list, whether the "prefix" really
+ * is an ancestor directory of path.  Returns the length of the longest
+ * ancestor directory, excluding any trailing slashes, or -1 if no prefix
+ * is an ancestry.  (Note that this means 0 is returned if prefix_list
+ * contains "/".)  "/foo" is not considered an ancestor of "/foobar".
+ * Directories are not considered to be their own ancestors.  Paths must
+ * be in a canonical form: empty components, or "." or ".." components
+ * are not allowed.  prefix_list may be null, which is like "".
+ */
+static int longest_ancestor_length(const char *path, const char *prefix_list)
+{
+	const char *ceil, *colon;
+	int max_len = -1;
+
+	if (prefix_list == NULL)
+		return -1;
+	/* "/" is a tricky edge case.  It should always return -1, though. */
+	if (!strcmp(path, "/"))
+		return -1;
+
+	ceil = prefix_list;
+	for (;;) {
+		int len;
+
+		/* Add strchrnul to compat? */
+		colon = strchr(ceil, ':');
+		if (colon)
+			len = colon - ceil;
+		else
+			len = strlen(ceil);
+
+		/* "" would otherwise be treated like "/". */
+		if (len) {
+			/* Trim trailing slashes. */
+			while (len && ceil[len-1] == '/')
+				len--;
+
+			if (!strncmp(path, ceil, len) &&
+					path[len] == '/' &&
+					len > max_len) {
+				max_len = len;
+			}
+		}
+
+		if (!colon)
+			break;
+		ceil = colon + 1;
+	}
+
+	return max_len;
+}
+
+#if 0
+static void test_longest_ancestor_length()
+{
+	assert(longest_ancestor_length("/", NULL           ) == -1);
+	assert(longest_ancestor_length("/", ""             ) == -1);
+	assert(longest_ancestor_length("/", "/"            ) == -1);
+
+	assert(longest_ancestor_length("/foo", NULL           ) == -1);
+	assert(longest_ancestor_length("/foo", ""             ) == -1);
+	assert(longest_ancestor_length("/foo", ":"            ) == -1);
+	assert(longest_ancestor_length("/foo", "/"            ) ==  0);
+	assert(longest_ancestor_length("/foo", "/fo"          ) == -1);
+	assert(longest_ancestor_length("/foo", "/foo"         ) == -1);
+	assert(longest_ancestor_length("/foo", "/foo/"        ) == -1);
+	assert(longest_ancestor_length("/foo", "/bar"         ) == -1);
+	assert(longest_ancestor_length("/foo", "/bar/"        ) == -1);
+	assert(longest_ancestor_length("/foo", "/foo/bar"     ) == -1);
+	assert(longest_ancestor_length("/foo", "/foo:/bar/"   ) == -1);
+	assert(longest_ancestor_length("/foo", "/foo/:/bar/"  ) == -1);
+	assert(longest_ancestor_length("/foo", "/foo::/bar/"  ) == -1);
+	assert(longest_ancestor_length("/foo", "/:/foo:/bar/" ) ==  0);
+	assert(longest_ancestor_length("/foo", "/foo:/:/bar/" ) ==  0);
+	assert(longest_ancestor_length("/foo", "/:/bar/:/foo" ) ==  0);
+
+	assert(longest_ancestor_length("/foo/bar", NULL           ) == -1);
+	assert(longest_ancestor_length("/foo/bar", ""             ) == -1);
+	assert(longest_ancestor_length("/foo/bar", "/"            ) ==  0);
+	assert(longest_ancestor_length("/foo/bar", "/fo"          ) == -1);
+	assert(longest_ancestor_length("/foo/bar", "/foo"         ) ==  4);
+	assert(longest_ancestor_length("/foo/bar", "/foo/"        ) ==  4);
+	assert(longest_ancestor_length("/foo/bar", "/foo/ba"      ) == -1);
+	assert(longest_ancestor_length("/foo/bar", "/:/fo"        ) ==  0);
+	assert(longest_ancestor_length("/foo/bar", "/foo:/foo/ba" ) ==  4);
+	assert(longest_ancestor_length("/foo/bar", "/bar"         ) == -1);
+	assert(longest_ancestor_length("/foo/bar", "/bar/"        ) == -1);
+	assert(longest_ancestor_length("/foo/bar", "/fo:"         ) == -1);
+	assert(longest_ancestor_length("/foo/bar", ":/fo"         ) == -1);
+	assert(longest_ancestor_length("/foo/bar", "/foo:/bar/"   ) ==  4);
+	assert(longest_ancestor_length("/foo/bar", "/:/foo:/bar/" ) ==  4);
+	assert(longest_ancestor_length("/foo/bar", "/foo:/:/bar/" ) ==  4);
+	assert(longest_ancestor_length("/foo/bar", "/:/bar/:/fo"  ) ==  0);
+	assert(longest_ancestor_length("/foo/bar", "/:/bar/"      ) ==  0);
+}
+#endif
+
+/*
  * 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);
+	const char *env_ceiling_dirs = getenv(CEILING_DIRS_ENVIRONMENT);
 	static char cwd[PATH_MAX+1];
 	const char *gitdirenv;
 	const char *gitfile_dir;
-	int len, offset;
+	int len, offset, ceil_offset;
 
 	/*
 	 * Let's assume that we are in a git repository.
@@ -415,6 +517,14 @@ const char *setup_git_directory_gently(int *nongit_ok)
 		die("Unable to read current working directory");
 
 	/*
+	 * Compute ceil_offset based on GIT_CEILING_DIRS.  It is actually the offset
+	 * of the first character in cwd after the trailing slash of the ceiling.
+	 * Putting it so far to the right is necessary in order to bail out of the
+	 * "--offset" loop early enough.
+	 */
+	ceil_offset = 1 + longest_ancestor_length(cwd, env_ceiling_dirs);
+
+	/*
 	 * Test in the following order (relative to the cwd):
 	 * - .git (file containing "gitdir: <path>")
 	 * - .git/
@@ -443,9 +553,8 @@ const char *setup_git_directory_gently(int *nongit_ok)
 			check_repository_format_gently(nongit_ok);
 			return NULL;
 		}
-		chdir("..");
 		do {
-			if (!offset) {
+			if (offset <= ceil_offset) {
 				if (nongit_ok) {
 					if (chdir(cwd))
 						die("Cannot come back to cwd");
@@ -455,6 +564,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
 				die("Not a git repository");
 			}
 		} while (cwd[--offset] != '/');
+		chdir("..");
 	}
 
 	inside_git_dir = 0;
diff --git a/t/t1504-ceiling-dirs.sh b/t/t1504-ceiling-dirs.sh
new file mode 100755
index 0000000..091baad
--- /dev/null
+++ b/t/t1504-ceiling-dirs.sh
@@ -0,0 +1,156 @@
+#!/bin/sh
+
+test_description='test GIT_CEILING_DIRS'
+. ./test-lib.sh
+
+test_prefix() {
+	test_expect_success "$1" \
+	"test '$2' = \"\$(git rev-parse --show-prefix)\""
+}
+
+test_fail() {
+	test_expect_code 128 "$1: prefix" \
+	"git rev-parse --show-prefix"
+}
+
+TRASH_ROOT="$(pwd)"
+ROOT_PARENT=$(dirname "$TRASH_ROOT")
+
+
+unset GIT_CEILING_DIRS
+test_prefix no_ceil ""
+
+export GIT_CEILING_DIRS=""
+test_prefix ceil_empty ""
+
+export GIT_CEILING_DIRS="$ROOT_PARENT"
+test_prefix ceil_at_parent ""
+
+export GIT_CEILING_DIRS="$ROOT_PARENT/"
+test_prefix ceil_at_parent_slash ""
+
+export GIT_CEILING_DIRS="$TRASH_ROOT"
+test_prefix ceil_at_trash ""
+
+export GIT_CEILING_DIRS="$TRASH_ROOT/"
+test_prefix ceil_at_trash_slash ""
+
+export GIT_CEILING_DIRS="$TRASH_ROOT/sub"
+test_prefix ceil_at_sub ""
+
+export GIT_CEILING_DIRS="$TRASH_ROOT/sub/"
+test_prefix ceil_at_sub_slash ""
+
+
+mkdir -p sub/dir || exit 1
+cd sub/dir || exit 1
+
+unset GIT_CEILING_DIRS
+test_prefix subdir_no_ceil "sub/dir/"
+
+export GIT_CEILING_DIRS=""
+test_prefix subdir_ceil_empty "sub/dir/"
+
+export GIT_CEILING_DIRS="$TRASH_ROOT"
+test_fail subdir_ceil_at_trash
+
+export GIT_CEILING_DIRS="$TRASH_ROOT/"
+test_fail subdir_ceil_at_trash_slash
+
+export GIT_CEILING_DIRS="$TRASH_ROOT/sub"
+test_fail subdir_ceil_at_sub
+
+export GIT_CEILING_DIRS="$TRASH_ROOT/sub/"
+test_fail subdir_ceil_at_sub_slash
+
+export GIT_CEILING_DIRS="$TRASH_ROOT/sub/dir"
+test_prefix subdir_ceil_at_subdir "sub/dir/"
+
+export GIT_CEILING_DIRS="$TRASH_ROOT/sub/dir/"
+test_prefix subdir_ceil_at_subdir_slash "sub/dir/"
+
+
+export GIT_CEILING_DIRS="$TRASH_ROOT/su"
+test_prefix subdir_ceil_at_su "sub/dir/"
+
+export GIT_CEILING_DIRS="$TRASH_ROOT/su/"
+test_prefix subdir_ceil_at_su_slash "sub/dir/"
+
+export GIT_CEILING_DIRS="$TRASH_ROOT/sub/di"
+test_prefix subdir_ceil_at_sub_di "sub/dir/"
+
+export GIT_CEILING_DIRS="$TRASH_ROOT/sub/di"
+test_prefix subdir_ceil_at_sub_di_slash "sub/dir/"
+
+export GIT_CEILING_DIRS="$TRASH_ROOT/subdi"
+test_prefix subdir_ceil_at_subdi "sub/dir/"
+
+export GIT_CEILING_DIRS="$TRASH_ROOT/subdi"
+test_prefix subdir_ceil_at_subdi_slash "sub/dir/"
+
+
+export GIT_CEILING_DIRS="foo:$TRASH_ROOT/sub"
+test_fail second_of_two
+
+export GIT_CEILING_DIRS="$TRASH_ROOT/sub:bar"
+test_fail first_of_two
+
+export GIT_CEILING_DIRS="foo:$TRASH_ROOT/sub:bar"
+test_fail second_of_three
+
+
+export GIT_CEILING_DIRS="$TRASH_ROOT/sub"
+export GIT_DIR=../../.git
+test_prefix git_dir_specified ""
+unset GIT_DIR
+
+
+cd ../.. || exit 1
+mkdir -p s/d || exit 1
+cd s/d || exit 1
+
+unset GIT_CEILING_DIRS
+test_prefix sd_no_ceil "s/d/"
+
+export GIT_CEILING_DIRS=""
+test_prefix sd_ceil_empty "s/d/"
+
+export GIT_CEILING_DIRS="$TRASH_ROOT"
+test_fail sd_ceil_at_trash
+
+export GIT_CEILING_DIRS="$TRASH_ROOT/"
+test_fail sd_ceil_at_trash_slash
+
+export GIT_CEILING_DIRS="$TRASH_ROOT/s"
+test_fail sd_ceil_at_s
+
+export GIT_CEILING_DIRS="$TRASH_ROOT/s/"
+test_fail sd_ceil_at_s_slash
+
+export GIT_CEILING_DIRS="$TRASH_ROOT/s/d"
+test_prefix sd_ceil_at_sd "s/d/"
+
+export GIT_CEILING_DIRS="$TRASH_ROOT/s/d/"
+test_prefix sd_ceil_at_sd_slash "s/d/"
+
+
+export GIT_CEILING_DIRS="$TRASH_ROOT/su"
+test_prefix sd_ceil_at_su "s/d/"
+
+export GIT_CEILING_DIRS="$TRASH_ROOT/su/"
+test_prefix sd_ceil_at_su_slash "s/d/"
+
+export GIT_CEILING_DIRS="$TRASH_ROOT/s/di"
+test_prefix sd_ceil_at_s_di "s/d/"
+
+export GIT_CEILING_DIRS="$TRASH_ROOT/s/di"
+test_prefix sd_ceil_at_s_di_slash "s/d/"
+
+export GIT_CEILING_DIRS="$TRASH_ROOT/sdi"
+test_prefix sd_ceil_at_sdi "s/d/"
+
+export GIT_CEILING_DIRS="$TRASH_ROOT/sdi"
+test_prefix sd_ceil_at_sdi_slash "s/d/"
+
+
+test_done
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 7c2a8ba..22899c1 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -35,6 +35,7 @@ unset GIT_WORK_TREE
 unset GIT_EXTERNAL_DIFF
 unset GIT_INDEX_FILE
 unset GIT_OBJECT_DIRECTORY
+unset GIT_CEILING_DIRS
 unset SHA1_FILE_DIRECTORIES
 unset SHA1_FILE_DIRECTORY
 GIT_MERGE_VERBOSITY=5
-- 
1.5.4

Re: [PATCH v2] Add support for GIT_CEILING_DIRS

[Johannes Sixt]

David Reiss schrieb:
>  const char *setup_git_directory_gently(int *nongit_ok)
>  {
>  	const char *work_tree_env = getenv(GIT_WORK_TREE_ENVIRONMENT);
> +	const char *env_ceiling_dirs = getenv(CEILING_DIRS_ENVIRONMENT);
>  	static char cwd[PATH_MAX+1];
>  	const char *gitdirenv;
>  	const char *gitfile_dir;
> -	int len, offset;
> +	int len, offset, ceil_offset;
>  
>  	/*
>  	 * Let's assume that we are in a git repository.
> @@ -415,6 +517,14 @@ const char *setup_git_directory_gently(int *nongit_ok)
>  		die("Unable to read current working directory");
>  
>  	/*
> +	 * Compute ceil_offset based on GIT_CEILING_DIRS.  It is actually the offset
> +	 * of the first character in cwd after the trailing slash of the ceiling.
> +	 * Putting it so far to the right is necessary in order to bail out of the
> +	 * "--offset" loop early enough.
> +	 */
> +	ceil_offset = 1 + longest_ancestor_length(cwd, env_ceiling_dirs);
> +
> +	/*
>  	 * Test in the following order (relative to the cwd):
>  	 * - .git (file containing "gitdir: <path>")
>  	 * - .git/
> @@ -443,9 +553,8 @@ const char *setup_git_directory_gently(int *nongit_ok)
>  			check_repository_format_gently(nongit_ok);
>  			return NULL;
>  		}
> -		chdir("..");
>  		do {
> -			if (!offset) {
> +			if (offset <= ceil_offset) {
>  				if (nongit_ok) {
>  					if (chdir(cwd))
>  						die("Cannot come back to cwd");
> @@ -455,6 +564,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
>  				die("Not a git repository");
>  			}
>  		} while (cwd[--offset] != '/');

If you make it so that the default value of ceil_offset is 0 (i.e. in the
absence of any GIT_CEILING_DIRS), and at this place you did

		} while (offset > ceil_offset && cwd[--offset] != '/');

you wouldn't have to bend backwards with this off-by-one magic, would you?
(But I admit that I haven't tried this code, I'm only comparing it to how
we do it mingw.git.)

-- Hannes

Re: [PATCH v2] Add support for GIT_CEILING_DIRS

[David Reiss]

> If you make it so that the default value of ceil_offset is 0 (i.e. in the
> absence of any GIT_CEILING_DIRS),
This is what the new version of the patch does.

> and at this place you did
> 
>                 } while (offset > ceil_offset && cwd[--offset] != '/');
> 
> you wouldn't have to bend backwards with this off-by-one magic, would you?
It seems like that would cause it to continue on with the outer loop, rather
than aborting (which is what the current version does) when you hit the ceiling.
Or maybe I'm misunderstanding something.

--David

Re: [PATCH v2] Add support for GIT_CEILING_DIRS

[Johannes Sixt]

David Reiss schrieb:
>> If you make it so that the default value of ceil_offset is 0 (i.e. in the
>> absence of any GIT_CEILING_DIRS),
> This is what the new version of the patch does.
> 
>> and at this place you did
>>
>>                 } while (offset > ceil_offset && cwd[--offset] != '/');
>>
>> you wouldn't have to bend backwards with this off-by-one magic, would you?
> It seems like that would cause it to continue on with the outer loop, rather
> than aborting (which is what the current version does) when you hit the ceiling.
> Or maybe I'm misunderstanding something.

Ok, I see. I'm proposing that the meaning of ceil_offset should be "do not look
at this many characters at the beginning of cwd". And we always have
cwd[ceil_offset] == '/'. Something like this on top of your patch.
Further tweaking will be necessary (I'm just illustrating my point),
in particular you could name it min_offset again.

-- Hannes

diff --git a/setup.c b/setup.c
index de9e7f1..5dbc080 100644
--- a/setup.c
+++ b/setup.c
@@ -368,13 +368,13 @@ const char *read_gitfile_gently(const char *path)
 static int longest_ancestor_length(const char *path, const char *prefix_list)
 {
 	const char *ceil, *colon;
-	int max_len = -1;
+	int max_len = 0;

 	if (prefix_list == NULL)
-		return -1;
-	/* "/" is a tricky edge case.  It should always return -1, though. */
+		return 0;
+	/* "/" is a tricky edge case.  It should always return 0, though. */
 	if (!strcmp(path, "/"))
-		return -1;
+		return 0;

 	ceil = prefix_list;
 	for (;;) {
@@ -522,7 +522,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
 	 * Putting it so far to the right is necessary in order to bail out of the
 	 * "--offset" loop early enough.
 	 */
-	ceil_offset = 1 + longest_ancestor_length(cwd, env_ceiling_dirs);
+	ceil_offset = longest_ancestor_length(cwd, env_ceiling_dirs);

 	/*
 	 * Test in the following order (relative to the cwd):
@@ -553,17 +553,16 @@ const char *setup_git_directory_gently(int *nongit_ok)
 			check_repository_format_gently(nongit_ok);
 			return NULL;
 		}
-		do {
-			if (offset <= ceil_offset) {
-				if (nongit_ok) {
-					if (chdir(cwd))
-						die("Cannot come back to cwd");
-					*nongit_ok = 1;
-					return NULL;
-				}
-				die("Not a git repository");
+		do { } while (offset > ceil_offset && cwd[--offset] != '/');
+		if (offset <= ceil_offset) {
+			if (nongit_ok) {
+				if (chdir(cwd))
+					die("Cannot come back to cwd");
+				*nongit_ok = 1;
+				return NULL;
 			}
-		} while (cwd[--offset] != '/');
+			die("Not a git repository");
+		}
 		chdir("..");
 	}

Re: [PATCH v2] Add support for GIT_CEILING_DIRS

[Johannes Schindelin]

Hi,

On Thu, 15 May 2008, Johannes Sixt wrote:

> +		do { } while (offset > ceil_offset && cwd[--offset] != '/');

You probably meant to remove the "do { }", and have an own line

			; /* do nothing */

but for the rest, I agree that it is easier on the eye (particularly the 
off-by-one issue, which is always a problem for this developer to get 
right; avoiding it is therefore the better option).

Ciao,
Dscho

Re: [PATCH v2] Add support for GIT_CEILING_DIRS

[David Reiss]

The problem with this implementation is that it does not distinguish
between GIT_CEILING_DIRS being unset and GIT_CEILING_DIRS="/".  For
example...

cd /
sudo git init
cd /home
git rev-parse --show-prefix

That series of commands works with either version of my patch, but fails
with "fatal: Not a git repository" if I apply this change.  I am
certainly open to changing this code, but I think we will always
need two separate values of ceil_offset to represent "unset" and "/".
It's just a question of whether they should be -1 and 0 or 0 and 1.

--David


Johannes Schindelin wrote:
> Hi,
> 
> On Thu, 15 May 2008, Johannes Sixt wrote:
> 
>> +             do { } while (offset > ceil_offset && cwd[--offset] != '/');
> 
> You probably meant to remove the "do { }", and have an own line
> 
>                         ; /* do nothing */
> 
> but for the rest, I agree that it is easier on the eye (particularly the
> off-by-one issue, which is always a problem for this developer to get
> right; avoiding it is therefore the better option).
> 
> Ciao,
> Dscho
> 

Re: [PATCH v2] Add support for GIT_CEILING_DIRS

[Johannes Schindelin]

Hi,

On Thu, 15 May 2008, David Reiss wrote:

> The problem with this implementation is that it does not distinguish 
> between GIT_CEILING_DIRS being unset and GIT_CEILING_DIRS="/".  For 
> example...
> 
> cd /
> sudo git init
> cd /home
> git rev-parse --show-prefix
> 
> That series of commands works with either version of my patch, but fails 
> with "fatal: Not a git repository" if I apply this change.  I am 
> certainly open to changing this code, but I think we will always need 
> two separate values of ceil_offset to represent "unset" and "/". It's 
> just a question of whether they should be -1 and 0 or 0 and 1.

You are much more familiar with the code, but I suspect that a simple 
change would fix that:

> >> +             do { } while (offset > ceil_offset && cwd[--offset] != '/');

Just use "--offset >= 0 && cwd[--offset] != '/'" here.  And maybe 
ceil_offset = -1 again.

I cannot quickly test, since I am short on time, and it would be too 
cumbersome to find which patches to apply first.

But I strongly believe that it is not beyond your capabilities to adjust 
Hannes' patch for your command series, keeping the elegance.

Ciao,
Dscho