Re: [PATCH v4 5/6] core.fsyncobjectfiles: tests for batch mode

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Sep 21, 2021, 23:54 UTC
Message-ID: <87a6k58or9.fsf@evledraar.gmail.com>
In-Reply-To: <afb0028e79648c1f7be8d77df5c6d675bd27d983.1632176111.git.gitgitgadget@gmail.com>

On Mon, Sep 20 2021, Neeraj Singh via GitGitGadget wrote:

Show 44 quoted lines

> From: Neeraj Singh <neerajsi@microsoft.com>
>
> Add test cases to exercise batch mode for 'git add'
> and 'git stash'. These tests ensure that the added
> data winds up in the object database.
>
> I verified the tests by introducing an incorrect rename
> in do_sync_and_rename.
>
> Signed-off-by: Neeraj Singh <neerajsi@microsoft.com>
> ---
>  t/lib-unique-files.sh | 34 ++++++++++++++++++++++++++++++++++
>  t/t3700-add.sh        | 11 +++++++++++
>  t/t3903-stash.sh      | 14 ++++++++++++++
>  3 files changed, 59 insertions(+)
>  create mode 100644 t/lib-unique-files.sh
>
> diff --git a/t/lib-unique-files.sh b/t/lib-unique-files.sh
> new file mode 100644
> index 00000000000..a8a25eba61d
> --- /dev/null
> +++ b/t/lib-unique-files.sh
> @@ -0,0 +1,34 @@
> +# Helper to create files with unique contents
> +
> +test_create_unique_files_base__=$(date -u)
> +test_create_unique_files_counter__=0
> +
> +# Create multiple files with unique contents. Takes the number of
> +# directories, the number of files in each directory, and the base
> +# directory.
> +#
> +# test_create_unique_files 2 3 . -- Creates 2 directories with 3 files
> +#				    each in the specified directory, all
> +#				    with unique contents.
> +
> +test_create_unique_files() {
> +	test "$#" -ne 3 && BUG "3 param"
> +
> +	local dirs=$1
> +	local files=$2
> +	local basedir=$3
> +
> +	rm -rf $basedir >/dev/null

Why the >/dev/null? It's not a "-rfv", and any errors would go to stderr.

> +		mkdir -p "$dir" > /dev/null

Ditto.

> +		for j in $(test_seq $files)
> +		do
> +			test_create_unique_files_counter__=$((test_create_unique_files_counter__ + 1))
> +			echo "$test_create_unique_files_base__.$test_create_unique_files_counter__"  >"$dir/file$j.txt"

Would be much more readable if we these variables were shorter.

But actually, why are we trying to create files as a function of "date -u" at all? This is all in the trash directory, which is rm -rf'd beween runs, why aren't names created with test_seq or whatever OK? I.e. just 1.txt, 2.txt....

Show 15 quoted lines

> +test_expect_success 'stash with core.fsyncobjectfiles=batch' "
> +	test_create_unique_files 2 4 fsync-files &&
> +	git -c core.fsyncobjectfiles=batch stash push -u -- ./fsync-files/ &&
> +	rm -f fsynced_files &&
> +
> +	# The files were untracked, so use the third parent,
> +	# which contains the untracked files
> +	git ls-tree -r stash^3 -- ./fsync-files/ > fsynced_files &&
> +	test_line_count = 8 fsynced_files &&
> +	cat fsynced_files | awk '{print \$3}' | xargs -n1 git cat-file -e
> +"
> +
> +
>  test_expect_success 'stash -c stash.useBuiltin=false warning ' '
>  	expected="stash.useBuiltin support has been removed" &&

We really prefer our tests to create the same data each time if possible, but as noted with the "date -u" comment above you're explicitly bypassing that, but I still can't see why...