{"thread":{"id":"48158","subject":"git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.","startedAt":"2018-03-26T21:27:56Z","lastAt":"2018-03-30T19:54:42Z","messageCount":54,"participants":["Rafael Ascensao","Ævar Arnfjörð Bjarmason","Jeff King","Duy Nguyen","Nguyễn Thái Ngọc Duy","Eric Sunshine","Stefan Beller","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"343068","messageId":"CACUQV5_3Pw+vnyyNUL4oE4tMLG_wKVdqdVk01rg4V92ufUYHHA@mail.gmail.com","threadId":"48158","inReplyTo":null,"subject":"git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.","fromName":"Rafael Ascensao","fromEmail":"rafa.almas@gmail.com","sentAt":"2018-03-26T21:27:09Z","receivedAt":"2018-03-26T21:27:56Z","isPatch":false,"sender":{"key":"rafa.almas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1923789?v=4"},"body":"One of the tools that manages PKGBUILDS for Arch Linux stores PKGBUILD\ngit repos inside a cache directory for reuse.\n\nOne of the repos is triggering some unexpected behaviour that can be\nreproduced in the CLI with:\n\n $ GIT_DIR=spotify/.git GIT_WORK_TREE=spotify git reset HEAD\n fatal: couldn't read spotify/.git/packed-refs: Not a directory\n\nMoving up one directory works as expected.\n $ cd ..\n $ GIT_DIR=sync/spotify/.git GIT_WORK_TREE=sync/spotify git reset HEAD\n\nUsing -C seems to work fine too.\n $ git -C spotify reset HEAD\n\n$ GIT_TRACE_SETUP=2 GIT_TRACE=2 GIT_DIR=\"spotify/.git\"\nGIT_WORK_TREE=\"spotify\" git reset HEAD\n22:10:53.020525 trace.c:315 setup: git_dir: spotify/.git\n22:10:53.020605 trace.c:316 setup: git_common_dir: spotify/.git\n22:10:53.020616 trace.c:317 setup: worktree:\n/home/rafasc/.cache/aurutils/sync/spotify\n22:10:53.020625 trace.c:318 setup: cwd:\n/home/rafasc/.cache/aurutils/sync\n22:10:53.020635 trace.c:319 setup: prefix: (null)\n22:10:53.020644 git.c:344 trace: built-in: git 'reset' 'HEAD'\nfatal: couldn't read spotify/.git/packed-refs: Not a directory\n\nThe issue seems to bisect to f57f37e2e1bf11ab4cdfd221ad47e961ba9353a0\nI can't pinpoint why this particular repo is behaving differently.\n\nCumprimentos,\nRafael Ascensão\n"},{"id":"343074","messageId":"87woxywlzh.fsf@evledraar.gmail.com","threadId":"48158","inReplyTo":"CACUQV5_3Pw+vnyyNUL4oE4tMLG_wKVdqdVk01rg4V92ufUYHHA@mail.gmail.com","subject":"Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-03-26T21:44:18Z","receivedAt":"2018-03-26T21:44:26Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Mar 26 2018, Rafael Ascensao wrote:\n\n> One of the tools that manages PKGBUILDS for Arch Linux stores PKGBUILD\n> git repos inside a cache directory for reuse.\n>\n> One of the repos is triggering some unexpected behaviour that can be\n> reproduced in the CLI with:\n>\n> $ GIT_DIR=spotify/.git GIT_WORK_TREE=spotify git reset HEAD\n> fatal: couldn't read spotify/.git/packed-refs: Not a directory\n>\n> Moving up one directory works as expected.\n> $ cd ..\n> $ GIT_DIR=sync/spotify/.git GIT_WORK_TREE=sync/spotify git reset HEAD\n>\n> Using -C seems to work fine too.\n> $ git -C spotify reset HEAD\n>\n> $ GIT_TRACE_SETUP=2 GIT_TRACE=2 GIT_DIR=\"spotify/.git\"\n> GIT_WORK_TREE=\"spotify\" git reset HEAD\n> 22:10:53.020525 trace.c:315 setup: git_dir: spotify/.git\n> 22:10:53.020605 trace.c:316 setup: git_common_dir: spotify/.git\n> 22:10:53.020616 trace.c:317 setup: worktree:\n> /home/rafasc/.cache/aurutils/sync/spotify\n> 22:10:53.020625 trace.c:318 setup: cwd:\n> /home/rafasc/.cache/aurutils/sync\n> 22:10:53.020635 trace.c:319 setup: prefix: (null)\n> 22:10:53.020644 git.c:344 trace: built-in: git 'reset' 'HEAD'\n> fatal: couldn't read spotify/.git/packed-refs: Not a directory\n>\n> The issue seems to bisect to f57f37e2e1bf11ab4cdfd221ad47e961ba9353a0\n> I can't pinpoint why this particular repo is behaving differently.\n\nCC-ing Duy, the author of that commit.\n"},{"id":"343110","messageId":"20180327063137.GA24044@sigill.intra.peff.net","threadId":"48158","inReplyTo":"CACUQV5_3Pw+vnyyNUL4oE4tMLG_wKVdqdVk01rg4V92ufUYHHA@mail.gmail.com","subject":"Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-27T06:31:38Z","receivedAt":"2018-03-27T06:31:45Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 26, 2018 at 10:27:09PM +0100, Rafael Ascensao wrote:\n\n> One of the tools that manages PKGBUILDS for Arch Linux stores PKGBUILD\n> git repos inside a cache directory for reuse.\n> \n> One of the repos is triggering some unexpected behaviour that can be\n> reproduced in the CLI with:\n> \n> $ GIT_DIR=spotify/.git GIT_WORK_TREE=spotify git reset HEAD\n> fatal: couldn't read spotify/.git/packed-refs: Not a directory\n> [...]\n> The issue seems to bisect to f57f37e2e1bf11ab4cdfd221ad47e961ba9353a0\n> I can't pinpoint why this particular repo is behaving differently.\n\nI think we're getting confused by the relative paths. Here's a related\nreproduction:\n\n $ git init repo\n $ git -C repo commit --allow-empty -m foo\n $ GIT_DIR=repo/.git GIT_WORK_TREE=repo git reset HEAD\n $ find repo/repo\n repo/repo\n repo/repo/.git\n repo/repo/.git/logs\n repo/repo/.git/logs/HEAD\n repo/repo/.git/HEAD\n\nEr, what? It looks like we kept looking at \"repo/.git\" as our git\ndirectory, even though we should have normalized it into an absolute\npath after moving into the root of the work-tree.\n\nI can also reproduce your exact error by inserting:\n\n git -C repo pack-refs\n echo whatever >repo/repo\n\nbefore the call to \"git reset\" (and then we get ENOTDIR trying to read\nthe packed-refs file, because the file \"repo\" is in the way).\n\nLooking at f57f37e2, I think the problem is that files_ref_store_create()\nsaves the value of get_git_dir() at that point. But later after we\nchdir for the working tree, presumably it's updated, but we continue to\nuse the out-dated relative path.\n\nSo one \"fix\" is something like this:\n\ndiff --git a/refs.c b/refs.c\nindex 20ba82b434..449bdf2437 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1643,11 +1643,14 @@ static struct ref_store *ref_store_init(const char *gitdir,\n \tconst char *be_name = \"files\";\n \tstruct ref_storage_be *be = find_ref_storage_backend(be_name);\n \tstruct ref_store *refs;\n+\tchar *abs_gitdir;\n \n \tif (!be)\n \t\tdie(\"BUG: reference backend %s is unknown\", be_name);\n \n-\trefs = be->init(gitdir, flags);\n+\tabs_gitdir = absolute_pathdup(gitdir);\n+\trefs = be->init(abs_gitdir, flags);\n+\tfree(abs_gitdir);\n \treturn refs;\n }\n \n\nBut that really feels like we're papering over the problem. It's not\nclear to me exactly what f57f37e2 is trying to accomplish, and whether\nit would work for it to look call get_git_dir() whenever it needed the\npath.\n\n-Peff\n"},{"id":"343131","messageId":"CACsJy8DP53Og1crS1bLoJf6w8cJhjGKy=ggfbsqzJ6AU4eNhPA@mail.gmail.com","threadId":"48158","inReplyTo":"20180327063137.GA24044@sigill.intra.peff.net","subject":"Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-03-27T14:56:00Z","receivedAt":"2018-03-27T14:56:36Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Mar 27, 2018 at 8:31 AM, Jeff King wrote:\n>...\n>\n> But that really feels like we're papering over the problem.\n\nIt is papering over the problem in my opinion. setup_work_tree() is\ninvolved here and when it moves cwd around, it re-init git-dir (and\nall other paths). Before my patch we call git_path() only when we need\nit and git-dir is likely already updated by setup_work_tree(). After\nthis patch, the path is set in stone before setup_work_tree() kicks\nin. Once it moves cwd, relative paths all become invalid.\n\nThe way setup_work_tree() does it is just bad to me. When it calls\nset_git_dir() again, the assumption is the new path is exactly the\nsame as the old one. The only difference is relative vs absolute. But\nthis is super hard to see from set_git_dir implementation. The new\nstruct repository design also inherits this (i.e. allowing to call\nset_git_dir multiple times, which really does not make sense), but\nthis won't fly long term. When cwd moves, all cached relative paths\nmust be adjusted, we need a better mechanism to tell everybody that,\nnot just do it for $GIT_DIR and related paths.\n\nI am planning to fix this. This is part of the \"setup cleanup\" effort\nto keep repository.c design less messy and hopefully unify how the\nmain repo and submodule repo are created/set up. But frankly that may\ntake a long while before I could submit anything substantial (I also\nhave the \"report multiple worktree's HEADs correctly and make fsck\ncount all HEADs\" task, which is not small and to me have higher\npriority).\n\nSo I would not mind papering over it right now (with an understanding\nthat absolute path pays some more overhead in path walking, which was\nth reason we tried to avoid it in setup code). A slightly better patch\nis trigger this path absolutization from setup_work_tree(), near\nset_git_dir(). But then you face some ugliness there: how to find out\nall ref stores to update, or just update the main ref store alone.\n\n> It's not\n> clear to me exactly what f57f37e2 is trying to accomplish, and whether\n> it would work for it to look call get_git_dir() whenever it needed the\n> path.\n>\n> -Peff\n-- \nDuy\n"},{"id":"343148","messageId":"20180327164757.GB24747@sigill.intra.peff.net","threadId":"48158","inReplyTo":"CACsJy8DP53Og1crS1bLoJf6w8cJhjGKy=ggfbsqzJ6AU4eNhPA@mail.gmail.com","subject":"Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-27T16:47:58Z","receivedAt":"2018-03-27T16:48:08Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 27, 2018 at 04:56:00PM +0200, Duy Nguyen wrote:\n\n> The way setup_work_tree() does it is just bad to me. When it calls\n> set_git_dir() again, the assumption is the new path is exactly the\n> same as the old one. The only difference is relative vs absolute. But\n> this is super hard to see from set_git_dir implementation. The new\n> struct repository design also inherits this (i.e. allowing to call\n> set_git_dir multiple times, which really does not make sense), but\n> this won't fly long term. When cwd moves, all cached relative paths\n> must be adjusted, we need a better mechanism to tell everybody that,\n> not just do it for $GIT_DIR and related paths.\n\nYeah, I agree it's questionable.\n\n> I am planning to fix this. This is part of the \"setup cleanup\" effort\n> to keep repository.c design less messy and hopefully unify how the\n> main repo and submodule repo are created/set up. But frankly that may\n> take a long while before I could submit anything substantial (I also\n> have the \"report multiple worktree's HEADs correctly and make fsck\n> count all HEADs\" task, which is not small and to me have higher\n> priority).\n> \n> So I would not mind papering over it right now (with an understanding\n> that absolute path pays some more overhead in path walking, which was\n> th reason we tried to avoid it in setup code). A slightly better patch\n> is trigger this path absolutization from setup_work_tree(), near\n> set_git_dir(). But then you face some ugliness there: how to find out\n> all ref stores to update, or just update the main ref store alone.\n\nI don't quite get why f57f37e2 doesn't want to call git_path(). Is it to\navoid the way the path is munged? Or is it to avoid some lazy-setup that\nis triggered by calling get_git_dir() at all (which doesn't make much\nsense to me, because we'd already have called get_git_dir() much\nearlier). Or is it just because we may sometimes fill in refs->git_dir\nwith something besides get_git_dir() (for a submodule or worktree or\nsomething)?\n\nI.e., can we do one of (depending on which of those answers is \"yes\"):\n\n 1. Stop caching the value of get_git_dir(), and instead call it\n on-demand instead of looking at refs->git_dir? (If we just want to\n avoid git_path()).\n\n 2. If we need to avoid even calling get_git_dir(), can we add a\n \"light\" version of it that avoids whatever side effects we're\n trying to skip?\n\n 3. If the problem is just that sometimes we need get_git_dir() and\n sometimes not, could we perhaps store NULL as a sentinel to mean\n \"look up get_git_dir() when you need it\"?\n\n That would let submodules and worktrees fill in their paths as\n necessary (assuming they never change after init), but handle the\n case of get_git_dir() changing.\n\nHmm. Typing that out, it seems like (3) is probably the right path.\nSomething like the patch below seems to fix it and passes the tests.\n\ndiff --git a/refs.c b/refs.c\nindex 20ba82b434..4094f0e7d4 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1656,7 +1656,7 @@ struct ref_store *get_main_ref_store(void)\n \tif (main_ref_store)\n \t\treturn main_ref_store;\n \n-\tmain_ref_store = ref_store_init(get_git_dir(), REF_STORE_ALL_CAPS);\n+\tmain_ref_store = ref_store_init(NULL, REF_STORE_ALL_CAPS);\n \treturn main_ref_store;\n }\n \ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex bec8e30e9e..22118b5764 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -69,6 +69,7 @@ struct files_ref_store {\n \tstruct ref_store base;\n \tunsigned int store_flags;\n \n+\t/* may be NULL to force lookup of get_git_dir() */\n \tchar *gitdir;\n \tchar *gitcommondir;\n \n@@ -94,17 +95,23 @@ static struct ref_store *files_ref_store_create(const char *gitdir,\n {\n \tstruct files_ref_store *refs = xcalloc(1, sizeof(*refs));\n \tstruct ref_store *ref_store = (struct ref_store *)refs;\n-\tstruct strbuf sb = STRBUF_INIT;\n \n \tbase_ref_store_init(ref_store, &refs_be_files);\n \trefs->store_flags = flags;\n \n-\trefs->gitdir = xstrdup(gitdir);\n-\tget_common_dir_noenv(&sb, gitdir);\n-\trefs->gitcommondir = strbuf_detach(&sb, NULL);\n-\tstrbuf_addf(&sb, \"%s/packed-refs\", refs->gitcommondir);\n-\trefs->packed_ref_store = packed_ref_store_create(sb.buf, flags);\n-\tstrbuf_release(&sb);\n+\tif (gitdir) {\n+\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\trefs->gitdir = xstrdup(gitdir);\n+\t\tget_common_dir_noenv(&sb, gitdir);\n+\t\trefs->gitcommondir = strbuf_detach(&sb, NULL);\n+\t\tstrbuf_addf(&sb, \"%s/packed-refs\", refs->gitcommondir);\n+\t\trefs->packed_ref_store = packed_ref_store_create(sb.buf, flags);\n+\t\tstrbuf_release(&sb);\n+\t} else {\n+\t\trefs->gitdir = NULL;\n+\t\trefs->gitcommondir = NULL;\n+\t\trefs->packed_ref_store = packed_ref_store_create(NULL, flags);\n+\t}\n \n \treturn ref_store;\n }\n@@ -147,6 +154,16 @@ static struct files_ref_store *files_downcast(struct ref_store *ref_store,\n \treturn refs;\n }\n \n+static const char *files_gitdir(const struct files_ref_store *refs)\n+{\n+\treturn refs->gitdir ? refs->gitdir : get_git_dir();\n+}\n+\n+static const char *files_gitcommondir(const struct files_ref_store *refs)\n+{\n+\treturn refs->gitcommondir ? refs->gitcommondir : get_git_common_dir();\n+}\n+\n static void files_reflog_path(struct files_ref_store *refs,\n \t\t\t struct strbuf *sb,\n \t\t\t const char *refname)\n@@ -154,10 +171,10 @@ static void files_reflog_path(struct files_ref_store *refs,\n \tswitch (ref_type(refname)) {\n \tcase REF_TYPE_PER_WORKTREE:\n \tcase REF_TYPE_PSEUDOREF:\n-\t\tstrbuf_addf(sb, \"%s/logs/%s\", refs->gitdir, refname);\n+\t\tstrbuf_addf(sb, \"%s/logs/%s\", files_gitdir(refs), refname);\n \t\tbreak;\n \tcase REF_TYPE_NORMAL:\n-\t\tstrbuf_addf(sb, \"%s/logs/%s\", refs->gitcommondir, refname);\n+\t\tstrbuf_addf(sb, \"%s/logs/%s\", files_gitcommondir(refs), refname);\n \t\tbreak;\n \tdefault:\n \t\tdie(\"BUG: unknown ref type %d of ref %s\",\n@@ -172,10 +189,10 @@ static void files_ref_path(struct files_ref_store *refs,\n \tswitch (ref_type(refname)) {\n \tcase REF_TYPE_PER_WORKTREE:\n \tcase REF_TYPE_PSEUDOREF:\n-\t\tstrbuf_addf(sb, \"%s/%s\", refs->gitdir, refname);\n+\t\tstrbuf_addf(sb, \"%s/%s\", files_gitdir(refs), refname);\n \t\tbreak;\n \tcase REF_TYPE_NORMAL:\n-\t\tstrbuf_addf(sb, \"%s/%s\", refs->gitcommondir, refname);\n+\t\tstrbuf_addf(sb, \"%s/%s\", files_gitcommondir(refs), refname);\n \t\tbreak;\n \tdefault:\n \t\tdie(\"BUG: unknown ref type %d of ref %s\",\n@@ -2151,13 +2168,13 @@ static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_st\n \t\tfiles_downcast(ref_store, REF_STORE_READ,\n \t\t\t \"reflog_iterator_begin\");\n \n-\tif (!strcmp(refs->gitdir, refs->gitcommondir)) {\n-\t\treturn reflog_iterator_begin(ref_store, refs->gitcommondir);\n+\tif (!strcmp(files_gitdir(refs), files_gitcommondir(refs))) {\n+\t\treturn reflog_iterator_begin(ref_store, files_gitcommondir(refs));\n \t} else {\n \t\treturn merge_ref_iterator_begin(\n \t\t\t0,\n-\t\t\treflog_iterator_begin(ref_store, refs->gitdir),\n-\t\t\treflog_iterator_begin(ref_store, refs->gitcommondir),\n+\t\t\treflog_iterator_begin(ref_store, files_gitdir(refs)),\n+\t\t\treflog_iterator_begin(ref_store, files_gitcommondir(refs)),\n \t\t\treflog_iterator_select, refs);\n \t}\n }\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 65288c6472..e5a5af1285 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -158,6 +158,16 @@ static void acquire_snapshot(struct snapshot *snapshot)\n \tsnapshot->referrers++;\n }\n \n+static const char *packed_refs_path(const struct packed_ref_store *refs)\n+{\n+\treturn refs->path ? refs->path : git_path(\"packed-refs\");\n+}\n+\n+static const char *snapshot_path(const struct snapshot *snapshot)\n+{\n+\treturn packed_refs_path(snapshot->refs);\n+}\n+\n /*\n * If the buffer in `snapshot` is active, then either munmap the\n * memory and close the file, or free the memory. Then set the buffer\n@@ -168,7 +178,7 @@ static void clear_snapshot_buffer(struct snapshot *snapshot)\n \tif (snapshot->mmapped) {\n \t\tif (munmap(snapshot->buf, snapshot->eof - snapshot->buf))\n \t\t\tdie_errno(\"error ummapping packed-refs file %s\",\n-\t\t\t\t snapshot->refs->path);\n+\t\t\t\t snapshot_path(snapshot));\n \t\tsnapshot->mmapped = 0;\n \t} else {\n \t\tfree(snapshot->buf);\n@@ -201,7 +211,7 @@ struct ref_store *packed_ref_store_create(const char *path,\n \tbase_ref_store_init(ref_store, &refs_be_packed);\n \trefs->store_flags = store_flags;\n \n-\trefs->path = xstrdup(path);\n+\trefs->path = xstrdup_or_null(path);\n \treturn ref_store;\n }\n \n@@ -342,7 +352,7 @@ static void sort_snapshot(struct snapshot *snapshot)\n \t\t\t/* The safety check should prevent this. */\n \t\t\tBUG(\"unterminated line found in packed-refs\");\n \t\tif (eol - pos < GIT_SHA1_HEXSZ + 2)\n-\t\t\tdie_invalid_line(snapshot->refs->path,\n+\t\t\tdie_invalid_line(snapshot_path(snapshot),\n \t\t\t\t\t pos, eof - pos);\n \t\teol++;\n \t\tif (eol < eof && *eol == '^') {\n@@ -454,7 +464,7 @@ static void verify_buffer_safe(struct snapshot *snapshot)\n \n \tlast_line = find_start_of_record(start, eof - 1);\n \tif (*(eof - 1) != '\\n' || eof - last_line < GIT_SHA1_HEXSZ + 2)\n-\t\tdie_invalid_line(snapshot->refs->path,\n+\t\tdie_invalid_line(snapshot_path(snapshot),\n \t\t\t\t last_line, eof - last_line);\n }\n \n@@ -473,7 +483,7 @@ static int load_contents(struct snapshot *snapshot)\n \tsize_t size;\n \tssize_t bytes_read;\n \n-\tfd = open(snapshot->refs->path, O_RDONLY);\n+\tfd = open(snapshot_path(snapshot), O_RDONLY);\n \tif (fd < 0) {\n \t\tif (errno == ENOENT) {\n \t\t\t/*\n@@ -485,14 +495,14 @@ static int load_contents(struct snapshot *snapshot)\n \t\t\t */\n \t\t\treturn 0;\n \t\t} else {\n-\t\t\tdie_errno(\"couldn't read %s\", snapshot->refs->path);\n+\t\t\tdie_errno(\"couldn't read %s\", snapshot_path(snapshot));\n \t\t}\n \t}\n \n \tstat_validity_update(&snapshot->validity, fd);\n \n \tif (fstat(fd, &st) < 0)\n-\t\tdie_errno(\"couldn't stat %s\", snapshot->refs->path);\n+\t\tdie_errno(\"couldn't stat %s\", snapshot_path(snapshot));\n \tsize = xsize_t(st.st_size);\n \n \tif (!size) {\n@@ -501,7 +511,7 @@ static int load_contents(struct snapshot *snapshot)\n \t\tsnapshot->buf = xmalloc(size);\n \t\tbytes_read = read_in_full(fd, snapshot->buf, size);\n \t\tif (bytes_read < 0 || bytes_read != size)\n-\t\t\tdie_errno(\"couldn't read %s\", snapshot->refs->path);\n+\t\t\tdie_errno(\"couldn't read %s\", snapshot_path(snapshot));\n \t\tsnapshot->mmapped = 0;\n \t} else {\n \t\tsnapshot->buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);\n@@ -629,14 +639,14 @@ static struct snapshot *create_snapshot(struct packed_ref_store *refs)\n \t\teol = memchr(snapshot->buf, '\\n',\n \t\t\t snapshot->eof - snapshot->buf);\n \t\tif (!eol)\n-\t\t\tdie_unterminated_line(refs->path,\n+\t\t\tdie_unterminated_line(packed_refs_path(refs),\n \t\t\t\t\t snapshot->buf,\n \t\t\t\t\t snapshot->eof - snapshot->buf);\n \n \t\ttmp = xmemdupz(snapshot->buf, eol - snapshot->buf);\n \n \t\tif (!skip_prefix(tmp, \"# pack-refs with:\", (const char **)&p))\n-\t\t\tdie_invalid_line(refs->path,\n+\t\t\tdie_invalid_line(packed_refs_path(refs),\n \t\t\t\t\t snapshot->buf,\n \t\t\t\t\t snapshot->eof - snapshot->buf);\n \n@@ -695,7 +705,8 @@ static struct snapshot *create_snapshot(struct packed_ref_store *refs)\n static void validate_snapshot(struct packed_ref_store *refs)\n {\n \tif (refs->snapshot &&\n-\t !stat_validity_check(&refs->snapshot->validity, refs->path))\n+\t !stat_validity_check(&refs->snapshot->validity,\n+\t\t\t\t packed_refs_path(refs)))\n \t\tclear_snapshot(refs);\n }\n \n@@ -739,7 +750,7 @@ static int packed_read_raw_ref(struct ref_store *ref_store,\n \t}\n \n \tif (get_oid_hex(rec, oid))\n-\t\tdie_invalid_line(refs->path, rec, snapshot->eof - rec);\n+\t\tdie_invalid_line(packed_refs_path(refs), rec, snapshot->eof - rec);\n \n \t*type = REF_ISPACKED;\n \treturn 0;\n@@ -795,12 +806,12 @@ static int next_record(struct packed_ref_iterator *iter)\n \tif (iter->eof - p < GIT_SHA1_HEXSZ + 2 ||\n \t parse_oid_hex(p, &iter->oid, &p) ||\n \t !isspace(*p++))\n-\t\tdie_invalid_line(iter->snapshot->refs->path,\n+\t\tdie_invalid_line(snapshot_path(iter->snapshot),\n \t\t\t\t iter->pos, iter->eof - iter->pos);\n \n \teol = memchr(p, '\\n', iter->eof - p);\n \tif (!eol)\n-\t\tdie_unterminated_line(iter->snapshot->refs->path,\n+\t\tdie_unterminated_line(snapshot_path(iter->snapshot),\n \t\t\t\t iter->pos, iter->eof - iter->pos);\n \n \tstrbuf_add(&iter->refname_buf, p, eol - p);\n@@ -825,7 +836,7 @@ static int next_record(struct packed_ref_iterator *iter)\n \t\tif (iter->eof - p < GIT_SHA1_HEXSZ + 1 ||\n \t\t parse_oid_hex(p, &iter->peeled, &p) ||\n \t\t *p++ != '\\n')\n-\t\t\tdie_invalid_line(iter->snapshot->refs->path,\n+\t\t\tdie_invalid_line(snapshot_path(iter->snapshot),\n \t\t\t\t\t iter->pos, iter->eof - iter->pos);\n \t\titer->pos = p;\n \n@@ -995,14 +1006,14 @@ int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)\n \t */\n \tif (hold_lock_file_for_update_timeout(\n \t\t\t &refs->lock,\n-\t\t\t refs->path,\n+\t\t\t packed_refs_path(refs),\n \t\t\t flags, timeout_value) < 0) {\n-\t\tunable_to_lock_message(refs->path, errno, err);\n+\t\tunable_to_lock_message(packed_refs_path(refs), errno, err);\n \t\treturn -1;\n \t}\n \n \tif (close_lock_file_gently(&refs->lock)) {\n-\t\tstrbuf_addf(err, \"unable to close %s: %s\", refs->path, strerror(errno));\n+\t\tstrbuf_addf(err, \"unable to close %s: %s\", packed_refs_path(refs), strerror(errno));\n \t\trollback_lock_file(&refs->lock);\n \t\treturn -1;\n \t}\n@@ -1471,21 +1482,21 @@ static int packed_transaction_finish(struct ref_store *ref_store,\n \t\t\tREF_STORE_READ | REF_STORE_WRITE | REF_STORE_ODB,\n \t\t\t\"ref_transaction_finish\");\n \tint ret = TRANSACTION_GENERIC_ERROR;\n-\tchar *packed_refs_path;\n+\tchar *path;\n \n \tclear_snapshot(refs);\n \n-\tpacked_refs_path = get_locked_file_path(&refs->lock);\n-\tif (rename_tempfile(&refs->tempfile, packed_refs_path)) {\n+\tpath = get_locked_file_path(&refs->lock);\n+\tif (rename_tempfile(&refs->tempfile, path)) {\n \t\tstrbuf_addf(err, \"error replacing %s: %s\",\n-\t\t\t refs->path, strerror(errno));\n+\t\t\t path, strerror(errno));\n \t\tgoto cleanup;\n \t}\n \n \tret = 0;\n \n cleanup:\n-\tfree(packed_refs_path);\n+\tfree(path);\n \tpacked_transaction_cleanup(refs, transaction);\n \treturn ret;\n }\n"},{"id":"343154","messageId":"CACsJy8Bog6U7X-jvzDhq14heQWx0HA_21HsSYR0nykU9aDsCYQ@mail.gmail.com","threadId":"48158","inReplyTo":"20180327164757.GB24747@sigill.intra.peff.net","subject":"Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-03-27T17:09:36Z","receivedAt":"2018-03-27T17:10:12Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Mar 27, 2018 at 6:47 PM, Jeff King wrote:\n>> So I would not mind papering over it right now (with an understanding\n>> that absolute path pays some more overhead in path walking, which was\n>> th reason we tried to avoid it in setup code). A slightly better patch\n>> is trigger this path absolutization from setup_work_tree(), near\n>> set_git_dir(). But then you face some ugliness there: how to find out\n>> all ref stores to update, or just update the main ref store alone.\n>\n> I don't quite get why f57f37e2 doesn't want to call git_path(). Is it to\n> avoid the way the path is munged? Or is it to avoid some lazy-setup that\n> is triggered by calling get_git_dir() at all (which doesn't make much\n> sense to me, because we'd already have called get_git_dir() much\n> earlier). Or is it just because we may sometimes fill in refs->git_dir\n> with something besides get_git_dir() (for a submodule or worktree or\n> something)?\n\nNone of those, I think. git_path() does some magic to translate paths\nso that refs/... ends up with $GIT_COMMON_DIR/refs/... while \"index\"\nends up with $GIT_DIR/index. Michael wanted to avoid that magic and\nkeep the control within refs code (i.e. this code knows refs/ and\npacked-refs are shared, and pseudo refs are not, what git_path()\ndecides does not matter).\n\n> I.e., can we do one of (depending on which of those answers is \"yes\"):\n>\n> 1. Stop caching the value of get_git_dir(), and instead call it\n> on-demand instead of looking at refs->git_dir? (If we just want to\n> avoid git_path()).\n\nThis probably works, but I count it as papering over the problem too.\n\n>\n> 2. If we need to avoid even calling get_git_dir(), can we add a\n> \"light\" version of it that avoids whatever side effects we're\n> trying to skip?\n>\n> 3. If the problem is just that sometimes we need get_git_dir() and\n> sometimes not, could we perhaps store NULL as a sentinel to mean\n> \"look up get_git_dir() when you need it\"?\n>\n> That would let submodules and worktrees fill in their paths as\n> necessary (assuming they never change after init), but handle the\n> case of get_git_dir() changing.\n>\n> Hmm. Typing that out, it seems like (3) is probably the right path.\n> Something like the patch below seems to fix it and passes the tests.\n\nHonestly I think this is just another way to work around the problem\n(with even more changes than your abspath approach). The problem is\nwith setup_work_tree(). We create a ref store at a specific location\nand it should stay working without lazily calling get_git_dir(), which\nhas nothing to do (anymore) with the path we have given a ref store.\nIf somebody changes a global setting like $CWD, it should be well\ncommunicated to everybody involved.\n\nI would rather have something like ref_store_reinit() in the same\nspirit as the second call of set_git_dir() in setup_work_tree. It is\nhacky, but it works and keeps changes to minimal (so that it could be\neasily replaced later).\n-- \nDuy\n"},{"id":"343156","messageId":"20180327173024.GA5017@duynguyen.home","threadId":"48158","inReplyTo":"CACsJy8Bog6U7X-jvzDhq14heQWx0HA_21HsSYR0nykU9aDsCYQ@mail.gmail.com","subject":"Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-03-27T17:30:24Z","receivedAt":"2018-03-27T17:30:46Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Mar 27, 2018 at 07:09:36PM +0200, Duy Nguyen wrote:\n> I would rather have something like ref_store_reinit() in the same\n> spirit as the second call of set_git_dir() in setup_work_tree. It is\n> hacky, but it works and keeps changes to minimal (so that it could be\n> easily replaced later).\n\nSo in the name of hacky and dirty things, it would look something like\nthis. This passed your test case. The test suite is still running\n(slow laptop) but I don't expect breakages there.\n\n-- 8< --\ndiff --git a/refs.c b/refs.c\nindex 20ba82b434..c6116c4f7a 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1660,6 +1660,16 @@ struct ref_store *get_main_ref_store(void)\n \treturn main_ref_store;\n }\n \n+void make_main_ref_store_use_absolute_paths(void)\n+{\n+\tfiles_force_absolute_paths(get_main_ref_store());\n+}\n+\n+void make_main_ref_store_use_relative_paths(const char *cwd)\n+{\n+\tfiles_make_relative_paths(get_main_ref_store(), cwd);\n+}\n+\n /*\n * Associate a ref store with a name. It is a fatal error to call this\n * function twice for the same name.\ndiff --git a/refs.h b/refs.h\nindex 01be5ae32f..532a4ad09d 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -759,6 +759,9 @@ int reflog_expire(const char *refname, const struct object_id *oid,\n int ref_storage_backend_exists(const char *name);\n \n struct ref_store *get_main_ref_store(void);\n+void make_main_ref_store_use_absolute_paths(void);\n+void make_main_ref_store_use_relative_paths(const char *cwd);\n+\n /*\n * Return the ref_store instance for the specified submodule. For the\n * main repository, use submodule==NULL; such a call cannot fail. For\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex bec8e30e9e..629198826f 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -3092,6 +3092,32 @@ static int files_reflog_expire(struct ref_store *ref_store,\n \treturn -1;\n }\n \n+void files_force_absolute_paths(struct ref_store *ref_store)\n+{\n+\tstruct files_ref_store *refs =\n+\t\tfiles_downcast(ref_store, REF_STORE_WRITE, \"don't ask\");\n+\n+\tchar *path = refs->gitdir;\n+\trefs->gitdir = absolute_pathdup(path);\n+\tfree(path);\n+\n+\tpath = refs->gitcommondir;\n+\trefs->gitcommondir = absolute_pathdup(path);\n+\tfree(path);\n+}\n+\n+void files_make_relative_paths(struct ref_store *ref_store, const char *cwd)\n+{\n+\tstruct files_ref_store *refs =\n+\t\tfiles_downcast(ref_store, REF_STORE_WRITE, \"don't ask\");\n+\n+\tconst char *path = remove_leading_path(refs->gitdir, cwd);\n+\trefs->gitdir = absolute_pathdup(path);\n+\n+\tpath = remove_leading_path(refs->gitcommondir, cwd);\n+\trefs->gitcommondir = absolute_pathdup(path);\n+}\n+\n static int files_init_db(struct ref_store *ref_store, struct strbuf *err)\n {\n \tstruct files_ref_store *refs =\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex dd834314bd..827e97bcca 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -669,4 +669,7 @@ struct ref_store {\n void base_ref_store_init(struct ref_store *refs,\n \t\t\t const struct ref_storage_be *be);\n \n+void files_force_absolute_paths(struct ref_store *refs);\n+void files_make_relative_paths(struct ref_store *refs, const char *cwd);\n+\n #endif /* REFS_REFS_INTERNAL_H */\ndiff --git a/setup.c b/setup.c\nindex 7287779642..a5f4396b4e 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -3,6 +3,7 @@\n #include \"config.h\"\n #include \"dir.h\"\n #include \"string-list.h\"\n+#include \"refs.h\"\n \n static int inside_git_dir = -1;\n static int inside_work_tree = -1;\n@@ -389,8 +390,10 @@ void setup_work_tree(void)\n \n \twork_tree = get_git_work_tree();\n \tgit_dir = get_git_dir();\n-\tif (!is_absolute_path(git_dir))\n+\tif (!is_absolute_path(git_dir)) {\n \t\tgit_dir = real_path(get_git_dir());\n+\t\tmake_main_ref_store_use_absolute_paths();\n+\t}\n \tif (!work_tree || chdir(work_tree))\n \t\tdie(_(\"this operation must be run in a work tree\"));\n \n@@ -402,6 +405,7 @@ void setup_work_tree(void)\n \t\tsetenv(GIT_WORK_TREE_ENVIRONMENT, \".\", 1);\n \n \tset_git_dir(remove_leading_path(git_dir, work_tree));\n+\tmake_main_ref_store_use_relative_paths(work_tree);\n \tinitialized = 1;\n }\n \n-- 8< --\n"},{"id":"343216","messageId":"20180328094733.GA1523@sigill.intra.peff.net","threadId":"48158","inReplyTo":"CACsJy8Bog6U7X-jvzDhq14heQWx0HA_21HsSYR0nykU9aDsCYQ@mail.gmail.com","subject":"Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-28T09:47:34Z","receivedAt":"2018-03-28T09:47:41Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 27, 2018 at 07:09:36PM +0200, Duy Nguyen wrote:\n\n> > I don't quite get why f57f37e2 doesn't want to call git_path(). Is it to\n> > avoid the way the path is munged? Or is it to avoid some lazy-setup that\n> > is triggered by calling get_git_dir() at all (which doesn't make much\n> > sense to me, because we'd already have called get_git_dir() much\n> > earlier). Or is it just because we may sometimes fill in refs->git_dir\n> > with something besides get_git_dir() (for a submodule or worktree or\n> > something)?\n> \n> None of those, I think. git_path() does some magic to translate paths\n> so that refs/... ends up with $GIT_COMMON_DIR/refs/... while \"index\"\n> ends up with $GIT_DIR/index. Michael wanted to avoid that magic and\n> keep the control within refs code (i.e. this code knows refs/ and\n> packed-refs are shared, and pseudo refs are not, what git_path()\n> decides does not matter).\n\nAh, OK (that is my first one, \"avoid the way the path is munged\", but\nobviously I didn't spell it out very clearly).\n\n> > Hmm. Typing that out, it seems like (3) is probably the right path.\n> > Something like the patch below seems to fix it and passes the tests.\n> \n> Honestly I think this is just another way to work around the problem\n> (with even more changes than your abspath approach). The problem is\n> with setup_work_tree(). We create a ref store at a specific location\n> and it should stay working without lazily calling get_git_dir(), which\n> has nothing to do (anymore) with the path we have given a ref store.\n> If somebody changes a global setting like $CWD, it should be well\n> communicated to everybody involved.\n\nYeah, I agree that the root of the problem is not the caching of\nget_git_dir(), but that chdir() may invalidate assumptions made by other\nparts of the program.\n\n> I would rather have something like ref_store_reinit() in the same\n> spirit as the second call of set_git_dir() in setup_work_tree. It is\n> hacky, but it works and keeps changes to minimal (so that it could be\n> easily replaced later).\n\nSo the non-hacky solution is to inform all callers that we've changed\ndirectories, and they may need to recompute any relative paths.\n\nIt does seem backwards for setup_work_tree() to need to know about the\nrefs code. Should we have a system by which interested code can register\nto learn about changes to global state? E.g., something like:\n\n typedef void (*chdir_notify_cb)(const char *old_cwd,\n const char *new_cwd,\n void *data);\n\n /* Register interest in hearing about chdir */\n void chdir_notify_register(chdir_notify_cb cb, void *data);\n\n /* Do a chdir and then tell everybody about it */\n void chdir_notify(const char *path);\n\nThen the ref code (or anybody else) should be able to write a function\nto normalize a relative path from the old_cwd into a relative one from\nthe new_cwd.\n\n-Peff\n"},{"id":"343217","messageId":"20180328095203.GB1523@sigill.intra.peff.net","threadId":"48158","inReplyTo":"20180327173024.GA5017@duynguyen.home","subject":"Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-28T09:52:03Z","receivedAt":"2018-03-28T09:52:11Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 27, 2018 at 07:30:24PM +0200, Duy Nguyen wrote:\n\n> On Tue, Mar 27, 2018 at 07:09:36PM +0200, Duy Nguyen wrote:\n> > I would rather have something like ref_store_reinit() in the same\n> > spirit as the second call of set_git_dir() in setup_work_tree. It is\n> > hacky, but it works and keeps changes to minimal (so that it could be\n> > easily replaced later).\n> \n> So in the name of hacky and dirty things, it would look something like\n> this. This passed your test case. The test suite is still running\n> (slow laptop) but I don't expect breakages there.\n\nI think this is the right direction. I mentioned in my last reply that\nit would be nice for this to be a bit more generic, in case we need to\nuse it again (and also just to keep the module boundaries sane).\n\nThis part confused me at first:\n\n> +void make_main_ref_store_use_absolute_paths(void)\n> +{\n> +\tfiles_force_absolute_paths(get_main_ref_store());\n> +}\n> +\n> +void make_main_ref_store_use_relative_paths(const char *cwd)\n> +{\n> +\tfiles_make_relative_paths(get_main_ref_store(), cwd);\n> +}\n\nsince I thought you were actually turning things into absolute paths.\nBut your procedure is basically \"turn absolute, then after chdir, turn\nthem back relative\".\n\nI think it might be clearer if a single call is given both the old and\nnew paths. That requires the caller of chdir() storing getcwd() before\nit moves, but I don't think that should be a big deal.\n\n-Peff\n"},{"id":"343218","messageId":"CACsJy8AVcPi=0i7s9o8cy-XGFuOLr72fe9WLRnEqo1SwLJEBOw@mail.gmail.com","threadId":"48158","inReplyTo":"20180328095203.GB1523@sigill.intra.peff.net","subject":"Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-03-28T10:10:21Z","receivedAt":"2018-03-28T10:10:58Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Mar 28, 2018 at 11:52 AM, Jeff King wrote:\n> On Tue, Mar 27, 2018 at 07:30:24PM +0200, Duy Nguyen wrote:\n>\n>> On Tue, Mar 27, 2018 at 07:09:36PM +0200, Duy Nguyen wrote:\n>> > I would rather have something like ref_store_reinit() in the same\n>> > spirit as the second call of set_git_dir() in setup_work_tree. It is\n>> > hacky, but it works and keeps changes to minimal (so that it could be\n>> > easily replaced later).\n>>\n>> So in the name of hacky and dirty things, it would look something like\n>> this. This passed your test case. The test suite is still running\n>> (slow laptop) but I don't expect breakages there.\n>\n> I think this is the right direction. I mentioned in my last reply that\n> it would be nice for this to be a bit more generic, in case we need to\n> use it again (and also just to keep the module boundaries sane).\n\nYes, that's why I called it hacky and dirty :) I keep thinking about\nthis, so I will probably fix it in a nicer way.\n\n> This part confused me at first:\n>\n>> +void make_main_ref_store_use_absolute_paths(void)\n>> +{\n>> + files_force_absolute_paths(get_main_ref_store());\n>> +}\n>> +\n>> +void make_main_ref_store_use_relative_paths(const char *cwd)\n>> +{\n>> + files_make_relative_paths(get_main_ref_store(), cwd);\n>> +}\n>\n> since I thought you were actually turning things into absolute paths.\n> But your procedure is basically \"turn absolute, then after chdir, turn\n> them back relative\".\n>\n> I think it might be clearer if a single call is given both the old and\n> new paths. That requires the caller of chdir() storing getcwd() before\n> it moves, but I don't think that should be a big deal.\n\nThe problem is switching relative paths relies on the old $CWD if I'm\nnot mistaken and we need getcwd() for this. I'd love to have one\ncallback that says \"$CWD has been switched from this path to that\npath, do whatever you need to\" that can be called any time, before or\nafter chdir(). I'll look more into it.\n-- \nDuy\n"},{"id":"343237","messageId":"20180328173656.GA29094@sigill.intra.peff.net","threadId":"48158","inReplyTo":"CACsJy8AVcPi=0i7s9o8cy-XGFuOLr72fe9WLRnEqo1SwLJEBOw@mail.gmail.com","subject":"Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-28T17:36:56Z","receivedAt":"2018-03-28T17:37:02Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 28, 2018 at 12:10:21PM +0200, Duy Nguyen wrote:\n\n> > I think it might be clearer if a single call is given both the old and\n> > new paths. That requires the caller of chdir() storing getcwd() before\n> > it moves, but I don't think that should be a big deal.\n> \n> The problem is switching relative paths relies on the old $CWD if I'm\n> not mistaken and we need getcwd() for this. I'd love to have one\n> callback that says \"$CWD has been switched from this path to that\n> path, do whatever you need to\" that can be called any time, before or\n> after chdir(). I'll look more into it.\n\nI think it should be OK to save getcwd() and just construct the original\npath after the fact. Here's some patches which do that in a nice way.\n\nFor those just joining us, this fixes a regression that was in v2.13 (so\nnothing we need to worry about as part of the 2.17-rc track).\n\n [1/4]: set_git_dir: die when setenv() fails\n [2/4]: add chdir-notify API\n [3/4]: set_work_tree: use chdir_notify\n [4/4]: refs: use chdir_notify to update cached relative paths\n\n Makefile | 1 +\n cache.h | 2 +-\n chdir-notify.c | 71 +++++++++++++++++++++++++++++++++++++++++++\n chdir-notify.h | 64 ++++++++++++++++++++++++++++++++++++++\n environment.c | 22 ++++++++++++--\n refs/files-backend.c | 4 +++\n refs/packed-backend.c | 3 ++\n setup.c | 9 ++----\n t/t1501-work-tree.sh | 12 ++++++++\n 9 files changed, 178 insertions(+), 10 deletions(-)\n create mode 100644 chdir-notify.c\n create mode 100644 chdir-notify.h\n\n-Peff\n"},{"id":"343238","messageId":"20180328173835.GA16274@sigill.intra.peff.net","threadId":"48158","inReplyTo":"20180328173656.GA29094@sigill.intra.peff.net","subject":"[PATCH 1/4] set_git_dir: die when setenv() fails","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-28T17:38:35Z","receivedAt":"2018-03-28T17:38:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The set_git_dir() function returns an error if setenv()\nfails, but there are zero callers who pay attention to this\nreturn value. If this ever were to happen, it could cause\nconfusing results, as sub-processes would see a potentially\nstale GIT_DIR (e.g., if it is relative and we chdir()-ed to\nthe root of the working tree).\n\nWe _could_ try to fix each caller, but there's really\nnothing useful to do after this failure except die. Let's\njust lump setenv() failure into the same category as malloc\nfailure: things that should never happen and cause us to\nabort catastrophically.\n\nSigned-off-by: Jeff King \n---\n cache.h | 2 +-\n environment.c | 5 ++---\n 2 files changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex a61b2d3f0d..5c24394d84 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -477,7 +477,7 @@ extern const char *get_git_common_dir(void);\n extern char *get_object_directory(void);\n extern char *get_index_file(void);\n extern char *get_graft_file(void);\n-extern int set_git_dir(const char *path);\n+extern void set_git_dir(const char *path);\n extern int get_common_dir_noenv(struct strbuf *sb, const char *gitdir);\n extern int get_common_dir(struct strbuf *sb, const char *gitdir);\n extern const char *get_git_namespace(void);\ndiff --git a/environment.c b/environment.c\nindex d6dd64662c..e01acf8b11 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -296,13 +296,12 @@ char *get_graft_file(void)\n \treturn the_repository->graft_file;\n }\n \n-int set_git_dir(const char *path)\n+void set_git_dir(const char *path)\n {\n \tif (setenv(GIT_DIR_ENVIRONMENT, path, 1))\n-\t\treturn error(\"Could not set GIT_DIR to '%s'\", path);\n+\t\tdie(\"could not set GIT_DIR to '%s'\", path);\n \trepo_set_gitdir(the_repository, path);\n \tsetup_git_env();\n-\treturn 0;\n }\n \n const char *get_log_output_encoding(void)\n-- \n2.17.0.rc1.515.g3cc84c0ca4\n\n"},{"id":"343239","messageId":"20180328174041.GB16274@sigill.intra.peff.net","threadId":"48158","inReplyTo":"20180328173656.GA29094@sigill.intra.peff.net","subject":"[PATCH 2/4] add chdir-notify API","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-28T17:40:41Z","receivedAt":"2018-03-28T17:40:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If one part of the code does a permanent chdir(), then this\ninvalidates any relative paths that may be held by other\nparts of the code. For example, setup_work_tree() moves us\nto the top of the working tree, which may invalidate a\npreviously stored relative gitdir.\n\nWe've hacked around this case by teaching setup_work_tree()\nto re-run set_git_dir() with an adjusted path, but this\nstomps all over the idea of module boundaries.\nsetup_work_tree() shouldn't have to know all of the places\nthat need to be fed an adjusted path. And indeed, there's at\nleast one other place (the refs code) which needs adjusting.\n\nLet's provide an API to let code that stores relative paths\n\"subscribe\" to updates to the current working directory.\nThis means that callers of chdir() don't need to know about\nall subscribers ahead of time; they can simply consult a\ndynamically built list.\n\nNote that our helper function to reparent relative paths\nuses the simple remove_leading_path(). We could in theory\nuse the much smarter relative_path(), but that led to some\nproblems as described in 41894ae3a3 (Use simpler\nrelative_path when set_git_dir, 2013-10-14). Since we're\naiming to replace the setup_work_tree() code here, let's\nfollow its lead.\n\nSigned-off-by: Jeff King \n---\n Makefile | 1 +\n chdir-notify.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++\n chdir-notify.h | 64 +++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 136 insertions(+)\n create mode 100644 chdir-notify.c\n create mode 100644 chdir-notify.h\n\ndiff --git a/Makefile b/Makefile\nindex a1d8775adb..0da98b9274 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -772,6 +772,7 @@ LIB_OBJS += branch.o\n LIB_OBJS += bulk-checkin.o\n LIB_OBJS += bundle.o\n LIB_OBJS += cache-tree.o\n+LIB_OBJS += chdir-notify.o\n LIB_OBJS += checkout.o\n LIB_OBJS += color.o\n LIB_OBJS += column.o\ndiff --git a/chdir-notify.c b/chdir-notify.c\nnew file mode 100644\nindex 0000000000..0b88bf0e5b\n--- /dev/null\n+++ b/chdir-notify.c\n@@ -0,0 +1,71 @@\n+#include \"cache.h\"\n+#include \"chdir-notify.h\"\n+#include \"list.h\"\n+#include \"strbuf.h\"\n+\n+struct chdir_notify_entry {\n+\tchdir_notify_callback cb;\n+\tvoid *data;\n+\tstruct list_head list;\n+};\n+static LIST_HEAD(chdir_notify_entries);\n+\n+void chdir_notify_register(chdir_notify_callback cb, void *data)\n+{\n+\tstruct chdir_notify_entry *e = xmalloc(sizeof(*e));\n+\te->cb = cb;\n+\te->data = data;\n+\tlist_add_tail(&e->list, &chdir_notify_entries);\n+}\n+\n+static void reparent_cb(const char *old_cwd,\n+\t\t\tconst char *new_cwd,\n+\t\t\tvoid *data)\n+{\n+\tchar **path = data;\n+\tchar *tmp = *path;\n+\t*path = reparent_relative_path(old_cwd, new_cwd, tmp);\n+\tfree(tmp);\n+}\n+\n+void chdir_notify_reparent(char **path)\n+{\n+\tif (!is_absolute_path(*path))\n+\t\tchdir_notify_register(reparent_cb, path);\n+}\n+\n+int chdir_notify(const char *new_cwd)\n+{\n+\tstruct strbuf old_cwd = STRBUF_INIT;\n+\tstruct list_head *pos;\n+\n+\tif (strbuf_getcwd(&old_cwd) < 0)\n+\t\treturn -1;\n+\tif (chdir(new_cwd) < 0)\n+\t\treturn -1;\n+\n+\tlist_for_each(pos, &chdir_notify_entries) {\n+\t\tstruct chdir_notify_entry *e =\n+\t\t\tlist_entry(pos, struct chdir_notify_entry, list);\n+\t\te->cb(old_cwd.buf, new_cwd, e->data);\n+\t}\n+\n+\tstrbuf_release(&old_cwd);\n+\treturn 0;\n+}\n+\n+char *reparent_relative_path(const char *old_cwd,\n+\t\t\t const char *new_cwd,\n+\t\t\t const char *path)\n+{\n+\tchar *ret, *full;\n+\n+\tif (is_absolute_path(path))\n+\t\treturn xstrdup(path);\n+\n+\tfull = xstrfmt(\"%s/%s\", old_cwd, path);\n+\tret = xstrdup(remove_leading_path(full, new_cwd));\n+\tfree(full);\n+\n+\treturn ret;\n+}\ndiff --git a/chdir-notify.h b/chdir-notify.h\nnew file mode 100644\nindex 0000000000..c3bd818a85\n--- /dev/null\n+++ b/chdir-notify.h\n@@ -0,0 +1,64 @@\n+#ifndef CHDIR_NOTIFY_H\n+#define CHDIR_NOTIFY_H\n+\n+/*\n+ * An API to let code \"subscribe\" to changes to the current working directory.\n+ * The general idea is that some code asks to be notified when the working\n+ * directory changes, and other code that calls chdir uses a special wrapper\n+ * that notifies everyone.\n+ */\n+\n+/*\n+ * Callers who need to know about changes can do:\n+ *\n+ * void foo(const char *old_path, const char *new_path, void *data)\n+ * {\n+ *\twarning(\"switched from %s to %s!\", old_path, new_path);\n+ * }\n+ * ...\n+ * chdir_notify_register(foo, data);\n+ *\n+ * In practice most callers will want to move a relative path to the new root;\n+ * they can use the reparent_relative_path() helper for that. If that's all\n+ * you're doing, you can also use the convenience function:\n+ *\n+ * chdir_notify_reparent(&my_path);\n+ *\n+ * Registered functions are called in the order in which they were added. Note\n+ * that there's currently no way to remove a function, so make sure that the\n+ * data parameter remains valid for the rest of the program.\n+ */\n+typedef void (*chdir_notify_callback)(const char *old_cwd,\n+\t\t\t\t const char *new_cwd,\n+\t\t\t\t void *data);\n+void chdir_notify_register(chdir_notify_callback cb, void *data);\n+void chdir_notify_reparent(char **path);\n+\n+/*\n+ *\n+ * Callers that want to chdir:\n+ *\n+ * chdir_notify(new_path);\n+ *\n+ * to switch to the new path and notify any callbacks.\n+ *\n+ * Note that you don't need to chdir_notify() if you're just temporarily moving\n+ * to a directory and back, as long as you don't call any subscribed code in\n+ * between (but it should be safe to do so if you're unsure).\n+ */\n+int chdir_notify(const char *new_cwd);\n+\n+/*\n+ * Reparent a relative path from old_root to new_root. For example:\n+ *\n+ * reparent_relative_path(\"/a\", \"/a/b\", \"b/rel\");\n+ *\n+ * would return the (newly allocated) string \"rel\". Note that we may return an\n+ * absolute path in some cases (e.g., if the resulting path is not inside\n+ * new_cwd).\n+ */\n+char *reparent_relative_path(const char *old_cwd,\n+\t\t\t const char *new_cwd,\n+\t\t\t const char *path);\n+\n+#endif /* CHDIR_NOTIFY_H */\n-- \n2.17.0.rc1.515.g3cc84c0ca4\n\n"},{"id":"343240","messageId":"20180328174215.GC16274@sigill.intra.peff.net","threadId":"48158","inReplyTo":"20180328173656.GA29094@sigill.intra.peff.net","subject":"[PATCH 3/4] set_work_tree: use chdir_notify","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-28T17:42:15Z","receivedAt":"2018-03-28T17:42:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we change to the top of the working tree, we manually\nre-adjust $GIT_DIR and call set_git_dir() again, in order to\nupdate any relative git-dir we'd compute earlier.\n\nInstead of the work-tree code having to know to call the\ngit-dir code, let's use the new chdir_notify interface.\nThere are two spots that need updating, with a few\nsubtleties in each:\n\n 1. the set_git_dir() code needs to chdir_notify_register()\n so it can be told when to update its path.\n\n Technically we could push this down into repo_set_gitdir(),\n so that even repository structs besides the_repository\n could benefit from this. But that opens up a lot of\n complications:\n\n - we'd still need to touch set_git_dir(), because it\n does some other setup (like setting $GIT_DIR in the\n environment)\n\n - submodules using other repository structs get\n cleaned up, which means we'd need to remove them\n from the chdir_notify list\n\n - it's unlikely to fix any bugs, since we shouldn't\n generally chdir() in the middle of working on a\n submodule\n\n 2. setup_work_tree now needs to call chdir_notify(), and\n can lose its manual set_git_dir() call.\n\n Note that at first glance it looks like this undoes the\n absolute-to-relative optimization added by 044bbbcb63\n (Make git_dir a path relative to work_tree in\n setup_work_tree(), 2008-06-19). But for the most part\n that optimization was just _undoing_ the\n relative-to-absolute conversion which the function was\n doing earlier (and which is now gone).\n\n It is true that if you already have an absolute git_dir\n that the setup_work_tree() function will no longer make\n it relative as a side effect. But:\n\n - we generally do have relative git-dir's due to the\n way the discovery code works\n\n - if we really care about making git-dir's relative\n when possible, then we should be relativizing them\n earlier (e.g., when we see an absolute $GIT_DIR we\n could turn it relative, whether we are going to\n chdir into a worktree or not). That would cover all\n cases, including ones that 044bbbcb63 did not.\n\nSigned-off-by: Jeff King \n---\n environment.c | 19 ++++++++++++++++++-\n setup.c | 9 +++------\n 2 files changed, 21 insertions(+), 7 deletions(-)\n\ndiff --git a/environment.c b/environment.c\nindex e01acf8b11..e1f535ae08 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -13,6 +13,7 @@\n #include \"refs.h\"\n #include \"fmt-merge-msg.h\"\n #include \"commit.h\"\n+#include \"chdir-notify.h\"\n \n int trust_executable_bit = 1;\n int trust_ctime = 1;\n@@ -296,7 +297,7 @@ char *get_graft_file(void)\n \treturn the_repository->graft_file;\n }\n \n-void set_git_dir(const char *path)\n+static void set_git_dir_1(const char *path)\n {\n \tif (setenv(GIT_DIR_ENVIRONMENT, path, 1))\n \t\tdie(\"could not set GIT_DIR to '%s'\", path);\n@@ -304,6 +305,22 @@ void set_git_dir(const char *path)\n \tsetup_git_env();\n }\n \n+static void update_relative_gitdir(const char *old_cwd,\n+\t\t\t\t const char *new_cwd,\n+\t\t\t\t void *data)\n+{\n+\tchar *path = reparent_relative_path(old_cwd, new_cwd, get_git_dir());\n+\tset_git_dir_1(path);\n+\tfree(path);\n+}\n+\n+void set_git_dir(const char *path)\n+{\n+\tset_git_dir_1(path);\n+\tif (!is_absolute_path(path))\n+\t\tchdir_notify_register(update_relative_gitdir, NULL);\n+}\n+\n const char *get_log_output_encoding(void)\n {\n \treturn git_log_output_encoding ? git_log_output_encoding\ndiff --git a/setup.c b/setup.c\nindex 7287779642..9eb2e808e1 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -3,6 +3,7 @@\n #include \"config.h\"\n #include \"dir.h\"\n #include \"string-list.h\"\n+#include \"chdir-notify.h\"\n \n static int inside_git_dir = -1;\n static int inside_work_tree = -1;\n@@ -378,7 +379,7 @@ int is_inside_work_tree(void)\n \n void setup_work_tree(void)\n {\n-\tconst char *work_tree, *git_dir;\n+\tconst char *work_tree;\n \tstatic int initialized = 0;\n \n \tif (initialized)\n@@ -388,10 +389,7 @@ void setup_work_tree(void)\n \t\tdie(_(\"unable to set up work tree using invalid config\"));\n \n \twork_tree = get_git_work_tree();\n-\tgit_dir = get_git_dir();\n-\tif (!is_absolute_path(git_dir))\n-\t\tgit_dir = real_path(get_git_dir());\n-\tif (!work_tree || chdir(work_tree))\n+\tif (!work_tree || chdir_notify(work_tree))\n \t\tdie(_(\"this operation must be run in a work tree\"));\n \n \t/*\n@@ -401,7 +399,6 @@ void setup_work_tree(void)\n \tif (getenv(GIT_WORK_TREE_ENVIRONMENT))\n \t\tsetenv(GIT_WORK_TREE_ENVIRONMENT, \".\", 1);\n \n-\tset_git_dir(remove_leading_path(git_dir, work_tree));\n \tinitialized = 1;\n }\n \n-- \n2.17.0.rc1.515.g3cc84c0ca4\n\n"},{"id":"343241","messageId":"20180328174347.GD16274@sigill.intra.peff.net","threadId":"48158","inReplyTo":"20180328173656.GA29094@sigill.intra.peff.net","subject":"[PATCH 4/4] refs: use chdir_notify to update cached relative paths","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-28T17:43:48Z","receivedAt":"2018-03-28T17:43:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit f57f37e2e1 (files-backend: remove the use of\ngit_path(), 2017-03-26) introduced a regression when a\nrelative $GIT_DIR is used in a working tree:\n\n - when we initialize the ref backend, we make a copy of\n get_git_dir(), which may be relative\n\n - later, we may call setup_work_tree() and chdir to the\n root of the working tree\n\n - further calls to the ref code will use the stored git\n directory, but relative paths will now point to the\n wrong place\n\nThe new test in t1501 demonstrates one such instance (the\nbug causes us to write the ref update to the nonsense\n\"relative/relative/.git\").\n\nSince setup_work_tree() now uses chdir_notify, we can just\nask it update our relative paths when necessary.\n\nReported-by: Rafael Ascensao \nHelped-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: Jeff King \n---\n refs/files-backend.c | 4 ++++\n refs/packed-backend.c | 3 +++\n t/t1501-work-tree.sh | 12 ++++++++++++\n 3 files changed, 19 insertions(+)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex bec8e30e9e..ab3e00ffa0 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -9,6 +9,7 @@\n #include \"../lockfile.h\"\n #include \"../object.h\"\n #include \"../dir.h\"\n+#include \"../chdir-notify.h\"\n \n /*\n * This backend uses the following flags in `ref_update::flags` for\n@@ -106,6 +107,9 @@ static struct ref_store *files_ref_store_create(const char *gitdir,\n \trefs->packed_ref_store = packed_ref_store_create(sb.buf, flags);\n \tstrbuf_release(&sb);\n \n+\tchdir_notify_reparent(&refs->gitdir);\n+\tchdir_notify_reparent(&refs->gitcommondir);\n+\n \treturn ref_store;\n }\n \ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 65288c6472..6725742f00 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -5,6 +5,7 @@\n #include \"packed-backend.h\"\n #include \"../iterator.h\"\n #include \"../lockfile.h\"\n+#include \"../chdir-notify.h\"\n \n enum mmap_strategy {\n \t/*\n@@ -202,6 +203,8 @@ struct ref_store *packed_ref_store_create(const char *path,\n \trefs->store_flags = store_flags;\n \n \trefs->path = xstrdup(path);\n+\tchdir_notify_reparent(&refs->path);\n+\n \treturn ref_store;\n }\n \ndiff --git a/t/t1501-work-tree.sh b/t/t1501-work-tree.sh\nindex b06210ec5e..a5db53337b 100755\n--- a/t/t1501-work-tree.sh\n+++ b/t/t1501-work-tree.sh\n@@ -431,4 +431,16 @@ test_expect_success 'error out gracefully on invalid $GIT_WORK_TREE' '\n \t)\n '\n \n+test_expect_success 'refs work with relative gitdir and work tree' '\n+\tgit init relative &&\n+\tgit -C relative commit --allow-empty -m one &&\n+\tgit -C relative commit --allow-empty -m two &&\n+\n+\tGIT_DIR=relative/.git GIT_WORK_TREE=relative git reset HEAD^ &&\n+\n+\tgit -C relative log -1 --format=%s >actual &&\n+\techo one >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.17.0.rc1.515.g3cc84c0ca4\n"},{"id":"343244","messageId":"20180328175537.17450-1-pclouds@gmail.com","threadId":"48158","inReplyTo":"20180328094733.GA1523@sigill.intra.peff.net","subject":"[PATCH 0/8] Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-03-28T17:55:29Z","receivedAt":"2018-03-28T17:56:29Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Mar 28, 2018 at 7:36 PM, Jeff King wrote:\n> On Wed, Mar 28, 2018 at 12:10:21PM +0200, Duy Nguyen wrote:\n>\n>> > I think it might be clearer if a single call is given both the old and\n>> > new paths. That requires the caller of chdir() storing getcwd() before\n>> > it moves, but I don't think that should be a big deal.\n>>\n>> The problem is switching relative paths relies on the old $CWD if I'm\n>> not mistaken and we need  getcwd() for this. I'd love to have one\n>> callback that says \"$CWD has been switched from this path to that\n>> path, do whatever you need to\" that can be called any time, before or\n>> after chdir(). I'll look more into it.\n>\n> I think it should be OK to save getcwd() and just construct the original\n> path after the fact. Here's some patches which do that in a nice way.\n\nHeh.. I should have checked mails more often while coding ;-)\n\nThis is what I got, which is slightly different from your series\nbecause I want to call set_git_dir() just one time (by\nsetup_git_directory) and never again. I think the API looks close\nenough.\n\nI will probably rework on top of your chdir-notify instead (and let\nyours to be merged earlier)\n\nNote, this one is built on a strange base, which is a merge of 'next'\nand 'sb/object-store' (I needed 'next' and Junio would have another\nevil merge if 'sb/object-store' was not in the base).\n\nNguyễn Thái Ngọc Duy (8):\n strbuf.c: add strbuf_ensure_trailing_dr_sep()\n strbuf.c: reintroduce get_pwd_cwd() (with strbuf_ prefix)\n trace.c: export trace_setup_key\n setup.c: introduce setup_adjust_path()\n setup.c: allow other code to be notified when $CWD moves\n environment.c: adjust env containing relpath when $CWD is moved\n repository: adjust repo paths when $CWD moves\n refs: adjust main repo paths when $CWD moves\n\n abspath.c | 4 +--\n builtin/difftool.c | 6 ++---\n cache.h | 8 ++++++\n dir-iterator.c | 3 +--\n environment.c | 46 +++++++++++++++++++++++++++++++++\n object-store.h | 3 +++\n object.c | 15 +++++++++++\n path.c | 9 +++----\n refs.c | 10 ++++++++\n refs/files-backend.c | 15 +++++++++++\n refs/packed-backend.c | 12 +++++++++\n refs/refs-internal.h | 4 +++\n repository.c | 28 ++++++++++++++++++++\n setup.c | 59 ++++++++++++++++++++++++++++++++++---------\n strbuf.c | 43 ++++++++++++++++++++-----------\n strbuf.h | 8 ++++++\n trace.c | 14 +++++-----\n trace.h | 1 +\n 18 files changed, 239 insertions(+), 49 deletions(-)\n\n-- \n2.17.0.rc1.439.gca064e2955\n\n"},{"id":"343245","messageId":"20180328175537.17450-2-pclouds@gmail.com","threadId":"48158","inReplyTo":"20180328175537.17450-1-pclouds@gmail.com","subject":"[PATCH 1/8] strbuf.c: add strbuf_ensure_trailing_dr_sep()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-03-28T17:55:30Z","receivedAt":"2018-03-28T17:56:32Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"From: Duy Nguyen \n\nThis is just good cleanup and the logic will also be needed in new\npatches.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n abspath.c | 4 +---\n builtin/difftool.c | 6 ++----\n dir-iterator.c | 3 +--\n path.c | 9 +++------\n strbuf.c | 6 ++++++\n strbuf.h | 2 ++\n 6 files changed, 15 insertions(+), 15 deletions(-)\n\ndiff --git a/abspath.c b/abspath.c\nindex 9857985329..994075b5c8 100644\n--- a/abspath.c\n+++ b/abspath.c\n@@ -122,9 +122,7 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,\n \t\t\tcontinue;\n \t\t}\n \n-\t\t/* append the next component and resolve resultant path */\n-\t\tif (!is_dir_sep(resolved->buf[resolved->len - 1]))\n-\t\t\tstrbuf_addch(resolved, '/');\n+\t\tstrbuf_ensure_trailing_dir_sep(resolved);\n \t\tstrbuf_addbuf(resolved, &next);\n \n \t\tif (lstat(resolved->buf, &st)) {\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex ee8dce019e..8d125c7968 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -88,8 +88,7 @@ static int parse_index_info(char *p, int *mode1, int *mode2,\n static void add_path(struct strbuf *buf, size_t base_len, const char *path)\n {\n \tstrbuf_setlen(buf, base_len);\n-\tif (buf->len && buf->buf[buf->len - 1] != '/')\n-\t\tstrbuf_addch(buf, '/');\n+\tstrbuf_ensure_trailing_dir_sep(buf);\n \tstrbuf_addstr(buf, path);\n }\n \n@@ -362,8 +361,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \tstrbuf_addf(&ldir, \"%s/left/\", tmpdir);\n \tstrbuf_addf(&rdir, \"%s/right/\", tmpdir);\n \tstrbuf_addstr(&wtdir, workdir);\n-\tif (!wtdir.len || !is_dir_sep(wtdir.buf[wtdir.len - 1]))\n-\t\tstrbuf_addch(&wtdir, '/');\n+\tstrbuf_ensure_trailing_dir_sep(&wtdir);\n \tmkdir(ldir.buf, 0700);\n \tmkdir(rdir.buf, 0700);\n \ndiff --git a/dir-iterator.c b/dir-iterator.c\nindex 34182a9a1c..249b5325cf 100644\n--- a/dir-iterator.c\n+++ b/dir-iterator.c\n@@ -65,8 +65,7 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n \t\t\t * Note: dir_iterator_begin() ensures that\n \t\t\t * path is not the empty string.\n \t\t\t */\n-\t\t\tif (!is_dir_sep(iter->base.path.buf[iter->base.path.len - 1]))\n-\t\t\t\tstrbuf_addch(&iter->base.path, '/');\n+\t\t\tstrbuf_ensure_trailing_dir_sep(&iter->base.path);\n \t\t\tlevel->prefix_len = iter->base.path.len;\n \n \t\t\tlevel->dir = opendir(iter->base.path.buf);\ndiff --git a/path.c b/path.c\nindex 3308b7b958..cd0ad89868 100644\n--- a/path.c\n+++ b/path.c\n@@ -408,8 +408,7 @@ static void do_git_path(const struct repository *repo,\n {\n \tint gitdir_len;\n \tstrbuf_worktree_gitdir(buf, repo, wt);\n-\tif (buf->len && !is_dir_sep(buf->buf[buf->len - 1]))\n-\t\tstrbuf_addch(buf, '/');\n+\tstrbuf_ensure_trailing_dir_sep(buf);\n \tgitdir_len = buf->len;\n \tstrbuf_vaddf(buf, fmt, args);\n \tif (!wt)\n@@ -512,8 +511,7 @@ static void do_worktree_path(const struct repository *repo,\n \t\t\t const char *fmt, va_list args)\n {\n \tstrbuf_addstr(buf, repo->worktree);\n-\tif(buf->len && !is_dir_sep(buf->buf[buf->len - 1]))\n-\t\tstrbuf_addch(buf, '/');\n+\tstrbuf_ensure_trailing_dir_sep(buf);\n \n \tstrbuf_vaddf(buf, fmt, args);\n \tstrbuf_cleanup_path(buf);\n@@ -608,8 +606,7 @@ static void do_git_common_path(const struct repository *repo,\n \t\t\t va_list args)\n {\n \tstrbuf_addstr(buf, repo->commondir);\n-\tif (buf->len && !is_dir_sep(buf->buf[buf->len - 1]))\n-\t\tstrbuf_addch(buf, '/');\n+\tstrbuf_ensure_trailing_dir_sep(buf);\n \tstrbuf_vaddf(buf, fmt, args);\n \tstrbuf_cleanup_path(buf);\n }\ndiff --git a/strbuf.c b/strbuf.c\nindex 83d05024e6..d5b7cda61e 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -122,6 +122,12 @@ void strbuf_ltrim(struct strbuf *sb)\n \tsb->buf[sb->len] = '\\0';\n }\n \n+void strbuf_ensure_trailing_dir_sep(struct strbuf *sb)\n+{\n+\tif (sb->len && !is_dir_sep(sb->buf[sb->len - 1]))\n+\t\tstrbuf_addch(sb, '/');\n+}\n+\n int strbuf_reencode(struct strbuf *sb, const char *from, const char *to)\n {\n \tchar *out;\ndiff --git a/strbuf.h b/strbuf.h\nindex c4de5e4588..62dc7f16fa 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -189,6 +189,8 @@ extern void strbuf_ltrim(struct strbuf *);\n \n /* Strip trailing directory separators */\n extern void strbuf_trim_trailing_dir_sep(struct strbuf *);\n+/* Append trailing directory separator if necessary */\n+extern void strbuf_ensure_trailing_dir_sep(struct strbuf *sb);\n \n /**\n * Replace the contents of the strbuf with a reencoded form. Returns -1\n-- \n2.17.0.rc1.439.gca064e2955\n\n"},{"id":"343246","messageId":"20180328175537.17450-3-pclouds@gmail.com","threadId":"48158","inReplyTo":"20180328175537.17450-1-pclouds@gmail.com","subject":"[PATCH 2/8] strbuf.c: reintroduce get_pwd_cwd() (with strbuf_ prefix)","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-03-28T17:55:31Z","receivedAt":"2018-03-28T17:56:34Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"From: Duy Nguyen \n\nThis function was added in 10c4c881c4 (Allow add_path() to add\nnon-existent directories to the path - 2008-07-21) because getcwd()\nmay fail on non-existing cwd and we want to construct non-existing\nabsolute paths sometimes.\n\nThe function was merged back in strbuf_add_absolute_path() some time\nlater. Move it out again because it will have another caller shortly.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n strbuf.c | 37 ++++++++++++++++++++++---------------\n strbuf.h | 6 ++++++\n 2 files changed, 28 insertions(+), 15 deletions(-)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex d5b7cda61e..aed4bec856 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -746,27 +746,34 @@ void strbuf_humanise_bytes(struct strbuf *buf, off_t bytes)\n \t}\n }\n \n+void strbuf_get_pwd_cwd(struct strbuf *sb)\n+{\n+\tstruct stat cwd_stat, pwd_stat;\n+\tchar *cwd = xgetcwd();\n+\tchar *pwd = getenv(\"PWD\");\n+\n+\tif (pwd && strcmp(pwd, cwd) &&\n+\t !stat(cwd, &cwd_stat) &&\n+\t (cwd_stat.st_dev || cwd_stat.st_ino) &&\n+\t !stat(pwd, &pwd_stat) &&\n+\t pwd_stat.st_dev == cwd_stat.st_dev &&\n+\t pwd_stat.st_ino == cwd_stat.st_ino)\n+\t\tstrbuf_addstr(sb, pwd);\n+\telse\n+\t\tstrbuf_addstr(sb, cwd);\n+\tfree(cwd);\n+}\n+\n void strbuf_add_absolute_path(struct strbuf *sb, const char *path)\n {\n \tif (!*path)\n \t\tdie(\"The empty string is not a valid path\");\n \tif (!is_absolute_path(path)) {\n-\t\tstruct stat cwd_stat, pwd_stat;\n \t\tsize_t orig_len = sb->len;\n-\t\tchar *cwd = xgetcwd();\n-\t\tchar *pwd = getenv(\"PWD\");\n-\t\tif (pwd && strcmp(pwd, cwd) &&\n-\t\t !stat(cwd, &cwd_stat) &&\n-\t\t (cwd_stat.st_dev || cwd_stat.st_ino) &&\n-\t\t !stat(pwd, &pwd_stat) &&\n-\t\t pwd_stat.st_dev == cwd_stat.st_dev &&\n-\t\t pwd_stat.st_ino == cwd_stat.st_ino)\n-\t\t\tstrbuf_addstr(sb, pwd);\n-\t\telse\n-\t\t\tstrbuf_addstr(sb, cwd);\n-\t\tif (sb->len > orig_len && !is_dir_sep(sb->buf[sb->len - 1]))\n-\t\t\tstrbuf_addch(sb, '/');\n-\t\tfree(cwd);\n+\n+\t\tstrbuf_get_pwd_cwd(sb);\n+\t\tif (sb->len > orig_len)\n+\t\t\tstrbuf_ensure_trailing_dir_sep(sb);\n \t}\n \tstrbuf_addstr(sb, path);\n }\ndiff --git a/strbuf.h b/strbuf.h\nindex 62dc7f16fa..f712c4ff92 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -458,6 +458,12 @@ extern int strbuf_getwholeline_fd(struct strbuf *, int, int);\n */\n extern int strbuf_getcwd(struct strbuf *sb);\n \n+/**\n+ * Return the current directory, fall back to $PWD if the\n+ * current directory does not exist.\n+ */\n+extern void strbuf_get_pwd_cwd(struct strbuf *sb);\n+\n /**\n * Add a path to a buffer, converting a relative path to an\n * absolute one in the process. Symbolic links are not\n-- \n2.17.0.rc1.439.gca064e2955\n\n"},{"id":"343247","messageId":"20180328175537.17450-4-pclouds@gmail.com","threadId":"48158","inReplyTo":"20180328175537.17450-1-pclouds@gmail.com","subject":"[PATCH 3/8] trace.c: export trace_setup_key","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-03-28T17:55:32Z","receivedAt":"2018-03-28T17:56:36Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This is so that we can print traces based on this key outside trace.c.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n trace.c | 14 +++++++-------\n trace.h | 1 +\n 2 files changed, 8 insertions(+), 7 deletions(-)\n\ndiff --git a/trace.c b/trace.c\nindex 7f3b08e148..fc623e91fd 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -26,6 +26,7 @@\n \n struct trace_key trace_default_key = { \"GIT_TRACE\", 0, 0, 0 };\n struct trace_key trace_perf_key = TRACE_KEY_INIT(PERFORMANCE);\n+struct trace_key trace_setup_key = TRACE_KEY_INIT(SETUP);\n \n /* Get a trace file descriptor from \"key\" env variable. */\n static int get_trace_fd(struct trace_key *key)\n@@ -300,11 +301,10 @@ static const char *quote_crnl(const char *path)\n /* FIXME: move prefix to startup_info struct and get rid of this arg */\n void trace_repo_setup(const char *prefix)\n {\n-\tstatic struct trace_key key = TRACE_KEY_INIT(SETUP);\n \tconst char *git_work_tree;\n \tchar *cwd;\n \n-\tif (!trace_want(&key))\n+\tif (!trace_want(&trace_setup_key))\n \t\treturn;\n \n \tcwd = xgetcwd();\n@@ -315,11 +315,11 @@ void trace_repo_setup(const char *prefix)\n \tif (!prefix)\n \t\tprefix = \"(null)\";\n \n-\ttrace_printf_key(&key, \"setup: git_dir: %s\\n\", quote_crnl(get_git_dir()));\n-\ttrace_printf_key(&key, \"setup: git_common_dir: %s\\n\", quote_crnl(get_git_common_dir()));\n-\ttrace_printf_key(&key, \"setup: worktree: %s\\n\", quote_crnl(git_work_tree));\n-\ttrace_printf_key(&key, \"setup: cwd: %s\\n\", quote_crnl(cwd));\n-\ttrace_printf_key(&key, \"setup: prefix: %s\\n\", quote_crnl(prefix));\n+\ttrace_printf_key(&trace_setup_key, \"setup: git_dir: %s\\n\", quote_crnl(get_git_dir()));\n+\ttrace_printf_key(&trace_setup_key, \"setup: git_common_dir: %s\\n\", quote_crnl(get_git_common_dir()));\n+\ttrace_printf_key(&trace_setup_key, \"setup: worktree: %s\\n\", quote_crnl(git_work_tree));\n+\ttrace_printf_key(&trace_setup_key, \"setup: cwd: %s\\n\", quote_crnl(cwd));\n+\ttrace_printf_key(&trace_setup_key, \"setup: prefix: %s\\n\", quote_crnl(prefix));\n \n \tfree(cwd);\n }\ndiff --git a/trace.h b/trace.h\nindex 88055abef7..2b6a1bc17c 100644\n--- a/trace.h\n+++ b/trace.h\n@@ -15,6 +15,7 @@ extern struct trace_key trace_default_key;\n \n #define TRACE_KEY_INIT(name) { \"GIT_TRACE_\" #name, 0, 0, 0 }\n extern struct trace_key trace_perf_key;\n+extern struct trace_key trace_setup_key;\n \n extern void trace_repo_setup(const char *prefix);\n extern int trace_want(struct trace_key *key);\n-- \n2.17.0.rc1.439.gca064e2955\n\n"},{"id":"343248","messageId":"20180328175537.17450-5-pclouds@gmail.com","threadId":"48158","inReplyTo":"20180328175537.17450-1-pclouds@gmail.com","subject":"[PATCH 4/8] setup.c: introduce setup_adjust_path()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-03-28T17:55:33Z","receivedAt":"2018-03-28T17:56:38Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"From: Duy Nguyen \n\nWhen $CWD is moved, relative path must be updated to be relative to\nthe new $CWD. This function helps do that. The _in_place version is\njust for convenient (and we will use it quite often in subsequent\npatches).\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n cache.h | 3 +++\n setup.c | 20 ++++++++++++++++++++\n 2 files changed, 23 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex bbaf5c349a..05f32c9659 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -522,6 +522,9 @@ extern void set_git_work_tree(const char *tree);\n \n #define ALTERNATE_DB_ENVIRONMENT \"GIT_ALTERNATE_OBJECT_DIRECTORIES\"\n \n+extern void setup_adjust_path(const char *name, char **path,\n+\t\t\t const char *old_cwd,\n+\t\t\t const char *new_cwd);\n extern void setup_work_tree(void);\n /*\n * Find the commondir and gitdir of the repository that contains the current\ndiff --git a/setup.c b/setup.c\nindex 664453fcef..e26f44185e 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -376,6 +376,26 @@ int is_inside_work_tree(void)\n \treturn inside_work_tree;\n }\n \n+void setup_adjust_path(const char *name, char **path,\n+\t\t const char *old_cwd,\n+\t\t const char *new_cwd)\n+{\n+\tchar *old_path = *path;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\n+\tif (!old_path || is_absolute_path(old_path))\n+\t\treturn;\n+\n+\tstrbuf_addstr(&sb, old_cwd);\n+\tstrbuf_ensure_trailing_dir_sep(&sb);\n+\tstrbuf_addstr(&sb, old_path);\n+\t*path = xstrdup(remove_leading_path(sb.buf, new_cwd));\n+\ttrace_printf_key(&trace_setup_key, \"setup: adjust '%s' to %s\",\n+\t\t\t name, *path);\n+\tstrbuf_release(&sb);\n+\tfree(old_path);\n+}\n+\n void setup_work_tree(void)\n {\n \tconst char *work_tree, *git_dir;\n-- \n2.17.0.rc1.439.gca064e2955\n\n"},{"id":"343249","messageId":"20180328175537.17450-6-pclouds@gmail.com","threadId":"48158","inReplyTo":"20180328175537.17450-1-pclouds@gmail.com","subject":"[PATCH 5/8] setup.c: allow other code to be notified when $CWD moves","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-03-28T17:55:34Z","receivedAt":"2018-03-28T17:56:40Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"From: Duy Nguyen \n\nWhen the current directory is moved, all relative paths may become\ninvalid because they are still relative to the old current\ndirectory. At this point in setup_work_tree() many objects have been\ninitialized and some keep relative paths in their data structure.\n\n$GIT_DIR and $GIT_WORK_TREE for example are the two examples which are\ndealt with explicitly in this code: $GIT_WORK_TREE is reset to \".\" and\nset_git_dir() is called the second time with a new relative path.\n\nIntroduce a more generic mechanism to let all code get notified and do\nthe path adjustment themselves instead of pulling all path adjustment\nlogic in here. The $GIT_DIR and $GIT_WORK_TREE adjustments will\nremoved from this code in later patches.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n cache.h | 5 +++++\n setup.c | 27 ++++++++++++++++++++++++++-\n 2 files changed, 31 insertions(+), 1 deletion(-)\n\ndiff --git a/cache.h b/cache.h\nindex 05f32c9659..895abe7e7e 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -522,6 +522,11 @@ extern void set_git_work_tree(const char *tree);\n \n #define ALTERNATE_DB_ENVIRONMENT \"GIT_ALTERNATE_OBJECT_DIRECTORIES\"\n \n+typedef void (*cwd_updated_fn)(const char *old_cwd,\n+\t\t\t const char *new_cwd,\n+\t\t\t void *cb_data);\n+void add_cwd_update_callback(cwd_updated_fn fn, void *cb_data);\n+void removet_cwd_update_callback(cwd_updated_fn fn, void *cb_data);\n extern void setup_adjust_path(const char *name, char **path,\n \t\t\t const char *old_cwd,\n \t\t\t const char *new_cwd);\ndiff --git a/setup.c b/setup.c\nindex e26f44185e..e340ee2130 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -376,6 +376,24 @@ int is_inside_work_tree(void)\n \treturn inside_work_tree;\n }\n \n+struct cwd_update_callback {\n+\tcwd_updated_fn fn;\n+\tvoid *cb_data;\n+};\n+\n+static struct cwd_update_callback *cwd_callbacks;\n+static int nr_cwd_callbacks;\n+\n+void add_cwd_update_callback(cwd_updated_fn fn, void *cb_data)\n+{\n+\tstruct cwd_update_callback *cb;\n+\n+\tREALLOC_ARRAY(cwd_callbacks, nr_cwd_callbacks + 1);\n+\tcb = cwd_callbacks + nr_cwd_callbacks++;\n+\tcb->fn = fn;\n+\tcb->cb_data = cb_data;\n+}\n+\n void setup_adjust_path(const char *name, char **path,\n \t\t const char *old_cwd,\n \t\t const char *new_cwd)\n@@ -398,8 +416,10 @@ void setup_adjust_path(const char *name, char **path,\n \n void setup_work_tree(void)\n {\n-\tconst char *work_tree, *git_dir;\n \tstatic int initialized = 0;\n+\tconst char *work_tree, *git_dir;\n+\tstruct strbuf old_cwd = STRBUF_INIT;\n+\tint i;\n \n \tif (initialized)\n \t\treturn;\n@@ -411,6 +431,7 @@ void setup_work_tree(void)\n \tgit_dir = get_git_dir();\n \tif (!is_absolute_path(git_dir))\n \t\tgit_dir = real_path(get_git_dir());\n+\tstrbuf_get_pwd_cwd(&old_cwd);\n \tif (!work_tree || chdir(work_tree))\n \t\tdie(_(\"this operation must be run in a work tree\"));\n \n@@ -421,6 +442,10 @@ void setup_work_tree(void)\n \tif (getenv(GIT_WORK_TREE_ENVIRONMENT))\n \t\tsetenv(GIT_WORK_TREE_ENVIRONMENT, \".\", 1);\n \n+\tfor (i = 0; i < nr_cwd_callbacks; i++)\n+\t\tcwd_callbacks[i].fn(old_cwd.buf, work_tree,\n+\t\t\t\t cwd_callbacks[i].cb_data);\n+\tstrbuf_release(&old_cwd);\n \tset_git_dir(remove_leading_path(git_dir, work_tree));\n \tinitialized = 1;\n }\n-- \n2.17.0.rc1.439.gca064e2955\n\n"},{"id":"343250","messageId":"20180328175537.17450-7-pclouds@gmail.com","threadId":"48158","inReplyTo":"20180328175537.17450-1-pclouds@gmail.com","subject":"[PATCH 6/8] environment.c: adjust env containing relpath when $CWD is moved","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-03-28T17:55:35Z","receivedAt":"2018-03-28T17:56:49Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"From: Duy Nguyen \n\nAs noted in the previous patch, when $CWD is moved, we recognize the\nproblem with relative paths and update $GIT_WORK_TREE and $GIT_DIR\nwith new ones.\n\nWe have plenty more environment variables that can contain paths\nthough. If they are read and cached before setup_work_tree() is\ncalled, nobody will update them and they become bad paths.\n\nHook into setup_work_tree() and update all those env variables. The\ncode to update $GIT_WORK_TREE is no longer needed and removed.\n\nTechnically we should remove the setenv() in set_git_dir() as well,\nbut that is also called _not_ by setup_work_tree() and we should keep\nthe behavior the same in that case for safety. set_git_dir() will be\nremoved from setup_work_tree() soon, which achieves the same goal.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n environment.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++\n setup.c | 7 -------\n 2 files changed, 46 insertions(+), 7 deletions(-)\n\ndiff --git a/environment.c b/environment.c\nindex 39b3d906c8..f9dcc1b99e 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -128,6 +128,20 @@ const char * const local_repo_env[] = {\n \tNULL\n };\n \n+/* A subset of local_repo_env[] that contains path */\n+const char * const local_repo_path_env[] = {\n+\tALTERNATE_DB_ENVIRONMENT,\n+\tCONFIG_ENVIRONMENT,\n+\tDB_ENVIRONMENT,\n+\tGIT_COMMON_DIR_ENVIRONMENT,\n+\tGIT_DIR_ENVIRONMENT,\n+\tGIT_SHALLOW_FILE_ENVIRONMENT,\n+\tGIT_WORK_TREE_ENVIRONMENT,\n+\tGRAFT_ENVIRONMENT,\n+\tINDEX_ENVIRONMENT,\n+\tNULL\n+};\n+\n static char *expand_namespace(const char *raw_namespace)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -149,6 +163,32 @@ static char *expand_namespace(const char *raw_namespace)\n \treturn strbuf_detach(&buf, NULL);\n }\n \n+static void update_path_envs(const char *old_cwd, const char *new_cwd,\n+\t\t\t void *cb)\n+{\n+\tint i;\n+\n+\t/*\n+\t * FIXME: special treatment needed for\n+\t * GIT_ALTERNATE_OBJECT_DIRECTORIES because it can contain\n+\t * multiple paths.\n+\t */\n+\tfor (i = 0; local_repo_path_env[i]; i++) {\n+\t\tconst char *name = local_repo_path_env[i];\n+\t\tconst char *value = getenv(name);\n+\t\tchar *new_value;\n+\n+\t\tif (!value)\n+\t\t\tcontinue;\n+\t\tif (is_absolute_path(value))\n+\t\t\tcontinue;\n+\t\tnew_value = xstrdup(value);\n+\t\tsetup_adjust_path(name, &new_value, old_cwd, new_cwd);\n+\t\tif (setenv(name, new_value, 10))\n+\t\t\terror(_(\"could not set %s to '%s'\"), name, value);\n+\t\tfree(new_value);\n+\t}\n+}\n /*\n * Wrapper of getenv() that returns a strdup value. This value is kept\n * in argv to be freed later.\n@@ -170,6 +210,12 @@ void setup_git_env(const char *git_dir)\n \tconst char *replace_ref_base;\n \tstruct set_gitdir_args args = { NULL };\n \tstruct argv_array to_free = ARGV_ARRAY_INIT;\n+\tstatic int added_cwd_callback;\n+\n+\tif (!added_cwd_callback) {\n+\t\tadd_cwd_update_callback(update_path_envs, NULL);\n+\t\tadded_cwd_callback = 1;\n+\t}\n \n \targs.commondir = getenv_safe(&to_free, GIT_COMMON_DIR_ENVIRONMENT);\n \targs.object_dir = getenv_safe(&to_free, DB_ENVIRONMENT);\ndiff --git a/setup.c b/setup.c\nindex e340ee2130..23b8f89ce2 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -435,13 +435,6 @@ void setup_work_tree(void)\n \tif (!work_tree || chdir(work_tree))\n \t\tdie(_(\"this operation must be run in a work tree\"));\n \n-\t/*\n-\t * Make sure subsequent git processes find correct worktree\n-\t * if $GIT_WORK_TREE is set relative\n-\t */\n-\tif (getenv(GIT_WORK_TREE_ENVIRONMENT))\n-\t\tsetenv(GIT_WORK_TREE_ENVIRONMENT, \".\", 1);\n-\n \tfor (i = 0; i < nr_cwd_callbacks; i++)\n \t\tcwd_callbacks[i].fn(old_cwd.buf, work_tree,\n \t\t\t\t cwd_callbacks[i].cb_data);\n-- \n2.17.0.rc1.439.gca064e2955\n\n"},{"id":"343251","messageId":"20180328175537.17450-9-pclouds@gmail.com","threadId":"48158","inReplyTo":"20180328175537.17450-1-pclouds@gmail.com","subject":"[PATCH 8/8] refs: adjust main repo paths when $CWD moves","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-03-28T17:55:37Z","receivedAt":"2018-03-28T17:56:53Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"From: Duy Nguyen \n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n refs.c | 10 ++++++++++\n refs/files-backend.c | 15 +++++++++++++++\n refs/packed-backend.c | 12 ++++++++++++\n refs/refs-internal.h | 4 ++++\n setup.c | 1 +\n 5 files changed, 42 insertions(+)\n\ndiff --git a/refs.c b/refs.c\nindex 8b7a77fe5e..2457a31d4d 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1651,12 +1651,22 @@ static struct ref_store *ref_store_init(const char *gitdir,\n \treturn refs;\n }\n \n+static void ref_store_adjust_paths(const char *old_cwd,\n+\t\t\t\t const char *new_cwd,\n+\t\t\t\t void *cb)\n+{\n+\tstruct ref_store *refs = cb;\n+\n+\trefs->be->adjust_paths(refs, old_cwd, new_cwd);\n+}\n+\n struct ref_store *get_main_ref_store(void)\n {\n \tif (main_ref_store)\n \t\treturn main_ref_store;\n \n \tmain_ref_store = ref_store_init(get_git_dir(), REF_STORE_ALL_CAPS);\n+\tadd_cwd_update_callback(ref_store_adjust_paths, main_ref_store);\n \treturn main_ref_store;\n }\n \ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex bec8e30e9e..d35a8db844 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -3092,6 +3092,20 @@ static int files_reflog_expire(struct ref_store *ref_store,\n \treturn -1;\n }\n \n+static void files_adjust_paths(struct ref_store *ref_store,\n+\t\t\t const char *old_cwd,\n+\t\t\t const char *new_cwd)\n+{\n+\tstruct files_ref_store *refs =\n+\t\tfiles_downcast(ref_store, REF_STORE_WRITE,\n+\t\t\t \"files_adjust_paths\");\n+\n+\tsetup_adjust_path(\"ref store's gitdir\",\n+\t\t\t &refs->gitdir, old_cwd, new_cwd);\n+\tsetup_adjust_path(\"ref store's commondir\",\n+\t\t\t &refs->gitcommondir, old_cwd, new_cwd);\n+}\n+\n static int files_init_db(struct ref_store *ref_store, struct strbuf *err)\n {\n \tstruct files_ref_store *refs =\n@@ -3117,6 +3131,7 @@ struct ref_storage_be refs_be_files = {\n \t\"files\",\n \tfiles_ref_store_create,\n \tfiles_init_db,\n+\tfiles_adjust_paths,\n \tfiles_transaction_prepare,\n \tfiles_transaction_finish,\n \tfiles_transaction_abort,\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 65288c6472..764d1250a5 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1047,6 +1047,17 @@ int packed_refs_is_locked(struct ref_store *ref_store)\n \treturn is_lock_file_locked(&refs->lock);\n }\n \n+static void packed_adjust_paths(struct ref_store *ref_store,\n+\t\t\t\tconst char *old_cwd,\n+\t\t\t\tconst char *new_cwd)\n+{\n+\tstruct packed_ref_store *refs =\n+\t\tpacked_downcast(ref_store, REF_STORE_WRITE,\n+\t\t\t\t\"packed_adjust_paths\");\n+\n+\tsetup_adjust_path(\"packed-refs\", &refs->path, old_cwd, new_cwd);\n+}\n+\n /*\n * The packed-refs header line that we write out. Perhaps other traits\n * will be added later.\n@@ -1632,6 +1643,7 @@ struct ref_storage_be refs_be_packed = {\n \t\"packed\",\n \tpacked_ref_store_create,\n \tpacked_init_db,\n+\tpacked_adjust_paths,\n \tpacked_transaction_prepare,\n \tpacked_transaction_finish,\n \tpacked_transaction_abort,\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex dd834314bd..73480543c0 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -505,6 +505,9 @@ struct ref_store;\n */\n typedef struct ref_store *ref_store_init_fn(const char *gitdir,\n \t\t\t\t\t unsigned int flags);\n+typedef void ref_store_adjust_paths_fn(struct ref_store *refs,\n+\t\t\t\t const char *old_cwd,\n+\t\t\t\t const char *new_cwd);\n \n typedef int ref_init_db_fn(struct ref_store *refs, struct strbuf *err);\n \n@@ -625,6 +628,7 @@ struct ref_storage_be {\n \tconst char *name;\n \tref_store_init_fn *init;\n \tref_init_db_fn *init_db;\n+\tref_store_adjust_paths_fn *adjust_paths;\n \n \tref_transaction_prepare_fn *transaction_prepare;\n \tref_transaction_finish_fn *transaction_finish;\ndiff --git a/setup.c b/setup.c\nindex e364aea7e5..35e89a03e5 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -3,6 +3,7 @@\n #include \"config.h\"\n #include \"dir.h\"\n #include \"string-list.h\"\n+#include \"refs.h\"\n \n static int inside_git_dir = -1;\n static int inside_work_tree = -1;\n-- \n2.17.0.rc1.439.gca064e2955\n\n"},{"id":"343252","messageId":"20180328175537.17450-8-pclouds@gmail.com","threadId":"48158","inReplyTo":"20180328175537.17450-1-pclouds@gmail.com","subject":"[PATCH 7/8] repository: adjust repo paths when $CWD moves","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-03-28T17:55:36Z","receivedAt":"2018-03-28T17:56:55Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"From: Duy Nguyen \n\nAs noted in previous and previous patches, when setup_work_tree()\nmoves $CWD, it calls set_git_dir() with a new path to make sure\nrelative $GIT_DIR is still correct.\n\nThe same problem from the previous patch applies here: if\n$GIT_ALTERNATE_OBJECT_DIRECTORIES or $GIT_OBJECT_DIRECTORY are also\nrelative, the user is screwed.\n\nFortuntely the previous patch indirectly fixes this as well:\nupdate_path_envs() in environment.c is actually called before\nsetup_git_env() is indirectly called by setup_work_tree() via\nset_git_dir(). At that point, it correctly gets the updated paths from\nenv and pass them down. This patch is here for another reason.\n\nFrom the design perspective, calling set_git_dir() the second time [1]\nis just bad because it could potentially be used to point repo code to\n_another_ repository. The promise that \"the new path actually points\nto the same place as the old one\" is not guaranteed and also hard to\nsee unless you know setup code well.\n\nThis patch avoids this. The second set_git_dir() call is just a\nworkaround to adjust paths. We can now do it directly from repo code\nby subscribing to the \"$CWD updated\" event and do everything needed by\nthe repo. After this we could die() on subsequent repo_set_gitdir()\ncalls because we do not support it (we never did).\n\n[1] The first time is done by setup_git_directory() code, which is\ncorrect: we have just found a repo and we want to initialize our code\nto read from that repo.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n object-store.h | 3 +++\n object.c | 15 +++++++++++++++\n repository.c | 28 ++++++++++++++++++++++++++++\n setup.c | 6 +-----\n 4 files changed, 47 insertions(+), 5 deletions(-)\n\ndiff --git a/object-store.h b/object-store.h\nindex fef33f345f..4af2c53bca 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -119,6 +119,9 @@ struct raw_object_store {\n };\n \n struct raw_object_store *raw_object_store_new(void);\n+void raw_object_store_adjust_paths(struct raw_object_store *o,\n+\t\t\t\t const char *old_cwd,\n+\t\t\t\t const char *new_cwd);\n void raw_object_store_clear(struct raw_object_store *o);\n \n /*\ndiff --git a/object.c b/object.c\nindex a0a756f24f..5318922efd 100644\n--- a/object.c\n+++ b/object.c\n@@ -457,6 +457,21 @@ struct raw_object_store *raw_object_store_new(void)\n \treturn o;\n }\n \n+void raw_object_store_adjust_paths(struct raw_object_store *o,\n+\t\t\t\t const char *old_cwd,\n+\t\t\t\t const char *new_cwd)\n+{\n+\t/*\n+\t * \"main repo\" is technically wrong because we don't know that\n+\t * from here. But it's the only repo that will execute this\n+\t * function for now.\n+\t */\n+\tsetup_adjust_path(\"main repo's object dir\",\n+\t\t\t &o->objectdir, old_cwd, new_cwd);\n+\tsetup_adjust_path(\"main repo's alt dir\",\n+\t\t\t &o->alternate_db, old_cwd, new_cwd);\n+}\n+\n static void free_alt_odb(struct alternate_object_database *alt)\n {\n \tstrbuf_release(&alt->scratch);\ndiff --git a/repository.c b/repository.c\nindex a4848c1bd0..c2edf227fe 100644\n--- a/repository.c\n+++ b/repository.c\n@@ -8,6 +8,10 @@\n static struct repository the_repo;\n struct repository *the_repository;\n \n+static void repo_adjust_paths(const char *old_cwd,\n+\t\t\t const char *new_cwd,\n+\t\t\t void *cb_data);\n+\n void initialize_the_repository(void)\n {\n \tthe_repository = &the_repo;\n@@ -15,6 +19,11 @@ void initialize_the_repository(void)\n \tthe_repo.index = &the_index;\n \tthe_repo.objects = raw_object_store_new();\n \trepo_set_hash_algo(&the_repo, GIT_HASH_SHA1);\n+\t/*\n+\t * non-main repos probably does not need this since $CWD should\n+\t * never change again by the time they are created.\n+\t */\n+\tadd_cwd_update_callback(repo_adjust_paths, the_repository);\n }\n \n static void expand_base_dir(char **out, const char *in,\n@@ -70,6 +79,25 @@ void repo_set_gitdir(struct repository *repo,\n \t\t\trepo->gitdir, \"index\");\n }\n \n+static void repo_adjust_paths(const char *old_cwd, const char *new_cwd,\n+\t\t\t void *cb_data)\n+{\n+\tstruct repository *repo = cb_data;\n+\n+\tif (repo != the_repository)\n+\t\tdie(\"BUG: this is supposed to happen to main repo only\");\n+\n+\tsetup_adjust_path(\"main repo's gitdir\",\n+\t\t\t &repo->gitdir, old_cwd, new_cwd);\n+\tsetup_adjust_path(\"main repo's commondir\",\n+\t\t\t &repo->commondir, old_cwd, new_cwd);\n+\tsetup_adjust_path(\"main repo's graft file\",\n+\t\t\t &repo->graft_file, old_cwd, new_cwd);\n+\tsetup_adjust_path(\"main repo's index file\",\n+\t\t\t &repo->index_file, old_cwd, new_cwd);\n+\traw_object_store_adjust_paths(repo->objects, old_cwd, new_cwd);\n+}\n+\n void repo_set_hash_algo(struct repository *repo, int hash_algo)\n {\n \trepo->hash_algo = &hash_algos[hash_algo];\ndiff --git a/setup.c b/setup.c\nindex 23b8f89ce2..e364aea7e5 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -417,7 +417,7 @@ void setup_adjust_path(const char *name, char **path,\n void setup_work_tree(void)\n {\n \tstatic int initialized = 0;\n-\tconst char *work_tree, *git_dir;\n+\tconst char *work_tree;\n \tstruct strbuf old_cwd = STRBUF_INIT;\n \tint i;\n \n@@ -428,9 +428,6 @@ void setup_work_tree(void)\n \t\tdie(_(\"unable to set up work tree using invalid config\"));\n \n \twork_tree = get_git_work_tree();\n-\tgit_dir = get_git_dir();\n-\tif (!is_absolute_path(git_dir))\n-\t\tgit_dir = real_path(get_git_dir());\n \tstrbuf_get_pwd_cwd(&old_cwd);\n \tif (!work_tree || chdir(work_tree))\n \t\tdie(_(\"this operation must be run in a work tree\"));\n@@ -439,7 +436,6 @@ void setup_work_tree(void)\n \t\tcwd_callbacks[i].fn(old_cwd.buf, work_tree,\n \t\t\t\t cwd_callbacks[i].cb_data);\n \tstrbuf_release(&old_cwd);\n-\tset_git_dir(remove_leading_path(git_dir, work_tree));\n \tinitialized = 1;\n }\n \n-- \n2.17.0.rc1.439.gca064e2955\n\n"},{"id":"343253","messageId":"CAPig+cQ0D+k+WmDWcaFHQj7DBb1RPfaUYwOvLosWXdHOzsoLOw@mail.gmail.com","threadId":"48158","inReplyTo":"20180328174041.GB16274@sigill.intra.peff.net","subject":"Re: [PATCH 2/4] add chdir-notify API","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-03-28T17:58:18Z","receivedAt":"2018-03-28T17:58:24Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Mar 28, 2018 at 1:40 PM, Jeff King wrote:\n> [...]\n> Let's provide an API to let code that stores relative paths\n> \"subscribe\" to updates to the current working directory.\n> This means that callers of chdir() don't need to know about\n> all subscribers ahead of time; they can simply consult a\n> dynamically built list.\n> [...]\n> Signed-off-by: Jeff King \n> ---\n> diff --git a/chdir-notify.c b/chdir-notify.c\n> @@ -0,0 +1,71 @@\n> +int chdir_notify(const char *new_cwd)\n> +{\n> + struct strbuf old_cwd = STRBUF_INIT;\n> + struct list_head *pos;\n> +\n> + if (strbuf_getcwd(&old_cwd) < 0)\n> + return -1;\n> + if (chdir(new_cwd) < 0)\n> + return -1;\n\nThis 'return' is leaking 'old_cwd', isn't it?\n\n> + list_for_each(pos, &chdir_notify_entries) {\n> + struct chdir_notify_entry *e =\n> + list_entry(pos, struct chdir_notify_entry, list);\n> + e->cb(old_cwd.buf, new_cwd, e->data);\n> + }\n> +\n> + strbuf_release(&old_cwd);\n> + return 0;\n> +}\n> diff --git a/chdir-notify.h b/chdir-notify.h\n> @@ -0,0 +1,64 @@\n> + * In practice most callers will want to move a relative path to the new root;\n> + * they can use the reparent_relative_path() helper for that. If that's all\n> + * you're doing, you can also use the convenience function:\n> + *\n> + * chdir_notify_reparent(&my_path);\n> + */\n> +typedef void (*chdir_notify_callback)(const char *old_cwd,\n> + const char *new_cwd,\n> + void *data);\n> +void chdir_notify_register(chdir_notify_callback cb, void *data);\n> +void chdir_notify_reparent(char **path);\n\nCan we have some documentation here (or in the chdir_notify_reparent()\nexample above) explaining that *path is a heap-allocated value? I had\nto consult the implementation to understand ownership.\n"},{"id":"343255","messageId":"20180328180210.GA16565@sigill.intra.peff.net","threadId":"48158","inReplyTo":"CAPig+cQ0D+k+WmDWcaFHQj7DBb1RPfaUYwOvLosWXdHOzsoLOw@mail.gmail.com","subject":"Re: [PATCH 2/4] add chdir-notify API","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-28T18:02:10Z","receivedAt":"2018-03-28T18:02:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 28, 2018 at 01:58:18PM -0400, Eric Sunshine wrote:\n\n> On Wed, Mar 28, 2018 at 1:40 PM, Jeff King wrote:\n> > [...]\n> > Let's provide an API to let code that stores relative paths\n> > \"subscribe\" to updates to the current working directory.\n> > This means that callers of chdir() don't need to know about\n> > all subscribers ahead of time; they can simply consult a\n> > dynamically built list.\n> > [...]\n> > Signed-off-by: Jeff King \n> > ---\n> > diff --git a/chdir-notify.c b/chdir-notify.c\n> > @@ -0,0 +1,71 @@\n> > +int chdir_notify(const char *new_cwd)\n> > +{\n> > + struct strbuf old_cwd = STRBUF_INIT;\n> > + struct list_head *pos;\n> > +\n> > + if (strbuf_getcwd(&old_cwd) < 0)\n> > + return -1;\n> > + if (chdir(new_cwd) < 0)\n> > + return -1;\n> \n> This 'return' is leaking 'old_cwd', isn't it?\n\nWhoops, yes. I wrote it the other way around first to avoid the leak,\nbut of course that has other problems. ;)\n\n> > diff --git a/chdir-notify.h b/chdir-notify.h\n> > @@ -0,0 +1,64 @@\n> > + * In practice most callers will want to move a relative path to the new root;\n> > + * they can use the reparent_relative_path() helper for that. If that's all\n> > + * you're doing, you can also use the convenience function:\n> > + *\n> > + * chdir_notify_reparent(&my_path);\n> > + */\n> > +typedef void (*chdir_notify_callback)(const char *old_cwd,\n> > + const char *new_cwd,\n> > + void *data);\n> > +void chdir_notify_register(chdir_notify_callback cb, void *data);\n> > +void chdir_notify_reparent(char **path);\n> \n> Can we have some documentation here (or in the chdir_notify_reparent()\n> example above) explaining that *path is a heap-allocated value? I had\n> to consult the implementation to understand ownership.\n\nSure. I originally had reparent_relative_path() handle the freeing and\nallocation, and it explained the ownership in its docstring. But as I\nrevised that to simply return its value, that got lost. I'll add it back\nin here.\n\n-Peff\n"},{"id":"343256","messageId":"CAGZ79kbofCd+8iscOFUNHTfQ2Y1cUDUjp5kOQQr_b1uPgPC2Eg@mail.gmail.com","threadId":"48158","inReplyTo":"20180328175537.17450-3-pclouds@gmail.com","subject":"Re: [PATCH 2/8] strbuf.c: reintroduce get_pwd_cwd() (with strbuf_ prefix)","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-03-28T18:02:34Z","receivedAt":"2018-03-28T18:02:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Mar 28, 2018 at 10:55 AM, Nguyễn Thái Ngọc Duy\n wrote:\n> +/**\n> + * Return the current directory, fall back to $PWD if the\n> + * current directory does not exist.\n> + */\n> +extern void strbuf_get_pwd_cwd(struct strbuf *sb);\n\nPlease see 89a9f2c862 (CodingGuidelines: mention \"static\" and \"extern\",\n2018-02-08) and drop the extern if you need to reroll.\n\nThanks,\nStefan\n"},{"id":"343257","messageId":"CACsJy8D6h_t0LTD6ML8o4imEaU7vqxZJyzaBU6McCfVqWOQkWA@mail.gmail.com","threadId":"48158","inReplyTo":"CAGZ79kbofCd+8iscOFUNHTfQ2Y1cUDUjp5kOQQr_b1uPgPC2Eg@mail.gmail.com","subject":"Re: [PATCH 2/8] strbuf.c: reintroduce get_pwd_cwd() (with strbuf_ prefix)","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-03-28T18:05:25Z","receivedAt":"2018-03-28T18:06:00Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Mar 28, 2018 at 8:02 PM, Stefan Beller wrote:\n> On Wed, Mar 28, 2018 at 10:55 AM, Nguyễn Thái Ngọc Duy\n> wrote:\n>> +/**\n>> + * Return the current directory, fall back to $PWD if the\n>> + * current directory does not exist.\n>> + */\n>> +extern void strbuf_get_pwd_cwd(struct strbuf *sb);\n>\n> Please see 89a9f2c862 (CodingGuidelines: mention \"static\" and \"extern\",\n> 2018-02-08) and drop the extern if you need to reroll.\n\nI'm aware (and in favor) of that actually. But this whole strbuf.h\nuses extern. Either I had to delete all extern first, or go with old\nstyle and maybe do the whole deletion later. I chose the former. Maybe\nbe I'll delete all \"extern\" as a prep patch.\n-- \nDuy\n"},{"id":"343258","messageId":"20180328181932.GB16565@sigill.intra.peff.net","threadId":"48158","inReplyTo":"20180328175537.17450-1-pclouds@gmail.com","subject":"Re: [PATCH 0/8] Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-28T18:19:32Z","receivedAt":"2018-03-28T18:19:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 28, 2018 at 07:55:29PM +0200, Nguyễn Thái Ngọc Duy wrote:\n\n> >> The problem is switching relative paths relies on the old $CWD if I'm\n> >> not mistaken and we need  getcwd() for this. I'd love to have one\n> >> callback that says \"$CWD has been switched from this path to that\n> >> path, do whatever you need to\" that can be called any time, before or\n> >> after chdir(). I'll look more into it.\n> >\n> > I think it should be OK to save getcwd() and just construct the original\n> > path after the fact. Here's some patches which do that in a nice way.\n> \n> Heh.. I should have checked mails more often while coding ;-)\n\nI was worried about that. I started this earlier as a \"well, I could\nprobably just knock this out quickly\" task. Many hours later, I found\nthere were quite a few subtleties. :)\n\n> This is what I got, which is slightly different from your series\n> because I want to call set_git_dir() just one time (by\n> setup_git_directory) and never again. I think the API looks close\n> enough.\n\nYeah, I actually think after this series we could enforce that\nset_git_dir() is never called twice.\n\nI think we could even make sure that repo_set_gitdir() is never called\ntwice, too, but it would involve a little more surgery (we'd have to\nteach it to set up a child_notify handler, and the way it interacts with\nthe set_git_dir() one is subtle. I did try it and it worked, but I went\nwith what you saw in patch 3 because it was simpler).\n\n> I will probably rework on top of your chdir-notify instead (and let\n> yours to be merged earlier)\n\nThanks. I like some of the related changes you made, like including this\nin the tracing output. That should be easy to do on top of mine, I\nthink.\n\nI didn't look for other spots that might need similar treatment (like\nthe object-store bits).\n\n-Peff\n"},{"id":"343259","messageId":"20180328183011.GA16931@sigill.intra.peff.net","threadId":"48158","inReplyTo":"20180328175537.17450-7-pclouds@gmail.com","subject":"Re: [PATCH 6/8] environment.c: adjust env containing relpath when $CWD is moved","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-28T18:30:11Z","receivedAt":"2018-03-28T18:30:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 28, 2018 at 07:55:35PM +0200, Nguyễn Thái Ngọc Duy wrote:\n\n> From: Duy Nguyen \n> \n> As noted in the previous patch, when $CWD is moved, we recognize the\n> problem with relative paths and update $GIT_WORK_TREE and $GIT_DIR\n> with new ones.\n> \n> We have plenty more environment variables that can contain paths\n> though. If they are read and cached before setup_work_tree() is\n> called, nobody will update them and they become bad paths.\n\nHmm, yeah, I missed these. It would be interesting to know if there are\neasy-to-run test cases that show off these bugs, or if they're\nhypothetical. (Even if they are hypothetical, I'm not opposed to fixing\nthem in the name of maintainability).\n\n> Hook into setup_work_tree() and update all those env variables. The\n> code to update $GIT_WORK_TREE is no longer needed and removed.\n\nThat's a nice cleanup.\n\n> Technically we should remove the setenv() in set_git_dir() as well,\n> but that is also called _not_ by setup_work_tree() and we should keep\n> the behavior the same in that case for safety. set_git_dir() will be\n> removed from setup_work_tree() soon, which achieves the same goal.\n\nMakes sense.\n\n> diff --git a/environment.c b/environment.c\n> index 39b3d906c8..f9dcc1b99e 100644\n> --- a/environment.c\n> +++ b/environment.c\n> @@ -128,6 +128,20 @@ const char * const local_repo_env[] = {\n> \tNULL\n> };\n> \n> +/* A subset of local_repo_env[] that contains path */\n> +const char * const local_repo_path_env[] = {\n> +\tALTERNATE_DB_ENVIRONMENT,\n> +\tCONFIG_ENVIRONMENT,\n> +\tDB_ENVIRONMENT,\n> +\tGIT_COMMON_DIR_ENVIRONMENT,\n> +\tGIT_DIR_ENVIRONMENT,\n> +\tGIT_SHALLOW_FILE_ENVIRONMENT,\n> +\tGIT_WORK_TREE_ENVIRONMENT,\n> +\tGRAFT_ENVIRONMENT,\n> +\tINDEX_ENVIRONMENT,\n> +\tNULL\n> +};\n\nIt might be nice to fold this list into local_repo_env automatically. I\nthink you'd have to do it with a macro.\n\nOTOH, it's possible that there could be a path-related environment\nvariable that _isn't_ actually part of local_repo_env. E.g., I think\nGIT_CONFIG might classify there (though I don't know if it's worth\nworrying about).\n\n> +static void update_path_envs(const char *old_cwd, const char *new_cwd,\n> +\t\t\t void *cb)\n> +{\n> +\tint i;\n> +\n> +\t/*\n> +\t * FIXME: special treatment needed for\n> +\t * GIT_ALTERNATE_OBJECT_DIRECTORIES because it can contain\n> +\t * multiple paths.\n> +\t */\n\nYuck. It just keeps getting more complicated. :(\n\nI do wonder if relative paths in variables like this are worth worrying\nabout. AFAIK, it's always been a \"well, it kind of works\" situation, but\nnot something we've tried to actively support. I think with the current\ncode you'd potentially get inconsistent results between a command which\nsets up the work tree and one which doesn't. So this would be fixing\nthat, but at the same time, I'm not sure how much we want to promise\nhere.\n\n-Peff\n"},{"id":"343262","messageId":"CACsJy8COj-usWbJzb_CRF95an94QhpTcc+uv-VDxmPbDfH+o6g@mail.gmail.com","threadId":"48158","inReplyTo":"20180328183011.GA16931@sigill.intra.peff.net","subject":"Re: [PATCH 6/8] environment.c: adjust env containing relpath when $CWD is moved","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-03-28T18:45:23Z","receivedAt":"2018-03-28T18:45:59Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Mar 28, 2018 at 8:30 PM, Jeff King wrote:\n> On Wed, Mar 28, 2018 at 07:55:35PM +0200, Nguyễn Thái Ngọc Duy wrote:\n>\n>> From: Duy Nguyen \n>>\n>> As noted in the previous patch, when $CWD is moved, we recognize the\n>> problem with relative paths and update $GIT_WORK_TREE and $GIT_DIR\n>> with new ones.\n>>\n>> We have plenty more environment variables that can contain paths\n>> though. If they are read and cached before setup_work_tree() is\n>> called, nobody will update them and they become bad paths.\n>\n> Hmm, yeah, I missed these. It would be interesting to know if there are\n> easy-to-run test cases that show off these bugs, or if they're\n> hypothetical. (Even if they are hypothetical, I'm not opposed to fixing\n> them in the name of maintainability).\n\nIt's kinda hard to show off these. But the GIT_ALTERNATE_OBJ..\nvariable could be one example in favor of this fix. Before, we lazily\nread the env var which is most likely after setup_work_tree() has run.\nAfter a bunch of code reorganization and stuff, GIT_ALTERNATE_OBJ is\nnow read very early, which should be safe (why not?) but it actually\nbreaks. Alternate db is only queried as the last resort if I'm not\nmistaken, so 90% of time you just hit an object in odb and never find\nout that your GIT_ALTERNATE_OBJ... points to nowhere.\n\n>> diff --git a/environment.c b/environment.c\n>> index 39b3d906c8..f9dcc1b99e 100644\n>> --- a/environment.c\n>> +++ b/environment.c\n>> @@ -128,6 +128,20 @@ const char * const local_repo_env[] = {\n>> NULL\n>> };\n>>\n>> +/* A subset of local_repo_env[] that contains path */\n>> +const char * const local_repo_path_env[] = {\n>> + ALTERNATE_DB_ENVIRONMENT,\n>> + CONFIG_ENVIRONMENT,\n>> + DB_ENVIRONMENT,\n>> + GIT_COMMON_DIR_ENVIRONMENT,\n>> + GIT_DIR_ENVIRONMENT,\n>> + GIT_SHALLOW_FILE_ENVIRONMENT,\n>> + GIT_WORK_TREE_ENVIRONMENT,\n>> + GRAFT_ENVIRONMENT,\n>> + INDEX_ENVIRONMENT,\n>> + NULL\n>> +};\n>\n> It might be nice to fold this list into local_repo_env automatically. I\n> think you'd have to do it with a macro.\n\nAha! I did not like the split either and wanted to turn\nlocal_repo_env[] to an array of struct so we can add attributes to\neach variable, but the way local_repo_env[] is being used, that's\nimpossible without more surgery.\n\n> OTOH, it's possible that there could be a path-related environment\n> variable that _isn't_ actually part of local_repo_env. E.g., I think\n> GIT_CONFIG might classify there (though I don't know if it's worth\n> worrying about).\n\nI'd rather fix it now and forget about it than trying to troubleshoot\na problem related to bad relative $GIT_CONFIG (even if the chance of\nthat happening is probably 1%)\n\n>> +static void update_path_envs(const char *old_cwd, const char *new_cwd,\n>> + void *cb)\n>> +{\n>> + int i;\n>> +\n>> + /*\n>> + * FIXME: special treatment needed for\n>> + * GIT_ALTERNATE_OBJECT_DIRECTORIES because it can contain\n>> + * multiple paths.\n>> + */\n>\n> Yuck. It just keeps getting more complicated. :(\n>\n> I do wonder if relative paths in variables like this are worth worrying\n> about. AFAIK, it's always been a \"well, it kind of works\" situation, but\n\nYeah. 99% of time $GIT_DIR is $GIT_WORK_TREE/.git and no path\nadjustment is needed.\n\n> not something we've tried to actively support. I think with the current\n> code you'd potentially get inconsistent results between a command which\n> sets up the work tree and one which doesn't. So this would be fixing\n> that, but at the same time, I'm not sure how much we want to promise\n> here.\n\nI would be just as happy to die() when we find out we have relative\npaths that takes too much work to reparent. It keeps the amount of\nwork down and will not bite us later (and will let us know if any user\nneeds it because they would have to report back after hitting the said\ndie()).\n-- \nDuy\n"},{"id":"343287","messageId":"xmqqh8oz7sai.fsf@gitster-ct.c.googlers.com","threadId":"48158","inReplyTo":"20180328175537.17450-1-pclouds@gmail.com","subject":"Re: [PATCH 0/8] Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-28T22:24:05Z","receivedAt":"2018-03-28T22:24:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> This is what I got, which is slightly different from your series\n> because I want to call set_git_dir() just one time (by\n> setup_git_directory) and never again. I think the API looks close\n> enough.\n>\n> I will probably rework on top of your chdir-notify instead (and let\n> yours to be merged earlier)\n\nI was occupied with the current cycle and did not have a chance to\nread this one over before seeing this exchange. After reading\nPeff's chdir-notify thing, I agree with its general direction and\nthe idea to refine on top of it, which you two seem to have agreed.\n\nThanks for working well together.\n"},{"id":"343326","messageId":"CACsJy8CCvshwb8a5ZozJ+x0D1kAYbeLSgJ0LDm4Z=uUZLtpsjg@mail.gmail.com","threadId":"48158","inReplyTo":"20180328174041.GB16274@sigill.intra.peff.net","subject":"Re: [PATCH 2/4] add chdir-notify API","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-03-29T14:53:42Z","receivedAt":"2018-03-29T14:54:20Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Mar 28, 2018 at 7:40 PM, Jeff King wrote:\n> +static void reparent_cb(const char *old_cwd,\n> + const char *new_cwd,\n> + void *data)\n> +{\n> + char **path = data;\n\nMaybe check data == NULL and return early. This is just for\nconvenience, e.g. instead of doing\n\npath = getenv(\"foo\");\nif (path)\n chdir_notify_reparent(&path);\n\nI could do\n\npath = getenv(\"foo\");\nchdir_notify_reparent(&path);\n\n> + char *tmp = *path;\n> + *path = reparent_relative_path(old_cwd, new_cwd, tmp);\n> + free(tmp);\n> +}\n> +\n> +void chdir_notify_reparent(char **path)\n> +{\n> + if (!is_absolute_path(*path))\n\nI think this check is a bit early. There could be a big gap between\nchdir_notify_reparent() and reparent_cb(). During that time, *path may\nbe updated and become a relative path. We can check for absolute path\ninside reparent_cb() instead.\n\n> + chdir_notify_register(reparent_cb, path);\n> +}\n\nOverall, I like this API very much! I will add another one similar to\nchdir_notify_reparent() but it takes an env name instead and the\ncallback will setenv() appropriately. The result looks super good.\n-- \nDuy\n"},{"id":"343328","messageId":"20180329145726.GA10253@duynguyen.home","threadId":"48158","inReplyTo":"20180328181932.GB16565@sigill.intra.peff.net","subject":"Re: [PATCH 0/8] Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-03-29T14:57:26Z","receivedAt":"2018-03-29T14:57:38Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Mar 28, 2018 at 02:19:32PM -0400, Jeff King wrote:\n> \n> > I will probably rework on top of your chdir-notify instead (and let\n> > yours to be merged earlier)\n> \n> Thanks. I like some of the related changes you made, like including this\n> in the tracing output. That should be easy to do on top of mine, I\n> think.\n\nYeah. But is it possible to sneak something like this in your series\n(I assume you will reroll anyway)? I could do it separately, but it\nlooks nicer if it's split out and merged in individual patches that\nadd new chdir-notify call site.\n\ndiff --git a/chdir-notify.c b/chdir-notify.c\nindex 7034e98d71..a2a33443f8 100644\n--- a/chdir-notify.c\n+++ b/chdir-notify.c\n@@ -4,21 +4,26 @@\n #include \"strbuf.h\"\n \n struct chdir_notify_entry {\n+\tconst char *name;\n \tchdir_notify_callback cb;\n \tvoid *data;\n \tstruct list_head list;\n };\n static LIST_HEAD(chdir_notify_entries);\n \n-void chdir_notify_register(chdir_notify_callback cb, void *data)\n+void chdir_notify_register(const char *name,\n+\t\t\t chdir_notify_callback cb,\n+\t\t\t void *data)\n {\n \tstruct chdir_notify_entry *e = xmalloc(sizeof(*e));\n \te->cb = cb;\n \te->data = data;\n+\te->name = name;\n \tlist_add_tail(&e->list, &chdir_notify_entries);\n }\n \n-static void reparent_cb(const char *old_cwd,\n+static void reparent_cb(const char *name,\n+\t\t\tconst char *old_cwd,\n \t\t\tconst char *new_cwd,\n \t\t\tvoid *data)\n {\n@@ -28,12 +33,16 @@ static void reparent_cb(const char *old_cwd,\n \tif (!tmp || !is_absolute_path(tmp))\n \t\treturn;\n \t*path = reparent_relative_path(old_cwd, new_cwd, tmp);\n+\tif (name)\n+\t\ttrace_printf_key(&trace_setup_key,\n+\t\t\t\t \"setup: reparent %s to '%s'\",\n+\t\t\t\t name, *path);\n \tfree(tmp);\n }\n \n-void chdir_notify_reparent(char **path)\n+void chdir_notify_reparent(const char *name, char **path)\n {\n-\tchdir_notify_register(reparent_cb, path);\n+\tchdir_notify_register(name, reparent_cb, path);\n }\n \n int chdir_notify(const char *new_cwd)\n@@ -45,11 +54,12 @@ int chdir_notify(const char *new_cwd)\n \t\treturn -1;\n \tif (chdir(new_cwd) < 0)\n \t\treturn -1;\n+\ttrace_printf_key(&trace_setup_key, \"setup: chdir to '%s'\", new_cwd);\n \n \tlist_for_each(pos, &chdir_notify_entries) {\n \t\tstruct chdir_notify_entry *e =\n \t\t\tlist_entry(pos, struct chdir_notify_entry, list);\n-\t\te->cb(old_cwd.buf, new_cwd, e->data);\n+\t\te->cb(e->name, old_cwd.buf, new_cwd, e->data);\n \t}\n \n \tstrbuf_release(&old_cwd);\ndiff --git a/chdir-notify.h b/chdir-notify.h\nindex c3bd818a85..b9be1b54ac 100644\n--- a/chdir-notify.h\n+++ b/chdir-notify.h\n@@ -16,23 +16,29 @@\n *\twarning(\"switched from %s to %s!\", old_path, new_path);\n * }\n * ...\n- * chdir_notify_register(foo, data);\n+ * chdir_notify_register(\"description\", foo, data);\n *\n * In practice most callers will want to move a relative path to the new root;\n * they can use the reparent_relative_path() helper for that. If that's all\n * you're doing, you can also use the convenience function:\n *\n- * chdir_notify_reparent(&my_path);\n+ * chdir_notify_reparent(\"description\", &my_path);\n *\n * Registered functions are called in the order in which they were added. Note\n * that there's currently no way to remove a function, so make sure that the\n * data parameter remains valid for the rest of the program.\n+ *\n+ * The first argument is used for tracing purpose only. when my_path is updated\n+ * by chdir_notify_reparent() it will print a message if $GIT_TRACE_SETUP is set.\n+ * This argument could be NULL.\n */\n-typedef void (*chdir_notify_callback)(const char *old_cwd,\n+typedef void (*chdir_notify_callback)(const char *name,\n+\t\t\t\t const char *old_cwd,\n \t\t\t\t const char *new_cwd,\n \t\t\t\t void *data);\n-void chdir_notify_register(chdir_notify_callback cb, void *data);\n-void chdir_notify_reparent(char **path);\n+void chdir_notify_register(const char *name, chdir_notify_callback cb,\n+\t\t\t void *data);\n+void chdir_notify_reparent(const char *name, char **path);\n \n /*\n *\ndiff --git a/environment.c b/environment.c\nindex 794a75a717..92701cbc0c 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -330,11 +330,15 @@ static void set_git_dir_1(const char *path)\n \tsetup_git_env(path);\n }\n \n-static void update_relative_gitdir(const char *old_cwd,\n+static void update_relative_gitdir(const char *name,\n+\t\t\t\t const char *old_cwd,\n \t\t\t\t const char *new_cwd,\n \t\t\t\t void *data)\n {\n \tchar *path = reparent_relative_path(old_cwd, new_cwd, get_git_dir());\n+\ttrace_printf_key(&trace_setup_key,\n+\t\t\t \"setup: move $GIR_DIR and others to '%s'\",\n+\t\t\t path);\n \tset_git_dir_1(path);\n \tfree(path);\n }\n@@ -343,7 +347,7 @@ void set_git_dir(const char *path)\n {\n \tset_git_dir_1(path);\n \tif (!is_absolute_path(path))\n-\t\tchdir_notify_register(update_relative_gitdir, NULL);\n+\t\tchdir_notify_register(NULL, update_relative_gitdir, NULL);\n }\n \n const char *get_log_output_encoding(void)\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex ab3e00ffa0..861072c0dc 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -107,8 +107,8 @@ static struct ref_store *files_ref_store_create(const char *gitdir,\n \trefs->packed_ref_store = packed_ref_store_create(sb.buf, flags);\n \tstrbuf_release(&sb);\n \n-\tchdir_notify_reparent(&refs->gitdir);\n-\tchdir_notify_reparent(&refs->gitcommondir);\n+\tchdir_notify_reparent(\"files-backend $GIT_DIR\", &refs->gitdir);\n+\tchdir_notify_reparent(\"files-backedn $GIT_COMMONDIR\", &refs->gitcommondir);\n \n \treturn ref_store;\n }\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 6725742f00..369c34f886 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -203,7 +203,7 @@ struct ref_store *packed_ref_store_create(const char *path,\n \trefs->store_flags = store_flags;\n \n \trefs->path = xstrdup(path);\n-\tchdir_notify_reparent(&refs->path);\n+\tchdir_notify_reparent(\"packed-refs\", &refs->path);\n \n \treturn ref_store;\n }\ndiff --git a/trace.c b/trace.c\nindex 7f3b08e148..fc623e91fd 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -26,6 +26,7 @@\n \n struct trace_key trace_default_key = { \"GIT_TRACE\", 0, 0, 0 };\n struct trace_key trace_perf_key = TRACE_KEY_INIT(PERFORMANCE);\n+struct trace_key trace_setup_key = TRACE_KEY_INIT(SETUP);\n \n /* Get a trace file descriptor from \"key\" env variable. */\n static int get_trace_fd(struct trace_key *key)\n@@ -300,11 +301,10 @@ static const char *quote_crnl(const char *path)\n /* FIXME: move prefix to startup_info struct and get rid of this arg */\n void trace_repo_setup(const char *prefix)\n {\n-\tstatic struct trace_key key = TRACE_KEY_INIT(SETUP);\n \tconst char *git_work_tree;\n \tchar *cwd;\n \n-\tif (!trace_want(&key))\n+\tif (!trace_want(&trace_setup_key))\n \t\treturn;\n \n \tcwd = xgetcwd();\n@@ -315,11 +315,11 @@ void trace_repo_setup(const char *prefix)\n \tif (!prefix)\n \t\tprefix = \"(null)\";\n \n-\ttrace_printf_key(&key, \"setup: git_dir: %s\\n\", quote_crnl(get_git_dir()));\n-\ttrace_printf_key(&key, \"setup: git_common_dir: %s\\n\", quote_crnl(get_git_common_dir()));\n-\ttrace_printf_key(&key, \"setup: worktree: %s\\n\", quote_crnl(git_work_tree));\n-\ttrace_printf_key(&key, \"setup: cwd: %s\\n\", quote_crnl(cwd));\n-\ttrace_printf_key(&key, \"setup: prefix: %s\\n\", quote_crnl(prefix));\n+\ttrace_printf_key(&trace_setup_key, \"setup: git_dir: %s\\n\", quote_crnl(get_git_dir()));\n+\ttrace_printf_key(&trace_setup_key, \"setup: git_common_dir: %s\\n\", quote_crnl(get_git_common_dir()));\n+\ttrace_printf_key(&trace_setup_key, \"setup: worktree: %s\\n\", quote_crnl(git_work_tree));\n+\ttrace_printf_key(&trace_setup_key, \"setup: cwd: %s\\n\", quote_crnl(cwd));\n+\ttrace_printf_key(&trace_setup_key, \"setup: prefix: %s\\n\", quote_crnl(prefix));\n \n \tfree(cwd);\n }\ndiff --git a/trace.h b/trace.h\nindex 88055abef7..2b6a1bc17c 100644\n--- a/trace.h\n+++ b/trace.h\n@@ -15,6 +15,7 @@ extern struct trace_key trace_default_key;\n \n #define TRACE_KEY_INIT(name) { \"GIT_TRACE_\" #name, 0, 0, 0 }\n extern struct trace_key trace_perf_key;\n+extern struct trace_key trace_setup_key;\n \n extern void trace_repo_setup(const char *prefix);\n extern int trace_want(struct trace_key *key);\n"},{"id":"343344","messageId":"CACsJy8CAjb4zdAgD5H9OZOAoHdrRFzGm9gaL8Wbna+GXJJCW4Q@mail.gmail.com","threadId":"48158","inReplyTo":"20180328174215.GC16274@sigill.intra.peff.net","subject":"Re: [PATCH 3/4] set_work_tree: use chdir_notify","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-03-29T17:02:21Z","receivedAt":"2018-03-29T17:02:58Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Mar 28, 2018 at 7:42 PM, Jeff King wrote:\n> When we change to the top of the working tree, we manually\n> re-adjust $GIT_DIR and call set_git_dir() again, in order to\n> update any relative git-dir we'd compute earlier.\n\nAnother way to approach this problem is not delaying chdir() at all.\nWe have to delay calling setup_work_tree() and not do it in\nsetup_git_directory() because it can die() when chdir() fails. But in\nmany cases, the command does not actually need the worktree and does\nnot deserve to die. But what if we make setup_work_tree be gentle?\n\nIf it successfully chdir() at the end of setup_git_directory() (and\nperhaps before the first set_git_dir call), great! The problem we're\ndealing here vanishes. If it fails, don't die, just set a flag. Later\non when a command requests a worktree, we can check this flag and now\ncan die().\n\nIt's less code this way, but it uses up more of your (or my) time\nbecause even though the first set_git_dir() call actually happens at 8\nplaces. Is it worth trying ?\n-- \nDuy\n"},{"id":"343345","messageId":"CACsJy8DP_D14ZwJnNjkqPDBXCyXYkW9mK_boqZDRxiTUFhMmMA@mail.gmail.com","threadId":"48158","inReplyTo":"CACsJy8CAjb4zdAgD5H9OZOAoHdrRFzGm9gaL8Wbna+GXJJCW4Q@mail.gmail.com","subject":"Re: [PATCH 3/4] set_work_tree: use chdir_notify","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-03-29T17:23:17Z","receivedAt":"2018-03-29T17:23:52Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Mar 29, 2018 at 7:02 PM, Duy Nguyen wrote:\n> ...\n>\n> It's less code this way, but it uses up more of your (or my) time\n> because even though the first set_git_dir() call actually happens at 8\n> places. Is it worth trying ?\n\nNever mind. I took a stab anyway. The setup code should have much less\nside effect before we can do stuff like this.\n-- \nDuy\n"},{"id":"343347","messageId":"20180329174820.GB31833@sigill.intra.peff.net","threadId":"48158","inReplyTo":"CACsJy8CCvshwb8a5ZozJ+x0D1kAYbeLSgJ0LDm4Z=uUZLtpsjg@mail.gmail.com","subject":"Re: [PATCH 2/4] add chdir-notify API","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-29T17:48:21Z","receivedAt":"2018-03-29T17:48:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 29, 2018 at 04:53:42PM +0200, Duy Nguyen wrote:\n\n> On Wed, Mar 28, 2018 at 7:40 PM, Jeff King wrote:\n> > +static void reparent_cb(const char *old_cwd,\n> > + const char *new_cwd,\n> > + void *data)\n> > +{\n> > + char **path = data;\n> \n> Maybe check data == NULL and return early. This is just for\n> convenience, e.g. instead of doing\n> \n> path = getenv(\"foo\");\n> if (path)\n> chdir_notify_reparent(&path);\n> \n> I could do\n> \n> path = getenv(\"foo\");\n> chdir_notify_reparent(&path);\n\nYou'd need to xstrdup(path) there anyway. I guess you could use\nxstrdup_or_null(), but it seems somewhat unlikely (unless your work on\ntop really does add such a callsite?).\n\nI guess it's not that much code to be careful, though.\n\n> > + char *tmp = *path;\n> > + *path = reparent_relative_path(old_cwd, new_cwd, tmp);\n> > + free(tmp);\n> > +}\n> > +\n> > +void chdir_notify_reparent(char **path)\n> > +{\n> > + if (!is_absolute_path(*path))\n> \n> I think this check is a bit early. There could be a big gap between\n> chdir_notify_reparent() and reparent_cb(). During that time, *path may\n> be updated and become a relative path. We can check for absolute path\n> inside reparent_cb() instead.\n\nMy thinking was that we'd be effectively zero-cost for an absolute path,\nthough really adding an element to the notification list is probably not\na big deal.\n\nI also assumed that whoever changed it to a relative path would then\ninstall the notification handler. One thing that my series kind of\nglosses over is whether somebody might call any of these functions\ntwice, which would involve setting up the handler twice. So e.g. if you\ndid:\n\n set_git_dir(\"foo\");\n set_git_dir(\"bar\");\n\nwe'd have two handlers, which would do the wrong thing when the second\none triggered. I hoped we could eventually add a BUG() if that happens,\nbut maybe we should simply do a:\n\n static int registered_chdir;\n\n if (!registered_chdir) {\n\tchdir_notify_reparent(&path);\n\tregistered_chdir = 1;\n }\n\nat each call-site. Another alternative would be to make it a noop to\nfeed the same path twice. That requires a linear walk through the list,\nbut it's a pretty small list.\n\n> > + chdir_notify_register(reparent_cb, path);\n> > +}\n> \n> Overall, I like this API very much! I will add another one similar to\n> chdir_notify_reparent() but it takes an env name instead and the\n> callback will setenv() appropriately. The result looks super good.\n\nOoh, that's clever.\n\nI had also thought of extending it to other notifications besides\nchdir(), like one string depends on another. But then we'd basically\nre-inventing a cascading data-flow system, which is probably getting a\nbit magical. :)\n\n-Peff\n"},{"id":"343348","messageId":"20180329175007.GC31833@sigill.intra.peff.net","threadId":"48158","inReplyTo":"CACsJy8CAjb4zdAgD5H9OZOAoHdrRFzGm9gaL8Wbna+GXJJCW4Q@mail.gmail.com","subject":"Re: [PATCH 3/4] set_work_tree: use chdir_notify","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-29T17:50:07Z","receivedAt":"2018-03-29T17:50:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 29, 2018 at 07:02:21PM +0200, Duy Nguyen wrote:\n\n> On Wed, Mar 28, 2018 at 7:42 PM, Jeff King wrote:\n> > When we change to the top of the working tree, we manually\n> > re-adjust $GIT_DIR and call set_git_dir() again, in order to\n> > update any relative git-dir we'd compute earlier.\n> \n> Another way to approach this problem is not delaying chdir() at all.\n> We have to delay calling setup_work_tree() and not do it in\n> setup_git_directory() because it can die() when chdir() fails. But in\n> many cases, the command does not actually need the worktree and does\n> not deserve to die. But what if we make setup_work_tree be gentle?\n> \n> If it successfully chdir() at the end of setup_git_directory() (and\n> perhaps before the first set_git_dir call), great! The problem we're\n> dealing here vanishes. If it fails, don't die, just set a flag. Later\n> on when a command requests a worktree, we can check this flag and now\n> can die().\n> \n> It's less code this way, but it uses up more of your (or my) time\n> because even though the first set_git_dir() call actually happens at 8\n> places. Is it worth trying ?\n\nI do kind of like that. I'm reasonably happy with the chdir_notify()\ninterface, but it would be nicer still if we could get rid of it in the\nfirst place. It's true that we _could_ chdir from other places, but\nrealistically speaking, we do our one big chdir as part of the setup\nprocess.\n\n-Peff\n"},{"id":"343349","messageId":"20180329175042.GD31833@sigill.intra.peff.net","threadId":"48158","inReplyTo":"CACsJy8DP_D14ZwJnNjkqPDBXCyXYkW9mK_boqZDRxiTUFhMmMA@mail.gmail.com","subject":"Re: [PATCH 3/4] set_work_tree: use chdir_notify","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-29T17:50:42Z","receivedAt":"2018-03-29T17:50:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 29, 2018 at 07:23:17PM +0200, Duy Nguyen wrote:\n\n> On Thu, Mar 29, 2018 at 7:02 PM, Duy Nguyen wrote:\n> > ...\n> >\n> > It's less code this way, but it uses up more of your (or my) time\n> > because even though the first set_git_dir() call actually happens at 8\n> > places. Is it worth trying ?\n> \n> Never mind. I took a stab anyway. The setup code should have much less\n> side effect before we can do stuff like this.\n\nOh, I should have read this before responding. Oh well. :)\n\n-Peff\n"},{"id":"343353","messageId":"CACsJy8CdqpNOw+zdMyugX-902Z=gLNij5_xcmE4jGLRBTqiO1g@mail.gmail.com","threadId":"48158","inReplyTo":"20180329175007.GC31833@sigill.intra.peff.net","subject":"Re: [PATCH 3/4] set_work_tree: use chdir_notify","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-03-29T18:01:33Z","receivedAt":"2018-03-29T18:02:11Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Mar 29, 2018 at 7:50 PM, Jeff King wrote:\n> On Thu, Mar 29, 2018 at 07:02:21PM +0200, Duy Nguyen wrote:\n>\n>> On Wed, Mar 28, 2018 at 7:42 PM, Jeff King wrote:\n>> > When we change to the top of the working tree, we manually\n>> > re-adjust $GIT_DIR and call set_git_dir() again, in order to\n>> > update any relative git-dir we'd compute earlier.\n>>\n>> Another way to approach this problem is not delaying chdir() at all.\n>> We have to delay calling setup_work_tree() and not do it in\n>> setup_git_directory() because it can die() when chdir() fails. But in\n>> many cases, the command does not actually need the worktree and does\n>> not deserve to die. But what if we make setup_work_tree be gentle?\n>>\n>> If it successfully chdir() at the end of setup_git_directory() (and\n>> perhaps before the first set_git_dir call), great! The problem we're\n>> dealing here vanishes. If it fails, don't die, just set a flag. Later\n>> on when a command requests a worktree, we can check this flag and now\n>> can die().\n>>\n>> It's less code this way, but it uses up more of your (or my) time\n>> because even though the first set_git_dir() call actually happens at 8\n>> places. Is it worth trying ?\n>\n> I do kind of like that. I'm reasonably happy with the chdir_notify()\n> interface, but it would be nicer still if we could get rid of it in the\n> first place. It's true that we _could_ chdir from other places, but\n\nThere's another place we do, that I should mention and keep\nforgetting. Our run-command.c code allows to switch cwd, and if\n$GIT_DIR and stuff is relative then we should reparent them too just\nlike we do with setup_work_tree(). Your chdir-notify makes it super\neasy to support that, we just need to move the prep_childenv() down\nbelow chdir(). But since nobody has complaint, I suppose that feature\nis not really popular (or at least not used to launch another git\nprocess anyway)\n\n> realistically speaking, we do our one big chdir as part of the setup\n> process.\n-- \nDuy\n"},{"id":"343354","messageId":"CACsJy8BUBim-yKgxxGicohPMr6fHOFB0ANFnZjMqnGFk1iaQHQ@mail.gmail.com","threadId":"48158","inReplyTo":"20180329174820.GB31833@sigill.intra.peff.net","subject":"Re: [PATCH 2/4] add chdir-notify API","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-03-29T18:12:01Z","receivedAt":"2018-03-29T18:12:39Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Mar 29, 2018 at 7:48 PM, Jeff King wrote:\n> On Thu, Mar 29, 2018 at 04:53:42PM +0200, Duy Nguyen wrote:\n>\n>> On Wed, Mar 28, 2018 at 7:40 PM, Jeff King wrote:\n>> > +static void reparent_cb(const char *old_cwd,\n>> > + const char *new_cwd,\n>> > + void *data)\n>> > +{\n>> > + char **path = data;\n>>\n>> Maybe check data == NULL and return early. This is just for\n>> convenience, e.g. instead of doing\n>>\n>> path = getenv(\"foo\");\n>> if (path)\n>> chdir_notify_reparent(&path);\n>>\n>> I could do\n>>\n>> path = getenv(\"foo\");\n>> chdir_notify_reparent(&path);\n>\n> You'd need to xstrdup(path) there anyway. I guess you could use\n> xstrdup_or_null(), but it seems somewhat unlikely (unless your work on\n> top really does add such a callsite?).\n\nIt actually exists in 'next', repository.c where we have this line\n\n repo->alternate_db = xstrdup_or_null(o->alternate_db);\n\n> I guess it's not that much code to be careful, though.\n\n>> > +void chdir_notify_reparent(char **path)\n>> > +{\n>> > + if (!is_absolute_path(*path))\n>>\n>> I think this check is a bit early. There could be a big gap between\n>> chdir_notify_reparent() and reparent_cb(). During that time, *path may\n>> be updated and become a relative path. We can check for absolute path\n>> inside reparent_cb() instead.\n>\n> My thinking was that we'd be effectively zero-cost for an absolute path,\n> though really adding an element to the notification list is probably not\n> a big deal.\n\nI think we could leave such optimization to the caller. I'm ok with\nkeeping this \"if\" too, but you may need to clarify it in the big\ncomment block in chdir-notify.h because this behavior to me is\nsurprising.\n\n> I also assumed that whoever changed it to a relative path would then\n> install the notification handler. One thing that my series kind of\n> glosses over is whether somebody might call any of these functions\n> twice, which would involve setting up the handler twice. So e.g. if you\n> did:\n>\n> set_git_dir(\"foo\");\n> set_git_dir(\"bar\");\n>\n> we'd have two handlers, which would do the wrong thing when the second\n> one triggered. I hoped we could eventually add a BUG() if that happens,\n> but maybe we should simply do a:\n>\n> static int registered_chdir;\n>\n> if (!registered_chdir) {\n> chdir_notify_reparent(&path);\n> registered_chdir = 1;\n> }\n>\n> at each call-site. Another alternative would be to make it a noop to\n> feed the same path twice. That requires a linear walk through the list,\n> but it's a pretty small list.\n\nWell, given the number of call sites is rather small at the moment, I\nthink chdir-notify can just stay dumb and let the caller deal with\nduplicate registration. One thing I like about your linked list design\nthough, is it makes it quite easy to remove (or even update) a\ncallback. You can simply return the (opaque) pointer to the entire\nchdir_notify_entry as a handle and we can free/unlink the entry based\non it. It's kinda hard to me to do it with array-based design.\n-- \nDuy\n"},{"id":"343440","messageId":"20180330172100.GA18648@sigill.intra.peff.net","threadId":"48158","inReplyTo":"20180329145726.GA10253@duynguyen.home","subject":"Re: [PATCH 0/8] Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-30T17:21:00Z","receivedAt":"2018-03-30T17:21:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 29, 2018 at 04:57:26PM +0200, Duy Nguyen wrote:\n\n> On Wed, Mar 28, 2018 at 02:19:32PM -0400, Jeff King wrote:\n> > \n> > > I will probably rework on top of your chdir-notify instead (and let\n> > > yours to be merged earlier)\n> > \n> > Thanks. I like some of the related changes you made, like including this\n> > in the tracing output. That should be easy to do on top of mine, I\n> > think.\n> \n> Yeah. But is it possible to sneak something like this in your series\n> (I assume you will reroll anyway)? I could do it separately, but it\n> looks nicer if it's split out and merged in individual patches that\n> add new chdir-notify call site.\n\nSure.\n\n> -void chdir_notify_register(chdir_notify_callback cb, void *data)\n> +void chdir_notify_register(const char *name,\n> +\t\t\t chdir_notify_callback cb,\n> +\t\t\t void *data)\n> {\n> \tstruct chdir_notify_entry *e = xmalloc(sizeof(*e));\n> \te->cb = cb;\n> \te->data = data;\n> +\te->name = name;\n> \tlist_add_tail(&e->list, &chdir_notify_entries);\n> }\n\nI'm tempted to make a copy of the name here (or at least document that\nit must remain valid forever).\n\n-Peff\n"},{"id":"343441","messageId":"20180330172324.GB18648@sigill.intra.peff.net","threadId":"48158","inReplyTo":"CACsJy8CdqpNOw+zdMyugX-902Z=gLNij5_xcmE4jGLRBTqiO1g@mail.gmail.com","subject":"Re: [PATCH 3/4] set_work_tree: use chdir_notify","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-30T17:23:24Z","receivedAt":"2018-03-30T17:23:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 29, 2018 at 08:01:33PM +0200, Duy Nguyen wrote:\n\n> > I do kind of like that. I'm reasonably happy with the chdir_notify()\n> > interface, but it would be nicer still if we could get rid of it in the\n> > first place. It's true that we _could_ chdir from other places, but\n> \n> There's another place we do, that I should mention and keep\n> forgetting. Our run-command.c code allows to switch cwd, and if\n> $GIT_DIR and stuff is relative then we should reparent them too just\n> like we do with setup_work_tree(). Your chdir-notify makes it super\n> easy to support that, we just need to move the prep_childenv() down\n> below chdir(). But since nobody has complaint, I suppose that feature\n> is not really popular (or at least not used to launch another git\n> process anyway)\n\nYeah, I hadn't really considered that, since it would only matter for\nenvironment variables, not for internal strings (which are all about to\nget thrown out due to execve anyway). And my patch was just focusing on\nthat.\n\nBut I also wonder if rewriting the environment variables would matter\nhere. If we're going to chdir for a child, we'd generaly be clearing\nrepo-related env anyway.\n\n-Peff\n"},{"id":"343447","messageId":"20180330183425.GA30575@sigill.intra.peff.net","threadId":"48158","inReplyTo":"20180328173656.GA29094@sigill.intra.peff.net","subject":"[PATCH v2 0/5] re-parenting relative directories after chdir","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-30T18:34:25Z","receivedAt":"2018-03-30T18:34:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 28, 2018 at 01:36:56PM -0400, Jeff King wrote:\n\n> For those just joining us, this fixes a regression that was in v2.13 (so\n> nothing we need to worry about as part of the 2.17-rc track).\n> \n> [1/4]: set_git_dir: die when setenv() fails\n> [2/4]: add chdir-notify API\n> [3/4]: set_work_tree: use chdir_notify\n> [4/4]: refs: use chdir_notify to update cached relative paths\n\nHere's a re-roll based on the feedback I got, including:\n\n - fixes the memory leak and vague comment pointed out by Eric\n\n - adds in tracing code from Duy\n\n - I took Duy's suggestions regarding \"least\" surprise in some of the\n functions (reparenting NULL is a noop, and registering a reparent\n handler does so even for an absolute path).\n\nI punted on the \"registering the same path twice\" thing. That is a\npotential way to misuse this API, but I don't think there's a good\nsolution. The \"reparent\" helper could figure this out for you, but in\nthe general case we actually install an arbitrary callback. So the\ncaller really has to handle it there.\n\nI think in the long run we'd want to outlaw calling set_git_dir() twice\nanyway. But possibly the handler-installers should be more careful.\n\nI'm dropping poor Rafael from the cc here, since it is turning into\nquite a lot of messages. :) I think at this point his bug has been duly\nreported and we are working on the fix.\n\n [1/5]: set_git_dir: die when setenv() fails\n [2/5]: trace.c: export trace_setup_key\n [3/5]: add chdir-notify API\n [4/5]: set_work_tree: use chdir_notify\n [5/5]: refs: use chdir_notify to update cached relative paths\n\n Makefile | 1 +\n cache.h | 2 +-\n chdir-notify.c | 93 +++++++++++++++++++++++++++++++++++++++++++\n chdir-notify.h | 73 +++++++++++++++++++++++++++++++++\n environment.c | 26 ++++++++++--\n refs/files-backend.c | 6 +++\n refs/packed-backend.c | 3 ++\n setup.c | 9 ++---\n t/t1501-work-tree.sh | 12 ++++++\n trace.c | 14 +++----\n trace.h | 1 +\n 11 files changed, 223 insertions(+), 17 deletions(-)\n create mode 100644 chdir-notify.c\n create mode 100644 chdir-notify.h\n\n-Peff\n"},{"id":"343448","messageId":"20180330183446.GA31135@sigill.intra.peff.net","threadId":"48158","inReplyTo":"20180330183425.GA30575@sigill.intra.peff.net","subject":"[PATCH v2 1/5] set_git_dir: die when setenv() fails","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-30T18:34:46Z","receivedAt":"2018-03-30T18:34:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The set_git_dir() function returns an error if setenv()\nfails, but there are zero callers who pay attention to this\nreturn value. If this ever were to happen, it could cause\nconfusing results, as sub-processes would see a potentially\nstale GIT_DIR (e.g., if it is relative and we chdir()-ed to\nthe root of the working tree).\n\nWe _could_ try to fix each caller, but there's really\nnothing useful to do after this failure except die. Let's\njust lump setenv() failure into the same category as malloc\nfailure: things that should never happen and cause us to\nabort catastrophically.\n\nSigned-off-by: Jeff King \n---\n cache.h | 2 +-\n environment.c | 5 ++---\n 2 files changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex a61b2d3f0d..5c24394d84 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -477,7 +477,7 @@ extern const char *get_git_common_dir(void);\n extern char *get_object_directory(void);\n extern char *get_index_file(void);\n extern char *get_graft_file(void);\n-extern int set_git_dir(const char *path);\n+extern void set_git_dir(const char *path);\n extern int get_common_dir_noenv(struct strbuf *sb, const char *gitdir);\n extern int get_common_dir(struct strbuf *sb, const char *gitdir);\n extern const char *get_git_namespace(void);\ndiff --git a/environment.c b/environment.c\nindex d6dd64662c..e01acf8b11 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -296,13 +296,12 @@ char *get_graft_file(void)\n \treturn the_repository->graft_file;\n }\n \n-int set_git_dir(const char *path)\n+void set_git_dir(const char *path)\n {\n \tif (setenv(GIT_DIR_ENVIRONMENT, path, 1))\n-\t\treturn error(\"Could not set GIT_DIR to '%s'\", path);\n+\t\tdie(\"could not set GIT_DIR to '%s'\", path);\n \trepo_set_gitdir(the_repository, path);\n \tsetup_git_env();\n-\treturn 0;\n }\n \n const char *get_log_output_encoding(void)\n-- \n2.17.0.rc2.594.gdb94a0ce02\n\n"},{"id":"343449","messageId":"20180330183459.GB31135@sigill.intra.peff.net","threadId":"48158","inReplyTo":"20180330183425.GA30575@sigill.intra.peff.net","subject":"[PATCH v2 2/5] trace.c: export trace_setup_key","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-30T18:34:59Z","receivedAt":"2018-03-30T18:35:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Nguyễn Thái Ngọc Duy \n\nThis is so that we can print traces based on this key outside trace.c.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: Jeff King \n---\n trace.c | 14 +++++++-------\n trace.h | 1 +\n 2 files changed, 8 insertions(+), 7 deletions(-)\n\ndiff --git a/trace.c b/trace.c\nindex 7f3b08e148..fc623e91fd 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -26,6 +26,7 @@\n \n struct trace_key trace_default_key = { \"GIT_TRACE\", 0, 0, 0 };\n struct trace_key trace_perf_key = TRACE_KEY_INIT(PERFORMANCE);\n+struct trace_key trace_setup_key = TRACE_KEY_INIT(SETUP);\n \n /* Get a trace file descriptor from \"key\" env variable. */\n static int get_trace_fd(struct trace_key *key)\n@@ -300,11 +301,10 @@ static const char *quote_crnl(const char *path)\n /* FIXME: move prefix to startup_info struct and get rid of this arg */\n void trace_repo_setup(const char *prefix)\n {\n-\tstatic struct trace_key key = TRACE_KEY_INIT(SETUP);\n \tconst char *git_work_tree;\n \tchar *cwd;\n \n-\tif (!trace_want(&key))\n+\tif (!trace_want(&trace_setup_key))\n \t\treturn;\n \n \tcwd = xgetcwd();\n@@ -315,11 +315,11 @@ void trace_repo_setup(const char *prefix)\n \tif (!prefix)\n \t\tprefix = \"(null)\";\n \n-\ttrace_printf_key(&key, \"setup: git_dir: %s\\n\", quote_crnl(get_git_dir()));\n-\ttrace_printf_key(&key, \"setup: git_common_dir: %s\\n\", quote_crnl(get_git_common_dir()));\n-\ttrace_printf_key(&key, \"setup: worktree: %s\\n\", quote_crnl(git_work_tree));\n-\ttrace_printf_key(&key, \"setup: cwd: %s\\n\", quote_crnl(cwd));\n-\ttrace_printf_key(&key, \"setup: prefix: %s\\n\", quote_crnl(prefix));\n+\ttrace_printf_key(&trace_setup_key, \"setup: git_dir: %s\\n\", quote_crnl(get_git_dir()));\n+\ttrace_printf_key(&trace_setup_key, \"setup: git_common_dir: %s\\n\", quote_crnl(get_git_common_dir()));\n+\ttrace_printf_key(&trace_setup_key, \"setup: worktree: %s\\n\", quote_crnl(git_work_tree));\n+\ttrace_printf_key(&trace_setup_key, \"setup: cwd: %s\\n\", quote_crnl(cwd));\n+\ttrace_printf_key(&trace_setup_key, \"setup: prefix: %s\\n\", quote_crnl(prefix));\n \n \tfree(cwd);\n }\ndiff --git a/trace.h b/trace.h\nindex 88055abef7..2b6a1bc17c 100644\n--- a/trace.h\n+++ b/trace.h\n@@ -15,6 +15,7 @@ extern struct trace_key trace_default_key;\n \n #define TRACE_KEY_INIT(name) { \"GIT_TRACE_\" #name, 0, 0, 0 }\n extern struct trace_key trace_perf_key;\n+extern struct trace_key trace_setup_key;\n \n extern void trace_repo_setup(const char *prefix);\n extern int trace_want(struct trace_key *key);\n-- \n2.17.0.rc2.594.gdb94a0ce02\n\n"},{"id":"343450","messageId":"20180330183504.GC31135@sigill.intra.peff.net","threadId":"48158","inReplyTo":"20180330183425.GA30575@sigill.intra.peff.net","subject":"[PATCH v2 3/5] add chdir-notify API","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-30T18:35:04Z","receivedAt":"2018-03-30T18:35:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If one part of the code does a permanent chdir(), then this\ninvalidates any relative paths that may be held by other\nparts of the code. For example, setup_work_tree() moves us\nto the top of the working tree, which may invalidate a\npreviously stored relative gitdir.\n\nWe've hacked around this case by teaching setup_work_tree()\nto re-run set_git_dir() with an adjusted path, but this\nstomps all over the idea of module boundaries.\nsetup_work_tree() shouldn't have to know all of the places\nthat need to be fed an adjusted path. And indeed, there's at\nleast one other place (the refs code) which needs adjusting.\n\nLet's provide an API to let code that stores relative paths\n\"subscribe\" to updates to the current working directory.\nThis means that callers of chdir() don't need to know about\nall subscribers ahead of time; they can simply consult a\ndynamically built list.\n\nNote that our helper function to reparent relative paths\nuses the simple remove_leading_path(). We could in theory\nuse the much smarter relative_path(), but that led to some\nproblems as described in 41894ae3a3 (Use simpler\nrelative_path when set_git_dir, 2013-10-14). Since we're\naiming to replace the setup_work_tree() code here, let's\nfollow its lead.\n\nSigned-off-by: Jeff King \n---\n Makefile | 1 +\n chdir-notify.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++\n chdir-notify.h | 73 +++++++++++++++++++++++++++++++++++++++\n 3 files changed, 167 insertions(+)\n create mode 100644 chdir-notify.c\n create mode 100644 chdir-notify.h\n\ndiff --git a/Makefile b/Makefile\nindex a1d8775adb..0da98b9274 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -772,6 +772,7 @@ LIB_OBJS += branch.o\n LIB_OBJS += bulk-checkin.o\n LIB_OBJS += bundle.o\n LIB_OBJS += cache-tree.o\n+LIB_OBJS += chdir-notify.o\n LIB_OBJS += checkout.o\n LIB_OBJS += color.o\n LIB_OBJS += column.o\ndiff --git a/chdir-notify.c b/chdir-notify.c\nnew file mode 100644\nindex 0000000000..5f7f2c2ac2\n--- /dev/null\n+++ b/chdir-notify.c\n@@ -0,0 +1,93 @@\n+#include \"cache.h\"\n+#include \"chdir-notify.h\"\n+#include \"list.h\"\n+#include \"strbuf.h\"\n+\n+struct chdir_notify_entry {\n+\tconst char *name;\n+\tchdir_notify_callback cb;\n+\tvoid *data;\n+\tstruct list_head list;\n+};\n+static LIST_HEAD(chdir_notify_entries);\n+\n+void chdir_notify_register(const char *name,\n+\t\t\t chdir_notify_callback cb,\n+\t\t\t void *data)\n+{\n+\tstruct chdir_notify_entry *e = xmalloc(sizeof(*e));\n+\te->name = name;\n+\te->cb = cb;\n+\te->data = data;\n+\tlist_add_tail(&e->list, &chdir_notify_entries);\n+}\n+\n+static void reparent_cb(const char *name,\n+\t\t\tconst char *old_cwd,\n+\t\t\tconst char *new_cwd,\n+\t\t\tvoid *data)\n+{\n+\tchar **path = data;\n+\tchar *tmp = *path;\n+\n+\tif (!tmp)\n+\t\treturn;\n+\n+\t*path = reparent_relative_path(old_cwd, new_cwd, tmp);\n+\tfree(tmp);\n+\n+\tif (name) {\n+\t\ttrace_printf_key(&trace_setup_key,\n+\t\t\t\t \"setup: reparent %s to '%s'\",\n+\t\t\t\t name, *path);\n+\t}\n+}\n+\n+void chdir_notify_reparent(const char *name, char **path)\n+{\n+\tchdir_notify_register(name, reparent_cb, path);\n+}\n+\n+int chdir_notify(const char *new_cwd)\n+{\n+\tstruct strbuf old_cwd = STRBUF_INIT;\n+\tstruct list_head *pos;\n+\n+\tif (strbuf_getcwd(&old_cwd) < 0)\n+\t\treturn -1;\n+\tif (chdir(new_cwd) < 0) {\n+\t\tint saved_errno = errno;\n+\t\tstrbuf_release(&old_cwd);\n+\t\terrno = saved_errno;\n+\t\treturn -1;\n+\t}\n+\n+\ttrace_printf_key(&trace_setup_key,\n+\t\t\t \"setup: chdir from '%s' to '%s'\",\n+\t\t\t old_cwd.buf, new_cwd);\n+\n+\tlist_for_each(pos, &chdir_notify_entries) {\n+\t\tstruct chdir_notify_entry *e =\n+\t\t\tlist_entry(pos, struct chdir_notify_entry, list);\n+\t\te->cb(e->name, old_cwd.buf, new_cwd, e->data);\n+\t}\n+\n+\tstrbuf_release(&old_cwd);\n+\treturn 0;\n+}\n+\n+char *reparent_relative_path(const char *old_cwd,\n+\t\t\t const char *new_cwd,\n+\t\t\t const char *path)\n+{\n+\tchar *ret, *full;\n+\n+\tif (is_absolute_path(path))\n+\t\treturn xstrdup(path);\n+\n+\tfull = xstrfmt(\"%s/%s\", old_cwd, path);\n+\tret = xstrdup(remove_leading_path(full, new_cwd));\n+\tfree(full);\n+\n+\treturn ret;\n+}\ndiff --git a/chdir-notify.h b/chdir-notify.h\nnew file mode 100644\nindex 0000000000..366e4c1ee9\n--- /dev/null\n+++ b/chdir-notify.h\n@@ -0,0 +1,73 @@\n+#ifndef CHDIR_NOTIFY_H\n+#define CHDIR_NOTIFY_H\n+\n+/*\n+ * An API to let code \"subscribe\" to changes to the current working directory.\n+ * The general idea is that some code asks to be notified when the working\n+ * directory changes, and other code that calls chdir uses a special wrapper\n+ * that notifies everyone.\n+ */\n+\n+/*\n+ * Callers who need to know about changes can do:\n+ *\n+ * void foo(const char *old_path, const char *new_path, void *data)\n+ * {\n+ *\twarning(\"switched from %s to %s!\", old_path, new_path);\n+ * }\n+ * ...\n+ * chdir_notify_register(\"description\", foo, data);\n+ *\n+ * In practice most callers will want to move a relative path to the new root;\n+ * they can use the reparent_relative_path() helper for that. If that's all\n+ * you're doing, you can also use the convenience function:\n+ *\n+ * chdir_notify_reparent(\"description\", &my_path);\n+ *\n+ * Whenever a chdir event occurs, that will update my_path (if it's relative)\n+ * to adjust for the new cwd by freeing any existing string and allocating a\n+ * new one.\n+ *\n+ * Registered functions are called in the order in which they were added. Note\n+ * that there's currently no way to remove a function, so make sure that the\n+ * data parameter remains valid for the rest of the program.\n+ *\n+ * The \"name\" argument is used only for printing trace output from\n+ * $GIT_TRACE_SETUP. It may be NULL, but if non-NULL should point to\n+ * storage which lasts as long as the registration is active.\n+ */\n+typedef void (*chdir_notify_callback)(const char *name,\n+\t\t\t\t const char *old_cwd,\n+\t\t\t\t const char *new_cwd,\n+\t\t\t\t void *data);\n+void chdir_notify_register(const char *name, chdir_notify_callback cb, void *data);\n+void chdir_notify_reparent(const char *name, char **path);\n+\n+/*\n+ *\n+ * Callers that want to chdir:\n+ *\n+ * chdir_notify(new_path);\n+ *\n+ * to switch to the new path and notify any callbacks.\n+ *\n+ * Note that you don't need to chdir_notify() if you're just temporarily moving\n+ * to a directory and back, as long as you don't call any subscribed code in\n+ * between (but it should be safe to do so if you're unsure).\n+ */\n+int chdir_notify(const char *new_cwd);\n+\n+/*\n+ * Reparent a relative path from old_root to new_root. For example:\n+ *\n+ * reparent_relative_path(\"/a\", \"/a/b\", \"b/rel\");\n+ *\n+ * would return the (newly allocated) string \"rel\". Note that we may return an\n+ * absolute path in some cases (e.g., if the resulting path is not inside\n+ * new_cwd).\n+ */\n+char *reparent_relative_path(const char *old_cwd,\n+\t\t\t const char *new_cwd,\n+\t\t\t const char *path);\n+\n+#endif /* CHDIR_NOTIFY_H */\n-- \n2.17.0.rc2.594.gdb94a0ce02\n\n"},{"id":"343451","messageId":"20180330183508.GD31135@sigill.intra.peff.net","threadId":"48158","inReplyTo":"20180330183425.GA30575@sigill.intra.peff.net","subject":"[PATCH v2 4/5] set_work_tree: use chdir_notify","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-30T18:35:08Z","receivedAt":"2018-03-30T18:35:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we change to the top of the working tree, we manually\nre-adjust $GIT_DIR and call set_git_dir() again, in order to\nupdate any relative git-dir we'd compute earlier.\n\nInstead of the work-tree code having to know to call the\ngit-dir code, let's use the new chdir_notify interface.\nThere are two spots that need updating, with a few\nsubtleties in each:\n\n 1. the set_git_dir() code needs to chdir_notify_register()\n so it can be told when to update its path.\n\n Technically we could push this down into repo_set_gitdir(),\n so that even repository structs besides the_repository\n could benefit from this. But that opens up a lot of\n complications:\n\n - we'd still need to touch set_git_dir(), because it\n does some other setup (like setting $GIT_DIR in the\n environment)\n\n - submodules using other repository structs get\n cleaned up, which means we'd need to remove them\n from the chdir_notify list\n\n - it's unlikely to fix any bugs, since we shouldn't\n generally chdir() in the middle of working on a\n submodule\n\n 2. setup_work_tree now needs to call chdir_notify(), and\n can lose its manual set_git_dir() call.\n\n Note that at first glance it looks like this undoes the\n absolute-to-relative optimization added by 044bbbcb63\n (Make git_dir a path relative to work_tree in\n setup_work_tree(), 2008-06-19). But for the most part\n that optimization was just _undoing_ the\n relative-to-absolute conversion which the function was\n doing earlier (and which is now gone).\n\n It is true that if you already have an absolute git_dir\n that the setup_work_tree() function will no longer make\n it relative as a side effect. But:\n\n - we generally do have relative git-dir's due to the\n way the discovery code works\n\n - if we really care about making git-dir's relative\n when possible, then we should be relativizing them\n earlier (e.g., when we see an absolute $GIT_DIR we\n could turn it relative, whether we are going to\n chdir into a worktree or not). That would cover all\n cases, including ones that 044bbbcb63 did not.\n\nSigned-off-by: Jeff King \n---\n environment.c | 23 ++++++++++++++++++++++-\n setup.c | 9 +++------\n 2 files changed, 25 insertions(+), 7 deletions(-)\n\ndiff --git a/environment.c b/environment.c\nindex e01acf8b11..903a6c9df7 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -13,6 +13,7 @@\n #include \"refs.h\"\n #include \"fmt-merge-msg.h\"\n #include \"commit.h\"\n+#include \"chdir-notify.h\"\n \n int trust_executable_bit = 1;\n int trust_ctime = 1;\n@@ -296,7 +297,7 @@ char *get_graft_file(void)\n \treturn the_repository->graft_file;\n }\n \n-void set_git_dir(const char *path)\n+static void set_git_dir_1(const char *path)\n {\n \tif (setenv(GIT_DIR_ENVIRONMENT, path, 1))\n \t\tdie(\"could not set GIT_DIR to '%s'\", path);\n@@ -304,6 +305,26 @@ void set_git_dir(const char *path)\n \tsetup_git_env();\n }\n \n+static void update_relative_gitdir(const char *name,\n+\t\t\t\t const char *old_cwd,\n+\t\t\t\t const char *new_cwd,\n+\t\t\t\t void *data)\n+{\n+\tchar *path = reparent_relative_path(old_cwd, new_cwd, get_git_dir());\n+\ttrace_printf_key(&trace_setup_key,\n+\t\t\t \"setup: move $GIT_DIR to '%s'\",\n+\t\t\t path);\n+\tset_git_dir_1(path);\n+\tfree(path);\n+}\n+\n+void set_git_dir(const char *path)\n+{\n+\tset_git_dir_1(path);\n+\tif (!is_absolute_path(path))\n+\t\tchdir_notify_register(NULL, update_relative_gitdir, NULL);\n+}\n+\n const char *get_log_output_encoding(void)\n {\n \treturn git_log_output_encoding ? git_log_output_encoding\ndiff --git a/setup.c b/setup.c\nindex 7287779642..9eb2e808e1 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -3,6 +3,7 @@\n #include \"config.h\"\n #include \"dir.h\"\n #include \"string-list.h\"\n+#include \"chdir-notify.h\"\n \n static int inside_git_dir = -1;\n static int inside_work_tree = -1;\n@@ -378,7 +379,7 @@ int is_inside_work_tree(void)\n \n void setup_work_tree(void)\n {\n-\tconst char *work_tree, *git_dir;\n+\tconst char *work_tree;\n \tstatic int initialized = 0;\n \n \tif (initialized)\n@@ -388,10 +389,7 @@ void setup_work_tree(void)\n \t\tdie(_(\"unable to set up work tree using invalid config\"));\n \n \twork_tree = get_git_work_tree();\n-\tgit_dir = get_git_dir();\n-\tif (!is_absolute_path(git_dir))\n-\t\tgit_dir = real_path(get_git_dir());\n-\tif (!work_tree || chdir(work_tree))\n+\tif (!work_tree || chdir_notify(work_tree))\n \t\tdie(_(\"this operation must be run in a work tree\"));\n \n \t/*\n@@ -401,7 +399,6 @@ void setup_work_tree(void)\n \tif (getenv(GIT_WORK_TREE_ENVIRONMENT))\n \t\tsetenv(GIT_WORK_TREE_ENVIRONMENT, \".\", 1);\n \n-\tset_git_dir(remove_leading_path(git_dir, work_tree));\n \tinitialized = 1;\n }\n \n-- \n2.17.0.rc2.594.gdb94a0ce02\n\n"},{"id":"343452","messageId":"20180330183512.GE31135@sigill.intra.peff.net","threadId":"48158","inReplyTo":"20180330183425.GA30575@sigill.intra.peff.net","subject":"[PATCH v2 5/5] refs: use chdir_notify to update cached relative paths","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-30T18:35:12Z","receivedAt":"2018-03-30T18:35:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit f57f37e2e1 (files-backend: remove the use of\ngit_path(), 2017-03-26) introduced a regression when a\nrelative $GIT_DIR is used in a working tree:\n\n - when we initialize the ref backend, we make a copy of\n get_git_dir(), which may be relative\n\n - later, we may call setup_work_tree() and chdir to the\n root of the working tree\n\n - further calls to the ref code will use the stored git\n directory, but relative paths will now point to the\n wrong place\n\nThe new test in t1501 demonstrates one such instance (the\nbug causes us to write the ref update to the nonsense\n\"relative/relative/.git\").\n\nSince setup_work_tree() now uses chdir_notify, we can just\nask it update our relative paths when necessary.\n\nReported-by: Rafael Ascensao \nHelped-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: Jeff King \n---\n refs/files-backend.c | 6 ++++++\n refs/packed-backend.c | 3 +++\n t/t1501-work-tree.sh | 12 ++++++++++++\n 3 files changed, 21 insertions(+)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex bec8e30e9e..a92a2aa821 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -9,6 +9,7 @@\n #include \"../lockfile.h\"\n #include \"../object.h\"\n #include \"../dir.h\"\n+#include \"../chdir-notify.h\"\n \n /*\n * This backend uses the following flags in `ref_update::flags` for\n@@ -106,6 +107,11 @@ static struct ref_store *files_ref_store_create(const char *gitdir,\n \trefs->packed_ref_store = packed_ref_store_create(sb.buf, flags);\n \tstrbuf_release(&sb);\n \n+\tchdir_notify_reparent(\"files-backend $GIT_DIR\",\n+\t\t\t &refs->gitdir);\n+\tchdir_notify_reparent(\"files-backend $GIT_COMMONDIR\",\n+\t\t\t &refs->gitcommondir);\n+\n \treturn ref_store;\n }\n \ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 65288c6472..369c34f886 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -5,6 +5,7 @@\n #include \"packed-backend.h\"\n #include \"../iterator.h\"\n #include \"../lockfile.h\"\n+#include \"../chdir-notify.h\"\n \n enum mmap_strategy {\n \t/*\n@@ -202,6 +203,8 @@ struct ref_store *packed_ref_store_create(const char *path,\n \trefs->store_flags = store_flags;\n \n \trefs->path = xstrdup(path);\n+\tchdir_notify_reparent(\"packed-refs\", &refs->path);\n+\n \treturn ref_store;\n }\n \ndiff --git a/t/t1501-work-tree.sh b/t/t1501-work-tree.sh\nindex b06210ec5e..a5db53337b 100755\n--- a/t/t1501-work-tree.sh\n+++ b/t/t1501-work-tree.sh\n@@ -431,4 +431,16 @@ test_expect_success 'error out gracefully on invalid $GIT_WORK_TREE' '\n \t)\n '\n \n+test_expect_success 'refs work with relative gitdir and work tree' '\n+\tgit init relative &&\n+\tgit -C relative commit --allow-empty -m one &&\n+\tgit -C relative commit --allow-empty -m two &&\n+\n+\tGIT_DIR=relative/.git GIT_WORK_TREE=relative git reset HEAD^ &&\n+\n+\tgit -C relative log -1 --format=%s >actual &&\n+\techo one >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.17.0.rc2.594.gdb94a0ce02\n"},{"id":"343468","messageId":"20180330193637.GA1620@duynguyen.home","threadId":"48158","inReplyTo":"20180330183425.GA30575@sigill.intra.peff.net","subject":"Re: [PATCH v2 0/5] re-parenting relative directories after chdir","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-03-30T19:36:37Z","receivedAt":"2018-03-30T19:36:47Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Mar 30, 2018 at 02:34:25PM -0400, Jeff King wrote:\n> On Wed, Mar 28, 2018 at 01:36:56PM -0400, Jeff King wrote:\n> \n> > For those just joining us, this fixes a regression that was in v2.13 (so\n> > nothing we need to worry about as part of the 2.17-rc track).\n> > \n> > [1/4]: set_git_dir: die when setenv() fails\n> > [2/4]: add chdir-notify API\n> > [3/4]: set_work_tree: use chdir_notify\n> > [4/4]: refs: use chdir_notify to update cached relative paths\n> \n> Here's a re-roll based on the feedback I got, including:\n> \n> - fixes the memory leak and vague comment pointed out by Eric\n> \n> - adds in tracing code from Duy\n> \n> - I took Duy's suggestions regarding \"least\" surprise in some of the\n> functions (reparenting NULL is a noop, and registering a reparent\n> handler does so even for an absolute path).\n> \n> I punted on the \"registering the same path twice\" thing. That is a\n> potential way to misuse this API, but I don't think there's a good\n> solution. The \"reparent\" helper could figure this out for you, but in\n> the general case we actually install an arbitrary callback. So the\n> caller really has to handle it there.\n\nThe series looks good to me.\n\n> \n> I think in the long run we'd want to outlaw calling set_git_dir() twice\n> anyway.\n\nOh yeah. With my latest WIP changes, the bottom of\nsetup_git_directory_gently() looks like this. Nowhere else in setup\ncode calls these functions anymore (except the current\nsetup_work_tree)\n\n-- 8< --\n\tif (result.worktree)\n\t\tset_git_work_tree(result.worktree);\n\tif (result.gitdir)\n\t\tset_git_dir(result.gitdir);\n\tif (startup_info->have_repository)\n\t\trepo_set_hash_algo(the_repository, result.repo_fmt.hash_algo);\n\t...\n\treturn result.prefix;\n-- 8< --\n\nFrom here on, it's not hard to see how to turn set_git_work_tree()\ninto setup_work_tree_gently() (without doing any set_git_dir) and the\nlast two calls into \"repo_init_gitdir(gitdir, hash_algo)\", which\nshould be where we allocate a new repository object and initialize\nrelated object store, ref store...\n\nBut I still have a couple setup corner cases to deal with first :(\n--\nDuy\n"},{"id":"343469","messageId":"xmqq8ta92vot.fsf@gitster-ct.c.googlers.com","threadId":"48158","inReplyTo":"20180330183459.GB31135@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/5] trace.c: export trace_setup_key","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-30T19:46:26Z","receivedAt":"2018-03-30T19:46:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> From: Nguyễn Thái Ngọc Duy \n>\n> This is so that we can print traces based on this key outside trace.c.\n\n\"this key\" meaning...? GIT_TRACE_SETUP?\n\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> Signed-off-by: Jeff King \n> ---\n> trace.c | 14 +++++++-------\n> trace.h | 1 +\n> 2 files changed, 8 insertions(+), 7 deletions(-)\n>\n> diff --git a/trace.c b/trace.c\n> index 7f3b08e148..fc623e91fd 100644\n> --- a/trace.c\n> +++ b/trace.c\n> @@ -26,6 +26,7 @@\n> \n> struct trace_key trace_default_key = { \"GIT_TRACE\", 0, 0, 0 };\n> struct trace_key trace_perf_key = TRACE_KEY_INIT(PERFORMANCE);\n> +struct trace_key trace_setup_key = TRACE_KEY_INIT(SETUP);\n> \n> /* Get a trace file descriptor from \"key\" env variable. */\n> static int get_trace_fd(struct trace_key *key)\n> @@ -300,11 +301,10 @@ static const char *quote_crnl(const char *path)\n> /* FIXME: move prefix to startup_info struct and get rid of this arg */\n> void trace_repo_setup(const char *prefix)\n> {\n> -\tstatic struct trace_key key = TRACE_KEY_INIT(SETUP);\n> \tconst char *git_work_tree;\n> \tchar *cwd;\n> \n> -\tif (!trace_want(&key))\n> +\tif (!trace_want(&trace_setup_key))\n> \t\treturn;\n> \n> \tcwd = xgetcwd();\n> @@ -315,11 +315,11 @@ void trace_repo_setup(const char *prefix)\n> \tif (!prefix)\n> \t\tprefix = \"(null)\";\n> \n> -\ttrace_printf_key(&key, \"setup: git_dir: %s\\n\", quote_crnl(get_git_dir()));\n> -\ttrace_printf_key(&key, \"setup: git_common_dir: %s\\n\", quote_crnl(get_git_common_dir()));\n> -\ttrace_printf_key(&key, \"setup: worktree: %s\\n\", quote_crnl(git_work_tree));\n> -\ttrace_printf_key(&key, \"setup: cwd: %s\\n\", quote_crnl(cwd));\n> -\ttrace_printf_key(&key, \"setup: prefix: %s\\n\", quote_crnl(prefix));\n> +\ttrace_printf_key(&trace_setup_key, \"setup: git_dir: %s\\n\", quote_crnl(get_git_dir()));\n> +\ttrace_printf_key(&trace_setup_key, \"setup: git_common_dir: %s\\n\", quote_crnl(get_git_common_dir()));\n> +\ttrace_printf_key(&trace_setup_key, \"setup: worktree: %s\\n\", quote_crnl(git_work_tree));\n> +\ttrace_printf_key(&trace_setup_key, \"setup: cwd: %s\\n\", quote_crnl(cwd));\n> +\ttrace_printf_key(&trace_setup_key, \"setup: prefix: %s\\n\", quote_crnl(prefix));\n> \n> \tfree(cwd);\n> }\n> diff --git a/trace.h b/trace.h\n> index 88055abef7..2b6a1bc17c 100644\n> --- a/trace.h\n> +++ b/trace.h\n> @@ -15,6 +15,7 @@ extern struct trace_key trace_default_key;\n> \n> #define TRACE_KEY_INIT(name) { \"GIT_TRACE_\" #name, 0, 0, 0 }\n> extern struct trace_key trace_perf_key;\n> +extern struct trace_key trace_setup_key;\n> \n> extern void trace_repo_setup(const char *prefix);\n> extern int trace_want(struct trace_key *key);\n"},{"id":"343470","messageId":"20180330194749.GA13537@sigill.intra.peff.net","threadId":"48158","inReplyTo":"xmqq8ta92vot.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 2/5] trace.c: export trace_setup_key","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-30T19:47:49Z","receivedAt":"2018-03-30T19:47:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 30, 2018 at 12:46:26PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > From: Nguyễn Thái Ngọc Duy \n> >\n> > This is so that we can print traces based on this key outside trace.c.\n> \n> \"this key\" meaning...? GIT_TRACE_SETUP?\n\nI think \"based on trace_setup_key\".\n\n-Peff\n"},{"id":"343471","messageId":"xmqq4lkx2vhh.fsf@gitster-ct.c.googlers.com","threadId":"48158","inReplyTo":"20180330194749.GA13537@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/5] trace.c: export trace_setup_key","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-30T19:50:50Z","receivedAt":"2018-03-30T19:50:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Mar 30, 2018 at 12:46:26PM -0700, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>> \n>> > From: Nguyễn Thái Ngọc Duy \n>> >\n>> > This is so that we can print traces based on this key outside trace.c.\n>> \n>> \"this key\" meaning...? GIT_TRACE_SETUP?\n>\n> I think \"based on trace_setup_key\".\n>\n> -Peff\n\nYeah, I read, but did not pay enough attention to, the subject X-<.\n"},{"id":"343473","messageId":"20180330195436.GA14217@sigill.intra.peff.net","threadId":"48158","inReplyTo":"xmqq4lkx2vhh.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 2/5] trace.c: export trace_setup_key","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-30T19:54:36Z","receivedAt":"2018-03-30T19:54:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 30, 2018 at 12:50:50PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > On Fri, Mar 30, 2018 at 12:46:26PM -0700, Junio C Hamano wrote:\n> >\n> >> Jeff King writes:\n> >> \n> >> > From: Nguyễn Thái Ngọc Duy \n> >> >\n> >> > This is so that we can print traces based on this key outside trace.c.\n> >> \n> >> \"this key\" meaning...? GIT_TRACE_SETUP?\n> >\n> > I think \"based on trace_setup_key\".\n> >\n> > -Peff\n> \n> Yeah, I read, but did not pay enough attention to, the subject X-<.\n\nTo be fair, one of our guidelines is that the commit message should not\noverly rely on the subject line. Though I am certainly guilty of\nstarting many a message with \"This\".\n\nPerhaps:\n\n The setup-tracing code is static-local to trace.c. In preparation for\n new GIT_TRACE_SETUP code outside of trace.c, let's make the trace_key\n globally available.\n\nwould be a better commit message.\n\n-Peff\n"}]}