Re: [PATCH] checkout: make delayed checkout respect --quiet and --no-progress

[Matheus Tavares Bernardino <matheus.bernardino@usp.br>]

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.