{"thread":{"id":"21777","subject":"[RFC/PATCH] t7011: Mark fixed test as such","startedAt":"2009-11-28T18:24:59Z","lastAt":"2009-11-30T13:18:20Z","messageCount":6,"participants":["Michael J Gruber","Nguyen Thai Ngoc Duy"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"128632","messageId":"0327ed3d7c4621f205d2d111254d716bd1b06c28.1259432535.git.git@drmicha.warpmail.net","threadId":"21777","inReplyTo":null,"subject":"[RFC/PATCH] t7011: Mark fixed test as such","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2009-11-28T18:24:59Z","receivedAt":"2009-11-28T18:24:59Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Test 16/17 had been fixed since its introduction in b4d1690 (Teach Git\nto respect skip-worktree bit (reading part), 2009-08-20). So, mark it as\nexpect_success rather than expect_failure.\n\nSigned-off-by: Michael J Gruber \n---\nI'm actually wondering about 17/17 as well.\nIf commit is called with a file name then shouldn't it simply commit the\ncurrent state of the file in the worktree, no matter what the index or\nskip-worktree say? I therefore think 17/17 should be expect_success\nand have no test_must_fail.\n\n t/t7011-skip-worktree-reading.sh | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/t/t7011-skip-worktree-reading.sh b/t/t7011-skip-worktree-reading.sh\nindex e996928..8960dd9 100755\n--- a/t/t7011-skip-worktree-reading.sh\n+++ b/t/t7011-skip-worktree-reading.sh\n@@ -148,7 +148,7 @@ test_expect_success 'git-rm succeeds on skip-worktree absent entries' '\n \tgit rm 1\n '\n \n-test_expect_failure 'commit on skip-worktree absent entries' '\n+test_expect_success 'commit on skip-worktree absent entries' '\n \tgit reset &&\n \tsetup_absent &&\n \ttest_must_fail git commit -m null 1\n-- \n1.6.6.rc0.274.g71380\n"},{"id":"128672","messageId":"fcaeb9bf0911290047t43ea3040x730e04baa81d8a98@mail.gmail.com","threadId":"21777","inReplyTo":"0327ed3d7c4621f205d2d111254d716bd1b06c28.1259432535.git.git@drmicha.warpmail.net","subject":"Re: [RFC/PATCH] t7011: Mark fixed test as such","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2009-11-29T08:47:35Z","receivedAt":"2009-11-29T08:47:35Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On 11/29/09, Michael J Gruber wrote:\n> Test 16/17 had been fixed since its introduction in b4d1690 (Teach Git\n> to respect skip-worktree bit (reading part), 2009-08-20). So, mark it as\n> expect_success rather than expect_failure.\n>\n> Signed-off-by: Michael J Gruber \n\nNo ACK. See below.\n\n> ---\n> I'm actually wondering about 17/17 as well.\n> If commit is called with a file name then shouldn't it simply commit the\n> current state of the file in the worktree, no matter what the index or\n> skip-worktree say? I therefore think 17/17 should be expect_success\n> and have no test_must_fail.\n\nBoth 16/17 and 17/17 ensure that Git won't look at files on worktree\nif they are marked as skip-worktree (by definition of skip-worktree,\nyou can safely ignore worktree, otherwise you would not mark them as\nsuch). 16/17 happens to pass, not because it does not touch worktree,\nbut because the base index does not have \"1\", which happens to is the\nsame situation in 16/17 (test commit when \"1\" is gone). The result is\nOK but it is actually not (17/17 shows this clearer as it commits the\nworktree version).\n-- \nDuy\n"},{"id":"128686","messageId":"4B127DC0.4020108@drmicha.warpmail.net","threadId":"21777","inReplyTo":"fcaeb9bf0911290047t43ea3040x730e04baa81d8a98@mail.gmail.com","subject":"Re: [RFC/PATCH] t7011: Mark fixed test as such","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2009-11-29T13:57:20Z","receivedAt":"2009-11-29T13:57:20Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Nguyen Thai Ngoc Duy venit, vidit, dixit 29.11.2009 09:47:\n> On 11/29/09, Michael J Gruber wrote:\n>> Test 16/17 had been fixed since its introduction in b4d1690 (Teach Git\n>> to respect skip-worktree bit (reading part), 2009-08-20). So, mark it as\n>> expect_success rather than expect_failure.\n>>\n>> Signed-off-by: Michael J Gruber \n> \n> No ACK. See below.\n> \n>> ---\n>> I'm actually wondering about 17/17 as well.\n>> If commit is called with a file name then shouldn't it simply commit the\n>> current state of the file in the worktree, no matter what the index or\n>> skip-worktree say? I therefore think 17/17 should be expect_success\n>> and have no test_must_fail.\n> \n> Both 16/17 and 17/17 ensure that Git won't look at files on worktree\n> if they are marked as skip-worktree (by definition of skip-worktree,\n> you can safely ignore worktree, otherwise you would not mark them as\n> such). 16/17 happens to pass, not because it does not touch worktree,\n> but because the base index does not have \"1\", which happens to is the\n> same situation in 16/17 (test commit when \"1\" is gone). The result is\n> OK but it is actually not (17/17 shows this clearer as it commits the\n> worktree version).\n\nOn 16/17, I really cannot agree. You explain that you expect the test to\nsucceed (we agree here), but that it succeeds for the wrong reasons. So\nit should be either \"expect_success\", or the test itself should be\nchanged so that it really tests what it intends to, otherwise it raises\na wrong \"FIXED\". I suggested and submitted the former.\n\nOn 17/17, it's not clear what should happen. \"skip-worktree\" says ignore\nthe worktree and look in the index instead of accessing worktree files.\nBut \"git commit file\" says ignore the index and stage and commit the\nfile from the worktree directly. And that is exactly what happens:\n\nYou say \"git commit file\".\nThat means \"ignore the index\".\nThat also means that git ignores the skip-worktree bit which is set in\nthe index.\nTherefore, file is committed with the content is has in the worktree.\n\nI'm going by the documentation for git-update-index and git-commit. It\ncould be that they are wrong, too, but they agree with the code, so\nwhat's the reference for saying both code and documentation are wrong?\n\nMichael\n"},{"id":"128711","messageId":"fcaeb9bf0911291756g1857fe32r8da8e50f0a137119@mail.gmail.com","threadId":"21777","inReplyTo":"4B127DC0.4020108@drmicha.warpmail.net","subject":"Re: [RFC/PATCH] t7011: Mark fixed test as such","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2009-11-30T01:56:52Z","receivedAt":"2009-11-30T01:56:52Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Nov 29, 2009 at 8:57 PM, Michael J Gruber\n wrote:\n> Nguyen Thai Ngoc Duy venit, vidit, dixit 29.11.2009 09:47:\n>> On 11/29/09, Michael J Gruber wrote:\n>>> Test 16/17 had been fixed since its introduction in b4d1690 (Teach Git\n>>>  to respect skip-worktree bit (reading part), 2009-08-20). So, mark it as\n>>>  expect_success rather than expect_failure.\n>>>\n>>>  Signed-off-by: Michael J Gruber \n>>\n>> No ACK. See below.\n>>\n>>>  ---\n>>>  I'm actually wondering about 17/17 as well.\n>>>  If commit is called with a file name then shouldn't it simply commit the\n>>>  current state of the file in the worktree, no matter what the index or\n>>>  skip-worktree say? I therefore think 17/17 should be expect_success\n>>>  and have no test_must_fail.\n>>\n>> Both 16/17 and 17/17 ensure that Git won't look at files on worktree\n>> if they are marked as skip-worktree (by definition of skip-worktree,\n>> you can safely ignore worktree, otherwise you would not mark them as\n>> such). 16/17 happens to pass, not because it does not touch worktree,\n>> but because the base index does not have \"1\", which happens to is the\n>> same situation in 16/17 (test commit when \"1\" is gone). The result is\n>> OK but it is actually not (17/17 shows this clearer as it commits the\n>> worktree version).\n>\n> On 16/17, I really cannot agree. You explain that you expect the test to\n> succeed (we agree here), but that it succeeds for the wrong reasons. So\n> it should be either \"expect_success\", or the test itself should be\n> changed so that it really tests what it intends to, otherwise it raises\n> a wrong \"FIXED\". I suggested and submitted the former.\n\nThat was my bad in setting up the environment for 16/17. I will fix\nthat in the next roll of nd/sparse.\n\n> On 17/17, it's not clear what should happen. \"skip-worktree\" says ignore\n> the worktree and look in the index instead of accessing worktree files.\n> But \"git commit file\" says ignore the index and stage and commit the\n> file from the worktree directly. And that is exactly what happens:\n>\n> You say \"git commit file\".\n> That means \"ignore the index\".\n> That also means that git ignores the skip-worktree bit which is set in\n> the index.\n> Therefore, file is committed with the content is has in the worktree.\n\nTo me, no command should break out skip-worktree mask. In reality I\nwould expect that case 17/17 only happens when a user accidentally\nleaves a file that is marked skip-worktree and tries to commit it. An\nerror would be more appropriate to keep consistency with other\ncommands (\"git diff HEAD -- 1\" would show nothing before committing),\nand to warn the user that he/she is stepping on the border. He/she can\nthen choose to extend worktree area if still wants to commit that\nfile. How does that sound?\n\n> I'm going by the documentation for git-update-index and git-commit. It\n> could be that they are wrong, too, but they agree with the code, so\n> what's the reference for saying both code and documentation are wrong?\n\nBoth code and documentation are for Git without skip-worktree. If you\nagree with my reasoning above, I will update documentation to reflect\nthis too.\n-- \nDuy\n"},{"id":"128762","messageId":"4B13BF3D.7010503@drmicha.warpmail.net","threadId":"21777","inReplyTo":"fcaeb9bf0911291756g1857fe32r8da8e50f0a137119@mail.gmail.com","subject":"Re: [RFC/PATCH] t7011: Mark fixed test as such","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2009-11-30T12:49:01Z","receivedAt":"2009-11-30T12:49:01Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Nguyen Thai Ngoc Duy venit, vidit, dixit 30.11.2009 02:56:\n> On Sun, Nov 29, 2009 at 8:57 PM, Michael J Gruber\n> wrote:\n>> Nguyen Thai Ngoc Duy venit, vidit, dixit 29.11.2009 09:47:\n>>> On 11/29/09, Michael J Gruber wrote:\n>>>> Test 16/17 had been fixed since its introduction in b4d1690 (Teach Git\n>>>> to respect skip-worktree bit (reading part), 2009-08-20). So, mark it as\n>>>> expect_success rather than expect_failure.\n>>>>\n>>>> Signed-off-by: Michael J Gruber \n>>>\n>>> No ACK. See below.\n>>>\n>>>> ---\n>>>> I'm actually wondering about 17/17 as well.\n>>>> If commit is called with a file name then shouldn't it simply commit the\n>>>> current state of the file in the worktree, no matter what the index or\n>>>> skip-worktree say? I therefore think 17/17 should be expect_success\n>>>> and have no test_must_fail.\n>>>\n>>> Both 16/17 and 17/17 ensure that Git won't look at files on worktree\n>>> if they are marked as skip-worktree (by definition of skip-worktree,\n>>> you can safely ignore worktree, otherwise you would not mark them as\n>>> such). 16/17 happens to pass, not because it does not touch worktree,\n>>> but because the base index does not have \"1\", which happens to is the\n>>> same situation in 16/17 (test commit when \"1\" is gone). The result is\n>>> OK but it is actually not (17/17 shows this clearer as it commits the\n>>> worktree version).\n>>\n>> On 16/17, I really cannot agree. You explain that you expect the test to\n>> succeed (we agree here), but that it succeeds for the wrong reasons. So\n>> it should be either \"expect_success\", or the test itself should be\n>> changed so that it really tests what it intends to, otherwise it raises\n>> a wrong \"FIXED\". I suggested and submitted the former.\n> \n> That was my bad in setting up the environment for 16/17. I will fix\n> that in the next roll of nd/sparse.\n> \n>> On 17/17, it's not clear what should happen. \"skip-worktree\" says ignore\n>> the worktree and look in the index instead of accessing worktree files.\n>> But \"git commit file\" says ignore the index and stage and commit the\n>> file from the worktree directly. And that is exactly what happens:\n>>\n>> You say \"git commit file\".\n>> That means \"ignore the index\".\n>> That also means that git ignores the skip-worktree bit which is set in\n>> the index.\n>> Therefore, file is committed with the content is has in the worktree.\n> \n> To me, no command should break out skip-worktree mask. In reality I\n> would expect that case 17/17 only happens when a user accidentally\n> leaves a file that is marked skip-worktree and tries to commit it. An\n> error would be more appropriate to keep consistency with other\n> commands (\"git diff HEAD -- 1\" would show nothing before committing),\n> and to warn the user that he/she is stepping on the border. He/she can\n> then choose to extend worktree area if still wants to commit that\n> file. How does that sound?\n> \n>> I'm going by the documentation for git-update-index and git-commit. It\n>> could be that they are wrong, too, but they agree with the code, so\n>> what's the reference for saying both code and documentation are wrong?\n> \n> Both code and documentation are for Git without skip-worktree. If you\n> agree with my reasoning above, I will update documentation to reflect\n> this too.\n\nThanks for the clarification. I see the background behind 17/17 now. I\nthink git-commit.txt needs a careful update then, because the\nexplanation of a common feature (git commit file) should not get\nconfusing because of a not so common feature (skip-worktree/sparse co).\n\nRegarding 16/17: Seeing a FIXED is very misleading. I caught it during\nmy work on a patch series and was confused. FIXED usually occurs only\nwhile working on a fix, before adjusting the test. So, unless the reroll\nof nd/sparse is to happen very soon, I still suggest marking it as\nexpect_success as proposed, and then modifying the test during the reroll.\n\nCheers,\nMichael\n"},{"id":"128768","messageId":"fcaeb9bf0911300518n4d4f96fdg28b13e45c2a4462d@mail.gmail.com","threadId":"21777","inReplyTo":"4B13BF3D.7010503@drmicha.warpmail.net","subject":"Re: [RFC/PATCH] t7011: Mark fixed test as such","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2009-11-30T13:18:20Z","receivedAt":"2009-11-30T13:18:20Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On 11/30/09, Michael J Gruber wrote:\n> Regarding 16/17: Seeing a FIXED is very misleading. I caught it during\n> my work on a patch series and was confused. FIXED usually occurs only\n> while working on a fix, before adjusting the test. So, unless the reroll\n> of nd/sparse is to happen very soon, I still suggest marking it as\n> expect_success as proposed, and then modifying the test during the reroll.\n\nI won't work on it until this weekend. If you insist, then better put\ncommand \"true\" after test_must_fail to indicate that the test is\nbroken (could be line-wrapped by gmail, but it's simple enough to\nrecreate)\n\ndiff --git a/t/t7011-skip-worktree-reading.sh b/t/t7011-skip-worktree-reading.sh\nindex e996928..da8dcbb 100755\n--- a/t/t7011-skip-worktree-reading.sh\n+++ b/t/t7011-skip-worktree-reading.sh\n@@ -152,6 +152,7 @@ test_expect_failure 'commit on skip-worktree\nabsent entries' '\n \tgit reset &&\n \tsetup_absent &&\n \ttest_must_fail git commit -m null 1\n+\ttrue\n '\n\n test_expect_failure 'commit on skip-worktree dirty entries' '\n-- \nDuy\n"}]}