On Tue, Feb 26, 2019 at 9:18 AM Duy Nguyen <pclouds@xxxxxxxxx> wrote:
>
> On Tue, Feb 26, 2019 at 12:18 PM Matheus Tavares
> <matheus.bernardino@xxxxxx> wrote:
> >
> > Extract dir creation code snippet from copy_or_link_directory to its own
> > function named mkdir_if_missing. This change will help removing
> > copy_or_link_directory's explicit recursion, which will be done in a
> > following patch. Also makes code more readable.
> >
> > Signed-off-by: Matheus Tavares <matheus.bernardino@xxxxxx>
> > ---
> > builtin/clone.c | 27 +++++++++++++++++++--------
> > 1 file changed, 19 insertions(+), 8 deletions(-)
> >
> > diff --git a/builtin/clone.c b/builtin/clone.c
> > index cae069f03b..fd580fa98d 100644
> > --- a/builtin/clone.c
> > +++ b/builtin/clone.c
> > @@ -392,6 +392,24 @@ static void copy_alternates(struct strbuf *src, struct strbuf *dst,
> > fclose(in);
> > }
> >
> > +static void mkdir_if_missing(const char *pathname, mode_t mode)
> > +{
> > + /*
> > + * Create a dir at pathname unless there's already one.
>
> This confused me for a second because I thought it described "st"
> variable. I think we usually put the description of the function on
> top (before the "void mkdir_if.." line). But with a such a short
> function and clear name like this, I don't think we need any comments.
>
Yes, I also don't like the description being after the function
declaration, but I did this to follow the pattern from other functions
on the same file (e.g. copy_alternates). Anyway, I do agree with you
that this function don't need a description, so I'm removing it for
the next version. Thanks!
> > + */
> > + struct stat st;
> > +
> > + if (mkdir(pathname, mode)) {
>
> Good opportunity to unindent this by doing
>
> if (!mkdir(...
> return;
>
> but it's up to you.
>
Ok. But being such a small snippet, is the indentation really a code
smell here? (sorry, I'm still getting used to git's coding guidelines)
> > + if (errno != EEXIST)
> > + die_errno(_("failed to create directory '%s'"),
> > + pathname);
> > + else if (stat(pathname, &st))
> > + die_errno(_("failed to stat '%s'"), pathname);
> > + else if (!S_ISDIR(st.st_mode))
> > + die(_("%s exists and is not a directory"), pathname);
> > + }
> > +}
> > +
> > static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,
> > const char *src_repo, int src_baselen)
> > {
> > @@ -404,14 +422,7 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,
> > if (!dir)
> > die_errno(_("failed to open '%s'"), src->buf);
> >
> > - if (mkdir(dest->buf, 0777)) {
> > - if (errno != EEXIST)
> > - die_errno(_("failed to create directory '%s'"), dest->buf);
> > - else if (stat(dest->buf, &buf))
> > - die_errno(_("failed to stat '%s'"), dest->buf);
> > - else if (!S_ISDIR(buf.st_mode))
> > - die(_("%s exists and is not a directory"), dest->buf);
> > - }
> > + mkdir_if_missing(dest->buf, 0777);
> >
> > strbuf_addch(src, '/');
> > src_len = src->len;
> > --
> > 2.20.1
> >
>
>
> --
> Duy
