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

On Thu, Oct 21, 2021 at 10:26 AM Jeff Hostetler via GitGitGadget
[off-list ref] wrote:

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 &&

+       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.

+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/ (local)
[2]: https://lore.kernel.org/git/CAPig+cQFFsLeE921WpzTxVnBMnNRiKs4N=hUQ2UQi1VznNEQwg@mail.gmail.com/ (local)

+               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.

+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).

+               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