[PATCH 1/6] Allow pack header preprocessing before unpack-objects/index-pack.

[PATCH 1/6] Allow pack header preprocessing before unpack-objects/index-pack.

[Nicolas Pitre]

Some applications which invoke unpack-objects or index-pack --stdin
may want to examine the pack header to determine the number of
objects contained in the pack and use that value to determine which
executable to invoke to handle the rest of the pack stream.

However if the caller consumes the pack header from the input stream
then its no longer available for unpack-objects or index-pack --stdin,
both of which need the version and object count to process the stream.

This change introduces --pack_header=ver,cnt as a command line option
that the caller can supply to indicate it has already consumed the
pack header and what version and object count were found in that
header.  As this option is only meant for low level applications
such as receive-pack we are not documenting it at this time.

Signed-off-by: Nicolas Pitre <nico@cam.org>
---

Patch description text shamelessly stolen from a similar patch from
Shawn Pearce <spearce@spearce.org>.



 builtin-unpack-objects.c |   15 +++++++++++++++
 index-pack.c             |   13 +++++++++++++
 2 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/builtin-unpack-objects.c b/builtin-unpack-objects.c
index 74a90c1..e6d7574 100644
--- a/builtin-unpack-objects.c
+++ b/builtin-unpack-objects.c
@@ -371,6 +371,21 @@ int cmd_unpack_objects(int argc, const c
 				recover = 1;
 				continue;
 			}
+			if (!strncmp(arg, "--pack_header=", 14)) {
+				struct pack_header *hdr;
+				char *c;
+
+				hdr = (struct pack_header *)buffer;
+				hdr->hdr_signature = htonl(PACK_SIGNATURE);
+				hdr->hdr_version = htonl(strtoul(arg + 14, &c, 10));
+				if (*c != ',')
+					die("bad %s", arg);
+				hdr->hdr_entries = htonl(strtoul(c + 1, &c, 10));
+				if (*c)
+					die("bad %s", arg);
+				len = sizeof(*hdr);
+				continue;
+			}
 			usage(unpack_usage);
 		}
 
diff --git a/index-pack.c b/index-pack.c
index b37dd78..a3b55f9 100644
--- a/index-pack.c
+++ b/index-pack.c
@@ -841,6 +841,19 @@ int main(int argc, char **argv)
 				keep_msg = "";
 			} else if (!strncmp(arg, "--keep=", 7)) {
 				keep_msg = arg + 7;
+			} else if (!strncmp(arg, "--pack_header=", 14)) {
+				struct pack_header *hdr;
+				char *c;
+
+				hdr = (struct pack_header *)input_buffer;
+				hdr->hdr_signature = htonl(PACK_SIGNATURE);
+				hdr->hdr_version = htonl(strtoul(arg + 14, &c, 10));
+				if (*c != ',')
+					die("bad %s", arg);
+				hdr->hdr_entries = htonl(strtoul(c + 1, &c, 10));
+				if (*c)
+					die("bad %s", arg);
+				input_len = sizeof(*hdr);
 			} else if (!strcmp(arg, "-v")) {
 				verbose = 1;
 			} else if (!strcmp(arg, "-o")) {
-- 
1.4.3.3.g87b2-dirty

[PATCH 2/6] Teach receive-pack how to keep pack files based on object count.

[Nicolas Pitre]

From: Shawn Pearce <spearce@spearce.org>

Since keeping a pushed pack or exploding it into loose objects
should be a local repository decision this teaches receive-pack
to decide if it should call unpack-objects or index-pack --stdin
--fix-thin based on the setting of receive.unpackLimit and the
number of objects contained in the received pack.

If the number of objects (hdr_entries) in the received pack is
below the value of receive.unpackLimit (which is 5000 by default)
then we unpack-objects as we have in the past.

If the hdr_entries >= receive.unpackLimit then we call index-pack and
ask it to include our pid and hostname in the .keep file to make it
easier to identify why a given pack has been kept in the repository.

Currently this leaves every received pack as a kept pack.  We really
don't want that as received packs will tend to be small.  Instead we
want to delete the .keep file automatically after all refs have
been updated.  That is being left as room for future improvement.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 Documentation/config.txt |   11 +++++-
 cache.h                  |    1 +
 receive-pack.c           |   98 +++++++++++++++++++++++++++++++++++++---------
 sha1_file.c              |    2 +-
 4 files changed, 91 insertions(+), 21 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index d9e73da..9d3c71c 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -301,7 +301,16 @@ imap::
 	The configuration variables in the 'imap' section are described
 	in gitlink:git-imap-send[1].
 
-receive.denyNonFastforwads::
+receive.unpackLimit::
+	If the number of objects received in a push is below this
+	limit then the objects will be unpacked into loose object
+	files. However if the number of received objects equals or
+	exceeds this limit then the received pack will be stored as
+	a pack, after adding any missing delta bases.  Storing the
+	pack from a push can make the push operation complete faster,
+	especially on slow filesystems.
+
+receive.denyNonFastForwards::
 	If set to true, git-receive-pack will deny a ref update which is
 	not a fast forward. Use this to prevent such an update via a push,
 	even if that push is forced. This configuration variable is
diff --git a/cache.h b/cache.h
index e997a85..6cb7e1d 100644
--- a/cache.h
+++ b/cache.h
@@ -376,6 +376,7 @@ extern struct packed_git *parse_pack_ind
 						char *idx_path);
 
 extern void prepare_packed_git(void);
