{"thread":{"id":"37045","subject":"[PATCH] t7300: repair filesystem permissions with test_when_finished","startedAt":"2014-07-02T18:44:30Z","lastAt":"2014-07-22T22:53:59Z","messageCount":8,"participants":["Jeff King","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"245340","messageId":"20140702184429.GA32538@sigill.intra.peff.net","threadId":"37045","inReplyTo":null,"subject":"[PATCH] t7300: repair filesystem permissions with test_when_finished","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-02T18:44:30Z","receivedAt":"2014-07-02T18:44:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We create a directory that cannot be removed, confirm that\nit cannot be removed, and then fix it like:\n\n chmod 0 foo &&\n test_must_fail git clean -d -f &&\n chmod 755 foo\n\nIf the middle step fails but leaves the directory (e.g., the\nbug is that clean does not notice the failure), this\npollutes the test repo with an unremovable directory. Not\nonly does this cause further tests to fail, but it means\nthat \"rm -rf\" fails on the whole trash directory, and the\nuser has to intervene manually to even re-run the test script.\n\nWe can bump the \"chmod 755\" recovery to a test_when_finished\nblock to be sure that it always runs.\n\nSigned-off-by: Jeff King \n---\n t/t7300-clean.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex 74de814..04118ad 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -426,10 +426,10 @@ test_expect_success SANITY 'removal failure' '\n \n \tmkdir foo &&\n \ttouch foo/bar &&\n+\ttest_when_finished \"chmod 755 foo\" &&\n \t(exec Signed-off-by: Jeff King \n> ---\n> t/t7300-clean.sh | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n\nDoes the later \"git clean -d with an unreadable empty directory\" test\nneed the same treatment?\n\nThanks,\nJonathan\n"},{"id":"245343","messageId":"20140702190331.GA9869@sigill.intra.peff.net","threadId":"37045","inReplyTo":"20140702190159.GC5816@google.com","subject":"Re: [PATCH] t7300: repair filesystem permissions with test_when_finished","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-02T19:03:31Z","receivedAt":"2014-07-02T19:03:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 02, 2014 at 12:01:59PM -0700, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > Signed-off-by: Jeff King \n> > ---\n> > t/t7300-clean.sh | 4 ++--\n> > 1 file changed, 2 insertions(+), 2 deletions(-)\n> \n> Does the later \"git clean -d with an unreadable empty directory\" test\n> need the same treatment?\n\nI don't think so, because it is also failing during the experiments I'm\ndoing and it is not causing the same problem.\n\n-Peff\n"},{"id":"245345","messageId":"20140702191114.GA3474@sigill.intra.peff.net","threadId":"37045","inReplyTo":"20140702184429.GA32538@sigill.intra.peff.net","subject":"[RFH] \"git clean\" deletes excluded files in untracked directories","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-02T19:11:14Z","receivedAt":"2014-07-02T19:11:14Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If you have an untracked directory that contains excluded files, like\nthis:\n\n mkdir foo\n echo content >foo/one\n echo content >foo/two\n echo \"foo/one\" >.gitignore\n\nthen \"git clean -d\" will notice that \"foo\" is untracked and recursively\ndelete it and its contents, including the ignored \"foo/one\".\n\nOur stance has always been that ignored files are not precious, so in\nthat sense it is not a big loss. But \"git clean\" does provide a \"-x\"\noption, and takes care to avoid deleting ignored files when it is not\ngiven. So I'd argue that we should delete \"foo/two\" but retain\n\"foo/one\", unless \"-x\" is given.\n\nI'm not sure of the best way to go about that, though. The patch below\nis my first attempt. It stops using DIR_SHOW_OTHER_DIRECTORIES when we\nfind the list of files to delete, which means we get the full set of\npaths (so instead of \"foo/\", we see \"foo/one\" and \"foo/two\" after\ncalling fill_directory). But then we have to notice that \"foo\" should go\naway if all of its contents do. For that, I use remove_path, which just\nwalks up the pathname, removing leading directories until we hit a\nnon-empty one.\n\nBut there are a few problems:\n\n 1. Is it possible that we want to remove \"foo/bar\" but not \"foo\"? The\n obvious case would be that \"foo\" is excluded but \"foo/bar\" is not. I\n guess you could get that with negative excludes, like:\n\n foo/\n !foo/bar\n\n 2. The output is different (we report each file we are removing,\n rather than just the root directory).\n\n 3. The error handling is different. If we have an unreadable untracked\n directory now, fill_directory does not recurse into it; we get the\n directory name and complain when we cannot remove it. But with this\n patch, fill_directory tries to enter it, can't, and simply fails to\n report any entries inside it (this causes test failures in t7300).\n\nSo this is probably not the right way to go about it. I'm not sure of\nthe best way.\n\nOne option is to actually teach clean's recursive remove_dirs function\nto actually check excludes itself for each file. That feels hacky,\nthough, since we should be checking them already in fill_directory.\n\nAnother is to add a flag to fill_directory to indicate that it should\nrecurse into directories to find excluded files, but not otherwise (we\nusually avoid it for reasons of efficiency, but since we would be\nimmediately entering them to delete anyway, I don't think that's a\nconcern here). I think this makes the fill_directory logic quite\ncomplicated, though.\n\nSuggestions?\n\n---\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 9a91515..f159eed 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -914,7 +914,8 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \tif (force > 1)\n \t\trm_flags = 0;\n \n-\tdir.flags |= DIR_SHOW_OTHER_DIRECTORIES;\n+\tif (!remove_directories)\n+\t\tdir.flags |= DIR_SHOW_OTHER_DIRECTORIES;\n \n \tif (read_cache() < 0)\n \t\tdie(_(\"index file corrupt\"));\n@@ -985,7 +986,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \t\t\t\tprintf(dry_run ? _(msg_would_remove) : _(msg_remove), qname);\n \t\t\t}\n \t\t} else {\n-\t\t\tres = dry_run ? 0 : unlink(abs_path.buf);\n+\t\t\tres = dry_run ? 0 : remove_path(abs_path.buf);\n \t\t\tif (res) {\n \t\t\t\tqname = quote_path_relative(item->string, NULL, &buf);\n \t\t\t\twarning(_(msg_warn_remove_failed), qname);\ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex 04118ad..647844f 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -527,4 +527,12 @@ test_expect_success 'git clean -d respects pathspecs (pathspec is prefix of dir)\n \ttest_path_is_dir foobar\n '\n \n+test_expect_success 'clean -d does not clean ignored files' '\n+\tmkdir -p foo &&\n+\techo content >foo/file &&\n+\techo \"foo/*\" >.gitignore &&\n+\tgit clean -df writes:\n\n> On Wed, Jul 02, 2014 at 12:01:59PM -0700, Jonathan Nieder wrote:\n>\n>> Jeff King wrote:\n>> \n>> > Signed-off-by: Jeff King \n>> > ---\n>> > t/t7300-clean.sh | 4 ++--\n>> > 1 file changed, 2 insertions(+), 2 deletions(-)\n>> \n>> Does the later \"git clean -d with an unreadable empty directory\" test\n>> need the same treatment?\n>\n> I don't think so, because it is also failing during the experiments I'm\n> doing and it is not causing the same problem.\n\nIt tests that \"clean -d\" is happy if a blind rmdir(2) removes the\ndirectory. If it fails for whatever reason (e.g. add \"exit(0)\" at\nthe beginning of cmd_clean(), for example) to remove the directory,\nwe do leave an empty unreadable directory behind.\n\nBut as long as that directory is empty, that will not cause us to\nremove the trash directory at the end, so we should be OK.\n"},{"id":"245350","messageId":"xmqq38ejh6er.fsf@gitster.dls.corp.google.com","threadId":"37045","inReplyTo":"xmqq7g3vh6h1.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] t7300: repair filesystem permissions with test_when_finished","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-02T20:06:36Z","receivedAt":"2014-07-02T20:06:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> It tests that \"clean -d\" is happy if a blind rmdir(2) removes the\n> directory. If it fails for whatever reason (e.g. add \"exit(0)\" at\n> the beginning of cmd_clean(), for example) to remove the directory,\n> we do leave an empty unreadable directory behind.\n>\n> But as long as that directory is empty, that will not cause us to\n> remove the trash directory at the end, so we should be OK.\n\ns/remove/fail to &/; sorry for the noise.\n"},{"id":"246528","messageId":"xmqq8unlf4z4.fsf@gitster.dls.corp.google.com","threadId":"37045","inReplyTo":"20140702191114.GA3474@sigill.intra.peff.net","subject":"Re: [RFH] \"git clean\" deletes excluded files in untracked directories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-22T21:39:27Z","receivedAt":"2014-07-22T21:39:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> If you have an untracked directory that contains excluded files, like\n> this:\n>\n> mkdir foo\n> echo content >foo/one\n> echo content >foo/two\n> echo \"foo/one\" >.gitignore\n>\n> then \"git clean -d\" will notice that \"foo\" is untracked and recursively\n> delete it and its contents, including the ignored \"foo/one\".\n\nHmph, starting from an empty repository and doing the above four\ncommands, and \"git clean\" without \"-d\" does not bother removing\neither foo/one or foo/two. Is this correct?\n\nIt gets worse. Following the above four commands and then these two:\n\n >foo/three\n git add foo/two\n\nand \"git clean\" (with or without \"-d\") suddenly notices that\n\"foo/three\" is expendable, but not foo/one nor foo/two, which sounds\nabout right.\n\nHonestly, as I do not use \"git clean\" myself, I do not know what the\n\"right\" behaviour is for that command. Anything it does seems just\narbitrary and wrong to me ;-)\n"},{"id":"246532","messageId":"xmqqvbqpdmyg.fsf@gitster.dls.corp.google.com","threadId":"37045","inReplyTo":"xmqq8unlf4z4.fsf@gitster.dls.corp.google.com","subject":"Re: [RFH] \"git clean\" deletes excluded files in untracked directories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-22T22:53:59Z","receivedAt":"2014-07-22T22:53:59Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> If you have an untracked directory that contains excluded files, like\n>> this:\n>>\n>> mkdir foo\n>> echo content >foo/one\n>> echo content >foo/two\n>> echo \"foo/one\" >.gitignore\n>>\n>> then \"git clean -d\" will notice that \"foo\" is untracked and recursively\n>> delete it and its contents, including the ignored \"foo/one\".\n>\n> Hmph, starting from an empty repository and doing the above four\n> commands, and \"git clean\" without \"-d\" does not bother removing\n> either foo/one or foo/two. Is this correct?\n>\n> It gets worse. Following the above four commands and then these two:\n>\n> >foo/three\n> git add foo/two\n>\n> and \"git clean\" (with or without \"-d\") suddenly notices that\n> \"foo/three\" is expendable, but not foo/one nor foo/two, which sounds\n> about right.\n>\n> Honestly, as I do not use \"git clean\" myself, I do not know what the\n> \"right\" behaviour is for that command. Anything it does seems just\n> arbitrary and wrong to me ;-)\n\nHaving said that...\n\nLet's play my usual \"I do not know what I am doing because I do not\nuse it myself, but instead of leaving the mess other people created\nthat may harm users, let's try to clean it up\", and the first step\nfor that is to clarify what is wrong about it, and what, if\nanything, could be a more sensible behaviour.\n\n\"git clean\" with or without \"-d\" in my two-command added to your\nfour-command example notices .gitignore and foo/three are to be\ncleaned, and further with \"-x\" makes foo/one eligible for\nremoval. I think these are all sensible.\n\n\"git clean\" without \"-d\" in your four-command example says\n\".gitignore is untracked and can be cleaned\", but is \"foo/two\" that\nis not tracked fundamentally different from \".gitignore\"?\n\nThe only difference I can see is one is at the top-level while\nthe other is not. What bad things could happen if we remove\n\"foo/two\" with or without \"-d\", making \"-d\" a no-op?\n\nI know \"-d\" talks about \"untracked directory\", but notice that with\nyour four-command example, there is no \"git add\"; there is no\ndifference between the top-level and \"foo/\" in tracked-ness, so it\nis a meaningless distinction. I could be pursuaded to buy a\nproposal to make \"-d\" almost no-op except that a directory that\nbecomes empty after running \"git clean [-x]\" is rmdir(2)'ed and\nwithout \"-d\" the empty directory is kept.\n\nWould such a pair of changes bring some sanity to the command?\n\nNo matter what we do, I suspect that people expect various\ncombinations of options to \"git clean\" has some parallel to\n\"ls-files -o\" with various combinations of options, to allow them to\nlist paths that will be cleaned with \"ls-files\", store them away,\nand then run \"clean\", or something, so any \"fix\" would involve\nupdating \"ls-files\" in a way consistent with the updated behaviour\nof \"clean\".\n"}]}