[GSoC,1/2] clone: extract function from copy_or_link_directory
diff mbox series

Message ID 20190215154913.18800-2-matheus.bernardino@usp.br
State New
Headers show
Series
  • clone: convert explicit traversal to
Related show

Commit Message

Matheus Tavares Bernardino Feb. 15, 2019, 3:49 p.m. UTC
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(-)

Comments

Christian Couder Feb. 16, 2019, 6:23 a.m. UTC | #1
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.
Matheus Tavares Bernardino Feb. 18, 2019, 9:28 p.m. UTC | #2
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.

Patch
diff mbox series

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;