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

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

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

> 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");
> +