* Bug: fsck and repack don't agree when a worktree index extension is "broken"
@ 2023-02-18 9:38 Johannes Sixt
2023-02-24 8:05 ` [PATCH 0/3] fsck index files from all worktrees Jeff King
0 siblings, 1 reply; 19+ messages in thread
From: Johannes Sixt @ 2023-02-18 9:38 UTC (permalink / raw)
To: Git Mailing List
[-- Attachment #1: Type: text/plain, Size: 1770 bytes --]
I came into a situation where a worktree index contains an invalid
object ID in an extension. This causes git gc to abort half-way:
$ git gc
Enumerating objects: 6, done.
Counting objects: 100% (6/6), done.
fatal: unable to read d3e1a3edd7d7851bbf811064090e03475d62fd44
fatal: failed to run repack
However, fsck does not find any problem:
$ git fsck
Checking object directories: 100% (256/256), done.
The problem is an invalid object ID that occurs in a worktree index. If
I copy the index to the main worktree, fsck does find the culprit:
$ cp .git/worktrees/wt/index .git/index
$ git fsck
Checking object directories: 100% (256/256), done.
error: d3e1a3edd7d7851bbf811064090e03475d62fd44: invalid sha1 pointer in
resolve-undo
error: 4b40bf1072d6dfeebc09b11ee4d4f22ca2ce3109: invalid sha1 pointer in
resolve-undo
error: 5a494fd3a2182795e0723300ab1ac75c0797be5b: invalid sha1 pointer in
resolve-undo
and git gc fails in the same way as before (of course).
I see three problems here:
- git fsck should detect the problem (if it really is one) in the
worktree index. It seems that it is just an index extension that is
affected. Perhaps it should be just a warning, not an error.
- If the objects mentioned in the index extension are precious, they
should not have been garbage-collected in earlier rounds of git gc
(which I certainly did at some point).
- I can't git gc the repository now, which is particularly annoying when
auto-gc is attempted after almost every git command. Of course, I know
how to get out of the situation, but it took some time to identify the
worktree index as the culprit. Not something that a beginner would be
able to do easily.
The repository I use for the above commands is attached. I hope vger
doesn't strip it away.
-- Hannes
[-- Attachment #2: git-bug-gc-w-worktree-index.tar.gz --]
[-- Type: application/gzip, Size: 3431 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 0/3] fsck index files from all worktrees
2023-02-18 9:38 Bug: fsck and repack don't agree when a worktree index extension is "broken" Johannes Sixt
@ 2023-02-24 8:05 ` Jeff King
2023-02-24 8:07 ` [PATCH 1/3] fsck: factor out index fsck Jeff King
` (4 more replies)
0 siblings, 5 replies; 19+ messages in thread
From: Jeff King @ 2023-02-24 8:05 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Git Mailing List
On Sat, Feb 18, 2023 at 10:38:33AM +0100, Johannes Sixt wrote:
> I see three problems here:
>
> - git fsck should detect the problem (if it really is one) in the
> worktree index. It seems that it is just an index extension that is
> affected. Perhaps it should be just a warning, not an error.
We do fsck the resolve-undo extension, but I think fsck just doesn't
know anything about worktrees. That should be easy enough to fix.
Patches below.
> - If the objects mentioned in the index extension are precious, they
> should not have been garbage-collected in earlier rounds of git gc
> (which I certainly did at some point).
Correct, but the gc error you're getting indicates that we _are_ trying
to treat them as included. I wonder if you ran git-gc long ago with an
older version of Git, and this breakage was waiting to surface. AFAICT
this was all fixed by 8a044c7f1d (Merge branch 'nd/prune-in-worktree',
2017-09-19).
> - I can't git gc the repository now, which is particularly annoying when
> auto-gc is attempted after almost every git command. Of course, I know
> how to get out of the situation, but it took some time to identify the
> worktree index as the culprit. Not something that a beginner would be
> able to do easily.
I think in general that "oops, there's something corrupt" can be hard to
get out of, just because there are so many possibilities. But if we can
at least report the nature of the problem and the offending filename via
git-fsck, that would help with pointing people in the right direction.
> The repository I use for the above commands is attached. I hope vger
> doesn't strip it away.
Thanks, it was nice to have a test case. I ended up writing a separate
test with a missing blob, just because that's simpler to do. It looks
like we don't test fsck_resolve_undo() or fsck_cache_tree() at all. That
might be a nice addition, but I punted for now to stay focused on the
worktree aspects.
[1/3]: fsck: factor out index fsck
[2/3]: fsck: check index files in all worktrees
[3/3]: fsck: mention file path for index errors
builtin/fsck.c | 93 ++++++++++++++++++++++++++++++++-----------------
t/t1450-fsck.sh | 30 ++++++++++++++++
2 files changed, 92 insertions(+), 31 deletions(-)
-Peff
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/3] fsck: factor out index fsck
2023-02-24 8:05 ` [PATCH 0/3] fsck index files from all worktrees Jeff King
@ 2023-02-24 8:07 ` Jeff King
2023-02-24 8:09 ` [PATCH 2/3] fsck: check index files in all worktrees Jeff King
` (3 subsequent siblings)
4 siblings, 0 replies; 19+ messages in thread
From: Jeff King @ 2023-02-24 8:07 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Git Mailing List
The code to fsck an index operates directly on the_index. Let's move it
into its own function in preparation for handling the index files from
other worktrees.
Since we now have only a single reference to the_index, let's drop
our USE_THE_INDEX_VARIABLE definition and just use the_repository.index
directly. That's a minor cleanup, but also ensures that we didn't miss
any references when moving the code into fsck_index().
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/fsck.c | 54 ++++++++++++++++++++++++++++----------------------
1 file changed, 30 insertions(+), 24 deletions(-)
diff --git a/builtin/fsck.c b/builtin/fsck.c
index d207bd909b..fa101e0db2 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -1,4 +1,3 @@
-#define USE_THE_INDEX_VARIABLE
#include "builtin.h"
#include "cache.h"
#include "repository.h"
@@ -796,6 +795,35 @@ static int fsck_resolve_undo(struct index_state *istate)
return 0;
}
+static void fsck_index(struct index_state *istate)
+{
+ unsigned int i;
+
+ /* TODO: audit for interaction with sparse-index. */
+ ensure_full_index(istate);
+ for (i = 0; i < istate->cache_nr; i++) {
+ unsigned int mode;
+ struct blob *blob;
+ struct object *obj;
+
+ mode = istate->cache[i]->ce_mode;
+ if (S_ISGITLINK(mode))
+ continue;
+ blob = lookup_blob(the_repository,
+ &istate->cache[i]->oid);
+ if (!blob)
+ continue;
+ obj = &blob->object;
+ obj->flags |= USED;
+ fsck_put_object_name(&fsck_walk_options, &obj->oid,
+ ":%s", istate->cache[i]->name);
+ mark_object_reachable(obj);
+ }
+ if (istate->cache_tree)
+ fsck_cache_tree(istate->cache_tree);
+ fsck_resolve_undo(istate);
+}
+
static void mark_object_for_connectivity(const struct object_id *oid)
{
struct object *obj = lookup_unknown_object(the_repository, oid);
@@ -959,29 +987,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
verify_index_checksum = 1;
verify_ce_order = 1;
repo_read_index(the_repository);
- /* TODO: audit for interaction with sparse-index. */
- ensure_full_index(&the_index);
- for (i = 0; i < the_index.cache_nr; i++) {
- unsigned int mode;
- struct blob *blob;
- struct object *obj;
-
- mode = the_index.cache[i]->ce_mode;
- if (S_ISGITLINK(mode))
- continue;
- blob = lookup_blob(the_repository,
- &the_index.cache[i]->oid);
- if (!blob)
- continue;
- obj = &blob->object;
- obj->flags |= USED;
- fsck_put_object_name(&fsck_walk_options, &obj->oid,
- ":%s", the_index.cache[i]->name);
- mark_object_reachable(obj);
- }
- if (the_index.cache_tree)
- fsck_cache_tree(the_index.cache_tree);
- fsck_resolve_undo(&the_index);
+ fsck_index(the_repository->index);
}
check_connectivity();
--
2.39.2.981.g6157336f25
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/3] fsck: check index files in all worktrees
2023-02-24 8:05 ` [PATCH 0/3] fsck index files from all worktrees Jeff King
2023-02-24 8:07 ` [PATCH 1/3] fsck: factor out index fsck Jeff King
@ 2023-02-24 8:09 ` Jeff King
2023-02-24 8:45 ` Jeff King
2023-02-24 8:12 ` [PATCH 3/3] fsck: mention file path for index errors Jeff King
` (2 subsequent siblings)
4 siblings, 1 reply; 19+ messages in thread
From: Jeff King @ 2023-02-24 8:09 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Git Mailing List
We check the index file for the main worktree, but completely ignore the
index files in other worktrees. These should be checked, too, as they
are part of the repository state (and in particular, errors in those
index files may cause repo-wide operations like "git gc" to complain).
Reported-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Jeff King <peff@peff.net>
---
I mostly cargo-culted the loop from add_index_objects_to_pending(),
which is how they get included in git-prune, git-repack, etc.
I'm not sure if we should be reporting something if we get a negative
return. Last time I dug into this, I think I found that the
index-reading code could not actually return an error (it dies instead).
If we get zero, there are no entries to check. But I'm not sure if we'd
still have extensions? So maybe this ought to be ignoring the return
value from read_index_from() entirely.
builtin/fsck.c | 17 +++++++++++++++--
t/t1450-fsck.sh | 12 ++++++++++++
2 files changed, 27 insertions(+), 2 deletions(-)
diff --git a/builtin/fsck.c b/builtin/fsck.c
index fa101e0db2..ddd13cb2b3 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -984,10 +984,23 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
}
if (keep_cache_objects) {
+ struct worktree **p;
+
verify_index_checksum = 1;
verify_ce_order = 1;
- repo_read_index(the_repository);
- fsck_index(the_repository->index);
+
+ for (p = get_worktrees(); *p; p++) {
+ struct worktree *wt = *p;
+ struct index_state istate =
+ INDEX_STATE_INIT(the_repository);
+
+ if (read_index_from(&istate,
+ worktree_git_path(wt, "index"),
+ get_worktree_git_dir(wt)) > 0)
+ fsck_index(&istate);
+ discard_index(&istate);
+ }
+
}
check_connectivity();
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index fdb886dfe4..3b70ad9e22 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -1023,4 +1023,16 @@ test_expect_success 'fsck error on gitattributes with excessive size' '
test_cmp expected actual
'
+test_expect_success 'fsck detects problems in worktree index' '
+ test_when_finished "git worktree remove -f wt" &&
+ git worktree add wt &&
+
+ echo "this will be removed to break the worktree index" >wt/file &&
+ git -C wt add file &&
+ blob=$(git -C wt rev-parse :file) &&
+ remove_object $blob &&
+
+ test_must_fail git fsck
+'
+
test_done
--
2.39.2.981.g6157336f25
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 3/3] fsck: mention file path for index errors
2023-02-24 8:05 ` [PATCH 0/3] fsck index files from all worktrees Jeff King
2023-02-24 8:07 ` [PATCH 1/3] fsck: factor out index fsck Jeff King
2023-02-24 8:09 ` [PATCH 2/3] fsck: check index files in all worktrees Jeff King
@ 2023-02-24 8:12 ` Jeff King
2023-05-11 6:39 ` Eric Sunshine
2023-06-01 12:15 ` Andreas Schwab
2023-02-24 17:30 ` [PATCH 0/3] fsck index files from all worktrees Junio C Hamano
2023-02-26 21:49 ` [PATCH 0/3] fsck index files from all worktrees Johannes Sixt
4 siblings, 2 replies; 19+ messages in thread
From: Jeff King @ 2023-02-24 8:12 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Git Mailing List
If we encounter an error in an index file, we may say something like:
error: 1234abcd: invalid sha1 pointer in resolve-undo
But if you have multiple worktrees, each with its own index, it can be
very helpful to know which file had the problem. So let's pass that path
down through the various index-fsck functions and use it where
appropriate. After this patch you should get something like:
error: 1234abcd: invalid sha1 pointer in resolve-undo of .git/worktrees/wt/index
That's a bit verbose, but since the point is that you shouldn't see this
normally, we're better to err on the side of more details.
I've also added the index filename to the name used by "fsck
--name-objects", which will show up if we find the object to be missing,
etc. This is bending the rules a little there, as the option claims to
write names that can be fed to rev-parse. But there is no revision
syntax to access the index of another worktree, so the best we can do is
make up something that a human will probably understand.
I did take care to retain the existing ":file" syntax for the current
worktree. So the uglier output should kick in only when it's actually
necessary. See the included tests for examples of both forms.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/fsck.c | 42 +++++++++++++++++++++++++++---------------
t/t1450-fsck.sh | 20 +++++++++++++++++++-
2 files changed, 46 insertions(+), 16 deletions(-)
diff --git a/builtin/fsck.c b/builtin/fsck.c
index ddd13cb2b3..e0974644a1 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -731,19 +731,19 @@ static int fsck_head_link(const char *head_ref_name,
return 0;
}
-static int fsck_cache_tree(struct cache_tree *it)
+static int fsck_cache_tree(struct cache_tree *it, const char *index_path)
{
int i;
int err = 0;
if (verbose)
- fprintf_ln(stderr, _("Checking cache tree"));
+ fprintf_ln(stderr, _("Checking cache tree of %s"), index_path);
if (0 <= it->entry_count) {
struct object *obj = parse_object(the_repository, &it->oid);
if (!obj) {
- error(_("%s: invalid sha1 pointer in cache-tree"),
- oid_to_hex(&it->oid));
+ error(_("%s: invalid sha1 pointer in cache-tree of %s"),
+ oid_to_hex(&it->oid), index_path);
errors_found |= ERROR_REFS;
return 1;
}
@@ -754,11 +754,12 @@ static int fsck_cache_tree(struct cache_tree *it)
err |= objerror(obj, _("non-tree in cache-tree"));
}
for (i = 0; i < it->subtree_nr; i++)
- err |= fsck_cache_tree(it->down[i]->cache_tree);
+ err |= fsck_cache_tree(it->down[i]->cache_tree, index_path);
return err;
}
-static int fsck_resolve_undo(struct index_state *istate)
+static int fsck_resolve_undo(struct index_state *istate,
+ const char *index_path)
{
struct string_list_item *item;
struct string_list *resolve_undo = istate->resolve_undo;
@@ -781,8 +782,9 @@ static int fsck_resolve_undo(struct index_state *istate)
obj = parse_object(the_repository, &ru->oid[i]);
if (!obj) {
- error(_("%s: invalid sha1 pointer in resolve-undo"),
- oid_to_hex(&ru->oid[i]));
+ error(_("%s: invalid sha1 pointer in resolve-undo of %s"),
+ oid_to_hex(&ru->oid[i]),
+ index_path);
errors_found |= ERROR_REFS;
continue;
}
@@ -795,7 +797,8 @@ static int fsck_resolve_undo(struct index_state *istate)
return 0;
}
-static void fsck_index(struct index_state *istate)
+static void fsck_index(struct index_state *istate, const char *index_path,
+ int is_main_index)
{
unsigned int i;
@@ -816,12 +819,14 @@ static void fsck_index(struct index_state *istate)
obj = &blob->object;
obj->flags |= USED;
fsck_put_object_name(&fsck_walk_options, &obj->oid,
- ":%s", istate->cache[i]->name);
+ "%s:%s",
+ is_main_index ? "" : index_path,
+ istate->cache[i]->name);
mark_object_reachable(obj);
}
if (istate->cache_tree)
- fsck_cache_tree(istate->cache_tree);
- fsck_resolve_undo(istate);
+ fsck_cache_tree(istate->cache_tree, index_path);
+ fsck_resolve_undo(istate, index_path);
}
static void mark_object_for_connectivity(const struct object_id *oid)
@@ -993,12 +998,19 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
struct worktree *wt = *p;
struct index_state istate =
INDEX_STATE_INIT(the_repository);
+ char *path;
- if (read_index_from(&istate,
- worktree_git_path(wt, "index"),
+ /*
+ * Make a copy since the buffer is reusable
+ * and may get overwritten by other calls
+ * while we're examining the index.
+ */
+ path = xstrdup(worktree_git_path(wt, "index"));
+ if (read_index_from(&istate, path,
get_worktree_git_dir(wt)) > 0)
- fsck_index(&istate);
+ fsck_index(&istate, path, wt->is_current);
discard_index(&istate);
+ free(path);
}
}
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index 3b70ad9e22..bca46378b2 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -1032,7 +1032,25 @@ test_expect_success 'fsck detects problems in worktree index' '
blob=$(git -C wt rev-parse :file) &&
remove_object $blob &&
- test_must_fail git fsck
+ test_must_fail git fsck --name-objects >actual 2>&1 &&
+ cat >expect <<-EOF &&
+ missing blob $blob (.git/worktrees/wt/index:file)
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'fsck reports problems in main index without filename' '
+ test_when_finished "rm -f .git/index && git read-tree HEAD" &&
+ echo "this object will be removed to break the main index" >file &&
+ git add file &&
+ blob=$(git rev-parse :file) &&
+ remove_object $blob &&
+
+ test_must_fail git fsck --name-objects >actual 2>&1 &&
+ cat >expect <<-EOF &&
+ missing blob $blob (:file)
+ EOF
+ test_cmp expect actual
'
test_done
--
2.39.2.981.g6157336f25
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 2/3] fsck: check index files in all worktrees
2023-02-24 8:09 ` [PATCH 2/3] fsck: check index files in all worktrees Jeff King
@ 2023-02-24 8:45 ` Jeff King
0 siblings, 0 replies; 19+ messages in thread
From: Jeff King @ 2023-02-24 8:45 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Git Mailing List
On Fri, Feb 24, 2023 at 03:09:58AM -0500, Jeff King wrote:
> + for (p = get_worktrees(); *p; p++) {
> + struct worktree *wt = *p;
> + struct index_state istate =
> + INDEX_STATE_INIT(the_repository);
> +
> + if (read_index_from(&istate,
> + worktree_git_path(wt, "index"),
> + get_worktree_git_dir(wt)) > 0)
> + fsck_index(&istate);
> + discard_index(&istate);
> + }
I didn't realize that get_worktrees() returns an allocated array, so
this is a small leak. I'll squash this in locally:
diff --git a/builtin/fsck.c b/builtin/fsck.c
index ddd13cb2b3..c11cb2a95f 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -984,12 +984,13 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
}
if (keep_cache_objects) {
- struct worktree **p;
+ struct worktree **worktrees, **p;
verify_index_checksum = 1;
verify_ce_order = 1;
- for (p = get_worktrees(); *p; p++) {
+ worktrees = get_worktrees();
+ for (p = worktrees; *p; p++) {
struct worktree *wt = *p;
struct index_state istate =
INDEX_STATE_INIT(the_repository);
@@ -1000,7 +1001,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
fsck_index(&istate);
discard_index(&istate);
}
-
+ free_worktrees(worktrees);
}
check_connectivity();
but I'll hold off for other comments before sending a re-roll.
-Peff
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 0/3] fsck index files from all worktrees
2023-02-24 8:05 ` [PATCH 0/3] fsck index files from all worktrees Jeff King
` (2 preceding siblings ...)
2023-02-24 8:12 ` [PATCH 3/3] fsck: mention file path for index errors Jeff King
@ 2023-02-24 17:30 ` Junio C Hamano
2023-02-26 22:29 ` [PATCH 4/3] fsck: check even zero-entry index files Jeff King
2023-02-26 21:49 ` [PATCH 0/3] fsck index files from all worktrees Johannes Sixt
4 siblings, 1 reply; 19+ messages in thread
From: Junio C Hamano @ 2023-02-24 17:30 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Sixt, Git Mailing List
Jeff King <peff@peff.net> writes:
> We do fsck the resolve-undo extension, but I think fsck just doesn't
> know anything about worktrees. That should be easy enough to fix.
> Patches below.
> ...
> Thanks, it was nice to have a test case. I ended up writing a separate
> test with a missing blob, just because that's simpler to do. It looks
> like we don't test fsck_resolve_undo() or fsck_cache_tree() at all. That
> might be a nice addition, but I punted for now to stay focused on the
> worktree aspects.
So we had a separate worktree with its index pointing at an object
by its resolve-undo (or cache-tree) extension, but somehow lost that
object to gc (I agree with your assessment that it should no longer
happen since 2017). gc these days knows about looking at the index
of all worktrees, finds the issue, and stops for safety. fsck that
is run in the primary worktree may not have noticed but fsck run
from that worktree would notice the issue.
Sounds like a frustrating one.
Thanks, both, for finding and fixing.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/3] fsck index files from all worktrees
2023-02-24 8:05 ` [PATCH 0/3] fsck index files from all worktrees Jeff King
` (3 preceding siblings ...)
2023-02-24 17:30 ` [PATCH 0/3] fsck index files from all worktrees Junio C Hamano
@ 2023-02-26 21:49 ` Johannes Sixt
4 siblings, 0 replies; 19+ messages in thread
From: Johannes Sixt @ 2023-02-26 21:49 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List
Am 24.02.23 um 09:05 schrieb Jeff King:
> On Sat, Feb 18, 2023 at 10:38:33AM +0100, Johannes Sixt wrote:
>
>> I see three problems here:
>>
>> - git fsck should detect the problem (if it really is one) in the
>> worktree index. It seems that it is just an index extension that is
>> affected. Perhaps it should be just a warning, not an error.
>
> We do fsck the resolve-undo extension, but I think fsck just doesn't
> know anything about worktrees. That should be easy enough to fix.
> Patches below.
>
>> - If the objects mentioned in the index extension are precious, they
>> should not have been garbage-collected in earlier rounds of git gc
>> (which I certainly did at some point).
>
> Correct, but the gc error you're getting indicates that we _are_ trying
> to treat them as included. I wonder if you ran git-gc long ago with an
> older version of Git, and this breakage was waiting to surface. AFAICT
> this was all fixed by 8a044c7f1d (Merge branch 'nd/prune-in-worktree',
> 2017-09-19).
I don't know how I got into the situation. The worktree is a lot younger
than that and was made with a Git version young enough to include this
commit. I'll see if it happens again.
>> - I can't git gc the repository now, which is particularly annoying when
>> auto-gc is attempted after almost every git command. Of course, I know
>> how to get out of the situation, but it took some time to identify the
>> worktree index as the culprit. Not something that a beginner would be
>> able to do easily.
>
> I think in general that "oops, there's something corrupt" can be hard to
> get out of, just because there are so many possibilities. But if we can
> at least report the nature of the problem and the offending filename via
> git-fsck, that would help with pointing people in the right direction.
Agreed. Thanks a lot for the patches, they are certainly helpful.
-- Hannes
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 4/3] fsck: check even zero-entry index files
2023-02-24 17:30 ` [PATCH 0/3] fsck index files from all worktrees Junio C Hamano
@ 2023-02-26 22:29 ` Jeff King
2023-02-27 12:09 ` Derrick Stolee
2023-02-27 15:58 ` Junio C Hamano
0 siblings, 2 replies; 19+ messages in thread
From: Jeff King @ 2023-02-26 22:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, Git Mailing List
On Fri, Feb 24, 2023 at 09:30:44AM -0800, Junio C Hamano wrote:
> So we had a separate worktree with its index pointing at an object
> by its resolve-undo (or cache-tree) extension, but somehow lost that
> object to gc (I agree with your assessment that it should no longer
> happen since 2017). gc these days knows about looking at the index
> of all worktrees, finds the issue, and stops for safety. fsck that
> is run in the primary worktree may not have noticed but fsck run
> from that worktree would notice the issue.
>
> Sounds like a frustrating one.
>
> Thanks, both, for finding and fixing.
I saw that this hit next, but I had a few fixups that I had planned to
squash in. I saw you got the leak-fix one, but I have one more. Since
this is the end of the cycle, we _could_ just squash it in when we
rewind next. But having now written it as a patch on top, I think the
explanation kind of merits its own commit.
-- >8 --
Subject: [PATCH] fsck: check even zero-entry index files
In fb64ca526a (fsck: check index files in all worktrees, 2023-02-24), we
swapped out a call to vanilla repo_read_index() for a series of
read_index_from() calls, one per worktree. The code for the latter was
copied from add_index_objects_to_pending(), which checks for a positive
return value from the index reading function, and we do the same here in
fsck now.
But this is probably the wrong thing. I had interpreted the check as
"don't operate on the index struct if there was an error". But in
reality, if there is an error then the index-reading code will simply
die (which admittedly is not great for fsck, but that is not a new
problem).
The return value here is actually the number of entries read. So it
makes sense for add_index_objects_to_pending() to ignore a zero-entry
index (there is nothing to add). But for fsck, we would still want to
check any extensions, etc (though presumably it is unlikely to have them
in an empty index, I don't think it's impossible).
So we should ignore the return value from read_index_from() entirely.
This matches the behavior before fb64ca526a, when we ignored the return
value from repo_read_index().
Signed-off-by: Jeff King <peff@peff.net>
---
On top of jk/fsck-indices-in-worktrees.
builtin/fsck.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/builtin/fsck.c b/builtin/fsck.c
index 1b032eebb1..64614b43b2 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -1007,9 +1007,8 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
* while we're examining the index.
*/
path = xstrdup(worktree_git_path(wt, "index"));
- if (read_index_from(&istate, path,
- get_worktree_git_dir(wt)) > 0)
- fsck_index(&istate, path, wt->is_current);
+ read_index_from(&istate, path, get_worktree_git_dir(wt));
+ fsck_index(&istate, path, wt->is_current);
discard_index(&istate);
free(path);
}
--
2.40.0.rc0.479.g8b3a13b6b0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 4/3] fsck: check even zero-entry index files
2023-02-26 22:29 ` [PATCH 4/3] fsck: check even zero-entry index files Jeff King
@ 2023-02-27 12:09 ` Derrick Stolee
2023-02-27 15:58 ` Junio C Hamano
1 sibling, 0 replies; 19+ messages in thread
From: Derrick Stolee @ 2023-02-27 12:09 UTC (permalink / raw)
To: Jeff King, Junio C Hamano; +Cc: Johannes Sixt, Git Mailing List
On 2/26/23 5:29 PM, Jeff King wrote:
> On Fri, Feb 24, 2023 at 09:30:44AM -0800, Junio C Hamano wrote:
>
>> So we had a separate worktree with its index pointing at an object
>> by its resolve-undo (or cache-tree) extension, but somehow lost that
>> object to gc (I agree with your assessment that it should no longer
>> happen since 2017). gc these days knows about looking at the index
>> of all worktrees, finds the issue, and stops for safety. fsck that
>> is run in the primary worktree may not have noticed but fsck run
>> from that worktree would notice the issue.
>>
>> Sounds like a frustrating one.
>>
>> Thanks, both, for finding and fixing.
>
> I saw that this hit next, but I had a few fixups that I had planned to
> squash in. I saw you got the leak-fix one, but I have one more. Since
> this is the end of the cycle, we _could_ just squash it in when we
> rewind next. But having now written it as a patch on top, I think the
> explanation kind of merits its own commit.
I just read all four (and a half) patches and agree that this
is a valuable change. Thanks for working on it.
-Stolee
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 4/3] fsck: check even zero-entry index files
2023-02-26 22:29 ` [PATCH 4/3] fsck: check even zero-entry index files Jeff King
2023-02-27 12:09 ` Derrick Stolee
@ 2023-02-27 15:58 ` Junio C Hamano
1 sibling, 0 replies; 19+ messages in thread
From: Junio C Hamano @ 2023-02-27 15:58 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Sixt, Git Mailing List
Jeff King <peff@peff.net> writes:
> The return value here is actually the number of entries read. So it
> makes sense for add_index_objects_to_pending() to ignore a zero-entry
> index (there is nothing to add). But for fsck, we would still want to
> check any extensions, etc (though presumably it is unlikely to have them
> in an empty index, I don't think it's impossible).
Good thinking.
Not all extensions record what needs to be fed to the reachability
machinery for fsck, but resolve-undo wants to record object names
that used to be in the directory (at higher stages) when they are
removed, so I think it is entirely possible for an index with no
entries to have index extensions that fsck needs to pay attention
to.
> So we should ignore the return value from read_index_from() entirely.
> This matches the behavior before fb64ca526a, when we ignored the return
> value from repo_read_index().
Good. Thanks.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> On top of jk/fsck-indices-in-worktrees.
>
> builtin/fsck.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/fsck.c b/builtin/fsck.c
> index 1b032eebb1..64614b43b2 100644
> --- a/builtin/fsck.c
> +++ b/builtin/fsck.c
> @@ -1007,9 +1007,8 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
> * while we're examining the index.
> */
> path = xstrdup(worktree_git_path(wt, "index"));
> - if (read_index_from(&istate, path,
> - get_worktree_git_dir(wt)) > 0)
> - fsck_index(&istate, path, wt->is_current);
> + read_index_from(&istate, path, get_worktree_git_dir(wt));
> + fsck_index(&istate, path, wt->is_current);
> discard_index(&istate);
> free(path);
> }
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] fsck: mention file path for index errors
2023-02-24 8:12 ` [PATCH 3/3] fsck: mention file path for index errors Jeff King
@ 2023-05-11 6:39 ` Eric Sunshine
2023-05-11 16:17 ` Jeff King
2023-06-01 12:15 ` Andreas Schwab
1 sibling, 1 reply; 19+ messages in thread
From: Eric Sunshine @ 2023-05-11 6:39 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List, Johannes Sixt
On 2/24/23 3:12 AM, Jeff King wrote:
> If we encounter an error in an index file, we may say something like:
>
> error: 1234abcd: invalid sha1 pointer in resolve-undo
>
> But if you have multiple worktrees, each with its own index, it can be
> very helpful to know which file had the problem. So let's pass that path
> down through the various index-fsck functions and use it where
> appropriate. After this patch you should get something like:
>
> error: 1234abcd: invalid sha1 pointer in resolve-undo of .git/worktrees/wt/index
>
> That's a bit verbose, but since the point is that you shouldn't see this
> normally, we're better to err on the side of more details.
>
> I've also added the index filename to the name used by "fsck
> --name-objects", which will show up if we find the object to be missing,
> etc. This is bending the rules a little there, as the option claims to
> write names that can be fed to rev-parse. But there is no revision
> syntax to access the index of another worktree, so the best we can do is
> make up something that a human will probably understand.
>
> I did take care to retain the existing ":file" syntax for the current
> worktree. So the uglier output should kick in only when it's actually
> necessary. See the included tests for examples of both forms.
This made me think of the work Duy did[1,2] to make it possible to
reference per-worktree refs from other worktrees which allows one to
say, for instance:
git rev-parse main-worktree/HEAD:somefile
git rev-parse worktrees/foo/HEAD:somefile
but, of course, that special syntax doesn't extend to "index", so your
made-up syntax is probably good enough.
[1]: 3a3b9d8cde (refs: new ref types to make per-worktree refs visible
to all worktrees, 2018-10-21)
[2]: ab3e1f78ae (revision.c: better error reporting on ref from
different worktrees, 2018-10-21)
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> diff --git a/builtin/fsck.c b/builtin/fsck.c
> @@ -795,7 +797,8 @@ static int fsck_resolve_undo(struct index_state *istate)
> -static void fsck_index(struct index_state *istate)
> +static void fsck_index(struct index_state *istate, const char *index_path,
> + int is_main_index)
This adds an `is_main_index` flag, but...
> @@ -993,12 +998,19 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
> + if (read_index_from(&istate, path,
> get_worktree_git_dir(wt)) > 0)
> - fsck_index(&istate);
> + fsck_index(&istate, path, wt->is_current);
...this accesses `is_current`, the value of which is "true" only for the
worktree in which the Git command was run, which is not necessarily the
main worktree. The main worktree, on the other hand, is guaranteed to be
the first entry returned by get_worktrees(), so shouldn't this instead be:
worktrees = get_worktrees();
for (p = worktrees; *p; p++) {
...
fsck_index(&istate, path, p == worktrees);
...
}
free_worktrees(worktrees);
Or am I fundamentally misunderstanding something?
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] fsck: mention file path for index errors
2023-05-11 6:39 ` Eric Sunshine
@ 2023-05-11 16:17 ` Jeff King
2023-05-11 16:28 ` Eric Sunshine
0 siblings, 1 reply; 19+ messages in thread
From: Jeff King @ 2023-05-11 16:17 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Git Mailing List, Johannes Sixt
On Thu, May 11, 2023 at 02:39:59AM -0400, Eric Sunshine wrote:
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
> > diff --git a/builtin/fsck.c b/builtin/fsck.c
> > @@ -795,7 +797,8 @@ static int fsck_resolve_undo(struct index_state *istate)
> > -static void fsck_index(struct index_state *istate)
> > +static void fsck_index(struct index_state *istate, const char *index_path,
> > + int is_main_index)
>
> This adds an `is_main_index` flag, but...
>
> > @@ -993,12 +998,19 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
> > + if (read_index_from(&istate, path,
> > get_worktree_git_dir(wt)) > 0)
> > - fsck_index(&istate);
> > + fsck_index(&istate, path, wt->is_current);
>
> ...this accesses `is_current`, the value of which is "true" only for the
> worktree in which the Git command was run, which is not necessarily the main
> worktree. The main worktree, on the other hand, is guaranteed to be the
> first entry returned by get_worktrees(), so shouldn't this instead be:
>
> worktrees = get_worktrees();
> for (p = worktrees; *p; p++) {
> ...
> fsck_index(&istate, path, p == worktrees);
> ...
> }
> free_worktrees(worktrees);
>
> Or am I fundamentally misunderstanding something?
I think "current" is what we want here, since the point was to return
the short-but-syntactically-correct ":path-in-index" for the current
worktree, which is where "rev-parse :path-in-index", etc, would look
when resolving that name.
So the code is working as intended, but I may have misused the term
"main" with respect to other worktree code. I didn't even know that was
a concept, not having dealt much with worktrees.
Maybe it's worth s/main/current/ here (and I guess in t1450)?
-Peff
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] fsck: mention file path for index errors
2023-05-11 16:17 ` Jeff King
@ 2023-05-11 16:28 ` Eric Sunshine
2023-05-11 17:01 ` Jeff King
0 siblings, 1 reply; 19+ messages in thread
From: Eric Sunshine @ 2023-05-11 16:28 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List, Johannes Sixt
On Thu, May 11, 2023 at 12:17 PM Jeff King <peff@peff.net> wrote:
> On Thu, May 11, 2023 at 02:39:59AM -0400, Eric Sunshine wrote:
> > > +static void fsck_index(struct index_state *istate, const char *index_path,
> > > + int is_main_index)
> >
> > This adds an `is_main_index` flag, but...
> >
> > > @@ -993,12 +998,19 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
> > > + fsck_index(&istate, path, wt->is_current);
> >
> > ...this accesses `is_current`, the value of which is "true" only for the
> > worktree in which the Git command was run, which is not necessarily the main
> > worktree. The main worktree, on the other hand, is guaranteed to be the
> > first entry returned by get_worktrees(), so shouldn't this instead be:
> >
> > for (p = worktrees; *p; p++) {
> > fsck_index(&istate, path, p == worktrees);
>
> I think "current" is what we want here, since the point was to return
> the short-but-syntactically-correct ":path-in-index" for the current
> worktree, which is where "rev-parse :path-in-index", etc, would look
> when resolving that name.
Okay, that makes sense.
> So the code is working as intended, but I may have misused the term
> "main" with respect to other worktree code. I didn't even know that was
> a concept, not having dealt much with worktrees.
>
> Maybe it's worth s/main/current/ here (and I guess in t1450)?
Yes, s/main/current/ probably would be helpful for future readers of
the code. It's unfortunate that the term "current" can ambiguously
also be read as meaning "the up-to-date index" or "the present-time
index" as opposed to "the index in this directory/worktree", which is
the intention here. But "current" is consistent with the existing
`struct worktree.is_current`, so hopefully should not be too
confusing.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] fsck: mention file path for index errors
2023-05-11 16:28 ` Eric Sunshine
@ 2023-05-11 17:01 ` Jeff King
2023-06-29 18:21 ` Eric Sunshine
0 siblings, 1 reply; 19+ messages in thread
From: Jeff King @ 2023-05-11 17:01 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Git Mailing List, Johannes Sixt
On Thu, May 11, 2023 at 12:28:45PM -0400, Eric Sunshine wrote:
> > So the code is working as intended, but I may have misused the term
> > "main" with respect to other worktree code. I didn't even know that was
> > a concept, not having dealt much with worktrees.
> >
> > Maybe it's worth s/main/current/ here (and I guess in t1450)?
>
> Yes, s/main/current/ probably would be helpful for future readers of
> the code. It's unfortunate that the term "current" can ambiguously
> also be read as meaning "the up-to-date index" or "the present-time
> index" as opposed to "the index in this directory/worktree", which is
> the intention here. But "current" is consistent with the existing
> `struct worktree.is_current`, so hopefully should not be too
> confusing.
I think in this context it should be pretty clear. Do you want to
prepare a patch?
-Peff
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] fsck: mention file path for index errors
2023-02-24 8:12 ` [PATCH 3/3] fsck: mention file path for index errors Jeff King
2023-05-11 6:39 ` Eric Sunshine
@ 2023-06-01 12:15 ` Andreas Schwab
2023-06-01 14:04 ` Jeff King
1 sibling, 1 reply; 19+ messages in thread
From: Andreas Schwab @ 2023-06-01 12:15 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Sixt, Git Mailing List
On Feb 24 2023, Jeff King wrote:
> If we encounter an error in an index file, we may say something like:
>
> error: 1234abcd: invalid sha1 pointer in resolve-undo
>
> But if you have multiple worktrees, each with its own index, it can be
> very helpful to know which file had the problem. So let's pass that path
> down through the various index-fsck functions and use it where
> appropriate. After this patch you should get something like:
>
> error: 1234abcd: invalid sha1 pointer in resolve-undo of .git/worktrees/wt/index
That is still suboptimal, because there is no obvious mapping from the
internal worktree name to the directory where it lives (git worktree
list doesn't mention the internal name). If you have several worktrees
with the same base name in different places, the name under
.git/worktrees is just made unique by appending a number. Normally you
would want to change to the affected worktree directory to repair it.
--
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] fsck: mention file path for index errors
2023-06-01 12:15 ` Andreas Schwab
@ 2023-06-01 14:04 ` Jeff King
0 siblings, 0 replies; 19+ messages in thread
From: Jeff King @ 2023-06-01 14:04 UTC (permalink / raw)
To: Andreas Schwab; +Cc: Johannes Sixt, Git Mailing List
On Thu, Jun 01, 2023 at 02:15:39PM +0200, Andreas Schwab wrote:
> On Feb 24 2023, Jeff King wrote:
>
> > If we encounter an error in an index file, we may say something like:
> >
> > error: 1234abcd: invalid sha1 pointer in resolve-undo
> >
> > But if you have multiple worktrees, each with its own index, it can be
> > very helpful to know which file had the problem. So let's pass that path
> > down through the various index-fsck functions and use it where
> > appropriate. After this patch you should get something like:
> >
> > error: 1234abcd: invalid sha1 pointer in resolve-undo of .git/worktrees/wt/index
>
> That is still suboptimal, because there is no obvious mapping from the
> internal worktree name to the directory where it lives (git worktree
> list doesn't mention the internal name). If you have several worktrees
> with the same base name in different places, the name under
> .git/worktrees is just made unique by appending a number. Normally you
> would want to change to the affected worktree directory to repair it.
I don't use worktrees all that much, and I never had to repair one of
these cases in the real world, but I would have imagined you'd chdir
into the affected .git directory to fix things (either by blowing away
the index, or by running Git commands inside there).
I don't think it would be too hard to print more information. The caller
of fsck_index() has the "struct worktree", which contains more path
information. But we'd need to figure out how to present it, as well as
which paths to show in fsck_cache_tree(), etc.
So I'd say "patches welcome" if anybody wants to figure out those
issues. :)
-Peff
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] fsck: mention file path for index errors
2023-05-11 17:01 ` Jeff King
@ 2023-06-29 18:21 ` Eric Sunshine
2023-06-29 19:37 ` Junio C Hamano
0 siblings, 1 reply; 19+ messages in thread
From: Eric Sunshine @ 2023-06-29 18:21 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List, Johannes Sixt
On Thu, May 11, 2023 at 1:01 PM Jeff King <peff@peff.net> wrote:
> On Thu, May 11, 2023 at 12:28:45PM -0400, Eric Sunshine wrote:
> > Yes, s/main/current/ probably would be helpful for future readers of
> > the code. It's unfortunate that the term "current" can ambiguously
> > also be read as meaning "the up-to-date index" or "the present-time
> > index" as opposed to "the index in this directory/worktree", which is
> > the intention here. But "current" is consistent with the existing
> > `struct worktree.is_current`, so hopefully should not be too
> > confusing.
>
> I think in this context it should be pretty clear. Do you want to
> prepare a patch?
Done. As usual, I forgot to use --in-reply-to=<this-thread> when
sending the patch despite having gone through the effort of looking up
the relevant message-ID of this thread. Oh well. The patch is here[1].
[1]: https://lore.kernel.org/git/20230629181333.87465-1-ericsunshine@charter.net/
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] fsck: mention file path for index errors
2023-06-29 18:21 ` Eric Sunshine
@ 2023-06-29 19:37 ` Junio C Hamano
0 siblings, 0 replies; 19+ messages in thread
From: Junio C Hamano @ 2023-06-29 19:37 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Jeff King, Git Mailing List, Johannes Sixt
Eric Sunshine <sunshine@sunshineco.com> writes:
> On Thu, May 11, 2023 at 1:01 PM Jeff King <peff@peff.net> wrote:
>> On Thu, May 11, 2023 at 12:28:45PM -0400, Eric Sunshine wrote:
>> > Yes, s/main/current/ probably would be helpful for future readers of
>> > the code. It's unfortunate that the term "current" can ambiguously
>> > also be read as meaning "the up-to-date index" or "the present-time
>> > index" as opposed to "the index in this directory/worktree", which is
>> > the intention here. But "current" is consistent with the existing
>> > `struct worktree.is_current`, so hopefully should not be too
>> > confusing.
>>
>> I think in this context it should be pretty clear. Do you want to
>> prepare a patch?
>
> Done. As usual, I forgot to use --in-reply-to=<this-thread> when
> sending the patch despite having gone through the effort of looking up
> the relevant message-ID of this thread. Oh well. The patch is here[1].
>
> [1]: https://lore.kernel.org/git/20230629181333.87465-1-ericsunshine@charter.net/
I've queued your patch on top of the jk/fsck-indices-in-worktrees
topic as-is, but the earlier discussion in the thread shows that
Peff already is in agreement with the change, so I would not mind
amending in his Acked-by: later.
Thanks, anyway.
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2023-06-29 19:37 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-18 9:38 Bug: fsck and repack don't agree when a worktree index extension is "broken" Johannes Sixt
2023-02-24 8:05 ` [PATCH 0/3] fsck index files from all worktrees Jeff King
2023-02-24 8:07 ` [PATCH 1/3] fsck: factor out index fsck Jeff King
2023-02-24 8:09 ` [PATCH 2/3] fsck: check index files in all worktrees Jeff King
2023-02-24 8:45 ` Jeff King
2023-02-24 8:12 ` [PATCH 3/3] fsck: mention file path for index errors Jeff King
2023-05-11 6:39 ` Eric Sunshine
2023-05-11 16:17 ` Jeff King
2023-05-11 16:28 ` Eric Sunshine
2023-05-11 17:01 ` Jeff King
2023-06-29 18:21 ` Eric Sunshine
2023-06-29 19:37 ` Junio C Hamano
2023-06-01 12:15 ` Andreas Schwab
2023-06-01 14:04 ` Jeff King
2023-02-24 17:30 ` [PATCH 0/3] fsck index files from all worktrees Junio C Hamano
2023-02-26 22:29 ` [PATCH 4/3] fsck: check even zero-entry index files Jeff King
2023-02-27 12:09 ` Derrick Stolee
2023-02-27 15:58 ` Junio C Hamano
2023-02-26 21:49 ` [PATCH 0/3] fsck index files from all worktrees Johannes Sixt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).