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

[Junio C Hamano <gitster@pobox.com>]

shejialuo <shejialuo@gmail.com> 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.

> 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?

> Mentored-by: Patrick Steinhardt <ps@pks.im>
> Mentored-by: Karthik Nayak <karthik.188@gmail.com>
> Signed-off-by: shejialuo <shejialuo@gmail.com>
> ---
>  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]?

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


Thanks.