Web lists-archives.com

Re: [PATCH v4 2/2] p0005-status: time status on very large repo




Hi,

Jeff Hostetler <jeffhost@xxxxxxxxxxxxx> wrote:

> Signed-off-by: Jeff Hostetler <jeffhost@xxxxxxxxxxxxx>

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.

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

[...]
> +## 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}

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

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