[WIP,RFC,v2,4/5] clone: extract function from copy_or_link_directory
diff mbox series

Message ID 20190226051804.10631-5-matheus.bernardino@usp.br
State New
Headers show
Series
  • clone: dir iterator refactoring with tests
Related show

Commit Message

Matheus Tavares Bernardino Feb. 26, 2019, 5:18 a.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 a
following patch. 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

Duy Nguyen Feb. 26, 2019, 12:18 p.m. UTC | #1
On Tue, Feb 26, 2019 at 12:18 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 a
> following patch. 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(-)
>
> 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.

> +        */
> +       struct stat st;
> +
> +       if (mkdir(pathname, mode)) {

Good opportunity to unindent this by doing

    if (!mkdir(...
         return;

but it's up to you.

> +               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
>
Matheus Tavares Bernardino Feb. 27, 2019, 5:30 p.m. UTC | #2
On Tue, Feb 26, 2019 at 9:18 AM Duy Nguyen <pclouds@gmail.com> wrote:
>
> On Tue, Feb 26, 2019 at 12:18 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 a
> > following patch. 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(-)
> >
> > 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
Thomas Gummerer Feb. 27, 2019, 10:45 p.m. UTC | #3
On 02/27, Matheus Tavares Bernardino wrote:
> On Tue, Feb 26, 2019 at 9:18 AM Duy Nguyen <pclouds@gmail.com> wrote:
> >
> > On Tue, Feb 26, 2019 at 12:18 PM Matheus Tavares
> > <matheus.bernardino@usp.br> wrote:
> > > +        */
> > > +       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)

I don't think the indentation here is too bad here, but I think the
code is slightly easier to read with less indentation, and it's easier
to see what's happening in the success case as well without reading
the whole method.

And since this patch is already refactoring code we could do it here.
I don't think it's a very big deal either way, which is why Duy left
the decision on whether to use the suggestion or not up to you.
Matheus Tavares Bernardino Feb. 27, 2019, 10:50 p.m. UTC | #4
On Wed, Feb 27, 2019 at 7:45 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
>
> On 02/27, Matheus Tavares Bernardino wrote:
> > On Tue, Feb 26, 2019 at 9:18 AM Duy Nguyen <pclouds@gmail.com> wrote:
> > >
> > > On Tue, Feb 26, 2019 at 12:18 PM Matheus Tavares
> > > <matheus.bernardino@usp.br> wrote:
> > > > +        */
> > > > +       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)
>
> I don't think the indentation here is too bad here, but I think the
> code is slightly easier to read with less indentation, and it's easier
> to see what's happening in the success case as well without reading
> the whole method.
>
> And since this patch is already refactoring code we could do it here.
> I don't think it's a very big deal either way, which is why Duy left
> the decision on whether to use the suggestion or not up to you.

Ok, so I will do it for the next version! I just asked because it was
a good chance for me to learn a bit more about git's code style :)

Patch
diff mbox series

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.
+	 */
+	struct stat st;
+
+	if (mkdir(pathname, mode)) {
+		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;