[GSoC][PATCH 0/2] clone: convert explicit traversal to

[GSoC][PATCH 0/2] clone: convert explicit traversal to

[Matheus Tavares]

Convert explicit recursive dir traversal at copy_or_link_directory of
clone.c to use dir-iterator API.

This is my microproject for GSoC 2019. Idea taken from
https://git.github.io/SoC-2019-Microprojects/#use-dir-iterator-to-avoid-explicit-recursive-directory-traversal

Matheus Tavares (2):
  clone: extract function from copy_or_link_directory
  clone: use dir-iterator to avoid explicit dir traversal

 builtin/clone.c | 64 ++++++++++++++++++++++++++++---------------------
 1 file changed, 37 insertions(+), 27 deletions(-)

-- 
2.20.1

[GSoC][PATCH 1/2] clone: extract function from copy_or_link_directory

[Matheus Tavares]

Extract dir creation code snippet from copy_or_link_directory to its own
function named mkdir_if_missing. This change will help removing
copy_or_link_directory's explicit recursion, which will be done in patch
"clone: use dir-iterator to avoid explicit dir traversal". Also makes
code more readable.

Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
---
 builtin/clone.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 50bde99618..2a1cc4dab9 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -392,6 +392,24 @@ static void copy_alternates(struct strbuf *src, struct strbuf *dst,
 	fclose(in);
 }
 
