On Tue, Feb 26, 2019 at 9:18 AM Duy Nguyen [off-list ref] wrote:

On Tue, Feb 26, 2019 at 12:18 PM Matheus Tavares
[off-list ref] wrote:

quoted

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

quoted

+        */
+       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)

quoted

+               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