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

[Matheus Tavares Bernardino <matheus.bernardino@usp.br>]

On Sat, Feb 23, 2019 at 6:48 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
>
> On 02/23, Matheus Tavares wrote:
> > ---
> > Changes in v2:
> >  - Improved patch message
> >  - Removed a now unused variable
>
> s/variable/parameter/ I believe?

Yes, you are right!

> > +     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)) {
> > +             strbuf_addstr(dest, iter->relative_path);
> > +
> > +             /*
> > +              * dir_iterator_advance already calls lstat to populate iter->st
> > +              * but, unlike stat, lstat does not checks for permissions on
> > +              * the given path.
> > +              */
>
> Hmm, lstat does check the permissions on the path, it just doesn't
> follow symlinks.  I think I actually mislead you in my previous review
> here, and was reading the code in dir-iterator.c all wrong.
>
> I thought it said "if (errno == ENOENT) warning(...)", however the
> condition is "errno != ENOENT", which is why I thought we were loosing
> warnings when errno == EACCES for example.
>
> As we decided that we would no longer follow symlinks now, I think we
> can actually get rid of the stat call here.  Sorry about the confusion.
>

Ok, I also read the lstat man page wrongly and though it didn't check
for permissions. Thanks for noticing that. I will remove the lstat
call in v3.