{"thread":{"id":"54446","subject":"[PATCH] t7518: fix flaky grep invocation","startedAt":"2020-10-16T23:39:59Z","lastAt":"2020-10-17T23:35:36Z","messageCount":4,"participants":["Elijah Newren via GitGitGadget","Taylor Blau","Junio C Hamano","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"407779","messageId":"pull.884.git.git.1602891594738.gitgitgadget@gmail.com","threadId":"54446","inReplyTo":null,"subject":"[PATCH] t7518: fix flaky grep invocation","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-16T23:39:54Z","receivedAt":"2020-10-16T23:39:59Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nt7518.1 added in commit 862e80a413 (\"ident: handle NULL email when\ncomplaining of empty name\", 2017-02-23), was trying to make sure that\nthe test with an empty ident did not segfault and did not result in\nglibc quiety translating a NULL pointer into a name of \"(null)\". It did\nthe latter by ensuring that a grep for \"null\" didn't appear in the\noutput, but on one automatic CI run I observed the following output:\n\nfatal: empty ident name (for ) not allowed\n\nNote that 'null' appears as a substring of the domain name, found\nwithin 'gcliasfzo2nullsdbrimjtbyhg'. Tighten the test by searching for\n\"(null)\" rather than \"null\".\n\nSigned-off-by: Elijah Newren \n---\n t7518: fix flaky grep invocation\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-884%2Fnewren%2Ffix-flaky-t7518-grep-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-884/newren/fix-flaky-t7518-grep-v1\nPull-Request: https://github.com/git/git/pull/884\n\n t/t7518-ident-corner-cases.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t7518-ident-corner-cases.sh b/t/t7518-ident-corner-cases.sh\nindex dc3e9c8c88..905957bd0a 100755\n--- a/t/t7518-ident-corner-cases.sh\n+++ b/t/t7518-ident-corner-cases.sh\n@@ -13,7 +13,7 @@ test_expect_success 'empty name and missing email' '\n \t\tsane_unset GIT_AUTHOR_EMAIL &&\n \t\tGIT_AUTHOR_NAME= &&\n \t\ttest_must_fail git commit --allow-empty -m foo 2>err &&\n-\t\ttest_i18ngrep ! null err\n+\t\ttest_i18ngrep ! \"(null)\" err\n \t)\n '\n \n\nbase-commit: d4a392452e292ff924e79ec8458611c0f679d6d4\n-- \ngitgitgadget\n"},{"id":"407782","messageId":"20201017000838.GA2211921@nand.local","threadId":"54446","inReplyTo":"pull.884.git.git.1602891594738.gitgitgadget@gmail.com","subject":"Re: [PATCH] t7518: fix flaky grep invocation","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-10-17T00:08:38Z","receivedAt":"2020-10-17T05:34:53Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Elijah,\n\nOn Fri, Oct 16, 2020 at 11:39:54PM +0000, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n>\n> t7518.1 added in commit 862e80a413 (\"ident: handle NULL email when\n> complaining of empty name\", 2017-02-23), was trying to make sure that\n> the test with an empty ident did not segfault and did not result in\n> glibc quiety translating a NULL pointer into a name of \"(null)\". It did\n> the latter by ensuring that a grep for \"null\" didn't appear in the\n> output, but on one automatic CI run I observed the following output:\n\n:-). I have always been surprised by glibc's behavior here; I'd almost\nalways rather have a segfault than silently turning a NULL pointer into\n\"(null)\" when formatted with \"%s\".\n\n> fatal: empty ident name (for ) not allowed\n>\n> Note that 'null' appears as a substring of the domain name, found\n> within 'gcliasfzo2nullsdbrimjtbyhg'. Tighten the test by searching for\n> \"(null)\" rather than \"null\".\n\nYep; that sounds like a straightforward fix that will make this test\nnon-flaky.\n\n Reviewed-by: Taylor Blau \n\nThanks,\nTaylor\n"},{"id":"407783","messageId":"xmqqr1px90u8.fsf@gitster.c.googlers.com","threadId":"54446","inReplyTo":"pull.884.git.git.1602891594738.gitgitgadget@gmail.com","subject":"Re: [PATCH] t7518: fix flaky grep invocation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-17T00:02:07Z","receivedAt":"2020-10-17T05:39:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> output, but on one automatic CI run I observed the following output:\n>\n> fatal: empty ident name (for ) not allowed\n>\n> Note that 'null' appears as a substring of the domain name, found\n> within 'gcliasfzo2nullsdbrimjtbyhg'. Tighten the test by searching for\n> \"(null)\" rather than \"null\".\n\nWhy do we even need grep again? If we were to segfault, wouldn't\ntest_must_fail catch it for us?\n\n... ahh, OK, your libc may allow NULL left in email and that is what\nwe are trying to catch?\n\nHonestly, I am not sure if it even makes sense to test it like this.\nThe code that the test tries to protect against future breakages\nroughly look like this these days.\n\n\tif (!email) {\n\t\t... one way to assign non-NULL to email ...\n\t}\n\tif (!email) {\n\t\t... do other things to assign non-NULL to email or die ...\n\t}\n\n\tif (want_name) {\n\t\t... here we require email to be set because we show\n\t\t... it in an error message\n\t}\n\nThe original problem was that the code had \"if (want_name)\" part\nfirst before email==NULL condition has been dealt with, and used\nemail==NULL in one of the error messages. The fix was to move the\npart that deals with email==NULL up, as it does not need any of the\neffects that happen in \"if (want_name)\" block.\n\nNow, what is this particular test protecting against? We may again\nmove the \"if (want_name)\" block up and trigger the error message in\nit that uses e-mail while having NULL in email again? If so, would\nit make more sense to do something along this line, lose the \"grep\"\nand keep \"test_must_fail\"? We expect and require at this point in\nthe code (the patch is inside that \"if (want_name)\" block) email\nmust have been set up already, so it would be a BUG() otherwise.\n\nHmm?\n\n ident.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git i/ident.c w/ident.c\nindex 6aba4b5cb6..9b71563d95 100644\n--- i/ident.c\n+++ w/ident.c\n@@ -430,6 +430,8 @@ const char *fmt_ident(const char *name, const char *email,\n \t\t\tif (strict) {\n \t\t\t\tif (using_default)\n \t\t\t\t\tident_env_hint(whose_ident);\n+\t\t\t\tif (!email)\n+\t\t\t\t\tBUG(\"NULL email when we need to complain???\");\n \t\t\t\tdie(_(\"empty ident name (for <%s>) not allowed\"), email);\n \t\t\t}\n \t\t\tpw = xgetpwuid_self(NULL);\n"},{"id":"407823","messageId":"CABPp-BHeoAHjmgxY6N1M57qH6Xx4P+8aaxn20y4ZRWSd7hj5DQ@mail.gmail.com","threadId":"54446","inReplyTo":"xmqqr1px90u8.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] t7518: fix flaky grep invocation","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-10-17T23:35:19Z","receivedAt":"2020-10-17T23:35:36Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Oct 16, 2020 at 5:02 PM Junio C Hamano wrote:\n>\n> \"Elijah Newren via GitGitGadget\" writes:\n>\n> > output, but on one automatic CI run I observed the following output:\n> >\n> > fatal: empty ident name (for ) not allowed\n> >\n> > Note that 'null' appears as a substring of the domain name, found\n> > within 'gcliasfzo2nullsdbrimjtbyhg'. Tighten the test by searching for\n> > \"(null)\" rather than \"null\".\n>\n> Why do we even need grep again? If we were to segfault, wouldn't\n> test_must_fail catch it for us?\n>\n> ... ahh, OK, your libc may allow NULL left in email and that is what\n> we are trying to catch?\n\nYes, as mentioned in the commit message:\n\n ...was trying to make sure that\n the test with an empty ident did not segfault and did not result in\n glibc quiety translating a NULL pointer into a name of \"(null)\".\n\nThis was also taken from the comment in the file just above the code\nin question (though it was just barely outside the context region).\n\n> Honestly, I am not sure if it even makes sense to test it like this.\n> The code that the test tries to protect against future breakages\n> roughly look like this these days.\n>\n> if (!email) {\n> ... one way to assign non-NULL to email ...\n> }\n> if (!email) {\n> ... do other things to assign non-NULL to email or die ...\n> }\n>\n> if (want_name) {\n> ... here we require email to be set because we show\n> ... it in an error message\n> }\n>\n> The original problem was that the code had \"if (want_name)\" part\n> first before email==NULL condition has been dealt with, and used\n> email==NULL in one of the error messages. The fix was to move the\n> part that deals with email==NULL up, as it does not need any of the\n> effects that happen in \"if (want_name)\" block.\n>\n> Now, what is this particular test protecting against? We may again\n> move the \"if (want_name)\" block up and trigger the error message in\n> it that uses e-mail while having NULL in email again? If so, would\n> it make more sense to do something along this line, lose the \"grep\"\n> and keep \"test_must_fail\"? We expect and require at this point in\n> the code (the patch is inside that \"if (want_name)\" block) email\n> must have been set up already, so it would be a BUG() otherwise.\n>\n> Hmm?\n\nIt may come down to the angle you view it from; \"the current code\ndoesn't have such a problem and can't within its newer design, so why\ntest it?\" vs. \"when you add a regression test at the time you make a\nbugfix, the regression test should demonstrate the failure with the\nold version of the code\". Without the grep, the test would not have\nfailed on the original old version of the code, at least not on some\nplatforms.\nI'm guessing that's why Peff put it there, and still had a preference\nto leave the grep in (see his comments on this issue at\nhttps://lore.kernel.org/git/20201016200031.GA3355643@coredump.intra.peff.net/;\nthis is a case where it'd be nice if gitgitgadget accepted an\nin-reply-to flag of some sort).\n\nHowever, although I have a slight preference to fixing what grep is\nsearching for, my primary concern was with avoiding flakiness in the\ntest. I'm happy if someone wants to make additional code changes or\ntweak the testcase here so long as we avoid the flakiness.\n\n> ident.c | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n> diff --git i/ident.c w/ident.c\n> index 6aba4b5cb6..9b71563d95 100644\n> --- i/ident.c\n> +++ w/ident.c\n> @@ -430,6 +430,8 @@ const char *fmt_ident(const char *name, const char *email,\n> if (strict) {\n> if (using_default)\n> ident_env_hint(whose_ident);\n> + if (!email)\n> + BUG(\"NULL email when we need to complain???\");\n> die(_(\"empty ident name (for <%s>) not allowed\"), email);\n> }\n> pw = xgetpwuid_self(NULL);\n"}]}