2018-01-27 0:46 GMT+03:00 Junio C Hamano <gitster@xxxxxxxxx>: > 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). OK, I will rename that function. Actually, not sure that we really need to have ideal name here because both functions will be deleted by the end of this patch. "mark_atom_in_object_info"? > > 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"). In the end of the patch we don't have both of those functions, so there would not be such problem. But, I need that split, it helps me to go further and apply new changes step-by-step. > > Thanks.
