threads / patch / 45487

patch[GSoC] remove_subtree(): reimplement using iterators

Subject: [PATCH] [GSoC] remove_subtree(): reimplement using iterators

## tl;dr

3 messages between Mar 24, 2017 and Mar 25, 2017. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Daniel Ferreira· Mar 24, 2017, 04:07 UTC · lore

Uses dir_iterator to traverse through remove_subtree()'s directory tree, avoiding the need for recursive calls to readdir() and simplifying code.

Suggested in the GSoC microproject list, as well as: https://public-inbox.org/git/xmqqk27m4h3h.fsf@gitster.mtv.corp.google.com/

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>
---

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 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.

Thank you.
 entry.c | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)
Show changes to entry.c +7 −18
diff --git a/entry.c b/entry.c
index c6eea24..a88c219 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,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);
 	}
-	closedir(dir);
+
 	if (rmdir(path->buf))
 		die_errno("cannot rmdir '%s'", path->buf);
 }
--
2.7.4 (Apple Git-66)
Stefan Beller· Mar 24, 2017, 17:02 UTC · re: Daniel Ferreira · lore

Re: [PATCH] [GSoC] remove_subtree(): reimplement using iterators

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.
Show 8 quoted lines
>
> 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. :)
Show 40 quoted lines
> --- 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?

Show 5 quoted lines
>         }
> -       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

Daniel Ferreira (theiostream)· Mar 25, 2017, 01:02 UTC · lore

Re: [PATCH] [GSoC] remove_subtree(): reimplement using iterators

> On Fri, Mar 24, 2017 at 2:02 PM, Stefan Beller <sbeller@google.com> wrote:
> Welcome to the Git community!
Thank you!
> Please use a more imperative style. (e.g. s/Uses/Use/ ...
> s/and simplfying/which simplifies/)
Thank you. Will do in a second version of this patch.
Show 5 quoted lines
> 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.
Perfect. I will remove it from the message.
Show 7 quoted lines
> 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?

Yes, we can! Thank you for the pointer. Will be in the next version of the patch.

> Also we'd need to handle (empty) directories differently for removal?
From what I've tested, we do not need to do it.
> Do we need to check the return code of dir_iterator_advance
> for ITER_ERROR as well?

I believe not – it only tries to perform an operation if we have ITER_OK. Since ITER_ERROR would end up in a no-op anyway I don't see how a check for it would be useful.

Show 11 quoted lines
>
>
> >         }
> > -       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.
I've tested it, and it does not yield in there.

Thank you for the advice, and as stated, will submit a v2 of the patch in short notice.

Thank you, Daniel.

← back to recent threads