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

From: Junio C Hamano <[email-masked]>
Date: Mon, 03 Jan 2022 18:06:42 -0800

Teng Long <[email-masked]> 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) {