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