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

From: Christian Couder <christian.couder@gmail.com>
Date: Feb 15, 2019, 08:59 UTC
Message-ID: <CAP8UFD0c7oPdaPkU55mus-3xgYcMmUSa_dsuGcGRF0ijn6r8iw@mail.gmail.com>
In-Reply-To: <CAHd-oW6uHKfa_P+fZNZxG4+pme=SH_Wi+SJkhxwOtfR+L=0JBA@mail.gmail.com>

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

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

Show 15 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?

If 2 patches instead of 1 makes it easier to review and understand what's going on, then I think you should indeed send 2 patches and mention the justification for splitting the function in the commit message of the first patch.

Show 7 quoted lines

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

Then it's ok to place it like you did. Sorry about the noise.

> But also, I couldn't
> find any instruction about that at Documentation/CodingGuidelines. So
> should I move it as you suggested?

I think we have not been very consistent over the years. Recently I think we have tried to add or move API documentation inside header files, and in general before functions in the code, but yeah it might not have been documented.