Calum McConnell <calumlikesapplepie@xxxxxxxxx> writes:
> 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 ...
> -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.
> 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");
