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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Apr 11, 2017, 21:35 UTC
Message-ID: <20170411213523.GQ8741@aiede.mtv.corp.google.com>
In-Reply-To: <20170411200802.31638-3-git@jeffhostetler.com>

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