Re: [PATCHv4 1/2] git-p4: add test case for RCS keywords

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 9, 2012, 22:55 UTC
Message-ID: <7v8vkbvcpg.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1328785409-30936-2-git-send-email-luke@diamand.org>

Luke Diamand <luke@diamand.org> writes:

> +create_kw_file() {
> + cat <<'EOF' > $1

Please start this shell function like this instead:

        create_kw_file () {
                cat <<\EOF >"$1"

The first line is merely cosmetic but matches the convention used in our shell scripts better.

As to the second line:

 * Quoting "$1" should not be strictly necessary according to POSIX rules,
   but we saw reports that some shells "helpfully" give unnecessary
   warnings when $1 (or whatever variable that holds the name of the file
   the output is redirected into) contains $IFS characters.

 * An indent in our shell scripts is a HT, not a SP (or four spaces or
   whatever).

 * And our convention is not to have an extra SP after redirection '>' (or
   '<') operator.

Show 19 quoted lines

> +test_expect_success 'init depot' '
> +	(
> +		cd "$cli" &&
> +		echo file1 >file1 &&
> +		p4 add file1 &&
> +		p4 submit -d "change 1" &&
> +		create_kw_file kwfile1.c &&
> +		p4 add kwfile1.c &&
> +		p4 submit -d "Add rcw kw file" kwfile1.c
> +	)
> +'
> +
> +p4_append_to_file() {
> + f=$1
> + p4 edit -t ktext $f &&
> + echo "/* $(date) */" >> $f &&
> + p4 submit -d "appending a line in p4" &&
> + cat $f
> +}

Surround all occurrences of '$f' with double-quotes, e.g.

	cat "$f"

to avoid unexpected breakage when later somebody tries to use a file whose name happens to contain a space. The first assignment "f=$1" can stay as-is.

Show 8 quoted lines

> +
> +# Create some files with RCS keywords. If they get modified
> +# elsewhere then the version number gets bumped which then
> +# results in a merge conflict if we touch the RCS kw lines,
> +# even though the change itself would otherwise apply cleanly.
> +test_expect_failure 'cope with rcs keyword expansion damage' '
> + "$GITP4" clone --dest="$git" //depot &&
> + cd "$git" &&

Please do not cd around in test script outside a subshell. Otherwise, any failure in a subsequent command in this && chain before you reach the next "cd" will leave you in an unexpected directory and can break later tests.

The worst example would be

	test_expect_something 'one' '
		mkdir -p git/play/pen &&
                cd git/play/pen &&
                do something &&
        	cd ../../.. &&
                do something that potentially can fail &&
                cd git/play/pen &&
                do something
	'
	# blindly assuming the above succeeded up to the last 'cd git'
        test_expect_something 'two' '
		cd ../../.. &&
        	rm -fr git &&
                do something else
	'

If the first test fails in the middle before it brings you back down to 'git/play/pen' with the last 'cd git/playpen', then the next test moves you up three levels and you will be running "rm -fr" on a wrong 'git'!