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

From: Christian Couder <christian.couder@gmail.com>
Date: Feb 14, 2019, 21:16 UTC
Message-ID: <CAP8UFD1aBn8=KOTp-D=CmO1iEnWUBS1-jR-aqfg3MZ7HZRU7Pg@mail.gmail.com>
In-Reply-To: <20190213205554.4086-1-matheus.bernardino@usp.br>

On Thu, Feb 14, 2019 at 1:16 PM Matheus Tavares <matheus.bernardino@usp.br> wrote:

Show 9 quoted lines

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

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

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

Show 6 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"