Re: [PATCH v7 6/6] Reftable support for git-core

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:

> diff --git a/t/t0031-reftable.sh b/t/t0031-reftable.sh
> new file mode 100755
> index 00000000000..d35211c9db2
> --- /dev/null
> +++ b/t/t0031-reftable.sh
> @@ -0,0 +1,31 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2020 Google LLC
> +#
> +
> +test_description='reftable basics'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'basic operation of reftable storage' '
> +        git init --ref-storage=reftable repo &&

Test script bodies are supposed to be indented with HT, not 8 SPs.

> +        cd repo &&

You are not supposed to chdir around inside tests, unless you do so
in a subshell.  i.e.

	test_expect_success 'basic operation' '
		git init --ref-storage=reftable repo &&
		(
			cd repo &&
			...
		)
	'

This is because the next test somebody else will add after this one
*will* start from inside that repo (but if and only if this step
gets run---and there are ways to skip steps).

> +        echo "hello" > world.txt &&

A shell redirection operator should immediately be followed by the
filename without SP in between, i.e.

	echo hello >world.txt &&

> +        git add world.txt &&
> +        git commit -m "first post" &&
> +        printf "HEAD\nrefs/heads/master\n" > want &&

There is an easier-to-read helper.  Also, by convention, the file
that holds the expected state is called "expect", while the file
that records the actual state observed is called "actual".

	test_write_lines HEAD refs/heads/master >expect &&

> +        git show-ref | cut -f2 -d" " > got &&

Is the order of "show-ref" output guaranteed in some way?  Otherwise
we may need to make sure that we sort it to keep it stable.

Without --show-head, HEAD should not appear in "git show-ref"
output, but the expected output has HEAD, which is puzzling.

We will not notice if a "git" command that placed on the left hand
side of a pipeline segfaults after showing its output.  

People often split a pipeline like this into separate command for
that reason (but there is probably a better way in this case, as we
see soon).

> +        test_cmp got want &&

Taking the above comments together, perhaps

	test_write_lines refs/heads/master >expect &&
	git for-each-ref --sort=refname >actual &&
	test_cmp expect actual &&

> +        for count in $(test_seq 1 10); do
> +                echo "hello" >> world.txt
> +                git commit -m "number ${count}" world.txt
> +        done &&

Style: write "do" on its own line, aligned with "for" on the
previous line.  Same for "if/then/else/fi" "while/do/done" etc.  

    An easy way to remember is that you never use any semicolon in
    your shell script, except for the double-semicolon you use to
    delimit your case arms.

When any of these steps fail, you do not notice it, unless the
failure happens to occur to "git commit" during the last round.  I
think you can use "return 1" to signal the failure to the test
framework, like so.

	for count in $(test_seq 1 10)
	do
		echo hello >>world.txt &&
		git commit -m "number $count} world.txt ||
		return 1
	done &&

> +        git gc &&
> +        nfiles=$(ls -1 .git/reftable | wc -l | tr -d "[ \t]" ) &&
> +        test "${nfiles}" = "2" &&

No need to pipe "wc -l" output to "tr" to strip spaces.  Just stop
double-quoting it on the using side, and even better is to use
test_line_count.  Perhaps

	ls -1 .git/reftable >files &&
	test_line_count = 2 files

> +        git reflog refs/heads/master > output &&
> +        grep "commit (initial): first post" output &&
> +        grep "commit: number 10" output

Do we give guarantees that we keep all the reflog entries?  Would it
help to count the number of lines in the output file here?

> +'
> +
> +test_done



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux