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:

> On Sun, Dec 19, 2010 at 06:26:55PM -0800, Junio C Hamano wrote:
> ...
>> 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?
>
> I'm not sure returning the empty string for a textconv is the right
> solution....
>
> So I stand by my thought that it should die(). But I don't think there
> _are_ any such bugs currently, so it probably doesn't matter much either
> way. I can live with "return 0", or even just leaving it alone.

I must have phrased it badly.  I am actually Ok either way (i.e. make this
function prepare for a future when we start passing the missing side to
the function, and have a special case for "if (!DIFF_FILE_VALID)" and
returning something like an empty string, or make this function refuse to
be fed the missing side by dying in "if (!DIFF_FILE_VALID)".  I was only
pointing out that the result of applying your patch does one in one case
and another in the other case, which is inconsistent.  And we do not know
in advance what is the reasonable fallback value for the missing side, so
we do not now "something like an empty string" is a reasonable thing to do
yet.  Hence "move the new check higher and remove ..." was my suggestion,
which would look like the attached, which would be consistent with your
message I am replying to.  IOW, I think we are on the same page.

 diff.c |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/diff.c b/diff.c
index 5422c43..a04ab2f 100644
--- a/diff.c
+++ b/diff.c
@@ -4401,11 +4401,18 @@ size_t fill_textconv(struct userdiff_driver *driver,
 {
 	size_t size;
 
+	/*
+	 * !DIFF_FILE_VALID(df) means this is a missing side of the
+	 * diff (preimage of creation, or postimage of deletion diff).
+	 * The caller should not try to textconv such a filespec, as
+	 * there is no such blob to begin with!
+	 */
+	
+	if (!DIFF_FILE_VALID(df))
+		die("Feeding missing side to fill_textconv?: '%s'",
+		    df->path);
+
 	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;