From: Stefan Beller <sbeller@google.com>
To: Daniel Ferreira <bnmvco@gmail.com>,
Junio C Hamano <gitster@pobox.com>,
Duy Nguyen <pclouds@gmail.com>
Cc: "git@vger.kernel.org" <git@vger.kernel.org>
Subject: Re: [PATCH] [GSoC] remove_subtree(): reimplement using iterators
Date: Fri, 24 Mar 2017 10:02:57 -0700 [thread overview]
Message-ID: <CAGZ79kZwT-9mHTiOJ5CEjk2wDFkn6+NcogjX0=vjhsAh16ANYg@mail.gmail.com> (raw)
In-Reply-To: <1490328420-75901-1-git-send-email-bnmvco@gmail.com>
Welcome to the Git community!
On Thu, Mar 23, 2017 at 9:07 PM, Daniel Ferreira <bnmvco@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.fsf@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 <bnmvco@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
next prev parent reply other threads:[~2017-03-24 17:03 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-03-24 4:07 [PATCH] [GSoC] remove_subtree(): reimplement using iterators Daniel Ferreira
2017-03-24 17:02 ` Stefan Beller [this message]
[not found] ` <CAEA2_RLZztaRwcppwS45XfXO1n_VKw5547uScOhQON=ktttW8g@mail.gmail.com>
2017-03-25 1:02 ` Daniel Ferreira (theiostream)
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='CAGZ79kZwT-9mHTiOJ5CEjk2wDFkn6+NcogjX0=vjhsAh16ANYg@mail.gmail.com' \
--to=sbeller@google.com \
--cc=bnmvco@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=pclouds@gmail.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).