Jonathan Tan <jonathantanmy@xxxxxxxxxx> writes:
> Add an option to struct object_info to suppress population of additional
> information about a packed object if unneeded. This allows an
> optimization in which sha1_object_info_extended() does not even need to
> access the pack if no information besides provenance is requested. A
> subsequent patch will make use of this optimization.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@xxxxxxxxxx>
I think the motivation is sound, but...
> diff --git a/sha1_file.c b/sha1_file.c
> index 24f7a146e..68e3a3400 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -3020,6 +3020,13 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi,
> }
> }
>
> + if (!oi->typep && !oi->sizep && !oi->disk_sizep &&
> + !oi->delta_base_sha1 && !oi->typename && !oi->contentp &&
> + !oi->populate_u) {
> + oi->whence = OI_PACKED;
> + return 0;
> + }
> +
... this "if" statement feels like a maintenance nightmare. The
intent of the guard, I think, is "when the call wants absolutely
nothing but whence", but the implementation of the guard will not
stay true to the intent whenever somebody adds a new field to oi.
I wonder if it makes more sense to have a new field "whence_only",
which is set only by such a specialized caller, which this guard
checks (and no other fields).
> diff --git a/streaming.c b/streaming.c
> index 9afa66b8b..deebc18a8 100644
> --- a/streaming.c
> +++ b/streaming.c
> @@ -113,6 +113,7 @@ static enum input_source istream_source(const unsigned char *sha1,
>
> oi->typep = type;
> oi->sizep = &size;
> + oi->populate_u = 1;
> status = sha1_object_info_extended(sha1, oi, 0);
> if (status < 0)
> return stream_error;
By the way, populate_u feels very misnamed. Even if that union
gains details about other types of representations, your caller that
flips populate_u would not care about them. This bit is about
learning even more detail about a packed object, so a name with
"packed" somewhere would be more appropriate, I would think.
