On Mon, Dec 20 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes: > >> Add a convenience function to wrap the xsnprintf() command that >> generates loose object headers. This code was copy/pasted in various >> parts of the codebase, let's define it in one place and re-use it from >> there. >> ... >> +/** >> + * format_loose_header() is a thin wrapper around s xsnprintf() that > > The name should have "object" somewhere in it. Not all readers can > be expected to know that you meant "loose" to be an acceptable short > hand for "loose object". *nod* > That nit aside, I think it is a good idea to give people a common > helper function to call. I am undecided if it is a good idea to > make it take enum or "const char *"; most everybody should be able > to say > > format_object_header(type_name(OBJ_COMMIT), ...) > > just fine, so two variants might be overkill, just to allow > > format_object_header(OBJ_COMMIT, ...) > > and to forbid > > format_object_header("connit", ...) > > I dunno. Ultimately only a single API caller in hash-object.c really cares about something else than the enum. I've got some patches locally to convert e.g. write_object_file() to use the enum, and it removes the need for some callers to convert enum to char *, only to have other things convert it back. So I think for any new APIs it makes sense to work towards sidelining the hash-object.c --literally caller.