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

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

[calumlikesapplepie]

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.

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

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.

> static int verify_one_pack(... ) { ...
> 	strvec_push(argv, "index-pack");
>
> 	if (stat_only)
> 		strvec_push(argv, "--verify-stat-only");
> 	else if (verbose)
> 		strvec_push(argv, "--verify-stat");
> 	else
> 		strvec_push(argv, "--verify");

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.

> dd if=/dev/urandom of=test123.rand count=10 bs=10M
> git init; git add .; git commit -am "test"
> git gc
> time git verify-pack .git/objects/pack/PACKFILE.idx
> time git verify-pack --stat-only .git/objects/pack/PACKFILE.idx

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.

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

[Calum McConnell]

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(-)

diff --git a/Documentation/git-verify-pack.txt b/Documentation/git-verify-pack.txt
index d7e8869..f734e90 100644
--- a/Documentation/git-verify-pack.txt
+++ b/Documentation/git-verify-pack.txt
@@ -30,8 +30,8 @@ OPTIONS
 
 -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.
 
 \--::
 	Do not interpret any more arguments as options.
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");
 
-- 
2.45.2

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

[Junio C Hamano]

Calum McConnell <calumlikesapplepie@gmail.com> 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");

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

[A bughunter]

[-- Attachment #1: Type: text/plain, Size: 1382 bytes --]

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

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.


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

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

[-- Attachment #2: publickey - A_bughunter@proton.me - 0x66540805.asc --]
[-- Type: application/pgp-keys, Size: 653 bytes --]

[-- Attachment #3: publickey - A_bughunter@proton.me - 0x66540805.asc.sig --]
[-- Type: application/pgp-signature, Size: 119 bytes --]