Re: [PATCH 14/17] sha1_file: introduce an nth_packed_object_oid function

On 01/01/2017 08:18 PM, brian m. carlson wrote:

There are places in the code where we would like to provide a struct
object_id *, yet read the hash directly from the pack.  Provide an
nth_packed_object_oid function that mirrors the nth_packed_object_sha1
function.

The required cast is questionable, but should be safe on all known
platforms.  The alternative of allocating an object as an intermediate
would be too inefficient and cause memory leaks.  If necessary, an
intermediate union could be used; this practice is allowed by GCC and
explicitly sanctioned by C11.  However, such a change will likely not be
necessary, and can be made if and when it is.

I have the feeling that this design decision has been discussed on the
mailing list. If so, could you include a URL here?

The obvious alternative to allocating a new object to return to the
caller would be to have the caller of `nth_packed_object_oid()` pass a
`struct object_id *` argument to be filled in (by copying the hash into
it). Aside from being legal C, this would also be a more robust step
towards a post-SHA-1 world, where the suggested hack wouldn't work.

Of course, the question is what the callers want to do with the
`object_id`. Are the return values of `nth_packed_object_sha1()` stored
to other longer-lived structures that rely on the lifetime of the
packfile mmap to keep the value valid? If so, then keeping track of the
lifetime of the `struct object_id` could be a big chore, not to mention
that needing to keep a 20-byte `struct object_id` around rather than a
8- or 4-byte pointer would also cost more RAM.

As you probably can tell, I'm not psyched about straying outside of
legal C. It would be nice to see more justification.

[...]

Michael