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