Re: [PATCH v2 1/4] t4014: clean up style

[Eric Sunshine <sunshine@sunshineco.com>]

On Mon, Aug 19, 2019 at 7:53 PM Denton Liu <liu.denton@gmail.com> wrote:
> In Git's tests, there is typically no space between the redirection
> operator and the filename. Remove these spaces.
>
> Since output is silenced when running without `-v` and debugging
> output is useful with `-v`, remove redirections to /dev/null.
>
> Change here-docs from `<<\EOF` to `<<-\EOF` so that they can be indented
> along with the rest of the test case.
>
> Convert all instances of `cnt=$(... | wc -l) && test $cnt = N` into
> uses of `test_line_count()`.
>
> For style, move the ending sq of test cases onto its own line whenever
> they do not conform.
>
> Rename output files from "expected" to "expect" to conform with the
> usual convention.
>
> Finally, refactor to remove Git commands upstream of pipe as well as Git
> commands that are in a non-variable-assignment subshell (e.g. `echo
> "base-commit: $(git rev-parse HEAD)"`. This way, if an invocation of a
> Git command fails, the return code won't be lost. Keep upstream non-Git
> commands since we have to assume a base level of sanity.
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> This patch is getting a little unwieldy. Perhaps we could split it into
> several smaller patches? Unfortunately, I'm not really sure where a
> logical place to split it would be.

The bullet points in the commit message, each of which is a distinct
change, give a strong hint as to how the commit could be split into
smaller pieces. It would make repeated review easier (though I'm not
sure it's worth re-rolling just for that).

> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> @@ -62,26 +63,24 @@ test_expect_success setup '
>  test_expect_success "format-patch --ignore-if-in-upstream" '
>
>         git format-patch --stdout \
>                 --ignore-if-in-upstream master..side >patch1 &&
> -       cnt=$(grep "^From " patch1 | wc -l) &&
> -       test $cnt = 2
> -
> +       grep "^From " patch1 >from1 &&
> +       test_line_count = 2 from1
>  '

Here you've removed the blank line following the body before the
closing quote, which brings the formatting more in line with current
style, however, you could do the same with the blank line before the
body. Ditto for other tests.

Another style fix would be to change the double quotes in the test
title to single quotes (here and in other tests).

> @@ -357,7 +369,7 @@ test_expect_success 'reroll count (-v)' '
>  check_threading () {
>         expect="$1" &&
>         shift &&
> -       (git format-patch --stdout "$@"; echo $? > status.out) |
> +       git format-patch --stdout "$@" >patch &&
>         # Prints everything between the Message-ID and In-Reply-To,
>         # and replaces all Message-ID-lookalikes by a sequence number
>         perl -ne '
> @@ -372,12 +384,11 @@ check_threading () {
>                         print;
>                 }
>                 print "---\n" if /^From /i;
> -       ' > actual &&
> -       test 0 = "$(cat status.out)" &&
> +       ' <patch >actual &&
>         test_cmp "$expect" actual
>  }

If you do break this patch into smaller pieces, this might deserve its
own patch (or not) because it requires a bit of extra reasoning by the
reviewer.

> @@ -852,20 +866,21 @@ test_expect_success 'format-patch --ignore-if-in-upstream HEAD' '
> -git_version="$(git --version | sed "s/.* //")"
> -
>  signature() {
> +       git_version="$(git --version >version && sed "s/.* //" version)" &&
>         printf "%s\n%s\n\n" "-- " "${1:-$git_version}"
>  }

Windows folks are not going to like you for this change since process
creation is so expensive on that platform, and the point of setting
that variable globally was to avoid repeated invocation (especially
since the output of "git --version" won't change). This function
appears to be invoked only three times presently, but that number
could increase in the future as new tests are added. Consequently, it
might be best to drop this change.