Re: [PATCH RFC 03/24] cat-file: split expand_atom into 2 functions

[Junio C Hamano <gitster@pobox.com>]

Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:

> Split expand_atom function into 2 different functions,
> expand_atom_into_fields prepares variable for further filling,
> (new) expand_atom creates resulting string.
> Need that for further reusing of formatting logic from ref-filter.
>
> Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored by: Jeff King <peff@peff.net>
> ---
>  builtin/cat-file.c | 73 +++++++++++++++++++++++++++++-------------------------
>  1 file changed, 39 insertions(+), 34 deletions(-)

As expand_atom() is file-scope static and its callers are well
isolated, it is OK to change its meaning while restructuring the
code like this patch does (as opposed to a public function to which
new callers may be added on other topics in flight).

The split itself looks sensible, but expand_atom_into_fields() is a
questionable name.  expand_atom() does fill the data in sb, but
calling expand_atom_into_fields() does not fill any data into
separated fields---it merely prepares somebody else to do so.

Helped by this comment:

	/*
	 * If mark_query is true, we do not expand anything, but rather
	 * just mark the object_info with items we wish to query.
	 */
	int mark_query;

we can guess that a better name would mention or hint "object_info",
"query" and probably "prepare" (because we would do so before
actually querying).

I am not sure if separating the logic into these two functions is a
good way to organize things.  When a new %(atom) is introduced, it
is more likely that a programmer adds it to one but forgets to make
a matching change to the other, no?  (here, "I am not sure" is just
that.  It is very different from "I am sure this is wrong").

Thanks.