| Message ID | 3816a65fe62b1039c396eb38d27bf2c22e8d84de.1644319434.git.dyroneteng@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ls-tree: "--object-only" and "--format" opts | expand |
On Tue, Feb 08 2022, Teng Long wrote: > This commit use "object_type()" to get the type, then remove > some of the nested if to let the codes here become more cleaner. > > Signed-off-by: Teng Long <dyronetengb@gmail.com> > --- > builtin/ls-tree.c | 18 ++++++------------ > 1 file changed, 6 insertions(+), 12 deletions(-) > > diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c > index ef8c414f61..9c57a36c8c 100644 > --- a/builtin/ls-tree.c > +++ b/builtin/ls-tree.c > @@ -66,19 +66,13 @@ static int show_tree(const struct object_id *oid, struct strbuf *base, > { > int recurse = 0; > size_t baselen; > - enum object_type type = OBJ_BLOB; > + enum object_type type = object_type(mode); > > - if (S_ISGITLINK(mode)) { > - type = OBJ_COMMIT; > - } else if (S_ISDIR(mode)) { > - if (show_recursive(base->buf, base->len, pathname)) { > - recurse = READ_TREE_RECURSIVE; > - if (!(ls_options & LS_SHOW_TREES)) > - return recurse; > - } > - type = OBJ_TREE; > - } > - else if (ls_options & LS_TREE_ONLY) > + 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)) { I think the use of object_type() here is good, but in any case I think doing a minimal change first for the "type" and then this proposed refactoring would be easier to look at, and to independently decide on the two. I find this much easier to read, both as a diff and as end-state: diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c index ef8c414f61a..0af09e94a03 100644 --- a/builtin/ls-tree.c +++ b/builtin/ls-tree.c @@ -66,20 +66,18 @@ static int show_tree(const struct object_id *oid, struct strbuf *base, { int recurse = 0; size_t baselen; - enum object_type type = OBJ_BLOB; + enum object_type type = object_type(mode); - if (S_ISGITLINK(mode)) { - type = OBJ_COMMIT; - } else if (S_ISDIR(mode)) { + if (type == OBJ_BLOB) { + if (ls_options & LS_TREE_ONLY) + return 0; + } else if (type == OBJ_TREE) { if (show_recursive(base->buf, base->len, pathname)) { recurse = READ_TREE_RECURSIVE; if (!(ls_options & LS_SHOW_TREES)) return recurse; } - type = OBJ_TREE; } - else if (ls_options & LS_TREE_ONLY) - return 0; if (!(ls_options & LS_NAME_ONLY)) { if (ls_options & LS_SHOW_SIZE) { Unrolling this from a logical if/else if to an if/if/if I think also doesn't make sense. At the cost of a slightly larger diff (could be done on top) we get rid of the show_recursive() branch too: diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c index ef8c414f61a..d4be71bad24 100644 --- a/builtin/ls-tree.c +++ b/builtin/ls-tree.c @@ -66,20 +66,17 @@ static int show_tree(const struct object_id *oid, struct strbuf *base, { int recurse = 0; size_t baselen; - enum object_type type = OBJ_BLOB; + enum object_type type = object_type(mode); - if (S_ISGITLINK(mode)) { - type = OBJ_COMMIT; - } else if (S_ISDIR(mode)) { - if (show_recursive(base->buf, base->len, pathname)) { - recurse = READ_TREE_RECURSIVE; - if (!(ls_options & LS_SHOW_TREES)) - return recurse; - } - type = OBJ_TREE; + if (type == OBJ_BLOB) { + if (ls_options & LS_TREE_ONLY) + return 0; + } else if (type == OBJ_TREE && + show_recursive(base->buf, base->len, pathname)) { + recurse = READ_TREE_RECURSIVE; + if (!(ls_options & LS_SHOW_TREES)) + return recurse; } - else if (ls_options & LS_TREE_ONLY) - return 0; if (!(ls_options & LS_NAME_ONLY)) { if (ls_options & LS_SHOW_SIZE) { Which, I think is also nicer to read, we're not checking "is it a tree?", setting "recursive", and then using that "recursive" as a boolean for no reason. Let's just continue on that "else if" chain we're already in instead...
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c index ef8c414f61..9c57a36c8c 100644 --- a/builtin/ls-tree.c +++ b/builtin/ls-tree.c @@ -66,19 +66,13 @@ static int show_tree(const struct object_id *oid, struct strbuf *base, { int recurse = 0; size_t baselen; - enum object_type type = OBJ_BLOB; + enum object_type type = object_type(mode); - if (S_ISGITLINK(mode)) { - type = OBJ_COMMIT; - } else if (S_ISDIR(mode)) { - if (show_recursive(base->buf, base->len, pathname)) { - recurse = READ_TREE_RECURSIVE; - if (!(ls_options & LS_SHOW_TREES)) - return recurse; - } - type = OBJ_TREE; - } - else if (ls_options & LS_TREE_ONLY) + 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)) {
This commit use "object_type()" to get the type, then remove some of the nested if to let the codes here become more cleaner. Signed-off-by: Teng Long <dyronetengb@gmail.com> --- builtin/ls-tree.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-)