Two small 'git repack' fixes

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 <[email protected]>
      
       ## 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 <[email protected]>

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