Re: [GSoC][PATCH v13 06/10] git refs: add verify subcommand

From: Patrick Steinhardt <ps@pks.im>
Date: Jul 30, 2024, 08:31 UTC
Message-ID: <Zqik6cgm_6HYVUKy@tanuki>
In-Reply-To: <ZqeYsNtl90N1fVDy@ArchLinux>

On Mon, Jul 29, 2024 at 09:27:12PM +0800, shejialuo wrote:

The subject should probably start with "builtin/refs", not "git refs".

Show 5 quoted lines

> Introduce a new subcommand "verify" in git-refs(1) to allow the user to
> check the reference database consistency and also this subcommand will
> be used as the entry point of checking refs for "git-fsck(1)". Last, add
> "verbose" field into "fsck_options" to indicate whether we should print
> verbose messages when checking refs and objects consistency.

Nice. I very much like that we now have a common home for such low-level ref-related commands. Also, "verify" is neatly in line with e.g. `git commit-graph verify".

Show 9 quoted lines

> @@ -39,6 +43,15 @@ include::ref-storage-format.txt[]
>  	can be used to double check that the migration works as expected before
>  	performing the actual migration.
>  
> +The following options are specific to 'git refs verify':
> +
> +--strict::
> +	Enable more strict checking, every WARN severity for the `Fsck Messages`
> +	be seen as ERROR. See linkgit:git-fsck[1].

How about:

    "Enable stricter error checking. This will cause warnings to be
    reported as errors. See linkgit:git-fsck[1]."

> +--verbose::
> +	When verifying the reference database consistency, be chatty.

I wonder whether this really helps all that much. It doesn't really say what it adds on top of the default mode. So unless we document what exactly this changes, I rather think we can just leave it aways as basically everyone knows what a "--verbose" flag does.

Show 31 quoted lines

>  KNOWN LIMITATIONS
>  -----------------
>  
> diff --git a/builtin/refs.c b/builtin/refs.c
> index 46dcd150d4..4831c9e28e 100644
> --- a/builtin/refs.c
> +++ b/builtin/refs.c
> @@ -1,4 +1,6 @@
>  #include "builtin.h"
> +#include "config.h"
> +#include "fsck.h"
>  #include "parse-options.h"
>  #include "refs.h"
>  #include "repository.h"
> @@ -7,6 +9,9 @@
>  #define REFS_MIGRATE_USAGE \
>  	N_("git refs migrate --ref-format=<format> [--dry-run]")
>  
> +#define REFS_VERIFY_USAGE \
> +	N_("git refs verify [--strict] [--verbose]")
> +
>  static int cmd_refs_migrate(int argc, const char **argv, const char *prefix)
>  {
>  	const char * const migrate_usage[] = {
> @@ -58,15 +63,54 @@ static int cmd_refs_migrate(int argc, const char **argv, const char *prefix)
>  	return err;
>  }
>  
> +static int cmd_refs_verify(int argc, const char **argv, const char *prefix)
> +{
> +	struct fsck_options fsck_refs_options = FSCK_REFS_OPTIONS_DEFAULT;

So we don't ever end up using `FSCK_REFS_OPTIONS_STRICT`? If so, I think we should just drop that declaration in the preceding patch.

Show 20 quoted lines

> +	const char * const verify_usage[] = {
> +		REFS_VERIFY_USAGE,
> +		NULL,
> +	};
> +	unsigned int verbose = 0, strict = 0;
> +	struct option options[] = {
> +		OPT__VERBOSE(&verbose, N_("be verbose")),
> +		OPT_BOOL(0, "strict", &strict, N_("enable strict checking")),
> +		OPT_END(),
> +	};
> +	int ret;
> +
> +	argc = parse_options(argc, argv, prefix, options, verify_usage, 0);
> +	if (argc)
> +		usage(_("'git refs verify' takes no arguments"));
> +
> +	if (verbose)
> +		fsck_refs_options.verbose = 1;
> +	if (strict)
> +		fsck_refs_options.strict = 1;

Instead of manually setting those variables, we can pass pointers to those member variables in the `struct option`s directly.

Show 10 quoted lines

> +	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);
> +
> +	/*
> +	 * Explicitly free the allocated array and "skip_oids" set
> +	 */
> +	free(fsck_refs_options.msg_type);
> +	oidset_clear(&fsck_refs_options.skip_oids);

Should we provide a `fsck_options_clear()` function that does this for us? Otherwise we'll have to adapt callsites of `refs_fsck` whenever internal implementation details of the subsystem add newly allocated members.

Show 26 quoted lines

> +	return ret;
> +}
> +
>  int cmd_refs(int argc, const char **argv, const char *prefix)
>  {
>  	const char * const refs_usage[] = {
>  		REFS_MIGRATE_USAGE,
> +		REFS_VERIFY_USAGE,
>  		NULL,
>  	};
>  	parse_opt_subcommand_fn *fn = NULL;
>  	struct option opts[] = {
>  		OPT_SUBCOMMAND("migrate", &fn, cmd_refs_migrate),
> +		OPT_SUBCOMMAND("verify", &fn, cmd_refs_verify),
>  		OPT_END(),
>  	};
>  
> diff --git a/fsck.h b/fsck.h
> index a4a4ba88ee..b03dba442e 100644
> --- a/fsck.h
> +++ b/fsck.h
> @@ -155,6 +155,7 @@ struct fsck_options {
>  	fsck_walk_func walk;
>  	fsck_error error_func;
>  	unsigned strict:1;
> +	unsigned verbose:1;

Okay. Let's see whether this field will be used in a subsequent patch. If not, we should drop it and get rid of the option altogether, I guess.

Patrick