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

[Daniel Ferreira (theiostream)]

> On Fri, Mar 24, 2017 at 2:02 PM, Stefan Beller  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.

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  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@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 
> ---
>
> 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, ))
> -   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