From: Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>
Date: Mon, 20 Dec 2021 14:48:09 GMT
Subject: [PATCH v2 0/2] Two small 'git repack' fixes
Message-ID: <pull.1098.v2.git.1640011691.gitgitgadget@gmail.com>
In-Reply-To: <pull.1098.git.1639758526.gitgitgadget@gmail.com>

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

-- 
gitgitgadget