| Message ID | 20190226051804.10631-5-matheus.bernardino@usp.br (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | clone: dir iterator refactoring with tests | expand |
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 >
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
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.
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 :)
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;
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(-)