Re: [PATCH 2/2] t0030-t0050: avoid pipes with Git on LHS

[Shubham Mishra <shivam828787@gmail.com>]

Hi,

Thanks for Reviewing.

> You're modifying some tests here that are using some old coding style,
> so maybe it's better to adjust it while we're at it?
>
> Also I think this would be a lot nicer with test_stdout_line_count and a
> helper to deal with that pritnf, e.g.:
>
>         diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
>         index ae1ca380c1a..d48a3579511 100755
>         --- a/t/t0030-stripspace.sh
>         +++ b/t/t0030-stripspace.sh
>         @@ -223,12 +223,15 @@ test_expect_success \
>              test_cmp expect actual
>          '
>
>         -test_expect_success \
>         -    'text without newline at end should end with newline' '
>         -    test $(printf "$ttt" | git stripspace | wc -l) -gt 0 &&
>         -    test $(printf "$ttt$ttt" | git stripspace | wc -l) -gt 0 &&
>         -    test $(printf "$ttt$ttt$ttt" | git stripspace | wc -l) -gt 0 &&
>         -    test $(printf "$ttt$ttt$ttt$ttt" | git stripspace | wc -l) -gt 0
>         +printf_git_stripspace () {
>         +       printf "$1" | git stripspace
>         +}
>         +
>         +test_expect_success 'text without newline at end should end with newline' '
>         +       test_stdout_line_count -gt 0 printf_git_stripspace "$ttt" &&
>         +       test_stdout_line_count -gt 0 printf_git_stripspace "$ttt$ttt" &&
>         +       test_stdout_line_count -gt 0 printf_git_stripspace "$ttt$ttt$ttt" &&
>         +       test_stdout_line_count -gt 0 printf_git_stripspace "$ttt$ttt$ttt$ttt"
>          '
>
>          # text plus spaces at the end:
>

it makes sense to improve this code as we are touching it, this way
looks much better. I will implement it.


> This is not on you, but generally we don't pipe "grep" to >/dev/null,
> and just let the --verbose option do its work.
I don't think I understood this, I guess you are talking about the
"-v" flag that stands for invert match? I didn't find "--verbose" with
grep.
Please correct me if I am wrong.

> With/without that change you no longer need the () subshell here.
sure.

Thanks,
Shubham