* [PATCH] [GSOC] remove_temporary_files(): reimplement using iterators @ 2017-04-01 0:24 Robert Stanca 2017-04-01 5:37 ` Jeff King 0 siblings, 1 reply; 3+ messages in thread From: Robert Stanca @ 2017-04-01 0:24 UTC (permalink / raw) To: git; +Cc: Robert Stanca Replaces recursive traversing of opendir with dir_iterator Signed-off-by: Robert Stanca <robert.stanca7@gmail.com> --- builtin/repack.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/builtin/repack.c b/builtin/repack.c index 677bc7c81..dba465814 100644 --- a/builtin/repack.c +++ b/builtin/repack.c @@ -7,6 +7,8 @@ #include "strbuf.h" #include "string-list.h" #include "argv-array.h" +#include "iterator.h" +#include "dir-iterator.h" static int delta_base_offset = 1; static int pack_kept_objects = -1; @@ -49,12 +51,7 @@ static void remove_temporary_files(void) { struct strbuf buf = STRBUF_INIT; size_t dirlen, prefixlen; - DIR *dir; - struct dirent *e; - - dir = opendir(packdir); - if (!dir) - return; + struct dir_iterator *diter = dir_iterator_begin(packdir); /* Point at the slash at the end of ".../objects/pack/" */ dirlen = strlen(packdir) + 1; @@ -62,14 +59,13 @@ static void remove_temporary_files(void) /* Hold the length of ".tmp-%d-pack-" */ prefixlen = buf.len - dirlen; - while ((e = readdir(dir))) { - if (strncmp(e->d_name, buf.buf + dirlen, prefixlen)) + while (dir_iterator_advance(diter) == ITER_OK) { + if (strncmp(diter->relative_path, buf.buf + dirlen, prefixlen)) continue; strbuf_setlen(&buf, dirlen); - strbuf_addstr(&buf, e->d_name); + strbuf_addstr(&buf, diter->relative_path); unlink(buf.buf); } - closedir(dir); strbuf_release(&buf); } -- 2.12.2.575.gb14f27f91.dirty ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] [GSOC] remove_temporary_files(): reimplement using iterators 2017-04-01 0:24 [PATCH] [GSOC] remove_temporary_files(): reimplement using iterators Robert Stanca @ 2017-04-01 5:37 ` Jeff King 2017-04-01 5:41 ` Jeff King 0 siblings, 1 reply; 3+ messages in thread From: Jeff King @ 2017-04-01 5:37 UTC (permalink / raw) To: Robert Stanca; +Cc: git On Sat, Apr 01, 2017 at 03:24:58AM +0300, Robert Stanca wrote: > @@ -49,12 +51,7 @@ static void remove_temporary_files(void) > { > struct strbuf buf = STRBUF_INIT; > size_t dirlen, prefixlen; > - DIR *dir; > - struct dirent *e; > - > - dir = opendir(packdir); > - if (!dir) > - return; > + struct dir_iterator *diter = dir_iterator_begin(packdir); > > /* Point at the slash at the end of ".../objects/pack/" */ > dirlen = strlen(packdir) + 1; > @@ -62,14 +59,13 @@ static void remove_temporary_files(void) > /* Hold the length of ".tmp-%d-pack-" */ > prefixlen = buf.len - dirlen; > > - while ((e = readdir(dir))) { > - if (strncmp(e->d_name, buf.buf + dirlen, prefixlen)) > + while (dir_iterator_advance(diter) == ITER_OK) { > + if (strncmp(diter->relative_path, buf.buf + dirlen, prefixlen)) > continue; > strbuf_setlen(&buf, dirlen); > - strbuf_addstr(&buf, e->d_name); > + strbuf_addstr(&buf, diter->relative_path); > unlink(buf.buf); > } > - closedir(dir); I think you could actually clean this code up more. The dir_iterator already does this strbuf magic to hold the full path, so you should be able to just run "unlink(iter->path.buf)", get rid of the extra strbuf entirely. We use that strbuf for the prefix-comparison, too, but the way it is done is rather confusing. AFAICT, we could just be comparing against "packtmp + strlen(packdir) + 1". Though it would be simpler still to make "packtmp" just the basename, rather than the full path. I do agree with the point Junio raised, though, that this loop isn't recursive, but dir_iterator is. I think there was talk elsewhere of giving it more options, so perhaps it could be taught a non-recursive version. Until we have that, though, I'm not sure this is a good spot to convert. -Peff ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] [GSOC] remove_temporary_files(): reimplement using iterators 2017-04-01 5:37 ` Jeff King @ 2017-04-01 5:41 ` Jeff King 0 siblings, 0 replies; 3+ messages in thread From: Jeff King @ 2017-04-01 5:41 UTC (permalink / raw) To: Robert Stanca; +Cc: git On Sat, Apr 01, 2017 at 01:37:16AM -0400, Jeff King wrote: > We use that strbuf for the prefix-comparison, too, but the way it is > done is rather confusing. AFAICT, we could just be comparing against > "packtmp + strlen(packdir) + 1". Though it would be simpler still to > make "packtmp" just the basename, rather than the full path. Something like the patch below, which would then free you up to replace "buf" there with the diter->path. diff --git a/builtin/repack.c b/builtin/repack.c index 677bc7c81..06a14d687 100644 --- a/builtin/repack.c +++ b/builtin/repack.c @@ -48,7 +48,7 @@ static int repack_config(const char *var, const char *value, void *cb) static void remove_temporary_files(void) { struct strbuf buf = STRBUF_INIT; - size_t dirlen, prefixlen; + size_t dirlen; DIR *dir; struct dirent *e; @@ -56,14 +56,11 @@ static void remove_temporary_files(void) if (!dir) return; - /* Point at the slash at the end of ".../objects/pack/" */ - dirlen = strlen(packdir) + 1; - strbuf_addstr(&buf, packtmp); - /* Hold the length of ".tmp-%d-pack-" */ - prefixlen = buf.len - dirlen; + strbuf_addf(&buf, "%s/", packdir); + dirlen = buf.len; while ((e = readdir(dir))) { - if (strncmp(e->d_name, buf.buf + dirlen, prefixlen)) + if (!starts_with(e->d_name, packtmp)) continue; strbuf_setlen(&buf, dirlen); strbuf_addstr(&buf, e->d_name); @@ -216,7 +213,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix) die(_(incremental_bitmap_conflict_error)); packdir = mkpathdup("%s/pack", get_object_directory()); - packtmp = mkpathdup("%s/.tmp-%d-pack", packdir, (int)getpid()); + packtmp = mkpathdup(".tmp-%d-pack", (int)getpid()); sigchain_push_common(remove_pack_on_signal); @@ -274,7 +271,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix) if (delta_base_offset) argv_array_push(&cmd.args, "--delta-base-offset"); - argv_array_push(&cmd.args, packtmp); + argv_array_pushf(&cmd.args, "%s/%s", packdir, packtmp); cmd.git_cmd = 1; cmd.out = -1; @@ -372,8 +369,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix) int exists = 0; fname = mkpathdup("%s/pack-%s%s", packdir, item->string, exts[ext].name); - fname_old = mkpathdup("%s-%s%s", - packtmp, item->string, exts[ext].name); + fname_old = mkpathdup("%s/%s-%s%s", + packdir, packtmp, item->string, exts[ext].name); if (!stat(fname_old, &statbuffer)) { statbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH); chmod(fname_old, statbuffer.st_mode); ^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-04-01 5:42 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-01 0:24 [PATCH] [GSOC] remove_temporary_files(): reimplement using iterators Robert Stanca 2017-04-01 5:37 ` Jeff King 2017-04-01 5:41 ` Jeff King
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.