Olga Telezhnaya <olyatelezhn...@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 <olyatelezhn...@gmail.com>
> Mentored-by: Christian Couder <christian.cou...@gmail.com>
> Mentored by: Jeff King <p...@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.
