Re: [PATCH] entry.c: use dir-iterator to avoid explicit dir traversal
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Dec 9, 2019, 21:18 UTC
- Message-ID
- <xmqqv9qptcyz.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <CAHd-oW4MzH9Wrb2zNPF0JBN8+FRB4NtDX0166Qt2Kbqs0mDgcw@mail.gmail.com>
Matheus Tavares Bernardino <matheus.bernardino@usp.br> writes:
Show 25 quoted lines
>> 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?