Re: [PATCH 46/67] write_loose_object: convert to strbuf

Jeff King [off-list ref] writes:

-	memcpy(buffer, filename, dirlen);
-	strcpy(buffer + dirlen, "tmp_obj_XXXXXX");
-	fd = git_mkstemp_mode(buffer, 0444);
+	strbuf_reset(tmp);
+	strbuf_add(tmp, filename, dirlen);
+	strbuf_addstr(tmp, "tmp_obj_XXXXXX");
+	fd = git_mkstemp_mode(tmp->buf, 0444);
 	if (fd < 0 && dirlen && errno == ENOENT) {
-		/* Make sure the directory exists */
-		memcpy(buffer, filename, dirlen);
-		buffer[dirlen-1] = 0;
-		if (mkdir(buffer, 0777) && errno != EEXIST)
+		/*
+		 * Make sure the directory exists; note that mkstemp will have
+		 * put a NUL in our buffer, so we have to rewrite the path,
+		 * rather than just chomping the length.
+		 */
+		strbuf_reset(tmp);
+		strbuf_add(tmp, filename, dirlen - 1);
+		if (mkdir(tmp->buf, 0777) && errno != EEXIST)
 			return -1;

I had to read the patch three times before understanding what the
business with NUL in this comment is about.

The old code was doing the same thing, i.e. instead of attempting to
reuse the early part of buffer[] it copied the early part of
filename[] there again, exactly for the same reason, but it didn't
even explain why the copy was necessary.  Now the new code explains
why strbuf_setlen() is not used here pretty nicely.

An unsuccessful call to git_mkstemp_mode() would destroy the
template buffer by placing a NUL at the beginning of it (and I was
confused because I did not read the unwritten "at the beginning" in
"put a NUL in our buffer" above).

The patch looks good.  Thanks.