| Message ID | d1405b781915c085ac8a8965dadf3efbe1b0f6aa.1629915330.git.matheus.bernardino@usp.br (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | checkout: make delayed checkout respect --quiet and --no-progress | expand |
On Wed, Aug 25 2021, Matheus Tavares wrote: > The test section of the patch is a bit long because it checks all the > verbosity options related to the progress report, and it also tests both > the check_updates() and checkout_worktree() code paths. If that is > overkill, I can remove some tests. More exhaustive tests are generally nice.. > +test_expect_success PERL 'setup for progress tests' ' > + git init progress && > + ( > + cd progress && > + git config filter.delay.process "rot13-filter.pl delay-progress.log clean smudge delay" && > + git config filter.delay.required true && > + > + echo "*.a filter=delay" >.gitattributes && > + touch test-delay10.a && > + git add . && > + git commit -m files > + ) > +' This doesn't seem to depend on PERL, should this really be a skip_all at the top if we don't have the TTY prereq, i.e. we shouldn't bother? > + > +for mode in pathspec branch > +do > + case "$mode" in > + pathspec) opt='.' ;; > + branch) opt='-f HEAD' ;; > + esac > + > + test_expect_success PERL,TTY "delayed checkout shows progress by default only on tty ($mode checkout)" ' All of the PERL,TTY can just be TTY, since TTY itself checks PERL. > + ( > + cd progress && > + rm -f *.a delay-progress.log && > + test_terminal env GIT_PROGRESS_DELAY=0 git checkout $opt 2>err && > + grep "IN: smudge test-delay10.a .* \\[DELAYED\\]" delay-progress.log && > + grep "Filtering content" err && This seems to need TTY... > + rm -f *.a delay-progress.log && > + GIT_PROGRESS_DELAY=0 git checkout $opt 2>err && > + grep "IN: smudge test-delay10.a .* \\[DELAYED\\]" delay-progress.log && > + ! grep "Filtering content" err But this one doesn't, perhaps it could be a non-TTY test? > + ) > + ' > + > + test_expect_success PERL,TTY "delayed checkout ommits progress with --quiet ($mode checkout)" ' > + ( > + cd progress && > + rm -f *.a delay-progress.log && > + test_terminal env GIT_PROGRESS_DELAY=0 git checkout --quiet $opt 2>err && > + grep "IN: smudge test-delay10.a .* \\[DELAYED\\]" delay-progress.log && > + ! grep "Filtering content" err > + ) > + ' > + > + test_expect_success PERL,TTY "delayed checkout honors --[no]-progress ($mode checkout)" ' > + ( > + cd progress && > + rm -f *.a delay-progress.log && > + test_terminal env GIT_PROGRESS_DELAY=0 git checkout --no-progress $opt 2>err && > + grep "IN: smudge test-delay10.a .* \\[DELAYED\\]" delay-progress.log && > + ! grep "Filtering content" err && > + > + rm -f *.a delay-progress.log && > + test_terminal env GIT_PROGRESS_DELAY=0 git checkout --quiet --progress $opt 2>err && > + grep "IN: smudge test-delay10.a .* \\[DELAYED\\]" delay-progress.log && > + grep "Filtering content" err > + ) > + ' It looks like these tests could be split into one helper function which just passed params for e.g. whether the "Filtering content" grep was negated, and what command should be run. Also if possible the two sections of the test could be split up, and then the "rm -rf" could just be a "test_when_finished" at the top...
Hi, Ævar Thanks for the comments! On Wed, Aug 25, 2021 at 8:39 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > On Wed, Aug 25 2021, Matheus Tavares wrote: > > > +test_expect_success PERL 'setup for progress tests' ' > > + git init progress && > > + ( > > + cd progress && > > + git config filter.delay.process "rot13-filter.pl delay-progress.log clean smudge delay" && > > + git config filter.delay.required true && > > + > > + echo "*.a filter=delay" >.gitattributes && > > + touch test-delay10.a && > > + git add . && > > + git commit -m files > > + ) > > +' > > This doesn't seem to depend on PERL, It actually depends on PERL because `git add .` will run the clean filter for `test-delay10.a`. > should this really be a skip_all at > the top if we don't have the TTY prereq, i.e. we shouldn't bother? Yeah, I think it could be a skip_all. But as you pointed out below, one of the tests doesn't really depend on TTY, so I guess we could leave the independent prereqs for each test. > > + > > +for mode in pathspec branch > > +do > > + case "$mode" in > > + pathspec) opt='.' ;; > > + branch) opt='-f HEAD' ;; > > + esac > > + > > + test_expect_success PERL,TTY "delayed checkout shows progress by default only on tty ($mode checkout)" ' > > All of the PERL,TTY can just be TTY, since TTY itself checks PERL. I don't mind changing that, but isn't it a bit clearer for readers to have both dependencies explicitly? > > + ( > > + cd progress && > > + rm -f *.a delay-progress.log && > > + test_terminal env GIT_PROGRESS_DELAY=0 git checkout $opt 2>err && > > + grep "IN: smudge test-delay10.a .* \\[DELAYED\\]" delay-progress.log && > > + grep "Filtering content" err && > > This seems to need TTY... > > > + rm -f *.a delay-progress.log && > > + GIT_PROGRESS_DELAY=0 git checkout $opt 2>err && > > + grep "IN: smudge test-delay10.a .* \\[DELAYED\\]" delay-progress.log && > > + ! grep "Filtering content" err > > But this one doesn't, perhaps it could be a non-TTY test? Good catch, I'll split this test in two. > > + ) > > + ' > > + > > + test_expect_success PERL,TTY "delayed checkout ommits progress with --quiet ($mode checkout)" ' > > + ( > > + cd progress && > > + rm -f *.a delay-progress.log && > > + test_terminal env GIT_PROGRESS_DELAY=0 git checkout --quiet $opt 2>err && > > + grep "IN: smudge test-delay10.a .* \\[DELAYED\\]" delay-progress.log && > > + ! grep "Filtering content" err > > + ) > > + ' > > + > > + test_expect_success PERL,TTY "delayed checkout honors --[no]-progress ($mode checkout)" ' > > + ( > > + cd progress && > > + rm -f *.a delay-progress.log && > > + test_terminal env GIT_PROGRESS_DELAY=0 git checkout --no-progress $opt 2>err && > > + grep "IN: smudge test-delay10.a .* \\[DELAYED\\]" delay-progress.log && > > + ! grep "Filtering content" err && > > + > > + rm -f *.a delay-progress.log && > > + test_terminal env GIT_PROGRESS_DELAY=0 git checkout --quiet --progress $opt 2>err && > > + grep "IN: smudge test-delay10.a .* \\[DELAYED\\]" delay-progress.log && > > + grep "Filtering content" err > > + ) > > + ' > > It looks like these tests could be split into one helper function which > just passed params for e.g. whether the "Filtering content" grep was > negated, and what command should be run. Makes sense, I'll do that. > Also if possible the two sections of the test could be split up, and > then the "rm -rf" could just be a "test_when_finished" at the top... Hmm, as we are removing the `test-delay10.a` file in order to check it out again with custom options, I think it's a bit clearer to remove it right before the actual git checkout invocation.
On Thu, Aug 26, 2021 at 11:26:46AM -0300, Matheus Tavares Bernardino wrote: > > > +for mode in pathspec branch > > > +do > > > + case "$mode" in > > > + pathspec) opt='.' ;; > > > + branch) opt='-f HEAD' ;; > > > + esac > > > + > > > + test_expect_success PERL,TTY "delayed checkout shows progress by default only on tty ($mode checkout)" ' > > > > All of the PERL,TTY can just be TTY, since TTY itself checks PERL. > > I don't mind changing that, but isn't it a bit clearer for readers to > have both dependencies explicitly? No just clearer, but the perl dependency of TTY is an implementation detail. It's conceivable that we could end up converting it to another language (e.g., I recall there are at least some races with the stdin mechanism, according to [0]). -Peff [0] https://lore.kernel.org/git/20190520125016.GA13474@sigill.intra.peff.net/
diff --git a/builtin/checkout.c b/builtin/checkout.c index b5d477919a..b23bc149d1 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -404,7 +404,7 @@ static int checkout_worktree(const struct checkout_opts *opts, mem_pool_discard(&ce_mem_pool, should_validate_cache_entries()); remove_marked_cache_entries(&the_index, 1); remove_scheduled_dirs(); - errs |= finish_delayed_checkout(&state, &nr_checkouts); + errs |= finish_delayed_checkout(&state, &nr_checkouts, opts->show_progress); if (opts->count_checkout_paths) { if (nr_unmerged) diff --git a/entry.c b/entry.c index 125fabdbd5..044e8ec92c 100644 --- a/entry.c +++ b/entry.c @@ -159,7 +159,8 @@ static int remove_available_paths(struct string_list_item *item, void *cb_data) return !available; } -int finish_delayed_checkout(struct checkout *state, int *nr_checkouts) +int finish_delayed_checkout(struct checkout *state, int *nr_checkouts, + int show_progress) { int errs = 0; unsigned delayed_object_count; @@ -173,7 +174,9 @@ int finish_delayed_checkout(struct checkout *state, int *nr_checkouts) dco->state = CE_RETRY; delayed_object_count = dco->paths.nr; - progress = start_delayed_progress(_("Filtering content"), delayed_object_count); + progress = show_progress + ? start_delayed_progress(_("Filtering content"), delayed_object_count) + : NULL; while (dco->filters.nr > 0) { for_each_string_list_item(filter, &dco->filters) { struct string_list available_paths = STRING_LIST_INIT_NODUP; diff --git a/entry.h b/entry.h index b8c0e170dc..7c889e58fd 100644 --- a/entry.h +++ b/entry.h @@ -43,7 +43,8 @@ static inline int checkout_entry(struct cache_entry *ce, } void enable_delayed_checkout(struct checkout *state); -int finish_delayed_checkout(struct checkout *state, int *nr_checkouts); +int finish_delayed_checkout(struct checkout *state, int *nr_checkouts, + int show_progress); /* * Unlink the last component and schedule the leading directories for diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh index b5749f327d..be12b92f21 100755 --- a/t/t0021-conversion.sh +++ b/t/t0021-conversion.sh @@ -6,6 +6,7 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME . ./test-lib.sh +. "$TEST_DIRECTORY"/lib-terminal.sh TEST_ROOT="$PWD" PATH=$TEST_ROOT:$PATH @@ -1061,4 +1062,66 @@ test_expect_success PERL,SYMLINKS,CASE_INSENSITIVE_FS \ ) ' +test_expect_success PERL 'setup for progress tests' ' + git init progress && + ( + cd progress && + git config filter.delay.process "rot13-filter.pl delay-progress.log clean smudge delay" && + git config filter.delay.required true && + + echo "*.a filter=delay" >.gitattributes && + touch test-delay10.a && + git add . && + git commit -m files + ) +' + +for mode in pathspec branch +do + case "$mode" in + pathspec) opt='.' ;; + branch) opt='-f HEAD' ;; + esac + + test_expect_success PERL,TTY "delayed checkout shows progress by default only on tty ($mode checkout)" ' + ( + cd progress && + rm -f *.a delay-progress.log && + test_terminal env GIT_PROGRESS_DELAY=0 git checkout $opt 2>err && + grep "IN: smudge test-delay10.a .* \\[DELAYED\\]" delay-progress.log && + grep "Filtering content" err && + + rm -f *.a delay-progress.log && + GIT_PROGRESS_DELAY=0 git checkout $opt 2>err && + grep "IN: smudge test-delay10.a .* \\[DELAYED\\]" delay-progress.log && + ! grep "Filtering content" err + ) + ' + + test_expect_success PERL,TTY "delayed checkout ommits progress with --quiet ($mode checkout)" ' + ( + cd progress && + rm -f *.a delay-progress.log && + test_terminal env GIT_PROGRESS_DELAY=0 git checkout --quiet $opt 2>err && + grep "IN: smudge test-delay10.a .* \\[DELAYED\\]" delay-progress.log && + ! grep "Filtering content" err + ) + ' + + test_expect_success PERL,TTY "delayed checkout honors --[no]-progress ($mode checkout)" ' + ( + cd progress && + rm -f *.a delay-progress.log && + test_terminal env GIT_PROGRESS_DELAY=0 git checkout --no-progress $opt 2>err && + grep "IN: smudge test-delay10.a .* \\[DELAYED\\]" delay-progress.log && + ! grep "Filtering content" err && + + rm -f *.a delay-progress.log && + test_terminal env GIT_PROGRESS_DELAY=0 git checkout --quiet --progress $opt 2>err && + grep "IN: smudge test-delay10.a .* \\[DELAYED\\]" delay-progress.log && + grep "Filtering content" err + ) + ' +done + test_done diff --git a/unpack-trees.c b/unpack-trees.c index 5786645f31..f07304f1b7 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -479,7 +479,7 @@ static int check_updates(struct unpack_trees_options *o, errs |= run_parallel_checkout(&state, pc_workers, pc_threshold, progress, &cnt); stop_progress(&progress); - errs |= finish_delayed_checkout(&state, NULL); + errs |= finish_delayed_checkout(&state, NULL, o->verbose_update); git_attr_set_direction(GIT_ATTR_CHECKIN); if (o->clone)
The 'Filtering contents...' progress report from delayed checkout is displayed even when checkout and clone are invoked with --quiet or --no-progress. Furthermore, it is displayed unconditionally, without first checking whether stdout is a tty. Let's fix these issues and also add some regression tests for the two code paths that currently use delayed checkout: unpack_trees.c:check_updates() and builtin/checkout.c:checkout_worktree(). Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br> --- The test section of the patch is a bit long because it checks all the verbosity options related to the progress report, and it also tests both the check_updates() and checkout_worktree() code paths. If that is overkill, I can remove some tests. builtin/checkout.c | 2 +- entry.c | 7 +++-- entry.h | 3 ++- t/t0021-conversion.sh | 63 +++++++++++++++++++++++++++++++++++++++++++ unpack-trees.c | 2 +- 5 files changed, 72 insertions(+), 5 deletions(-)