+extern void reprepare_packed_git(void);
 extern void install_packed_git(struct packed_git *pack);
 
 extern struct packed_git *find_sha1_pack(const unsigned char *sha1, 
diff --git a/receive-pack.c b/receive-pack.c
index e2ba8a8..be4c3fb 100644
--- a/receive-pack.c
+++ b/receive-pack.c
@@ -1,4 +1,5 @@
 #include "cache.h"
+#include "pack.h"
 #include "refs.h"
 #include "pkt-line.h"
 #include "run-command.h"
@@ -7,9 +8,8 @@
 
 static const char receive_pack_usage[] = "git-receive-pack <git-dir>";
 
-static const char *unpacker[] = { "unpack-objects", NULL };
-
 static int deny_non_fast_forwards = 0;
+static int unpack_limit = 5000;
 static int report_status;
 
 static char capabilities[] = "report-status";
@@ -25,6 +25,12 @@ static int receive_pack_config(const cha
 		return 0;
 	}
 
+	if (strcmp(var, "receive.unpacklimit") == 0)
+	{
+		unpack_limit = git_config_int(var, value);
+		return 0;
+	}
+
 	return 0;
 }
 
@@ -227,27 +233,81 @@ static void read_head_info(void)
 	}
 }
 
+static const char *parse_pack_header(struct pack_header *hdr)
+{
+	char *c = (char*)hdr;
+	ssize_t remaining = sizeof(struct pack_header);
+	do {
+		ssize_t r = xread(0, c, remaining);
+		if (r <= 0)
+			return "eof before pack header was fully read";
+		remaining -= r;
+		c += r;
+	} while (remaining > 0);
+	if (hdr->hdr_signature != htonl(PACK_SIGNATURE))
+		return "protocol error (pack signature mismatch detected)";
+	if (!pack_version_ok(hdr->hdr_version))
+		return "protocol error (pack version unsupported)";
+	return NULL;
+}
+
 static const char *unpack(void)
 {
-	int code = run_command_v_opt(1, unpacker, RUN_GIT_CMD);
+	struct pack_header hdr;
+	const char *hdr_err;
+	char hdr_arg[38];
+	int code;
+
+	hdr_err = parse_pack_header(&hdr);
+	if (hdr_err)
+		return hdr_err;
+	snprintf(hdr_arg, sizeof(hdr_arg), "--pack_header=%u,%u",
+			ntohl(hdr.hdr_version), ntohl(hdr.hdr_entries));
+
+	if (ntohl(hdr.hdr_entries) < unpack_limit) {
+		const char *unpacker[3];
+		unpacker[0] = "unpack-objects";
+		unpacker[1] = hdr_arg;
+		unpacker[2] = NULL;
+		code = run_command_v_opt(1, unpacker, RUN_GIT_CMD);
+	} else {
+		const char *keeper[6];
+		char my_host[255], keep_arg[128 + 255];
+
+		if (gethostname(my_host, sizeof(my_host)))
+			strcpy(my_host, "localhost");
+		snprintf(keep_arg, sizeof(keep_arg),
+				"--keep=receive-pack %i on %s",
+				getpid(), my_host);
+
+		keeper[0] = "index-pack";
+		keeper[1] = "--stdin";
+		keeper[2] = "--fix-thin";
+		keeper[3] = hdr_arg;
+		keeper[4] = keep_arg;
+		keeper[5] = NULL;
+		code = run_command_v_opt(1, keeper, RUN_GIT_CMD);
+		if (!code)
+			reprepare_packed_git();
+	}
 
 	switch (code) {
-	case 0:
-		return NULL;
-	case -ERR_RUN_COMMAND_FORK:
-		return "unpack fork failed";
-	case -ERR_RUN_COMMAND_EXEC:
-		return "unpack execute failed";
-	case -ERR_RUN_COMMAND_WAITPID:
-		return "waitpid failed";
-	case -ERR_RUN_COMMAND_WAITPID_WRONG_PID:
-		return "waitpid is confused";
-	case -ERR_RUN_COMMAND_WAITPID_SIGNAL:
-		return "unpacker died of signal";
-	case -ERR_RUN_COMMAND_WAITPID_NOEXIT:
-		return "unpacker died strangely";
-	default:
-		return "unpacker exited with error code";
+		case 0:
+			return NULL;
+		case -ERR_RUN_COMMAND_FORK:
+			return "unpack fork failed";
+		case -ERR_RUN_COMMAND_EXEC:
+			return "unpack execute failed";
+		case -ERR_RUN_COMMAND_WAITPID:
+			return "waitpid failed";
+		case -ERR_RUN_COMMAND_WAITPID_WRONG_PID:
+			return "waitpid is confused";
+		case -ERR_RUN_COMMAND_WAITPID_SIGNAL:
+			return "unpacker died of signal";
+		case -ERR_RUN_COMMAND_WAITPID_NOEXIT:
+			return "unpacker died strangely";
+		default:
+			return "unpacker exited with error code";
 	}
 }
 
diff --git a/sha1_file.c b/sha1_file.c
index 5e6c8b8..7bda2d4 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -663,7 +663,7 @@ void prepare_packed_git(void)
 	prepare_packed_git_run_once = 1;
 }
 
