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

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