Re: [PATCH 1/2] index-pack: Create .keep files with same permissions and .pack/.idx

From: Johan Herland <johan@herland.net>
Date: Apr 1, 2011, 23:37 UTC
Message-ID: <201104020137.28951.johan@herland.net>
In-Reply-To: <AANLkTimMJxni=Vuja+iHyMp8ydm7ZBvWnkSe68g6+pyH@mail.gmail.com>

On Friday 01 April 2011, Shawn Pearce wrote:

Show 10 quoted lines

> The only problem is a cpio based clone, which may link the objects
> directory before the refs, and miss linking the new pack but wind up
> linking the new ref, making the clone corrupt. But that is a bug in
> the cpio clone implementation. Using file:// to use the classical pipe
> is safe here, because the refs are scanned before the objects are.
> IMHO, if you think clone during push is unsafe because of this, we
> should fix the cpio clone path to do a `git ls-remote` on the source,
> cache the refs in memory, copy the objects, then write out a
> packed-refs file containing the refs we snapshotted *before* linking
> the objects directory into the new clone.

Looking at clone_local() in builtin/clone.c (which I guess is the culprit here), wouldn't we fix this simply by swapping the two parts of the function, so that the refs part is done before the objects part? Like this:

static const struct ref *clone_local(const char *src_repo,
				     const char *dest_repo)
{
	const struct ref *ret;
	struct strbuf src = STRBUF_INIT;
	struct strbuf dest = STRBUF_INIT;
	struct remote *remote;
	struct transport *transport;

	remote = remote_get(src_repo);
	transport = transport_get(remote, src_repo);
	ret = transport_get_remote_refs(transport);
	transport_disconnect(transport);

	if (option_shared)
		add_to_alternates_file(src_repo);
	else {
		strbuf_addf(&src, "%s/objects", src_repo);
		strbuf_addf(&dest, "%s/objects", dest_repo);
		copy_or_link_directory(&src, &dest);
		strbuf_release(&src);
		strbuf_release(&dest);
	}

	if (0 <= option_verbosity)
		printf("done.\n");
	return ret;
}

Have fun! :)

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net