Re: [PATCH v2 07/10] t4205: cover `git log --reflog -z` blindspot

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Nov 8, 2019, 20:36 UTC
Message-ID: <CAPig+cQDMSwP5-D-=LgjBPH2kJK16Fv4c619Pg6OTU95CQ9sgw@mail.gmail.com>
In-Reply-To: <b5950823ce90dd2476f002ed0370b7e0099a4d85.1573241590.git.liu.denton@gmail.com>

On Fri, Nov 8, 2019 at 3:08 PM Denton Liu <liu.denton@gmail.com> wrote:

Show 12 quoted lines

> The test suite does not include any tests where `--reflog` and `-z` are
> used together in `git log`. Cover this blindspot. Note that the
> `--pretty=oneline` case is written separately because it follows a
> slightly different codepath.
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
> @@ -134,6 +134,41 @@ test_expect_failure C_LOCALE_OUTPUT 'NUL termination with --stat' '
> +emit_nul () {
> +       echo | tr '\n' '\000'
> +}

A simple:

    printf "\0"

would be simpler, and I don't think you even need to introduce a shell function to encapsulate it, as it's quite clear at a glance what it does.

> +for p in short medium full fuller email raw
> +do
> +       test_expect_success "NUL termination with --reflog --pretty=$p" '
> +               >expect &&

You can drop this line...

Show 5 quoted lines

> +               revs="$(git rev-list --reflog)" &&
> +               for r in $revs
> +               do
> +                       git show -s "$r" --pretty='$p' >>expect || return 1
> +                       emit_nul >>expect

...and simplify all this capturing into 'expect'...

> +               done &&

... by just redirecting the output of the for-loop itself:

    for r in $(git rev-list --reflog)
    do
        git show -s --pretty="$p" "$r" &&
        printf "\0" || return 1
    done >expect &&

For completeness, the above example also drops the unnecessary 'revs' variable, uses double quotes rather than single when interpolating $p, and makes the loop early-exit a bit more idiomatic.

> +               git log -z --reflog --pretty='$p' >actual &&
> +               emit_nul >>actual &&

Likewise, you can capture 'actual' in its entirety:

    {
        git log -z --reflog --pretty="$p" &&
        printf "\0"
    } >actual &&

Show 14 quoted lines

> +               test_cmp expect actual
> +       '
> +done
> +
> +test_expect_success 'NUL termination with --reflog --pretty=oneline' '
> +       >expect &&
> +       revs="$(git rev-list --reflog)" &&
> +       for r in $revs
> +       do
> +               # trim trailing newline
> +               output="$(git show -s --pretty=oneline "$r")" || return 1
> +               printf "%s" "$output" >>expect
> +               emit_nul >>expect
> +       done &&

Replacing the newline with NUL could be done more simply and idiomatically (with regard to other test scripts) by passing the output of "git show" through the lf_to_nul() function from test-lib-functions.sh. Something like this should do it:

    for r in $(git rev-list --reflog)
    do
        git show -s --pretty=oneline "$r" >raw &&
        cat raw | lf_to_nul || return 1
    done >expect &&

> +       git log -z --pretty=oneline --reflog >actual &&
> +       # no trailing NUL

To what is this comment referring?

> +       test_cmp expect actual
> +'