{"thread":{"id":"59266","subject":"Bug: fsck and repack don't agree when a worktree index extension is \"broken\"","startedAt":"2023-02-18T09:38:41Z","lastAt":"2023-06-29T19:37:59Z","messageCount":19,"participants":["Johannes Sixt","Jeff King","Junio C Hamano","Derrick Stolee","Eric Sunshine","Andreas Schwab"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"472299","messageId":"c6246ed5-bffc-7af9-1540-4e2071eff5dc@kdbg.org","threadId":"59266","inReplyTo":null,"subject":"Bug: fsck and repack don't agree when a worktree index extension is \"broken\"","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2023-02-18T09:38:33Z","receivedAt":"2023-02-18T09:38:41Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"I came into a situation where a worktree index contains an invalid\nobject ID in an extension. This causes git gc to abort half-way:\n\n$ git gc\nEnumerating objects: 6, done.\nCounting objects: 100% (6/6), done.\nfatal: unable to read d3e1a3edd7d7851bbf811064090e03475d62fd44\nfatal: failed to run repack\n\nHowever, fsck does not find any problem:\n\n$ git fsck\nChecking object directories: 100% (256/256), done.\n\nThe problem is an invalid object ID that occurs in a worktree index. If\nI copy the index to the main worktree, fsck does find the culprit:\n\n$ cp .git/worktrees/wt/index .git/index\n$ git fsck\nChecking object directories: 100% (256/256), done.\nerror: d3e1a3edd7d7851bbf811064090e03475d62fd44: invalid sha1 pointer in\nresolve-undo\nerror: 4b40bf1072d6dfeebc09b11ee4d4f22ca2ce3109: invalid sha1 pointer in\nresolve-undo\nerror: 5a494fd3a2182795e0723300ab1ac75c0797be5b: invalid sha1 pointer in\nresolve-undo\n\nand git gc fails in the same way as before (of course).\n\nI see three problems here:\n\n- git fsck should detect the problem (if it really is one) in the\nworktree index. It seems that it is just an index extension that is\naffected. Perhaps it should be just a warning, not an error.\n\n- If the objects mentioned in the index extension are precious, they\nshould not have been garbage-collected in earlier rounds of git gc\n(which I certainly did at some point).\n\n- I can't git gc the repository now, which is particularly annoying when\nauto-gc is attempted after almost every git command. Of course, I know\nhow to get out of the situation, but it took some time to identify the\nworktree index as the culprit. Not something that a beginner would be\nable to do easily.\n\nThe repository I use for the above commands is attached. I hope vger\ndoesn't strip it away.\n\n-- Hannes"},{"id":"472641","messageId":"Y/hv0MXAyBY3HEo9@coredump.intra.peff.net","threadId":"59266","inReplyTo":"c6246ed5-bffc-7af9-1540-4e2071eff5dc@kdbg.org","subject":"[PATCH 0/3] fsck index files from all worktrees","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-02-24T08:05:36Z","receivedAt":"2023-02-24T08:05:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 18, 2023 at 10:38:33AM +0100, Johannes Sixt wrote:\n\n> I see three problems here:\n> \n> - git fsck should detect the problem (if it really is one) in the\n> worktree index. It seems that it is just an index extension that is\n> affected. Perhaps it should be just a warning, not an error.\n\nWe do fsck the resolve-undo extension, but I think fsck just doesn't\nknow anything about worktrees. That should be easy enough to fix.\nPatches below.\n\n> - If the objects mentioned in the index extension are precious, they\n> should not have been garbage-collected in earlier rounds of git gc\n> (which I certainly did at some point).\n\nCorrect, but the gc error you're getting indicates that we _are_ trying\nto treat them as included. I wonder if you ran git-gc long ago with an\nolder version of Git, and this breakage was waiting to surface. AFAICT\nthis was all fixed by 8a044c7f1d (Merge branch 'nd/prune-in-worktree',\n2017-09-19).\n\n> - I can't git gc the repository now, which is particularly annoying when\n> auto-gc is attempted after almost every git command. Of course, I know\n> how to get out of the situation, but it took some time to identify the\n> worktree index as the culprit. Not something that a beginner would be\n> able to do easily.\n\nI think in general that \"oops, there's something corrupt\" can be hard to\nget out of, just because there are so many possibilities. But if we can\nat least report the nature of the problem and the offending filename via\ngit-fsck, that would help with pointing people in the right direction.\n\n> The repository I use for the above commands is attached. I hope vger\n> doesn't strip it away.\n\nThanks, it was nice to have a test case. I ended up writing a separate\ntest with a missing blob, just because that's simpler to do. It looks\nlike we don't test fsck_resolve_undo() or fsck_cache_tree() at all. That\nmight be a nice addition, but I punted for now to stay focused on the\nworktree aspects.\n\n  [1/3]: fsck: factor out index fsck\n  [2/3]: fsck: check index files in all worktrees\n  [3/3]: fsck: mention file path for index errors\n\n builtin/fsck.c  | 93 ++++++++++++++++++++++++++++++++-----------------\n t/t1450-fsck.sh | 30 ++++++++++++++++\n 2 files changed, 92 insertions(+), 31 deletions(-)\n\n-Peff\n"},{"id":"472642","messageId":"Y/hwRCwaH/pglVVI@coredump.intra.peff.net","threadId":"59266","inReplyTo":"Y/hv0MXAyBY3HEo9@coredump.intra.peff.net","subject":"[PATCH 1/3] fsck: factor out index fsck","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-02-24T08:07:32Z","receivedAt":"2023-02-24T08:07:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The code to fsck an index operates directly on the_index. Let's move it\ninto its own function in preparation for handling the index files from\nother worktrees.\n\nSince we now have only a single reference to the_index, let's drop\nour USE_THE_INDEX_VARIABLE definition and just use the_repository.index\ndirectly. That's a minor cleanup, but also ensures that we didn't miss\nany references when moving the code into fsck_index().\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/fsck.c | 54 ++++++++++++++++++++++++++++----------------------\n 1 file changed, 30 insertions(+), 24 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex d207bd909b..fa101e0db2 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -1,4 +1,3 @@\n-#define USE_THE_INDEX_VARIABLE\n #include \"builtin.h\"\n #include \"cache.h\"\n #include \"repository.h\"\n@@ -796,6 +795,35 @@ static int fsck_resolve_undo(struct index_state *istate)\n \treturn 0;\n }\n \n+static void fsck_index(struct index_state *istate)\n+{\n+\tunsigned int i;\n+\n+\t/* TODO: audit for interaction with sparse-index. */\n+\tensure_full_index(istate);\n+\tfor (i = 0; i < istate->cache_nr; i++) {\n+\t\tunsigned int mode;\n+\t\tstruct blob *blob;\n+\t\tstruct object *obj;\n+\n+\t\tmode = istate->cache[i]->ce_mode;\n+\t\tif (S_ISGITLINK(mode))\n+\t\t\tcontinue;\n+\t\tblob = lookup_blob(the_repository,\n+\t\t\t\t   &istate->cache[i]->oid);\n+\t\tif (!blob)\n+\t\t\tcontinue;\n+\t\tobj = &blob->object;\n+\t\tobj->flags |= USED;\n+\t\tfsck_put_object_name(&fsck_walk_options, &obj->oid,\n+\t\t\t\t     \":%s\", istate->cache[i]->name);\n+\t\tmark_object_reachable(obj);\n+\t}\n+\tif (istate->cache_tree)\n+\t\tfsck_cache_tree(istate->cache_tree);\n+\tfsck_resolve_undo(istate);\n+}\n+\n static void mark_object_for_connectivity(const struct object_id *oid)\n {\n \tstruct object *obj = lookup_unknown_object(the_repository, oid);\n@@ -959,29 +987,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t\tverify_index_checksum = 1;\n \t\tverify_ce_order = 1;\n \t\trepo_read_index(the_repository);\n-\t\t/* TODO: audit for interaction with sparse-index. */\n-\t\tensure_full_index(&the_index);\n-\t\tfor (i = 0; i < the_index.cache_nr; i++) {\n-\t\t\tunsigned int mode;\n-\t\t\tstruct blob *blob;\n-\t\t\tstruct object *obj;\n-\n-\t\t\tmode = the_index.cache[i]->ce_mode;\n-\t\t\tif (S_ISGITLINK(mode))\n-\t\t\t\tcontinue;\n-\t\t\tblob = lookup_blob(the_repository,\n-\t\t\t\t\t   &the_index.cache[i]->oid);\n-\t\t\tif (!blob)\n-\t\t\t\tcontinue;\n-\t\t\tobj = &blob->object;\n-\t\t\tobj->flags |= USED;\n-\t\t\tfsck_put_object_name(&fsck_walk_options, &obj->oid,\n-\t\t\t\t\t     \":%s\", the_index.cache[i]->name);\n-\t\t\tmark_object_reachable(obj);\n-\t\t}\n-\t\tif (the_index.cache_tree)\n-\t\t\tfsck_cache_tree(the_index.cache_tree);\n-\t\tfsck_resolve_undo(&the_index);\n+\t\tfsck_index(the_repository->index);\n \t}\n \n \tcheck_connectivity();\n-- \n2.39.2.981.g6157336f25\n\n"},{"id":"472643","messageId":"Y/hw1YVgCYWX2yNK@coredump.intra.peff.net","threadId":"59266","inReplyTo":"Y/hv0MXAyBY3HEo9@coredump.intra.peff.net","subject":"[PATCH 2/3] fsck: check index files in all worktrees","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-02-24T08:09:57Z","receivedAt":"2023-02-24T08:10:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We check the index file for the main worktree, but completely ignore the\nindex files in other worktrees. These should be checked, too, as they\nare part of the repository state (and in particular, errors in those\nindex files may cause repo-wide operations like \"git gc\" to complain).\n\nReported-by: Johannes Sixt <j6t@kdbg.org>\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI mostly cargo-culted the loop from add_index_objects_to_pending(),\nwhich is how they get included in git-prune, git-repack, etc.\n\nI'm not sure if we should be reporting something if we get a negative\nreturn. Last time I dug into this, I think I found that the\nindex-reading code could not actually return an error (it dies instead).\n\nIf we get zero, there are no entries to check. But I'm not sure if we'd\nstill have extensions? So maybe this ought to be ignoring the return\nvalue from read_index_from() entirely.\n\n builtin/fsck.c  | 17 +++++++++++++++--\n t/t1450-fsck.sh | 12 ++++++++++++\n 2 files changed, 27 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex fa101e0db2..ddd13cb2b3 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -984,10 +984,23 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (keep_cache_objects) {\n+\t\tstruct worktree **p;\n+\n \t\tverify_index_checksum = 1;\n \t\tverify_ce_order = 1;\n-\t\trepo_read_index(the_repository);\n-\t\tfsck_index(the_repository->index);\n+\n+\t\tfor (p = get_worktrees(); *p; p++) {\n+\t\t\tstruct worktree *wt = *p;\n+\t\t\tstruct index_state istate =\n+\t\t\t\tINDEX_STATE_INIT(the_repository);\n+\n+\t\t\tif (read_index_from(&istate,\n+\t\t\t\t\t    worktree_git_path(wt, \"index\"),\n+\t\t\t\t\t    get_worktree_git_dir(wt)) > 0)\n+\t\t\t\tfsck_index(&istate);\n+\t\t\tdiscard_index(&istate);\n+\t\t}\n+\n \t}\n \n \tcheck_connectivity();\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex fdb886dfe4..3b70ad9e22 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -1023,4 +1023,16 @@ test_expect_success 'fsck error on gitattributes with excessive size' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'fsck detects problems in worktree index' '\n+\ttest_when_finished \"git worktree remove -f wt\" &&\n+\tgit worktree add wt &&\n+\n+\techo \"this will be removed to break the worktree index\" >wt/file &&\n+\tgit -C wt add file &&\n+\tblob=$(git -C wt rev-parse :file) &&\n+\tremove_object $blob &&\n+\n+\ttest_must_fail git fsck\n+'\n+\n test_done\n-- \n2.39.2.981.g6157336f25\n\n"},{"id":"472644","messageId":"Y/hxW9i9GyKblNV4@coredump.intra.peff.net","threadId":"59266","inReplyTo":"Y/hv0MXAyBY3HEo9@coredump.intra.peff.net","subject":"[PATCH 3/3] fsck: mention file path for index errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-02-24T08:12:11Z","receivedAt":"2023-02-24T08:12:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If we encounter an error in an index file, we may say something like:\n\n  error: 1234abcd: invalid sha1 pointer in resolve-undo\n\nBut if you have multiple worktrees, each with its own index, it can be\nvery helpful to know which file had the problem. So let's pass that path\ndown through the various index-fsck functions and use it where\nappropriate. After this patch you should get something like:\n\n  error: 1234abcd: invalid sha1 pointer in resolve-undo of .git/worktrees/wt/index\n\nThat's a bit verbose, but since the point is that you shouldn't see this\nnormally, we're better to err on the side of more details.\n\nI've also added the index filename to the name used by \"fsck\n--name-objects\", which will show up if we find the object to be missing,\netc. This is bending the rules a little there, as the option claims to\nwrite names that can be fed to rev-parse. But there is no revision\nsyntax to access the index of another worktree, so the best we can do is\nmake up something that a human will probably understand.\n\nI did take care to retain the existing \":file\" syntax for the current\nworktree. So the uglier output should kick in only when it's actually\nnecessary. See the included tests for examples of both forms.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/fsck.c  | 42 +++++++++++++++++++++++++++---------------\n t/t1450-fsck.sh | 20 +++++++++++++++++++-\n 2 files changed, 46 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex ddd13cb2b3..e0974644a1 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -731,19 +731,19 @@ static int fsck_head_link(const char *head_ref_name,\n \treturn 0;\n }\n \n-static int fsck_cache_tree(struct cache_tree *it)\n+static int fsck_cache_tree(struct cache_tree *it, const char *index_path)\n {\n \tint i;\n \tint err = 0;\n \n \tif (verbose)\n-\t\tfprintf_ln(stderr, _(\"Checking cache tree\"));\n+\t\tfprintf_ln(stderr, _(\"Checking cache tree of %s\"), index_path);\n \n \tif (0 <= it->entry_count) {\n \t\tstruct object *obj = parse_object(the_repository, &it->oid);\n \t\tif (!obj) {\n-\t\t\terror(_(\"%s: invalid sha1 pointer in cache-tree\"),\n-\t\t\t      oid_to_hex(&it->oid));\n+\t\t\terror(_(\"%s: invalid sha1 pointer in cache-tree of %s\"),\n+\t\t\t      oid_to_hex(&it->oid), index_path);\n \t\t\terrors_found |= ERROR_REFS;\n \t\t\treturn 1;\n \t\t}\n@@ -754,11 +754,12 @@ static int fsck_cache_tree(struct cache_tree *it)\n \t\t\terr |= objerror(obj, _(\"non-tree in cache-tree\"));\n \t}\n \tfor (i = 0; i < it->subtree_nr; i++)\n-\t\terr |= fsck_cache_tree(it->down[i]->cache_tree);\n+\t\terr |= fsck_cache_tree(it->down[i]->cache_tree, index_path);\n \treturn err;\n }\n \n-static int fsck_resolve_undo(struct index_state *istate)\n+static int fsck_resolve_undo(struct index_state *istate,\n+\t\t\t     const char *index_path)\n {\n \tstruct string_list_item *item;\n \tstruct string_list *resolve_undo = istate->resolve_undo;\n@@ -781,8 +782,9 @@ static int fsck_resolve_undo(struct index_state *istate)\n \n \t\t\tobj = parse_object(the_repository, &ru->oid[i]);\n \t\t\tif (!obj) {\n-\t\t\t\terror(_(\"%s: invalid sha1 pointer in resolve-undo\"),\n-\t\t\t\t      oid_to_hex(&ru->oid[i]));\n+\t\t\t\terror(_(\"%s: invalid sha1 pointer in resolve-undo of %s\"),\n+\t\t\t\t      oid_to_hex(&ru->oid[i]),\n+\t\t\t\t      index_path);\n \t\t\t\terrors_found |= ERROR_REFS;\n \t\t\t\tcontinue;\n \t\t\t}\n@@ -795,7 +797,8 @@ static int fsck_resolve_undo(struct index_state *istate)\n \treturn 0;\n }\n \n-static void fsck_index(struct index_state *istate)\n+static void fsck_index(struct index_state *istate, const char *index_path,\n+\t\t       int is_main_index)\n {\n \tunsigned int i;\n \n@@ -816,12 +819,14 @@ static void fsck_index(struct index_state *istate)\n \t\tobj = &blob->object;\n \t\tobj->flags |= USED;\n \t\tfsck_put_object_name(&fsck_walk_options, &obj->oid,\n-\t\t\t\t     \":%s\", istate->cache[i]->name);\n+\t\t\t\t     \"%s:%s\",\n+\t\t\t\t     is_main_index ? \"\" : index_path,\n+\t\t\t\t     istate->cache[i]->name);\n \t\tmark_object_reachable(obj);\n \t}\n \tif (istate->cache_tree)\n-\t\tfsck_cache_tree(istate->cache_tree);\n-\tfsck_resolve_undo(istate);\n+\t\tfsck_cache_tree(istate->cache_tree, index_path);\n+\tfsck_resolve_undo(istate, index_path);\n }\n \n static void mark_object_for_connectivity(const struct object_id *oid)\n@@ -993,12 +998,19 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t\t\tstruct worktree *wt = *p;\n \t\t\tstruct index_state istate =\n \t\t\t\tINDEX_STATE_INIT(the_repository);\n+\t\t\tchar *path;\n \n-\t\t\tif (read_index_from(&istate,\n-\t\t\t\t\t    worktree_git_path(wt, \"index\"),\n+\t\t\t/*\n+\t\t\t * Make a copy since the buffer is reusable\n+\t\t\t * and may get overwritten by other calls\n+\t\t\t * while we're examining the index.\n+\t\t\t */\n+\t\t\tpath = xstrdup(worktree_git_path(wt, \"index\"));\n+\t\t\tif (read_index_from(&istate, path,\n \t\t\t\t\t    get_worktree_git_dir(wt)) > 0)\n-\t\t\t\tfsck_index(&istate);\n+\t\t\t\tfsck_index(&istate, path, wt->is_current);\n \t\t\tdiscard_index(&istate);\n+\t\t\tfree(path);\n \t\t}\n \n \t}\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 3b70ad9e22..bca46378b2 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -1032,7 +1032,25 @@ test_expect_success 'fsck detects problems in worktree index' '\n \tblob=$(git -C wt rev-parse :file) &&\n \tremove_object $blob &&\n \n-\ttest_must_fail git fsck\n+\ttest_must_fail git fsck --name-objects >actual 2>&1 &&\n+\tcat >expect <<-EOF &&\n+\tmissing blob $blob (.git/worktrees/wt/index:file)\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'fsck reports problems in main index without filename' '\n+\ttest_when_finished \"rm -f .git/index && git read-tree HEAD\" &&\n+\techo \"this object will be removed to break the main index\" >file &&\n+\tgit add file &&\n+\tblob=$(git rev-parse :file) &&\n+\tremove_object $blob &&\n+\n+\ttest_must_fail git fsck --name-objects >actual 2>&1 &&\n+\tcat >expect <<-EOF &&\n+\tmissing blob $blob (:file)\n+\tEOF\n+\ttest_cmp expect actual\n '\n \n test_done\n-- \n2.39.2.981.g6157336f25\n"},{"id":"472645","messageId":"Y/h5G+D3jRLXeD16@coredump.intra.peff.net","threadId":"59266","inReplyTo":"Y/hw1YVgCYWX2yNK@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] fsck: check index files in all worktrees","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-02-24T08:45:15Z","receivedAt":"2023-02-24T08:45:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 24, 2023 at 03:09:58AM -0500, Jeff King wrote:\n\n> +\t\tfor (p = get_worktrees(); *p; p++) {\n> +\t\t\tstruct worktree *wt = *p;\n> +\t\t\tstruct index_state istate =\n> +\t\t\t\tINDEX_STATE_INIT(the_repository);\n> +\n> +\t\t\tif (read_index_from(&istate,\n> +\t\t\t\t\t    worktree_git_path(wt, \"index\"),\n> +\t\t\t\t\t    get_worktree_git_dir(wt)) > 0)\n> +\t\t\t\tfsck_index(&istate);\n> +\t\t\tdiscard_index(&istate);\n> +\t\t}\n\nI didn't realize that get_worktrees() returns an allocated array, so\nthis is a small leak. I'll squash this in locally:\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex ddd13cb2b3..c11cb2a95f 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -984,12 +984,13 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (keep_cache_objects) {\n-\t\tstruct worktree **p;\n+\t\tstruct worktree **worktrees, **p;\n \n \t\tverify_index_checksum = 1;\n \t\tverify_ce_order = 1;\n \n-\t\tfor (p = get_worktrees(); *p; p++) {\n+\t\tworktrees = get_worktrees();\n+\t\tfor (p = worktrees; *p; p++) {\n \t\t\tstruct worktree *wt = *p;\n \t\t\tstruct index_state istate =\n \t\t\t\tINDEX_STATE_INIT(the_repository);\n@@ -1000,7 +1001,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t\t\t\tfsck_index(&istate);\n \t\t\tdiscard_index(&istate);\n \t\t}\n-\n+\t\tfree_worktrees(worktrees);\n \t}\n \n \tcheck_connectivity();\n\nbut I'll hold off for other comments before sending a re-roll.\n\n-Peff\n"},{"id":"472657","messageId":"xmqqr0uf0y4b.fsf@gitster.g","threadId":"59266","inReplyTo":"Y/hv0MXAyBY3HEo9@coredump.intra.peff.net","subject":"Re: [PATCH 0/3] fsck index files from all worktrees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-24T17:30:44Z","receivedAt":"2023-02-24T17:30:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> We do fsck the resolve-undo extension, but I think fsck just doesn't\n> know anything about worktrees. That should be easy enough to fix.\n> Patches below.\n> ...\n> Thanks, it was nice to have a test case. I ended up writing a separate\n> test with a missing blob, just because that's simpler to do. It looks\n> like we don't test fsck_resolve_undo() or fsck_cache_tree() at all. That\n> might be a nice addition, but I punted for now to stay focused on the\n> worktree aspects.\n\nSo we had a separate worktree with its index pointing at an object\nby its resolve-undo (or cache-tree) extension, but somehow lost that\nobject to gc (I agree with your assessment that it should no longer\nhappen since 2017).  gc these days knows about looking at the index\nof all worktrees, finds the issue, and stops for safety.  fsck that\nis run in the primary worktree may not have noticed but fsck run\nfrom that worktree would notice the issue.\n\nSounds like a frustrating one.  \n\nThanks, both, for finding and fixing.\n\n"},{"id":"472759","messageId":"ed3b6f63-5693-62e4-72a9-715e6c90a681@kdbg.org","threadId":"59266","inReplyTo":"Y/hv0MXAyBY3HEo9@coredump.intra.peff.net","subject":"Re: [PATCH 0/3] fsck index files from all worktrees","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2023-02-26T21:49:57Z","receivedAt":"2023-02-26T21:50:13Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 24.02.23 um 09:05 schrieb Jeff King:\n> On Sat, Feb 18, 2023 at 10:38:33AM +0100, Johannes Sixt wrote:\n> \n>> I see three problems here:\n>>\n>> - git fsck should detect the problem (if it really is one) in the\n>> worktree index. It seems that it is just an index extension that is\n>> affected. Perhaps it should be just a warning, not an error.\n> \n> We do fsck the resolve-undo extension, but I think fsck just doesn't\n> know anything about worktrees. That should be easy enough to fix.\n> Patches below.\n> \n>> - If the objects mentioned in the index extension are precious, they\n>> should not have been garbage-collected in earlier rounds of git gc\n>> (which I certainly did at some point).\n> \n> Correct, but the gc error you're getting indicates that we _are_ trying\n> to treat them as included. I wonder if you ran git-gc long ago with an\n> older version of Git, and this breakage was waiting to surface. AFAICT\n> this was all fixed by 8a044c7f1d (Merge branch 'nd/prune-in-worktree',\n> 2017-09-19).\n\nI don't know how I got into the situation. The worktree is a lot younger\nthan that and was made with a Git version young enough to include this\ncommit. I'll see if it happens again.\n\n>> - I can't git gc the repository now, which is particularly annoying when\n>> auto-gc is attempted after almost every git command. Of course, I know\n>> how to get out of the situation, but it took some time to identify the\n>> worktree index as the culprit. Not something that a beginner would be\n>> able to do easily.\n> \n> I think in general that \"oops, there's something corrupt\" can be hard to\n> get out of, just because there are so many possibilities. But if we can\n> at least report the nature of the problem and the offending filename via\n> git-fsck, that would help with pointing people in the right direction.\n\nAgreed. Thanks a lot for the patches, they are certainly helpful.\n\n-- Hannes\n\n"},{"id":"472761","messageId":"Y/vdV4bjorvRYoaR@coredump.intra.peff.net","threadId":"59266","inReplyTo":"xmqqr0uf0y4b.fsf@gitster.g","subject":"[PATCH 4/3] fsck: check even zero-entry index files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-02-26T22:29:43Z","receivedAt":"2023-02-26T22:30:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 24, 2023 at 09:30:44AM -0800, Junio C Hamano wrote:\n\n> So we had a separate worktree with its index pointing at an object\n> by its resolve-undo (or cache-tree) extension, but somehow lost that\n> object to gc (I agree with your assessment that it should no longer\n> happen since 2017).  gc these days knows about looking at the index\n> of all worktrees, finds the issue, and stops for safety.  fsck that\n> is run in the primary worktree may not have noticed but fsck run\n> from that worktree would notice the issue.\n> \n> Sounds like a frustrating one.  \n> \n> Thanks, both, for finding and fixing.\n\nI saw that this hit next, but I had a few fixups that I had planned to\nsquash in. I saw you got the leak-fix one, but I have one more. Since\nthis is the end of the cycle, we _could_ just squash it in when we\nrewind next. But having now written it as a patch on top, I think the\nexplanation kind of merits its own commit.\n\n-- >8 --\nSubject: [PATCH] fsck: check even zero-entry index files\n\nIn fb64ca526a (fsck: check index files in all worktrees, 2023-02-24), we\nswapped out a call to vanilla repo_read_index() for a series of\nread_index_from() calls, one per worktree. The code for the latter was\ncopied from add_index_objects_to_pending(), which checks for a positive\nreturn value from the index reading function, and we do the same here in\nfsck now.\n\nBut this is probably the wrong thing. I had interpreted the check as\n\"don't operate on the index struct if there was an error\". But in\nreality, if there is an error then the index-reading code will simply\ndie (which admittedly is not great for fsck, but that is not a new\nproblem).\n\nThe return value here is actually the number of entries read. So it\nmakes sense for add_index_objects_to_pending() to ignore a zero-entry\nindex (there is nothing to add). But for fsck, we would still want to\ncheck any extensions, etc (though presumably it is unlikely to have them\nin an empty index, I don't think it's impossible).\n\nSo we should ignore the return value from read_index_from() entirely.\nThis matches the behavior before fb64ca526a, when we ignored the return\nvalue from repo_read_index().\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nOn top of jk/fsck-indices-in-worktrees.\n\n builtin/fsck.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 1b032eebb1..64614b43b2 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -1007,9 +1007,8 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t\t\t * while we're examining the index.\n \t\t\t */\n \t\t\tpath = xstrdup(worktree_git_path(wt, \"index\"));\n-\t\t\tif (read_index_from(&istate, path,\n-\t\t\t\t\t    get_worktree_git_dir(wt)) > 0)\n-\t\t\t\tfsck_index(&istate, path, wt->is_current);\n+\t\t\tread_index_from(&istate, path, get_worktree_git_dir(wt));\n+\t\t\tfsck_index(&istate, path, wt->is_current);\n \t\t\tdiscard_index(&istate);\n \t\t\tfree(path);\n \t\t}\n-- \n2.40.0.rc0.479.g8b3a13b6b0\n\n"},{"id":"472777","messageId":"c6c82351-4d41-990f-0cdd-565bf2955100@github.com","threadId":"59266","inReplyTo":"Y/vdV4bjorvRYoaR@coredump.intra.peff.net","subject":"Re: [PATCH 4/3] fsck: check even zero-entry index files","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-02-27T12:09:18Z","receivedAt":"2023-02-27T12:09:30Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 2/26/23 5:29 PM, Jeff King wrote:\n> On Fri, Feb 24, 2023 at 09:30:44AM -0800, Junio C Hamano wrote:\n> \n>> So we had a separate worktree with its index pointing at an object\n>> by its resolve-undo (or cache-tree) extension, but somehow lost that\n>> object to gc (I agree with your assessment that it should no longer\n>> happen since 2017).  gc these days knows about looking at the index\n>> of all worktrees, finds the issue, and stops for safety.  fsck that\n>> is run in the primary worktree may not have noticed but fsck run\n>> from that worktree would notice the issue.\n>>\n>> Sounds like a frustrating one.  \n>>\n>> Thanks, both, for finding and fixing.\n> \n> I saw that this hit next, but I had a few fixups that I had planned to\n> squash in. I saw you got the leak-fix one, but I have one more. Since\n> this is the end of the cycle, we _could_ just squash it in when we\n> rewind next. But having now written it as a patch on top, I think the\n> explanation kind of merits its own commit.\n\nI just read all four (and a half) patches and agree that this\nis a valuable change. Thanks for working on it.\n\n-Stolee\n"},{"id":"472796","messageId":"xmqqv8jnt81c.fsf@gitster.g","threadId":"59266","inReplyTo":"Y/vdV4bjorvRYoaR@coredump.intra.peff.net","subject":"Re: [PATCH 4/3] fsck: check even zero-entry index files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-27T15:58:07Z","receivedAt":"2023-02-27T15:58:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> The return value here is actually the number of entries read. So it\n> makes sense for add_index_objects_to_pending() to ignore a zero-entry\n> index (there is nothing to add). But for fsck, we would still want to\n> check any extensions, etc (though presumably it is unlikely to have them\n> in an empty index, I don't think it's impossible).\n\nGood thinking.\n\nNot all extensions record what needs to be fed to the reachability\nmachinery for fsck, but resolve-undo wants to record object names\nthat used to be in the directory (at higher stages) when they are\nremoved, so I think it is entirely possible for an index with no\nentries to have index extensions that fsck needs to pay attention\nto.\n\n> So we should ignore the return value from read_index_from() entirely.\n> This matches the behavior before fb64ca526a, when we ignored the return\n> value from repo_read_index().\n\nGood.  Thanks.\n\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> On top of jk/fsck-indices-in-worktrees.\n>\n>  builtin/fsck.c | 5 ++---\n>  1 file changed, 2 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/fsck.c b/builtin/fsck.c\n> index 1b032eebb1..64614b43b2 100644\n> --- a/builtin/fsck.c\n> +++ b/builtin/fsck.c\n> @@ -1007,9 +1007,8 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n>  \t\t\t * while we're examining the index.\n>  \t\t\t */\n>  \t\t\tpath = xstrdup(worktree_git_path(wt, \"index\"));\n> -\t\t\tif (read_index_from(&istate, path,\n> -\t\t\t\t\t    get_worktree_git_dir(wt)) > 0)\n> -\t\t\t\tfsck_index(&istate, path, wt->is_current);\n> +\t\t\tread_index_from(&istate, path, get_worktree_git_dir(wt));\n> +\t\t\tfsck_index(&istate, path, wt->is_current);\n>  \t\t\tdiscard_index(&istate);\n>  \t\t\tfree(path);\n>  \t\t}\n"},{"id":"477026","messageId":"305ccc55-25e3-6b01-cd86-9a9035839d06@sunshineco.com","threadId":"59266","inReplyTo":"Y/hxW9i9GyKblNV4@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] fsck: mention file path for index errors","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-05-11T06:39:59Z","receivedAt":"2023-05-11T06:41:37Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On 2/24/23 3:12 AM, Jeff King wrote:\n> If we encounter an error in an index file, we may say something like:\n> \n>    error: 1234abcd: invalid sha1 pointer in resolve-undo\n> \n> But if you have multiple worktrees, each with its own index, it can be\n> very helpful to know which file had the problem. So let's pass that path\n> down through the various index-fsck functions and use it where\n> appropriate. After this patch you should get something like:\n> \n>    error: 1234abcd: invalid sha1 pointer in resolve-undo of .git/worktrees/wt/index\n> \n> That's a bit verbose, but since the point is that you shouldn't see this\n> normally, we're better to err on the side of more details.\n> \n> I've also added the index filename to the name used by \"fsck\n> --name-objects\", which will show up if we find the object to be missing,\n> etc. This is bending the rules a little there, as the option claims to\n> write names that can be fed to rev-parse. But there is no revision\n> syntax to access the index of another worktree, so the best we can do is\n> make up something that a human will probably understand.\n>\n> I did take care to retain the existing \":file\" syntax for the current\n> worktree. So the uglier output should kick in only when it's actually\n> necessary. See the included tests for examples of both forms.\n\nThis made me think of the work Duy did[1,2] to make it possible to \nreference per-worktree refs from other worktrees which allows one to \nsay, for instance:\n\n     git rev-parse main-worktree/HEAD:somefile\n     git rev-parse worktrees/foo/HEAD:somefile\n\nbut, of course, that special syntax doesn't extend to \"index\", so your \nmade-up syntax is probably good enough.\n\n[1]: 3a3b9d8cde (refs: new ref types to make per-worktree refs visible \nto all worktrees, 2018-10-21)\n\n[2]: ab3e1f78ae (revision.c: better error reporting on ref from \ndifferent worktrees, 2018-10-21)\n\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> diff --git a/builtin/fsck.c b/builtin/fsck.c\n> @@ -795,7 +797,8 @@ static int fsck_resolve_undo(struct index_state *istate)\n> -static void fsck_index(struct index_state *istate)\n> +static void fsck_index(struct index_state *istate, const char *index_path,\n> +\t\t       int is_main_index)\n\nThis adds an `is_main_index` flag, but...\n\n> @@ -993,12 +998,19 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n> +\t\t\tif (read_index_from(&istate, path,\n>   \t\t\t\t\t    get_worktree_git_dir(wt)) > 0)\n> -\t\t\t\tfsck_index(&istate);\n> +\t\t\t\tfsck_index(&istate, path, wt->is_current);\n\n...this accesses `is_current`, the value of which is \"true\" only for the \nworktree in which the Git command was run, which is not necessarily the \nmain worktree. The main worktree, on the other hand, is guaranteed to be \nthe first entry returned by get_worktrees(), so shouldn't this instead be:\n\n     worktrees = get_worktrees();\n     for (p = worktrees; *p; p++) {\n         ...\n         fsck_index(&istate, path, p == worktrees);\n         ...\n     }\n     free_worktrees(worktrees);\n\nOr am I fundamentally misunderstanding something?\n\n"},{"id":"477040","messageId":"20230511161757.GA1973344@coredump.intra.peff.net","threadId":"59266","inReplyTo":"305ccc55-25e3-6b01-cd86-9a9035839d06@sunshineco.com","subject":"Re: [PATCH 3/3] fsck: mention file path for index errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-05-11T16:17:57Z","receivedAt":"2023-05-11T16:18:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 11, 2023 at 02:39:59AM -0400, Eric Sunshine wrote:\n\n> > Signed-off-by: Jeff King <peff@peff.net>\n> > ---\n> > diff --git a/builtin/fsck.c b/builtin/fsck.c\n> > @@ -795,7 +797,8 @@ static int fsck_resolve_undo(struct index_state *istate)\n> > -static void fsck_index(struct index_state *istate)\n> > +static void fsck_index(struct index_state *istate, const char *index_path,\n> > +\t\t       int is_main_index)\n> \n> This adds an `is_main_index` flag, but...\n> \n> > @@ -993,12 +998,19 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n> > +\t\t\tif (read_index_from(&istate, path,\n> >   \t\t\t\t\t    get_worktree_git_dir(wt)) > 0)\n> > -\t\t\t\tfsck_index(&istate);\n> > +\t\t\t\tfsck_index(&istate, path, wt->is_current);\n> \n> ...this accesses `is_current`, the value of which is \"true\" only for the\n> worktree in which the Git command was run, which is not necessarily the main\n> worktree. The main worktree, on the other hand, is guaranteed to be the\n> first entry returned by get_worktrees(), so shouldn't this instead be:\n> \n>     worktrees = get_worktrees();\n>     for (p = worktrees; *p; p++) {\n>         ...\n>         fsck_index(&istate, path, p == worktrees);\n>         ...\n>     }\n>     free_worktrees(worktrees);\n> \n> Or am I fundamentally misunderstanding something?\n\nI think \"current\" is what we want here, since the point was to return\nthe short-but-syntactically-correct \":path-in-index\" for the current\nworktree, which is where \"rev-parse :path-in-index\", etc, would look\nwhen resolving that name.\n\nSo the code is working as intended, but I may have misused the term\n\"main\" with respect to other worktree code. I didn't even know that was\na concept, not having dealt much with worktrees.\n\nMaybe it's worth s/main/current/ here (and I guess in t1450)?\n\n-Peff\n"},{"id":"477043","messageId":"CAPig+cQP736+944k40wgE8Vybk=ajD-kLTDHM6Y92dKEeWMB8g@mail.gmail.com","threadId":"59266","inReplyTo":"20230511161757.GA1973344@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] fsck: mention file path for index errors","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-05-11T16:28:45Z","receivedAt":"2023-05-11T16:29:00Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, May 11, 2023 at 12:17 PM Jeff King <peff@peff.net> wrote:\n> On Thu, May 11, 2023 at 02:39:59AM -0400, Eric Sunshine wrote:\n> > > +static void fsck_index(struct index_state *istate, const char *index_path,\n> > > +                  int is_main_index)\n> >\n> > This adds an `is_main_index` flag, but...\n> >\n> > > @@ -993,12 +998,19 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n> > > +                           fsck_index(&istate, path, wt->is_current);\n> >\n> > ...this accesses `is_current`, the value of which is \"true\" only for the\n> > worktree in which the Git command was run, which is not necessarily the main\n> > worktree. The main worktree, on the other hand, is guaranteed to be the\n> > first entry returned by get_worktrees(), so shouldn't this instead be:\n> >\n> >     for (p = worktrees; *p; p++) {\n> >         fsck_index(&istate, path, p == worktrees);\n>\n> I think \"current\" is what we want here, since the point was to return\n> the short-but-syntactically-correct \":path-in-index\" for the current\n> worktree, which is where \"rev-parse :path-in-index\", etc, would look\n> when resolving that name.\n\nOkay, that makes sense.\n\n> So the code is working as intended, but I may have misused the term\n> \"main\" with respect to other worktree code. I didn't even know that was\n> a concept, not having dealt much with worktrees.\n>\n> Maybe it's worth s/main/current/ here (and I guess in t1450)?\n\nYes, s/main/current/ probably would be helpful for future readers of\nthe code. It's unfortunate that the term \"current\" can ambiguously\nalso be read as meaning \"the up-to-date index\" or \"the present-time\nindex\" as opposed to \"the index in this directory/worktree\", which is\nthe intention here. But \"current\" is consistent with the existing\n`struct worktree.is_current`, so hopefully should not be too\nconfusing.\n"},{"id":"477045","messageId":"20230511170133.GA1977634@coredump.intra.peff.net","threadId":"59266","inReplyTo":"CAPig+cQP736+944k40wgE8Vybk=ajD-kLTDHM6Y92dKEeWMB8g@mail.gmail.com","subject":"Re: [PATCH 3/3] fsck: mention file path for index errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-05-11T17:01:33Z","receivedAt":"2023-05-11T17:01:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 11, 2023 at 12:28:45PM -0400, Eric Sunshine wrote:\n\n> > So the code is working as intended, but I may have misused the term\n> > \"main\" with respect to other worktree code. I didn't even know that was\n> > a concept, not having dealt much with worktrees.\n> >\n> > Maybe it's worth s/main/current/ here (and I guess in t1450)?\n> \n> Yes, s/main/current/ probably would be helpful for future readers of\n> the code. It's unfortunate that the term \"current\" can ambiguously\n> also be read as meaning \"the up-to-date index\" or \"the present-time\n> index\" as opposed to \"the index in this directory/worktree\", which is\n> the intention here. But \"current\" is consistent with the existing\n> `struct worktree.is_current`, so hopefully should not be too\n> confusing.\n\nI think in this context it should be pretty clear. Do you want to\nprepare a patch?\n\n-Peff\n"},{"id":"477872","messageId":"mvmzg5j8jkk.fsf@suse.de","threadId":"59266","inReplyTo":"Y/hxW9i9GyKblNV4@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] fsck: mention file path for index errors","fromName":"Andreas Schwab","fromEmail":"schwab@suse.de","sentAt":"2023-06-01T12:15:39Z","receivedAt":"2023-06-01T12:16:25Z","isPatch":true,"sender":{"key":"schwab@suse.de","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"On Feb 24 2023, Jeff King wrote:\n\n> If we encounter an error in an index file, we may say something like:\n>\n>   error: 1234abcd: invalid sha1 pointer in resolve-undo\n>\n> But if you have multiple worktrees, each with its own index, it can be\n> very helpful to know which file had the problem. So let's pass that path\n> down through the various index-fsck functions and use it where\n> appropriate. After this patch you should get something like:\n>\n>   error: 1234abcd: invalid sha1 pointer in resolve-undo of .git/worktrees/wt/index\n\nThat is still suboptimal, because there is no obvious mapping from the\ninternal worktree name to the directory where it lives (git worktree\nlist doesn't mention the internal name).  If you have several worktrees\nwith the same base name in different places, the name under\n.git/worktrees is just made unique by appending a number.  Normally you\nwould want to change to the affected worktree directory to repair it.\n\n-- \nAndreas Schwab, SUSE Labs, schwab@suse.de\nGPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7\n\"And now for something completely different.\"\n"},{"id":"477879","messageId":"20230601140436.GB2458601@coredump.intra.peff.net","threadId":"59266","inReplyTo":"mvmzg5j8jkk.fsf@suse.de","subject":"Re: [PATCH 3/3] fsck: mention file path for index errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-06-01T14:04:36Z","receivedAt":"2023-06-01T14:05:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 01, 2023 at 02:15:39PM +0200, Andreas Schwab wrote:\n\n> On Feb 24 2023, Jeff King wrote:\n> \n> > If we encounter an error in an index file, we may say something like:\n> >\n> >   error: 1234abcd: invalid sha1 pointer in resolve-undo\n> >\n> > But if you have multiple worktrees, each with its own index, it can be\n> > very helpful to know which file had the problem. So let's pass that path\n> > down through the various index-fsck functions and use it where\n> > appropriate. After this patch you should get something like:\n> >\n> >   error: 1234abcd: invalid sha1 pointer in resolve-undo of .git/worktrees/wt/index\n> \n> That is still suboptimal, because there is no obvious mapping from the\n> internal worktree name to the directory where it lives (git worktree\n> list doesn't mention the internal name).  If you have several worktrees\n> with the same base name in different places, the name under\n> .git/worktrees is just made unique by appending a number.  Normally you\n> would want to change to the affected worktree directory to repair it.\n\nI don't use worktrees all that much, and I never had to repair one of\nthese cases in the real world, but I would have imagined you'd chdir\ninto the affected .git directory to fix things (either by blowing away\nthe index, or by running Git commands inside there).\n\nI don't think it would be too hard to print more information. The caller\nof fsck_index() has the \"struct worktree\", which contains more path\ninformation. But we'd need to figure out how to present it, as well as\nwhich paths to show in fsck_cache_tree(), etc.\n\nSo I'd say \"patches welcome\" if anybody wants to figure out those\nissues. :)\n\n-Peff\n"},{"id":"479020","messageId":"CAPig+cSeQKr-MNN7_44wuGBCYDMm8H+1mi+X6dd-0p2DkFY2sg@mail.gmail.com","threadId":"59266","inReplyTo":"20230511170133.GA1977634@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] fsck: mention file path for index errors","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-06-29T18:21:31Z","receivedAt":"2023-06-29T18:21:56Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, May 11, 2023 at 1:01 PM Jeff King <peff@peff.net> wrote:\n> On Thu, May 11, 2023 at 12:28:45PM -0400, Eric Sunshine wrote:\n> > Yes, s/main/current/ probably would be helpful for future readers of\n> > the code. It's unfortunate that the term \"current\" can ambiguously\n> > also be read as meaning \"the up-to-date index\" or \"the present-time\n> > index\" as opposed to \"the index in this directory/worktree\", which is\n> > the intention here. But \"current\" is consistent with the existing\n> > `struct worktree.is_current`, so hopefully should not be too\n> > confusing.\n>\n> I think in this context it should be pretty clear. Do you want to\n> prepare a patch?\n\nDone. As usual, I forgot to use --in-reply-to=<this-thread> when\nsending the patch despite having gone through the effort of looking up\nthe relevant message-ID of this thread. Oh well. The patch is here[1].\n\n[1]: https://lore.kernel.org/git/20230629181333.87465-1-ericsunshine@charter.net/\n"},{"id":"479027","messageId":"xmqqwmzm83g0.fsf@gitster.g","threadId":"59266","inReplyTo":"CAPig+cSeQKr-MNN7_44wuGBCYDMm8H+1mi+X6dd-0p2DkFY2sg@mail.gmail.com","subject":"Re: [PATCH 3/3] fsck: mention file path for index errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-29T19:37:51Z","receivedAt":"2023-06-29T19:37:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> On Thu, May 11, 2023 at 1:01 PM Jeff King <peff@peff.net> wrote:\n>> On Thu, May 11, 2023 at 12:28:45PM -0400, Eric Sunshine wrote:\n>> > Yes, s/main/current/ probably would be helpful for future readers of\n>> > the code. It's unfortunate that the term \"current\" can ambiguously\n>> > also be read as meaning \"the up-to-date index\" or \"the present-time\n>> > index\" as opposed to \"the index in this directory/worktree\", which is\n>> > the intention here. But \"current\" is consistent with the existing\n>> > `struct worktree.is_current`, so hopefully should not be too\n>> > confusing.\n>>\n>> I think in this context it should be pretty clear. Do you want to\n>> prepare a patch?\n>\n> Done. As usual, I forgot to use --in-reply-to=<this-thread> when\n> sending the patch despite having gone through the effort of looking up\n> the relevant message-ID of this thread. Oh well. The patch is here[1].\n>\n> [1]: https://lore.kernel.org/git/20230629181333.87465-1-ericsunshine@charter.net/\n\nI've queued your patch on top of the jk/fsck-indices-in-worktrees\ntopic as-is, but the earlier discussion in the thread shows that\nPeff already is in agreement with the change, so I would not mind\namending in his Acked-by: later.\n\nThanks, anyway.\n"}]}