Re: [PATCH 1/1] t3301: fix false negative

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

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> In 6956f858f6 (notes: implement helpers needed for note copying during
> rewrite, 2010-03-12), we introduced a test case that verifies that the
> config setting `notes.rewriteRef` can be overridden via the environment
> variable `GIT_NOTES_REWRITE_REF`.
>
> Back when it was introduced, it relied on a side effect of an earlier
> test case that configured `core.noteRef` to point to `refs/notes/other`.
>
> In 908a320363 (t3301: modernize style, 2014-11-12), this side effect was
> removed.
>
> The test case *still* passed, but for the wrong reason: we no longer
> overrode the rewrite ref, but there simply was nothing to rewrite
> anymore, as the overridden notes ref was "modernized" away.

Wow.  Thanks for digging thru this

> Let's let that test case pass for the correct reason again.
>
> To make sure of that, let's change the idea of the original test case:
> it configured `notes.rewriteRef` to point to the actual notes ref,
> forced that to be ignored and then verified that the notes were *not*
> rewritten.
>
> By turning that idea upside down (configure the `notes.rewriteRef` to
> another notes ref, override it via the environment variable to force the
> notes to be copied, and then verify that the notes *were* rewritten), we
> make it much harder for that test case to pass for the wrong reason.

Yup.  I agree that testing positive side, expressing what we want to
happen in a more explicit manner, is always a better alternative.

Will queue.  Thanks.

> index 84bbf88cf9..704bbc6541 100755
> --- a/t/t3301-notes.sh
> +++ b/t/t3301-notes.sh
> @@ -1120,9 +1120,10 @@ test_expect_success 'GIT_NOTES_REWRITE_REF overrides config' '
>  	test_config notes.rewriteMode overwrite &&
>  	test_config notes.rewriteRef refs/notes/other &&
>  	echo $(git rev-parse HEAD^) $(git rev-parse HEAD) |
> -	GIT_NOTES_REWRITE_REF= git notes copy --for-rewrite=foo &&
> +	GIT_NOTES_REWRITE_REF=refs/notes/commits \
> +		git notes copy --for-rewrite=foo &&
>  	git log -1 >actual &&
> -	test_cmp expect actual
> +	grep "replacement note 3" actual
>  '
>  
>  test_expect_success 'git notes copy diagnoses too many or too few parameters' '