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.

Thanks, -Stolee

Derrick Stolee (2):
repack: respect kept objects with '--write-midx -b'
repack: make '--quiet' disable progress

 builtin/repack.c  |  8 +++++---
 t/t7700-repack.sh | 15 +++++++++++++++
 2 files changed, 20 insertions(+), 3 deletions(-)

base-commit: 69a9c10c95e28df457e33b3c7400b16caf2e2962
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1098%2Fderrickstolee%2Frepack-fixes-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1098/derrickstolee/repack-fixes-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1098

gitgitgadget

From: Derrick Stolee <[email protected]>

Historically, we needed a single packfile in order to have reachability bitmaps. This introduced logic that when 'git repack' had a '-b' option that we should stop sending the '--honor-pack-keep' option to the 'git pack-objects' child process, ensuring that we create a packfile containing all reachable objects.

In the world of multi-pack-index bitmaps, we no longer need to repack all objects into a single pack to have valid bitmaps. Thus, we should continue sending the '--honor-pack-keep' flag to 'git pack-objects'.

The fix is very simple: only disable the flag when writing bitmaps but also _not_ writing the multi-pack-index.

This opens the door to new repacking strategies that might want to keep some historical set of objects in a stable pack-file while only repacking more recent objects.

---
 builtin/repack.c  |  2 +-
 t/t7700-repack.sh | 15 +++++++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

index 9b0be6a6ab3..1f128b7c90b 100644
@@ -693,7 +693,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 		write_bitmaps = 0;
 	}
 	if (pack_kept_objects < 0)
-		pack_kept_objects = write_bitmaps > 0;
+		pack_kept_objects = write_bitmaps > 0 && !write_midx;
 
 	if (write_bitmaps && !(pack_everything & ALL_INTO_ONE) && !write_midx)
 		die(_(incremental_bitmap_conflict_error));

index 0260ad6f0e0..8c4ba6500be 100755
@@ -372,4 +372,19 @@ 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\"
+	)
+'
+
 test_done

gitgitgadget

That all makes sense. Another way of thinking about it: we disable --honor-pack-keep so we that keep packs do not interfere with writing a pack bitmap. But with --write-midx, we skip the pack bitmap entirely.

In the end it's the same, but I wanted to emphasize that the important hing is not so much that we are writing a midx bitmap as that we are _not_ writing a pack bitmap. And that is what makes this OK to do (that the repack code already disables the pack bitmap when writing a midx one).

  test_expect_success '--write-midx -b packs non-kept objects' '
          GIT_TRACE2_EVENT="$(pwd)/midx-keep-bitmap.trace" \
                  git -C midx repack --write-midx -a -b &&
          grep "\"event\":\"start\".*\"pack-objects\".*\"--honor-pack-keep\"" \
                  midx-keep-bitmap.trace
  '

  check_trace2_arg () {
	grep "\"event\":\"start\".*\"$2\".*\"$3\"" "$1"

...

Thanks for the suggestions. I decided to adapt the 'test_subcommand' helper into a 'test_subcommand_inexact' helper. The existing helper requires the full argument list in exact order, but the new one only cares about the given arguments (in that relative order).

Thanks,
-Stolee

Heh, I should have looked to see if we had faced this problem before. Extending that family of helpers is better still than my suggestion.

-Peff

From: Derrick Stolee <[email protected]>

While testing some ideas in 'git repack', I ran it with '--quiet' and discovered that some progress output was still shown. Specifically, the output for writing the multi-pack-index showed the progress.

The 'show_progress' variable in cmd_repack() is initialized with isatty(2) and is not modified at all by the '--quiet' flag. The '--quiet' flag modifies the po_args.quiet option which is translated into a '--quiet' flag for the 'git pack-objects' child process. However, 'show_progress' is used to directly send progress information to the multi-pack-index writing logic which does not use a child process.

The fix here is to modify 'show_progress' to be false if po_opts.quiet 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.

---
 builtin/repack.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

index 1f128b7c90b..c393a5db774 100644
@@ -612,7 +612,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	struct tempfile *refs_snapshot = NULL;
 	int i, ext, ret;
 	FILE *out;
-	int show_progress = isatty(2);
+	int show_progress;
 
 	/* variables to be filled by option parsing */
 	int pack_everything = 0;

@@ -725,6 +725,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 
 	prepare_pack_objects(&cmd, &po_args);
 
+	show_progress = !po_args.quiet && isatty(2);
+
 	strvec_push(&cmd.args, "--keep-true-parents");
 	if (!pack_kept_objects)
 		strvec_push(&cmd.args, "--honor-pack-keep");

@@ -926,7 +928,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 			}
 			strbuf_release(&buf);
 		}
-		if (!po_args.quiet && show_progress)
+		if (show_progress)
 			opts |= PRUNE_PACKED_VERBOSE;
 		prune_packed_objects(opts);
 

gitgitgadget

Makes sense. I wondered if you might have to decide what to do with "--progress --quiet", but we do not have an explicit progress option for git-repack in the first place.

