Re: [PATCH 4/6] Add a test script for "git notes"

[Junio C Hamano <gitster@pobox.com>]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
> new file mode 100755
> index 0000000..eb50191
> --- /dev/null
> +++ b/t/t3301-notes.sh
> @@ -0,0 +1,63 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2007 Johannes E. Schindelin
> +#
> +
> +test_description='Test commit notes'
> +
> +. ./test-lib.sh
> +
> +test_expect_success setup '
> +	: > a1 &&
> +	git add a1 &&
> +	test_tick &&
> +	git commit -m 1st &&
> +	: > a2 &&
> +	git add a2 &&
> +	test_tick &&
> +	git commit -m 2nd
> +'

Does not test the failure mode of not having a HEAD yet.

> +cat > fake_editor.sh << EOF
> +echo "\$MSG" > "\$1"
> +echo "\$MSG" >& 2
> +EOF

You can avoid all these backslashes by saying:

	cat >fake_editor.sh <<\EOF
        echo "$MSG" >"$1"
        echo "$MSG" >&2
	EOF

> +chmod a+x fake_editor.sh
> +VISUAL="$(pwd)"/fake_editor.sh
> +export VISUAL

Not that it hurts anybody, but do you really need that $(pwd),
instead of "./fake_editor.sh"?

> +
> +test_expect_success 'need notes ref' '
> +	! MSG=1 git notes edit &&
> +	! MSG=2 git notes show
> +'
> +
> +test_expect_success 'create notes' '
> +	git config core.notesRef refs/notes/commits &&
> +	MSG=b1 git notes edit &&
> +cat .git/new-notes &&
> +test b1 = "$(cat .git/new-notes)" &&
> +	test 1 = $(git ls-tree refs/notes/commits | wc -l) &&
> +	test b1 = $(git notes show) &&
> +	git show HEAD^ &&
> +	! git notes show HEAD^
> +'

Is there particular reason for that (lack of) indentation for
the two lines among them?

I think it is a bug to leave ".git/new-notes" and friends
behind.

> +
> +cat > expect << EOF
> +commit 268048bfb8a1fb38e703baceb8ab235421bf80c5
> +Author: A U Thor <author@example.com>
> +Date:   Thu Apr 7 15:14:13 2005 -0700
> +
> +    2nd
> +
> +Notes:
> +    b1
> +EOF
> +
> +test_expect_success 'show notes' '
> +	! (git cat-file commit HEAD | grep b1) &&
> +	git log -1 > output &&
> +	git diff expect output
> +'
> +
> +test_done

Hmph.  This makes the reader wonder why this is not optional,
perhaps linked to --decorate option somehow.