Re: [PATCH v4 7/8] sha1_file: do not access pack if unneeded

[Jonathan Tan <jonathantanmy@google.com>]

On Sat, Jun 24, 2017 at 1:39 PM, Jeff King <peff@peff.net> wrote:
> On Sat, Jun 24, 2017 at 11:41:39AM -0700, Junio C Hamano wrote:
> If we are open to writing anything, then I think it should follow the
> same pointer-to-data pattern that the rest of the struct does. I.e.,
> declare the extra pack-data struct as a pointer, and let callers fill it
> in or not. It's excessive in the sense that it's not a variable-sized
> answer, but it at least makes the interface consistent.
>
> And no callers who read it would be silently broken; the actual data
> type in "struct object_info" would change, so they'd get a noisy compile
> error.

I considered that, but there was some trickiness in streaming.c -
open_istream() would need to establish that pointer even though that
is not its responsibility, or istream_source would need to
heap-allocate some memory then point to it from `oi` (it has to be
heap-allocated since the memory must outlive the function).

Also, it does not solve the "maintenance nightmare" issue that Junio
described (in that in order to optimize the pack read away, we would
need a big "if" statement).

Those issues are probably surmountable, but in the end I settled on
just allowing the caller to pass NULL and having
sha1_object_info_extended() substitute an empty struct when that
happens, as you can see in v5 [2]. That allows most of
sha1_object_info_extended() to not have to handle NULL, but also
allows for the specific optimization (optimizing the pack read away)
that I want.

[1] https://public-inbox.org/git/xmqq8tklqkbe.fsf@gitster.mtv.corp.google.com/
[2] https://public-inbox.org/git/ddbbc86204c131c83b3a1ff3b52789be9ed2a5b1.1498091579.git.jonathantanmy@google.com/