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, 08:42 UTC
- Message-ID
- <7v4oa8esy4.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <20101220044214.GA5942@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 18 quoted lines
> 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;