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

Johan Herland [off-list ref] writes:

While pushing to a remote repo, Git transiently adds a .keep file for the
pack being pushed, to protect it from a concurrent "git gc". However, the
permissions on this .keep file are such that if a different user attempts
a local cross-filesystem clone ("git clone --no-hardlinks") on the server
while the .keep file is present (either because of a concurrent push, or
because of a prior failed push that left a stale .keep file), the clone
will fail because the second user cannot access the .keep file created by
the first user.

While I am not sure if letting a clone proceed while there is a concurrent
push is even a good idea to begin with, I agree that a stale .keep file is
a problem.

I am kind of surprised that we are not using atexit(3) to clean them just
like we do for lockfiles; wouldn't that be a better solution?

On Fri, Apr 01, 2011 at 02:39:21PM -0700, Junio C Hamano wrote:

Johan Herland [off-list ref] writes:

quoted

While pushing to a remote repo, Git transiently adds a .keep file for the
pack being pushed, to protect it from a concurrent "git gc". However, the
permissions on this .keep file are such that if a different user attempts
a local cross-filesystem clone ("git clone --no-hardlinks") on the server
while the .keep file is present (either because of a concurrent push, or
because of a prior failed push that left a stale .keep file), the clone
will fail because the second user cannot access the .keep file created by
the first user.

While I am not sure if letting a clone proceed while there is a concurrent
push is even a good idea to begin with, I agree that a stale .keep file is
a problem.

I am kind of surprised that we are not using atexit(3) to clean them just
like we do for lockfiles; wouldn't that be a better solution?

We definitely should do that, but it would also be nice if a power
failure, kill -9, or segfault in receive-pack didn't leave a repo
unusable.

-Peff

On Fri, Apr 1, 2011 at 17:39, Junio C Hamano [off-list ref] wrote:

While I am not sure if letting a clone proceed while there is a concurrent
push is even a good idea to begin with,

What? Why?

Are you suggesting that Git hosting sites disable readers while there
is a push occurring?

We have tried hard to design Git to be concurrent reader/writer safe,
*except* the actual garbage collection part of `git gc` that deletes
loose objects. There is no reason to prevent concurrent readers while
there is a push in progress.

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.

-- 
Shawn.

On Friday 01 April 2011, Shawn Pearce wrote:

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, [off-list ref]
www.herland.net