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 <dreiss@xxxxxxxxxxxx>
> ---
> 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.
> 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.
> @@ -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.)
> @@ -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.
> @@ -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).
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
