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