{"thread":{"id":"13523","subject":"[PATCH v2] Add support for GIT_CEILING_DIRS","startedAt":"2008-05-15T01:35:25Z","lastAt":"2008-05-15T17:45:44Z","messageCount":7,"participants":["David Reiss","Johannes Sixt","Johannes Schindelin"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"77007","messageId":"482B935D.20105@facebook.com","threadId":"13523","inReplyTo":null,"subject":"[PATCH v2] Add support for GIT_CEILING_DIRS","fromName":"David Reiss","fromEmail":"dreiss@facebook.com","sentAt":"2008-05-15T01:35:25Z","receivedAt":"2008-05-15T01:35:25Z","isPatch":true,"sender":{"key":"dreiss@facebook.com","avatar":null},"body":"Make git recognize a new environment variable that prevents it from\nchdir'ing up into specified directories when looking for a GIT_DIR.\nUseful for avoiding slow network directories.\n\nFor example, I use git in an environment where homedirs are automounted\nand \"ls /home/nonexistent\" takes about 9 seconds.  Setting\nGIT_CEILING_DIRS=\"/home\" allows \"git help -a\" (for bash completion) and\n\"git symbolic-ref\" (for my shell prompt) to run in a reasonable time.\n\nThis also moves the chdir call to after computing the new cwd.\nThis should be a no-op because the cwd is not read in the interim\nand any nonlocal exits either chdir to an absolute path or die.\n\nSigned-off-by: David Reiss <dreiss@facebook.com>\n---\nRegarding initializing min_offset to -1, it is necessary, given that\ncode to have the root directory be a git repository.  Setting min_offset,\nto 0 would prevent \"/.git\" and \"/objects\" from being checked.\n\nRemoved all C99-style comments.  Old habits die hard. :)\n\nI moved the prefix-calculating code into a separate function, as you\nsuggested, and removed the allocation (which actually wasn't really\nnecessary to begin with).  This also has the nice effect that it is\nmuch easier to test it in isolation.  I added a test for it, #if 0'ed\nout.  I wasn't able to find a convention for this sort of test.\n\nRegarding the \"big, ugly change\", your suggestion has an off-by-one error.\nWith that code, as soon as \"cwd[--offset] != '/'\" is false, the loop ends,\nand we would not detect that offset is now equal to min_offset.  Getting\nthis loop right was really tricky because it is basically checking three\nthings at once (beginning of string, ceiling, directory boundary).  The\nkey to getting it to look like you suggested was to add 1 to min_offset\nin all cases.  I renamed it ceil_offset to reflect the fact that it is\none less than the miinimum offset we are willing to test and added a\ncomment explaining exactly what it means (because it is a little\ncounter-intuitive).  I also added a bunch of tests to make sure that this\nbehavior doesn't break down with single-character directories.\n\nFor my tests, you said \"I think it would make more sense to write these out.\"\nCould you explain what you mean by this?  Also, I removed the \"&& return\".\nI copied these from t1500, and I obviously wasn't paying close attention.\n\n Documentation/git.txt   |    8 +++\n cache.h                 |    1 +\n setup.c                 |  116 ++++++++++++++++++++++++++++++++++-\n t/t1504-ceiling-dirs.sh |  156 +++++++++++++++++++++++++++++++++++++++++++++++\n t/test-lib.sh           |    1 +\n 5 files changed, 279 insertions(+), 3 deletions(-)\n create mode 100755 t/t1504-ceiling-dirs.sh\n\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex 6f445b1..8aea331 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -415,6 +415,14 @@ git so take care if using Cogito etc.\n \tThis can also be controlled by the '--work-tree' command line\n \toption and the core.worktree configuration variable.\n \n+'GIT_CEILING_DIRS'::\n+\tThis should be a colon-separated list of absolute paths.\n+\tIf set, it is a list of directories that git should not chdir\n+\tup into while looking for a repository directory.\n+\tIt will not exclude the current working directory or\n+\ta GIT_DIR set on the command line or in the environment.\n+\t(Useful for excluding slow-loading network directories.)\n+\n git Commits\n ~~~~~~~~~~~\n 'GIT_AUTHOR_NAME'::\ndiff --git a/cache.h b/cache.h\nindex 9cee9a5..8300acc 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -300,6 +300,7 @@ static inline enum object_type object_type(unsigned int mode)\n #define CONFIG_ENVIRONMENT \"GIT_CONFIG\"\n #define CONFIG_LOCAL_ENVIRONMENT \"GIT_CONFIG_LOCAL\"\n #define EXEC_PATH_ENVIRONMENT \"GIT_EXEC_PATH\"\n+#define CEILING_DIRS_ENVIRONMENT \"GIT_CEILING_DIRS\"\n #define GITATTRIBUTES_FILE \".gitattributes\"\n #define INFOATTRIBUTES_FILE \"info/attributes\"\n #define ATTRIBUTE_MACRO_PREFIX \"[attr]\"\ndiff --git a/setup.c b/setup.c\nindex b8fd476..de9e7f1 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -353,16 +353,118 @@ const char *read_gitfile_gently(const char *path)\n }\n \n /*\n+ * path = Canonical absolute path\n+ * prefix_list = Colon-separated list of canonical absolute paths\n+ *\n+ * Determines, for each path in parent_list, whether the \"prefix\" really\n+ * is an ancestor directory of path.  Returns the length of the longest\n+ * ancestor directory, excluding any trailing slashes, or -1 if no prefix\n+ * is an ancestry.  (Note that this means 0 is returned if prefix_list\n+ * contains \"/\".)  \"/foo\" is not considered an ancestor of \"/foobar\".\n+ * Directories are not considered to be their own ancestors.  Paths must\n+ * be in a canonical form: empty components, or \".\" or \"..\" components\n+ * are not allowed.  prefix_list may be null, which is like \"\".\n+ */\n+static int longest_ancestor_length(const char *path, const char *prefix_list)\n+{\n+\tconst char *ceil, *colon;\n+\tint max_len = -1;\n+\n+\tif (prefix_list == NULL)\n+\t\treturn -1;\n+\t/* \"/\" is a tricky edge case.  It should always return -1, though. */\n+\tif (!strcmp(path, \"/\"))\n+\t\treturn -1;\n+\n+\tceil = prefix_list;\n+\tfor (;;) {\n+\t\tint len;\n+\n+\t\t/* Add strchrnul to compat? */\n+\t\tcolon = strchr(ceil, ':');\n+\t\tif (colon)\n+\t\t\tlen = colon - ceil;\n+\t\telse\n+\t\t\tlen = strlen(ceil);\n+\n+\t\t/* \"\" would otherwise be treated like \"/\". */\n+\t\tif (len) {\n+\t\t\t/* Trim trailing slashes. */\n+\t\t\twhile (len && ceil[len-1] == '/')\n+\t\t\t\tlen--;\n+\n+\t\t\tif (!strncmp(path, ceil, len) &&\n+\t\t\t\t\tpath[len] == '/' &&\n+\t\t\t\t\tlen > max_len) {\n+\t\t\t\tmax_len = len;\n+\t\t\t}\n+\t\t}\n+\n+\t\tif (!colon)\n+\t\t\tbreak;\n+\t\tceil = colon + 1;\n+\t}\n+\n+\treturn max_len;\n+}\n+\n+#if 0\n+static void test_longest_ancestor_length()\n+{\n+\tassert(longest_ancestor_length(\"/\", NULL           ) == -1);\n+\tassert(longest_ancestor_length(\"/\", \"\"             ) == -1);\n+\tassert(longest_ancestor_length(\"/\", \"/\"            ) == -1);\n+\n+\tassert(longest_ancestor_length(\"/foo\", NULL           ) == -1);\n+\tassert(longest_ancestor_length(\"/foo\", \"\"             ) == -1);\n+\tassert(longest_ancestor_length(\"/foo\", \":\"            ) == -1);\n+\tassert(longest_ancestor_length(\"/foo\", \"/\"            ) ==  0);\n+\tassert(longest_ancestor_length(\"/foo\", \"/fo\"          ) == -1);\n+\tassert(longest_ancestor_length(\"/foo\", \"/foo\"         ) == -1);\n+\tassert(longest_ancestor_length(\"/foo\", \"/foo/\"        ) == -1);\n+\tassert(longest_ancestor_length(\"/foo\", \"/bar\"         ) == -1);\n+\tassert(longest_ancestor_length(\"/foo\", \"/bar/\"        ) == -1);\n+\tassert(longest_ancestor_length(\"/foo\", \"/foo/bar\"     ) == -1);\n+\tassert(longest_ancestor_length(\"/foo\", \"/foo:/bar/\"   ) == -1);\n+\tassert(longest_ancestor_length(\"/foo\", \"/foo/:/bar/\"  ) == -1);\n+\tassert(longest_ancestor_length(\"/foo\", \"/foo::/bar/\"  ) == -1);\n+\tassert(longest_ancestor_length(\"/foo\", \"/:/foo:/bar/\" ) ==  0);\n+\tassert(longest_ancestor_length(\"/foo\", \"/foo:/:/bar/\" ) ==  0);\n+\tassert(longest_ancestor_length(\"/foo\", \"/:/bar/:/foo\" ) ==  0);\n+\n+\tassert(longest_ancestor_length(\"/foo/bar\", NULL           ) == -1);\n+\tassert(longest_ancestor_length(\"/foo/bar\", \"\"             ) == -1);\n+\tassert(longest_ancestor_length(\"/foo/bar\", \"/\"            ) ==  0);\n+\tassert(longest_ancestor_length(\"/foo/bar\", \"/fo\"          ) == -1);\n+\tassert(longest_ancestor_length(\"/foo/bar\", \"/foo\"         ) ==  4);\n+\tassert(longest_ancestor_length(\"/foo/bar\", \"/foo/\"        ) ==  4);\n+\tassert(longest_ancestor_length(\"/foo/bar\", \"/foo/ba\"      ) == -1);\n+\tassert(longest_ancestor_length(\"/foo/bar\", \"/:/fo\"        ) ==  0);\n+\tassert(longest_ancestor_length(\"/foo/bar\", \"/foo:/foo/ba\" ) ==  4);\n+\tassert(longest_ancestor_length(\"/foo/bar\", \"/bar\"         ) == -1);\n+\tassert(longest_ancestor_length(\"/foo/bar\", \"/bar/\"        ) == -1);\n+\tassert(longest_ancestor_length(\"/foo/bar\", \"/fo:\"         ) == -1);\n+\tassert(longest_ancestor_length(\"/foo/bar\", \":/fo\"         ) == -1);\n+\tassert(longest_ancestor_length(\"/foo/bar\", \"/foo:/bar/\"   ) ==  4);\n+\tassert(longest_ancestor_length(\"/foo/bar\", \"/:/foo:/bar/\" ) ==  4);\n+\tassert(longest_ancestor_length(\"/foo/bar\", \"/foo:/:/bar/\" ) ==  4);\n+\tassert(longest_ancestor_length(\"/foo/bar\", \"/:/bar/:/fo\"  ) ==  0);\n+\tassert(longest_ancestor_length(\"/foo/bar\", \"/:/bar/\"      ) ==  0);\n+}\n+#endif\n+\n+/*\n  * We cannot decide in this function whether we are in the work tree or\n  * not, since the config can only be read _after_ this function was called.\n  */\n const char *setup_git_directory_gently(int *nongit_ok)\n {\n \tconst char *work_tree_env = getenv(GIT_WORK_TREE_ENVIRONMENT);\n+\tconst char *env_ceiling_dirs = getenv(CEILING_DIRS_ENVIRONMENT);\n \tstatic char cwd[PATH_MAX+1];\n \tconst char *gitdirenv;\n \tconst char *gitfile_dir;\n-\tint len, offset;\n+\tint len, offset, ceil_offset;\n \n \t/*\n \t * Let's assume that we are in a git repository.\n@@ -415,6 +517,14 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \t\tdie(\"Unable to read current working directory\");\n \n \t/*\n+\t * Compute ceil_offset based on GIT_CEILING_DIRS.  It is actually the offset\n+\t * of the first character in cwd after the trailing slash of the ceiling.\n+\t * Putting it so far to the right is necessary in order to bail out of the\n+\t * \"--offset\" loop early enough.\n+\t */\n+\tceil_offset = 1 + longest_ancestor_length(cwd, env_ceiling_dirs);\n+\n+\t/*\n \t * Test in the following order (relative to the cwd):\n \t * - .git (file containing \"gitdir: <path>\")\n \t * - .git/\n@@ -443,9 +553,8 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \t\t\tcheck_repository_format_gently(nongit_ok);\n \t\t\treturn NULL;\n \t\t}\n-\t\tchdir(\"..\");\n \t\tdo {\n-\t\t\tif (!offset) {\n+\t\t\tif (offset <= ceil_offset) {\n \t\t\t\tif (nongit_ok) {\n \t\t\t\t\tif (chdir(cwd))\n \t\t\t\t\t\tdie(\"Cannot come back to cwd\");\n@@ -455,6 +564,7 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \t\t\t\tdie(\"Not a git repository\");\n \t\t\t}\n \t\t} while (cwd[--offset] != '/');\n+\t\tchdir(\"..\");\n \t}\n \n \tinside_git_dir = 0;\ndiff --git a/t/t1504-ceiling-dirs.sh b/t/t1504-ceiling-dirs.sh\nnew file mode 100755\nindex 0000000..091baad\n--- /dev/null\n+++ b/t/t1504-ceiling-dirs.sh\n@@ -0,0 +1,156 @@\n+#!/bin/sh\n+\n+test_description='test GIT_CEILING_DIRS'\n+. ./test-lib.sh\n+\n+test_prefix() {\n+\ttest_expect_success \"$1\" \\\n+\t\"test '$2' = \\\"\\$(git rev-parse --show-prefix)\\\"\"\n+}\n+\n+test_fail() {\n+\ttest_expect_code 128 \"$1: prefix\" \\\n+\t\"git rev-parse --show-prefix\"\n+}\n+\n+TRASH_ROOT=\"$(pwd)\"\n+ROOT_PARENT=$(dirname \"$TRASH_ROOT\")\n+\n+\n+unset GIT_CEILING_DIRS\n+test_prefix no_ceil \"\"\n+\n+export GIT_CEILING_DIRS=\"\"\n+test_prefix ceil_empty \"\"\n+\n+export GIT_CEILING_DIRS=\"$ROOT_PARENT\"\n+test_prefix ceil_at_parent \"\"\n+\n+export GIT_CEILING_DIRS=\"$ROOT_PARENT/\"\n+test_prefix ceil_at_parent_slash \"\"\n+\n+export GIT_CEILING_DIRS=\"$TRASH_ROOT\"\n+test_prefix ceil_at_trash \"\"\n+\n+export GIT_CEILING_DIRS=\"$TRASH_ROOT/\"\n+test_prefix ceil_at_trash_slash \"\"\n+\n+export GIT_CEILING_DIRS=\"$TRASH_ROOT/sub\"\n+test_prefix ceil_at_sub \"\"\n+\n+export GIT_CEILING_DIRS=\"$TRASH_ROOT/sub/\"\n+test_prefix ceil_at_sub_slash \"\"\n+\n+\n+mkdir -p sub/dir || exit 1\n+cd sub/dir || exit 1\n+\n+unset GIT_CEILING_DIRS\n+test_prefix subdir_no_ceil \"sub/dir/\"\n+\n+export GIT_CEILING_DIRS=\"\"\n+test_prefix subdir_ceil_empty \"sub/dir/\"\n+\n+export GIT_CEILING_DIRS=\"$TRASH_ROOT\"\n+test_fail subdir_ceil_at_trash\n+\n+export GIT_CEILING_DIRS=\"$TRASH_ROOT/\"\n+test_fail subdir_ceil_at_trash_slash\n+\n+export GIT_CEILING_DIRS=\"$TRASH_ROOT/sub\"\n+test_fail subdir_ceil_at_sub\n+\n+export GIT_CEILING_DIRS=\"$TRASH_ROOT/sub/\"\n+test_fail subdir_ceil_at_sub_slash\n+\n+export GIT_CEILING_DIRS=\"$TRASH_ROOT/sub/dir\"\n+test_prefix subdir_ceil_at_subdir \"sub/dir/\"\n+\n+export GIT_CEILING_DIRS=\"$TRASH_ROOT/sub/dir/\"\n+test_prefix subdir_ceil_at_subdir_slash \"sub/dir/\"\n+\n+\n+export GIT_CEILING_DIRS=\"$TRASH_ROOT/su\"\n+test_prefix subdir_ceil_at_su \"sub/dir/\"\n+\n+export GIT_CEILING_DIRS=\"$TRASH_ROOT/su/\"\n+test_prefix subdir_ceil_at_su_slash \"sub/dir/\"\n+\n+export GIT_CEILING_DIRS=\"$TRASH_ROOT/sub/di\"\n+test_prefix subdir_ceil_at_sub_di \"sub/dir/\"\n+\n+export GIT_CEILING_DIRS=\"$TRASH_ROOT/sub/di\"\n+test_prefix subdir_ceil_at_sub_di_slash \"sub/dir/\"\n+\n+export GIT_CEILING_DIRS=\"$TRASH_ROOT/subdi\"\n+test_prefix subdir_ceil_at_subdi \"sub/dir/\"\n+\n+export GIT_CEILING_DIRS=\"$TRASH_ROOT/subdi\"\n+test_prefix subdir_ceil_at_subdi_slash \"sub/dir/\"\n+\n+\n+export GIT_CEILING_DIRS=\"foo:$TRASH_ROOT/sub\"\n+test_fail second_of_two\n+\n+export GIT_CEILING_DIRS=\"$TRASH_ROOT/sub:bar\"\n+test_fail first_of_two\n+\n+export GIT_CEILING_DIRS=\"foo:$TRASH_ROOT/sub:bar\"\n+test_fail second_of_three\n+\n+\n+export GIT_CEILING_DIRS=\"$TRASH_ROOT/sub\"\n+export GIT_DIR=../../.git\n+test_prefix git_dir_specified \"\"\n+unset GIT_DIR\n+\n+\n+cd ../.. || exit 1\n+mkdir -p s/d || exit 1\n+cd s/d || exit 1\n+\n+unset GIT_CEILING_DIRS\n+test_prefix sd_no_ceil \"s/d/\"\n+\n+export GIT_CEILING_DIRS=\"\"\n+test_prefix sd_ceil_empty \"s/d/\"\n+\n+export GIT_CEILING_DIRS=\"$TRASH_ROOT\"\n+test_fail sd_ceil_at_trash\n+\n+export GIT_CEILING_DIRS=\"$TRASH_ROOT/\"\n+test_fail sd_ceil_at_trash_slash\n+\n+export GIT_CEILING_DIRS=\"$TRASH_ROOT/s\"\n+test_fail sd_ceil_at_s\n+\n+export GIT_CEILING_DIRS=\"$TRASH_ROOT/s/\"\n+test_fail sd_ceil_at_s_slash\n+\n+export GIT_CEILING_DIRS=\"$TRASH_ROOT/s/d\"\n+test_prefix sd_ceil_at_sd \"s/d/\"\n+\n+export GIT_CEILING_DIRS=\"$TRASH_ROOT/s/d/\"\n+test_prefix sd_ceil_at_sd_slash \"s/d/\"\n+\n+\n+export GIT_CEILING_DIRS=\"$TRASH_ROOT/su\"\n+test_prefix sd_ceil_at_su \"s/d/\"\n+\n+export GIT_CEILING_DIRS=\"$TRASH_ROOT/su/\"\n+test_prefix sd_ceil_at_su_slash \"s/d/\"\n+\n+export GIT_CEILING_DIRS=\"$TRASH_ROOT/s/di\"\n+test_prefix sd_ceil_at_s_di \"s/d/\"\n+\n+export GIT_CEILING_DIRS=\"$TRASH_ROOT/s/di\"\n+test_prefix sd_ceil_at_s_di_slash \"s/d/\"\n+\n+export GIT_CEILING_DIRS=\"$TRASH_ROOT/sdi\"\n+test_prefix sd_ceil_at_sdi \"s/d/\"\n+\n+export GIT_CEILING_DIRS=\"$TRASH_ROOT/sdi\"\n+test_prefix sd_ceil_at_sdi_slash \"s/d/\"\n+\n+\n+test_done\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 7c2a8ba..22899c1 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -35,6 +35,7 @@ unset GIT_WORK_TREE\n unset GIT_EXTERNAL_DIFF\n unset GIT_INDEX_FILE\n unset GIT_OBJECT_DIRECTORY\n+unset GIT_CEILING_DIRS\n unset SHA1_FILE_DIRECTORIES\n unset SHA1_FILE_DIRECTORY\n GIT_MERGE_VERBOSITY=5\n-- \n1.5.4\n"},{"id":"77026","messageId":"482BE0EB.6040306@viscovery.net","threadId":"13523","inReplyTo":"482B935D.20105@facebook.com","subject":"Re: [PATCH v2] Add support for GIT_CEILING_DIRS","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-05-15T07:06:19Z","receivedAt":"2008-05-15T07:06:19Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"David Reiss schrieb:\n>  const char *setup_git_directory_gently(int *nongit_ok)\n>  {\n>  \tconst char *work_tree_env = getenv(GIT_WORK_TREE_ENVIRONMENT);\n> +\tconst char *env_ceiling_dirs = getenv(CEILING_DIRS_ENVIRONMENT);\n>  \tstatic char cwd[PATH_MAX+1];\n>  \tconst char *gitdirenv;\n>  \tconst char *gitfile_dir;\n> -\tint len, offset;\n> +\tint len, offset, ceil_offset;\n>  \n>  \t/*\n>  \t * Let's assume that we are in a git repository.\n> @@ -415,6 +517,14 @@ const char *setup_git_directory_gently(int *nongit_ok)\n>  \t\tdie(\"Unable to read current working directory\");\n>  \n>  \t/*\n> +\t * Compute ceil_offset based on GIT_CEILING_DIRS.  It is actually the offset\n> +\t * of the first character in cwd after the trailing slash of the ceiling.\n> +\t * Putting it so far to the right is necessary in order to bail out of the\n> +\t * \"--offset\" loop early enough.\n> +\t */\n> +\tceil_offset = 1 + longest_ancestor_length(cwd, env_ceiling_dirs);\n> +\n> +\t/*\n>  \t * Test in the following order (relative to the cwd):\n>  \t * - .git (file containing \"gitdir: <path>\")\n>  \t * - .git/\n> @@ -443,9 +553,8 @@ const char *setup_git_directory_gently(int *nongit_ok)\n>  \t\t\tcheck_repository_format_gently(nongit_ok);\n>  \t\t\treturn NULL;\n>  \t\t}\n> -\t\tchdir(\"..\");\n>  \t\tdo {\n> -\t\t\tif (!offset) {\n> +\t\t\tif (offset <= ceil_offset) {\n>  \t\t\t\tif (nongit_ok) {\n>  \t\t\t\t\tif (chdir(cwd))\n>  \t\t\t\t\t\tdie(\"Cannot come back to cwd\");\n> @@ -455,6 +564,7 @@ const char *setup_git_directory_gently(int *nongit_ok)\n>  \t\t\t\tdie(\"Not a git repository\");\n>  \t\t\t}\n>  \t\t} while (cwd[--offset] != '/');\n\nIf you make it so that the default value of ceil_offset is 0 (i.e. in the\nabsence of any GIT_CEILING_DIRS), and at this place you did\n\n\t\t} while (offset > ceil_offset && cwd[--offset] != '/');\n\nyou wouldn't have to bend backwards with this off-by-one magic, would you?\n(But I admit that I haven't tried this code, I'm only comparing it to how\nwe do it mingw.git.)\n\n-- Hannes\n"},{"id":"77027","messageId":"482BE238.5020309@facebook.com","threadId":"13523","inReplyTo":"482BE0EB.6040306@viscovery.net","subject":"Re: [PATCH v2] Add support for GIT_CEILING_DIRS","fromName":"David Reiss","fromEmail":"dreiss@facebook.com","sentAt":"2008-05-15T07:11:52Z","receivedAt":"2008-05-15T07:11:52Z","isPatch":true,"sender":{"key":"dreiss@facebook.com","avatar":null},"body":"> If you make it so that the default value of ceil_offset is 0 (i.e. in the\n> absence of any GIT_CEILING_DIRS),\nThis is what the new version of the patch does.\n\n> and at this place you did\n> \n>                 } while (offset > ceil_offset && cwd[--offset] != '/');\n> \n> you wouldn't have to bend backwards with this off-by-one magic, would you?\nIt seems like that would cause it to continue on with the outer loop, rather\nthan aborting (which is what the current version does) when you hit the ceiling.\nOr maybe I'm misunderstanding something.\n\n--David\n"},{"id":"77030","messageId":"482BF69C.6020604@viscovery.net","threadId":"13523","inReplyTo":"482BE238.5020309@facebook.com","subject":"Re: [PATCH v2] Add support for GIT_CEILING_DIRS","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-05-15T08:38:52Z","receivedAt":"2008-05-15T08:38:52Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"David Reiss schrieb:\n>> If you make it so that the default value of ceil_offset is 0 (i.e. in the\n>> absence of any GIT_CEILING_DIRS),\n> This is what the new version of the patch does.\n> \n>> and at this place you did\n>>\n>>                 } while (offset > ceil_offset && cwd[--offset] != '/');\n>>\n>> you wouldn't have to bend backwards with this off-by-one magic, would you?\n> It seems like that would cause it to continue on with the outer loop, rather\n> than aborting (which is what the current version does) when you hit the ceiling.\n> Or maybe I'm misunderstanding something.\n\nOk, I see. I'm proposing that the meaning of ceil_offset should be \"do not look\nat this many characters at the beginning of cwd\". And we always have\ncwd[ceil_offset] == '/'. Something like this on top of your patch.\nFurther tweaking will be necessary (I'm just illustrating my point),\nin particular you could name it min_offset again.\n\n-- Hannes\n\ndiff --git a/setup.c b/setup.c\nindex de9e7f1..5dbc080 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -368,13 +368,13 @@ const char *read_gitfile_gently(const char *path)\n static int longest_ancestor_length(const char *path, const char *prefix_list)\n {\n \tconst char *ceil, *colon;\n-\tint max_len = -1;\n+\tint max_len = 0;\n\n \tif (prefix_list == NULL)\n-\t\treturn -1;\n-\t/* \"/\" is a tricky edge case.  It should always return -1, though. */\n+\t\treturn 0;\n+\t/* \"/\" is a tricky edge case.  It should always return 0, though. */\n \tif (!strcmp(path, \"/\"))\n-\t\treturn -1;\n+\t\treturn 0;\n\n \tceil = prefix_list;\n \tfor (;;) {\n@@ -522,7 +522,7 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \t * Putting it so far to the right is necessary in order to bail out of the\n \t * \"--offset\" loop early enough.\n \t */\n-\tceil_offset = 1 + longest_ancestor_length(cwd, env_ceiling_dirs);\n+\tceil_offset = longest_ancestor_length(cwd, env_ceiling_dirs);\n\n \t/*\n \t * Test in the following order (relative to the cwd):\n@@ -553,17 +553,16 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \t\t\tcheck_repository_format_gently(nongit_ok);\n \t\t\treturn NULL;\n \t\t}\n-\t\tdo {\n-\t\t\tif (offset <= ceil_offset) {\n-\t\t\t\tif (nongit_ok) {\n-\t\t\t\t\tif (chdir(cwd))\n-\t\t\t\t\t\tdie(\"Cannot come back to cwd\");\n-\t\t\t\t\t*nongit_ok = 1;\n-\t\t\t\t\treturn NULL;\n-\t\t\t\t}\n-\t\t\t\tdie(\"Not a git repository\");\n+\t\tdo { } while (offset > ceil_offset && cwd[--offset] != '/');\n+\t\tif (offset <= ceil_offset) {\n+\t\t\tif (nongit_ok) {\n+\t\t\t\tif (chdir(cwd))\n+\t\t\t\t\tdie(\"Cannot come back to cwd\");\n+\t\t\t\t*nongit_ok = 1;\n+\t\t\t\treturn NULL;\n \t\t\t}\n-\t\t} while (cwd[--offset] != '/');\n+\t\t\tdie(\"Not a git repository\");\n+\t\t}\n \t\tchdir(\"..\");\n \t}\n"},{"id":"77034","messageId":"alpine.DEB.1.00.0805151004400.30431@racer","threadId":"13523","inReplyTo":"482BF69C.6020604@viscovery.net","subject":"Re: [PATCH v2] Add support for GIT_CEILING_DIRS","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-05-15T09:06:22Z","receivedAt":"2008-05-15T09:06:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 15 May 2008, Johannes Sixt wrote:\n\n> +\t\tdo { } while (offset > ceil_offset && cwd[--offset] != '/');\n\nYou probably meant to remove the \"do { }\", and have an own line\n\n\t\t\t; /* do nothing */\n\nbut for the rest, I agree that it is easier on the eye (particularly the \noff-by-one issue, which is always a problem for this developer to get \nright; avoiding it is therefore the better option).\n\nCiao,\nDscho\n"},{"id":"77053","messageId":"482C644F.9090903@facebook.com","threadId":"13523","inReplyTo":"alpine.DEB.1.00.0805151004400.30431@racer","subject":"Re: [PATCH v2] Add support for GIT_CEILING_DIRS","fromName":"David Reiss","fromEmail":"dreiss@facebook.com","sentAt":"2008-05-15T16:26:55Z","receivedAt":"2008-05-15T16:26:55Z","isPatch":true,"sender":{"key":"dreiss@facebook.com","avatar":null},"body":"The problem with this implementation is that it does not distinguish\nbetween GIT_CEILING_DIRS being unset and GIT_CEILING_DIRS=\"/\".  For\nexample...\n\ncd /\nsudo git init\ncd /home\ngit rev-parse --show-prefix\n\nThat series of commands works with either version of my patch, but fails\nwith \"fatal: Not a git repository\" if I apply this change.  I am\ncertainly open to changing this code, but I think we will always\nneed two separate values of ceil_offset to represent \"unset\" and \"/\".\nIt's just a question of whether they should be -1 and 0 or 0 and 1.\n\n--David\n\n\nJohannes Schindelin wrote:\n> Hi,\n> \n> On Thu, 15 May 2008, Johannes Sixt wrote:\n> \n>> +             do { } while (offset > ceil_offset && cwd[--offset] != '/');\n> \n> You probably meant to remove the \"do { }\", and have an own line\n> \n>                         ; /* do nothing */\n> \n> but for the rest, I agree that it is easier on the eye (particularly the\n> off-by-one issue, which is always a problem for this developer to get\n> right; avoiding it is therefore the better option).\n> \n> Ciao,\n> Dscho\n> \n"},{"id":"77056","messageId":"alpine.DEB.1.00.0805151841040.30431@racer","threadId":"13523","inReplyTo":"482C644F.9090903@facebook.com","subject":"Re: [PATCH v2] Add support for GIT_CEILING_DIRS","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-05-15T17:45:44Z","receivedAt":"2008-05-15T17:45:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 15 May 2008, David Reiss wrote:\n\n> The problem with this implementation is that it does not distinguish \n> between GIT_CEILING_DIRS being unset and GIT_CEILING_DIRS=\"/\".  For \n> example...\n> \n> cd /\n> sudo git init\n> cd /home\n> git rev-parse --show-prefix\n> \n> That series of commands works with either version of my patch, but fails \n> with \"fatal: Not a git repository\" if I apply this change.  I am \n> certainly open to changing this code, but I think we will always need \n> two separate values of ceil_offset to represent \"unset\" and \"/\". It's \n> just a question of whether they should be -1 and 0 or 0 and 1.\n\nYou are much more familiar with the code, but I suspect that a simple \nchange would fix that:\n\n> >> +             do { } while (offset > ceil_offset && cwd[--offset] != '/');\n\nJust use \"--offset >= 0 && cwd[--offset] != '/'\" here.  And maybe \nceil_offset = -1 again.\n\nI cannot quickly test, since I am short on time, and it would be too \ncumbersome to find which patches to apply first.\n\nBut I strongly believe that it is not beyond your capabilities to adjust \nHannes' patch for your command series, keeping the elegance.\n\nCiao,\nDscho\n"}]}