Re: [PATCH 07/15] t0020: drop redirections to /dev/null

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Dec 17, 2019, 19:32 UTC
Message-ID: <CAPig+cQJRCAYkn+9CoK0YOXa66B7ocv2qXGLBXy8f-sgE3ipBA@mail.gmail.com>
In-Reply-To: <d228dcfdc7d3f41c53a3813c52e56638cd8dd95e.1576583819.git.liu.denton@gmail.com>

On Tue, Dec 17, 2019 at 7:02 AM Denton Liu <liu.denton@gmail.com> wrote:

Show 12 quoted lines

> Since output is silenced when running without `-v` and debugging output
> is useful with `-v`, remove redirections to /dev/null as it is not
> useful.
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh
> @@ -5,7 +5,7 @@ test_description='CRLF conversion'
>  has_cr() {
> -       tr '\015' Q <"$1" | grep Q >/dev/null
> +       tr '\015' Q <"$1" | grep Q
>  }

I'm not sure that I agree with this change since dropping >/dev/null doesn't improve the situation when someone is trying to debug a failing test. What this change will do is fill the -v output with a bunch of lines ending with "Q" when CR is expected -- the normal _successful_ case for about half the calls to has_cr() -- so -v output will become a lot noisier without really adding much, if any, debugging value. If you really want to help people trying to diagnose failures, I could see you replacing has_cr() with two new functions which actually provide useful diagnostic output; for instance something like:

    expect_cr () {
        if ! tr '\015' Q <"$1" | grep Q >/dev/null
        then
            echo "missing CR termination in: $1" >&2 &&
            return 1
        fi
    }

    expect_no_cr () {
        if tr '\015' Q <"$1" | grep Q >/dev/null
        then
            echo "unexpected CR termination in: $1" >&2 &&
            return 1
        fi
    }

However, I'm not convinced that introducing and debugging these functions is worth the effort over simply dropping this patch from the series.