-static void reprepare_packed_git(void)
+void reprepare_packed_git(void)
 {
 	prepare_packed_git_run_once = 0;
 	prepare_packed_git();
-- 
1.4.3.3.g87b2-dirty

[PATCH 3/6] git-fetch can use both --thin and --keep with fetch-pack now

[Nicolas Pitre]

Signed-off-by: Nicolas Pitre <nico@cam.org>
---
 git-fetch.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-fetch.sh b/git-fetch.sh
index 539dff6..2b5538f 100755
--- a/git-fetch.sh
+++ b/git-fetch.sh
@@ -20,7 +20,7 @@ verbose=
 update_head_ok=
 exec=
 upload_pack=
-keep=--thin
+keep=
 while case "$#" in 0) break ;; esac
 do
 	case "$1" in
@@ -370,7 +370,7 @@ fetch_main () {
     ( : subshell because we muck with IFS
       IFS=" 	$LF"
       (
-	  git-fetch-pack $exec $keep "$remote" $rref || echo failed "$remote"
+	  git-fetch-pack --thin $exec $keep "$remote" $rref || echo failed "$remote"
       ) |
       while read sha1 remote_name
       do
-- 
1.4.3.3.g87b2-dirty

[PATCH 4/6] improve fetch-pack's handling of kept packs

[Nicolas Pitre]

Since functions in fetch-clone.c were only used from fetch-pack.c,
its content has been merged with fetch-pack.c.  This allows for better
coupling of features with much simpler implementations.

One new thing is that the (abscence of) --thin also enforce it on
index-pack now, such that index-pack will abort if a thin pack was
_not_ asked for.

The -k or --keep, when provided twice, now causes the fetched pack
to be left as a kept pack just like receive-pack currently does.
Eventually this will be used to close a race against concurrent
repacking.

Signed-off-by: Nicolas Pitre <nico@cam.org>
---
 Documentation/git-fetch-pack.txt |    3 +-
 Makefile                         |    2 +-
 cache.h                          |    4 --
 fetch-clone.c                    |   87 ------------------------------
 fetch-pack.c                     |  110 +++++++++++++++++++++++++++++++++++---
 5 files changed, 106 insertions(+), 100 deletions(-)

diff --git a/Documentation/git-fetch-pack.txt b/Documentation/git-fetch-pack.txt
index bff9aa6..3e6cd88 100644
--- a/Documentation/git-fetch-pack.txt
+++ b/Documentation/git-fetch-pack.txt
@@ -32,7 +32,8 @@ OPTIONS
 -k::
 	Do not invoke 'git-unpack-objects' on received data, but
 	create a single packfile out of it instead, and store it
-	in the object database.
+	in the object database. If provided twice then the pack is
+	locked against repacking.
 
 --exec=<git-upload-pack>::
 	Use this to specify the path to 'git-upload-pack' on the
diff --git a/Makefile b/Makefile
index 28a5d91..80d4a4e 100644
--- a/Makefile
+++ b/Makefile
@@ -260,7 +260,7 @@ LIB_OBJS = \
 	quote.o read-cache.o refs.o run-command.o dir.o object-refs.o \
 	server-info.o setup.o sha1_file.o sha1_name.o strbuf.o \
 	tag.o tree.o usage.o config.o environment.o ctype.o copy.o \
-	fetch-clone.o revision.o pager.o tree-walk.o xdiff-interface.o \
+	revision.o pager.o tree-walk.o xdiff-interface.o \
 	write_or_die.o trace.o list-objects.o grep.o \
 	alloc.o merge-file.o path-list.o help.o unpack-trees.o $(DIFF_OBJS) \
 	color.o wt-status.o archive-zip.o archive-tar.o
diff --git a/cache.h b/cache.h
index 6cb7e1d..f2ec5c8 100644
--- a/cache.h
+++ b/cache.h
@@ -416,10 +416,6 @@ extern int copy_fd(int ifd, int ofd);
 extern void write_or_die(int fd, const void *buf, size_t count);
 extern int write_or_whine(int fd, const void *buf, size_t count, const char *msg);
 
-/* Finish off pack transfer receiving end */
-extern int receive_unpack_pack(int fd[2], const char *me, int quiet, int);
-extern int receive_keep_pack(int fd[2], const char *me, int quiet, int);
-
 /* pager.c */
 extern void setup_pager(void);
 extern int pager_in_use;
diff --git a/fetch-clone.c b/fetch-clone.c
deleted file mode 100644
index f629d8d..0000000
--- a/fetch-clone.c
+++ /dev/null
@@ -1,87 +0,0 @@
-#include "cache.h"
-#include "exec_cmd.h"
-#include "pkt-line.h"
-#include "sideband.h"
-#include <sys/wait.h>
-
-static pid_t setup_sideband(int sideband, const char *me, int fd[2], int xd[2])
-{
-	pid_t side_pid;
-
-	if (!sideband) {
-		fd[0] = xd[0];
-		fd[1] = xd[1];
-		return 0;
-	}
-	/* xd[] is talking with upload-pack; subprocess reads from
-	 * xd[0], spits out band#2 to stderr, and feeds us band#1
-	 * through our fd[0].
-	 */
-	if (pipe(fd) < 0)
-		die("%s: unable to set up pipe", me);
-	side_pid = fork();
-	if (side_pid < 0)
-		die("%s: unable to fork off sideband demultiplexer", me);
-	if (!side_pid) {
-		/* subprocess */
-		close(fd[0]);
-		if (xd[0] != xd[1])
-			close(xd[1]);
-		if (recv_sideband(me, xd[0], fd[1], 2))
-			exit(1);
-		exit(0);
-	}
-	close(xd[0]);
-	close(fd[1]);
-	fd[1] = xd[1];
-	return side_pid;
-}
-
-static int get_pack(int xd[2], const char *me, int sideband, const char **argv)
-{
-	int status;
-	pid_t pid, side_pid;
-	int fd[2];
-
-	side_pid = setup_sideband(sideband, me, fd, xd);
-	pid = fork();
-	if (pid < 0)
-		die("%s: unable to fork off %s", me, argv[0]);
-	if (!pid) {
-		dup2(fd[0], 0);
-		close(fd[0]);
-		close(fd[1]);
-		execv_git_cmd(argv);
-		die("%s exec failed", argv[0]);
-	}
-	close(fd[0]);
-	close(fd[1]);
-	while (waitpid(pid, &status, 0) < 0) {
-		if (errno != EINTR)
-			die("waiting for %s: %s", argv[0], strerror(errno));
-	}
-	if (WIFEXITED(status)) {
-		int code = WEXITSTATUS(status);
-		if (code)
-			die("%s died with error code %d", argv[0], code);
-		return 0;
-	}
-	if (WIFSIGNALED(status)) {
-		int sig = WTERMSIG(status);
-		die("%s died of signal %d", argv[0], sig);
-	}
-	die("%s died of unnatural causes %d", argv[0], status);
-}
-
-int receive_unpack_pack(int xd[2], const char *me, int quiet, int sideband)
-{
-	const char *argv[3] = { "unpack-objects", quiet ? "-q" : NULL, NULL };
-	return get_pack(xd, me, sideband, argv);
-}
-
-int receive_keep_pack(int xd[2], const char *me, int quiet, int sideband)
-{
-	const char *argv[5] = { "index-pack", "--stdin", "--fix-thin",
-				quiet ? NULL : "-v", NULL };
-	return get_pack(xd, me, sideband, argv);
-}
diff --git a/fetch-pack.c b/fetch-pack.c
index 36ea092..0a169dc 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -3,6 +3,9 @@
 #include "pkt-line.h"
 #include "commit.h"
 #include "tag.h"
+#include "exec_cmd.h"
+#include "sideband.h"
+#include <sys/wait.h>
 
 static int keep_pack;
 static int quiet;
@@ -416,6 +419,103 @@ static int everything_local(struct ref *
 	return retval;
 }
 
+static pid_t setup_sideband(int fd[2], int xd[2])
+{
+	pid_t side_pid;
+
+	if (!use_sideband) {
+		fd[0] = xd[0];
+		fd[1] = xd[1];
+		return 0;
+	}
+	/* xd[] is talking with upload-pack; subprocess reads from
+	 * xd[0], spits out band#2 to stderr, and feeds us band#1
+	 * through our fd[0].
+	 */
+	if (pipe(fd) < 0)
+		die("fetch-pack: unable to set up pipe");
+	side_pid = fork();
+	if (side_pid < 0)
+		die("fetch-pack: unable to fork off sideband demultiplexer");
+	if (!side_pid) {
+		/* subprocess */
+		close(fd[0]);
+		if (xd[0] != xd[1])
+			close(xd[1]);
+		if (recv_sideband("fetch-pack", xd[0], fd[1], 2))
+			exit(1);
+		exit(0);
+	}
+	close(xd[0]);
+	close(fd[1]);
+	fd[1] = xd[1];
+	return side_pid;
+}
+
+static int get_pack(int xd[2], const char **argv)
+{
+	int status;
+	pid_t pid, side_pid;
+	int fd[2];
+
+	side_pid = setup_sideband(fd, xd);
+	pid = fork();
+	if (pid < 0)
+		die("fetch-pack: unable to fork off %s", argv[0]);
+	if (!pid) {
+		dup2(fd[0], 0);
+		close(fd[0]);
+		close(fd[1]);
+		execv_git_cmd(argv);
+		die("%s exec failed", argv[0]);
+	}
+	close(fd[0]);
+	close(fd[1]);
+	while (waitpid(pid, &status, 0) < 0) {
+		if (errno != EINTR)
+			die("waiting for %s: %s", argv[0], strerror(errno));
+	}
+	if (WIFEXITED(status)) {
+		int code = WEXITSTATUS(status);
+		if (code)
+			die("%s died with error code %d", argv[0], code);
+		return 0;
+	}
+	if (WIFSIGNALED(status)) {
+		int sig = WTERMSIG(status);
+		die("%s died of signal %d", argv[0], sig);
+	}
+	die("%s died of unnatural causes %d", argv[0], status);
+}
+
+static int explode_rx_pack(int xd[2])
+{
+	const char *argv[3] = { "unpack-objects", quiet ? "-q" : NULL, NULL };
+	return get_pack(xd, argv);
+}
+
+static int keep_rx_pack(int xd[2])
+{
+	const char *argv[6];
+	char keep_arg[256];
+	int n = 0;
+
+	argv[n++] = "index-pack";
+	argv[n++] = "--stdin";
+	if (!quiet)
+		argv[n++] = "-v";
+	if (use_thin_pack)
+		argv[n++] = "--fix-thin";
+	if (keep_pack > 1) {
+		int s = sprintf(keep_arg, "--keep=fetch-pack %i on ", getpid());
+		if (gethostname(keep_arg + s, sizeof(keep_arg) - s))
+			strcpy(keep_arg + s, "localhost");
+		argv[n++] = keep_arg;
+	}
+	argv[n] = NULL;
+	return get_pack(xd, argv);
+}
+
 static int fetch_pack(int fd[2], int nr_match, char **match)
 {
 	struct ref *ref;
@@ -447,17 +547,13 @@ static int fetch_pack(int fd[2], int nr_
 		goto all_done;
 	}
 	if (find_common(fd, sha1, ref) < 0)
-		if (!keep_pack)
+		if (keep_pack != 1)
 			/* When cloning, it is not unusual to have
 			 * no common commit.
 			 */
 			fprintf(stderr, "warning: no common commits\n");
 
-	if (keep_pack)
-		status = receive_keep_pack(fd, "git-fetch-pack", quiet, use_sideband);
-	else
-		status = receive_unpack_pack(fd, "git-fetch-pack", quiet, use_sideband);
-
+	status = (keep_pack) ? keep_rx_pack(fd) : explode_rx_pack(fd);
 	if (status)
 		die("git-fetch-pack: fetch failed.");
 
@@ -494,7 +590,7 @@ int main(int argc, char **argv)
 				continue;
 			}
 			if (!strcmp("--keep", arg) || !strcmp("-k", arg)) {
-				keep_pack = 1;
+				keep_pack++;
 				continue;
 			}
 			if (!strcmp("--thin", arg)) {
-- 
1.4.3.3.g87b2-dirty

[PATCH 5/6] have index-pack create .keep file more carefully

[Nicolas Pitre]

If by chance we receive a pack which content (list of objects) matches
another pack that we already have, and if that pack is marked with a
.keep file, then we should not overwrite it.

Signed-off-by: Nicolas Pitre <nico@cam.org>
---
 index-pack.c |   17 ++++++++++-------
 1 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/index-pack.c b/index-pack.c
index a3b55f9..8d64a88 100644
--- a/index-pack.c
+++ b/index-pack.c
@@ -788,14 +788,17 @@ static void final(const char *final_pack
 				 get_object_directory(), sha1_to_hex(sha1));
 			keep_name = name;
 		}
-		keep_fd = open(keep_name, O_RDWR | O_CREAT, 0600);
-		if (keep_fd < 0)
-			die("cannot write keep file");
-		if (keep_msg_len > 0) {
-			write_or_die(keep_fd, keep_msg, keep_msg_len);
-			write_or_die(keep_fd, "\n", 1);
+		keep_fd = open(keep_name, O_RDWR|O_CREAT|O_EXCL, 0600);
+		if (keep_fd < 0) {
+			if (errno != EEXIST)
+				die("cannot write keep file");
+		} else {
+			if (keep_msg_len > 0) {
+				write_or_die(keep_fd, keep_msg, keep_msg_len);
+				write_or_die(keep_fd, "\n", 1);
+			}
+			close(keep_fd);
 		}
-		close(keep_fd);
 	}
 
 	if (final_pack_name != curr_pack_name) {
-- 
1.4.3.3.g87b2-dirty

[PATCH 6/6] remove .keep pack lock files when done with refs update

[Nicolas Pitre]

This makes both git-fetch and git-push (fetch-pack and receive-pack)
safe against a possible race with aparallel git-repack -a -d that could
prune the new pack while it is not yet referenced, and remove the .keep
file after refs have been updated.

Signed-off-by: Nicolas Pitre <nico@cam.org>
---
 Documentation/git-index-pack.txt |   11 ++++
 git-fetch.sh                     |   10 ++++-
 index-pack.c                     |   38 +++++++++------
 receive-pack.c                   |   96 +++++++++++++++++++++++++++++---------
 4 files changed, 116 insertions(+), 39 deletions(-)

diff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt
index 1235416..2229ee8 100644
--- a/Documentation/git-index-pack.txt
+++ b/Documentation/git-index-pack.txt
@@ -69,6 +69,17 @@ OPTIONS
 	locate any which have outlived their usefulness.
 
 
+Note
+----
+
+Once the index has been created, the list of object names is sorted
+and the SHA1 hash of that list is printed to stdout. If --stdin was
+also used then this is prefixed by either "pack\t", or "keep\t" if a
+new .keep file was successfully created. This is useful to remove a
+.keep file used as a lock to prevent the race with gitlink:git-repack[1]
+mentioned above.
+
+
 Author
 ------
 Written by Sergey Vlasov <vsu@altlinux.ru>
diff --git a/git-fetch.sh b/git-fetch.sh
index 2b5538f..e8ce296 100755
--- a/git-fetch.sh
+++ b/git-fetch.sh
@@ -51,7 +51,7 @@ do
 		verbose=Yes
 		;;
 	-k|--k|--ke|--kee|--keep)
-		keep=--keep
+		keep=-k -k
 		;;
 	--reflog-action=*)
 		rloga=`expr "z$1" : 'z-[^=]*=\(.*\)'`
@@ -368,6 +368,7 @@ fetch_main () {
       ;; # we are already done.
   *)
     ( : subshell because we muck with IFS
+      pack_lockfile=
       IFS=" 	$LF"
       (
 	  git-fetch-pack --thin $exec $keep "$remote" $rref || echo failed "$remote"
@@ -378,6 +379,12 @@ fetch_main () {
 	  failed)
 		  echo >&2 "Fetch failure: $remote"
 		  exit 1 ;;
+	  # special line coming from index-pack with the pack name
+	  pack)
+		  continue ;;
+	  keep)
+		  pack_lockfile="$GIT_OBJECT_DIRECTORY/pack/pack-$remote_name.keep"
+		  continue ;;
 	  esac
 	  found=
 	  single_force=
@@ -408,6 +415,7 @@ fetch_main () {
 	  append_fetch_head "$sha1" "$remote" \
 		  "$remote_name" "$remote_nick" "$local_name" "$not_for_merge"
       done
+      if [ "$pack_lockfile" ]; then rm -f "$pack_lockfile"; fi
     ) || exit ;;
   esac
 
diff --git a/index-pack.c b/index-pack.c
index 8d64a88..042aea8 100644
--- a/index-pack.c
+++ b/index-pack.c
@@ -757,6 +757,7 @@ static void final(const char *final_pack
 		  const char *keep_name, const char *keep_msg,
 		  unsigned char *sha1)
 {
+	char *report = "pack";
 	char name[PATH_MAX];
 	int err;
 
@@ -767,18 +768,6 @@ static void final(const char *final_pack
 		if (err)
 			die("error while closing pack file: %s", strerror(errno));
 		chmod(curr_pack_name, 0444);
-
-		/*
-		 * Let's just mimic git-unpack-objects here and write
-		 * the last part of the buffer to stdout.
-		 */
-		while (input_len) {
-			err = xwrite(1, input_buffer + input_offset, input_len);
-			if (err <= 0)
-				break;
-			input_len -= err;
-			input_offset += err;
-		}
 	}
 
 	if (keep_msg) {
@@ -798,6 +787,7 @@ static void final(const char *final_pack
 				write_or_die(keep_fd, "\n", 1);
 			}
 			close(keep_fd);
+			report = "keep";
 		}
 	}
 
@@ -821,6 +811,27 @@ static void final(const char *final_pack
 		if (move_temp_to_file(curr_index_name, final_index_name))
 			die("cannot store index file");
 	}
+
+	if (!from_stdin) {
+		printf("%s\n", sha1_to_hex(sha1));
+	} else {
+		char buf[48];
+		int len = snprintf(buf, sizeof(buf), "%s\t%s\n",
+				   report, sha1_to_hex(sha1));
+		xwrite(1, buf, len);
+
+		/*
+		 * Let's just mimic git-unpack-objects here and write
+		 * the last part of the input buffer to stdout.
+		 */
+		while (input_len) {
+			err = xwrite(1, input_buffer + input_offset, input_len);
+			if (err <= 0)
+				break;
+			input_len -= err;
+			input_offset += err;
+		}
+	}
 }
 
 int main(int argc, char **argv)
