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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 26, 2018, 21:46 UTC
Message-ID: <xmqqvafo5m5h.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <0102016133ff3afb-6bab2c11-e0f3-4bef-9626-fbcc826bcb15-000000@eu-west-1.amazonses.com>

Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:

Show 11 quoted lines

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