{"thread":{"id":"45578","subject":"[PATCH] [GSOC] remove_temporary_files(): reimplement using iterators","startedAt":"2017-04-01T00:25:17Z","lastAt":"2017-04-01T05:42:08Z","messageCount":3,"participants":["Robert Stanca","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"315973","messageId":"20170401002458.3494-1-robert.stanca7@gmail.com","threadId":"45578","inReplyTo":null,"subject":"[PATCH] [GSOC] remove_temporary_files(): reimplement using iterators","fromName":"Robert Stanca","fromEmail":"robert.stanca7@gmail.com","sentAt":"2017-04-01T00:24:58Z","receivedAt":"2017-04-01T00:25:17Z","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 <robert.stanca7@gmail.com>\n---\n builtin/repack.c | 16 ++++++----------\n 1 file changed, 6 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 677bc7c81..dba465814 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -7,6 +7,8 @@\n #include \"strbuf.h\"\n #include \"string-list.h\"\n #include \"argv-array.h\"\n+#include \"iterator.h\"\n+#include \"dir-iterator.h\"\n \n static int delta_base_offset = 1;\n static int pack_kept_objects = -1;\n@@ -49,12 +51,7 @@ static void remove_temporary_files(void)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tsize_t dirlen, prefixlen;\n-\tDIR *dir;\n-\tstruct dirent *e;\n-\n-\tdir = opendir(packdir);\n-\tif (!dir)\n-\t\treturn;\n+\tstruct dir_iterator *diter = dir_iterator_begin(packdir);\n \n \t/* Point at the slash at the end of \".../objects/pack/\" */\n \tdirlen = strlen(packdir) + 1;\n@@ -62,14 +59,13 @@ static void remove_temporary_files(void)\n \t/* Hold the length of  \".tmp-%d-pack-\" */\n \tprefixlen = buf.len - dirlen;\n \n-\twhile ((e = readdir(dir))) {\n-\t\tif (strncmp(e->d_name, buf.buf + dirlen, prefixlen))\n+\twhile (dir_iterator_advance(diter) == ITER_OK) {\n+\t\tif (strncmp(diter->relative_path, buf.buf + dirlen, prefixlen))\n \t\t\tcontinue;\n \t\tstrbuf_setlen(&buf, dirlen);\n-\t\tstrbuf_addstr(&buf, e->d_name);\n+\t\tstrbuf_addstr(&buf, diter->relative_path);\n \t\tunlink(buf.buf);\n \t}\n-\tclosedir(dir);\n \tstrbuf_release(&buf);\n }\n \n-- \n2.12.2.575.gb14f27f91.dirty\n\n"},{"id":"315989","messageId":"20170401053716.yzxqxokuz3awqki4@sigill.intra.peff.net","threadId":"45578","inReplyTo":"20170401002458.3494-1-robert.stanca7@gmail.com","subject":"Re: [PATCH] [GSOC] remove_temporary_files(): reimplement using iterators","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-01T05:37:16Z","receivedAt":"2017-04-01T05:37:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 01, 2017 at 03:24:58AM +0300, Robert Stanca wrote:\n\n> @@ -49,12 +51,7 @@ static void remove_temporary_files(void)\n>  {\n>  \tstruct strbuf buf = STRBUF_INIT;\n>  \tsize_t dirlen, prefixlen;\n> -\tDIR *dir;\n> -\tstruct dirent *e;\n> -\n> -\tdir = opendir(packdir);\n> -\tif (!dir)\n> -\t\treturn;\n> +\tstruct dir_iterator *diter = dir_iterator_begin(packdir);\n>  \n>  \t/* Point at the slash at the end of \".../objects/pack/\" */\n>  \tdirlen = strlen(packdir) + 1;\n> @@ -62,14 +59,13 @@ static void remove_temporary_files(void)\n>  \t/* Hold the length of  \".tmp-%d-pack-\" */\n>  \tprefixlen = buf.len - dirlen;\n>  \n> -\twhile ((e = readdir(dir))) {\n> -\t\tif (strncmp(e->d_name, buf.buf + dirlen, prefixlen))\n> +\twhile (dir_iterator_advance(diter) == ITER_OK) {\n> +\t\tif (strncmp(diter->relative_path, buf.buf + dirlen, prefixlen))\n>  \t\t\tcontinue;\n>  \t\tstrbuf_setlen(&buf, dirlen);\n> -\t\tstrbuf_addstr(&buf, e->d_name);\n> +\t\tstrbuf_addstr(&buf, diter->relative_path);\n>  \t\tunlink(buf.buf);\n>  \t}\n> -\tclosedir(dir);\n\nI think you could actually clean this code up more. The dir_iterator\nalready does this strbuf magic to hold the full path, so you should be\nable to just run \"unlink(iter->path.buf)\", get rid of the extra strbuf\nentirely.\n\nWe use that strbuf for the prefix-comparison, too, but the way it is\ndone is rather confusing. AFAICT, we could just be comparing against\n\"packtmp + strlen(packdir) + 1\". Though it would be simpler still to\nmake \"packtmp\" just the basename, rather than the full path.\n\nI do agree with the point Junio raised, though, that this loop isn't\nrecursive, but dir_iterator is. I think there was talk elsewhere of\ngiving it more options, so perhaps it could be taught a non-recursive\nversion. Until we have that, though, I'm not sure this is a good spot to\nconvert.\n\n-Peff\n"},{"id":"315990","messageId":"20170401054158.77ftxkba6kj2mdvp@sigill.intra.peff.net","threadId":"45578","inReplyTo":"20170401053716.yzxqxokuz3awqki4@sigill.intra.peff.net","subject":"Re: [PATCH] [GSOC] remove_temporary_files(): reimplement using iterators","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-01T05:41:59Z","receivedAt":"2017-04-01T05:42:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 01, 2017 at 01:37:16AM -0400, Jeff King wrote:\n\n> We use that strbuf for the prefix-comparison, too, but the way it is\n> done is rather confusing. AFAICT, we could just be comparing against\n> \"packtmp + strlen(packdir) + 1\". Though it would be simpler still to\n> make \"packtmp\" just the basename, rather than the full path.\n\nSomething like the patch below, which would then free you up to replace\n\"buf\" there with the diter->path.\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 677bc7c81..06a14d687 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -48,7 +48,7 @@ static int repack_config(const char *var, const char *value, void *cb)\n static void remove_temporary_files(void)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n-\tsize_t dirlen, prefixlen;\n+\tsize_t dirlen;\n \tDIR *dir;\n \tstruct dirent *e;\n \n@@ -56,14 +56,11 @@ static void remove_temporary_files(void)\n \tif (!dir)\n \t\treturn;\n \n-\t/* Point at the slash at the end of \".../objects/pack/\" */\n-\tdirlen = strlen(packdir) + 1;\n-\tstrbuf_addstr(&buf, packtmp);\n-\t/* Hold the length of  \".tmp-%d-pack-\" */\n-\tprefixlen = buf.len - dirlen;\n+\tstrbuf_addf(&buf, \"%s/\", packdir);\n+\tdirlen = buf.len;\n \n \twhile ((e = readdir(dir))) {\n-\t\tif (strncmp(e->d_name, buf.buf + dirlen, prefixlen))\n+\t\tif (!starts_with(e->d_name, packtmp))\n \t\t\tcontinue;\n \t\tstrbuf_setlen(&buf, dirlen);\n \t\tstrbuf_addstr(&buf, e->d_name);\n@@ -216,7 +213,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\tdie(_(incremental_bitmap_conflict_error));\n \n \tpackdir = mkpathdup(\"%s/pack\", get_object_directory());\n-\tpacktmp = mkpathdup(\"%s/.tmp-%d-pack\", packdir, (int)getpid());\n+\tpacktmp = mkpathdup(\".tmp-%d-pack\", (int)getpid());\n \n \tsigchain_push_common(remove_pack_on_signal);\n \n@@ -274,7 +271,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tif (delta_base_offset)\n \t\targv_array_push(&cmd.args,  \"--delta-base-offset\");\n \n-\targv_array_push(&cmd.args, packtmp);\n+\targv_array_pushf(&cmd.args, \"%s/%s\", packdir, packtmp);\n \n \tcmd.git_cmd = 1;\n \tcmd.out = -1;\n@@ -372,8 +369,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\tint exists = 0;\n \t\t\tfname = mkpathdup(\"%s/pack-%s%s\",\n \t\t\t\t\tpackdir, item->string, exts[ext].name);\n-\t\t\tfname_old = mkpathdup(\"%s-%s%s\",\n-\t\t\t\t\tpacktmp, item->string, exts[ext].name);\n+\t\t\tfname_old = mkpathdup(\"%s/%s-%s%s\",\n+\t\t\t\t\tpackdir, packtmp, item->string, exts[ext].name);\n \t\t\tif (!stat(fname_old, &statbuffer)) {\n \t\t\t\tstatbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);\n \t\t\t\tchmod(fname_old, statbuffer.st_mode);\n"}]}