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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Apr 5, 2017, 21:33 UTC
Message-ID: <20170405213355.GH8741@aiede.mtv.corp.google.com>
In-Reply-To: <20170405195600.54801-3-git@jeffhostetler.com>

Hi,

git@jeffhostetler.com wrote:

Show 33 quoted lines

> +++ b/t/perf/p0005-status.sh
> @@ -0,0 +1,70 @@
> +#!/bin/sh
> +
> +test_description="Tests performance of read-tree"
> +
> +. ./perf-lib.sh
> +
> +test_perf_default_repo
> +test_checkout_worktree
> +
> +## usage: dir depth width files
> +make_paths () {
> +	for f in $(seq $4)
> +	do
> +		echo $1/file$f
> +	done;
> +	if test $2 -gt 0;
> +	then
> +		for w in $(seq $3)
> +		do
> +			make_paths $1/dir$w $(($2 - 1)) $3 $4
> +		done
> +	fi
> +	return 0
> +}
> +
> +fill_index () {
> +	make_paths $1 $2 $3 $4 |
> +	sed "s/^/100644 $EMPTY_BLOB	/" |
> +	git update-index --index-info
> +	return 0
> +}

Makes sense.

Show 18 quoted lines

> +
> +br_work1=xxx_work1_xxx
> +
> +new_dir=xxx_dir_xxx
> +
> +## (5, 10, 9) will create 999,999 files.
> +## (4, 10, 9) will create  99,999 files.
> +depth=5
> +width=10
> +files=9
> +
> +export br_work1
> +
> +export new_dir
> +
> +export depth
> +export width
> +export files

Why are these exported? test_expect_success code (unlike test_per code) runs in the same shell as outside, so it doesn't seem necessary.

> +
> +## Inflate the index with thousands of empty files and commit it.
> +test_expect_success 'inflate the index' '
> +	git reset --hard &&

What does this line do?

> +	git branch $br_work1 &&
> +	git checkout $br_work1 &&

Is it useful for these parameters to exist in the test script? I'd find this easier to read if it named the branch explicitly. For example:

	test_expect_success 'set up large index' '
		git checkout -B million &&
		# (4, 10, 9) would create 99,999 files.
		# (5, 10, 9) creates 999,999 files.
		fill_index dir 5 10 9 &&
		git commit -m "large commit"
	'

> +	fill_index $new_dir $depth $width $files &&
> +	git commit -m $br_work1 &&
> +	git reset --hard

What does this line do?

Show 10 quoted lines

> +'
> +
> +## The number of files in the xxx_work1_xxx branch.
> +nr_work1=$(git ls-files | wc -l)
> +export nr_work1
> +
> +test_perf "read-tree status work1 ($nr_work1)" '
> +	git read-tree HEAD &&
> +	git status
> +'

Looks reasonable.

Thanks and hope that helps, Jonathan