On Tue, Dec 10, 2019 at 6:38 AM Matheus Tavares Bernardino
<matheus.bernardino@xxxxxx> wrote:
>
> On Mon, Dec 9, 2019 at 6:18 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
> >
> > Matheus Tavares Bernardino <matheus.bernardino@xxxxxx> 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?
>
> I think there is no current test that covers remove_subtree() being
> called with nested directories. But we could use the test proposed by
> Daniel[1], which does fail when this current patch is applied. So,
> maybe, Plato could also include this test in v2, before performing the
> dir-iterator convertion.
>
> [1]: https://public-inbox.org/git/1493226219-33423-3-git-send-email-bnmvco@xxxxxxxxx/
Hello!
Thanks for the warm welcome and sorry for the late reply.
Your review and hints you provided were very helpful and appreciated.
Yes, I can work and include the test in v2, as well as performing the
dir-iterator conversion. The required process and the necessary
changes I need to make were very clear.
This week I'm very busy. I'll work on this in great detail from next week.
Thanks,
Plato
