Re: [RFC/PATCH 2/3] replace_object: add mechanism to replace objects found in "refs/replace/"

Le mercredi 7 janvier 2009, Junio C Hamano a écrit :

Yeah, but read_sha1_file is called to read all object files, not just commits. So putting the hook there will:

	1) add a lookup overhead when reading any object,
	2) make it possible to replace any object,

And there is also the following problem:

	3) this function is often called like this:

	buffer = read_sha1_file(sha1, &type, &size);
	if (!buffer)
		die("Cannot read %s", sha1_to_hex(sha1));

	so in case of error, it will give an error message with a bad sha1
	in it because the sha1 of the file that we cannot read is the sha1
	in the replace ref not the one passed to read_sha1_file.

To avoid the above problems, maybe we can try to also improve what read_sha1_file does:

1) allow callers to pass a type in the "type" argument and only lookup in 
the replace refs if we say we want a commit, but this makes calling this 
function more error prone
2) when we say we want an object with a given type, check if the object we 
read has this type (and die if not)
3) die in read_sha1_file when there is an error and we are replacing so that 
callers don't need to die themself and so that we can always report an 
accurate sha1 in the error message

Something like this (totally untested):

void *read_sha1_file(const unsigned char *sha1, enum object_type *type,
		     unsigned long *size)
{
	void *data;
	enum object_type read_type;
	int replacing = (type && *type == OBJ_COMMIT);

	/* only replace commits when we ask for one */
	if (replacing)
		sha1 = lookup_replace_object(sha1);
	data = read_object(sha1, &read_type, size);
	/* legacy behavior is to die on corrupted objects */
	if (!data && (has_loose_object(sha1) || has_packed_and_bad(sha1)))
		die("object %s is corrupted", sha1_to_hex(sha1));
	if (type && *type != OBJ_NONE && *type != read_type)
		die("type of object %s (%d) is different from the type we want (%d)",
		    sha1_to_hex(sha1), read_type, *type);
	if (!data && replacing)
		die("Cannot read commit %s", sha1_to_hex(sha1));

	return data;
}

Or perhaps it is better to leave read_sha1_file as it is and add a new function like this:

void *read_commit(const unsigned char *sha1, unsigned long *size)
{
	void *data;
	enum object_type type;

	sha1 = lookup_replace_object(sha1);
	data = read_sha1_file(sha1, &type, size);

	if (*type != OBJ_COMMIT)
		die("read_commit: object %s is not a commit", sha1_to_hex(sha1));
	if (!data)
		die("read_commit: cannot read commit %s", sha1_to_hex(sha1));

	return data;
}

The latter seems better to me. But I haven't looked much at the 60 read_sha1_file callers yet.

[...]

Yeah, I agree there are drawbacks to do it at parse_commit_buffer time.

Thanks, Christian.