@@ -937,8 +948,5 @@ int main(int argc, char **argv)
 	free(index_name_buf);
 	free(keep_name_buf);
 
-	if (!from_stdin)
-		printf("%s\n", sha1_to_hex(sha1));
-
 	return 0;
 }
diff --git a/receive-pack.c b/receive-pack.c
index be4c3fb..8bb5683 100644
--- a/receive-pack.c
+++ b/receive-pack.c
@@ -3,8 +3,10 @@
 #include "refs.h"
 #include "pkt-line.h"
 #include "run-command.h"
+#include "exec_cmd.h"
 #include "commit.h"
 #include "object.h"
+#include <sys/wait.h>
 
 static const char receive_pack_usage[] = "git-receive-pack <git-dir>";
 
@@ -251,12 +253,13 @@ static const char *parse_pack_header(str
 	return NULL;
 }
 
+static const char *pack_lockfile;
+
 static const char *unpack(void)
 {
 	struct pack_header hdr;
 	const char *hdr_err;
 	char hdr_arg[38];
-	int code;
 
 	hdr_err = parse_pack_header(&hdr);
 	if (hdr_err)
@@ -265,33 +268,13 @@ static const char *unpack(void)
 			ntohl(hdr.hdr_version), ntohl(hdr.hdr_entries));
 
 	if (ntohl(hdr.hdr_entries) < unpack_limit) {
+		int code;
 		const char *unpacker[3];
 		unpacker[0] = "unpack-objects";
 		unpacker[1] = hdr_arg;
 		unpacker[2] = NULL;
 		code = run_command_v_opt(1, unpacker, RUN_GIT_CMD);
-	} else {
-		const char *keeper[6];
-		char my_host[255], keep_arg[128 + 255];
-
-		if (gethostname(my_host, sizeof(my_host)))
-			strcpy(my_host, "localhost");
-		snprintf(keep_arg, sizeof(keep_arg),
-				"--keep=receive-pack %i on %s",
-				getpid(), my_host);
-
-		keeper[0] = "index-pack";
-		keeper[1] = "--stdin";
-		keeper[2] = "--fix-thin";
-		keeper[3] = hdr_arg;
-		keeper[4] = keep_arg;
-		keeper[5] = NULL;
-		code = run_command_v_opt(1, keeper, RUN_GIT_CMD);
-		if (!code)
-			reprepare_packed_git();
-	}
-
-	switch (code) {
+		switch (code) {
 		case 0:
 			return NULL;
 		case -ERR_RUN_COMMAND_FORK:
@@ -308,6 +291,71 @@ static const char *unpack(void)
 			return "unpacker died strangely";
 		default:
 			return "unpacker exited with error code";
+		}
+	} else {
+		const char *keeper[6];
+		int fd[2], s, len, status;
+		pid_t pid;
+		char keep_arg[256];
+		char packname[46];
+
+		s = sprintf(keep_arg, "--keep=receive-pack %i on ", getpid());
+		if (gethostname(keep_arg + s, sizeof(keep_arg) - s))
+			strcpy(keep_arg + s, "localhost");
+
+		keeper[0] = "index-pack";
+		keeper[1] = "--stdin";
+		keeper[2] = "--fix-thin";
+		keeper[3] = hdr_arg;
+		keeper[4] = keep_arg;
+		keeper[5] = NULL;
+
+		if (pipe(fd) < 0)
+			return "index-pack pipe failed";
+		pid = fork();
+		if (pid < 0)
+			return "index-pack fork failed";
+		if (!pid) {
+			dup2(fd[1], 1);
+			close(fd[1]);
+			close(fd[0]);
+			execv_git_cmd(keeper);
+			die("execv of index-pack failed");
+		}
+		close(fd[1]);
+
+		/*
+		 * The first thing we expects from index-pack's output
+		 * is "pack\t%40s\n" or "keep\t%40s\n" (46 bytes) where
+		 * %40s is the newly created pack SHA1 name.  In the "keep"
+		 * case, we need it to remove the corresponding .keep file
+		 * later on.  If we don't get that then tough luck with it.
+		 */
+		for (len = 0;
+		     len < 46 && (s = xread(fd[0], packname+len, 46-len)) > 0;
+		     len += s);
+		close(fd[0]);
+		if (len == 46 && packname[45] == '\n' &&
+		    memcmp(packname, "keep\t", 5) == 0) {
+			char path[PATH_MAX];
+			packname[45] = 0;
+			snprintf(path, sizeof(path), "%s/pack/pack-%s.keep",
+				 get_object_directory(), packname + 5);
+			pack_lockfile = xstrdup(path);
+		}
+
+		/* Then wrap our index-pack process. */
+		while (waitpid(pid, &status, 0) < 0)
+			if (errno != EINTR)
+				return "waitpid failed";
+		if (WIFEXITED(status)) {
+			int code = WEXITSTATUS(status);
+			if (code)
+				return "index-pack exited with error code";
+			reprepare_packed_git();
+			return NULL;
+		}
+		return "index-pack abnormal exit";
 	}
 }
 
