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

From: Matheus Tavares Bernardino <matheus.bernardino@usp.br>
Date: Feb 14, 2019, 22:03 UTC
Message-ID: <CAHd-oW6uHKfa_P+fZNZxG4+pme=SH_Wi+SJkhxwOtfR+L=0JBA@mail.gmail.com>
In-Reply-To: <CAP8UFD1aBn8=KOTp-D=CmO1iEnWUBS1-jR-aqfg3MZ7HZRU7Pg@mail.gmail.com>

On Thu, Feb 14, 2019 at 7:16 PM Christian Couder <christian.couder@gmail.com> wrote:

Show 15 quoted lines

>
> On Thu, Feb 14, 2019 at 1:16 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.
> >
> > Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
> > ---
> > This is my microproject for GSoC 2019. It's still a RFC because I have
> > some questions. Any help will be much appreciated.
>
> Thanks for working on a microproject!
>

Hi, Christian. Thank you for the review and comments.

Show 8 quoted lines

> > There're three places inside copy_or_link_directory's loop where
> > die_errno() is called. Should I call dir_iterator_abort, at these
> > places, before die_errno() is called (to free resources)?
>
> I don't think it's necessary. We care about freeing resources when we
> report errors (for example by returning an error code from inside a
> function), but not when we are exiting.
>

Ok, thanks!

Show 9 quoted lines

> > -static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,
> > -                                  const char *src_repo, int src_baselen)
> > +static void mkdir_if_missing(const char *pathname, mode_t mode)
>
> It looks like your patch is both splitting copy_or_link_directory()
> into 2 functions and converting it to use the dir-iterator API. Maybe
> it would be better to have 2 patches instead, one for splitting it and
> one for converting it.
>

Got it. As the justification for splitting the function was to use the extracted part in the section that was previously recursive, I thought both changes should be in the same patch. But I really was in doubt about that. Should I split it into two patches and mention that justification at the first one? Or just split?

Show 10 quoted lines

> >  {
> > -       struct dirent *de;
> > +       /*
> > +        * Tries to create a dir at pathname. If pathname already exists and
> > +        * is a dir, do nothing.
> > +        */
>
> I think we usually put such comments just before the function. And
> maybe it could be shortened to "Create a dir at pathname unless
> there's already one"

Right, the shortened version does sounds much better, thanks! About the comment placement, I followed what I saw in other functions from the same file ("copy_alternates", for example). But also, I couldn't find any instruction about that at Documentation/CodingGuidelines. So should I move it as you suggested?