Re: [PATCH v4 29/29] t7527: test status with untracked-cache and fsmonitor--daemon

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Oct 22, 2021, 05:23 UTC
Message-ID: <CAPig+cT1j6NsZaSM+_JgGksmH4XGsnh7Pe=dbELoBU+FisHGag@mail.gmail.com>
In-Reply-To: <507020bbef08a02afc53815754cc999c390eb7c7.1634826309.git.gitgitgadget@gmail.com>

On Thu, Oct 21, 2021 at 10:26 AM Jeff Hostetler via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 12 quoted lines

> Create 2x2 test matrix with the untracked-cache and fsmonitor--daemon
> features and a series of edits and verify that status output is
> identical.
>
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---
> diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh
> @@ -508,4 +510,98 @@ test_expect_success 'cleanup worktrees' '
> +test_lazy_prereq UNTRACKED_CACHE '
> +       { git update-index --test-untracked-cache; ret=$?; } &&
> +       test $ret -ne 1
> +'

I may be missing something obvious, but can't this be expressed more simply as:

    test_lazy_prereq UNTRACKED_CACHE '
        git update-index --test-untracked-cache
        test $? -ne 1
    '

How significant is it to test specifically against 1? If not, then even simpler would be:

    test_lazy_prereq UNTRACKED_CACHE '
        git update-index --test-untracked-cache
    '

which is also more robust since it won't be fooled by die() or crashes.

> +test_expect_success 'Matrix: setup for untracked-cache,fsmonitor matrix' '
> +       test_might_fail git config --unset core.useBuiltinFSMonitor &&

More idiomatic:

    test_unconfig core.useBuiltinFSMonitor &&

Show 8 quoted lines

> +       git update-index --no-fsmonitor &&
> +       test_might_fail git fsmonitor--daemon stop
> +'
> +
> +matrix_clean_up_repo () {
> +       git reset --hard HEAD
> +       git clean -fd
> +}

Since calls to this function are part of the &&-chain in tests, it probably would be a good idea to maintain the &&-chain within the body of the function, as well.

Show 5 quoted lines

> +matrix_try () {
> +       test_expect_success "Matrix[uc:$uc][fsm:$fsm] $fn" '
> +               matrix_clean_up_repo &&
> +               $fn &&
> +               if test $uc = false -a $fsm = false

We avoid -a and -o with `test` and instead chain them with &&:

    if test $uc = false && test $fsm = false

Documentation/CodingGuidelines mentions this. Also see [1] & [2].

[1]: https://lore.kernel.org/git/xmqqa6qkb5fi.fsf@gitster.g/ [2]: https://lore.kernel.org/git/CAPig+cQFFsLeE921WpzTxVnBMnNRiKs4N=hUQ2UQi1VznNEQwg@mail.gmail.com/

Show 7 quoted lines

> +               then
> +                       git status --porcelain=v1 >.git/expect.$fn
> +               else
> +                       git status --porcelain=v1 >.git/actual.$fn
> +                       test_cmp .git/expect.$fn .git/actual.$fn
> +               fi
> +       '

Broken &&-chain in the `else` arm.

> +       return $?
> +}

No callers care about the return value of this function, so the `return $?` can be dropped.

Show 24 quoted lines

> +uc_values="false"
> +test_have_prereq UNTRACKED_CACHE && uc_values="false true"
> +for uc_val in $uc_values
> +do
> +       if test $uc_val = false
> +       then
> +               test_expect_success "Matrix[uc:$uc_val] disable untracked cache" '
> +                       git config core.untrackedcache false &&
> +                       git update-index --no-untracked-cache
> +               '
> +       else
> +               test_expect_success "Matrix[uc:$uc_val] enable untracked cache" '
> +                       git config core.untrackedcache true &&
> +                       git update-index --untracked-cache
> +               '
> +       fi
> +
> +       fsm_values="false true"
> +       for fsm_val in $fsm_values
> +       do
> +               if test $fsm_val = false
> +               then
> +                       test_expect_success "Matrix[uc:$uc_val][fsm:$fsm_val] disable fsmonitor" '
> +                               test_might_fail git config --unset core.useBuiltinFSMonitor &&

Ditto: test_unconfig()

> +                               git update-index --no-fsmonitor &&
> +                               test_might_fail git fsmonitor--daemon stop 2>/dev/null
> +                       '

stderr is redirected within tests anyhow, so we normally don't suppress it manually like this (especially since it may come in handy when debugging a failing test).

Show 19 quoted lines

> +               else
> +                       test_expect_success "Matrix[uc:$uc_val][fsm:$fsm_val] enable fsmonitor" '
> +                               git config core.useBuiltinFSMonitor true &&
> +                               git fsmonitor--daemon start &&
> +                               git update-index --fsmonitor
> +                       '
> +               fi
> +
> +               matrix_try $uc_val $fsm_val edit_files
> +               matrix_try $uc_val $fsm_val delete_files
> +               matrix_try $uc_val $fsm_val create_files
> +               matrix_try $uc_val $fsm_val rename_files
> +               matrix_try $uc_val $fsm_val file_to_directory
> +               matrix_try $uc_val $fsm_val directory_to_file
> +
> +               if test $fsm_val = true
> +               then
> +                       test_expect_success "Matrix[uc:$uc_val][fsm:$fsm_val] disable fsmonitor at end" '
> +                               test_might_fail git config --unset core.useBuiltinFSMonitor &&

Ditto: test_unconfig()

> +                               git update-index --no-fsmonitor &&
> +                               test_might_fail git fsmonitor--daemon stop 2>/dev/null

Ditto: stderr

> +                       '
> +               fi
> +       done
> +done