{"thread":{"id":"42960","subject":"[PATCH] Fix failing test t3700-add.sh","startedAt":"2016-07-29T12:43:23Z","lastAt":"2016-07-29T16:49:42Z","messageCount":4,"participants":["Ingo Brückl","Johannes Sixt","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"292510","messageId":"579b4ca1.18da2703.bm000@wupperonline.de","threadId":"42960","inReplyTo":null,"subject":"[PATCH] Fix failing test t3700-add.sh","fromName":"Ingo Brückl","fromEmail":"ib@wupperonline.de","sentAt":"2016-07-29T12:31:28Z","receivedAt":"2016-07-29T12:43:23Z","isPatch":true,"sender":{"key":"ib@wupperonline.de","avatar":"https://avatars.githubusercontent.com/u/123327?v=4"},"body":"At the time of the test xfoo1 already exists and is a link.\nAs a result, the check for file mode 100644 fails.\n\nCreate not yet existing file xfoo instead.\n\nSigned-off-by: Ingo Brückl <ib@wupperonline.de>\n---\n t/t3700-add.sh | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t3700-add.sh b/t/t3700-add.sh\nindex 4865304..aee61b9 100755\n--- a/t/t3700-add.sh\n+++ b/t/t3700-add.sh\n@@ -342,12 +342,12 @@ test_expect_success 'git add --chmod=+x stages a non-executable file with +x' '\n '\n\n test_expect_success 'git add --chmod=-x stages an executable file with -x' '\n-\techo foo >xfoo1 &&\n-\tchmod 755 xfoo1 &&\n-\tgit add --chmod=-x xfoo1 &&\n-\tcase \"$(git ls-files --stage xfoo1)\" in\n-\t100644\" \"*xfoo1) echo pass;;\n-\t*) echo fail; git ls-files --stage xfoo1; (exit 1);;\n+\techo foo >xfoo &&\n+\tchmod 755 xfoo &&\n+\tgit add --chmod=-x xfoo &&\n+\tcase \"$(git ls-files --stage xfoo)\" in\n+\t100644\" \"*xfoo) echo pass;;\n+\t*) echo fail; git ls-files --stage xfoo; (exit 1);;\n \tesac\n '\n\n--\n2.9.2\n\n"},{"id":"292527","messageId":"6708d224-9a44-1446-8543-7c3c6403e506@kdbg.org","threadId":"42960","inReplyTo":"579b4ca1.18da2703.bm000@wupperonline.de","subject":"Re: [PATCH] Fix failing test t3700-add.sh","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-07-29T16:23:03Z","receivedAt":"2016-07-29T16:23:13Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 29.07.2016 um 14:31 schrieb Ingo Brückl:\n> At the time of the test xfoo1 already exists and is a link.\n> As a result, the check for file mode 100644 fails.\n>\n> Create not yet existing file xfoo instead.\n>\n> Signed-off-by: Ingo Brückl <ib@wupperonline.de>\n> ---\n>  t/t3700-add.sh | 12 ++++++------\n>  1 file changed, 6 insertions(+), 6 deletions(-)\n>\n> diff --git a/t/t3700-add.sh b/t/t3700-add.sh\n> index 4865304..aee61b9 100755\n> --- a/t/t3700-add.sh\n> +++ b/t/t3700-add.sh\n> @@ -342,12 +342,12 @@ test_expect_success 'git add --chmod=+x stages a non-executable file with +x' '\n>  '\n>\n>  test_expect_success 'git add --chmod=-x stages an executable file with -x' '\n> -\techo foo >xfoo1 &&\n> -\tchmod 755 xfoo1 &&\n> -\tgit add --chmod=-x xfoo1 &&\n> -\tcase \"$(git ls-files --stage xfoo1)\" in\n> -\t100644\" \"*xfoo1) echo pass;;\n> -\t*) echo fail; git ls-files --stage xfoo1; (exit 1);;\n> +\techo foo >xfoo &&\n> +\tchmod 755 xfoo &&\n> +\tgit add --chmod=-x xfoo &&\n> +\tcase \"$(git ls-files --stage xfoo)\" in\n> +\t100644\" \"*xfoo) echo pass;;\n> +\t*) echo fail; git ls-files --stage xfoo; (exit 1);;\n>  \tesac\n>  '\n\nThe commit that added this test is already 2 months old. How could that \nhave been missed?\n\nIn fact, I cannot verify that there is xfoo1 in the directory or in the \nindex before this test case runs. The general statement that the commit \nmessage makes is clearly not correct. What am I missing?\n\n-- Hannes\n\n"},{"id":"292528","messageId":"20160729163937.GD29773@sigill.intra.peff.net","threadId":"42960","inReplyTo":"579b4ca1.18da2703.bm000@wupperonline.de","subject":"Re: [PATCH] Fix failing test t3700-add.sh","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-29T16:39:38Z","receivedAt":"2016-07-29T16:39:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"[+cc Ed, who wrote 4e55ed3 (add: add --chmod=+x / --chmod=-x options,\n2016-05-31)]\n\nOn Fri, Jul 29, 2016 at 02:31:28PM +0200, Ingo Brückl wrote:\n\n> At the time of the test xfoo1 already exists and is a link.\n> As a result, the check for file mode 100644 fails.\n> \n> Create not yet existing file xfoo instead.\n\nHrm. So in the original code:\n\n>  test_expect_success 'git add --chmod=-x stages an executable file with -x' '\n> -\techo foo >xfoo1 &&\n> -\tchmod 755 xfoo1 &&\n> -\tgit add --chmod=-x xfoo1 &&\n> -\tcase \"$(git ls-files --stage xfoo1)\" in\n> -\t100644\" \"*xfoo1) echo pass;;\n> -\t*) echo fail; git ls-files --stage xfoo1; (exit 1);;\n\nI would have expected \"git add --chmod\" to drop the \"-x\" bit in addition\nto actually overwriting the file contents (and switching a symlink to a\nfile). And it does. The culprit is actually the \"echo foo >xfoo1\" line.\nIf \"xfoo1\" is a symlink, then it silently writes to the symlink\ndestination, and xfoo1 remains a symlink (and thus tweaking its execute\nbit is a noop).\n\nI was also puzzled why the test fails for you; it does not for me.\nRunning the test script as root does make it fail. There are some\nearlier tests which are skipped in this case, which run \"git reset\n--hard\" with xfoo1 in the index, which cleans it up.\n\n> +\techo foo >xfoo &&\n> +\tchmod 755 xfoo &&\n> +\tgit add --chmod=-x xfoo &&\n> +\tcase \"$(git ls-files --stage xfoo)\" in\n> +\t100644\" \"*xfoo) echo pass;;\n> +\t*) echo fail; git ls-files --stage xfoo; (exit 1);;\n\nHere you just pick another name, \"xfoo\", which does happen to work. But\nit seems like that has the same potential for flakiness if earlier tests\nget adjusted or skipped, since they also use that name.\n\nHow about just:\n\n  rm -f xfoo1\n\nat the top of the test, which explicitly documents the state we are\nlooking for?\n\nI also wondered if this test, which calls \"chmod 755 xfoo1\", should be\nmarked with the POSIXPERM prerequisite. But I guess since its goal is to\nstrip the executable bit, it \"works\" even on systems where that chmod is\na noop (the \"git add --chmod\" doesn't do anything, but one way or the\nother we end up at the end state we expect).\n\n-Peff\n"},{"id":"292530","messageId":"xmqqmvl0pext.fsf@gitster.mtv.corp.google.com","threadId":"42960","inReplyTo":"20160729163937.GD29773@sigill.intra.peff.net","subject":"Re: [PATCH] Fix failing test t3700-add.sh","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-29T16:49:34Z","receivedAt":"2016-07-29T16:49:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I was also puzzled why the test fails for you; it does not for me.\n> Running the test script as root does make it fail. There are some\n> earlier tests which are skipped in this case, which run \"git reset\n> --hard\" with xfoo1 in the index, which cleans it up.\n>\n>> +\techo foo >xfoo &&\n>> +\tchmod 755 xfoo &&\n>> +\tgit add --chmod=-x xfoo &&\n>> +\tcase \"$(git ls-files --stage xfoo)\" in\n>> +\t100644\" \"*xfoo) echo pass;;\n>> +\t*) echo fail; git ls-files --stage xfoo; (exit 1);;\n>\n> Here you just pick another name, \"xfoo\", which does happen to work. But\n> it seems like that has the same potential for flakiness if earlier tests\n> get adjusted or skipped, since they also use that name.\n>\n> How about just:\n>\n>   rm -f xfoo1\n>\n> at the top of the test, which explicitly documents the state we are\n> looking for?\n\nThat's much more sensible.\n\n> I also wondered if this test, which calls \"chmod 755 xfoo1\", should be\n> marked with the POSIXPERM prerequisite. But I guess since its goal is to\n> strip the executable bit, it \"works\" even on systems where that chmod is\n> a noop (the \"git add --chmod\" doesn't do anything, but one way or the\n> other we end up at the end state we expect).\n\nWe could make sure --chmod=[-+]x works both ways, which would be\nmore robust on either type of underlying platform.  Something along\nthe lines of\n\n\techo foo >xfoo1 &&\n        git add --chmod=+x xfoo1 &&\n        test_mode_in_index 100755 xfoo1 &&\n        git add --chmod=-x xfoo1 &&\n        test_mode_in_index 100644 xfoo1\n\nwith an obvious addition of a test_mode_in_index helper function as\nthe same \"case $(ls-files -s) in ... esac\" pattern appears number of\ntimes.\n\n"}]}