Re: [PATCH] entry.c: use dir-iterator to avoid explicit dir traversal

>
> Matheus Tavares Bernardino <matheus.bernardino@usp.br> writes:
>
> >> The changes pass the test suite t/ and Travis CI.
> >> Please point out any mistakes.
> >> ...
> >> -               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))
> >> -                       die_errno("cannot unlink '%s'", path->buf);
> >> -               strbuf_setlen(path, origlen);
> >> +               if (unlink(iter->path.buf)) {
> >
> > unlink()-ing a directory in Linux will return a EISDIR error. So I
> > think you still need to use S_ISDIR() to check if iter->path.buf is a
> > directory and call rmdir(), in this case.
> >
> > However, note that the dir-iterator API gives entries in pre-order.
> > I.e. a directory appears before its subentries. In the use case of
> > remove_subtree(), though, we need to traverse in post-order, since we
> > have to remove the subentries before removing the directory where they
> > reside. My suggestion is that you add a preliminary patch,
> > implementing a new DIR_ITERATOR_POST_ORDER flag to dir-iterator.h, and
> > then use it in this patch.
>
> Thanks for a review and a few hints to nudge a new contributor in
> the right direction.  Very much appreciated.
>
> I wonder why the bugs in this patch weren't caught by self test we
> already have, by the way.  We need a bit better test coverage,
> perhaps?