Re: [PATCH 2/2] fill_textconv(): Don't get/put cache if sha1 is not valid
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:50:16
Jeff King [off-list ref] 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?
quoted hunk
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"); +