{"thread":{"id":"47802","subject":"[RFC PATCH 3/3] fsck: Check HEAD of other worktrees as well","startedAt":"2018-02-09T23:14:20Z","lastAt":"2018-02-10T20:11:27Z","messageCount":7,"participants":["Elijah Newren","Duy Nguyen","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"338957","messageId":"20180209231330.4457-4-newren@gmail.com","threadId":"47802","inReplyTo":"20180209231330.4457-1-newren@gmail.com","subject":"[RFC PATCH 3/3] fsck: Check HEAD of other worktrees as well","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-02-09T23:13:30Z","receivedAt":"2018-02-09T23:14:20Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"This takes advantage of the fact that \"worktrees/$WORKTREE/HEAD\" will\ncurrently resolve as a ref, which may not be true in the future with\ndifferent ref backends.  I don't think it locks us in to supporting\nresolving other worktree HEADs with this syntax, as I view the\nuser-visible error message as more of a pointer to a pathname that the\nuser will need to fix.  If the underlying ref storage changes, naturally\nboth this code and the hint may need to change to match.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n builtin/fsck.c  | 60 +++++++++++++++++++++++++++++++++++++++++++++------------\n t/t1450-fsck.sh |  6 +++---\n 2 files changed, 51 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 4132034170..850b217e93 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -451,17 +451,51 @@ static int fsck_handle_ref(const char *refname, const struct object_id *oid,\n \treturn 0;\n }\n \n-static int fsck_head_link(const char **head_points_at,\n+static void get_worktree_names(struct string_list *names)\n+{\n+\tstruct strbuf path = STRBUF_INIT;\n+\tDIR *dir;\n+\tstruct dirent *d;\n+\n+\tstrbuf_addf(&path, \"%s/worktrees\", get_git_common_dir());\n+\tdir = opendir(path.buf);\n+\tstrbuf_release(&path);\n+\tif (dir) {\n+\t\twhile ((d = readdir(dir)) != NULL) {\n+\t\t\tif (!is_dot_or_dotdot(d->d_name))\n+\t\t\t\tstring_list_append(names, d->d_name);\n+\t\t}\n+\t\tclosedir(dir);\n+\t}\n+}\n+\n+static int fsck_head_link(const char *head_ref_name,\n+\t\t\t  const char **head_points_at,\n \t\t\t  struct object_id *head_oid);\n \n static void get_default_heads(void)\n {\n \tconst char *head_points_at;\n \tstruct object_id head_oid;\n+\tstruct string_list worktree_names = STRING_LIST_INIT_DUP;\n+\tstruct string_list_item *worktree_item;\n+\tstruct strbuf head_name = STRBUF_INIT;\n \n-\tfsck_head_link(&head_points_at, &head_oid);\n+\tfsck_head_link(\"HEAD\", &head_points_at, &head_oid);\n \tif (head_points_at && !is_null_oid(&head_oid))\n \t\tfsck_handle_ref(\"HEAD\", &head_oid, 0, NULL);\n+\n+\tget_worktree_names(&worktree_names);\n+\tfor_each_string_list_item(worktree_item, &worktree_names) {\n+\t\tstrbuf_reset(&head_name);\n+\t\tstrbuf_addf(&head_name, \"worktrees/%s/HEAD\",\n+\t\t\t    worktree_item->string);\n+\t\tfsck_head_link(head_name.buf, &head_points_at, &head_oid);\n+\t\tif (head_points_at && !is_null_oid(&head_oid))\n+\t\t\tfsck_handle_ref(head_name.buf, &head_oid, 0, NULL);\n+\t}\n+\tstrbuf_release(&head_name);\n+\n \tfor_each_rawref(fsck_handle_ref, NULL);\n \tif (include_reflogs)\n \t\tfor_each_reflog(fsck_handle_reflog, NULL);\n@@ -553,34 +587,36 @@ static void fsck_object_dir(const char *path)\n \tstop_progress(&progress);\n }\n \n-static int fsck_head_link(const char **head_points_at,\n+static int fsck_head_link(const char *head_ref_name,\n+\t\t\t  const char **head_points_at,\n \t\t\t  struct object_id *head_oid)\n {\n \tint null_is_error = 0;\n \n \tif (verbose)\n-\t\tfprintf(stderr, \"Checking HEAD link\\n\");\n+\t\tfprintf(stderr, \"Checking %s link\\n\", head_ref_name);\n \n-\t*head_points_at = resolve_ref_unsafe(\"HEAD\", 0, head_oid, NULL);\n+\t*head_points_at = resolve_ref_unsafe(head_ref_name, 0, head_oid, NULL);\n \tif (!*head_points_at) {\n \t\terrors_found |= ERROR_REFS;\n-\t\treturn error(\"Invalid HEAD\");\n+\t\treturn error(\"Invalid %s\", head_ref_name);\n \t}\n-\tif (!strcmp(*head_points_at, \"HEAD\"))\n+\tif (!strcmp(*head_points_at, head_ref_name))\n \t\t/* detached HEAD */\n \t\tnull_is_error = 1;\n \telse if (!starts_with(*head_points_at, \"refs/heads/\")) {\n \t\terrors_found |= ERROR_REFS;\n-\t\treturn error(\"HEAD points to something strange (%s)\",\n-\t\t\t     *head_points_at);\n+\t\treturn error(\"%s points to something strange (%s)\",\n+\t\t\t     head_ref_name, *head_points_at);\n \t}\n \tif (is_null_oid(head_oid)) {\n \t\tif (null_is_error) {\n \t\t\terrors_found |= ERROR_REFS;\n-\t\t\treturn error(\"HEAD: detached HEAD points at nothing\");\n+\t\t\treturn error(\"%s: detached HEAD points at nothing\",\n+\t\t\t\t     head_ref_name);\n \t\t}\n-\t\tfprintf(stderr, \"notice: HEAD points to an unborn branch (%s)\\n\",\n-\t\t\t*head_points_at + 11);\n+\t\tfprintf(stderr, \"notice: %s points to an unborn branch (%s)\\n\",\n+\t\t\thead_ref_name, *head_points_at + 11);\n \t}\n \treturn 0;\n }\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex fa94c59458..3da651be4c 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -102,7 +102,7 @@ test_expect_success 'HEAD link pointing at a funny place' '\n \tgrep \"HEAD points to something strange\" out\n '\n \n-test_expect_failure 'other worktree HEAD link pointing at a funny object' '\n+test_expect_success 'other worktree HEAD link pointing at a funny object' '\n \ttest_when_finished \"rm -rf .git/worktrees\" &&\n \tmkdir -p .git/worktrees/other &&\n \techo 0000000000000000000000000000000000000000 >.git/worktrees/other/HEAD &&\n@@ -111,7 +111,7 @@ test_expect_failure 'other worktree HEAD link pointing at a funny object' '\n \tgrep \"worktrees/other/HEAD: detached HEAD points\" out\n '\n \n-test_expect_failure 'other worktree HEAD link pointing at missing object' '\n+test_expect_success 'other worktree HEAD link pointing at missing object' '\n \ttest_when_finished \"rm -rf .git/worktrees\" &&\n \tmkdir -p .git/worktrees/other &&\n \techo \"Contents missing from repo\" | git hash-object --stdin >.git/worktrees/other/HEAD &&\n@@ -120,7 +120,7 @@ test_expect_failure 'other worktree HEAD link pointing at missing object' '\n \tgrep \"worktrees/other/HEAD: invalid sha1 pointer\" out\n '\n \n-test_expect_failure 'other worktree HEAD link pointing at a funny place' '\n+test_expect_success 'other worktree HEAD link pointing at a funny place' '\n \ttest_when_finished \"rm -rf .git/worktrees\" &&\n \tmkdir -p .git/worktrees/other &&\n \techo \"ref: refs/funny/place\" >.git/worktrees/other/HEAD &&\n-- \n2.16.1.75.gc01c8fdd7d\n\n"},{"id":"338958","messageId":"20180209231330.4457-2-newren@gmail.com","threadId":"47802","inReplyTo":"20180209231330.4457-1-newren@gmail.com","subject":"[RFC PATCH 1/3] fsck: Move fsck_head_link() to get_default_heads() to avoid some globals","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-02-09T23:13:28Z","receivedAt":"2018-02-09T23:14:57Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"This will make it easier to check the HEAD of other worktrees from fsck.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n builtin/fsck.c | 27 ++++++++++++++++-----------\n 1 file changed, 16 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 04846d46f9..4132034170 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -33,8 +33,6 @@ static int check_strict;\n static int keep_cache_objects;\n static struct fsck_options fsck_walk_options = FSCK_OPTIONS_DEFAULT;\n static struct fsck_options fsck_obj_options = FSCK_OPTIONS_DEFAULT;\n-static struct object_id head_oid;\n-static const char *head_points_at;\n static int errors_found;\n static int write_lost_and_found;\n static int verbose;\n@@ -453,8 +451,15 @@ static int fsck_handle_ref(const char *refname, const struct object_id *oid,\n \treturn 0;\n }\n \n+static int fsck_head_link(const char **head_points_at,\n+\t\t\t  struct object_id *head_oid);\n+\n static void get_default_heads(void)\n {\n+\tconst char *head_points_at;\n+\tstruct object_id head_oid;\n+\n+\tfsck_head_link(&head_points_at, &head_oid);\n \tif (head_points_at && !is_null_oid(&head_oid))\n \t\tfsck_handle_ref(\"HEAD\", &head_oid, 0, NULL);\n \tfor_each_rawref(fsck_handle_ref, NULL);\n@@ -548,33 +553,34 @@ static void fsck_object_dir(const char *path)\n \tstop_progress(&progress);\n }\n \n-static int fsck_head_link(void)\n+static int fsck_head_link(const char **head_points_at,\n+\t\t\t  struct object_id *head_oid)\n {\n \tint null_is_error = 0;\n \n \tif (verbose)\n \t\tfprintf(stderr, \"Checking HEAD link\\n\");\n \n-\thead_points_at = resolve_ref_unsafe(\"HEAD\", 0, &head_oid, NULL);\n-\tif (!head_points_at) {\n+\t*head_points_at = resolve_ref_unsafe(\"HEAD\", 0, head_oid, NULL);\n+\tif (!*head_points_at) {\n \t\terrors_found |= ERROR_REFS;\n \t\treturn error(\"Invalid HEAD\");\n \t}\n-\tif (!strcmp(head_points_at, \"HEAD\"))\n+\tif (!strcmp(*head_points_at, \"HEAD\"))\n \t\t/* detached HEAD */\n \t\tnull_is_error = 1;\n-\telse if (!starts_with(head_points_at, \"refs/heads/\")) {\n+\telse if (!starts_with(*head_points_at, \"refs/heads/\")) {\n \t\terrors_found |= ERROR_REFS;\n \t\treturn error(\"HEAD points to something strange (%s)\",\n-\t\t\t     head_points_at);\n+\t\t\t     *head_points_at);\n \t}\n-\tif (is_null_oid(&head_oid)) {\n+\tif (is_null_oid(head_oid)) {\n \t\tif (null_is_error) {\n \t\t\terrors_found |= ERROR_REFS;\n \t\t\treturn error(\"HEAD: detached HEAD points at nothing\");\n \t\t}\n \t\tfprintf(stderr, \"notice: HEAD points to an unborn branch (%s)\\n\",\n-\t\t\thead_points_at + 11);\n+\t\t\t*head_points_at + 11);\n \t}\n \treturn 0;\n }\n@@ -686,7 +692,6 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \n \tgit_config(fsck_config, NULL);\n \n-\tfsck_head_link();\n \tif (connectivity_only) {\n \t\tfor_each_loose_object(mark_loose_for_connectivity, NULL, 0);\n \t\tfor_each_packed_object(mark_packed_for_connectivity, NULL, 0);\n-- \n2.16.1.75.gc01c8fdd7d\n\n"},{"id":"338959","messageId":"20180209231330.4457-1-newren@gmail.com","threadId":"47802","inReplyTo":null,"subject":"[RFC PATCH 0/3] Make fsck check other worktree HEADs","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-02-09T23:13:27Z","receivedAt":"2018-02-09T23:15:00Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"This patchset adds checking of other worktree HEADs to fsck.\n\nThe reason I've marked this RFC is that I'm worried my incidental\nreliance on \"worktrees/$WORKTREE/HEAD\" resolving as a ref (in patch 3)\nmight raise some flags for others.  In particular, in [1] Peff said\nthat this refname resolves right now mostly by accident and will\nprobably stop working in the future.  However, I feel that since fsck\nchecks the storage format as well as contents, it seems natural that a\nchange of storage model would result in the fsck code changing and\nthus that I'm not locking in any particular ref format long term with\nthese changes.  But I want to flag this issue for discussion.\n\n[1] https://public-inbox.org/git/20180207181706.GA4227@sigill.intra.peff.net/\n\nElijah Newren (3):\n  fsck: Move fsck_head_link() to get_default_heads() to avoid some\n    globals\n  t1450-fsck: Add tests for HEAD of other worktrees\n  fsck: Check HEAD of other worktrees as well\n\n builtin/fsck.c  | 73 ++++++++++++++++++++++++++++++++++++++++++++-------------\n t/t1450-fsck.sh | 27 +++++++++++++++++++++\n 2 files changed, 84 insertions(+), 16 deletions(-)\n\n-- \n2.16.1.75.gc01c8fdd7d\n\n"},{"id":"338960","messageId":"20180209231330.4457-3-newren@gmail.com","threadId":"47802","inReplyTo":"20180209231330.4457-1-newren@gmail.com","subject":"[RFC PATCH 2/3] t1450-fsck: Add tests for HEAD of other worktrees","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-02-09T23:13:29Z","receivedAt":"2018-02-09T23:15:48Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Signed-off-by: Elijah Newren <newren@gmail.com>\n---\n t/t1450-fsck.sh | 27 +++++++++++++++++++++++++++\n 1 file changed, 27 insertions(+)\n\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex cb4b66e29d..fa94c59458 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -102,6 +102,33 @@ test_expect_success 'HEAD link pointing at a funny place' '\n \tgrep \"HEAD points to something strange\" out\n '\n \n+test_expect_failure 'other worktree HEAD link pointing at a funny object' '\n+\ttest_when_finished \"rm -rf .git/worktrees\" &&\n+\tmkdir -p .git/worktrees/other &&\n+\techo 0000000000000000000000000000000000000000 >.git/worktrees/other/HEAD &&\n+\ttest_must_fail git fsck 2>out &&\n+\tcat out &&\n+\tgrep \"worktrees/other/HEAD: detached HEAD points\" out\n+'\n+\n+test_expect_failure 'other worktree HEAD link pointing at missing object' '\n+\ttest_when_finished \"rm -rf .git/worktrees\" &&\n+\tmkdir -p .git/worktrees/other &&\n+\techo \"Contents missing from repo\" | git hash-object --stdin >.git/worktrees/other/HEAD &&\n+\ttest_must_fail git fsck 2>out &&\n+\tcat out &&\n+\tgrep \"worktrees/other/HEAD: invalid sha1 pointer\" out\n+'\n+\n+test_expect_failure 'other worktree HEAD link pointing at a funny place' '\n+\ttest_when_finished \"rm -rf .git/worktrees\" &&\n+\tmkdir -p .git/worktrees/other &&\n+\techo \"ref: refs/funny/place\" >.git/worktrees/other/HEAD &&\n+\ttest_must_fail git fsck 2>out &&\n+\tcat out &&\n+\tgrep \"worktrees/other/HEAD points to something strange\" out\n+'\n+\n test_expect_success 'email without @ is okay' '\n \tgit cat-file commit HEAD >basis &&\n \tsed \"s/@/AT/\" basis >okay &&\n-- \n2.16.1.75.gc01c8fdd7d\n\n"},{"id":"338975","messageId":"20180210095952.GA9035@ash","threadId":"47802","inReplyTo":"20180209231330.4457-4-newren@gmail.com","subject":"Re: [RFC PATCH 3/3] fsck: Check HEAD of other worktrees as well","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-02-10T09:59:52Z","receivedAt":"2018-02-10T10:00:06Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Feb 09, 2018 at 03:13:30PM -0800, Elijah Newren wrote:\n> This takes advantage of the fact that \"worktrees/$WORKTREE/HEAD\" will\n> currently resolve as a ref, which may not be true in the future with\n> different ref backends.  I don't think it locks us in to supporting\n> resolving other worktree HEADs with this syntax, as I view the\n> user-visible error message as more of a pointer to a pathname that the\n> user will need to fix.  If the underlying ref storage changes, naturally\n> both this code and the hint may need to change to match.\n\nI'm leaning more about something like this patch below (which does not\neven build, just to demonstrate). A couple points:\n\n- Instead of doing the hacky refs worktrees/foo/HEAD, we get the\n  correct ref store for each worktree\n- We have an API for getting all (non-broken) worktrees. I realize for\n  fsck, we may even want to examine semi-broken worktrees as well, but\n  get_worktrees() can take a flag to accomplish that if needed.\n- As you can see, I print %p from the ref store instead of something\n  human friendly. This is something I was stuck at last time. I need\n  better ref store description (or even the worktree name)\n- This ref_name() function makes me think if we should have an\n  extended sha1 syntax for accessing per-worktree refs from a\n  different worktree, something like HEAD@{worktree:foo} to resolve to\n  worktrees/foo/HEAD. Then we have a better description here that can\n  actually be used from command line, as a regular ref, if needed.\n\n-- 8< --\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 4132034170..73cfcbc07a 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -16,6 +16,7 @@\n #include \"streaming.h\"\n #include \"decorate.h\"\n #include \"packfile.h\"\n+#include \"worktree.h\"\n \n #define REACHABLE 0x0001\n #define SEEN      0x0002\n@@ -451,17 +452,39 @@ static int fsck_handle_ref(const char *refname, const struct object_id *oid,\n \treturn 0;\n }\n \n-static int fsck_head_link(const char **head_points_at,\n+static const char *ref_name(struct ref_store *refs, const char *name)\n+{\n+\tstatic struct strbuf sb = STRBUF_INIT;\n+\n+\tif (!refs)\n+\t\treturn name;\n+\tstrbuf_reset(&sb);\n+\tstrbuf_addf(&sb, \"%s (from %p)\", name, refs);\n+\treturn sb.buf;\n+}\n+\n+static int fsck_head_link(struct ref_store *refs,\n+\t\t\t  const char **head_points_at,\n \t\t\t  struct object_id *head_oid);\n \n static void get_default_heads(void)\n {\n \tconst char *head_points_at;\n \tstruct object_id head_oid;\n+\tstruct worktree **worktrees, **wt;\n \n-\tfsck_head_link(&head_points_at, &head_oid);\n+\tfsck_head_link(NULL, &head_points_at, &head_oid);\n \tif (head_points_at && !is_null_oid(&head_oid))\n \t\tfsck_handle_ref(\"HEAD\", &head_oid, 0, NULL);\n+\n+\tworktrees = get_worktrees(0);\n+\tfor (wt = worktrees; *wt; wt++) {\n+\t\tfsck_head_link(get_worktree_ref_store(*wt), &head_points_at, &head_oid);\n+\t\tif (head_points_at && !is_null_oid(&head_oid))\n+\t\t\tfsck_handle_ref(*wt, \"HEAD\", &head_oid, 0, NULL);\n+\t}\n+\tfree_worktrees(wt);\n+\n \tfor_each_rawref(fsck_handle_ref, NULL);\n \tif (include_reflogs)\n \t\tfor_each_reflog(fsck_handle_reflog, NULL);\n@@ -553,34 +576,36 @@ static void fsck_object_dir(const char *path)\n \tstop_progress(&progress);\n }\n \n-static int fsck_head_link(const char **head_points_at,\n+static int fsck_head_link(struct ref_store *refs,\n+\t\t\t  const char **head_points_at,\n \t\t\t  struct object_id *head_oid)\n {\n \tint null_is_error = 0;\n \n \tif (verbose)\n-\t\tfprintf(stderr, \"Checking HEAD link\\n\");\n+\t\tfprintf(stderr, \"Checking %s link\\n\", ref_name(refs, \"HEAD\"));\n \n-\t*head_points_at = resolve_ref_unsafe(\"HEAD\", 0, head_oid, NULL);\n+\t*head_points_at = refs_resolve_ref_unsafe(refs, \"HEAD\", 0, head_oid, NULL);\n \tif (!*head_points_at) {\n \t\terrors_found |= ERROR_REFS;\n-\t\treturn error(\"Invalid HEAD\");\n+\t\treturn error(\"Invalid HEAD from ref-store %p\", refs);\n \t}\n \tif (!strcmp(*head_points_at, \"HEAD\"))\n \t\t/* detached HEAD */\n \t\tnull_is_error = 1;\n \telse if (!starts_with(*head_points_at, \"refs/heads/\")) {\n \t\terrors_found |= ERROR_REFS;\n-\t\treturn error(\"HEAD points to something strange (%s)\",\n-\t\t\t     *head_points_at);\n+\t\treturn error(\"%s points to something strange (%s)\",\n+\t\t\t     ref_name(refs, \"HEAD\"), *head_points_at);\n \t}\n \tif (is_null_oid(head_oid)) {\n \t\tif (null_is_error) {\n \t\t\terrors_found |= ERROR_REFS;\n-\t\t\treturn error(\"HEAD: detached HEAD points at nothing\");\n+\t\t\treturn error(\"%s: detached HEAD points at nothing\",\n+\t\t\t\t     ref_name(refs, \"HEAD\"));\n \t\t}\n-\t\tfprintf(stderr, \"notice: HEAD points to an unborn branch (%s)\\n\",\n-\t\t\t*head_points_at + 11);\n+\t\tfprintf(stderr, \"notice: %s points to an unborn branch (%s)\\n\",\n+\t\t\tref_name(refs, \"HEAD\"), *head_points_at + 11);\n \t}\n \treturn 0;\n }\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex fa94c59458..3da651be4c 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -102,7 +102,7 @@ test_expect_success 'HEAD link pointing at a funny place' '\n \tgrep \"HEAD points to something strange\" out\n '\n \n-test_expect_failure 'other worktree HEAD link pointing at a funny object' '\n+test_expect_success 'other worktree HEAD link pointing at a funny object' '\n \ttest_when_finished \"rm -rf .git/worktrees\" &&\n \tmkdir -p .git/worktrees/other &&\n \techo 0000000000000000000000000000000000000000 >.git/worktrees/other/HEAD &&\n@@ -111,7 +111,7 @@ test_expect_failure 'other worktree HEAD link pointing at a funny object' '\n \tgrep \"worktrees/other/HEAD: detached HEAD points\" out\n '\n \n-test_expect_failure 'other worktree HEAD link pointing at missing object' '\n+test_expect_success 'other worktree HEAD link pointing at missing object' '\n \ttest_when_finished \"rm -rf .git/worktrees\" &&\n \tmkdir -p .git/worktrees/other &&\n \techo \"Contents missing from repo\" | git hash-object --stdin >.git/worktrees/other/HEAD &&\n@@ -120,7 +120,7 @@ test_expect_failure 'other worktree HEAD link pointing at missing object' '\n \tgrep \"worktrees/other/HEAD: invalid sha1 pointer\" out\n '\n \n-test_expect_failure 'other worktree HEAD link pointing at a funny place' '\n+test_expect_success 'other worktree HEAD link pointing at a funny place' '\n \ttest_when_finished \"rm -rf .git/worktrees\" &&\n \tmkdir -p .git/worktrees/other &&\n \techo \"ref: refs/funny/place\" >.git/worktrees/other/HEAD &&\n\n\n-- 8< --\n"},{"id":"338986","messageId":"20180210123424.GC21843@sigill.intra.peff.net","threadId":"47802","inReplyTo":"20180210095952.GA9035@ash","subject":"Re: [RFC PATCH 3/3] fsck: Check HEAD of other worktrees as well","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-10T12:34:24Z","receivedAt":"2018-02-10T12:34:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 10, 2018 at 04:59:52PM +0700, Duy Nguyen wrote:\n\n> On Fri, Feb 09, 2018 at 03:13:30PM -0800, Elijah Newren wrote:\n> > This takes advantage of the fact that \"worktrees/$WORKTREE/HEAD\" will\n> > currently resolve as a ref, which may not be true in the future with\n> > different ref backends.  I don't think it locks us in to supporting\n> > resolving other worktree HEADs with this syntax, as I view the\n> > user-visible error message as more of a pointer to a pathname that the\n> > user will need to fix.  If the underlying ref storage changes, naturally\n> > both this code and the hint may need to change to match.\n> \n> I'm leaning more about something like this patch below (which does not\n> even build, just to demonstrate). A couple points:\n> \n> - Instead of doing the hacky refs worktrees/foo/HEAD, we get the\n>   correct ref store for each worktree\n> - We have an API for getting all (non-broken) worktrees. I realize for\n>   fsck, we may even want to examine semi-broken worktrees as well, but\n>   get_worktrees() can take a flag to accomplish that if needed.\n> - As you can see, I print %p from the ref store instead of something\n>   human friendly. This is something I was stuck at last time. I need\n>   better ref store description (or even the worktree name)\n\nYeah, I think this is the right approach. We know about worktrees\ninternally, and we should be able to iterate over their ref stores, even\nif we have no way to succinctly name the resulting ref.\n\n> - This ref_name() function makes me think if we should have an\n>   extended sha1 syntax for accessing per-worktree refs from a\n>   different worktree, something like HEAD@{worktree:foo} to resolve to\n>   worktrees/foo/HEAD. Then we have a better description here that can\n>   actually be used from command line, as a regular ref, if needed.\n\nYeah, I agree this would be very useful. I peeked at how bad it would be\nto hanlde this. The parsing is pretty easy in get_oid_basic(), but you'd\nhave to plumb through the ref_store in quite a few places:\n\n - interpret_nth_prior_checkout() would probably want to use the\n   worktree's HEAD (for \"@{worktree:foo}@{-1}\")\n\n - dwim_ref() would need to know about the ref store\n\n - that implies that substitute_branch_name() knows about it, too\n\nSo it may turn into a big job. But I think it's moving in the right\ndirection. And it may not be the end of the world if all features do not\nwork from day one (e.g., if HEAD@{worktree:foo} works, but\nHEAD@{worktree:foo}@{upstream} does not yet, with plans to eventually\nmake that work).\n\n-Peff\n"},{"id":"338994","messageId":"xmqqmv0gtxjs.fsf@gitster-ct.c.googlers.com","threadId":"47802","inReplyTo":"20180210095952.GA9035@ash","subject":"Re: [RFC PATCH 3/3] fsck: Check HEAD of other worktrees as well","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-10T20:11:19Z","receivedAt":"2018-02-10T20:11:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> On Fri, Feb 09, 2018 at 03:13:30PM -0800, Elijah Newren wrote:\n>> This takes advantage of the fact that \"worktrees/$WORKTREE/HEAD\" will\n>> currently resolve as a ref, which may not be true in the future with\n>> different ref backends.  I don't think it locks us in to supporting\n>> resolving other worktree HEADs with this syntax, as I view the\n>> user-visible error message as more of a pointer to a pathname that the\n>> user will need to fix.  If the underlying ref storage changes, naturally\n>> both this code and the hint may need to change to match.\n>\n> I'm leaning more about something like this patch below (which does not\n> even build, just to demonstrate). A couple points:\n>\n> - Instead of doing the hacky refs worktrees/foo/HEAD, we get the\n>   correct ref store for each worktree\n> - We have an API for getting all (non-broken) worktrees. I realize for\n>   fsck, we may even want to examine semi-broken worktrees as well, but\n>   get_worktrees() can take a flag to accomplish that if needed.\n\nVery sensible.  \n\nWhen I saw that \"fsck\" thing, the first thing I wondered was \"wait,\nhow are we doing prune and repack while making sure objects\nreachable only from HEAD in other worktrees are not lost?  we must\nalready have an API that gives us where the refs of them are stored\nin\".\n\nThanks for a quick response for course correction.\n"}]}