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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 21, 2017, 18:15 UTC
Message-ID: <xmqq8tklqkbe.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <fb656dfeb96e3c69f612fed924d0d9dec592c772.1497920092.git.jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

Show 7 quoted lines

> 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@google.com>

I think the motivation is sound, but...

Show 15 quoted lines

> 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).

Show 12 quoted lines

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