[v8,5/8] ls-tree: split up the "init" part of show_tree()

Commit Message

Teng Long Jan. 1, 2022, 1:50 p.m. UTC

From: Ævar Arnfjörð Bjarmason <[e-mail]>

Split up the "init" part of the show_tree() function where we decide
what the "type" is, and whether we'll return early. This makes things
a bit less readable for now, but we'll soon re-use this in a sibling
function, and avoiding the duplication will be worth it.

Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
---
 builtin/ls-tree.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

Comments

Junio C Hamano Jan. 4, 2022, 2:06 a.m. UTC | #1

Teng Long <[e-mail]> writes:


> -static int show_tree(const struct object_id *oid, struct strbuf *base,
> -		const char *pathname, unsigned mode, void *context)
> +static int show_tree_init(enum object_type *type, struct strbuf *base,
> +			  const char *pathname, unsigned mode, int *retval)

Don't we need some comment that explains what the function does,
what its return value means, etc.?

>  {
> -	int retval = 0;
> -	size_t baselen;
> -	enum object_type type = OBJ_BLOB;
> -
>  	if (S_ISGITLINK(mode)) {
> -		type = OBJ_COMMIT;
> +		*type = OBJ_COMMIT;
>  	} else if (S_ISDIR(mode)) {
>  		if (show_recursive(base->buf, base->len, pathname)) {
> -			retval = READ_TREE_RECURSIVE;
> +			*retval = READ_TREE_RECURSIVE;
>  			if (!(ls_options & LS_SHOW_TREES))
> -				return retval;
> +				return 1;
>  		}
> -		type = OBJ_TREE;
> +		*type = OBJ_TREE;
>  	}
>  	else if (ls_options & LS_TREE_ONLY)
> -		return 0;
> +		return 1;
> +	return 0;
> +}

It seems that even from its returned value, the caller cannot tell
if *retval was set by the function or not.  Perhaps it makes a much
cleaner API to assign 0 to *retval at the beginning of this function,
just like the original did so anyway? ...

> +static int show_tree(const struct object_id *oid, struct strbuf *base,
> +		const char *pathname, unsigned mode, void *context)
> +{
> +	int retval = 0;

... It would mean we can lose this initialization.

> +	size_t baselen;
> +	enum object_type type = OBJ_BLOB;
> +
> +	if (show_tree_init(&type, base, pathname, mode, &retval))
> +		return retval;

>  
>  	if (!(ls_options & LS_NAME_ONLY)) {
>  		if (ls_options & LS_SHOW_SIZE) {

Teng Long Jan. 4, 2022, 9:49 a.m. UTC | #2

Junio C Hamano writes:

> Don't we need some comment that explains what the function does,
> what its return value means, etc.?
>
> It seems that even from its returned value, the caller cannot tell
> if *retval was set by the function or not.  Perhaps it makes a much
> cleaner API to assign 0 to *retval at the beginning of this function,
> just like the original did so anyway? ...

Oh, sorry for that, I did not notice the "retval" before because the
naming is unimpressive and the tests were passed, though...

I just looked at it, actually, it's important, not as what it is named, it
affects the result. The "retval" actually determine whether  to
CONTINUE reading the current "tree" or BREAK into the next
one [1] .

So, I think this commit should be modified despite the tests are passed,
firstly, I want to rename "retval" to another name that makes sense,
then just make the relevant "if" and "return" logic more clearly with the
newname, finally, it'll be consistent with the definitions in "read_tree_at()"
at "tree.c" [1].


[1] https://github.com/dyrone/git/blob/master/tree.c#L40

Thanks.

Junio C Hamano <[e-mail]> 于2022年1月4日周二 10:06写道：
>
> Teng Long <[e-mail]> writes:
>
>
> > -static int show_tree(const struct object_id *oid, struct strbuf *base,
> > -             const char *pathname, unsigned mode, void *context)
> > +static int show_tree_init(enum object_type *type, struct strbuf *base,
> > +                       const char *pathname, unsigned mode, int *retval)
>
> Don't we need some comment that explains what the function does,
> what its return value means, etc.?
>
> >  {
> > -     int retval = 0;
> > -     size_t baselen;
> > -     enum object_type type = OBJ_BLOB;
> > -
> >       if (S_ISGITLINK(mode)) {
> > -             type = OBJ_COMMIT;
> > +             *type = OBJ_COMMIT;
> >       } else if (S_ISDIR(mode)) {
> >               if (show_recursive(base->buf, base->len, pathname)) {
> > -                     retval = READ_TREE_RECURSIVE;
> > +                     *retval = READ_TREE_RECURSIVE;
> >                       if (!(ls_options & LS_SHOW_TREES))
> > -                             return retval;
> > +                             return 1;
> >               }
> > -             type = OBJ_TREE;
> > +             *type = OBJ_TREE;
> >       }
> >       else if (ls_options & LS_TREE_ONLY)
> > -             return 0;
> > +             return 1;
> > +     return 0;
> > +}
>
> It seems that even from its returned value, the caller cannot tell
> if *retval was set by the function or not.  Perhaps it makes a much
> cleaner API to assign 0 to *retval at the beginning of this function,
> just like the original did so anyway? ...
>
> > +static int show_tree(const struct object_id *oid, struct strbuf *base,
> > +             const char *pathname, unsigned mode, void *context)
> > +{
> > +     int retval = 0;
>
> ... It would mean we can lose this initialization.
>
> > +     size_t baselen;
> > +     enum object_type type = OBJ_BLOB;
> > +
> > +     if (show_tree_init(&type, base, pathname, mode, &retval))
> > +             return retval;
>
> >
> >       if (!(ls_options & LS_NAME_ONLY)) {
> >               if (ls_options & LS_SHOW_SIZE) {

Patch

diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index eecc7482d5..df8312408d 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -61,25 +61,33 @@  static int show_recursive(const char *base, size_t baselen, const char *pathname
 	return 0;
 }
 
-static int show_tree(const struct object_id *oid, struct strbuf *base,
-		const char *pathname, unsigned mode, void *context)
+static int show_tree_init(enum object_type *type, struct strbuf *base,
+			  const char *pathname, unsigned mode, int *retval)
 {
-	int retval = 0;
-	size_t baselen;
-	enum object_type type = OBJ_BLOB;
-
 	if (S_ISGITLINK(mode)) {
-		type = OBJ_COMMIT;
+		*type = OBJ_COMMIT;
 	} else if (S_ISDIR(mode)) {
 		if (show_recursive(base->buf, base->len, pathname)) {
-			retval = READ_TREE_RECURSIVE;
+			*retval = READ_TREE_RECURSIVE;
 			if (!(ls_options & LS_SHOW_TREES))
-				return retval;
+				return 1;
 		}
-		type = OBJ_TREE;
+		*type = OBJ_TREE;
 	}
 	else if (ls_options & LS_TREE_ONLY)
-		return 0;
+		return 1;
+	return 0;
+}
+
+static int show_tree(const struct object_id *oid, struct strbuf *base,
+		const char *pathname, unsigned mode, void *context)
+{
+	int retval = 0;
+	size_t baselen;
+	enum object_type type = OBJ_BLOB;
+
+	if (show_tree_init(&type, base, pathname, mode, &retval))
+		return retval;
 
 	if (!(ls_options & LS_NAME_ONLY)) {
 		if (ls_options & LS_SHOW_SIZE) {