Subject: Re: [GSoC][PATCH v14 09/11] builtin/fsck: add `git-refs verify` child process

From: Patrick Steinhardt <[email-masked]>
Date: Mon, 5 Aug 2024 14:58:28 +0200

On Thu, Aug 01, 2024 at 11:15:00PM +0800, shejialuo wrote:
> Introduce a new function "fsck_refs" that initializes and runs a child
> process to execute the "git-refs verify" command.

It's `git refs verify`, not `git-refs verify` both in the commit body
and subject.

> Mentored-by: Patrick Steinhardt <[email-masked]>
> Mentored-by: Karthik Nayak <[email-masked]>
> Signed-off-by: shejialuo <[email-masked]>
> ---
>  builtin/fsck.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/builtin/fsck.c b/builtin/fsck.c
> index 766bbd014d..b6ac878270 100644
> --- a/builtin/fsck.c
> +++ b/builtin/fsck.c
> @@ -899,6 +899,21 @@ static int check_pack_rev_indexes(struct repository *r, int show_progress)
>  	return res;
>  }
>  
> +static void fsck_refs(void)
> +{
> +	struct child_process refs_verify = CHILD_PROCESS_INIT;
> +	child_process_init(&refs_verify);
> +	refs_verify.git_cmd = 1;
> +	strvec_pushl(&refs_verify.args, "refs", "verify", NULL);
> +	if (verbose)
> +		strvec_push(&refs_verify.args, "--verbose");
> +	if (check_strict)
> +		strvec_push(&refs_verify.args, "--strict");
> +
> +	if (run_command(&refs_verify))
> +		errors_found |= ERROR_REFS;
> +}

Okay. I think that it's sensible to execute this as part of git-fsck(1).
But do we want to provide an option to disable this new check, as well?

It does feel a bit like opening a can of worms, though. None of the
other checks have trivial ways to disable them, and git-fsck(1) is
gaining more and more checks. So if we can disable ref checks, we may
also want to have options to disable checks for objects, connectivity,
reverse indices, indices, commit graphs and whatnot. In other words, in
my opinion we need to think a bit bigger and design a proper UI around
this.

But I don't think that should happen as part of this commit series, as
it is already big enough. So either we just accept this patch as-is. Or
we evict it from this series and handle it in the future together with
all the other taks that one may potentially want to disable.

I'd rather pick option two.

Patrick