BUG: git verify-pack --stat-only is nonfunctional as documented

Hello maintainers,

There are two problems with `git verify-pack --stat-only`. The first one I noticed is that it does not work as specified when the --verbose option is passed. The second, and more serious, is that it simply doesn't work in general; `verify-pack` runs at the same speed regardlesds of if `--stat-only` is specified.

The manpage of `git verify-pack` specifies that when both the  `--verbose` and `--stat-only` options are passed, that the command outputs both a complete list of objects and a histogram.

However, running `git verify-pack -sv` only outputs the histogram. Examining the source code reveals that this is the expected behavior in all cases; if --stat-only is specified, --verbose is ignored.

While trying to determine how to patch this function and `index-pack.c` to support the manpage specified behavior, I realized that I couldn't even locate where --verify-stat-only prevented the hashing of the full pack file; the `stat_only` variable only serves to prevent printing of individual object information. Timing data confirms that `--stat-only` does not prevent verifying the packfiles; the following test case shows either command taking about a second to run on my machine.

Both issues were likley added when `verify-pack` was refactored to call `index-pack`. It may be simpler to edit the manpage to reflect the current behavior, rather than conduct the needed refactoring of `index- pack`. A patch that does this is incoming..

Thank you, Calum McConnell

P.S. Sorry if this message seemed rude or too short, it's the second time I've written it, since Evolution decided to discard my message text.

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. Interested consumers could use such data to more rapidly estimate the effectiveness of git's compression, such as to determine if their .gitignore is adequate, or if they should be removing additional files. 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.

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

Signed-off-by: Calum McConnell <calumlikesapplepie@gmail.com>
---
 Documentation/git-verify-pack.txt | 4 ++--
 builtin/verify-pack.c             | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

Calum McConnell <calumlikesapplepie@gmail.com> writes:

Thanks for noticing.

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

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

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

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

My reply to Çalum is interspersed sparse quotes bottom posting. 

from A_bughunter@proton.me

Sent with Proton Mail secure email.

On Sunday, December 8th, 2024 at 20:39, calumlikesapplepie@gmail.com <calumlikesapplepie@gmail.com> wrote:

You know I say documentations is a good half the application. It may be easier to make the good half reflect the other but it may be better that the other reflect the good half.

Yeah doesn't that urk you? Imagine using android where buttons get pressed by accident, smart algorithims change focus, and latency makes taps miss and buttons move. Terrible. 
-----BEGIN PGP SIGNATURE-----
Version: ProtonMail

wnUEARYKACcFgmdZLc0JkKkWZTlQrvKZFiEEZlQIBcAycZ2lO9z2qRZlOVCu 8pkAALUWAP9e3LHx+d8m8f87v3tGNw5lO4SvGIGFG2TlFIYdhHifEwEA25ce fkfmYQNa3esNYmqbTbdVL3Fr02y1PYzkJhYgCgk= =EYvS -----END PGP SIGNATURE-----