@@ -363,6 +411,8 @@ int main(int argc, char **argv)
 		const char *unpack_status = unpack();
 		if (!unpack_status)
 			execute_commands();
+		if (pack_lockfile)
+			unlink(pack_lockfile);
 		if (report_status)
 			report(unpack_status);
 	}
-- 
1.4.3.3.g87b2-dirty

Re: [PATCH 1/6] Allow pack header preprocessing before unpack-objects/index-pack.

[Junio C Hamano]

Rather nicely done.  I see upto 2/6 are identical to what has
sitting in "pu" for the past few days, modulo some indentation?

I wonder how much testing has this series seen, by the way.

Also, I wonder what happens after 5/6 errors out.  It dies and
the caller (typically receive-pack) reports it back, which would
fail the push (and does not update the refs).  Retrying in such
a case would probably use the same set of refs on both ends,
resulting in exactly the same pack...

Re: [PATCH 1/6] Allow pack header preprocessing before unpack-objects/index-pack.

[Nicolas Pitre]

On Wed, 1 Nov 2006, Junio C Hamano wrote:

> Rather nicely done.  I see upto 2/6 are identical to what has
> sitting in "pu" for the past few days, modulo some indentation?

Yes.  I just inserted them in the serie as I didn't see them in pu until 
a very recent pull.

