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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

> Ævar Arnfjörð Bjarmason  <avarab@xxxxxxxxx> 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.




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux