Subject: Re: [PATCH v11 06/13] ls-tree: simplify nesting if/else logic in "show_tree()"

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Sat, 19 Feb 2022 07:06:51 +0100

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 <[email-masked]>
> ---
>  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...