> I wonder how much testing has this series seen, by the way.

It has been tested a bit.  At least until it started behaving 
correctly for me. There might still be bugs remaining, but nothing that 
should screw a repository up. I think it should be good enough for next.

> Also, I wonder what happens after 5/6 errors out.  It dies and
> the caller (typically receive-pack) reports it back, which would
> fail the push (and does not update the refs).  Retrying in such
> a case would probably use the same set of refs on both ends,
> resulting in exactly the same pack...

Well, the purpose of 5/6 is to avoid creating a .keep file if it already 
exists.  This is more a security thing so if you decide to mark one of 
your pack with .keep (say an history pack that doesn't need to be 
repacked) then you don't want a funny git server to send you the same 
pack just for the purpose of overwriting the same .keep file (that could 
be removed right away if it ends up being considered as a lock file).  
This is made more obvious with 6/6 as the .keep file is not removed if 
it was there before.  So this is not actually an error if creating a 
.keep file fails with EEXIST.

As for index-pack dying, it will be reported the same way as 
unpack-objects dying -- no difference there.  Of course if it dies on 
the first push, it is likely to die on all subsequent pushes since the 
condition for that to happen is most probably due to a serious problem 
(out of disk space, corruption in the existing objects in the 
repository, etc).

The pack is put in place with move_temp_to_file() so if a previous 
attempt already stored a pack with the same name before successfully 
updating corresponding refs then the old pack will silently be kept 
since we know both packs contain the same objects when they have the 
same name.  When a pack is moved it is always complete so there 
shouldn't be any (new) instance of repository corruption in normal 
circumbstances even in the presence of index-pack dying.

