Bug: fsck and repack don't agree when a worktree index extension is "broken"

> 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.

> - 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.

> +		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);
> +		}

> 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>
> ---
> 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)

> @@ -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);

> > 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?

> 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.

> 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)?

> > 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.

> 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?

> 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/

> 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

> 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.

> 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.

> 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.

> 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).

>
> 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);
>  		}

> 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.