Subject: Re: [GSoC][PATCH v10 06/10] builtin/refs: add verify subcommand and verbose_refs for "fsck_options"

From: shejialuo <[email-masked]>
Date: Thu, 11 Jul 2024 20:39:52 +0800

On Wed, Jul 10, 2024 at 02:31:03PM -0700, Junio C Hamano wrote:
> shejialuo <[email-masked]> writes:
> 
> > Subject: Re: [GSoC][PATCH v10 06/10] builtin/refs: add verify subcommand and verbose_refs for "fsck_options"
> 
> Just saying
> 
> 	git refs: add verify subcommand
> 
> would be clearer.  If you really want to talk about two modes, you
> could say
> 
> 	git refs: add "verify [--strict|--verbose]" subcommand
> 
> but that may be too much.
> 

Thanks, I will change in the next version.

> > 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_refs" field into "fsck_options" to indicate whether we should
> > print verbose messages when checking refs consistency.
> 
> Is there a reason why this has to be verbose_refs and not a simple
> verbose bit?  When people see how it is useful to ask for the
> verbose output while checking refs, wouldn't people wish to add the
> same "--verbose" support while checking objects, and at that point,
> wouldn't it be awkward to add verbose_objs member to the struct and
> having to flip both bits on?
> 

Actually, this is really what I thought. I just want to provide more
find-grained control here. However, when I implemented the code, I also
felt awkward about this. I can't find the balance here.

I will improve this in the next version.

> > Mentored-by: Patrick Steinhardt <[email-masked]>
> > Mentored-by: Karthik Nayak <[email-masked]>
> > Signed-off-by: shejialuo <[email-masked]>
> > ---
> >  Documentation/git-refs.txt | 13 +++++++++++
> >  builtin/refs.c             | 44 ++++++++++++++++++++++++++++++++++++++
> >  fsck.h                     |  1 +
> >  3 files changed, 58 insertions(+)
> >
> > diff --git a/Documentation/git-refs.txt b/Documentation/git-refs.txt
> > index 5b99e04385..1244a85b64 100644
> > --- a/Documentation/git-refs.txt
> > +++ b/Documentation/git-refs.txt
> > @@ -10,6 +10,7 @@ SYNOPSIS
> >  --------
> >  [verse]
> >  'git refs migrate' --ref-format=<format> [--dry-run]
> > +'git refs verify' [--strict] [--verbose]
> >  
> >  DESCRIPTION
> >  -----------
> > @@ -22,6 +23,9 @@ COMMANDS
> >  migrate::
> >  	Migrate ref store between different formats.
> >  
> > +verify::
> > +	Verify reference database consistency.
> > +
> 
> The error reporting function for refs consistency check was still
> about reporting a problem for a single ref.  I am wondering how
> consistency violations that are not about a single ref should be
> handled.  For example, if refs/packed-backend.c:packed_fsck() finds
> that the file is not sorted properly or has some unparseable garbage
> in it, it is not something you can report as "refs/heads/main is
> broken", but those who are interested in seeing the "reference
> database consistency" verified, it is very much what they want the
> tool to notice.  How would detection of such a breakage that is not
> attributed to a single ref fit in this "ref consistency check
> infrastructure" that was introduced by [05/10]?
> 

Yes, I didn't consider other cases. Although I have said in the subject
that this series is to set up the infrastructure of fscking refs. It's
a little hard for me to set up a perfect "fsck_refs_report" at the
moment.

As you said, I think currently I should consider about the packed-refs
in this series. I will find a way to achieve this in the next version.
Well, I could say I intentionally ignored this problem. But we should
face the problem directly.

Really thanks.

> > +	argc = parse_options(argc, argv, prefix, options, verify_usage, 0);
> > +	if (argc)
> > +		usage(_("too many arguments"));
> 
> I do not think we want to change this line in this topic, but
> because I noticed that the issue is widespread, let me make a note
> here that we may want to clean up all the commands that give this
> message as a #leftoverbit item:
> 
>     $ git cmd foo baz
>     usage: too many arguments
> 
> is very unfriendly in that it is not immediately obvious to users
> which arguments are excess.  Should they have given "git cmd<RET>"?
> Is it "git cmd foo" that does not take any argument?
> 
> If you said something like
> 
>     $ git refs verify baz
>     error: 'git refs verify' takes no arguments
> 
> or even
> 
>     $ git refs verify baz
>     error: unknown argument 'baz' given to 'git refs verify'
> 
> it would be much more helpful.
> 

I will improve this in the next version.

> Thanks.