On Thu, Feb 14, 2019 at 7:16 PM Christian Couder
<[email protected]> wrote:
>
> On Thu, Feb 14, 2019 at 1:16 PM Matheus Tavares
> <[email protected]> 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 <[email protected]>
> > ---
> > 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.
> > 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!
> > -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?
> > {
> > - 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?
