From: "Daniel Ferreira (theiostream)" <bnmvco@gmail.com>
To: Duy Nguyen <pclouds@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>,
Junio C Hamano <gitster@pobox.com>,
Stefan Beller <sbeller@google.com>
Subject: Re: [PATCH v2] [GSoC] remove_subtree(): reimplement using iterators
Date: Sat, 25 Mar 2017 09:13:35 -0300 [thread overview]
Message-ID: <CAEA2_RK=zMwKeidPDS9u5Ep1htb3TUeOqJ6Cfd644ira4DeXaA@mail.gmail.com> (raw)
In-Reply-To: <CACsJy8Aajf1Lb4mKC=NEUQTNy7QRK99Aont_UCRnHJfho8n+UA@mail.gmail.com>
You are correct, which shows that since all tests pass, we need to
come up with better cases for this function.
As for a solution, I believe that the best way to go for it is to
dir_iterator's implementation to have an "Option to iterate over
directory paths before vs. after their contents" (something predicted
in the commit that created it). If it iterates over directories after
all of its contents (currently it does so before) we just need to
check if the entry is a directory and if so, rmdir() it. Does that
make sense?
On Sat, Mar 25, 2017 at 8:51 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Sat, Mar 25, 2017 at 6:27 PM, Daniel Ferreira <bnmvco@gmail.com> wrote:
>> Use dir_iterator to traverse through remove_subtree()'s directory tree,
>> avoiding the need for recursive calls to readdir(). Simplify
>> remove_subtree()'s code.
>>
>> A conversion similar in purpose was previously done at 46d092a
>> ("for_each_reflog(): reimplement using iterators", 2016-05-21).
>>
>> Signed-off-by: Daniel Ferreira <bnmvco@gmail.com>
>> ---
>>
>> This is a second-version patch of the Google Summer of Code microproject for
>> refactoring recursive readdir() calls to use dir_iterator instead. v1 can be
>> found in:
>>
>> https://public-inbox.org/git/CAGZ79kZwT-9mHTiOJ5CEjk2wDFkn6+NcogjX0=vjhsAh16ANYg@mail.gmail.com/T/#mae023e7a7d7626f00e0923833c4359f5af493730
>>
>> Additionally, for debugging purposes I turned remove_subtree() into a no-op
>> and ran git tests. Some failures were at:
>>
>> * t2000-checkout-cache-clash.sh
>> * t2003-checkout-cache-mkdir.sh
>>
>> If you guys could check those files out and warn me if any additional tests
>> would be welcome, please let me know.
>>
>> Thanks.
>>
>> entry.c | 28 +++++++---------------------
>> 1 file changed, 7 insertions(+), 21 deletions(-)
>>
>> diff --git a/entry.c b/entry.c
>> index c6eea240b..3cb92592d 100644
>> --- 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,13 @@ 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;
>> - 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;
>> -
>> - 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))
>> + struct dir_iterator *diter = dir_iterator_begin(path->buf);
>> +
>> + while (dir_iterator_advance(diter) == ITER_OK) {
>> + if (unlink(diter->path.buf))
>> die_errno("cannot unlink '%s'", path->buf);
>> - strbuf_setlen(path, origlen);
>> }
>> - closedir(dir);
>> +
>> if (rmdir(path->buf))
>> die_errno("cannot rmdir '%s'", path->buf);
>
> Even though it's very nice that lots of code is deleted. This is not
> entirely correct, is it? Before this patch, rmdir() is called for
> every recursive remove_subtree() call. After this patch, it's only
> called once (and likely fails unless you have no subdirectories).
>
>> }
>> --
>> 2.12.1.433.g82305b74f.dirty
>>
>
>
>
> --
> Duy
next prev parent reply other threads:[~2017-03-25 12:14 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-03-25 11:27 [PATCH v2] [GSoC] remove_subtree(): reimplement using iterators Daniel Ferreira
2017-03-25 11:51 ` Duy Nguyen
2017-03-25 12:13 ` Daniel Ferreira (theiostream) [this message]
2017-03-25 12:29 ` Duy Nguyen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAEA2_RK=zMwKeidPDS9u5Ep1htb3TUeOqJ6Cfd644ira4DeXaA@mail.gmail.com' \
--to=bnmvco@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=pclouds@gmail.com \
--cc=sbeller@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).