{"thread":{"id":"11211","subject":"[PATCH WIP] Move worktree setup out of setup_git_directory*","startedAt":"2007-12-09T10:24:32Z","lastAt":"2007-12-09T10:24:32Z","messageCount":1,"participants":["Nguyễn Thái Ngọc Duy"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"62475","messageId":"20071209102432.GA18362@laptop","threadId":"11211","inReplyTo":null,"subject":"[PATCH WIP] Move worktree setup out of setup_git_directory*","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2007-12-09T10:24:32Z","receivedAt":"2007-12-09T10:24:32Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"setup_git_directory* now work as if there is no worktree.\nIt may set some worktree-related variables but its prefix\n(and current directory) should not be affected by worktree\nsettings.\n\nWhile at it, put set_work_tree() code into\nsetup_git_directory_gently() for cleaner code.\n\nPreviously only setup_git_directory() takes worktree into\naccount when calculating prefix (with an exception). This\nbehaviour possibly gives commands that do only call\nsetup_git_directory_gently() wrong prefix. The behavior does\nnot expose often as setup_git_directory_gently() tries to\nmake correct prefix if both GIT_DIR and GIT_WORK_TREE\nare set (the mentioned exception). Only if core.worktree\nis used, the defect is summoned. Additional test is added\nto catch this.\n\nsetup_work_tree() will now take the role of recalculating\nprefix when worktree is required.\n\nThis also effectively reverts\ndd5c8af176bb935a0b01a7dc2d5e022565c3aac3. The problem is IMHO\nthat git-diff does not setup worktree when it needs it, so\nsetting up worktree from setup_git_directory_gently() is a fix\nin a wrong place.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n Now I'm pretty sure I made it wrong in builtin-diff.c. But I\n found no better solution as diff territory is too far from my home.\n\n One more thing, with this change, \"git ls-files -c\" would show\n full path as it does not require a worktree, hence no worktree setup,\n NULL prefix or so. Is it bad?\n\n builtin-blame.c     |    4 +-\n builtin-diff.c      |    7 ++++-\n builtin-ls-files.c  |   10 +++---\n builtin-rev-parse.c |    2 +\n builtin-rm.c        |    2 +-\n cache.h             |    2 +-\n git.c               |    2 +-\n setup.c             |   80 ++++++++++++++++----------------------------------\n t/t1501-worktree.sh |    8 ++++-\n 9 files changed, 51 insertions(+), 66 deletions(-)\n\ndiff --git a/builtin-blame.c b/builtin-blame.c\nindex c158d31..160a1cd 100644\n--- a/builtin-blame.c\n+++ b/builtin-blame.c\n@@ -2280,6 +2280,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \t\t\t/* (3) */\n \t\t\tif (argc <= i)\n \t\t\t\tusage(blame_usage);\n+\t\t\tprefix = setup_work_tree(prefix);\n \t\t\tpath = add_prefix(prefix, argv[i]);\n \t\t\tif (i + 1 == argc - 1) {\n \t\t\t\tfinal_commit_name = argv[i + 1];\n@@ -2295,7 +2296,6 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \t\t\telse if (i != argc - 1)\n \t\t\t\tusage(blame_usage); /* garbage at end */\n \n-\t\t\tsetup_work_tree();\n \t\t\tif (!has_path_in_work_tree(path))\n \t\t\t\tdie(\"cannot stat path %s: %s\",\n \t\t\t\t    path, strerror(errno));\n@@ -2343,7 +2343,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \t\t * do not default to HEAD, but use the working tree\n \t\t * or \"--contents\".\n \t\t */\n-\t\tsetup_work_tree();\n+\t\tprefix = setup_work_tree(prefix);\n \t\tsb.final = fake_working_tree_commit(path, contents_from);\n \t\tadd_pending_object(&revs, &(sb.final->object), \":\");\n \t}\ndiff --git a/builtin-diff.c b/builtin-diff.c\nindex 1b61599..24c3080 100644\n--- a/builtin-diff.c\n+++ b/builtin-diff.c\n@@ -246,8 +246,13 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \n \tif (!setup_diff_no_index(&rev, argc, argv, nongit, prefix))\n \t\targc = 0;\n-\telse\n+\telse {\n+\t\tprefix = setup_work_tree(prefix);\n+\t\t/* reinitialize rev as prefix can change */\n+\t\tinit_revisions(&rev, prefix);\n+\t\trev.diffopt.skip_stat_unmatch = !!diff_auto_refresh_index;\n \t\targc = setup_revisions(argc, argv, &rev, NULL);\n+\t}\n \tif (!rev.diffopt.output_format) {\n \t\trev.diffopt.output_format = DIFF_FORMAT_PATCH;\n \t\tif (diff_setup_done(&rev.diffopt) < 0)\ndiff --git a/builtin-ls-files.c b/builtin-ls-files.c\nindex 0f0ab2d..3da6d0c 100644\n--- a/builtin-ls-files.c\n+++ b/builtin-ls-files.c\n@@ -23,7 +23,7 @@ static int show_valid_bit;\n static int line_terminator = '\\n';\n \n static int prefix_len;\n-static int prefix_offset;\n+static int prefix_offset = -1;\n static const char **pathspec;\n static int error_unmatch;\n static char *ps_matched;\n@@ -433,8 +433,6 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)\n \tstruct dir_struct dir;\n \n \tmemset(&dir, 0, sizeof(dir));\n-\tif (prefix)\n-\t\tprefix_offset = strlen(prefix);\n \tgit_config(git_default_config);\n \n \tfor (i = 1; i < argc; i++) {\n@@ -566,8 +564,10 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)\n \t\tbreak;\n \t}\n \n-\tif (require_work_tree && !is_inside_work_tree())\n-\t\tsetup_work_tree();\n+\tif (require_work_tree)\n+\t\tprefix = setup_work_tree(prefix);\n+\tif (prefix_offset == -1)\n+\t\tprefix_offset = prefix ? strlen(prefix) : 0;\n \n \tpathspec = get_pathspec(prefix, argv + i);\n \ndiff --git a/builtin-rev-parse.c b/builtin-rev-parse.c\nindex 20d1789..e48c7ea 100644\n--- a/builtin-rev-parse.c\n+++ b/builtin-rev-parse.c\n@@ -441,6 +441,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--show-prefix\")) {\n+\t\t\t\tif (is_inside_work_tree())\n+\t\t\t\t\tprefix = setup_work_tree(prefix);\n \t\t\t\tif (prefix)\n \t\t\t\t\tputs(prefix);\n \t\t\t\tcontinue;\ndiff --git a/builtin-rm.c b/builtin-rm.c\nindex a3d25e6..920a18f 100644\n--- a/builtin-rm.c\n+++ b/builtin-rm.c\n@@ -156,7 +156,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(builtin_rm_usage, builtin_rm_options);\n \n \tif (!index_only)\n-\t\tsetup_work_tree();\n+\t\tprefix = setup_work_tree(prefix);\n \n \tpathspec = get_pathspec(prefix, argv);\n \tseen = NULL;\ndiff --git a/cache.h b/cache.h\nindex b5811ec..08d7bf6 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -229,7 +229,7 @@ extern const char *get_git_work_tree(void);\n #define ALTERNATE_DB_ENVIRONMENT \"GIT_ALTERNATE_OBJECT_DIRECTORIES\"\n \n extern const char **get_pathspec(const char *prefix, const char **pathspec);\n-extern void setup_work_tree(void);\n+extern const char *setup_work_tree(const char *prefix);\n extern const char *setup_git_directory_gently(int *);\n extern const char *setup_git_directory(void);\n extern const char *prefix_path(const char *prefix, int len, const char *path);\ndiff --git a/git.c b/git.c\nindex c8b7e74..4b3b6b9 100644\n--- a/git.c\n+++ b/git.c\n@@ -259,7 +259,7 @@ static int run_command(struct cmd_struct *p, int argc, const char **argv)\n \tif (p->option & USE_PAGER)\n \t\tsetup_pager();\n \tif (p->option & NEED_WORK_TREE)\n-\t\tsetup_work_tree();\n+\t\tprefix = setup_work_tree(prefix);\n \n \ttrace_argv_printf(argv, \"trace: built-in: git\");\n \ndiff --git a/setup.c b/setup.c\nindex b59dbe7..e23ee8d 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -188,38 +188,25 @@ int is_inside_work_tree(void)\n \treturn inside_work_tree;\n }\n \n-/*\n- * set_work_tree() is only ever called if you set GIT_DIR explicitely.\n- * The old behaviour (which we retain here) is to set the work tree root\n- * to the cwd, unless overridden by the config, the command line, or\n- * GIT_WORK_TREE.\n- */\n-static const char *set_work_tree(const char *dir)\n-{\n-\tchar buffer[PATH_MAX + 1];\n-\n-\tif (!getcwd(buffer, sizeof(buffer)))\n-\t\tdie (\"Could not get the current working directory\");\n-\tgit_work_tree_cfg = xstrdup(buffer);\n-\tinside_work_tree = 1;\n-\n-\treturn NULL;\n-}\n-\n-void setup_work_tree(void)\n+const char *setup_work_tree(const char *prefix)\n {\n \tconst char *work_tree, *git_dir;\n-\tstatic int initialized = 0;\n+\tstatic char buffer[PATH_MAX + 1];\n+\tchar *rel;\n \n-\tif (initialized)\n-\t\treturn;\n-\twork_tree = get_git_work_tree();\n \tgit_dir = get_git_dir();\n \tif (!is_absolute_path(git_dir))\n \t\tset_git_dir(make_absolute_path(git_dir));\n-\tif (!work_tree || chdir(work_tree))\n+\twork_tree = get_git_work_tree();\n+\tif (!work_tree)\n+\t\tdie(\"This operation must be run in a work tree\");\n+\tif (prefix && chdir(prefix))\n+\t\tdie (\"Could not jump back into original cwd\");\n+\trel = get_relative_cwd(buffer, PATH_MAX, work_tree);\n+\ttrace_printf(\"test: worktree = %s\\n\", rel ? rel : \"NULL\");\n+\tif (chdir(work_tree))\n \t\tdie(\"This operation must be run in a work tree\");\n-\tinitialized = 1;\n+\treturn rel && *rel ? strcat(rel, \"/\") : NULL;\n }\n \n static int check_repository_format_gently(int *nongit_ok)\n@@ -262,24 +249,21 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \t\t\tstatic char buffer[1024 + 1];\n \t\t\tconst char *retval;\n \n+\t\t\t/*\n+\t\t\t * The old behaviour (which we retain here) is to set\n+\t\t\t * the work tree root to the cwd, unless overridden by\n+\t\t\t * the config, the command line, or GIT_WORK_TREE.\n+\t\t\t */\n \t\t\tif (!work_tree_env) {\n-\t\t\t\tretval = set_work_tree(gitdirenv);\n-\t\t\t\t/* config may override worktree */\n-\t\t\t\tif (check_repository_format_gently(nongit_ok))\n-\t\t\t\t\treturn NULL;\n-\t\t\t\treturn retval;\n+\t\t\t\tchar buffer[PATH_MAX + 1];\n+\n+\t\t\t\tif (!getcwd(buffer, sizeof(buffer)))\n+\t\t\t\t\tdie (\"Could not get the current working directory\");\n+\t\t\t\tgit_work_tree_cfg = xstrdup(buffer);\n+\t\t\t\tinside_work_tree = 1;\n \t\t\t}\n-\t\t\tif (check_repository_format_gently(nongit_ok))\n-\t\t\t\treturn NULL;\n-\t\t\tretval = get_relative_cwd(buffer, sizeof(buffer) - 1,\n-\t\t\t\t\tget_git_work_tree());\n-\t\t\tif (!retval || !*retval)\n-\t\t\t\treturn NULL;\n-\t\t\tset_git_dir(make_absolute_path(gitdirenv));\n-\t\t\tif (chdir(work_tree_env) < 0)\n-\t\t\t\tdie (\"Could not chdir to %s\", work_tree_env);\n-\t\t\tstrcat(buffer, \"/\");\n-\t\t\treturn retval;\n+\t\t\tcheck_repository_format_gently(nongit_ok);\n+\t\t\treturn NULL;\n \t\t}\n \t\tif (nongit_ok) {\n \t\t\t*nongit_ok = 1;\n@@ -387,17 +371,5 @@ int check_repository_format(void)\n \n const char *setup_git_directory(void)\n {\n-\tconst char *retval = setup_git_directory_gently(NULL);\n-\n-\t/* If the work tree is not the default one, recompute prefix */\n-\tif (inside_work_tree < 0) {\n-\t\tstatic char buffer[PATH_MAX + 1];\n-\t\tchar *rel;\n-\t\tif (retval && chdir(retval))\n-\t\t\tdie (\"Could not jump back into original cwd\");\n-\t\trel = get_relative_cwd(buffer, PATH_MAX, get_git_work_tree());\n-\t\treturn rel && *rel ? strcat(rel, \"/\") : NULL;\n-\t}\n-\n-\treturn retval;\n+\treturn setup_git_directory_gently(NULL);\n }\ndiff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh\nindex 7ee3820..3b610ac 100755\n--- a/t/t1501-worktree.sh\n+++ b/t/t1501-worktree.sh\n@@ -58,6 +58,12 @@ cd sub/dir || exit 1\n test_rev_parse 'subdirectory' false false true sub/dir/\n cd ../../.. || exit 1\n \n+test_expect_success 'setup_work_tree() gives correct prefix' '\n+\t(cd work/sub && touch untracked &&\n+\ttest \"$(git ls-files -o)\" = untracked)'\n+\n+rm work/sub/untracked || exit 1\n+\n say \"GIT_WORK_TREE=relative path (override core.worktree)\"\n export GIT_DIR=$(pwd)/repo.git\n export GIT_CONFIG=$GIT_DIR/config\n@@ -103,7 +109,7 @@ test_expect_success 'repo finds its work tree from work tree, too' '\n \t test sub/dir/tracked = \"$(git ls-files)\")\n '\n \n-test_expect_success '_gently() groks relative GIT_DIR & GIT_WORK_TREE' '\n+test_expect_success '\"git diff\" setup worktree properly' '\n \tcd repo.git/work/sub/dir &&\n \tGIT_DIR=../../.. GIT_WORK_TREE=../.. GIT_PAGER= \\\n \t\tgit diff --exit-code tracked &&\n-- \n1.5.3.7.2155.g4c25\n"}]}