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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 8, 2009, 23:55 UTC
Message-ID: <7v8wpl4akr.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <200901081831.22616.chriscool@tuxfamily.org>

Christian Couder <chriscool@tuxfamily.org> writes:

Show 5 quoted lines

> 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,

I actually see (2) as an improvement, and (1) as an associated cost.

Show 11 quoted lines

> 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.

You have refs/replace/$A that records $B, to tell git that the real object $A in the history is replaced by another object $B. The caller feeds $A in the above snippet to read_sha1_file(), and your read_sha1_file() notices that it needs to read $B instead, returns the buffer from the object $B, and reports its type and size. If there is no $B available, it may return NULL and the caller says "I asked for $A but in this repository I cannot get to it". That sounds consistent to me, but I agree it would be more helpful to report "and the reason why I cannot get to it is because you have replacement defined as $B which you do not have."

Show 6 quoted lines

> 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

This is debatable, but can go either way.

> 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)

That we already do anyway, don't we? parse_commit() gets data from read_sha1_file() and would complain if it gets a blob, etc.

> 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

I expect the use of graft and object replacement (or if you insist, "commit replacement") rather rare, and I think it is probably Ok to declare it a grave repository misconfiguration if somebody claims that $A is replaced by $B without actually having $B:

	void *read_sha1_file(sha1, type, size)
        {
		void *data;
        	unsigned char *replacement = lookup_replace_object(sha1);
                if (replacement) {
                	data = read_sha1_file(replacement, type, size);
                        if (!data)
                        	die("replacement %s not found for %s",
                                    get_sha1_hex(replacement),
                                    get_sha1_hex(sha1));
		} else {
			data = read_object(sha1, type, size);
		}
                ... existing code ...
                return data;                
        }

To disable replacement for connectivity walkers, lookup_replace_object() can look at a some global defined in environment.c, perhaps.