+static void mkdir_if_missing(const char *pathname, mode_t mode)
+{
+	/*
+	 * Create a dir at pathname unless there's already one.
+	 */
+	struct stat buf;
+
+	if (mkdir(pathname, mode)) {
+		if (errno != EEXIST)
+			die_errno(_("failed to create directory '%s'"),
+				  pathname);
+		else if (stat(pathname, &buf))
+			die_errno(_("failed to stat '%s'"), pathname);
+		else if (!S_ISDIR(buf.st_mode))
+			die(_("%s exists and is not a directory"), pathname);
+	}
+}
+
 static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,
 				   const char *src_repo, int src_baselen)
 {
@@ -404,14 +422,7 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,
 	if (!dir)
 		die_errno(_("failed to open '%s'"), src->buf);
 
-	if (mkdir(dest->buf, 0777)) {
-		if (errno != EEXIST)
-			die_errno(_("failed to create directory '%s'"), dest->buf);
-		else if (stat(dest->buf, &buf))
-			die_errno(_("failed to stat '%s'"), dest->buf);
-		else if (!S_ISDIR(buf.st_mode))
-			die(_("%s exists and is not a directory"), dest->buf);
-	}
+	mkdir_if_missing(dest->buf, 0777);
 
 	strbuf_addch(src, '/');
 	src_len = src->len;
-- 
2.20.1

Re: [GSoC][PATCH 1/2] clone: extract function from copy_or_link_directory

[Christian Couder]

On Fri, Feb 15, 2019 at 5:39 PM Matheus Tavares
<matheus.bernardino@usp.br> wrote:
>
> Extract dir creation code snippet from copy_or_link_directory to its own
> function named mkdir_if_missing. This change will help removing
> copy_or_link_directory's explicit recursion, which will be done in patch
> "clone: use dir-iterator to avoid explicit dir traversal".

"which will be done in a following patch" is enough and perhaps even
better as the following patch can then be changed independently of
this one.

> Also makes
> code more readable.
>
> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>

[...]

> +static void mkdir_if_missing(const char *pathname, mode_t mode)
> +{
> +       /*
> +        * Create a dir at pathname unless there's already one.
> +        */
> +       struct stat buf;

I know that the variable was already called "buf" in
copy_or_link_directory() and that there are a few other places in the
code where a 'struct stat' is called "buf", but in most places the
'struct stat' variables are called "st":

$ git grep 'struct stat ' '*.c' | perl -ne 'print "$1\n" if (m/struct
stat ([\w_]+)/);' | sort | uniq -c | sort -nr
    129 st
      6 sb
      3 buf
      2 statbuf
      1 st_stdin
      1 st_git
      1 statbuffer
      1 st2
      1 st1
      1 nst
      1 loginfo
      1 cwd_stat
      1 argstat

So I wonder if we should use "st" here instead of "buf". We also often
use "buf" for 'struct strbuf' variables which can be confusing.

Otherwise the patch looks good to me.

Re: [GSoC][PATCH 1/2] clone: extract function from copy_or_link_directory

[Matheus Tavares Bernardino]

On Sat, Feb 16, 2019 at 4:23 AM Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Fri, Feb 15, 2019 at 5:39 PM Matheus Tavares
> <matheus.bernardino@usp.br> wrote:
> >
> > Extract dir creation code snippet from copy_or_link_directory to its own
> > function named mkdir_if_missing. This change will help removing
> > copy_or_link_directory's explicit recursion, which will be done in patch
> > "clone: use dir-iterator to avoid explicit dir traversal".
>
> "which will be done in a following patch" is enough and perhaps even
> better as the following patch can then be changed independently of
> this one.
>

Ok! I will fix that in v2.

> > Also makes
> > code more readable.
> >
> > Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
>
> [...]
>
> > +static void mkdir_if_missing(const char *pathname, mode_t mode)
> > +{
> > +       /*
> > +        * Create a dir at pathname unless there's already one.
> > +        */
> > +       struct stat buf;
>
> I know that the variable was already called "buf" in
> copy_or_link_directory() and that there are a few other places in the
> code where a 'struct stat' is called "buf", but in most places the
> 'struct stat' variables are called "st":
>
> $ git grep 'struct stat ' '*.c' | perl -ne 'print "$1\n" if (m/struct
> stat ([\w_]+)/);' | sort | uniq -c | sort -nr
>     129 st
>       6 sb
>       3 buf
>       2 statbuf
>       1 st_stdin
>       1 st_git
>       1 statbuffer
>       1 st2
>       1 st1
>       1 nst
>       1 loginfo
>       1 cwd_stat
>       1 argstat
>
> So I wonder if we should use "st" here instead of "buf". We also often
> use "buf" for 'struct strbuf' variables which can be confusing.
>

Right! I'm changing that in v2 too.

Thanks,
Matheus

> Otherwise the patch looks good to me.

[GSoC][PATCH 2/2] clone: use dir-iterator to avoid explicit dir traversal

[Matheus Tavares]

Replace usage of opendir/readdir/closedir API to traverse directories
recursively, at copy_or_link_directory function, by the dir-iterator
API.

Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
---
 builtin/clone.c | 39 +++++++++++++++++++--------------------
 1 file changed, 19 insertions(+), 20 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 2a1cc4dab9..66ae347f79 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -23,6 +23,8 @@
 #include "transport.h"
 #include "strbuf.h"
 #include "dir.h"
+#include "dir-iterator.h"
+#include "iterator.h"
 #include "sigchain.h"
 #include "branch.h"
 #include "remote.h"
@@ -413,40 +415,33 @@ static void mkdir_if_missing(const char *pathname, mode_t mode)
 static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,
 				   const char *src_repo, int src_baselen)
 {
-	struct dirent *de;
-	struct stat buf;
 	int src_len, dest_len;
-	DIR *dir;
-
-	dir = opendir(src->buf);
-	if (!dir)
-		die_errno(_("failed to open '%s'"), src->buf);
+	struct dir_iterator *iter;
+	int iter_status;
 
 	mkdir_if_missing(dest->buf, 0777);
 
+	iter = dir_iterator_begin(src->buf);
+
 	strbuf_addch(src, '/');
 	src_len = src->len;
 	strbuf_addch(dest, '/');
 	dest_len = dest->len;
 
-	while ((de = readdir(dir)) != NULL) {
+	while ((iter_status = dir_iterator_advance(iter)) == ITER_OK) {
 		strbuf_setlen(src, src_len);
-		strbuf_addstr(src, de->d_name);
+		strbuf_addstr(src, iter->relative_path);
 		strbuf_setlen(dest, dest_len);
-		strbuf_addstr(dest, de->d_name);
-		if (stat(src->buf, &buf)) {
-			warning (_("failed to stat %s\n"), src->buf);
-			continue;
-		}
-		if (S_ISDIR(buf.st_mode)) {
-			if (de->d_name[0] != '.')
-				copy_or_link_directory(src, dest,
-						       src_repo, src_baselen);
+		strbuf_addstr(dest, iter->relative_path);
+
+		if (S_ISDIR(iter->st.st_mode)) {
+			if (iter->basename[0] != '.')
+				mkdir_if_missing(dest->buf, 0777);
 			continue;
 		}
 
 		/* Files that cannot be copied bit-for-bit... */
-		if (!strcmp(src->buf + src_baselen, "/info/alternates")) {
+		if (!strcmp(iter->relative_path, "info/alternates")) {
 			copy_alternates(src, dest, src_repo);
 			continue;
 		}
@@ -463,7 +458,11 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,
 		if (copy_file_with_time(dest->buf, src->buf, 0666))
 			die_errno(_("failed to copy file to '%s'"), dest->buf);
 	}
-	closedir(dir);
+
+	if (iter_status != ITER_DONE) {
+		strbuf_setlen(src, src_len);
+		die(_("failed to iterate over '%s'"), src->buf);
+	}
 }
 
 static void clone_local(const char *src_repo, const char *dest_repo)
-- 
2.20.1

Re: [GSoC][PATCH 2/2] clone: use dir-iterator to avoid explicit dir traversal

[Christian Couder]

On Fri, Feb 15, 2019 at 5:39 PM Matheus Tavares
<matheus.bernardino@usp.br> wrote:
>
> Replace usage of opendir/readdir/closedir API to traverse directories
> recursively, at copy_or_link_directory function, by the dir-iterator
> API.

You may want to add that this simplifies the code and avoid recursive
calls of copy_or_link_directory().

Otherwise the patch looks good to me. Thanks.

Re: [GSoC][PATCH 2/2] clone: use dir-iterator to avoid explicit dir traversal

[Thomas Gummerer]

On 02/15, Matheus Tavares wrote:
> Replace usage of opendir/readdir/closedir API to traverse directories
> recursively, at copy_or_link_directory function, by the dir-iterator
> API.
> 
> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
> ---
>  builtin/clone.c | 39 +++++++++++++++++++--------------------
>  1 file changed, 19 insertions(+), 20 deletions(-)
> 
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 2a1cc4dab9..66ae347f79 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -23,6 +23,8 @@
>  #include "transport.h"
>  #include "strbuf.h"
>  #include "dir.h"
> +#include "dir-iterator.h"
> +#include "iterator.h"
>  #include "sigchain.h"
>  #include "branch.h"
>  #include "remote.h"
> @@ -413,40 +415,33 @@ static void mkdir_if_missing(const char *pathname, mode_t mode)
>  static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,
>  				   const char *src_repo, int src_baselen)
>  {

The src_baselen parameter is now unused, and should be removed.  Our
codebase currently doesn't compile with -Wunused-parameter, so this is
not something the compiler can catch at the moment unfortunately.
However there is some work going on towards removing unused parameter
from the codebase, so it would be nice to not make things worse here.

*1*: https://public-inbox.org/git/20190214054736.GA20091@sigill.intra.peff.net

> -	struct dirent *de;
> -	struct stat buf;
>  	int src_len, dest_len;
> -	DIR *dir;
> -
> -	dir = opendir(src->buf);
> -	if (!dir)
> -		die_errno(_("failed to open '%s'"), src->buf);
> +	struct dir_iterator *iter;
> +	int iter_status;
>  
>  	mkdir_if_missing(dest->buf, 0777);
>  
> +	iter = dir_iterator_begin(src->buf);
> +
>  	strbuf_addch(src, '/');
>  	src_len = src->len;
>  	strbuf_addch(dest, '/');
>  	dest_len = dest->len;
>  
> -	while ((de = readdir(dir)) != NULL) {
> +	while ((iter_status = dir_iterator_advance(iter)) == ITER_OK) {
>  		strbuf_setlen(src, src_len);
> -		strbuf_addstr(src, de->d_name);
> +		strbuf_addstr(src, iter->relative_path);
>  		strbuf_setlen(dest, dest_len);
> -		strbuf_addstr(dest, de->d_name);
> -		if (stat(src->buf, &buf)) {
> -			warning (_("failed to stat %s\n"), src->buf);
> -			continue;
> -		}

Why was this warning removed?  I don't see a corresponding warning in
the iterator API.  The one thing the iterator API is doing is that it
does an lstat on the path to check if it exists.  However that does
not follow symlinks, and ignores possible other errors such as
permission errors.

If there is a good reason to remove the warning, that would be useful
to describe in the commit message.

> -		if (S_ISDIR(buf.st_mode)) {
> -			if (de->d_name[0] != '.')
> -				copy_or_link_directory(src, dest,
> -						       src_repo, src_baselen);
> +		strbuf_addstr(dest, iter->relative_path);
> +
> +		if (S_ISDIR(iter->st.st_mode)) {
> +			if (iter->basename[0] != '.')
> +				mkdir_if_missing(dest->buf, 0777);
>  			continue;
>  		}
>  
>  		/* Files that cannot be copied bit-for-bit... */
> -		if (!strcmp(src->buf + src_baselen, "/info/alternates")) {
> +		if (!strcmp(iter->relative_path, "info/alternates")) {
>  			copy_alternates(src, dest, src_repo);
>  			continue;
>  		}
> @@ -463,7 +458,11 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,
>  		if (copy_file_with_time(dest->buf, src->buf, 0666))
>  			die_errno(_("failed to copy file to '%s'"), dest->buf);
>  	}
> -	closedir(dir);
> +
> +	if (iter_status != ITER_DONE) {
> +		strbuf_setlen(src, src_len);
> +		die(_("failed to iterate over '%s'"), src->buf);
> +	}

Interestingly enough, this is not something that can currently
happen if I read the dir-iterator code correctly.  Even though the
dir_iterator_advance function says it can return ITER_ERROR, it never
actually does.  The only way the iter_status can be not ITER_DONE at
this point is if we would 'break' out of the loop.

I don't think it hurts to be defensive here in case someone decides to
break out of the loop in the future, just something odd I noticed
while reviewing the code.

>  }
>  
>  static void clone_local(const char *src_repo, const char *dest_repo)
> -- 
> 2.20.1
> 

Re: [GSoC][PATCH 2/2] clone: use dir-iterator to avoid explicit dir traversal

[Matheus Tavares Bernardino]

On Sat, Feb 16, 2019 at 12:38 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
>
> On 02/15, Matheus Tavares wrote:
> > Replace usage of opendir/readdir/closedir API to traverse directories
> > recursively, at copy_or_link_directory function, by the dir-iterator
> > API.
> >
> > Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
> > ---
> >  builtin/clone.c | 39 +++++++++++++++++++--------------------
> >  1 file changed, 19 insertions(+), 20 deletions(-)
> >
> > diff --git a/builtin/clone.c b/builtin/clone.c
> > index 2a1cc4dab9..66ae347f79 100644
> > --- a/builtin/clone.c
> > +++ b/builtin/clone.c
> > @@ -23,6 +23,8 @@
> >  #include "transport.h"
> >  #include "strbuf.h"
> >  #include "dir.h"
> > +#include "dir-iterator.h"
> > +#include "iterator.h"
> >  #include "sigchain.h"
> >  #include "branch.h"
> >  #include "remote.h"
> > @@ -413,40 +415,33 @@ static void mkdir_if_missing(const char *pathname, mode_t mode)
> >  static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,
> >                                  const char *src_repo, int src_baselen)
> >  {
>

Hi, Thomas. Thanks for the review.

> The src_baselen parameter is now unused, and should be removed.  Our
> codebase currently doesn't compile with -Wunused-parameter, so this is
> not something the compiler can catch at the moment unfortunately.
> However there is some work going on towards removing unused parameter
> from the codebase, so it would be nice to not make things worse here.
>

Nice! I will fix that in v2.

> *1*: https://public-inbox.org/git/20190214054736.GA20091@sigill.intra.peff.net
>
> > -     struct dirent *de;
> > -     struct stat buf;
> >       int src_len, dest_len;
> > -     DIR *dir;
> > -
> > -     dir = opendir(src->buf);
> > -     if (!dir)
> > -             die_errno(_("failed to open '%s'"), src->buf);
> > +     struct dir_iterator *iter;
> > +     int iter_status;
> >
> >       mkdir_if_missing(dest->buf, 0777);
> >
> > +     iter = dir_iterator_begin(src->buf);
> > +
> >       strbuf_addch(src, '/');
> >       src_len = src->len;
> >       strbuf_addch(dest, '/');
> >       dest_len = dest->len;
> >
> > -     while ((de = readdir(dir)) != NULL) {
> > +     while ((iter_status = dir_iterator_advance(iter)) == ITER_OK) {
> >               strbuf_setlen(src, src_len);
> > -             strbuf_addstr(src, de->d_name);
> > +             strbuf_addstr(src, iter->relative_path);
> >               strbuf_setlen(dest, dest_len);
> > -             strbuf_addstr(dest, de->d_name);
> > -             if (stat(src->buf, &buf)) {
> > -                     warning (_("failed to stat %s\n"), src->buf);
> > -                     continue;
> > -             }
>
> Why was this warning removed?  I don't see a corresponding warning in
> the iterator API.  The one thing the iterator API is doing is that it
> does an lstat on the path to check if it exists.  However that does
> not follow symlinks, and ignores possible other errors such as
> permission errors.
>

You are right. I didn't know the differences from lstat and stat. And
reflecting on this now, I realize that the problem is even deeper:
copy_or_link_directory follows symlinks but dir-iterator don't, so I
cannot use dir-iterator without falling back to recursion on
copy_or_link_directory. Because of that, I thought off adding an
option for dir-iterator to follow symlinks. Does this seem like a good
idea?

Also, I just noticed that dir-iterator follows hidden paths while
copy_or_link_directory don't. Maybe another option to add for
dir-iterator?

> If there is a good reason to remove the warning, that would be useful
> to describe in the commit message.
>
> > -             if (S_ISDIR(buf.st_mode)) {
> > -                     if (de->d_name[0] != '.')
> > -                             copy_or_link_directory(src, dest,
> > -                                                    src_repo, src_baselen);
> > +             strbuf_addstr(dest, iter->relative_path);
> > +
> > +             if (S_ISDIR(iter->st.st_mode)) {
> > +                     if (iter->basename[0] != '.')
> > +                             mkdir_if_missing(dest->buf, 0777);
> >                       continue;
> >               }
> >
> >               /* Files that cannot be copied bit-for-bit... */
> > -             if (!strcmp(src->buf + src_baselen, "/info/alternates")) {
> > +             if (!strcmp(iter->relative_path, "info/alternates")) {
> >                       copy_alternates(src, dest, src_repo);
> >                       continue;
> >               }
> > @@ -463,7 +458,11 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,
> >               if (copy_file_with_time(dest->buf, src->buf, 0666))
> >                       die_errno(_("failed to copy file to '%s'"), dest->buf);
> >       }
> > -     closedir(dir);
> > +
> > +     if (iter_status != ITER_DONE) {
> > +             strbuf_setlen(src, src_len);
> > +             die(_("failed to iterate over '%s'"), src->buf);
> > +     }
>
> Interestingly enough, this is not something that can currently
> happen if I read the dir-iterator code correctly.  Even though the
> dir_iterator_advance function says it can return ITER_ERROR, it never
> actually does.  The only way the iter_status can be not ITER_DONE at
> this point is if we would 'break' out of the loop.
>
> I don't think it hurts to be defensive here in case someone decides to
> break out of the loop in the future, just something odd I noticed
> while reviewing the code.
>

Yes, I also noticed that. But I thought it would be nice to make this
check so that this code stays consistent to the documentation at
dir-iterator.h (although implementation is different).

Something I just noticed now is that copy_or_link_directory dies upon
an opendir error, but dir-iterator just prints a warning and keeps
going (looking for another file/dir to return for the caller). Is this
ok? Or should I, perhaps, add a "pedantic" option to dir-iterator so
that, when enabled, it immediately returns ITER_ERROR when an error
occurs instead of keep going?

I'm proposing some new options to dir-iterator because as the patch
that adds it says "There are obviously a lot of features that could
easily be added to this class", and maybe those would be useful for
other dir-iterator users. But I don't know if that would be the best
way of doing it, so any feedback on this will be much appreciated.
Also, I saw on public-inbox[1] a patchset from 2017 proposing new
features/options for dir-iterator, but I don't really know if (and
why) it was rejected. (I couldn't find the patches on master/pu log)

[1] https://public-inbox.org/git/1493226219-33423-1-git-send-email-bnmvco@gmail.com/

Thanks,
Matheus Tavares


> >  }
> >
> >  static void clone_local(const char *src_repo, const char *dest_repo)
> > --
> > 2.20.1
> >

Re: [GSoC][PATCH 2/2] clone: use dir-iterator to avoid explicit dir traversal

[Thomas Gummerer]

On 02/18, Matheus Tavares Bernardino wrote:
> On Sat, Feb 16, 2019 at 12:38 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
> >
> > On 02/15, Matheus Tavares wrote:
> > > -     struct dirent *de;
> > > -     struct stat buf;
> > >       int src_len, dest_len;
> > > -     DIR *dir;
> > > -
> > > -     dir = opendir(src->buf);
> > > -     if (!dir)
> > > -             die_errno(_("failed to open '%s'"), src->buf);
> > > +     struct dir_iterator *iter;
> > > +     int iter_status;
> > >
> > >       mkdir_if_missing(dest->buf, 0777);
> > >
> > > +     iter = dir_iterator_begin(src->buf);
> > > +
> > >       strbuf_addch(src, '/');
> > >       src_len = src->len;
> > >       strbuf_addch(dest, '/');
> > >       dest_len = dest->len;
> > >
> > > -     while ((de = readdir(dir)) != NULL) {
> > > +     while ((iter_status = dir_iterator_advance(iter)) == ITER_OK) {
> > >               strbuf_setlen(src, src_len);
> > > -             strbuf_addstr(src, de->d_name);
> > > +             strbuf_addstr(src, iter->relative_path);
> > >               strbuf_setlen(dest, dest_len);
> > > -             strbuf_addstr(dest, de->d_name);
> > > -             if (stat(src->buf, &buf)) {
> > > -                     warning (_("failed to stat %s\n"), src->buf);
> > > -                     continue;
> > > -             }
> >
> > Why was this warning removed?  I don't see a corresponding warning in
> > the iterator API.  The one thing the iterator API is doing is that it
> > does an lstat on the path to check if it exists.  However that does
> > not follow symlinks, and ignores possible other errors such as
> > permission errors.
> >
> 
> You are right. I didn't know the differences from lstat and stat. And
> reflecting on this now, I realize that the problem is even deeper:
> copy_or_link_directory follows symlinks but dir-iterator don't, so I
> cannot use dir-iterator without falling back to recursion on
> copy_or_link_directory. Because of that, I thought off adding an
> option for dir-iterator to follow symlinks. Does this seem like a good
> idea?

Hmm looking at how this function is actually called, I don't think we
need to worry about symlinks too much.  It only copies the
.git/objects directory, which normally shouldn't contain any
symlinks.  I think it's still good to preserve the warning, in case
someone gave some object directories bad permissions, but following
symlinks in there may be overly paranoid, dunno.

Adding an option to dir-iterator to follow symlinks may be a good
idea, but it raises a few questions I think, e.g. how are we going to
deal with symlinks that point to a parent directory.  The answer for
this function is that we currently don't and just keep recursing,
until 'stat()' fails, but that is probably not the behaviour we'd want
if we were to add this functionality to dir-iterator.

At this point I'm almost convinced that not following symlinks may be
the better solution here.  That is a change in behaviour though, and
it should be described in the commit message why that change is a good
change.

> Also, I just noticed that dir-iterator follows hidden paths while
> copy_or_link_directory don't. Maybe another option to add for
> dir-iterator?

That feels like quite a special case in 'copy_or_link_directory()'.  I
don't think it's worth adding a feature like that for now, at least
not until we find another use case for it.  It's easy enough to skip
over hidden directories here.

> > If there is a good reason to remove the warning, that would be useful
> > to describe in the commit message.
> >
> > > -             if (S_ISDIR(buf.st_mode)) {
> > > -                     if (de->d_name[0] != '.')
> > > -                             copy_or_link_directory(src, dest,
> > > -                                                    src_repo, src_baselen);
> > > +             strbuf_addstr(dest, iter->relative_path);
> > > +
> > > +             if (S_ISDIR(iter->st.st_mode)) {
> > > +                     if (iter->basename[0] != '.')
> > > +                             mkdir_if_missing(dest->buf, 0777);
> > >                       continue;
> > >               }
> > >
> > >               /* Files that cannot be copied bit-for-bit... */
> > > -             if (!strcmp(src->buf + src_baselen, "/info/alternates")) {
> > > +             if (!strcmp(iter->relative_path, "info/alternates")) {
> > >                       copy_alternates(src, dest, src_repo);
> > >                       continue;
> > >               }
> > > @@ -463,7 +458,11 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,
> > >               if (copy_file_with_time(dest->buf, src->buf, 0666))
> > >                       die_errno(_("failed to copy file to '%s'"), dest->buf);
> > >       }
> > > -     closedir(dir);
> > > +
> > > +     if (iter_status != ITER_DONE) {
> > > +             strbuf_setlen(src, src_len);
> > > +             die(_("failed to iterate over '%s'"), src->buf);
> > > +     }
> >
> > Interestingly enough, this is not something that can currently
> > happen if I read the dir-iterator code correctly.  Even though the
> > dir_iterator_advance function says it can return ITER_ERROR, it never
> > actually does.  The only way the iter_status can be not ITER_DONE at
> > this point is if we would 'break' out of the loop.
> >
> > I don't think it hurts to be defensive here in case someone decides to
> > break out of the loop in the future, just something odd I noticed
> > while reviewing the code.
> >
> 
> Yes, I also noticed that. But I thought it would be nice to make this
> check so that this code stays consistent to the documentation at
> dir-iterator.h (although implementation is different).

Yeah, I'm not opposed to keep this check to be more defensive against
future changes of the API and the code here.

> Something I just noticed now is that copy_or_link_directory dies upon
> an opendir error, but dir-iterator just prints a warning and keeps
> going (looking for another file/dir to return for the caller). Is this
> ok? Or should I, perhaps, add a "pedantic" option to dir-iterator so
> that, when enabled, it immediately returns ITER_ERROR when an error
> occurs instead of keep going?

Good point, I failed to notice that during my earlier review.  I do
think a "pedantic" option would be a good idea here, to preserve the
previous behaviour.

> I'm proposing some new options to dir-iterator because as the patch
> that adds it says "There are obviously a lot of features that could
> easily be added to this class", and maybe those would be useful for
> other dir-iterator users. But I don't know if that would be the best
> way of doing it, so any feedback on this will be much appreciated.
> Also, I saw on public-inbox[1] a patchset from 2017 proposing new
> features/options for dir-iterator, but I don't really know if (and
> why) it was rejected. (I couldn't find the patches on master/pu log)

I don't think we should necessarily add new options because the
original patch said we should do so.  Instead the best way to
introduce new options is to introduce them when they are actually
going to be needed in subsequent patches.  So it would be nice to
implement features that are actually needed by your patches, but I
don't think it's worth trying to introduce new features that could
potentially be useful in the future, but where we don't know yet
whether they really will be or not.

I can't recall those patches, and from a quick look at the history I
can't tell why they were never merged.  The what's cooking emails from
Junio around that time may have a bit more background, but I don't
have time to dig them up now.

> [1] https://public-inbox.org/git/1493226219-33423-1-git-send-email-bnmvco@gmail.com/
> 
> Thanks,
> Matheus Tavares
> 
> 
> > >  }
> > >
> > >  static void clone_local(const char *src_repo, const char *dest_repo)
> > > --
> > > 2.20.1
> > >

Re: [GSoC][PATCH 2/2] clone: use dir-iterator to avoid explicit dir traversal

[Matheus Tavares Bernardino]

On Mon, Feb 18, 2019 at 8:35 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
>
> >
> > You are right. I didn't know the differences from lstat and stat. And
> > reflecting on this now, I realize that the problem is even deeper:
> > copy_or_link_directory follows symlinks but dir-iterator don't, so I
> > cannot use dir-iterator without falling back to recursion on
> > copy_or_link_directory. Because of that, I thought off adding an
> > option for dir-iterator to follow symlinks. Does this seem like a good
> > idea?
>
> Hmm looking at how this function is actually called, I don't think we
> need to worry about symlinks too much.  It only copies the
> .git/objects directory, which normally shouldn't contain any
> symlinks.  I think it's still good to preserve the warning, in case
> someone gave some object directories bad permissions, but following
> symlinks in there may be overly paranoid, dunno.
>
> Adding an option to dir-iterator to follow symlinks may be a good
> idea, but it raises a few questions I think, e.g. how are we going to
> deal with symlinks that point to a parent directory.  The answer for
> this function is that we currently don't and just keep recursing,
> until 'stat()' fails, but that is probably not the behaviour we'd want
> if we were to add this functionality to dir-iterator.
>
> At this point I'm almost convinced that not following symlinks may be
> the better solution here.  That is a change in behaviour though, and
> it should be described in the commit message why that change is a good
> change.
>

Ok, so I will add this note at v2's commit message.

> > Also, I just noticed that dir-iterator follows hidden paths while
> > copy_or_link_directory don't. Maybe another option to add for
> > dir-iterator?
>
> That feels like quite a special case in 'copy_or_link_directory()'.  I
> don't think it's worth adding a feature like that for now, at least
> not until we find another use case for it.  It's easy enough to skip
> over hidden directories here.
>

Ok, I agree. I noticed copy_or_link_directory only skips hidden
directories, not hidden files. And I couldn't think why we would want
to skip one but not the other... Could that be unintentional? I mean,
maybe the idea was to skip just "." and ".."? If that is the case, I
don't even need to make an if check at copy_or_link_directory, since
dir-iterator already skips "." and "..". What do you think about that?

> > > If there is a good reason to remove the warning, that would be useful
> > > to describe in the commit message.
> > >
> > > > -             if (S_ISDIR(buf.st_mode)) {
> > > > -                     if (de->d_name[0] != '.')
> > > > -                             copy_or_link_directory(src, dest,
> > > > -                                                    src_repo, src_baselen);
> > > > +             strbuf_addstr(dest, iter->relative_path);
> > > > +
> > > > +             if (S_ISDIR(iter->st.st_mode)) {
> > > > +                     if (iter->basename[0] != '.')
> > > > +                             mkdir_if_missing(dest->buf, 0777);
> > > >                       continue;
> > > >               }
> > > >
> > > >               /* Files that cannot be copied bit-for-bit... */
> > > > -             if (!strcmp(src->buf + src_baselen, "/info/alternates")) {
> > > > +             if (!strcmp(iter->relative_path, "info/alternates")) {
> > > >                       copy_alternates(src, dest, src_repo);
> > > >                       continue;
> > > >               }
> > > > @@ -463,7 +458,11 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,
> > > >               if (copy_file_with_time(dest->buf, src->buf, 0666))
> > > >                       die_errno(_("failed to copy file to '%s'"), dest->buf);
> > > >       }
> > > > -     closedir(dir);
> > > > +
> > > > +     if (iter_status != ITER_DONE) {
> > > > +             strbuf_setlen(src, src_len);
> > > > +             die(_("failed to iterate over '%s'"), src->buf);
> > > > +     }
> > >
> > > Interestingly enough, this is not something that can currently
> > > happen if I read the dir-iterator code correctly.  Even though the
> > > dir_iterator_advance function says it can return ITER_ERROR, it never
> > > actually does.  The only way the iter_status can be not ITER_DONE at
> > > this point is if we would 'break' out of the loop.
> > >
> > > I don't think it hurts to be defensive here in case someone decides to
> > > break out of the loop in the future, just something odd I noticed
> > > while reviewing the code.
> > >
> >
> > Yes, I also noticed that. But I thought it would be nice to make this
> > check so that this code stays consistent to the documentation at
> > dir-iterator.h (although implementation is different).
>
> Yeah, I'm not opposed to keep this check to be more defensive against
> future changes of the API and the code here.
>
> > Something I just noticed now is that copy_or_link_directory dies upon
> > an opendir error, but dir-iterator just prints a warning and keeps
> > going (looking for another file/dir to return for the caller). Is this
> > ok? Or should I, perhaps, add a "pedantic" option to dir-iterator so
> > that, when enabled, it immediately returns ITER_ERROR when an error
> > occurs instead of keep going?
>
> Good point, I failed to notice that during my earlier review.  I do
> think a "pedantic" option would be a good idea here, to preserve the
> previous behaviour.
>

Ok, I'm going to work on that. Thanks.

> > I'm proposing some new options to dir-iterator because as the patch
> > that adds it says "There are obviously a lot of features that could
> > easily be added to this class", and maybe those would be useful for
> > other dir-iterator users. But I don't know if that would be the best
> > way of doing it, so any feedback on this will be much appreciated.
> > Also, I saw on public-inbox[1] a patchset from 2017 proposing new
> > features/options for dir-iterator, but I don't really know if (and
> > why) it was rejected. (I couldn't find the patches on master/pu log)
>
> I don't think we should necessarily add new options because the
> original patch said we should do so.  Instead the best way to
> introduce new options is to introduce them when they are actually
> going to be needed in subsequent patches.  So it would be nice to
> implement features that are actually needed by your patches, but I
> don't think it's worth trying to introduce new features that could
> potentially be useful in the future, but where we don't know yet
> whether they really will be or not.
>

Yes, I agree with you. What I meant is that there is some space for
new features at dir-iterator and maybe those that I suggested could be
nice not just for my patch, on clone.c, but for others. But I see your
point. We really should try to keep it simpler and just add the
feature when (and if) there is a more expressive usage for it.

Thanks again for all the help and feedback.

Re: [GSoC][PATCH 2/2] clone: use dir-iterator to avoid explicit dir traversal

[Thomas Gummerer]

On 02/19, Matheus Tavares Bernardino wrote:
> On Mon, Feb 18, 2019 at 8:35 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
> > > Also, I just noticed that dir-iterator follows hidden paths while
> > > copy_or_link_directory don't. Maybe another option to add for
> > > dir-iterator?
> >
> > That feels like quite a special case in 'copy_or_link_directory()'.  I
> > don't think it's worth adding a feature like that for now, at least
> > not until we find another use case for it.  It's easy enough to skip
> > over hidden directories here.
> >
> 
> Ok, I agree. I noticed copy_or_link_directory only skips hidden
> directories, not hidden files. And I couldn't think why we would want
> to skip one but not the other... Could that be unintentional? I mean,
> maybe the idea was to skip just "." and ".."? If that is the case, I
> don't even need to make an if check at copy_or_link_directory, since
> dir-iterator already skips "." and "..". What do you think about that?

I don't know.  The best way to look at these things is usually to use
'git blame' and have a look at the commit that introduced whatever you
are looking at, in this case why we are skipping anything starting
with a '.'.  But looking at that commit it's just the translation from
shell script to builtin.  It's also more than 10 years old by now, so
it's unlikely that the original author would still remember why
exactly they made this choice.  (Note that I didn't take the time to
actually dig into the original shell script).

I feel like you are probably right in that it could be unintentional,
and I don't think anyone should be messing with the objects
directory.  However that is no guarantee that changing this wouldn't
break *something*.

For the purpose of this change I would try to keep the same behaviour
as we currently have where possible, to not increase the scope too
much.

> > > I'm proposing some new options to dir-iterator because as the patch
> > > that adds it says "There are obviously a lot of features that could
> > > easily be added to this class", and maybe those would be useful for
> > > other dir-iterator users. But I don't know if that would be the best
> > > way of doing it, so any feedback on this will be much appreciated.
> > > Also, I saw on public-inbox[1] a patchset from 2017 proposing new
> > > features/options for dir-iterator, but I don't really know if (and
> > > why) it was rejected. (I couldn't find the patches on master/pu log)
> >
> > I don't think we should necessarily add new options because the
> > original patch said we should do so.  Instead the best way to
> > introduce new options is to introduce them when they are actually
> > going to be needed in subsequent patches.  So it would be nice to
> > implement features that are actually needed by your patches, but I
> > don't think it's worth trying to introduce new features that could
> > potentially be useful in the future, but where we don't know yet
> > whether they really will be or not.
> >
> 
> Yes, I agree with you. What I meant is that there is some space for
> new features at dir-iterator and maybe those that I suggested could be
> nice not just for my patch, on clone.c, but for others. But I see your
> point. We really should try to keep it simpler and just add the
> feature when (and if) there is a more expressive usage for it.

Right, if you wanted to pursue this, that could be a separate patch
series.  However generally patch series that just introduce something
that might be used later, but doesn't add much value on its own tend
to be much more likely to be rejected.

> Thanks again for all the help and feedback.

Thanks for working on this!

Re: [GSoC][PATCH 2/2] clone: use dir-iterator to avoid explicit dir traversal

[Matheus Tavares Bernardino]

Ok, I think I'm almost there and I should be able to send a v2 on the
weekend. But again, a few questions arose while I'm coding v2. Please,
see inline.

On Tue, Feb 19, 2019 at 8:45 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
>
> On 02/19, Matheus Tavares Bernardino wrote:
> > Ok, I agree. I noticed copy_or_link_directory only skips hidden
> > directories, not hidden files. And I couldn't think why we would want
> > to skip one but not the other... Could that be unintentional? I mean,
> > maybe the idea was to skip just "." and ".."? If that is the case, I
> > don't even need to make an if check at copy_or_link_directory, since
> > dir-iterator already skips "." and "..". What do you think about that?

[...]

> I feel like you are probably right in that it could be unintentional,
> and I don't think anyone should be messing with the objects
> directory.  However that is no guarantee that changing this wouldn't
> break *something*.
>
> For the purpose of this change I would try to keep the same behaviour
> as we currently have where possible, to not increase the scope too
> much.
>

I understand your point, but to make copy_or_link_directory() skip
just hidden directories using dir-iterator seems, now, a little
tricker than I though before. The "if (iter->basename[0] != '.')"
statement in the patch I sent for v1 only skips the hidden directories
creation, but it does not avoid the attempt to copy the hidden
directory contents (which would obviously fail). If we were to do
that, we would need to check every directory in the relative path
string of each iteration entry looking for a hidden directory. That
seems a little too much, IMHO. Because of that and the intuition that
skipping over hidden dirs was an unintentional operation, I think we
could modify copy_or_link_directory() to skip '.' and '..', only.
Also, hidden dirs/files are not expected to be at .git/objects anyway,
are they?

And to have copy_or_link_directory()'s behaviour changed as little as
possible, I could add the option to not follow hidden paths (dirs and
regular files) at dir-iterator.c and use it at
copy_or_link_directory() (now that I'm adding the 'pedantic' option,
this won't be so difficult, anyway). Would these suggestions be a good
plan?

Thanks,
Matheus Tavares

Re: [GSoC][PATCH 2/2] clone: use dir-iterator to avoid explicit dir traversal

[Thomas Gummerer]

On 02/21, Matheus Tavares Bernardino wrote:
> Ok, I think I'm almost there and I should be able to send a v2 on the
> weekend. But again, a few questions arose while I'm coding v2. Please,
> see inline.
> 
> On Tue, Feb 19, 2019 at 8:45 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
> >
> > On 02/19, Matheus Tavares Bernardino wrote:
> > > Ok, I agree. I noticed copy_or_link_directory only skips hidden
> > > directories, not hidden files. And I couldn't think why we would want
> > > to skip one but not the other... Could that be unintentional? I mean,
> > > maybe the idea was to skip just "." and ".."? If that is the case, I
> > > don't even need to make an if check at copy_or_link_directory, since
> > > dir-iterator already skips "." and "..". What do you think about that?
> 
> [...]
> 
> > I feel like you are probably right in that it could be unintentional,
> > and I don't think anyone should be messing with the objects
> > directory.  However that is no guarantee that changing this wouldn't
> > break *something*.
> >
> > For the purpose of this change I would try to keep the same behaviour
> > as we currently have where possible, to not increase the scope too
> > much.
> >
> 
> I understand your point, but to make copy_or_link_directory() skip
> just hidden directories using dir-iterator seems, now, a little
> tricker than I though before. The "if (iter->basename[0] != '.')"
> statement in the patch I sent for v1 only skips the hidden directories
> creation, but it does not avoid the attempt to copy the hidden
> directory contents (which would obviously fail). If we were to do
> that, we would need to check every directory in the relative path
> string of each iteration entry looking for a hidden directory. That
> seems a little too much, IMHO. Because of that and the intuition that
> skipping over hidden dirs was an unintentional operation, I think we
> could modify copy_or_link_directory() to skip '.' and '..', only.
> Also, hidden dirs/files are not expected to be at .git/objects anyway,
> are they?

No, as far as I know hidden dirs/files should not be in .git/objects.
The only reason they would be there is if somebody would create them
manually.  I didn't think of hidden child directories, which as you
noticed should be excluded as well to keep current behaviour.

With that in mind, I think I agree with the change of only excluding
'.' and '..', for which we don't need to do anything in your
dir-iterator patches.  Also thinking of what the worst case that could
happen is, it's that while cloning a repository locally a few more
directories could be copied, which is not all that bad.

So I think I was overly paranoid here and am on board with skipping
'.' and '..' only.  I even think that this can be just described in
detail in the commit message, rather than being done in a separate
patch, though others may disagree with that.  

> And to have copy_or_link_directory()'s behaviour changed as little as
> possible, I could add the option to not follow hidden paths (dirs and
> regular files) at dir-iterator.c and use it at
> copy_or_link_directory() (now that I'm adding the 'pedantic' option,
> this won't be so difficult, anyway). Would these suggestions be a good
> plan?

I think this would be a valid plan as well, however thinking this
through again I don't feel like it is necessary.  I'm happy with
whichever way you prefer here.

> Thanks,
> Matheus Tavares

Re: [GSoC][PATCH 2/2] clone: use dir-iterator to avoid explicit dir traversal

[Daniel Ferreira (theiostream)]

Hi there! I'm happy to see this series has gotten renewed attention.

The series was fine per se, and it even got through to the pu branch.
But then it got ejected since it conflicted non-trivially with another
patch series that was added along the way (see
https://lwn.net/Articles/724166/). At that point I did not keep
pushing it forward.

I'd be happy to help anyone willing to make some progress on this! (Or
even ultimately solve the conflict myself, if it serves a greater goal
than just being a GSoC microproject.)

Best,
-- Daniel.