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

From: shejialuo <shejialuo@gmail.com>
Date: Jul 30, 2024, 15:59 UTC
Message-ID: <ZqkN4wNK0PIRyNky@ArchLinux>
In-Reply-To: <Zqik6cgm_6HYVUKy@tanuki>

On Tue, Jul 30, 2024 at 10:31:37AM +0200, Patrick Steinhardt wrote:

> On Mon, Jul 29, 2024 at 09:27:12PM +0800, shejialuo wrote:
> 
> The subject should probably start with "builtin/refs", not "git refs".
>

Yes, I will improve this in the next version.

Show 15 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]."
>

Yes, it is much more clear. Actually, I really feel hard to write a good document.

Show 8 quoted lines

> > +--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.
>

Yes, I think so. `--verbose` is a common flag. However, we have already added this, so we may just leave it here. It's not bad to add more information.

Show 7 quoted lines

> > +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.
>

I agree here. I will delete `FSCK_REFS_OPTIONS_STRICT`.

Show 24 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.
>

Yes, but I have tried but found that the types are mismatching, I will find a way to do this.

Show 16 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.
>

Yes, I agree with this. I wanna talk more on this. In the first time, I did not call `oidset_clear` and I failed the CI tests. It made me confused. Because we never use "skip_oids" in the ref check, why the tests said that "fsck_refs.options.skip_oids" was not freed.

This is because when executing the command "git -c fsck.skipList=.. fsck", in the subprocess `git refs verify`, the code will still setup the "skip_oids" by the config. So we should explicitly free the "skip_oids".

But how does "fsck.c" free "skip_oids", actually "fsck.c" never frees "skip_oids". This is because "git-fsck(1)" defines the following:

  static struct fsck_options fsck_walk_options = FSCK_OPTIONS_DEFAULT;
  static struct fsck_options fsck_obj_options = FSCK_OPTIONS_DEFAULT;

Because these two options are "static", so there is no memory leak. We leave it to the operating system. So maybe a more simple way is just to add "static" identifier in "cmd_refs_verify" which means:

  - struct fsck_options fsck_refs_options = FSCK_REFS_OPTIONS_DEFAULT;
  + static struct fsck_options fsck_refs_options = FSCK_REFS_OPTIONS_DEFAULT;

But I don't think we should use `static`, because Eric has told me that making a variable "static" will make the code harder to "libfy". So let's use "fsck_options_clear" function instead.