Re: [PATCH] Transitively read alternatives

From: Junio C Hamano <junkio@cox.net>
Date: May 3, 2006, 04:28 UTC
Message-ID: <7v8xpjhguu.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <20060502073847.GK20847@admingilde.org>

Martin Waitz <tali@admingilde.org> writes:

> When adding an alternate object store then add entries from its
> info/alternates files, too.

Not quite, I'm afraid...

Show 15 quoted lines

> +static int link_alt_odb_entry(const char * entry, int len, const char * relative_base)
> +{
> +	struct stat st;
> +	const char *objdir = get_object_directory();
> +	struct alternate_object_database *ent;
> +	struct alternate_object_database *alt;
> +	/* 43 = 40-byte + 2 '/' + terminating NUL */
> +	int pfxlen = len;
> +	int entlen = pfxlen + 43;
> +	int base_len = -1;
> +
> +	if (*entry != '/' && relative_base) {
> +		/* Relative alt-odb */
> +		if (base_len < 0)
> +			base_len = strlen(relative_base) + 1;

Wouldn't base_len be always -1 here?

Show 7 quoted lines

> +	if (*entry != '/' && relative_base) {
> +		memcpy(ent->base, relative_base, base_len - 1);
> +		ent->base[base_len - 1] = '/';
> +		memcpy(ent->base + base_len, entry, len);
> +	}
> +	else
> +		memcpy(ent->base, entry, pfxlen);

Handling of full path sounds sensible; with relative_base case, the referred-to object directory is relative to our object/ directory, so "A/.git/objects/info/alternates" would typically have "../../../B/.git/objects/" if A borrows from B that lives in the same subdirectory as A itself.

> +	/* recursively add alternates */
> +	read_info_alternates(ent->base);

But using that "../../../B/.git/objects/", we would read its alternates. If it points at a neighbor we already borrow from, say C (we would refer to it as "../../../C/.git/objects/"), prefixing with relative_base would yield

	../../../B/.git/objects/../../../C/.git/objects/

and we would end up getting the same thing twice, which sounds a bit unfortunate. Maybe it is easier not to do the recursive thing unless the alternate is absolute path, and also as a purely safety measure, limit the maximum recursion depth to something low like 5, similar to recursive symlink resolution.

Hmm?