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@xxxxxxxxx>
> ---
> 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...
