Re: [PATCH 2/2] Add support for GIT_CEILING_DIRS
From: Johannes Schindelin <hidden>
Date: 2016-06-15 22:44:36
Hi, On Tue, 13 May 2008, David Reiss wrote:
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. Signed-off-by: David Reiss <redacted> --- Just a bit of context about the motivation for this. I use git in an environment where homedirs are automounted and "ls /home/nonexistent" takes about 9 seconds.
I think this is a very strong argument in favor of your patch, so it belongs into the commit message.
quoted hunk ↗ jump to hunk
diff --git a/setup.c b/setup.c index c54f2b6..d7d986c 100644 --- a/setup.c +++ b/setup.c@@ -359,10 +359,11 @@ const char *read_gitfile_gently(const char *path) 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, min_offset = -1;
AFAIR we have min_offset in the mingw branch, too, but I think it is initialized to 0.
quoted hunk ↗ jump to hunk
@@ -414,6 +415,37 @@ const char *setup_git_directory_gently(int *nongit_ok) if (!getcwd(cwd, sizeof(cwd)-1)) die("Unable to read current working directory"); + // Compute min_offset based on GIT_CEILING_DIRS.
We do not like C99 style comments. Remember, there are people who compile Git on something else than the super-latest Linux with cutting-edge GCC.
+ if (env_ceiling_dirs) {
+ char *ceils, *ceil, *colon;
+ ceil = ceils = xstrdup(env_ceiling_dirs);
I think it is quite possible to do this without data copying.
Besides, I think that this whole block should be a function in path.c, if
only to _document_ what it does: it is much easier to read for people who
did not write the code.
Something like
int longest_prefix(const char *path, const char *prefix_list)
{
int max_length = 0, length = 0, i;
for (i = 0; *prefix_list; i++)
if (prefix_list[i] == ':') {
if (length > max_length)
max_length = length;
length = 0;
}
else if (length >= 0) {
if (prefix_list[i] == path[length])
length++;
else {
if (length > max_length)
max_length = length;
length = -1;
}
}
return max_length;
}
(Completely untested, though.)
quoted hunk ↗ jump to hunk
@@ -427,6 +459,7 @@ const char *setup_git_directory_gently(int *nongit_ok) */ offset = len = strlen(cwd); for (;;) { + // Check the current directory (.git first).
Again, C99 comments are a no-go.
quoted hunk ↗ jump to hunk
@@ -443,17 +476,29 @@ const char *setup_git_directory_gently(int *nongit_ok) check_repository_format_gently(nongit_ok); return NULL; } - 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"); + + // Did we just check the root dir? + if (!offset) { + not_a_repo: + 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"); + } + + while (cwd[--offset] != '/') { + assert(offset > 0); + } + + // Don't chdir into the ceiling. + if (offset <= min_offset) { + assert(offset == min_offset); + goto not_a_repo; + } + chdir("..");
Why this big, ugly change in the code? I would have expected you to simply change the "if (!offset)" to "if (offset <= min_offset)" (together with the initialization to 0).
quoted hunk ↗ jump to hunk
diff --git a/t/t1504-ceiling-dirs.sh b/t/t1504-ceiling-dirs.sh new file mode 100755 index 0000000..d4eaa13 --- /dev/null +++ b/t/t1504-ceiling-dirs.sh@@ -0,0 +1,104 @@ +#!/bin/sh + +test_description='test GIT_CEILING_DIRS' +. ./test-lib.sh + +test_prefix() { + test_expect_success "$1" \ + "test '$2' = \"\$(git rev-parse --show-prefix)\"" + shift + [ $# -eq 0 ] && return +} + +test_fail() { + test_expect_code 128 "$1: prefix" \ + "git rev-parse --show-prefix" + shift + [ $# -eq 0 ] && return +}
I think it would make more sense to write these out. Besides, I think that the "&& return" is not necessary. And your tests are, uhm, quite extensive. Ciao, Dscho