| Message ID | pull.1098.v2.git.1640011691.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Two small 'git repack' fixes | expand |
On Mon, Dec 20 2021, Derrick Stolee via GitGitGadget wrote: > I was experimenting with some ideas in 'git repack' and discovered these two > bugs. This iteration looks good to me. > + ## Documentation/git-repack.txt ## > +@@ Documentation/git-repack.txt: to the new separate pack will be written. > + linkgit:git-pack-objects[1]. > + > + -q:: > +- Pass the `-q` option to 'git pack-objects'. See > +- linkgit:git-pack-objects[1]. > ++--quiet:: > ++ Show no progress over the standard error stream and pass the `-q` > ++ option to 'git pack-objects'. See linkgit:git-pack-objects[1]. > + > + -n:: > + Do not update the server information with > + Nit: I think the addition of --quiet here is good, but also wouldn't mind a prep change to add it as a separate step. FWIW it appears to have been an unintended/hidden effect of a1bbc6c0176 (repack: rewrite the shell script in C, 2013-09-15), i.e. we started using OPT__QUIET() which added "--quiet", but the shellscript just supported -q. Here we update the docs, but the SYNOPSIS stillj just lists -q. I think all of that's fine & this v2 is good enough. Just braindumping notes/observations while reading along/checking the v1->v2 diff.
I was experimenting with some ideas in 'git repack' and discovered these two bugs. The first is a "real" bug in that it repacks much more data than is necessary when repacking with '--write-midx -b' and there exists a .keep pack. The fix is simple, which is to change a condition that was added for the '-b' case before '--write-midx' existed. The second is a UX bug in that '--quiet' did not disable all progress, at least when stderr was interactive. Updates in v2 ============= Thanks for the quick review! * Test for --honor-pack-keep is cleaner with a reusable helper. * TTY test is added. * Docs are updated. Thanks, -Stolee Derrick Stolee (2): repack: respect kept objects with '--write-midx -b' repack: make '--quiet' disable progress Documentation/git-repack.txt | 5 +++-- builtin/repack.c | 8 +++++--- t/t7700-repack.sh | 13 +++++++++++++ t/test-lib-functions.sh | 34 ++++++++++++++++++++++++++++++++++ 4 files changed, 55 insertions(+), 5 deletions(-) base-commit: 69a9c10c95e28df457e33b3c7400b16caf2e2962 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1098%2Fderrickstolee%2Frepack-fixes-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1098/derrickstolee/repack-fixes-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/1098 Range-diff vs v1: 1: 1ed91f6d255 ! 1: 747328a4dd6 repack: respect kept objects with '--write-midx -b' @@ Commit message some historical set of objects in a stable pack-file while only repacking more recent objects. + To test, create a new 'test_subcommand_inexact' helper that is more + flexible than 'test_subcommand'. This allows us to look for the + --honor-pack-keep flag without over-indexing on the exact set of + arguments. + Signed-off-by: Derrick Stolee <dstolee@microsoft.com> ## builtin/repack.c ## @@ t/t7700-repack.sh: test_expect_success '--write-midx with preferred bitmap tips' ' +test_expect_success '--write-midx -b packs non-kept objects' ' -+ git init midx-kept && -+ test_when_finished "rm -fr midx-kept" && -+ ( -+ cd midx-kept && -+ test_commit_bulk 100 && -+ GIT_TRACE2_EVENT="$(pwd)/trace.txt" \ -+ git repack --write-midx -a -b && -+ cat trace.txt | \ -+ grep \"event\":\"start\" | \ -+ grep pack-objects | \ -+ grep \"--honor-pack-keep\" -+ ) ++ GIT_TRACE2_EVENT="$(pwd)/trace.txt" \ ++ git repack --write-midx -a -b && ++ test_subcommand_inexact git pack-objects --honor-pack-keep <trace.txt +' + test_done + + ## t/test-lib-functions.sh ## +@@ t/test-lib-functions.sh: test_subcommand () { + fi + } + ++# Check that the given command was invoked as part of the ++# trace2-format trace on stdin, but without an exact set of ++# arguments. ++# ++# test_subcommand [!] <command> <args>... < <trace> ++# ++# For example, to look for an invocation of "git pack-objects" ++# with the "--honor-pack-keep" argument, use ++# ++# GIT_TRACE2_EVENT=event.log git repack ... && ++# test_subcommand git pack-objects --honor-pack-keep <event.log ++# ++# If the first parameter passed is !, this instead checks that ++# the given command was not called. ++# ++test_subcommand_inexact () { ++ local negate= ++ if test "$1" = "!" ++ then ++ negate=t ++ shift ++ fi ++ ++ local expr=$(printf '"%s".*' "$@") ++ expr="${expr%,}" ++ ++ if test -n "$negate" ++ then ++ ! grep "\"event\":\"child_start\".*\[$expr\]" ++ else ++ grep "\"event\":\"child_start\".*\[$expr\]" ++ fi ++} ++ + # Check that the given command was invoked as part of the + # trace2-format trace on stdin. + # 2: 3eff83d9ae1 ! 2: 41260bf0829 repack: make '--quiet' disable progress @@ Commit message is true, and isatty(2) otherwise. This new expectation simplifies a later condition that checks both. - This is difficult to test because the isatty(2) already prevents the - progess indicators from appearing when we redirect stderr to a file. + Update the documentation to make it clear that '-q' will disable all + progress in addition to ensuring the 'git pack-objects' child process + will receive the flag. + Use 'test_terminal' to check that this works to get around the isatty(2) + check. + + Helped-by: Jeff King <peff@peff.net> Signed-off-by: Derrick Stolee <dstolee@microsoft.com> + ## Documentation/git-repack.txt ## +@@ Documentation/git-repack.txt: to the new separate pack will be written. + linkgit:git-pack-objects[1]. + + -q:: +- Pass the `-q` option to 'git pack-objects'. See +- linkgit:git-pack-objects[1]. ++--quiet:: ++ Show no progress over the standard error stream and pass the `-q` ++ option to 'git pack-objects'. See linkgit:git-pack-objects[1]. + + -n:: + Do not update the server information with + ## builtin/repack.c ## @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix) struct tempfile *refs_snapshot = NULL; @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix opts |= PRUNE_PACKED_VERBOSE; prune_packed_objects(opts); + + ## t/t7700-repack.sh ## +@@ t/t7700-repack.sh: test_description='git repack works correctly' + . ./test-lib.sh + . "${TEST_DIRECTORY}/lib-bitmap.sh" + . "${TEST_DIRECTORY}/lib-midx.sh" ++. "${TEST_DIRECTORY}/lib-terminal.sh" + + commit_and_pack () { + test_commit "$@" 1>&2 && +@@ t/t7700-repack.sh: test_expect_success '--write-midx -b packs non-kept objects' ' + test_subcommand_inexact git pack-objects --honor-pack-keep <trace.txt + ' + ++test_expect_success TTY '--quiet disables progress' ' ++ test_terminal env GIT_PROGRESS_DELAY=0 \ ++ git -C midx repack -ad --quiet --write-midx 2>stderr && ++ test_must_be_empty stderr ++' ++ + test_done