{"thread":{"id":"62614","subject":"BUG: git verify-pack --stat-only is nonfunctional as documented","startedAt":"2024-12-08T20:40:01Z","lastAt":"2024-12-11T06:14:52Z","messageCount":4,"participants":["calumlikesapplepie@gmail.com","Calum McConnell","Junio C Hamano","A bughunter"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"508837","messageId":"1ee9f3ef2bffd148b6225138135462d2d4a5928d.camel@gmail.com","threadId":"62614","inReplyTo":null,"subject":"BUG: git verify-pack --stat-only is nonfunctional as documented","fromName":"","fromEmail":"calumlikesapplepie@gmail.com","sentAt":"2024-12-08T20:39:58Z","receivedAt":"2024-12-08T20:40:01Z","isPatch":false,"sender":{"key":"calumlikesapplepie@gmail.com","avatar":null},"body":"Hello maintainers,\n\nThere are two problems with `git verify-pack --stat-only`.  The first\none I noticed is that it does not work as specified when the --verbose\noption is passed.  The second, and more serious, is that it simply\ndoesn't work in general; `verify-pack` runs at the same speed\nregardlesds of if `--stat-only` is specified.\n\nThe manpage of `git verify-pack` specifies that when both the \n`--verbose` and `--stat-only` options are passed, that the command\noutputs both a complete list of objects and a histogram.\n\n> -s, --stat-only\n>       Do not verify the pack contents; only show the histogram \n> \tof delta chain length. With --verbose, the list of objects\n>\tis also shown.\n\nHowever, running `git verify-pack -sv` only outputs the histogram.\nExamining the source code reveals that this is the expected behavior in\nall cases; if --stat-only is specified, --verbose is ignored.\n\n> static int verify_one_pack(... ) { ...\n> \tstrvec_push(argv, \"index-pack\");\n>\n> \tif (stat_only)\n> \t\tstrvec_push(argv, \"--verify-stat-only\");\n> \telse if (verbose)\n> \t\tstrvec_push(argv, \"--verify-stat\");\n> \telse\n> \t\tstrvec_push(argv, \"--verify\");\n\nWhile trying to determine how to patch this function and `index-pack.c`\nto support the manpage specified behavior, I realized that I couldn't\neven locate where --verify-stat-only prevented the hashing of the full\npack file; the `stat_only` variable only serves to prevent printing of\nindividual object information.  Timing data confirms that `--stat-only`\ndoes not prevent verifying the packfiles; the following test case shows\neither command taking about a second to run on my machine.\n\n> dd if=/dev/urandom of=test123.rand count=10 bs=10M\n> git init; git add .; git commit -am \"test\"\n> git gc\n> time git verify-pack .git/objects/pack/PACKFILE.idx\n> time git verify-pack --stat-only .git/objects/pack/PACKFILE.idx\n\nBoth issues were likley added when `verify-pack` was refactored to call\n`index-pack`.  It may be simpler to edit the manpage to reflect the\ncurrent behavior, rather than conduct the needed refactoring of `index-\npack`.  A patch that does this is incoming..\n\nThank you,\nCalum McConnell\n\nP.S. Sorry if this message seemed rude or too short, it's the second\ntime I've written it, since Evolution decided to discard my message\ntext.\n"},{"id":"508838","messageId":"20241208204733.304109-2-calumlikesapplepie@gmail.com","threadId":"62614","inReplyTo":"1ee9f3ef2bffd148b6225138135462d2d4a5928d.camel@gmail.com","subject":"[PATCH] verify-pack: Fix documentation of --stat-only to reflect behavior","fromName":"Calum McConnell","fromEmail":"calumlikesapplepie@gmail.com","sentAt":"2024-12-08T20:47:17Z","receivedAt":"2024-12-08T20:48:25Z","isPatch":true,"sender":{"key":"calumlikesapplepie@gmail.com","avatar":null},"body":"Ever since verify-pack was refactored to use `index-pack.c` in commit\n3de89c9 (verify-pack: use index-pack --verify, 2011-06-06), the\n--stat-only option has been verifying the full pack, rather than just\nreading the index file, as it was originally documented to do.\n\nAllowing users to get details of packed objects rapidly without\nneeding to hash all the objects in packfile is a useful ability.\nInterested consumers could use such data to more rapidly estimate the\neffectiveness of git's compression, such as to determine if their\n.gitignore is adequate, or if they should be removing additional files.\nHowever, implementing that ability would require more changes to index-pack\nthan the author is able to do at this time, and so a quick fix to simply\nupdate the documentation to reflect current behavior is done instead.\n\nThis commit also re-orders the if-else block, to ensure that if both\n--stat-only and --verbose are specified, the verbose details are provided.\nThis fixes another longstanding documentation bug with `verify-pack`.\n\nSigned-off-by: Calum McConnell <calumlikesapplepie@gmail.com>\n---\n Documentation/git-verify-pack.txt | 4 ++--\n builtin/verify-pack.c             | 6 +++---\n 2 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-verify-pack.txt b/Documentation/git-verify-pack.txt\nindex d7e8869..f734e90 100644\n--- a/Documentation/git-verify-pack.txt\n+++ b/Documentation/git-verify-pack.txt\n@@ -30,8 +30,8 @@ OPTIONS\n \n -s::\n --stat-only::\n-\tDo not verify the pack contents; only show the histogram of delta\n-\tchain length.  With `--verbose`, the list of objects is also shown.\n+\tAs --verbose, but only show the histogram of delta\n+\tchain length.\n \n \\--::\n \tDo not interpret any more arguments as options.\ndiff --git a/builtin/verify-pack.c b/builtin/verify-pack.c\nindex 34e4ed7..5860a96 100644\n--- a/builtin/verify-pack.c\n+++ b/builtin/verify-pack.c\n@@ -20,10 +20,10 @@ static int verify_one_pack(const char *path, unsigned int flags, const char *has\n \n \tstrvec_push(argv, \"index-pack\");\n \n-\tif (stat_only)\n-\t\tstrvec_push(argv, \"--verify-stat-only\");\n-\telse if (verbose)\n+\tif (verbose)\n \t\tstrvec_push(argv, \"--verify-stat\");\n+\telse if (stat_only)\n+\t\tstrvec_push(argv, \"--verify-stat-only\");\n \telse\n \t\tstrvec_push(argv, \"--verify\");\n \n-- \n2.45.2\n\n"},{"id":"508841","messageId":"xmqq7c89r853.fsf@gitster.g","threadId":"62614","inReplyTo":"20241208204733.304109-2-calumlikesapplepie@gmail.com","subject":"Re: [PATCH] verify-pack: Fix documentation of --stat-only to reflect behavior","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-09T00:51:20Z","receivedAt":"2024-12-09T00:51:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Calum McConnell <calumlikesapplepie@gmail.com> writes:\n\n> Ever since verify-pack was refactored to use `index-pack.c` in commit\n> 3de89c9 (verify-pack: use index-pack --verify, 2011-06-06), the\n> --stat-only option has been verifying the full pack, rather than just\n> reading the index file, as it was originally documented to do.\n>\n> Allowing users to get details of packed objects rapidly without\n> needing to hash all the objects in packfile is a useful ability.\n\nThanks for noticing.\n\n> However, implementing that ability would require more changes to index-pack\n> than the author is able to do at this time, and so a quick fix to simply\n> update the documentation to reflect current behavior is done instead.\n\nWouldn't it etch the \"wrong\" behaviour even more strongly into\nstone, making future fixes harder, though?\n\n> This commit also re-orders the if-else block, to ensure that if both\n> --stat-only and --verbose are specified, the verbose details are provided.\n> This fixes another longstanding documentation bug with `verify-pack`.\n\nThis part is puzzling.  My understanding is that a documentation bug\nwould be fixed by adjusting the documentation to reality, so a\nchange to the code would not be involved.\n\nIs this closer to what is happening?\n\n - There are two gotchas that the actual behaviour and the\n   documentation do not match.\n\n - \"--stat-only\" being described as \"quickly count without\n   verifying\" but doing a lot more than statistics gathering is one.\n   This is \"fixed\" by updating the documentation to match the\n   implemented behaviour.\n\n - \"--verbose\" is documented to be verbose even when given together\n   with \"--stat-only\", but when \"--stat-only\" is given, it is\n   ignored.  This is \"fixed\" by updating the behaviour to match the\n   documentation.\n\nBut the thing is, the third point, the second \"fix\", to allow you to\ntreat \"-v -s\" or \"-s -v\" as if they were \"-v\" comes from the second\nsentence in this paragraph:\n\n        -s::\n        --stat-only::\n                Do not verify the pack contents; only show the histogram of delta\n                chain length.  With `--verbose`, the list of objects is also shown.\n\nBut ...\n\n>  -s::\n>  --stat-only::\n> -\tDo not verify the pack contents; only show the histogram of delta\n> -\tchain length.  With `--verbose`, the list of objects is also shown.\n> +\tAs --verbose, but only show the histogram of delta\n> +\tchain length.\n\n... this change loses the \"list of objects is also shown\", which I\nthink is the justification for passing \"--verify-stat\" when both are\ngiven.\n\nSo, I dunno.\n\n> diff --git a/builtin/verify-pack.c b/builtin/verify-pack.c\n> index 34e4ed7..5860a96 100644\n> --- a/builtin/verify-pack.c\n> +++ b/builtin/verify-pack.c\n> @@ -20,10 +20,10 @@ static int verify_one_pack(const char *path, unsigned int flags, const char *has\n>  \n>  \tstrvec_push(argv, \"index-pack\");\n>  \n> -\tif (stat_only)\n> -\t\tstrvec_push(argv, \"--verify-stat-only\");\n> -\telse if (verbose)\n> +\tif (verbose)\n>  \t\tstrvec_push(argv, \"--verify-stat\");\n> +\telse if (stat_only)\n> +\t\tstrvec_push(argv, \"--verify-stat-only\");\n>  \telse\n>  \t\tstrvec_push(argv, \"--verify\");\n"},{"id":"508945","messageId":"08q-qTFvcLocW4XT7FvsEByB5_1V1a1gGkKOtPQL-3BO2ILt9z2Xt29LFd10bqUyfS5KzDVRmPN1KfIeaLOCw0NeNJoQgxAfaELOLQ9UFEc=@proton.me","threadId":"62614","inReplyTo":"1ee9f3ef2bffd148b6225138135462d2d4a5928d.camel@gmail.com","subject":"Re: BUG: git verify-pack --stat-only is nonfunctional as documented","fromName":"A bughunter","fromEmail":"a_bughunter@proton.me","sentAt":"2024-12-11T06:14:41Z","receivedAt":"2024-12-11T06:14:52Z","isPatch":false,"sender":{"key":"a_bughunter@proton.me","avatar":null},"body":"-----BEGIN PGP SIGNED MESSAGE-----\nHash: SHA512\n\n\n\n\nMy reply to Çalum is interspersed sparse quotes bottom posting. \n\nfrom A_bughunter@proton.me\n\nSent with Proton Mail secure email.\n\nOn Sunday, December 8th, 2024 at 20:39, calumlikesapplepie@gmail.com <calumlikesapplepie@gmail.com> wrote:\n> \n> Both issues were likley added when `verify-pack` was refactored to call\n> `index-pack`. It may be simpler to edit the manpage to reflect the\n> current behavior, rather than conduct the needed refactoring of `index- pack`. A patch that does this is incoming..\n\nYou 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.\n\n\n> Thank you,\n> Calum McConnell\n> \n> P.S. Sorry if this message seemed rude or too short, it's the second\n> time I've written it, since Evolution decided to discard my message\n> text.\n\nYeah 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. \n-----BEGIN PGP SIGNATURE-----\nVersion: ProtonMail\n\nwnUEARYKACcFgmdZLc0JkKkWZTlQrvKZFiEEZlQIBcAycZ2lO9z2qRZlOVCu\n8pkAALUWAP9e3LHx+d8m8f87v3tGNw5lO4SvGIGFG2TlFIYdhHifEwEA25ce\nfkfmYQNa3esNYmqbTbdVL3Fr02y1PYzkJhYgCgk=\n=EYvS\n-----END PGP SIGNATURE-----\n"}]}