Christian Couder <chriscool@xxxxxxxxxxxxx> writes:
> 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.
> 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."
> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
