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

> 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").