fetching packs and storing them as packs

With the last few patches I just posted it is now possible to receive 
(fetch) packs, validate them on the fly, complete them if they are thin 
packs, and store them directly without exploding them into loose 
objects.

There are advantages and inconvenients to both methods, so I think this 
should become a configuration option and/or even a command line argument 
to git-fetch. I think there are many more advantages to keeping packs 
packed hence I think using index-pack should become the default.

But I'm a bit tired to play with it and the final integration is for 
someone else to do.  I've tested it lightly using the extremely crude 
patch below to hook it in the fetch process.

Have fun!

diff --git a/fetch-clone.c b/fetch-clone.c
index 76b99af..28796c3 100644
--- a/fetch-clone.c
+++ b/fetch-clone.c

@@ -142,7 +142,8 @@ int receive_unpack_pack(int xd[2], const
 		dup2(fd[0], 0);
 		close(fd[0]);
 		close(fd[1]);
-		execl_git_cmd("unpack-objects", quiet ? "-q" : NULL, NULL);
+		execl_git_cmd("index-pack", "--stdin", "--fix-thin",
+			      quiet ? NULL : "-v", NULL);
 		die("git-unpack-objects exec failed");
 	}
 	close(fd[0]);

diff --git a/receive-pack.c b/receive-pack.c
index 1fcf3a9..7f6dc49 100644
--- a/receive-pack.c
+++ b/receive-pack.c

@@ -7,7 +7,7 @@
 
 static const char receive_pack_usage[] = "git-receive-pack <git-dir>";
 
-static const char *unpacker[] = { "unpack-objects", NULL };
+static const char *unpacker[] = { "index-pack", "-v", "--stdin", "--fix-thin", NULL };
 
 static int report_status;

On Thu, 26 Oct 2006, Eran Tromer wrote:

This creates a race condition w.r.t. "git repack -a -d", similar to the
existing race condition between "git fetch --keep" and
"git repack -a -d". There's a point in time where the new pack is stored
but not yet referenced, and if "git repack -a -d" runs at that point it
will eradicate the pack. When the heads are finally updated, you get a
corrupted repository.

(I note that there's a whole thread on this, but I was off doing other 
things, so I probably missed part of it)

We really should _never_ create a pack in-place with the final name.

The way to fix the race is to simply not create the patch as

	.git/objects/packed/pack-xyz.{pack|idx}

in the first place, but simply "mv" them into place later. If you do that 
after you've written the temporary pointer to it, there is no race (the 
temporary pointer may not be usable, of course, but that's a separate 
issue).

That said, I think some of the "git repack -d" logic is also unnecessarily 
fragile. In particular, it shouldn't just do

	existing=$(find . -type f \( -name '*.pack' -o -name '*.idx' \))

like it does to generate the "existing" list, it should probably only ever 
remove a pack-file and index file _pair_, ie it should do something like

	existing=$(find . -type f -name '*.pack')

and then do

	for pack in $existing
	do
		index="$(basename $pack).idx"
		if [ -f $index ] && [ "$pack"!= "$newpack" ]
		then
			rm -f "$pack" "$index"
		fi
	done

etc, exactly so that it would never remove anything that is getting 
indexed or is otherwise half-way done, regardless of any other issues.

Hmm?

On 2006-10-26 05:44, Nicolas Pitre wrote:

This creates a race condition w.r.t. "git repack -a -d", similar to the
existing race condition between "git fetch --keep" and
"git repack -a -d". There's a point in time where the new pack is stored
but not yet referenced, and if "git repack -a -d" runs at that point it
will eradicate the pack. When the heads are finally updated, you get a
corrupted repository.

(That's for the shell implementation of git-repack, at least. I assume
the new builtin preserves the old semantics.)

Since people run the supposedly safe "git repack -a -d" on regular
basis, this is going to bite.

  Eran