Re: [PATCH v6 4/9] ref: support multiple worktrees check for refs

[Patrick Steinhardt <ps@pks.im>]

On Mon, Oct 21, 2024 at 09:34:40PM +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.

I don't quite follow that logic: the fact that we perform more checks
for the ref content doesn't necessarily mean that we also have to check
worktree refs. We rather want to do that so that we get feature parity
with git-fsck(1) eventually, don't we?

> @@ -66,6 +67,7 @@ static int cmd_refs_migrate(int argc, const char **argv, const char *prefix)
>  static int cmd_refs_verify(int argc, const char **argv, const char *prefix)
>  {
>  	struct fsck_options fsck_refs_options = FSCK_REFS_OPTIONS_DEFAULT;
> +	struct worktree **worktrees, **p;
>  	const char * const verify_usage[] = {
>  		REFS_VERIFY_USAGE,
>  		NULL,

Instead of declaring the `**p` variable we can instead...

> @@ -84,9 +86,15 @@ 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, wt);
> +	}
> +

... refactor this loop like this:

    for (size_t i = 0; worktrees[i]; i++)
        ret |= refs_fsck(get_worktree_ref_store(worktrees[i]),
                         &fsck_refs_options, worktrees[i]);

I was briefly wondering whether we also get worktrees in case the repo
is bare, as we don't actually have a proper worktree there. But the
answer seems to be "yes".

> @@ -3558,6 +3560,9 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,
>  		} else if (S_ISREG(iter->st.st_mode) ||
>  			   S_ISLNK(iter->st.st_mode)) {
>  			strbuf_reset(&target_name);
> +
> +			if (!is_main_worktree(wt))
> +				strbuf_addf(&target_name, "worktrees/%s/", wt->id);
>  			strbuf_addf(&target_name, "%s/%s", refs_check_dir,
>  				    iter->relative_path);
>  

Hm. Isn't it somewhat duplicate to pass both the prepended target name
_and_ the worktree to the callback? I imagine that we'd have to
eventually strip the worktree prefix to find the correct ref, unless we
end up using the main ref store to look up the ref.

> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> index 07c57fd541..46dcaec654 100644
> --- a/refs/packed-backend.c
> +++ b/refs/packed-backend.c
> @@ -13,6 +13,7 @@
>  #include "../lockfile.h"
>  #include "../chdir-notify.h"
>  #include "../statinfo.h"
> +#include "../worktree.h"
>  #include "../wrapper.h"
>  #include "../write-or-die.h"
>  #include "../trace2.h"
> @@ -1754,8 +1755,13 @@ static struct ref_iterator *packed_reflog_iterator_begin(struct ref_store *ref_s
>  }
>  
>  static int packed_fsck(struct ref_store *ref_store UNUSED,
> -		       struct fsck_options *o UNUSED)
> +		       struct fsck_options *o UNUSED,
> +		       struct worktree *wt)
>  {
> +
> +	if (!is_main_worktree(wt))
> +		return 0;
> +
>  	return 0;
>  }

It's somewhat funny to have this condition here, but it does make sense
overall as worktrees never have packed refs in the first place.

Patrick