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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Jun 26, 2017, 16:28 UTC
Message-ID: <CAGf8dg+9OzDoZW4NaNXi_JJSQhSJZabDHkf_3rSisvuY4HhtAg@mail.gmail.com>
In-Reply-To: <20170624203927.r35esm7nvcqnarlr@sigill.intra.peff.net>

On Sat, Jun 24, 2017 at 1:39 PM, Jeff King <peff@peff.net> wrote:

Show 10 quoted lines

> 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/