Subject: Re: [PATCH v2 05/10] {commit,tree,blob,tag}.c: add a create_{commit,tree,blob,tag}()

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Thu, 29 Apr 2021 14:01:47 +0200

On Thu, Apr 29 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <[email-masked]> writes:
>
>> Add a create_*() function for our built-in types as a handy but
>> trivial wrapper around their calls to create_object().
>>
>> This allows for slightly simplifying code added in
>> 96af91d410c (commit-graph: verify objects exist, 2018-06-27). The
>> remaining three functions are added for consistency for now.
>
> "for now" puzzles me.  As file-scope static functions, they do not
> hurt all that much, but on the other hand, having to say
> "create_object(r, oid, alloc_blob_node(r))" is not hurting at all.
>
> The worst part of this "consistency" is that callers cannot call
> create_blob() because it is not external, even though they learn
> create_commit() as a handy way to use the create_object() API, which
> is not consistent at all.
>
> And since most callers should be calling lookup_blob() etc., and
> should not be calling create_blob(), we shouldn't tempt people to
> push for making them externally available.

The API is for our own internal use. So I figured it was better to leave
the ones that aren't used elsewhere "static" for now, and if anyone
needed them in the future that commit could remove the "static".

> Which in turn makes me wonder if the use of create_object() added to
> the commit-graph.c was a good idea to begin with.

Yes we could just drop this and inline the various "alloc", i.e. not
this & similar in the future:

-		odb_commit = (struct commit *)create_object(r, &cur_oid, alloc_commit_node(r));
+		odb_commit = create_commit(r, &cur_oid);

It just seemed like a net improvement for maintenance/readability to
have the simpler wrapper for the allocation / object creation v.s. the
existing alloc_X_node() + cast.