Hi, Ævar Thanks for the comments! On Wed, Aug 25, 2021 at 8:39 PM Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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.
