Subject: Re: [GSoC][PATCH v9 5/9] builtin/refs: add verify subcommand

From: shejialuo <[email-masked]>
Date: Wed, 10 Jul 2024 20:32:15 +0800

On Tue, Jul 09, 2024 at 05:30:42PM -0500, Justin Tobler wrote:
> On 24/07/09 08:35PM, shejialuo wrote:
> > Introduce a new subcommand "verify" in git-refs(1) to allow the user to
> > check the reference database consistency.
> 
> The next patch in the series uses `git-refs verify` to when running
> git-fsck(1). It might be worth mentioning here that this is also
> intended as the entry point for fscking refs.
> 

Yes, I will improve the commit message, in the next patch.

> > 
> > Mentored-by: Patrick Steinhardt <[email-masked]>
> > Mentored-by: Karthik Nayak <[email-masked]>
> > Signed-off-by: shejialuo <[email-masked]>
> [snip]
> > @@ -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;
> > +	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 = 0;
> 
> nit: Being that we always assign a value to `ret`, defaulting here to 
> zero is redundant.
> 

I agree.

> > +
> > +	argc = parse_options(argc, argv, prefix, options, verify_usage, 0);
> > +	if (argc)
> > +		usage(_("too many arguments"));
> > +
> > +	if (verbose)
> > +		fsck_refs_options.verbose_refs = 1;
> > +	if (strict)
> > +		fsck_refs_options.strict = 1;
> > +
> > +	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 "oid_skiplist"
> 
> s/oid_skiplist/skip_oids
> 
> I think we forgot to update this comment after the variable was renamed
> in a previous patch version.
> 

Yes, thanks for you remind. I have just forgotten here.