| Message ID | b04188c822c32aab6ef59099a0c9078aeda065c3.1641978175.git.dyroneteng@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ls-tree: "--object-only" and "--format" opts | expand |
On Thu, Jan 13 2022, Teng Long wrote: Re the $subject: Is "optimize naming" here just referring to the s/retval/recurse/g? Personally I think just a s/retval/ret/g here would make more senes if we're doing any change at all, and in either case having this variable re-rename split up as its own commit would make the proposed control flow changes clearer. > The variable which "show_tree()" return is named "retval", a name that's > a little hard to understand. This commit tries to make the variable > and the related codes more clear in the context. > > The commit firstly rename "retval" to "recurse" which is a more > meaningful name than before. Secondly, "get_type()" is introduced > to setup the "type" by "mode", this will remove some of the nested if. > After this, The codes here become a little bit clearer, so we do not > need to take a look at "read_tree_at()" in "tree.c" to make sure the > context of the return value. > > Signed-off-by: Teng Long <dyronetengb@gmail.com> > --- > builtin/ls-tree.c | 33 ++++++++++++++++++--------------- > 1 file changed, 18 insertions(+), 15 deletions(-) > > diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c > index eecc7482d5..9729854a3d 100644 > --- a/builtin/ls-tree.c > +++ b/builtin/ls-tree.c > @@ -61,24 +61,27 @@ static int show_recursive(const char *base, size_t baselen, const char *pathname > return 0; > } > > +static enum object_type get_type(unsigned int mode) > +{ > + return (S_ISGITLINK(mode) > + ? OBJ_COMMIT > + : S_ISDIR(mode) > + ? OBJ_TREE > + : OBJ_BLOB); > +} This new function is a re-invention of the object_type() utility in cache.h, and isn't needed. I.e.... > static int show_tree(const struct object_id *oid, struct strbuf *base, > const char *pathname, unsigned mode, void *context) > { > - int retval = 0; > + int recurse = 0; > size_t baselen; > - enum object_type type = OBJ_BLOB; > - > - if (S_ISGITLINK(mode)) { > - type = OBJ_COMMIT; > - } else if (S_ISDIR(mode)) { > - if (show_recursive(base->buf, base->len, pathname)) { > - retval = READ_TREE_RECURSIVE; > - if (!(ls_options & LS_SHOW_TREES)) > - return retval; > - } > - type = OBJ_TREE; > - } > - else if (ls_options & LS_TREE_ONLY) > + enum object_type type = get_type(mode); ...just drop it and do this: - enum object_type type = get_type(mode); + enum object_type type = object_type(mode);
On Thu, Jan 13, 2022 at 2:55 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > Re the $subject: Is "optimize naming" here just referring to the > s/retval/recurse/g? Yes. > Personally I think just a s/retval/ret/g here would make more senes if > we're doing any change at all, and in either case having this variable > re-rename split up as its own commit would make the proposed control > flow changes clearer. Do you mean that I can split the current one into two commits, one does the renaming work and another one does the left work? If so, I will do this in the next patch. > > This new function is a re-invention of the object_type() utility in > cache.h, and isn't needed. I.e.... > > ...just drop it and do this: > > - enum object_type type = get_type(mode); > + enum object_type type = object_type(mode); You are absolutely correct. I will replace get_type() to object_type() in the next patch.
On Fri, Jan 14 2022, Teng Long wrote: > On Thu, Jan 13, 2022 at 2:55 PM Ævar Arnfjörð Bjarmason > <avarab@gmail.com> wrote: > >> Re the $subject: Is "optimize naming" here just referring to the >> s/retval/recurse/g? > > Yes. > >> Personally I think just a s/retval/ret/g here would make more senes if >> we're doing any change at all, and in either case having this variable >> re-rename split up as its own commit would make the proposed control >> flow changes clearer. > > Do you mean that I can split the current one into two commits, one does > the renaming work and another one does the left work? > > If so, I will do this in the next patch. Yes, at least I would find it easier to read :) >> >> This new function is a re-invention of the object_type() utility in >> cache.h, and isn't needed. I.e.... >> >> ...just drop it and do this: >> >> - enum object_type type = get_type(mode); >> + enum object_type type = object_type(mode); > > You are absolutely correct. > I will replace get_type() to object_type() in the next patch. Thanks!
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c index eecc7482d5..9729854a3d 100644 --- a/builtin/ls-tree.c +++ b/builtin/ls-tree.c @@ -61,24 +61,27 @@ static int show_recursive(const char *base, size_t baselen, const char *pathname return 0; } +static enum object_type get_type(unsigned int mode) +{ + return (S_ISGITLINK(mode) + ? OBJ_COMMIT + : S_ISDIR(mode) + ? OBJ_TREE + : OBJ_BLOB); +} + static int show_tree(const struct object_id *oid, struct strbuf *base, const char *pathname, unsigned mode, void *context) { - int retval = 0; + int recurse = 0; size_t baselen; - enum object_type type = OBJ_BLOB; - - if (S_ISGITLINK(mode)) { - type = OBJ_COMMIT; - } else if (S_ISDIR(mode)) { - if (show_recursive(base->buf, base->len, pathname)) { - retval = READ_TREE_RECURSIVE; - if (!(ls_options & LS_SHOW_TREES)) - return retval; - } - type = OBJ_TREE; - } - else if (ls_options & LS_TREE_ONLY) + enum object_type type = get_type(mode); + + if (type == OBJ_TREE && show_recursive(base->buf, base->len, pathname)) + recurse = READ_TREE_RECURSIVE; + if (type == OBJ_TREE && recurse && !(ls_options & LS_SHOW_TREES)) + return recurse; + if (type == OBJ_BLOB && (ls_options & LS_TREE_ONLY)) return 0; if (!(ls_options & LS_NAME_ONLY)) { @@ -109,7 +112,7 @@ static int show_tree(const struct object_id *oid, struct strbuf *base, chomp_prefix ? ls_tree_prefix : NULL, stdout, line_termination); strbuf_setlen(base, baselen); - return retval; + return recurse; } int cmd_ls_tree(int argc, const char **argv, const char *prefix)
The variable which "show_tree()" return is named "retval", a name that's a little hard to understand. This commit tries to make the variable and the related codes more clear in the context. The commit firstly rename "retval" to "recurse" which is a more meaningful name than before. Secondly, "get_type()" is introduced to setup the "type" by "mode", this will remove some of the nested if. After this, The codes here become a little bit clearer, so we do not need to take a look at "read_tree_at()" in "tree.c" to make sure the context of the return value. Signed-off-by: Teng Long <dyronetengb@gmail.com> --- builtin/ls-tree.c | 33 ++++++++++++++++++--------------- 1 file changed, 18 insertions(+), 15 deletions(-)