Olga Telezhnaya <olyatelezhnaya@xxxxxxxxx> 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@xxxxxxxxx> > Mentored-by: Christian Couder <christian.couder@xxxxxxxxx> > Mentored by: Jeff King <peff@xxxxxxxx> > --- > 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.
