{"thread":{"id":"12280","subject":"[PATCH] Move worktree setup out of setup_git_directory*","startedAt":"2008-02-24T04:46:51Z","lastAt":"2008-02-25T19:01:08Z","messageCount":4,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Nguyen Thai Ngoc Duy"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"69758","messageId":"20080224044650.GA25343@laptop","threadId":"12280","inReplyTo":null,"subject":"[PATCH] Move worktree setup out of setup_git_directory*","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2008-02-24T04:46:51Z","receivedAt":"2008-02-24T04:46:51Z","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.  It may set\nsome worktree-related variables but its prefix (and current directory)\nshould not be affected by worktree settings.\n\nPreviously only setup_git_directory() takes worktree into account when\ncalculating prefix (with an exception). This behaviour possibly gives\ncommands that do only call setup_git_directory_gently() wrong prefix.\nThe behavior does not expose often as setup_git_directory_gently()\ntries to make correct prefix if both GIT_DIR and GIT_WORK_TREE are set\n(the mentioned exception). Only if core.worktree is used, the defect\nis summoned.\n\nsetup_work_tree() will now take the role of recalculating prefix when\nworktree is required.\n\nThis also effectively reverts dd5c8af (Fix\nsetup_git_directory_gently() with relative GIT_DIR & GIT_WORK_TREE).\nThe problem is IMHO that git-diff does not setup worktree when it\nneeds it, so setting up worktree from setup_git_directory_gently() is\na fix in a wrong place.\n\nWhile at it, fix builtin-diff-files.c as well with the same problem.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n setup_git_directory() is now just a thin wrapper around _gently(). Sweet.\n\n builtin-blame.c      |    4 +-\n builtin-diff-files.c |    4 ++-\n builtin-diff.c       |    5 ++-\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  |   30 ++++++++++++++++++-\n 10 files changed, 74 insertions(+), 67 deletions(-)\n\ndiff --git a/builtin-blame.c b/builtin-blame.c\nindex bfd562d..f5a878b 100644\n--- a/builtin-blame.c\n+++ b/builtin-blame.c\n@@ -2291,6 +2291,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@@ -2306,7 +2307,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@@ -2354,7 +2354,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\tsetup_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-files.c b/builtin-diff-files.c\nindex 4abe3c2..e3cc5cc 100644\n--- a/builtin-diff-files.c\n+++ b/builtin-diff-files.c\n@@ -26,8 +26,10 @@ int cmd_diff_files(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\trev.prefix = prefix = setup_work_tree(prefix);\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_RAW;\n \tresult = run_diff_files_cmd(&rev, argc, argv);\ndiff --git a/builtin-diff.c b/builtin-diff.c\nindex 444ff2f..93221a0 100644\n--- a/builtin-diff.c\n+++ b/builtin-diff.c\n@@ -244,8 +244,11 @@ 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\trev.prefix = prefix = setup_work_tree(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 25dbfb4..4d833e1 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@@ -435,8 +435,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@@ -568,8 +566,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 b9af1a5..9f05cb3 100644\n--- a/builtin-rev-parse.c\n+++ b/builtin-rev-parse.c\n@@ -472,6 +472,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 c0a8bb6..2c15d66 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 a5b9ffd..dc768db 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -282,7 +282,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 4f804a2..90451ee 100644\n--- a/git.c\n+++ b/git.c\n@@ -252,7 +252,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 89c81e5..b181b24 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -262,38 +262,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@@ -336,24 +323,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@@ -462,17 +446,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..2b1629b 100755\n--- a/t/t1501-worktree.sh\n+++ b/t/t1501-worktree.sh\n@@ -58,6 +58,34 @@ cd sub/dir || exit 1\n test_rev_parse 'subdirectory' false false true sub/dir/\n cd ../../.. || exit 1\n \n+test_expect_success 'git ls-files -o gets 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+cat <<EOF >expected\n+:100644 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0000000000000000000000000000000000000000 M\tsub/tracked\n+EOF\n+\n+test_expect_success 'git diff-files gets correct prefix' '\n+\t(cd work/sub  && touch tracked &&\n+\tgit add tracked && echo modified > tracked &&\n+\tgit diff-files > ../../result &&\n+\tgit rm -f tracked) &&\n+\tcmp result expected'\n+\n+cat <<EOF >expected\n+:100644 100644 e69de29... 0000000... M\tsub/tracked\n+EOF\n+\n+test_expect_success 'git diff gets correct prefix' '\n+\t(cd work/sub  && touch tracked &&\n+\tgit add tracked && echo modified > tracked &&\n+\tgit diff --raw > ../../result &&\n+\tgit rm -f tracked) &&\n+\tcmp result expected'\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 +131,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.4.2.281.g28d0e\n"},{"id":"69836","messageId":"7vr6f1sur3.fsf@gitster.siamese.dyndns.org","threadId":"12280","inReplyTo":"20080224044650.GA25343@laptop","subject":"Re: [PATCH] Move worktree setup out of setup_git_directory*","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-25T01:13:52Z","receivedAt":"2008-02-25T01:13:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:\n\n> setup_git_directory* now work as if there is no worktree.  It may set\n> some worktree-related variables but its prefix (and current directory)\n> should not be affected by worktree settings.\n> ...\n> setup_work_tree() will now take the role of recalculating prefix when\n> worktree is required.\n\nThis definitely needs documented in-tree, Documentation/technical/.\n\nI think we have a good understanding from the end-user point-of-view\nhow these things work:\n\n - If you have GIT_DIR exported, then no discovery is attempted.\n   We use the GIT_DIR you set it, and the repository lives\n   there.  $GIT_DIR/config is the repository config.\n\n - Otherwise we do the usual discovery going up to find the\n   repository.\n\n - If you have GIT_WORK_TREE exported, or otherwise if the\n   config has core.worktree, that's where your worktree is.\n   Otherwise, if you have GIT_DIR exported, you do not have a\n   worktree.  Else one level above your $GIT_DIR is the toplevel\n   of your worktree.\n\nWe would need a set of rules for application writers, written\nlike this (here is just an example of _how_ the rules might be\nwritten, I do not think the contents are correct):\n\n - At startup:\n\n   - If the command always need to run from a repository, call\n     setup_git_directory(), which would complain and die if you\n     are outside.\n\n   - If the command can optionally run in a repository, use\n     setup_git_directory_gently(&nongit_ok); it won't complain\n     but sets nongit_ok to true if run outside a repository.\n\n   In either case, they return prefix string to be used for\n   setup_work_tree() later.\n\n   In either case, there is no chdir() done at this point.\n\n - After calling setup_git_directory(), or you made sure you are\n   in a repository after callilng setup_git_directory_gently(),\n   call setup_work_tree().  This will chdir to the top of the\n   work tree.\n\n - When you need to know where $GIT_DIR is, call get_git_dir().\n   When you need to know where the work tree is, call\n   get_git_work_tree().  However, it is an error to call these\n   functions before calling setup_git_directory and\n   setup_work_tree.\n"},{"id":"69881","messageId":"fcaeb9bf0802250517x2192b51epf2a162b0a984e99c@mail.gmail.com","threadId":"12280","inReplyTo":"7vr6f1sur3.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Move worktree setup out of setup_git_directory*","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2008-02-25T13:17:36Z","receivedAt":"2008-02-25T13:17:36Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Feb 25, 2008 at 8:13 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>   - At startup:\n>\n>    - If the command always need to run from a repository, call\n>      setup_git_directory(), which would complain and die if you\n>      are outside.\n>\n>    - If the command can optionally run in a repository, use\n>      setup_git_directory_gently(&nongit_ok); it won't complain\n>      but sets nongit_ok to true if run outside a repository.\n>\n>    In either case, they return prefix string to be used for\n>    setup_work_tree() later.\n>\n>    In either case, there is no chdir() done at this point.\n\nInteresting. It currently does chdir(). But it is pointless now as\nworktree should not be accessed until setup_work_tree() is called.\nsetup_git_directory* should not return a prefix at all.\n\n>   - After calling setup_git_directory(), or you made sure you are\n>    in a repository after callilng setup_git_directory_gently(),\n>    call setup_work_tree().  This will chdir to the top of the\n>    work tree.\n>\n>   - When you need to know where $GIT_DIR is, call get_git_dir().\n>    When you need to know where the work tree is, call\n>    get_git_work_tree().  However, it is an error to call these\n>    functions before calling setup_git_directory and\n>    setup_work_tree.\n\nI guess I just need to verify your statements and put them to\nDoc/technical/api-setup.txt ;)\n-- \nDuy\n"},{"id":"69903","messageId":"7v7igskgi3.fsf@gitster.siamese.dyndns.org","threadId":"12280","inReplyTo":"fcaeb9bf0802250517x2192b51epf2a162b0a984e99c@mail.gmail.com","subject":"Re: [PATCH] Move worktree setup out of setup_git_directory*","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-25T19:01:08Z","receivedAt":"2008-02-25T19:01:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Nguyen Thai Ngoc Duy\" <pclouds@gmail.com> writes:\n\n> Interesting. It currently does chdir(). But it is pointless now as\n> worktree should not be accessed until setup_work_tree() is called.\n> setup_git_directory* should not return a prefix at all.\n> ...\n> I guess I just need to verify your statements and put them to\n> Doc/technical/api-setup.txt ;)\n\nThe above, as I said, was written out of thin air without\nre-reading the current code nor carefully reading what your\npatch does, so I am afraid there needs a lot more than that\nto happen before we have something to validate the callers\nagainst, but I think you got why I think it is a good idea to\nhave that documented.\n"}]}