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

From: shejialuo <shejialuo@gmail.com>
Date: Oct 7, 2024, 08:42 UTC
Message-ID: <ZwOe7YVWmhshRhI9@ArchLinux>
In-Reply-To: <ZwOBwxiSZpxJlsfT@pks.im>

On Mon, Oct 07, 2024 at 08:58:30AM +0200, Patrick Steinhardt wrote:

Show 22 quoted lines

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

Show 24 quoted lines

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

Show 25 quoted lines

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

Show 5 quoted lines

> 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