On Mon, Oct 07, 2024 at 08:58:30AM +0200, Patrick Steinhardt wrote:
> On Sun, Sep 29, 2024 at 03:15:26PM +0800, shejialuo wrote:
> > We have already set up the infrastructure to check the consistency for
> > refs, but we do not support multiple worktrees. As we decide to add more
> > checks for ref content, we need to set up support for multiple
> > worktrees. Use "get_worktrees" and "get_worktree_ref_store" to check
> > refs under the worktrees.
>
> Makes sense.
>
> > Because we should only check once for "packed-refs", let's call the fsck
> > function for packed-backend when in the main worktree. In order to know
> > which directory we check, we should default print this information
> > instead of specifying "--verbose".
>
> This change should likely be evicted into its own commit with a bit more
> explanation.
>
> > It's not suitable to print these information to the stderr. So, change
> > to stdout.
>
> This one, too. Why exactly is in not suitable to print to stderr?
>
I am sorry for the confusion. We should not print which directory we
check here into stderr. Because I think this will make test script
contain many unrelated info when using "git refs verify 2>err".
The reason here is when checking the consistency of refs in multiple
worktrees. The ref name could be repeat. For example, worktree A
has its own ref called "test" under ".git/worktrees/A/refs/worktree/test"
and worktree B has its own ref still called "test" under
".git/worktrees/B/refs/worktree/test".
However, the refname would be printed to "refs/worktree/test". It will
make the user confused which "refs/worktree/test" is checked. So, we
should print this information like:
Checking references consistency in .git
...
checking references consistency in .git/worktrees/A
...
checking references consistency in .git/worktrees/B
However, when writing this, I feel a ".git" is a bad usage. It will make
the user think it will check everything here. This should be improved in
the next version.
> > @@ -75,7 +77,7 @@ static int cmd_refs_verify(int argc, const char **argv, const char *prefix)
> > OPT_BOOL(0, "strict", &fsck_refs_options.strict, N_("enable strict checking")),
> > OPT_END(),
> > };
> > - int ret;
> > + int ret = 0;
> >
> > argc = parse_options(argc, argv, prefix, options, verify_usage, 0);
> > if (argc)
> > @@ -84,9 +86,14 @@ static int cmd_refs_verify(int argc, const char **argv, const char *prefix)
> > git_config(git_fsck_config, &fsck_refs_options);
> > prepare_repo_settings(the_repository);
> >
> > - ret = refs_fsck(get_main_ref_store(the_repository), &fsck_refs_options);
> > + worktrees = get_worktrees();
> > + for (p = worktrees; *p; p++) {
> > + struct worktree *wt = *p;
> > + ret += refs_fsck(get_worktree_ref_store(wt), &fsck_refs_options);
> > + }
>
> I think it is more customary to say `ret |=` instead of `ref +=`.
> Otherwise we could at least in theory wrap around and even land at `ret
> == 0`, even though this is quite unlikely.
>
I agree here. I will improve this in the next version.
[snip]
> > @@ -3600,8 +3600,16 @@ static int files_fsck(struct ref_store *ref_store,
> > struct files_ref_store *refs =
> > files_downcast(ref_store, REF_STORE_READ, "fsck");
> >
> > - return files_fsck_refs(ref_store, o) |
> > - refs->packed_ref_store->be->fsck(refs->packed_ref_store, o);
> > + int ret = files_fsck_refs(ref_store, o);
> > +
> > + /*
> > + * packed-refs should only be checked once because it is shared
> > + * between all worktrees.
> > + */
> > + if (!strcmp(ref_store->gitdir, ref_store->repo->gitdir))
> > + ret += refs->packed_ref_store->be->fsck(refs->packed_ref_store, o);
> > +
> > + return ret;
> > }
> >
> > struct ref_storage_be refs_be_files = {
>
> What is the current behaviour? Is it that we verify the packed-refs file
> multiple times, or rather that we call `packed_ref_store->be->fsck()`
> many times even though we know it won't do anything for anything except
> for the main worktree?
>
That's a good question. I think the second is the current behaviour. We
will call `packed_ref_store->be->fsck()` many times. I understand what
you mean here, we just put the check into `packed_ref_store->be->fsck()`
function.
> If it is the former I very much agree that we should make this
> conditional. If it's the latter I'm more in the camp of letting it be
> such that if worktrees were to ever gain support for "packed-refs" we
> wouldn't have to change anything.
>
I agree.
> In any case, as proposed I think it would make sense to evict this into
> a standalone commit such that these details can be explained in the
> commit message.
>
Yes, the current commit message lacks of details.
> Patrick
Thanks,
Jialuo
