Re: [PATCH v5 2/9] builtin/refs: support multiple worktrees check for refs.

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:

quoted

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.

quoted

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.

quoted

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.

quoted

@@ -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]

quoted

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