Re: [RFC PATCH 4/6] ls-tree: improving cohension in the print code

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Nov 17 2022, Teng Long wrote:

> From: Teng Long <dyroneteng@gmail.com>
>
> We use 'show_tree_long()' and 'show_tree_default()' to print entries
> when we want the output in 'default' or 'default long' formats.
>
> Function 'show_tree_common_default_long()' prints the "path" field as
> the last part of output, the previous part which separately implements
> in 'show_tree_long()' and 'show_tree_default()'.
>
> We can package the separate implementation together by the extension of
> "size_text" in struct "show_tree_data". By improving the cohesion in
> these two locations, some benefits such as uniform processing of the
> output can be achieved in future.
>
> Signed-off-by: Teng Long <dyroneteng@gmail.com>
> ---
>  builtin/ls-tree.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
> index afb65af4280..7661170f7ca 100644
> --- a/builtin/ls-tree.c
> +++ b/builtin/ls-tree.c
> @@ -30,6 +30,7 @@ struct show_tree_data {
>  	const struct object_id *oid;
>  	const char *pathname;
>  	struct strbuf *base;
> +	char *size_text;
>  };
>  
>  static const char * const ls_tree_usage[] = {
> @@ -186,6 +187,7 @@ static int show_tree_common(struct show_tree_data *data, int *recurse,
>  	data->oid = oid;
>  	data->pathname = pathname;
>  	data->base = base;
> +	data->size_text = NULL;
>  
>  	if (type == OBJ_BLOB) {
>  		if (ls_options & LS_TREE_ONLY)
> @@ -204,6 +206,13 @@ static void show_tree_common_default_long(struct show_tree_data *data)
>  {
>  	int base_len = data->base->len;
>  
> +	if (data->size_text)
> +		printf("%06o %s %s %7s\t", data->mode, type_name(data->type),
> +		       find_unique_abbrev(data->oid, abbrev), data->size_text);
> +	else
> +		printf("%06o %s %s\t", data->mode, type_name(data->type),
> +		       find_unique_abbrev(data->oid, abbrev));
> +
>  	strbuf_addstr(data->base, data->pathname);
>  	write_name_quoted_relative(data->base->buf,
>  				   chomp_prefix ? ls_tree_prefix : NULL, stdout,
> @@ -223,8 +232,6 @@ static int show_tree_default(const struct object_id *oid, struct strbuf *base,
>  	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(&data);
>  	return recurse;
>  }
> @@ -253,8 +260,7 @@ static int show_tree_long(const struct object_id *oid, struct strbuf *base,
>  		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);
> +	data.size_text = size_text;
>  	show_tree_common_default_long(&data);
>  	return recurse;
>  }

I think this is a good example of how not to split up commits.

So, in this case the reader is left wondering what the point is, how
does having two callers, and introducing an exra parameter to flip
between what one or the other wants make sense for a "common" function?

It doesn't, that code should just belong in those callers, so this is
just making things more indirect.

We find out later in the series that the real reason is that this
eventually becomes an append to a "struct strbuf".

At that point we need to re-write these lines again, so in terms of
reviewers having to look at it they'd wonder here, and then look a that
code again...

This is better just either squashed into a 6/6, or in an explicit
"here's some refactoring to make a subsequent change smaller", which
e.g. could move to using the strbuf already, which we'd later make "real" use of.