On Wed, Jul 10, 2024 at 02:31:03PM -0700, Junio C Hamano wrote:
> shejialuo <shejialuo@xxxxxxxxx> 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 <ps@xxxxxx>
> > Mentored-by: Karthik Nayak <karthik.188@xxxxxxxxx>
> > Signed-off-by: shejialuo <shejialuo@xxxxxxxxx>
> > ---
> > 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.
