Re: [PATCH 2/2] fill_textconv(): Don't get/put cache if sha1 is not valid

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 20, 2010, 02:26 UTC
Message-ID: <7vr5dddvrk.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20101218161337.GB18643@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 6 quoted lines

> PS It is a little disturbing that in fill_textconv, we handle
> case (1), !DIFF_FILE_VALID for the non-textconv case, but not so for the
> textconv case. I think we are OK, as get_textconv will never load a
> textconv driver for a !DIFF_FILE_VALID filespec, so we always follow the
> non-textconv codepath in that case. But I am tempted to do this just to
> be more defensive:

FILE_VALID() is about "does that side have a blob there, or is this create/delete diff?", so the caller should be handling this properly as you said, but your fill_textconv() already prepares for the case where the caller for some reason calls this function with "no blob on this side" and returns an empty string (see the precontext of your patch).

I think it is fine to be defensive to prepare for such a case, but then dying like this patch does is inconsistent. Perhaps we should move the new check higher and remove the *outbuf = "" while at it?

Show 19 quoted lines

> diff --git a/diff.c b/diff.c
> index b0ee213..5320849 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -4404,22 +4404,25 @@ size_t fill_textconv(struct userdiff_driver *driver,
>  	if (!driver || !driver->textconv) {
>  		if (!DIFF_FILE_VALID(df)) {
>  			*outbuf = "";
>  			return 0;
>  		}
>  		if (diff_populate_filespec(df, 0))
>  			die("unable to read files to diff");
>  		*outbuf = df->data;
>  		return df->size;
>  	}
>  
> +	if (!DIFF_FILE_VALID(df))
> +		die("BUG: attempt to textconv an invalid filespec");
> +