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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 4, 2022, 02:06 UTC
Message-ID: <xmqqwnjgfe4t.fsf@gitster.g>
In-Reply-To: <d77c895a4b9d5cb86443279c0156196254895ef4.1641043500.git.dyroneteng@gmail.com>

Teng Long <dyroneteng@gmail.com> 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.?

Show 24 quoted lines

>  {
> -	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.

Show 5 quoted lines

> +	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) {