Re: [PATCH 2/2] t0030-t0050: avoid pipes with Git on LHS
- From
Shubham Mishra <shivam828787@gmail.com>
- Date
- Feb 26, 2022, 12:09 UTC
- Message-ID
- <CAC316V4a37rsVpemARu17h2e0bj=ahsWOxjBEwO=BiWou+4aBw@mail.gmail.com>
- In-Reply-To
- <220224.86o82wab31.gmgdl@evledraar.gmail.com>
Hi,
Thanks for Reviewing.
Show 33 quoted lines
> 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