Re: [PATCH 6/6] remove .keep pack lock files when done with refs update

[Shawn Pearce]

Nicolas Pitre <nico@cam.org> wrote:
> This makes both git-fetch and git-push (fetch-pack and receive-pack)
> safe against a possible race with aparallel git-repack -a -d that could
> prune the new pack while it is not yet referenced, and remove the .keep
> file after refs have been updated.

Thanks for finishing this series out.  I've gotten sidetracked the
past two days and wasn't able to finish it myself.  Nicely done
too, your code looks much better than my drafts...  :-)

-- 

Re: [PATCH 6/6] remove .keep pack lock files when done with refs update

[Junio C Hamano]

Nicolas Pitre <nico@cam.org> writes:

> --- a/git-fetch.sh
> +++ b/git-fetch.sh
> @@ -51,7 +51,7 @@ do
>  		verbose=Yes
>  		;;
>  	-k|--k|--ke|--kee|--keep)
> -		keep=--keep
> +		keep=-k -k
>  		;;
>  	--reflog-action=*)
>  		rloga=`expr "z$1" : 'z-[^=]*=\(.*\)'`

Micronit.  This cannot be right without quoting.

Re: [PATCH 6/6] remove .keep pack lock files when done with refs update

[Nicolas Pitre]

On Wed, 1 Nov 2006, Junio C Hamano wrote:

