2018-01-27 0:46 GMT+03:00 Junio C Hamano <gits...@pobox.com>: > 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).
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.
