| Message ID | 3eff83d9ae14023f3527dfeb419cf8259f6d053d.1639758526.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Two small 'git repack' fixes | expand |
On Fri, Dec 17, 2021 at 04:28:46PM +0000, Derrick Stolee via GitGitGadget wrote: > From: Derrick Stolee <dstolee@microsoft.com> > > 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. 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. > This is difficult to test because the isatty(2) already prevents the > progess indicators from appearing when we redirect stderr to a file. You'd need test_terminal. Something like this: diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh index 8c4ba6500b..b673c49650 100755 --- a/t/t7700-repack.sh +++ b/t/t7700-repack.sh @@ -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
On Fri, Dec 17 2021, Derrick Stolee via GitGitGadget wrote: > From: Derrick Stolee <dstolee@microsoft.com> > > 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. > > Signed-off-by: Derrick Stolee <dstolee@microsoft.com> > --- > builtin/repack.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/builtin/repack.c b/builtin/repack.c > index 1f128b7c90b..c393a5db774 100644 > --- a/builtin/repack.c > +++ b/builtin/repack.c > @@ -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); This seems like a good change, but the documentation could really use an update (also before this change). It just says about -q: Pass the -q option to git pack-objects. See git-pack-objects(1). But do various things in "git-repack" itself differently if it's supplied.
On 12/17/2021 1:10 PM, Jeff King wrote: > On Fri, Dec 17, 2021 at 04:28:46PM +0000, Derrick Stolee via GitGitGadget wrote: >> This is difficult to test because the isatty(2) already prevents the >> progess indicators from appearing when we redirect stderr to a file. > > You'd need test_terminal. Something like this: > > diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh > index 8c4ba6500b..b673c49650 100755 > --- a/t/t7700-repack.sh > +++ b/t/t7700-repack.sh > @@ -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 Thanks. I added this test. When first running the test, it failed because I didn't have the IO::Pty Perl module installed. I'm not sure why I don't fail with other tests that use test_terminal. If someone knows more about what is going on, then maybe we need to expand the TTY prereq? Thanks, -Stolee
On 12/18/2021 4:55 AM, Ævar Arnfjörð Bjarmason wrote: > This seems like a good change, but the documentation could really use an > update (also before this change). It just says about -q: > > Pass the -q option to git pack-objects. See git-pack-objects(1). > > But do various things in "git-repack" itself differently if it's > supplied. Good point. I will update the docs. Thanks, -Stolee
On Mon, Dec 20, 2021 at 08:37:52AM -0500, Derrick Stolee wrote: > > +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 > > Thanks. I added this test. > > When first running the test, it failed because I didn't have the > IO::Pty Perl module installed. I'm not sure why I don't fail with > other tests that use test_terminal. If someone knows more about > what is going on, then maybe we need to expand the TTY prereq? 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
On 12/20/2021 8:49 AM, Jeff King wrote: > On Mon, Dec 20, 2021 at 08:37:52AM -0500, Derrick Stolee wrote: > >>> +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 >> >> Thanks. I added this test. >> >> When first running the test, it failed because I didn't have the >> IO::Pty Perl module installed. I'm not sure why I don't fail with >> other tests that use test_terminal. If someone knows more about >> what is going on, then maybe we need to expand the TTY prereq? > > 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? Ok, I got this same error and misread it as an error. The prereq is working just fine. Thanks for checking. -Stolee
diff --git a/builtin/repack.c b/builtin/repack.c index 1f128b7c90b..c393a5db774 100644 --- a/builtin/repack.c +++ b/builtin/repack.c @@ -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);