Re: [PATCH v1 1/2] entry.c: update cache entry only for existing files

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 5, 2017, 23:01 UTC
Message-ID: <xmqqefqh6vxf.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20171005112658.p7hohhtkdkcapwe6@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 18 quoted lines

> On Thu, Oct 05, 2017 at 08:19:13PM +0900, Junio C Hamano wrote:
>
>> This is unrelated to the main topic of this patch, but we see this
>> just before the precontext of this hunk:
>> 
>> 			if (dco && dco->state != CE_NO_DELAY) {
>> 				/* Do not send the blob in case of a retry. */
>> 				if (dco->state == CE_RETRY) {
>> 					new = NULL;
>> 					size = 0;
>> 				}
>> 				ret = async_convert_to_working_tree(
>> 					ce->name, new, size, &buf, dco);
>> 
>> Aren't we leaking "new" in that CE_RETRY case?
>
> Yes, it certainly looks like it. Wouldn't we want to avoid reading the
> file from disk entirely in that case?

Probably.  But that is more of a removal of pessimization than a fix ;-)

> I.e., I think free(new) is sufficient to fix the leak you
> mentioned.

In addition to keeping the new = NULL assignment, of course.

> But
> I think we'd want to protect the read_blob_entry() call at the top of
> the case with a check for dco->state == CE_RETRY.

Yeah, I think that makes more sense.

A patch may look like this on top of these two patches, but I'd prefer to see Lars's eyeballing and possibly wrapping it up in an applicable patch after taking the authorship.

I considered initializing new to NULL and size to 0 but decided against it, as that would lose the justification to have an if statement that marks that "dco->state == CE_RETRY" is a special case. I think explicit if() with clearing these two variables makes it clearer to show what is going on.

By the way, the S_IFLNK handling seems iffy with or without this change (or for that matter, I suspect this iffy-ness existed before Lars's delayed filtering change). On a platform without symlinks, we do the same as S_IFREG, but obviously we do not want any content conversion that happens to the regular files in such a case. So we may further want to fix that, but I left it outside the scope of fixing the leak of NULL and optimizing the blob reading out.

 entry.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/entry.c b/entry.c
index cac5bf5af2..74e35f942c 100644
--- a/entry.c
+++ b/entry.c
@@ -274,14 +274,12 @@ static int write_entry(struct cache_entry *ce,
 	}
 
 	switch (ce_mode_s_ifmt) {
-	case S_IFREG:
 	case S_IFLNK:
 		new = read_blob_entry(ce, &size);
 		if (!new)
 			return error("unable to read sha1 file of %s (%s)",
 				path, oid_to_hex(&ce->oid));
-
-		if (ce_mode_s_ifmt == S_IFLNK && has_symlinks && !to_tempfile) {
+		if (has_symlinks && !to_tempfile) {
 			ret = symlink(new, path);
 			free(new);
 			if (ret)
@@ -289,18 +287,28 @@ static int write_entry(struct cache_entry *ce,
 						   path);
 			break;
 		}
-
+		/* fallthru */
+	case S_IFREG:
 		/*
 		 * Convert from git internal format to working tree format
 		 */
 		if (ce_mode_s_ifmt == S_IFREG) {
 			struct delayed_checkout *dco = state->delayed_checkout;
+
+			/* 
+			 * In case of a retry, we do not send blob, hence no
+			 * need to read it, either.
+			 */
+			if (dco && dco->state == CE_RETRY) {
+				new = NULL;
+				size = 0;
+			} else {
+				new = read_blob_entry(ce, &size);
+				if (!new)
+					return error("unable to read sha1 file of %s (%s)",
+						     path, oid_to_hex(&ce->oid));
+			}
 			if (dco && dco->state != CE_NO_DELAY) {
-				/* Do not send the blob in case of a retry. */
-				if (dco->state == CE_RETRY) {
-					new = NULL;
-					size = 0;
-				}
 				ret = async_convert_to_working_tree(
 					ce->name, new, size, &buf, dco);
 				if (ret && string_list_has_string(&dco->paths, ce->name)) {