Re: [PATCH 14/17] sha1_file: introduce an nth_packed_object_oid function

On Mon, Jan 02, 2017 at 07:18:58PM +0100, Michael Haggerty wrote:

quoted

Given that this patch only converts one caller, I'd be more inclined to
just have the caller do its own hashcpy. Like:

diff --git a/sha1_file.c b/sha1_file.c
index 1173071859..16345688b5 100644
--- a/sha1_file.c
+++ b/sha1_file.c

@@ -3769,12 +3769,14 @@ static int for_each_object_in_pack(struct packed_git *p, each_packed_object_fn c
 
 	for (i = 0; i < p->num_objects; i++) {
 		const unsigned char *sha1 = nth_packed_object_sha1(p, i);
+		struct object_id oid;
 
 		if (!sha1)
 			return error("unable to get sha1 of object %u in %s",
 				     i, p->pack_name);
 
-		r = cb(sha1, p, i, data);
+		hashcpy(oid.hash, sha1);
+		r = cb(&oid, p, i, data);
 		if (r)
 			break;
 	}

That punts on the issue for all the other callers, but like I said, I'm
not quite sure if, when, and how it would make sense to convert them to
using a "struct oid".

Your change is not safe if any of the callback functions ("cb") tuck
away a copy of the pointer that they are passed, expecting it to contain
the same object id later.

I think it's generally a given that callback functions should not assume
the lifetime of parameters extend beyond the end of the callback. That
said, I didn't audit the callers (although I'm pretty sure I wrote all
of them myself in this particular case).

-Peff