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

[Patrick Steinhardt <ps@pks.im>]

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?

> Mentored-by: Patrick Steinhardt <ps@pks.im>
> Mentored-by: Karthik Nayak <karthik.188@gmail.com>
> Signed-off-by: shejialuo <shejialuo@gmail.com>
> ---
>  builtin/refs.c           | 11 ++++++--
>  refs/files-backend.c     | 18 ++++++++----
>  t/t0602-reffiles-fsck.sh | 59 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 81 insertions(+), 7 deletions(-)
> 
> diff --git a/builtin/refs.c b/builtin/refs.c
> index 24978a7b7b..3c492ea922 100644
> --- a/builtin/refs.c
> +++ b/builtin/refs.c
> @@ -5,6 +5,7 @@
>  #include "parse-options.h"
>  #include "refs.h"
>  #include "strbuf.h"
> +#include "worktree.h"
>  
>  #define REFS_MIGRATE_USAGE \
>  	N_("git refs migrate --ref-format=<format> [--dry-run]")
> @@ -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,
> @@ -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.

>  	fsck_options_clear(&fsck_refs_options);
> +	free_worktrees(worktrees);
>  	return ret;
>  }
>  
[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?

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.

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.

Patrick