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

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

[Daniel Ferreira]

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

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)

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

[Stefan Beller]

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

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

[Daniel Ferreira (theiostream)]

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

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

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

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