Re: [PATCH v4 2/2] p0005-status: time status on very large repo
Hi,
Jeff Hostetler <jeffhost@microsoft.com> wrote:
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
Usually the commit message is a place to put some of the motivation behind the patch or why I should want to apply it. In this example, everything is answered by the previous patch, which suggests that it would be easier to understand if they were squashed into a single patch.
[...]
Show 8 quoted lines
> +++ b/t/perf/p0005-status.sh
> @@ -0,0 +1,51 @@
> +#!/bin/sh
> +##
> +## This test measures the performance of various read-tree
> +## and status operations. It is primarily interested in
> +## the algorithmic costs of index operations and recursive
> +## tree traversal -- and NOT disk I/O on thousands of files.
Nit: git usually uses a single # to start comments in shell scripts
(and likewise below).
[...]
Show 13 quoted lines
> +## If the test repo was generated by ./repos/many-files.sh
> +## then we know something about the data shape and branches,
> +## so we can isolate testing to the ballast-related commits
> +## and setup sparse-checkout so we don't have to populate
> +## the ballast files and directories.
> +##
> +## Otherwise, we make some general assumptions about the
> +## repo and consider the entire history of the current
> +## branch to be the ballast.
> +
> +git branch | grep p0006-ballast >/dev/null 2>&1
> +synthetic=$?
> +if test "$synthetic" = 0
nit: no need to run a command and read $? when using the command directly from if would do:
if git branch | grep p0006-ballast
This can be simplified further by using a lower-level command for the test:
if git rev-parse --verify refs/heads/p0006-ballast^{commit}Show 12 quoted lines
> +then
> + echo Assuming synthetic repo from many-files.sh
> + git branch br_base master
> + git branch br_ballast p0006-ballast
> + echo '/*' >.git/info/sparse-checkout
> + echo '!ballast/*' >>.git/info/sparse-checkout
> + git config --local core.sparsecheckout 1
> +else
> + echo Assuming non-synthetic repo...
> + git branch br_base $(git rev-list HEAD | tail -n 1)
> + git branch br_ballast HEAD
> +fi
This kind of setup instructions usually go in a test_expect_success block so that the perf harness can detect whether they failed.
> +
> +test_expect_success 'setup' '
> + git checkout -q br_ballast
nit: no need to use "-q" --- most output from test_expect_success is suppressed unless the script is run with "-v", at which point it becomes useful for debugging.
> +'
> +
> +nr_files=$(git ls-files | wc -l)
This also can go in the test_expect_success block.
Show 5 quoted lines
> +
> +test_perf "read-tree status br_ballast ($nr_files)" '
> + git read-tree HEAD &&
> + git status
> +'
Looks nice and simple. Thanks for writing it.
Hope that helps, Jonathan
