Re: [PATCH v14 15/15] ls-tree: split up "fast path" callbacks

[Josh Steadmon <steadmon@google.com>]

On 2022.03.23 17:13, Teng Long wrote:
> From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> 
> Make the various if/else in the callbacks for the "fast path" a lot
> easier to read by just using common functions for the parts that are
> common, and have per-format callbacks for those parts that are
> different.
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: Teng Long <dyroneteng@gmail.com>
> ---
>  builtin/ls-tree.c | 199 +++++++++++++++++++++++++++++-----------------
>  1 file changed, 125 insertions(+), 74 deletions(-)
> 
> diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
> index 6550f27dfe..44a91cf9d0 100644
> --- a/builtin/ls-tree.c
> +++ b/builtin/ls-tree.c
> @@ -173,108 +173,157 @@ static int show_tree_fmt(const struct object_id *oid, struct strbuf *base,
>  	return recurse;
>  }
>  
> -static int show_default(struct show_tree_data *data)
> +static int show_tree_common(struct show_tree_data *data, int *recurse,
> +			    const struct object_id *oid, struct strbuf *base,
> +			    const char *pathname, unsigned mode)
>  {
> -	size_t baselen = data->base->len;
> -
> -	if (cmdmode == MODE_LONG) {
> -		char size_text[24];
> -		if (data->type == OBJ_BLOB) {
> -			unsigned long size;
> -			if (oid_object_info(the_repository, data->oid, &size) == OBJ_BAD)
> -				xsnprintf(size_text, sizeof(size_text), "BAD");
> -			else
> -				xsnprintf(size_text, sizeof(size_text),
> -					  "%" PRIuMAX, (uintmax_t)size);
> -		} else {
> -			xsnprintf(size_text, sizeof(size_text), "-");
> -		}
> -		printf("%06o %s %s %7s\t", data->mode, type_name(data->type),
> -		find_unique_abbrev(data->oid, abbrev), size_text);
> -	} else {
> -		printf("%06o %s %s\t", data->mode, type_name(data->type),
> -		find_unique_abbrev(data->oid, abbrev));
> -	}
> -	baselen = data->base->len;
> -	strbuf_addstr(data->base, data->pathname);
> -	write_name_quoted_relative(data->base->buf,
> -				   chomp_prefix ? ls_tree_prefix : NULL, stdout,
> -				   line_termination);
> -	strbuf_setlen(data->base, baselen);
> -	return 1;
> -}
> -
> -static int show_tree(const struct object_id *oid, struct strbuf *base,
> -		const char *pathname, unsigned mode, void *context)
> -{
> -	int recurse = 0;
> -	size_t baselen;
>  	enum object_type type = object_type(mode);
> -	struct show_tree_data data = {
> -		.mode = mode,
> -		.type = type,
> -		.oid = oid,
> -		.pathname = pathname,
> -		.base = base,
> -	};
> +	int ret = -1;
> +
> +	*recurse = 0;
> +	data->mode = mode;
> +	data->type = type;
> +	data->oid = oid;
> +	data->pathname = pathname;
> +	data->base = base;
>  
>  	if (type == OBJ_BLOB) {
>  		if (ls_options & LS_TREE_ONLY)
> -			return 0;
> +			ret = 0;
>  	} else if (type == OBJ_TREE &&
>  		   show_recursive(base->buf, base->len, pathname)) {
> -		recurse = READ_TREE_RECURSIVE;
> +		*recurse = READ_TREE_RECURSIVE;
>  		if (!(ls_options & LS_SHOW_TREES))
> -			return recurse;
> +			ret = *recurse;
>  	}
>  
> -	if (cmdmode == MODE_OBJECT_ONLY) {
> -		printf("%s%c", find_unique_abbrev(oid, abbrev), line_termination);
> -		return recurse;
> -	}
> +	return ret;
> +}
>  
> -	if (cmdmode == MODE_NAME_ONLY) {
> -		baselen = base->len;
> -		strbuf_addstr(base, pathname);
> -		write_name_quoted_relative(base->buf,
> -					   chomp_prefix ? ls_tree_prefix : NULL,
> -					   stdout, line_termination);
> -		strbuf_setlen(base, baselen);
> -		return recurse;
> +static void show_tree_common_default_long(struct strbuf *base,
> +					  const char *pathname,
> +					  const size_t baselen)
> +{
> +	strbuf_addstr(base, pathname);
> +	write_name_quoted_relative(base->buf,
> +				   chomp_prefix ? ls_tree_prefix : NULL, stdout,
> +				   line_termination);
> +	strbuf_setlen(base, baselen);
> +}
> +
> +static int show_tree_default(const struct object_id *oid, struct strbuf *base,
> +			     const char *pathname, unsigned mode,
> +			     void *context)
> +{
> +	int early;
> +	int recurse;
> +	struct show_tree_data data = { 0 };
> +
> +	early = show_tree_common(&data, &recurse, oid, base, pathname, mode);
> +	if (early >= 0)
> +		return early;
> +
> +	printf("%06o %s %s\t", data.mode, type_name(data.type),
> +	       find_unique_abbrev(data.oid, abbrev));
> +	show_tree_common_default_long(base, pathname, data.base->len);
> +	return recurse;
> +}
> +
> +static int show_tree_long(const struct object_id *oid, struct strbuf *base,
> +			  const char *pathname, unsigned mode, void *context)
> +{
> +	int early;
> +	int recurse;
> +	struct show_tree_data data = { 0 };
> +	char size_text[24];
> +
> +	early = show_tree_common(&data, &recurse, oid, base, pathname, mode);
> +	if (early >= 0)
> +		return early;
> +
> +	if (data.type == OBJ_BLOB) {
> +		unsigned long size;
> +		if (oid_object_info(the_repository, data.oid, &size) == OBJ_BAD)
> +			xsnprintf(size_text, sizeof(size_text), "BAD");
> +		else
> +			xsnprintf(size_text, sizeof(size_text),
> +				  "%" PRIuMAX, (uintmax_t)size);
> +	} else {
> +		xsnprintf(size_text, sizeof(size_text), "-");
>  	}
>  
> -	if (cmdmode == MODE_LONG ||
> -		(!ls_options || (ls_options & LS_RECURSIVE)
> -		 || (ls_options & LS_SHOW_TREES)
> -		 || (ls_options & LS_TREE_ONLY)))
> -			 show_default(&data);
> +	printf("%06o %s %s %7s\t", data.mode, type_name(data.type),
> +	       find_unique_abbrev(data.oid, abbrev), size_text);
> +	show_tree_common_default_long(base, pathname, data.base->len);
> +	return 1;
> +}

This commit makes `git ls-tree -l ...` list files recursively,
regardless of whether `-r` was provided or not. I believe it's due to
show_tree_long() returning 1 instead of the value of recurse. Is there a
reason why we unconditionally return 1 here?