{"thread":{"id":"45580","subject":"[PATCH] [GSOC] prune_worktrees(): reimplement with dir_iterator","startedAt":"2017-04-01T01:21:49Z","lastAt":"2017-04-01T18:33:12Z","messageCount":4,"participants":["Robert Stanca","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"315976","messageId":"1491009687-10419-1-git-send-email-robert.stanca7@gmail.com","threadId":"45580","inReplyTo":null,"subject":"[PATCH] [GSOC] prune_worktrees(): reimplement with dir_iterator","fromName":"Robert Stanca","fromEmail":"robert.stanca7@gmail.com","sentAt":"2017-04-01T01:21:27Z","receivedAt":"2017-04-01T01:21:49Z","isPatch":true,"sender":{"key":"robert.stanca7@gmail.com","avatar":null},"body":"Replaces recursive traversing of opendir with dir_iterator\n\nSigned-off-by: Robert Stanca \n---\n builtin/worktree.c | 17 +++++++----------\n 1 file changed, 7 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 9993ded..7cfd78c 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -10,6 +10,8 @@\n #include \"refs.h\"\n #include \"utf8.h\"\n #include \"worktree.h\"\n+#include \"iterator.h\"\n+#include \"dir-iterator.h\"\n \n static const char * const worktree_usage[] = {\n \tN_(\"git worktree add [] []\"),\n@@ -91,30 +93,25 @@ static void prune_worktrees(void)\n {\n \tstruct strbuf reason = STRBUF_INIT;\n \tstruct strbuf path = STRBUF_INIT;\n-\tDIR *dir = opendir(git_path(\"worktrees\"));\n-\tstruct dirent *d;\n+\tstruct dir_iterator *diter = dir_iterator_begin(git_path(\"worktrees\"));\n \tint ret;\n-\tif (!dir)\n-\t\treturn;\n-\twhile ((d = readdir(dir)) != NULL) {\n-\t\tif (is_dot_or_dotdot(d->d_name))\n-\t\t\tcontinue;\n+\n+\twhile (dir_iterator_advance(diter) == ITER_OK) {\n \t\tstrbuf_reset(&reason);\n-\t\tif (!prune_worktree(d->d_name, &reason))\n+\t\tif (!prune_worktree(diter->relative_path, &reason))\n \t\t\tcontinue;\n \t\tif (show_only || verbose)\n \t\t\tprintf(\"%s\\n\", reason.buf);\n \t\tif (show_only)\n \t\t\tcontinue;\n \t\tstrbuf_reset(&path);\n-\t\tstrbuf_addstr(&path, git_path(\"worktrees/%s\", d->d_name));\n+\t\tstrbuf_addstr(&path, git_path(\"worktrees/%s\", diter->relative_path));\n \t\tret = remove_dir_recursively(&path, 0);\n \t\tif (ret < 0 && errno == ENOTDIR)\n \t\t\tret = unlink(path.buf);\n \t\tif (ret)\n \t\t\terror_errno(_(\"failed to remove '%s'\"), path.buf);\n \t}\n-\tclosedir(dir);\n \tif (!show_only)\n \t\trmdir(git_path(\"worktrees\"));\n \tstrbuf_release(&reason);\n-- \n2.7.4\n\n"},{"id":"315980","messageId":"xmqqlgrkx3l5.fsf@gitster.mtv.corp.google.com","threadId":"45580","inReplyTo":"1491009687-10419-1-git-send-email-robert.stanca7@gmail.com","subject":"Re: [PATCH] [GSOC] prune_worktrees(): reimplement with dir_iterator","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-01T02:29:42Z","receivedAt":"2017-04-01T02:29:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Robert Stanca writes:\n\n> Replaces recursive traversing of opendir with dir_iterator\n\nThe original code for this one, and also the other one, is not\nrecursive traversing. This one enumerates all the _direct_\nsubdirectories of \".git/worktrees\", and feeds them to\nprune_worktree() without recursing. The other one scans all the\nfiles _directly_ underneath \".git/objects/pack\" to find the ones\nthat begin with the packtmp prefix, and unlinks them without\nrecursing. Neither of them touches anything in subdirectory of\n\".git/worktrees/\" nor \".git/objects/pack/\".\n\nUsing dir_iterator() to make it recursive is not just overkill but\nis a positively wrong change, isn't it?\n"},{"id":"316005","messageId":"CAJYcaSOMkoAnW82g=moQxz0H9t7C+F++V4Ob1-UiTLiw=gm_0g@mail.gmail.com","threadId":"45580","inReplyTo":"xmqqlgrkx3l5.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] [GSOC] prune_worktrees(): reimplement with dir_iterator","fromName":"Robert Stanca","fromEmail":"robert.stanca7@gmail.com","sentAt":"2017-04-01T13:29:29Z","receivedAt":"2017-04-01T13:29:36Z","isPatch":true,"sender":{"key":"robert.stanca7@gmail.com","avatar":null},"body":"On Sat, Apr 1, 2017 at 5:29 AM, Junio C Hamano wrote:\n>\n> Robert Stanca writes:\n>\n> > Replaces recursive traversing of opendir with dir_iterator\n>\n> The original code for this one, and also the other one, is not\n> recursive traversing. This one enumerates all the _direct_\n> subdirectories of \".git/worktrees\", and feeds them to\n> prune_worktree() without recursing. The other one scans all the\n> files _directly_ underneath \".git/objects/pack\" to find the ones\n> that begin with the packtmp prefix, and unlinks them without\n> recursing. Neither of them touches anything in subdirectory of\n> \".git/worktrees/\" nor \".git/objects/pack/\".\n>\n> Using dir_iterator() to make it recursive is not just overkill but\n> is a positively wrong change, isn't it?\n\n\nThanks for the review, and yes the commit message is misleading (my\nbad there). I understood that remove_temp_files has no recursion\ncomponent to it and it removes all \".tmp-id-pack-\" files from\n/objects/pack , but shouldn't dir-iterator work even if there's no\nrecursion level?\nMy understanding of dir-iterator is that it is used for directory\niteration (recursive or not) and where are no subdirectories it's\nbasically recursive with level = 0 .\n"},{"id":"316020","messageId":"xmqq7f34ugf2.fsf@gitster.mtv.corp.google.com","threadId":"45580","inReplyTo":"CAJYcaSOMkoAnW82g=moQxz0H9t7C+F++V4Ob1-UiTLiw=gm_0g@mail.gmail.com","subject":"Re: [PATCH] [GSOC] prune_worktrees(): reimplement with dir_iterator","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-01T18:33:05Z","receivedAt":"2017-04-01T18:33:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Robert Stanca writes:\n\n> On Sat, Apr 1, 2017 at 5:29 AM, Junio C Hamano wrote:\n>>\n>> Using dir_iterator() to make it recursive is not just overkill but\n>> is a positively wrong change, isn't it?\n>\n> Thanks for the review, and yes the commit message is misleading (my\n> bad there). I understood that remove_temp_files has no recursion\n> component to it and it removes all \".tmp-id-pack-\" files from\n> /objects/pack , but shouldn't dir-iterator work even if there's no\n> recursion level?\n\nThe point is what should happen when somebody (perhaps a newer\nversion of Git, or a third-party add-on that works with Git) adds a\nsubdirectory in .git/objects/pack/ or .git/worktrees/. The answer\nis that files and directories in such a subdirectory must not be\ntouched, because prune_worktrees() or remove_temporary_files() do\nnot know what these files and directories are.\n\nThe dir-iterator API does not allow you to say \"only scan the\ndirectory without recursing into the subdirectories\" so your code\nends up recursing into them, without knowing what the files and\ndirectories inside are (or are not). As Peff mentioned in his\nreview on the other one, if we add a knob to dir-iterator API to\nallow you to tell it not to recurse, then it would make it possible\nto do a faithful conversion from the original, but without such a\nchange, you are changing the behaviour.\n"}]}