You'd need test_terminal. Something like this:

index 8c4ba6500b..b673c49650 100755
@@ -5,6 +5,7 @@ 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 &&

@@ -387,4 +388,10 @@ test_expect_success '--write-midx -b packs non-kept objects' '
 	)
 '
 
+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

-Peff

Weird. I uninstalled IO::Pty, and get:

checking prerequisite: TTY

[...prereq code...]

+ perl /home/peff/compile/git/t/test-terminal.perl sh -c test -t 1 && test -t 2
+ command /usr/bin/perl /home/peff/compile/git/t/test-terminal.perl sh -c test -t 1 && test -t 2 Can't locate IO/Pty.pm in @INC (you may need to install the IO::Pty module) (@INC contains: [...etc...] BEGIN failed--compilation aborted at /home/peff/compile/git/t/test-terminal.perl line 5. prerequisite TTY not satisfied
ok 25 # skip --quiet disables progress (missing TTY)

What does running with "-x -i" say for the prereq?

-Peff

Ok, I got this same error and misread it as an error. The prereq is working just fine. Thanks for checking.

-Stolee

Good point. I will update the docs.

Thanks,
-Stolee

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

From: Derrick Stolee <[email protected]>

Historically, we needed a single packfile in order to have reachability bitmaps. This introduced logic that when 'git repack' had a '-b' option that we should stop sending the '--honor-pack-keep' option to the 'git pack-objects' child process, ensuring that we create a packfile containing all reachable objects.

In the world of multi-pack-index bitmaps, we no longer need to repack all objects into a single pack to have valid bitmaps. Thus, we should continue sending the '--honor-pack-keep' flag to 'git pack-objects'.

The fix is very simple: only disable the flag when writing bitmaps but also _not_ writing the multi-pack-index.

This opens the door to new repacking strategies that might want to keep 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.

---
 builtin/repack.c        |  2 +-
 t/t7700-repack.sh       |  6 ++++++
 t/test-lib-functions.sh | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 41 insertions(+), 1 deletion(-)

index 9b0be6a6ab3..1f128b7c90b 100644
@@ -693,7 +693,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 		write_bitmaps = 0;
 	}
 	if (pack_kept_objects < 0)
-		pack_kept_objects = write_bitmaps > 0;
+		pack_kept_objects = write_bitmaps > 0 && !write_midx;
 
 	if (write_bitmaps && !(pack_everything & ALL_INTO_ONE) && !write_midx)
 		die(_(incremental_bitmap_conflict_error));

index 0260ad6f0e0..63c9a247f57 100755
@@ -372,4 +372,10 @@ test_expect_success '--write-midx with preferred bitmap tips' '
 	)
 '
 
+test_expect_success '--write-midx -b packs non-kept objects' '
+	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

index 389153e5916..c3d38aaccbd 100644
@@ -1759,6 +1759,40 @@ 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.
 #

gitgitgadget

From: Derrick Stolee <[email protected]>

While testing some ideas in 'git repack', I ran it with '--quiet' and discovered that some progress output was still shown. Specifically, the output for writing the multi-pack-index showed the progress.

The 'show_progress' variable in cmd_repack() is initialized with isatty(2) and is not modified at all by the '--quiet' flag. The '--quiet' flag modifies the po_args.quiet option which is translated into a '--quiet' flag for the 'git pack-objects' child process. However, 'show_progress' is used to directly send progress information to the multi-pack-index writing logic which does not use a child process.

The fix here is to modify 'show_progress' to be false if po_opts.quiet is true, and isatty(2) otherwise. This new expectation simplifies a later condition that checks both.

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.

---
 Documentation/git-repack.txt | 5 +++--
 builtin/repack.c             | 6 ++++--
 t/t7700-repack.sh            | 7 +++++++
 3 files changed, 14 insertions(+), 4 deletions(-)

index 7183fb498f4..ee30edc178a 100644
@@ -76,8 +76,9 @@ 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

index 1f128b7c90b..c393a5db774 100644
@@ -612,7 +612,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	struct tempfile *refs_snapshot = NULL;
 	int i, ext, ret;
 	FILE *out;
-	int show_progress = isatty(2);
+	int show_progress;
 
 	/* variables to be filled by option parsing */
 	int pack_everything = 0;

@@ -725,6 +725,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 
 	prepare_pack_objects(&cmd, &po_args);
 
+	show_progress = !po_args.quiet && isatty(2);
+
 	strvec_push(&cmd.args, "--keep-true-parents");
 	if (!pack_kept_objects)
 		strvec_push(&cmd.args, "--honor-pack-keep");

@@ -926,7 +928,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 			}
 			strbuf_release(&buf);
 		}
-		if (!po_args.quiet && show_progress)
+		if (show_progress)
 			opts |= PRUNE_PACKED_VERBOSE;
 		prune_packed_objects(opts);
 

index 63c9a247f57..fe2b493d0ee 100755
@@ -5,6 +5,7 @@ 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 &&

@@ -378,4 +379,10 @@ 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

This iteration looks good to me.

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.