Re: [PATCHv2 2/3] t3312-notes-empty: Test that 'git notes' removes empty notes by default

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 5, 2014, 19:00 UTC
Message-ID: <xmqqmw85o4lj.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1415176347-18694-2-git-send-email-johan@herland.net>

Johan Herland <johan@herland.net> writes:

Show 33 quoted lines

> Add test cases documenting the current behavior when trying to
> add/append/edit empty notes. This is in preparation for adding
> --allow-empty; to allow empty notes to be stored.
>
> Improved-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Johan Herland <johan@herland.net>
> ---
>  t/t3312-notes-empty.sh | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
>  create mode 100755 t/t3312-notes-empty.sh
>
> diff --git a/t/t3312-notes-empty.sh b/t/t3312-notes-empty.sh
> new file mode 100755
> index 0000000..44074f6
> --- /dev/null
> +++ b/t/t3312-notes-empty.sh
> @@ -0,0 +1,48 @@
> +#!/bin/sh
> +
> +test_description='Test adding/editing of empty notes'
> +. ./test-lib.sh
> +
> +write_script fake_editor <<\EOF
> +	echo "$MSG" >"$1"
> +EOF
> +GIT_EDITOR=./fake_editor
> +export GIT_EDITOR
> +
> +test_expect_success 'setup' '
> +	test_commit one &&
> +	git log -1 >expect_missing &&
> +	empty_blob=$(git hash-object -w /dev/null)
> +'

Show 6 quoted lines

> +cleanup_notes() {
> +	git update-ref -d refs/notes/commits
> +}
> +
> +verify_missing() {
> +	git log -1 > actual &&

Hmph, it was unclear what exactly you are trying to check with this one and the other "git log -1 >expect_missing".

Perhaps a comment that says "We are interested in the trailing 'Notes: ...' in the output" is necessary here, or (probably even better) use the --format='%N' to make it crystal clear?

> +	test_cmp expect_missing actual &&
> +	! git notes list HEAD

Isn't this test_must_fail (i.e. if somebody screws up to make "git notes list" segfault, the test should fail, not taking the dying with SEGV as a sign of success)?

Show 21 quoted lines

> +}
> +
> +for cmd in \
> +	'add' \
> +	'add -F /dev/null' \
> +	'add -m ""' \
> +	'add -c "$empty_blob"' \
> +	'add -C "$empty_blob"' \
> +	'append' \
> +	'append -F /dev/null' \
> +	'append -m ""' \
> +	'append -c "$empty_blob"' \
> +	'append -C "$empty_blob"' \
> +	'edit'
> +do
> +	test_expect_success "'git notes $cmd' removes empty note" "
> +		cleanup_notes &&
> +		MSG= git notes $cmd &&
> +		verify_missing
> +	"
> +done

Perhaps just a taste issue, but I would think

	while read cmd
        do
        	... that test eval with $cmd interpolation ...
	done <<-\EOF
        add
        add -F /dev/null
        ...
        EOF

would be easier to maintain and to read, without having to worry about quoting and backslashing.

> +
> +test_done