Welcome to the Git community! On Thu, Mar 23, 2017 at 9:07 PM, Daniel Ferreira <bnm...@gmail.com> wrote: > Uses dir_iterator to traverse through remove_subtree()'s directory tree, > avoiding the need for recursive calls to readdir() and simplifying code.
Please use a more imperative style. (e.g. s/Uses/Use/ ... s/and simplfying/which simplifies/) > > Suggested in the GSoC microproject list, as well as: > https://public-inbox.org/git/xmqqk27m4h3h....@gitster.mtv.corp.google.com/ Thanks for this link. It gives good context for reviewing the change, but it will not be good context to record as a commit message. (When someone looks at a commit message later on, they are usually trying to figure out what the author was thinking; if there were any special cases to be thought about. Was performance on the authors mind? etc) So I propose to put the link into the more informal section if a reroll is needed. I cc'd Duy, who came up with this Microproject. > A conversion similar in purpose was previously done at 46d092a > ("for_each_reflog(): reimplement using iterators", 2016-05-21). Thanks for pointing at another conversion. > > Signed-off-by: Daniel Ferreira <bnm...@gmail.com> > --- > > Hey there! This is my microproject for Google Summer of Code on git. > It has passed on Travis CI (https://travis-ci.org/theiostream/git), > although I would appreciate any suggestion to improve test coverage > for the affected function. This function is deep down in the worktree update mechanism, so any run of "git reset", "git checkout", git cherry-pick" (and all the others), which remove a directory (possibly recursive) covers the functionality. If I were to search for test coverage for this function in particular, I'd start by looking at "(cd t && ls t1*)". > This is, to my knowledge, one of the few microprojects that have not > yet been started by someone on this list, but please let me know if > someone else is already on it. cool. :) > --- a/entry.c > +++ b/entry.c > @@ -2,6 +2,8 @@ > #include "blob.h" > #include "dir.h" > #include "streaming.h" > +#include "iterator.h" > +#include "dir-iterator.h" > > static void create_directories(const char *path, int path_len, > const struct checkout *state) > @@ -46,29 +48,17 @@ static void create_directories(const char *path, int > path_len, > > static void remove_subtree(struct strbuf *path) > { > - DIR *dir = opendir(path->buf); > - struct dirent *de; > + struct dir_iterator *diter = dir_iterator_begin(path->buf); > int origlen = path->len; > > - if (!dir) > - die_errno("cannot opendir '%s'", path->buf); > - while ((de = readdir(dir)) != NULL) { > - struct stat st; > - > - if (is_dot_or_dotdot(de->d_name)) > - continue; > - > + while (dir_iterator_advance(diter) == ITER_OK) { > strbuf_addch(path, '/'); > - strbuf_addstr(path, de->d_name); > - if (lstat(path->buf, &st)) > - die_errno("cannot lstat '%s'", path->buf); > - if (S_ISDIR(st.st_mode)) > - remove_subtree(path); > - else if (unlink(path->buf)) > + strbuf_addstr(path, diter->relative_path); > + if (unlink(path->buf)) > die_errno("cannot unlink '%s'", path->buf); > strbuf_setlen(path, origlen); Instead of constructing the path again here based on relative path and the path parameter, I wonder if we could use if (unlink(diter->path)) .. here? Then we would not need the strbuf at all? Also we'd need to handle (empty) directories differently for removal? Do we need to check the return code of dir_iterator_advance for ITER_ERROR as well? > } > - closedir(dir); > + > if (rmdir(path->buf)) > die_errno("cannot rmdir '%s'", path->buf); This would remove the "top level" directory as given by path. When reading the dir-iterator code, I am not sure if this is also part of the yield in dir_iterator_advance. Thanks for working on this micro project! Stefan
