Hi,
Matheus Tavares wrote:
> Add tests to populate the working tree during clone and checkout using
> the sequential and parallel modes, to confirm that they produce
> identical results. Also test basic checkout mechanics, such as checking
> for symlinks in the leading directories and the abidance to --force.
Thanks for implementing parallel checkout! I'm excited about the
feature. And thanks for including these tests.
[...]
> --- /dev/null
> +++ b/t/lib-parallel-checkout.sh
> @@ -0,0 +1,39 @@
[...]
> +# Runs `git -c checkout.workers=$1 -c checkout.thesholdForParallelism=$2 ${@:4}`
> +# and checks that the number of workers spawned is equal to $3.
> +git_pc()
nit: what does git_pc mean? Can this spell it out more verbosely, or
could callers take on more of the burden? (Perhaps it would make sense
to use a helper that uses test_config to set the relevant configuration,
and then the caller can use plain "git clone"?)
[...]
> + GIT_TRACE2="$(pwd)/trace" git \
> + -c checkout.workers=$workers \
> + -c checkout.thresholdForParallelism=$threshold \
> + -c advice.detachedHead=0 \
> + $@ &&
$@ needs to be quoted, or else it will act like $* (and in particular it
won't handle parameters with embedded spaces).
> +
> + # Check that the expected number of workers has been used. Note that it
> + # can be different than the requested number in two cases: when the
> + # quantity of entries to be checked out is less than the number of
> + # workers; and when the threshold has not been reached.
> + #
> + local workers_in_trace=$(grep "child_start\[.\+\] git checkout--helper" trace | wc -l) &&
Do we use grep's \+ operator in other tests? I thought we preferred to
use the more portable *, but it may be that I'm out of date.
[...]
> +# Verify that both the working tree and the index were created correctly
> +verify_checkout()
> +{
> + git -C $1 diff-index --quiet HEAD -- &&
> + git -C $1 diff-index --quiet --cached HEAD -- &&
> + git -C $1 status --porcelain >$1.status &&
> + test_must_be_empty $1.status
> +}
Like git_pc, this is not easy to take in at a glance.
"$1" needs to be quoted if we are to handle paths with spaces.
[...]
> --- /dev/null
> +++ b/t/t2080-parallel-checkout-basics.sh
> @@ -0,0 +1,197 @@
> +#!/bin/sh
> +
> +test_description='parallel-checkout basics
> +
> +Ensure that parallel-checkout basically works on clone and checkout, spawning
> +the required number of workers and correctly populating both the index and
> +working tree.
> +'
> +
> +TEST_NO_CREATE_REPO=1
> +. ./test-lib.sh
> +. "$TEST_DIRECTORY/lib-parallel-checkout.sh"
> +
> +# NEEDSWORK: cloning a SHA1 repo with GIT_TEST_DEFAULT_HASH set to "sha256"
> +# currently produces a wrong result (See
> +# https://lore.kernel.org/git/20200911151717.43475-1-matheus.bernardino@xxxxxx/).
> +# So we skip the "parallel-checkout during clone" tests when this test flag is
> +# set to "sha256". Remove this when the bug is fixed.
> +#
> +if test "$GIT_TEST_DEFAULT_HASH" = "sha256"
> +then
> + skip_all="t2080 currently don't work with GIT_TEST_DEFAULT_HASH=sha256"
> + test_done
> +fi
> +
> +R_BASE=$GIT_BUILD_DIR
> +
> +test_expect_success 'sequential clone' '
> + git_pc 1 0 0 clone --quiet -- $R_BASE r_sequential &&
This fails when I run it when building from a tarball, which is
presenting me from releasing this patch series to Debian experimental.
Can we use an artificial repo instead of git.git? Using git.git as
test data seems like a recipe for hard-to-reproduce test failures.
Thanks and hope that helps,
Jonathan
