| Message ID | 20190215154913.18800-2-matheus.bernardino@usp.br (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | clone: convert explicit traversal to | expand |
On Fri, Feb 15, 2019 at 5:39 PM Matheus Tavares <matheus.bernardino@usp.br> 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 patch > "clone: use dir-iterator to avoid explicit dir traversal". "which will be done in a following patch" is enough and perhaps even better as the following patch can then be changed independently of this one. > Also makes > code more readable. > > Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br> [...] > +static void mkdir_if_missing(const char *pathname, mode_t mode) > +{ > + /* > + * Create a dir at pathname unless there's already one. > + */ > + struct stat buf; I know that the variable was already called "buf" in copy_or_link_directory() and that there are a few other places in the code where a 'struct stat' is called "buf", but in most places the 'struct stat' variables are called "st": $ git grep 'struct stat ' '*.c' | perl -ne 'print "$1\n" if (m/struct stat ([\w_]+)/);' | sort | uniq -c | sort -nr 129 st 6 sb 3 buf 2 statbuf 1 st_stdin 1 st_git 1 statbuffer 1 st2 1 st1 1 nst 1 loginfo 1 cwd_stat 1 argstat So I wonder if we should use "st" here instead of "buf". We also often use "buf" for 'struct strbuf' variables which can be confusing. Otherwise the patch looks good to me.
On Sat, Feb 16, 2019 at 4:23 AM Christian Couder <christian.couder@gmail.com> wrote: > > On Fri, Feb 15, 2019 at 5:39 PM Matheus Tavares > <matheus.bernardino@usp.br> 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 patch > > "clone: use dir-iterator to avoid explicit dir traversal". > > "which will be done in a following patch" is enough and perhaps even > better as the following patch can then be changed independently of > this one. > Ok! I will fix that in v2. > > Also makes > > code more readable. > > > > Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br> > > [...] > > > +static void mkdir_if_missing(const char *pathname, mode_t mode) > > +{ > > + /* > > + * Create a dir at pathname unless there's already one. > > + */ > > + struct stat buf; > > I know that the variable was already called "buf" in > copy_or_link_directory() and that there are a few other places in the > code where a 'struct stat' is called "buf", but in most places the > 'struct stat' variables are called "st": > > $ git grep 'struct stat ' '*.c' | perl -ne 'print "$1\n" if (m/struct > stat ([\w_]+)/);' | sort | uniq -c | sort -nr > 129 st > 6 sb > 3 buf > 2 statbuf > 1 st_stdin > 1 st_git > 1 statbuffer > 1 st2 > 1 st1 > 1 nst > 1 loginfo > 1 cwd_stat > 1 argstat > > So I wonder if we should use "st" here instead of "buf". We also often > use "buf" for 'struct strbuf' variables which can be confusing. > Right! I'm changing that in v2 too. Thanks, Matheus > Otherwise the patch looks good to me.
diff --git a/builtin/clone.c b/builtin/clone.c index 50bde99618..2a1cc4dab9 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. + */ + struct stat buf; + + if (mkdir(pathname, mode)) { + if (errno != EEXIST) + die_errno(_("failed to create directory '%s'"), + pathname); + else if (stat(pathname, &buf)) + die_errno(_("failed to stat '%s'"), pathname); + else if (!S_ISDIR(buf.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;
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 patch "clone: use dir-iterator to avoid explicit dir traversal". Also makes code more readable. Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br> --- builtin/clone.c | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-)