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

[Christian Couder <chriscool@tuxfamily.org>]

Le mercredi 7 janvier 2009, Junio C Hamano a écrit :
> Christian Couder <chriscool@tuxfamily.org> writes:
> > In "parse_commit_buffer", the parent sha1s from the original commit
> > or from a commit graft that match a ref name in "refs/replace/" are
> > replaced by the commit sha1 that has been read in the ref.
>
> I may be reading this wrong, but if you replace the parent of the commit,
> that won't improve anything over the graft mechanism, which we already
> have.
>
> What I was hoping to see was to give replacing commit back when a commit
> is asked for by normal callers, 

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.

[...]

> Doing replacement at parse_commit_buffer() time is one step too late. 
> For example, if you have replacement information for the commit that sits
> at the tip of 'master' branch, I think your "git log master" will ignore
> that replacement information.  Creating one new commit on top of it and
> saying "git log master" then will show the new commit, and suddenly
> starts showing the replacement commit for the one you used to have at the
> tip of the branch.

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

Thanks,
Christian.