Re: [PATCH] verify-pack: Fix documentation of --stat-only to reflect behavior

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 9, 2024, 00:51 UTC
Message-ID: <xmqq7c89r853.fsf@gitster.g>
In-Reply-To: <20241208204733.304109-2-calumlikesapplepie@gmail.com>

Calum McConnell <calumlikesapplepie@gmail.com> writes:

Show 7 quoted lines

> Ever since verify-pack was refactored to use `index-pack.c` in commit
> 3de89c9 (verify-pack: use index-pack --verify, 2011-06-06), the
> --stat-only option has been verifying the full pack, rather than just
> reading the index file, as it was originally documented to do.
>
> Allowing users to get details of packed objects rapidly without
> needing to hash all the objects in packfile is a useful ability.

Thanks for noticing.

> However, implementing that ability would require more changes to index-pack
> than the author is able to do at this time, and so a quick fix to simply
> update the documentation to reflect current behavior is done instead.

Wouldn't it etch the "wrong" behaviour even more strongly into stone, making future fixes harder, though?

> This commit also re-orders the if-else block, to ensure that if both
> --stat-only and --verbose are specified, the verbose details are provided.
> This fixes another longstanding documentation bug with `verify-pack`.

This part is puzzling. My understanding is that a documentation bug would be fixed by adjusting the documentation to reality, so a change to the code would not be involved.

Is this closer to what is happening?

 - There are two gotchas that the actual behaviour and the
   documentation do not match.

 - "--stat-only" being described as "quickly count without
   verifying" but doing a lot more than statistics gathering is one.
   This is "fixed" by updating the documentation to match the
   implemented behaviour.

 - "--verbose" is documented to be verbose even when given together
   with "--stat-only", but when "--stat-only" is given, it is
   ignored.  This is "fixed" by updating the behaviour to match the
   documentation.

But the thing is, the third point, the second "fix", to allow you to treat "-v -s" or "-s -v" as if they were "-v" comes from the second sentence in this paragraph:

        -s::
        --stat-only::
                Do not verify the pack contents; only show the histogram of delta
                chain length.  With `--verbose`, the list of objects is also shown.

But ...

Show 6 quoted lines

>  -s::
>  --stat-only::
> -	Do not verify the pack contents; only show the histogram of delta
> -	chain length.  With `--verbose`, the list of objects is also shown.
> +	As --verbose, but only show the histogram of delta
> +	chain length.

... this change loses the "list of objects is also shown", which I think is the justification for passing "--verify-stat" when both are given.

So, I dunno.

Show 17 quoted lines

> diff --git a/builtin/verify-pack.c b/builtin/verify-pack.c
> index 34e4ed7..5860a96 100644
> --- a/builtin/verify-pack.c
> +++ b/builtin/verify-pack.c
> @@ -20,10 +20,10 @@ static int verify_one_pack(const char *path, unsigned int flags, const char *has
>  
>  	strvec_push(argv, "index-pack");
>  
> -	if (stat_only)
> -		strvec_push(argv, "--verify-stat-only");
> -	else if (verbose)
> +	if (verbose)
>  		strvec_push(argv, "--verify-stat");
> +	else if (stat_only)
> +		strvec_push(argv, "--verify-stat-only");
>  	else
>  		strvec_push(argv, "--verify");