From: Eric Sunshine <sunshine@sunshineco.com>
Date: Tue, 29 Jun 2021 14:11:20 GMT
Subject: Re: [PATCH v4 1/2] t6400: preserve git ls-files exit status code
Message-ID: <CAPig+cSKOzebGRyoytUGORhq56P0rijYrKO6uu7q7fWnzwiQkw@mail.gmail.com>
In-Reply-To: <49104273b8b801fc61811347120c5f4c42a3700b.1624974969.git.congdanhqx@gmail.com>

On Tue, Jun 29, 2021 at 9:57 AM Đoàn Trần Công Danh
<congdanhqx@gmail.com> wrote:
> In t6400, we're checking number of files in the index and the working
> tree by piping the output of "git ls-files" to "wc -l", thus losing the
> exit status code of git.
>
> Let's write the output of "git ls-files" to a temporary file, in order
> to check exit status code of "git ls-files" properly.

Thanks, the simplicity of this version over the previous attempts is appealing.

Just a few extremely minor style nits below; don't know if any of them
are worth a re-roll.

> Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
> ---
> diff --git a/t/t6400-merge-df.sh b/t/t6400-merge-df.sh
> @@ -9,6 +9,20 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>
>  . ./test-lib.sh
>
> +check_ls_files_count() {

style: funcname () {

> +       local ops val
> +       if test "$#" -le 2

I also &&-chain the `local` declaration:

    local ops val &&
    if test "$#" -le 2

By making it easy to see the `&&` upfront, when new code is inserted,
there is a better chance that the &&-chain will be kept intact:

    local ops val &&
    my new code &&
    if test "$#" -le 2

> +       then
> +               BUG "Expect 2 or more arguments"
> +       fi &&

A quick grep of the tests indicates that they are consistent about
using lowercase for the first word in a BUG():

    BUG "expect 2 or more arguments"

> +       ops="$1" &&
> +       val="$2" &&
> +       shift 2 &&
> +       mkdir -p .git/trash &&
> +       git ls-files "$@" >.git/trash/output &&
> +       test_line_count "$ops" "$val" .git/trash/output
> +}