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

From: Junio C Hamano <[email-masked]>
Date: Mon, 05 Aug 2024 11:11:22 -0700

shejialuo <[email-masked]> writes:

>> > +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.
>> 
>
> After talking with Patrick offline, we decide to drop this patch. At
> current, we should put this change slowly for the user. Because many
> people use "git-fsck(1)", currently we don't have a way to disable ref
> checks by default. It's a little beyond this series.
>
> We may consider later.

Hmph, I am fine with the approach to take it slower.

BUT.

Here is what the diffstat for the whole thing in the updated round
v15 looked like.  Do most of these changes outside refs/ still
needed if we do not give any way to even optionally enable it via
"fsck" for those who feel adventurous?  Should we still be touching
fsck.[ch] and other fsck related infrastructure in the series?

 Documentation/fsck-msgids.txt |   6 ++
 Documentation/git-refs.txt    |  13 +++++
 builtin/fsck.c                |  17 +++---
 builtin/mktag.c               |   3 +-
 builtin/refs.c                |  34 +++++++++++
 fsck.c                        | 127 +++++++++++++++++++++++++++++++++---------
 fsck.h                        |  76 +++++++++++++++++++------
 object-file.c                 |   9 ++-
 refs.c                        |   5 ++
 refs.h                        |   8 +++
 refs/debug.c                  |  11 ++++
 refs/files-backend.c          | 116 +++++++++++++++++++++++++++++++++++++-
 refs/packed-backend.c         |   8 +++
 refs/refs-internal.h          |   6 ++
 refs/reftable-backend.c       |   8 +++
 t/t0602-reffiles-fsck.sh      |  92 ++++++++++++++++++++++++++++++
 16 files changed, 480 insertions(+), 59 deletions(-)