> Nicolas Pitre <nico@cam.org> writes:
> 
> > --- a/git-fetch.sh
> > +++ b/git-fetch.sh
> > @@ -51,7 +51,7 @@ do
> >  		verbose=Yes
> >  		;;
> >  	-k|--k|--ke|--kee|--keep)
> > -		keep=--keep
> > +		keep=-k -k
> >  		;;
> >  	--reflog-action=*)
> >  		rloga=`expr "z$1" : 'z-[^=]*=\(.*\)'`
> 
> Micronit.  This cannot be right without quoting.

OK.  Shell programming isn't really my turf.

Another area of that patch I'm not really sure of is this:

@@ -408,6 +415,7 @@ fetch_main () {
          append_fetch_head "$sha1" "$remote" \
                  "$remote_name" "$remote_nick" "$local_name" "$not_for_merge"
       done
+      if [ "$pack_lockfile" ]; then rm -f "$pack_lockfile"; fi
     ) || exit ;;
   esac

How can I make sure to not clobber a non-zero return code with the rm 
that would prevent the exit while still performing the rm in all cases?

Re: [PATCH 6/6] remove .keep pack lock files when done with refs update

[Nicolas Pitre]

On Thu, 2 Nov 2006, Nicolas Pitre wrote:

> Another area of that patch I'm not really sure of is this:
> 
> @@ -408,6 +415,7 @@ fetch_main () {
>           append_fetch_head "$sha1" "$remote" \
>                   "$remote_name" "$remote_nick" "$local_name" "$not_for_merge"
>        done
> +      if [ "$pack_lockfile" ]; then rm -f "$pack_lockfile"; fi
>      ) || exit ;;
>    esac
> 
> How can I make sure to not clobber a non-zero return code with the rm 
> that would prevent the exit while still performing the rm in all cases?

OK looking at it again I think the || exit is only meant to match the 
exit 1 before that point. Since this is a subshell then the exit 1 
wouldn't exit it all on its own.  Otherwise there is no errors expected 
to trickle out of the while loop and I _think_ the above is correct.

Do you still want a new patch for the micronit quoting issue?

Re: [PATCH 6/6] remove .keep pack lock files when done with refs update

[Junio C Hamano]

Nicolas Pitre <nico@cam.org> writes:

> On Thu, 2 Nov 2006, Nicolas Pitre wrote:
>
>> Another area of that patch I'm not really sure of is this:
>> 
>> @@ -408,6 +415,7 @@ fetch_main () {
>>           append_fetch_head "$sha1" "$remote" \
>>                   "$remote_name" "$remote_nick" "$local_name" "$not_for_merge"
>>        done
>> +      if [ "$pack_lockfile" ]; then rm -f "$pack_lockfile"; fi
>>      ) || exit ;;
>>    esac
>> 
>> How can I make sure to not clobber a non-zero return code with the rm 
>> that would prevent the exit while still performing the rm in all cases?
>
> OK looking at it again I think the || exit is only meant to match the 
> exit 1 before that point. Since this is a subshell then the exit 1 
> wouldn't exit it all on its own.  Otherwise there is no errors expected 
> to trickle out of the while loop and I _think_ the above is correct.

I think so too.

> Do you still want a new patch for the micronit quoting issue?

Not needed, will park them in "pu" and play with them --
hopefully in "next" by the end of the week.

Thanks.