{"thread":{"id":"50899","subject":"[PATCH 0/1] Fix a false negative in t3301-notes.sh","startedAt":"2019-04-09T10:41:25Z","lastAt":"2019-04-09T11:33:14Z","messageCount":3,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"373477","messageId":"pull.176.git.gitgitgadget@gmail.com","threadId":"50899","inReplyTo":null,"subject":"[PATCH 0/1] Fix a false negative in t3301-notes.sh","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-04-09T10:41:22Z","receivedAt":"2019-04-09T10:41:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"It is always bad when test cases fail for the wrong reasons, but it is in\nsome ways more scary when they pass for the wrong reasons.\n\nI stumbled over this issue while chasing down a Windows-specific issue that\ncaused two other test cases to fail, and should have caused this one to\nfail, too, but didn't.\n\nJohannes Schindelin (1):\n t3301: fix false negative\n\n t/t3301-notes.sh | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\n\nbase-commit: 8104ec994ea3849a968b4667d072fedd1e688642\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-176%2Fdscho%2Ffix-false-negative-in-t3301-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-176/dscho/fix-false-negative-in-t3301-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/176\n-- \ngitgitgadget\n"},{"id":"373478","messageId":"a01a8c704ba03213aa59c59384dba46502089522.1554806481.git.gitgitgadget@gmail.com","threadId":"50899","inReplyTo":"pull.176.git.gitgitgadget@gmail.com","subject":"[PATCH 1/1] t3301: fix false negative","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-04-09T10:41:23Z","receivedAt":"2019-04-09T10:41:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn 6956f858f6 (notes: implement helpers needed for note copying during\nrewrite, 2010-03-12), we introduced a test case that verifies that the\nconfig setting `notes.rewriteRef` can be overridden via the environment\nvariable `GIT_NOTES_REWRITE_REF`.\n\nBack when it was introduced, it relied on a side effect of an earlier\ntest case that configured `core.noteRef` to point to `refs/notes/other`.\n\nIn 908a320363 (t3301: modernize style, 2014-11-12), this side effect was\nremoved.\n\nThe test case *still* passed, but for the wrong reason: we no longer\noverrode the rewrite ref, but there simply was nothing to rewrite\nanymore, as the overridden notes ref was \"modernized\" away.\n\nLet's let that test case pass for the correct reason again.\n\nTo make sure of that, let's change the idea of the original test case:\nit configured `notes.rewriteRef` to point to the actual notes ref,\nforced that to be ignored and then verified that the notes were *not*\nrewritten.\n\nBy turning that idea upside down (configure the `notes.rewriteRef` to\nanother notes ref, override it via the environment variable to force the\nnotes to be copied, and then verify that the notes *were* rewritten), we\nmake it much harder for that test case to pass for the wrong reason.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t3301-notes.sh | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t3301-notes.sh b/t/t3301-notes.sh\nindex 84bbf88cf9..704bbc6541 100755\n--- a/t/t3301-notes.sh\n+++ b/t/t3301-notes.sh\n@@ -1120,9 +1120,10 @@ test_expect_success 'GIT_NOTES_REWRITE_REF overrides config' '\n \ttest_config notes.rewriteMode overwrite &&\n \ttest_config notes.rewriteRef refs/notes/other &&\n \techo $(git rev-parse HEAD^) $(git rev-parse HEAD) |\n-\tGIT_NOTES_REWRITE_REF= git notes copy --for-rewrite=foo &&\n+\tGIT_NOTES_REWRITE_REF=refs/notes/commits \\\n+\t\tgit notes copy --for-rewrite=foo &&\n \tgit log -1 >actual &&\n-\ttest_cmp expect actual\n+\tgrep \"replacement note 3\" actual\n '\n \n test_expect_success 'git notes copy diagnoses too many or too few parameters' '\n-- \ngitgitgadget\n"},{"id":"373483","messageId":"xmqq4l775s7d.fsf@gitster-ct.c.googlers.com","threadId":"50899","inReplyTo":"a01a8c704ba03213aa59c59384dba46502089522.1554806481.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] t3301: fix false negative","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-09T11:33:10Z","receivedAt":"2019-04-09T11:33:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> In 6956f858f6 (notes: implement helpers needed for note copying during\n> rewrite, 2010-03-12), we introduced a test case that verifies that the\n> config setting `notes.rewriteRef` can be overridden via the environment\n> variable `GIT_NOTES_REWRITE_REF`.\n>\n> Back when it was introduced, it relied on a side effect of an earlier\n> test case that configured `core.noteRef` to point to `refs/notes/other`.\n>\n> In 908a320363 (t3301: modernize style, 2014-11-12), this side effect was\n> removed.\n>\n> The test case *still* passed, but for the wrong reason: we no longer\n> overrode the rewrite ref, but there simply was nothing to rewrite\n> anymore, as the overridden notes ref was \"modernized\" away.\n\nWow. Thanks for digging thru this\n\n> Let's let that test case pass for the correct reason again.\n>\n> To make sure of that, let's change the idea of the original test case:\n> it configured `notes.rewriteRef` to point to the actual notes ref,\n> forced that to be ignored and then verified that the notes were *not*\n> rewritten.\n>\n> By turning that idea upside down (configure the `notes.rewriteRef` to\n> another notes ref, override it via the environment variable to force the\n> notes to be copied, and then verify that the notes *were* rewritten), we\n> make it much harder for that test case to pass for the wrong reason.\n\nYup. I agree that testing positive side, expressing what we want to\nhappen in a more explicit manner, is always a better alternative.\n\nWill queue. Thanks.\n\n> index 84bbf88cf9..704bbc6541 100755\n> --- a/t/t3301-notes.sh\n> +++ b/t/t3301-notes.sh\n> @@ -1120,9 +1120,10 @@ test_expect_success 'GIT_NOTES_REWRITE_REF overrides config' '\n> \ttest_config notes.rewriteMode overwrite &&\n> \ttest_config notes.rewriteRef refs/notes/other &&\n> \techo $(git rev-parse HEAD^) $(git rev-parse HEAD) |\n> -\tGIT_NOTES_REWRITE_REF= git notes copy --for-rewrite=foo &&\n> +\tGIT_NOTES_REWRITE_REF=refs/notes/commits \\\n> +\t\tgit notes copy --for-rewrite=foo &&\n> \tgit log -1 >actual &&\n> -\ttest_cmp expect actual\n> +\tgrep \"replacement note 3\" actual\n> '\n> \n> test_expect_success 'git notes copy diagnoses too many or too few parameters' '\n"}]}