| Message ID | 20241208204733.304109-2-calumlikesapplepie@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | verify-pack: Fix documentation of --stat-only to reflect behavior | expand |
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");
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");
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(-)