[PATCH] Add git-bundle: move objects and references by archive

[PATCH] Add git-bundle: move objects and references by archive

[Johannes Schindelin]

Some workflows require use of repositories on machines that cannot be 
connected, preventing use of git-fetch / git-push to transport objects and 
references between the repositories.

git-bundle provides an alternate transport mechanism, effectively allowing 
git-fetch and git-pull to operate using sneakernet transport. `git-bundle 
create` allows the user to create a bundle containing one or more branches 
or tags, but with specified basis assumed to exist on the target 
repository. At the receiving end, git-bundle acts like git-fetch-pack, 
allowing the user to invoke git-fetch or git-pull using the bundle file as 
the URL. git-fetch and git-ls-remote determine they have a bundle URL by 
checking that the URL points to a file, but are otherwise unchanged in 
operation with bundles.

The original patch was done by Mark Levedahl <mdl123@verizon.net>.

It was updated to make git-bundle a builtin, and get rid of the tar 
format: now, the first line is supposed to say "# v2 git bundle", the next 
lines either contain a prerequisite ("-" followed by the hash of the 
needed commit), or a ref (the hash of a commit, followed by the name of 
the ref), and finally the pack. As a result, the bundle argument can be 
"-" now.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	This patch should apply cleanly to "next".

	Mark, could you test on cygwin?

 .gitignore                   |    1 +
 Documentation/cmd-list.perl  |    1 +
 Documentation/git-bundle.txt |  139 +++++++++++++++
 Makefile                     |    1 +
 builtin-bundle.c             |  389 ++++++++++++++++++++++++++++++++++++++++++
 builtin.h                    |    1 +
 git-fetch.sh                 |    7 +
 git-ls-remote.sh             |    7 +-
 git.c                        |    1 +
 index-pack.c                 |    4 +-
 t/t5510-fetch.sh             |   28 +++-
 11 files changed, 575 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/git-bundle.txt
 create mode 100644 builtin-bundle.c

diff --git a/.gitignore b/.gitignore
index f15155d..4d3c66d 100644
--- a/.gitignore
+++ b/.gitignore
@@ -13,6 +13,7 @@ git-archive
 git-bisect
 git-blame
 git-branch
+git-bundle
 git-cat-file
 git-check-ref-format
 git-checkout
diff --git a/Documentation/cmd-list.perl b/Documentation/cmd-list.perl
index a2d6268..f61c77a 100755
--- a/Documentation/cmd-list.perl
+++ b/Documentation/cmd-list.perl
@@ -70,6 +70,7 @@ git-archive                             mainporcelain
 git-bisect                              mainporcelain
 git-blame                               ancillaryinterrogators
 git-branch                              mainporcelain
+git-bundle                              mainporcelain
 git-cat-file                            plumbinginterrogators
 git-checkout-index                      plumbingmanipulators
 git-checkout                            mainporcelain
diff --git a/Documentation/git-bundle.txt b/Documentation/git-bundle.txt
new file mode 100644
index 0000000..4ea9e85
--- /dev/null
+++ b/Documentation/git-bundle.txt
@@ -0,0 +1,139 @@
+git-bundle(1)
+=============
+
+NAME
+----
+git-bundle - Move objects and refs by archive
+
+
+SYNOPSIS
+--------
+'git-bundle' create <file> [git-rev-list args]
+'git-bundle' verify <file>
+'git-bundle' list-heads <file> [refname...]
+'git-bundle' unbundle <file> [refname...]
+
+DESCRIPTION
+-----------
+
+Some workflows require that one or more branches of development on one
+machine be replicated on another machine, but the two machines cannot
+be directly connected so the interactive git protocols (git, ssh,
+rsync, http) cannot be used.  This command provides suport for
+git-fetch and git-pull to operate by packaging objects and references
+in an archive at the originating machine, then importing those into
+another repository using gitlink:git-fetch[1] and gitlink:git-pull[1]
+after moving the archive by some means (i.e., by sneakernet).  As no
+direct connection between repositories exists, the user must specify a
+basis for the bundle that is held by the destination repository: the
+bundle assumes that all objects in the basis are already in the
+destination repository.
+
+OPTIONS
+-------
+
+create <file>::
+       Used to create a bundle named 'file'.  This requires the
+       git-rev-list arguments to define the bundle contents.
+
+verify <file>::
+       Used to check that a bundle file is valid and will apply
+       cleanly to the current repository.  This includes checks on the
+       bundle format itself as well as checking that the prerequisite
+       commits exist and are fully linked in the current repository.
+       git-bundle prints a list of missing commits, if any, and exits
+       with non-zero status.
+
+list-heads <file>::
+       Lists the references defined in the bundle.  If followed by a
+       list of references, only references matching those given are
+       printed out.
+
+unbundle <file>::
+       Passes the objects in the bundle to gitlink:git-index-pack[1]
+       for storage in the repository, then prints the names of all
+       defined references. If a reflist is given, only references
+       matching those in the given list are printed. This command is
+       really plumbing, intended to be called only by
+       gitlink:git-fetch[1].
+
+[git-rev-list-args...]::
+       A list of arguments, accepatble to git-rev-parse and
+       git-rev-list, that specify the specific objects and references
+       to transport.  For example, "master~10..master" causes the
+       current master reference to be packaged along with all objects
+       added since its 10th ancestor commit.  There is no explicit
+       limit to the number of references and objects that may be
+       packaged.
+
+
+[refname...]::
+       A list of references used to limit the references reported as
+       available. This is principally of use to git-fetch, which
+       expects to recieve only those references asked for and not
+       necessarily everything in the pack (in this case, git-bundle is
+       acting like gitlink:git-fetch-pack[1]).
+
+SPECIFYING REFERENCES
+---------------------
+
+git-bundle will only package references that are shown by
+git-show-ref: this includes heads, tags, and remote heads.  References
+such as master~1 cannot be packaged, but are perfectly suitable for
+defining the basis.  More than one reference may be packaged, and more
+than one basis can be specified.  The objects packaged are those not
+contained in the union of the given bases.  Each basis can be
+specified explicitly (e.g., ^master~10), or implicitly (e.g.,
+master~10..master, master --since=10.days.ago).
+
+It is very important that the basis used be held by the destination.
+It is ok to err on the side of conservatism, causing the bundle file
+to contain objects already in the destination as these are ignored
+when unpacking at the destination.
+
+EXAMPLE
+-------
+
+Assume two repositories exist as R1 on machine A, and R2 on machine B.
+For whatever reason, direct connection between A and B is not allowed,
+but we can move data from A to B via some mechanism (CD, email, etc).
+We want to update R2 with developments made on branch master in R1.
+We set a tag in R1 (lastR2bundle) after the previous such transport,
+and move it afterwards to help build the bundle.
+
+in R1 on A:
+$ git-bundle create mybundle master ^lastR2bundle
+$ git tag -f lastR2bundle master
+
+(move mybundle from A to B by some mechanism)
+
+in R2 on B:
+$ git-bundle verify mybundle
+$ git-fetch mybundle  refspec
+
+where refspec is refInBundle:localRef
+
+
+Also, with something like this in your config:
+
+[remote "bundle"]
+    url = /home/me/tmp/file.bdl
+    fetch = refs/heads/*:refs/remotes/origin/*
+
+You can first sneakernet the bundle file to ~/tmp/file.bdl and
+then these commands:
+
+$ git ls-remote bundle
+$ git fetch bundle
+$ git pull bundle
+
+would treat it as if it is talking with a remote side over the
+network.
+
+Author
+------
+Written by Mark Levedahl <mdl123@verizon.net>
+
+GIT
+---
+Part of the gitlink:git[7] suite
diff --git a/Makefile b/Makefile
index 56bb0b8..03a7f4a 100644
--- a/Makefile
+++ b/Makefile
@@ -276,6 +276,7 @@ BUILTIN_OBJS = \
 	builtin-archive.o \
 	builtin-blame.o \
 	builtin-branch.o \
+	builtin-bundle.o \
 	builtin-cat-file.o \
 	builtin-checkout-index.o \
 	builtin-check-ref-format.o \
diff --git a/builtin-bundle.c b/builtin-bundle.c
new file mode 100644
index 0000000..4bd596a
--- /dev/null
+++ b/builtin-bundle.c
@@ -0,0 +1,389 @@
+#include "cache.h"
+#include "object.h"
+#include "commit.h"
+#include "diff.h"
+#include "revision.h"
+#include "list-objects.h"
+#include "exec_cmd.h"
+
+/*
+ * Basic handler for bundle files to connect repositories via sneakernet.
+ * Invocation must include action.
+ * This function can create a bundle or provide information on an existing
+ * bundle supporting git-fetch, git-pull, and git-ls-remote
+ */
+
+static const char *bundle_usage="git-bundle (--create <bundle> <git-rev-list args> | --verify <bundle> | --list-heads <bundle> [refname]... | --unbundle <bundle> [refname]... )";
+
+static const char bundle_signature[] = "# v2 git bundle\n";
+
+struct ref_list {
+	unsigned int nr, alloc;
+	struct {
+		unsigned char sha1[20];
+		char *name;
+	} *list;
+};
+
+static void add_to_ref_list(const unsigned char *sha1, const char *name,
+		struct ref_list *list)
+{
+	if (list->nr + 1 >= list->alloc) {
+		list->alloc = alloc_nr(list->nr + 1);
+		list->list = xrealloc(list->list,
+				list->alloc * sizeof(list->list[0]));
+	}
+	memcpy(list->list[list->nr].sha1, sha1, 20);
+	list->list[list->nr].name = xstrdup(name);
+	list->nr++;
+}
+
+struct bundle_header {
+	struct ref_list prerequisites, references;
+};
+
+/* this function returns the length of the string */
+static int read_string(int fd, char *buffer, int size)
+{
+	int i;
+	for (i = 0; i < size - 1; i++) {
+		int count = read(fd, buffer + i, 1);
+		if (count < 0)
+			return error("Read error: %s", strerror(errno));
+		if (count == 0) {
+			i--;
+			break;
+		}
+		if (buffer[i] == '\n')
+			break;
+	}
+	buffer[i + 1] = '\0';
+	return i + 1;
+}
+
+/* returns an fd */
+static int read_header(const char *path, struct bundle_header *header) {
+	char buffer[1024];
+	int fd = open(path, O_RDONLY);
+
+	if (fd < 0)
+		return error("could not open '%s'", path);
+	if (read_string(fd, buffer, sizeof(buffer)) < 0 ||
+			strcmp(buffer, bundle_signature)) {
+		close(fd);
+		return error("'%s' does not look like a v2 bundle file", path);
+	}
+	while (read_string(fd, buffer, sizeof(buffer)) > 0
+			&& buffer[0] != '\n') {
+		int offset = buffer[0] == '-' ? 1 : 0;
+		int len = strlen(buffer);
+		unsigned char sha1[20];
+		struct ref_list *list = offset ? &header->prerequisites
+			: &header->references;
+		if (get_sha1_hex(buffer + offset, sha1)) {
+			close(fd);
+			return error("invalid SHA1: %s", buffer);
+		}
+		if (buffer[len - 1] == '\n')
+			buffer[len - 1] = '\0';
+		add_to_ref_list(sha1, buffer + 41 + offset, list);
+	}
+	return fd;
+}
+
+/* if in && *in >= 0, take that as input file descriptor instead */
+static int fork_with_pipe(const char **argv, int *in, int *out)
+{
+	int needs_in, needs_out;
+	int fdin[2], fdout[2], pid;
+
+	needs_in = in && *in < 0;
+	if (needs_in) {
+		if (pipe(fdin) < 0)
+			return error("could not setup pipe");
+		*in = fdin[1];
+	}
+
+	needs_out = out && *out < 0;
+	if (needs_out) {
+		if (pipe(fdout) < 0)
+			return error("could not setup pipe");
+		*out = fdout[0];
+	}
+
+	if ((pid = fork()) < 0) {
+		if (needs_in) {
+			close(fdin[0]);
+			close(fdin[1]);
+		}
+		if (needs_out) {
+			close(fdout[0]);
+			close(fdout[1]);
+		}
+		return error("could not fork");
+	}
+	if (!pid) {
+		if (needs_in) {
+			dup2(fdin[0], 0);
+			close(fdin[0]);
+			close(fdin[1]);
+		} else if (in)
+			dup2(*in, 0);
+		if (needs_out) {
+			dup2(fdout[1], 1);
+			close(fdout[0]);
+			close(fdout[1]);
+		} else if (out)
+			dup2(*out, 1);
+		exit(execv_git_cmd(argv));
+	}
+	if (needs_in)
+		close(fdin[0]);
+	if (needs_out)
+		close(fdout[1]);
+	return pid;
+}
+
+static int verify_bundle(struct bundle_header *header)
+{
+	/*
+	 * Do fast check, then if any prereqs are missing then go line by line
+	 * to be verbose about the errors
+	 */
+	struct ref_list *p = &header->prerequisites;
+	const char *argv[5] = {"rev-list", "--stdin", "--not", "--all", NULL};
+	int pid, in, out, i, ret = 0;
+	char buffer[1024];
+
+	in = out = -1;
+	pid = fork_with_pipe(argv, &in, &out);
+	if (pid < 0)
+		return error("Could not fork rev-list");
+	if (!fork()) {
+		for (i = 0; i < p->nr; i++) {
+			write(in, sha1_to_hex(p->list[i].sha1), 40);
+			write(in, "\n", 1);
+		}
+		close(in);
+		exit(0);
+	}
+	close(in);
+	while (read_string(out, buffer, sizeof(buffer)) > 0) {
+		if (ret++ == 0)
+			error ("The bundle requires the following commits you lack:");
+		fprintf(stderr, "%s", buffer);
+	}
+	close(out);
+	while (waitpid(pid, &i, 0) < 0)
+		if (errno != EINTR)
+			return -1;
+	if (!ret && (!WIFEXITED(i) || WEXITSTATUS(i)))
+		return error("At least one prerequisite is lacking.");
+
+	return ret;
+}
+
+static int list_heads(struct bundle_header *header, int argc, const char **argv)
+{
+	int i;
+	struct ref_list *r = &header->references;
+
+	for (i = 0; i < r->nr; i++) {
+		if (argc > 1) {
+			int j;
+			for (j = 1; j < argc; j++)
+				if (!strcmp(r->list[i].name, argv[j]))
+					break;
+			if (j == argc)
+				continue;
+		}
+		printf("%s %s\n", sha1_to_hex(r->list[i].sha1),
+				r->list[i].name);
+	}
+	return 0;
+}
+
+static void show_commit(struct commit *commit)
+{
+	write(1, sha1_to_hex(commit->object.sha1), 40);
+	write(1, "\n", 1);
+	if (commit->parents) {
+		free_commit_list(commit->parents);
+		commit->parents = NULL;
+	}
+}
+
+static void show_object(struct object_array_entry *p)
+{
+	/* An object with name "foo\n0000000..." can be used to
+	 *          * confuse downstream git-pack-objects very badly.
+	 *                   */
+	const char *ep = strchr(p->name, '\n');
+	int len = ep ? ep - p->name : strlen(p->name);
+	write(1, sha1_to_hex(p->item->sha1), 40);
+	write(1, " ", 1);
+	if (len)
+		write(1, p->name, len);
+	write(1, "\n", 1);
+}
+
+static int create_bundle(struct bundle_header *header, const char *path,
+		int argc, const char **argv)
+{
+	int bundle_fd = -1;
+	const char **argv_boundary = xmalloc((argc + 3) * sizeof(const char *));
+	const char **argv_pack = xmalloc(4 * sizeof(const char *));
+	int pid, in, out, i, status;
+	char buffer[1024];
+	struct rev_info revs;
+
+	bundle_fd = (!strcmp(path, "-") ? 1 :
+			open(path, O_CREAT | O_WRONLY, 0666));
+	if (bundle_fd < 0)
+		return error("Could not write to '%s'", path);
+
+	/* write signature */
+	write(bundle_fd, bundle_signature, strlen(bundle_signature));
+
+	/* write prerequisites */
+	memcpy(argv_boundary + 2, argv + 1, argc * sizeof(const char *));
+	argv_boundary[0] = "rev-list";
+	argv_boundary[1] = "--boundary";
+	argv_boundary[argc + 1] = NULL;
+	out = -1;
+	pid = fork_with_pipe(argv_boundary, NULL, &out);
+	if (pid < 0)
+		return -1;
+	while ((i = read_string(out, buffer, sizeof(buffer))) > 0)
+		if (buffer[0] == '-')
+			write(bundle_fd, buffer, i);
+	while ((i = waitpid(pid, &status, 0)) < 0)
+		if (errno != EINTR)
+			return error("rev-list died");
+	if (!WIFEXITED(status) || WEXITSTATUS(status))
+		return error("rev-list died %d", WEXITSTATUS(status));
+
+	/* write references */
+	save_commit_buffer = 0;
+	init_revisions(&revs, NULL);
+	revs.tag_objects = 1;
+	revs.tree_objects = 1;
+	revs.blob_objects = 1;
+	argc = setup_revisions(argc, argv, &revs, NULL);
+	if (argc > 1)
+		return error("unrecognized argument: %s'", argv[1]);
+	for (i = 0; i < revs.pending.nr; i++) {
+		struct object_array_entry *e = revs.pending.objects + i;
+		if (!(e->item->flags & UNINTERESTING)) {
+			unsigned char sha1[20];
+			char *ref;
+			if (dwim_ref(e->name, strlen(e->name), sha1, &ref) != 1)
+				continue;
+			write(bundle_fd, sha1_to_hex(e->item->sha1), 40);
+			write(bundle_fd, " ", 1);
+			write(bundle_fd, ref, strlen(ref));
+			write(bundle_fd, "\n", 1);
+			free(ref);
+		}
+	}
+
+	/* end header */
+	write(bundle_fd, "\n", 1);
+
+	/* write pack */
+	argv_pack[0] = "pack-objects";
+	argv_pack[1] = "--all-progress";
+	argv_pack[2] = "--stdout";
+	argv_pack[3] = NULL;
+	in = -1;
+	out = bundle_fd;
+	pid = fork_with_pipe(argv_pack, &in, &out);
+	if (pid < 0)
+		return error("Could not spawn pack-objects");
+	close(1);
+	close(bundle_fd);
+	dup2(in, 1);
+	close(in);
+	prepare_revision_walk(&revs);
+	traverse_commit_list(&revs, show_commit, show_object);
+	close(1);
+	while (waitpid(pid, &status, 0) < 0)
+		if (errno != EINTR)
+			return -1;
+	if (!WIFEXITED(status) || WEXITSTATUS(status))
+		return error ("pack-objects died");
+	return 0;
+}
+
+static int unbundle(struct bundle_header *header, int bundle_fd,
+		int argc, const char **argv)
+{
+	const char *argv_index_pack[] = {"index-pack", "--stdin", NULL};
+	int pid, status, dev_null;
+
+	if (verify_bundle(header))
+		return -1;
+	dev_null = open("/dev/null", O_WRONLY);
+	pid = fork_with_pipe(argv_index_pack, &bundle_fd, &dev_null);
+	if (pid < 0)
+		return error("Could not spawn index-pack");
+	close(bundle_fd);
+	while (waitpid(pid, &status, 0) < 0)
+		if (errno != EINTR)
+			return error("index-pack died");
+	if (!WIFEXITED(status) || WEXITSTATUS(status))
+		return error("index-pack exited with status %d",
+				WEXITSTATUS(status));
+	return list_heads(header, argc, argv);
+}
+
+int cmd_bundle(int argc, const char **argv, const char *prefix)
+{
+	struct bundle_header header;
+	int nongit = 0;
+	const char *cmd, *bundle_file;
+	int bundle_fd = -1;
+	char buffer[PATH_MAX];
+
+	if (argc < 3)
+		usage(bundle_usage);
+
+	cmd = argv[1];
+	bundle_file = argv[2];
+	argc -= 2;
+	argv += 2;
+
+	prefix = setup_git_directory_gently(&nongit);
+	if (prefix && bundle_file[0] != '/') {
+		snprintf(buffer, sizeof(buffer), "%s/%s", prefix, bundle_file);
+		bundle_file = buffer;
+	}
+
+	memset(&header, 0, sizeof(header));
+	if (strcmp(cmd, "create") &&
+			!(bundle_fd = read_header(bundle_file, &header)))
+		return 1;
+
+	if (!strcmp(cmd, "verify")) {
+		close(bundle_fd);
+		if (verify_bundle(&header))
+			return 1;
+		fprintf(stderr, "%s is okay\n", bundle_file);
+		return 0;
+	}
+	if (!strcmp(cmd, "list-heads")) {
+		close(bundle_fd);
+		return !!list_heads(&header, argc, argv);
+	}
+	if (!strcmp(cmd, "create")) {
+		if (nongit)
+			die("Need a repository to create a bundle.");
+		return !!create_bundle(&header, bundle_file, argc, argv);
+	} else if (!strcmp(cmd, "unbundle")) {
+		if (nongit)
+			die("Need a repository to unbundle.");
+		return !!unbundle(&header, bundle_fd, argc, argv);
+	} else
+		usage(bundle_usage);
+}
+
diff --git a/builtin.h b/builtin.h
index 57e8741..528074b 100644
--- a/builtin.h
+++ b/builtin.h
@@ -19,6 +19,7 @@ extern int cmd_apply(int argc, const char **argv, const char *prefix);
 extern int cmd_archive(int argc, const char **argv, const char *prefix);
 extern int cmd_blame(int argc, const char **argv, const char *prefix);
 extern int cmd_branch(int argc, const char **argv, const char *prefix);
+extern int cmd_bundle(int argc, const char **argv, const char *prefix);
 extern int cmd_cat_file(int argc, const char **argv, const char *prefix);
 extern int cmd_checkout_index(int argc, const char **argv, const char *prefix);
 extern int cmd_check_ref_format(int argc, const char **argv, const char *prefix);
diff --git a/git-fetch.sh b/git-fetch.sh
index 5d3fec0..5ae0d28 100755
--- a/git-fetch.sh
+++ b/git-fetch.sh
@@ -388,9 +388,16 @@ fetch_main () {
     ( : subshell because we muck with IFS
       IFS=" 	$LF"
       (
+	if test -f "$remote" ; then
+	    test -n "$shallow_depth" &&
+		die "shallow clone with bundle is not supported"
+	    git-bundle unbundle "$remote" $rref ||
+	    echo failed "$remote"
+	else
 	  git-fetch-pack --thin $exec $keep $shallow_depth $no_progress \
 		"$remote" $rref ||
 	  echo failed "$remote"
+	fi
       ) |
       (
 	trap '
diff --git a/git-ls-remote.sh b/git-ls-remote.sh
index 8ea5c5e..a6ed99a 100755
--- a/git-ls-remote.sh
+++ b/git-ls-remote.sh
@@ -89,8 +89,13 @@ rsync://* )
 	;;
 
 * )
-	git-peek-remote $exec "$peek_repo" ||
+	if test -f "$peek_repo" ; then
+		git bundle list-heads "$peek_repo" ||
 		echo "failed	slurping"
+	else
+		git-peek-remote $exec "$peek_repo" ||
+		echo "failed	slurping"
+	fi
 	;;
 esac |
 sort -t '	' -k 2 |
diff --git a/git.c b/git.c
index 83f3d90..a184d47 100644
--- a/git.c
+++ b/git.c
@@ -229,6 +229,7 @@ static void handle_internal_command(int argc, const char **argv, char **envp)
 		{ "archive", cmd_archive },
 		{ "blame", cmd_blame, RUN_SETUP },
 		{ "branch", cmd_branch, RUN_SETUP },
+		{ "bundle", cmd_bundle },
 		{ "cat-file", cmd_cat_file, RUN_SETUP },
 		{ "checkout-index", cmd_checkout_index, RUN_SETUP },
 		{ "check-ref-format", cmd_check_ref_format },
diff --git a/index-pack.c b/index-pack.c
index fa9a0e7..5ccf4c4 100644
--- a/index-pack.c
+++ b/index-pack.c
@@ -457,8 +457,8 @@ static void parse_pack_objects(unsigned char *sha1)
 	/* If input_fd is a file, we should have reached its end now. */
 	if (fstat(input_fd, &st))
 		die("cannot fstat packfile: %s", strerror(errno));
-	if (S_ISREG(st.st_mode) && st.st_size != consumed_bytes)
-		die("pack has junk at the end");
+	if (input_fd && S_ISREG(st.st_mode) && st.st_size != consumed_bytes)
+		die("pack has junk at the end: 0%o, %d, %d %d", st.st_mode, (int)st.st_size, (int)consumed_bytes, input_fd);
 
 	if (!nr_deltas)
 		return;
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 50c6485..fa76662 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -35,7 +35,9 @@ test_expect_success "clone and setup child repos" '
 		echo "URL: ../two/.git/"
 		echo "Pull: refs/heads/master:refs/heads/two"
 		echo "Pull: refs/heads/one:refs/heads/one"
-	} >.git/remotes/two
+	} >.git/remotes/two &&
+	cd .. &&
+	git clone . bundle
 '
 
 test_expect_success "fetch test" '
@@ -81,4 +83,28 @@ test_expect_success 'fetch following tags' '
 
 '
 
+test_expect_success 'create bundle 1' '
+	cd "$D" &&
+	echo >file updated again by origin &&
+	git commit -a -m "tip" &&
+	git bundle create bundle1 master^..master
+'
+
+test_expect_success 'create bundle 2' '
+	cd "$D" &&
+	git bundle create bundle2 master~2..master
+'
+
+test_expect_failure 'unbundle 1' '
+	cd "$D/bundle" &&
+	git checkout -b some-branch &&
+	git fetch "$D/bundle1" master:master
+'
+
+test_expect_success 'unbundle 2' '
+	cd "$D/bundle" &&
+	git fetch ../bundle2 master:master &&
+	test "tip" = "$(git log -1 --pretty=oneline master | cut -b42-)"
+'
+
 test_done
-- 
1.5.0.1.616.gc6c4-dirty

Re: [PATCH] Add git-bundle: move objects and references by archive

[Nicolas Pitre]

On Thu, 22 Feb 2007, Johannes Schindelin wrote:

> diff --git a/index-pack.c b/index-pack.c
> index fa9a0e7..5ccf4c4 100644
> --- a/index-pack.c
> +++ b/index-pack.c
> @@ -457,8 +457,8 @@ static void parse_pack_objects(unsigned char *sha1)
>  	/* If input_fd is a file, we should have reached its end now. */
>  	if (fstat(input_fd, &st))
>  		die("cannot fstat packfile: %s", strerror(errno));
> -	if (S_ISREG(st.st_mode) && st.st_size != consumed_bytes)
> -		die("pack has junk at the end");
> +	if (input_fd && S_ISREG(st.st_mode) && st.st_size != consumed_bytes)
> +		die("pack has junk at the end: 0%o, %d, %d %d", st.st_mode, (int)st.st_size, (int)consumed_bytes, input_fd);
>  
>  	if (!nr_deltas)
>  		return;

What is this supposed to mean?


Nicolas

Re: [PATCH] Add git-bundle: move objects and references by archive

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> It was updated to make git-bundle a builtin, and get rid of the tar 
> format: now, the first line is supposed to say "# v2 git bundle", the next 
> lines either contain a prerequisite ("-" followed by the hash of the 
> needed commit), or a ref (the hash of a commit, followed by the name of 
> the ref), and finally the pack. As a result, the bundle argument can be 
> "-" now.

That in BNF would be?

	bundle = signature prereq* ref* pack
	signature = "# v2 git bundle\n"
        prereq = "-" sha1 " " "\n"
        ref = sha1 " " name "\n"
	sha1 = [0-9a-f]{40}
	name = <name of ref>
	pack = <output stream from "pack-objects --stdout">

I suspect that we might want to possibly:

 - have checksum protection over the part before pack data?

 - give descriptive name to prereq, so that an error message can
   say "you need v1.4.4" instead of "you need e267c2f6"?

 - make the fields extensible without updating "v2"?

> diff --git a/Documentation/cmd-list.perl b/Documentation/cmd-list.perl
> index a2d6268..f61c77a 100755
> --- a/Documentation/cmd-list.perl
> +++ b/Documentation/cmd-list.perl
> @@ -70,6 +70,7 @@ git-archive                             mainporcelain
>  git-bisect                              mainporcelain
>  git-blame                               ancillaryinterrogators
>  git-branch                              mainporcelain
> +git-bundle                              mainporcelain
>  git-cat-file                            plumbinginterrogators
>  git-checkout-index                      plumbingmanipulators
>  git-checkout                            mainporcelain

Is this really a mainporcelain?
I would say ancillarymanipulators (or perhaps synchelpers).

> diff --git a/Documentation/git-bundle.txt b/Documentation/git-bundle.txt
> new file mode 100644
> index 0000000..4ea9e85
> ...
> +Author
> +------
> +Written by Mark Levedahl <mdl123@verizon.net>

... and Dscho.

> diff --git a/builtin-bundle.c b/builtin-bundle.c
> new file mode 100644
> index 0000000..4bd596a
> --- /dev/null
> +++ b/builtin-bundle.c
> ...
> +static const char *bundle_usage="git-bundle (--create <bundle> <git-rev-list args> | --verify <bundle> | --list-heads <bundle> [refname]... | --unbundle <bundle> [refname]... )";

I thought you removed "--" prefixes from subcommands.

> +struct bundle_header {
> +	struct ref_list prerequisites, references;
> +};

(minor style) I find these two members each on its own line
easier to read, as in:

        struct bundle_header {
                struct ref_list prerequisites;
                struct ref_list references;
        };

> +/* this function returns the length of the string */
> +static int read_string(int fd, char *buffer, int size)
> +{
> +	int i;
> +	for (i = 0; i < size - 1; i++) {
> +		int count = read(fd, buffer + i, 1);
> +		if (count < 0)
> +			return error("Read error: %s", strerror(errno));
> +		if (count == 0) {
> +			i--;
> +			break;
> +		}
> +		if (buffer[i] == '\n')
> +			break;
> +	}
> +	buffer[i + 1] = '\0';
> +	return i + 1;
> +}

At least xread() please.  I know the reason you read one byte at
a time is because you cannot over-read into pack area, but
somehow this makes me go "hmmmmmm".  It's not performance
critical so let's leave it that way.  Is bundle supposed to be
streamable?  Otherwise we could probably seek back.

> +/* returns an fd */
> +static int read_header(const char *path, struct bundle_header *header) {
> +	char buffer[1024];
> +	int fd = open(path, O_RDONLY);
> +
> +	if (fd < 0)
> +		return error("could not open '%s'", path);
> +	if (read_string(fd, buffer, sizeof(buffer)) < 0 ||
> +			strcmp(buffer, bundle_signature)) {
> +		close(fd);
> +		return error("'%s' does not look like a v2 bundle file", path);
> +	}
> +	while (read_string(fd, buffer, sizeof(buffer)) > 0
> +			&& buffer[0] != '\n') {
> +		int offset = buffer[0] == '-' ? 1 : 0;
> +		int len = strlen(buffer);
> +		unsigned char sha1[20];
> +		struct ref_list *list = offset ? &header->prerequisites
> +			: &header->references;
> +		if (get_sha1_hex(buffer + offset, sha1)) {
> +			close(fd);
> +			return error("invalid SHA1: %s", buffer);
> +		}
> +		if (buffer[len - 1] == '\n')
> +			buffer[len - 1] = '\0';
> +		add_to_ref_list(sha1, buffer + 41 + offset, list);
> +	}
> +	return fd;
> +}

Don't you want to look at and validate buffer[40+offset] in any
way, other than what get_sha1_hex() does (which is issspace())?

> +/* if in && *in >= 0, take that as input file descriptor instead */
> +static int fork_with_pipe(const char **argv, int *in, int *out)
> +{
> +	int needs_in, needs_out;
> +	int fdin[2], fdout[2], pid;
> +
> +	needs_in = in && *in < 0;
> +	if (needs_in) {
> +		if (pipe(fdin) < 0)
> +			return error("could not setup pipe");
> +		*in = fdin[1];
> +	}
> +
> +	needs_out = out && *out < 0;
> +	if (needs_out) {
> +		if (pipe(fdout) < 0)
> +			return error("could not setup pipe");
> +		*out = fdout[0];
> +	}
> +
> +	if ((pid = fork()) < 0) {
> +		if (needs_in) {
> +			close(fdin[0]);
> +			close(fdin[1]);
> +		}
> +		if (needs_out) {
> +			close(fdout[0]);
> +			close(fdout[1]);
> +		}
> +		return error("could not fork");
> +	}
> +	if (!pid) {
> +		if (needs_in) {
> +			dup2(fdin[0], 0);
> +			close(fdin[0]);
> +			close(fdin[1]);
> +		} else if (in)
> +			dup2(*in, 0);
> +		if (needs_out) {
> +			dup2(fdout[1], 1);
> +			close(fdout[0]);
> +			close(fdout[1]);
> +		} else if (out)
> +			dup2(*out, 1);
> +		exit(execv_git_cmd(argv));
> +	}

Do you need to close *in and *out that are given by the caller
after dup2() in the child?

> +static int verify_bundle(struct bundle_header *header)
> +{
> +	/*
> +	 * Do fast check, then if any prereqs are missing then go line by line
> +	 * to be verbose about the errors
> +	 */
> +	struct ref_list *p = &header->prerequisites;
> +	const char *argv[5] = {"rev-list", "--stdin", "--not", "--all", NULL};
> +	int pid, in, out, i, ret = 0;
> +	char buffer[1024];
> +
> +	in = out = -1;
> +	pid = fork_with_pipe(argv, &in, &out);
> +	if (pid < 0)
> +		return error("Could not fork rev-list");
> +	if (!fork()) {
> +		for (i = 0; i < p->nr; i++) {
> +			write(in, sha1_to_hex(p->list[i].sha1), 40);
> +			write(in, "\n", 1);
> +		}
> +		close(in);
> +		exit(0);
> +	}
> +	close(in);

What if write() fails?  That can happen when one of the objects
you feed here, or its parent objects, is missing from your
repository -- receiving rev-list would die() and the writing
child would sigpipe.

I also wonder who waits for this child...

> +	while (read_string(out, buffer, sizeof(buffer)) > 0) {
> +		if (ret++ == 0)
> +			error ("The bundle requires the following commits you lack:");
> +		fprintf(stderr, "%s", buffer);
> +	}
> +	close(out);

I do not think this error message would buy you anything.  If
you say:

	echo commit | rev-list --stdin --not --all

and you got commit back, that means you _do_ have that commit,
and that commit reaches some ref you have (objects associated to
that commit may be still missing).  If commit is truly missing,
then you do not get commit output from rev-list as it cannot
even determine if it is "--not --all" or not -- it would error
out, which is the more important check to see if prereqs are
lacking.

> +	while (waitpid(pid, &i, 0) < 0)
> +		if (errno != EINTR)
> +			return -1;
> +	if (!ret && (!WIFEXITED(i) || WEXITSTATUS(i)))
> +		return error("At least one prerequisite is lacking.");

So you would want to keep this error, perhaps even make it more
helpful by suggesting which ones are missing.

> +static int list_heads(struct bundle_header *header, int argc, const char **argv)
> +{
> +	int i;
> +	struct ref_list *r = &header->references;
> +
> +	for (i = 0; i < r->nr; i++) {
> +		if (argc > 1) {
> +			int j;
> +			for (j = 1; j < argc; j++)
> +				if (!strcmp(r->list[i].name, argv[j]))
> +					break;
> +			if (j == argc)
> +				continue;
> +		}
> +		printf("%s %s\n", sha1_to_hex(r->list[i].sha1),
> +				r->list[i].name);
> +	}
> +	return 0;
> +}

I wonder if we want to have a way to list prereqs in similar way.

> +static void show_commit(struct commit *commit)
> +{
> +	write(1, sha1_to_hex(commit->object.sha1), 40);
> +	write(1, "\n", 1);
> +	if (commit->parents) {
> +		free_commit_list(commit->parents);
> +		commit->parents = NULL;
> +	}
> +}

(everywhere) We would want write_in_full() with error handling
that dies even on EPIPE.

> +
> +static void show_object(struct object_array_entry *p)
> +{
> +	/* An object with name "foo\n0000000..." can be used to
> +	 *          * confuse downstream git-pack-objects very badly.
> +	 *                   */

An interesting way to wrap comments.

> +	/* write prerequisites */
> +	memcpy(argv_boundary + 2, argv + 1, argc * sizeof(const char *));
> +	argv_boundary[0] = "rev-list";
> +	argv_boundary[1] = "--boundary";
> +	argv_boundary[argc + 1] = NULL;
> +	out = -1;
> +	pid = fork_with_pipe(argv_boundary, NULL, &out);
> +	if (pid < 0)
> +		return -1;
> +	while ((i = read_string(out, buffer, sizeof(buffer))) > 0)
> +		if (buffer[0] == '-')
> +			write(bundle_fd, buffer, i);

It would be helpful for the recipient if you can append output
from git-describe (or name-rev) when the buffer lacks "name".

> +static int unbundle(struct bundle_header *header, int bundle_fd,
> +		int argc, const char **argv)
> +{
> +	const char *argv_index_pack[] = {"index-pack", "--stdin", NULL};
> +	int pid, status, dev_null;
> +
> +	if (verify_bundle(header))
> +		return -1;
> +	dev_null = open("/dev/null", O_WRONLY);
> +	pid = fork_with_pipe(argv_index_pack, &bundle_fd, &dev_null);
> +	if (pid < 0)
> +		return error("Could not spawn index-pack");
> +	close(bundle_fd);
> +	while (waitpid(pid, &status, 0) < 0)
> +		if (errno != EINTR)
> +			return error("index-pack died");
> +	if (!WIFEXITED(status) || WEXITSTATUS(status))
> +		return error("index-pack exited with status %d",
> +				WEXITSTATUS(status));
> +	return list_heads(header, argc, argv);
> +}

We might want to later use --keep for this as well...

> diff --git a/index-pack.c b/index-pack.c
> index fa9a0e7..5ccf4c4 100644
> --- a/index-pack.c
> +++ b/index-pack.c
> @@ -457,8 +457,8 @@ static void parse_pack_objects(unsigned char *sha1)
>  	/* If input_fd is a file, we should have reached its end now. */
>  	if (fstat(input_fd, &st))
>  		die("cannot fstat packfile: %s", strerror(errno));
> -	if (S_ISREG(st.st_mode) && st.st_size != consumed_bytes)
> -		die("pack has junk at the end");
> +	if (input_fd && S_ISREG(st.st_mode) && st.st_size != consumed_bytes)
> +		die("pack has junk at the end: 0%o, %d, %d %d", st.st_mode, (int)st.st_size, (int)consumed_bytes, input_fd);
>  
>  	if (!nr_deltas)
>  		return;

??

Re: [PATCH] Add git-bundle: move objects and references by archive

[Junio C Hamano]

Junio C Hamano <junkio@cox.net> writes:

>> +static int verify_bundle(struct bundle_header *header)
>> +{
>> +	/*
>> +	 * Do fast check, then if any prereqs are missing then go line by line
>> +	 * to be verbose about the errors
>> +	 */
>> +	struct ref_list *p = &header->prerequisites;
>> +	const char *argv[5] = {"rev-list", "--stdin", "--not", "--all", NULL};
>> +	int pid, in, out, i, ret = 0;
>> +	char buffer[1024];
>> +
>> +	in = out = -1;
>> +	pid = fork_with_pipe(argv, &in, &out);
>> +	if (pid < 0)
>> +		return error("Could not fork rev-list");
>> +	if (!fork()) {
>> +		for (i = 0; i < p->nr; i++) {
>> +			write(in, sha1_to_hex(p->list[i].sha1), 40);
>> +			write(in, "\n", 1);
>> +		}
>> +		close(in);
>> +		exit(0);
>> +	}
>> +	close(in);
>
> What if write() fails?  That can happen when one of the objects
> you feed here, or its parent objects, is missing from your
> repository -- receiving rev-list would die() and the writing
> child would sigpipe.
>
> I also wonder who waits for this child...

In general, fork() to avoid bidirectional pipe deadlock is a
good discipline, but in this particular case I think it would be
easier to handle errors if you don't (and it would save another
process).  The other side "rev-list --stdin --not --all" is
running a limited traversal, and would not emit anything until
you stop feeding it from --stdin, or until it dies because you
fed it a commit that does not exist.  So as long as you check
the error condition from write() for EPIPE to notice the other
end died, I think you are Ok.

Re: [PATCH] Add git-bundle: move objects and references by archive

[Johannes Sixt]

Johannes Schindelin wrote:
> +/* if in && *in >= 0, take that as input file descriptor instead */
> +static int fork_with_pipe(const char **argv, int *in, int *out)
> +{
> +       int needs_in, needs_out;
> +       int fdin[2], fdout[2], pid;
> +
> +       needs_in = in && *in < 0;
> +       if (needs_in) {
> +               if (pipe(fdin) < 0)
> +                       return error("could not setup pipe");
> +               *in = fdin[1];
> +       }
> +
> +       needs_out = out && *out < 0;
> +       if (needs_out) {
> +               if (pipe(fdout) < 0)
> +                       return error("could not setup pipe");
> +               *out = fdout[0];
> +       }
> +
> +       if ((pid = fork()) < 0) {
> +               if (needs_in) {
> +                       close(fdin[0]);
> +                       close(fdin[1]);
> +               }
> +               if (needs_out) {
> +                       close(fdout[0]);
> +                       close(fdout[1]);
> +               }
> +               return error("could not fork");
> +       }
> +       if (!pid) {
> +               if (needs_in) {
> +                       dup2(fdin[0], 0);
> +                       close(fdin[0]);
> +                       close(fdin[1]);
> +               } else if (in)
> +                       dup2(*in, 0);
> +               if (needs_out) {
> +                       dup2(fdout[1], 1);
> +                       close(fdout[0]);
> +                       close(fdout[1]);
> +               } else if (out)
> +                       dup2(*out, 1);
> +               exit(execv_git_cmd(argv));
> +       }
> +       if (needs_in)
> +               close(fdin[0]);
> +       if (needs_out)
> +               close(fdout[1]);
> +       return pid;
> +}

This function looks very similar to spawnvppe, which I use in the MinGW
port for a number of fork/exec pairs. Do you see a chance to make this
into a helper that can be used in more places (so that it reduces the
differences to the MinGW code)?

> +       in = out = -1;
> +       pid = fork_with_pipe(argv, &in, &out);
> +       if (pid < 0)
> +               return error("Could not fork rev-list");
> +       if (!fork()) {
> +               for (i = 0; i < p->nr; i++) {
> +                       write(in, sha1_to_hex(p->list[i].sha1), 40);
> +                       write(in, "\n", 1);
> +               }
> +               close(in);
> +               exit(0);
> +       }
> +       close(in);

Oops, fork error check missing?

-- Hannes

Re: [PATCH] Add git-bundle: move objects and references by archive

[Johannes Schindelin]

Hi,

On Wed, 21 Feb 2007, Nicolas Pitre wrote:

> On Thu, 22 Feb 2007, Johannes Schindelin wrote:
> 
> > diff --git a/index-pack.c b/index-pack.c
> > index fa9a0e7..5ccf4c4 100644
> > --- a/index-pack.c
> > +++ b/index-pack.c
> > @@ -457,8 +457,8 @@ static void parse_pack_objects(unsigned char *sha1)
> >  	/* If input_fd is a file, we should have reached its end now. */
> >  	if (fstat(input_fd, &st))
> >  		die("cannot fstat packfile: %s", strerror(errno));
> > -	if (S_ISREG(st.st_mode) && st.st_size != consumed_bytes)
> > -		die("pack has junk at the end");
> > +	if (input_fd && S_ISREG(st.st_mode) && st.st_size != consumed_bytes)
> > +		die("pack has junk at the end: 0%o, %d, %d %d", st.st_mode, (int)st.st_size, (int)consumed_bytes, input_fd);
> >  
> >  	if (!nr_deltas)
> >  		return;
> 
> What is this supposed to mean?

The funny thing is, if you stream part of the bundle file to index-pack, 
S_ISREG(st.st_mode) is true, even if input_fd == 0.

Then, because it is only part of the bundle file, the size check fails.

Ciao,
Dscho

Re: [PATCH] Add git-bundle: move objects and references by archive

[Simon 'corecode' Schubert]

[-- Attachment #1: Type: text/plain, Size: 1804 bytes --]

Johannes Schindelin wrote:
> Hi,
> 
> On Wed, 21 Feb 2007, Nicolas Pitre wrote:
> 
>> On Thu, 22 Feb 2007, Johannes Schindelin wrote:
>>
>>> diff --git a/index-pack.c b/index-pack.c
>>> index fa9a0e7..5ccf4c4 100644
>>> --- a/index-pack.c
>>> +++ b/index-pack.c
>>> @@ -457,8 +457,8 @@ static void parse_pack_objects(unsigned char *sha1)
>>>  	/* If input_fd is a file, we should have reached its end now. */
>>>  	if (fstat(input_fd, &st))
>>>  		die("cannot fstat packfile: %s", strerror(errno));
>>> -	if (S_ISREG(st.st_mode) && st.st_size != consumed_bytes)
>>> -		die("pack has junk at the end");
>>> +	if (input_fd && S_ISREG(st.st_mode) && st.st_size != consumed_bytes)
>>> +		die("pack has junk at the end: 0%o, %d, %d %d", st.st_mode, (int)st.st_size, (int)consumed_bytes, input_fd);
>>>  
>>>  	if (!nr_deltas)
>>>  		return;
>> What is this supposed to mean?
> 
> The funny thing is, if you stream part of the bundle file to index-pack, 
> S_ISREG(st.st_mode) is true, even if input_fd == 0.

Well, of course:  you opened a regular file and pass this as stdin to index-pack.

Maybe something like this would be cleaner:

if (IS_REF(st.st_mode) && lseek(input_fd, 0, SEEK_CUR) != st.st_size)
	die("...");

Because the point isn't actually that we consumed less bytes than the file's size, but that there is still data available after we are done with handling the pack.

Anyways, is this a reason to die(), or rather just a remark?

cheers
  simon

-- 
Serve - BSD     +++  RENT this banner advert  +++    ASCII Ribbon   /"\
Work - Mac      +++  space for low €€€ NOW!1  +++      Campaign     \ /
Party Enjoy Relax   |   http://dragonflybsd.org      Against  HTML   \
Dude 2c 2 the max   !   http://golden-apple.biz       Mail + News   / \


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: [PATCH] Add git-bundle: move objects and references by archive

[Johannes Schindelin]

Hi,

On Wed, 21 Feb 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > It was updated to make git-bundle a builtin, and get rid of the tar 
> > format: now, the first line is supposed to say "# v2 git bundle", the next 
> > lines either contain a prerequisite ("-" followed by the hash of the 
> > needed commit), or a ref (the hash of a commit, followed by the name of 
> > the ref), and finally the pack. As a result, the bundle argument can be 
> > "-" now.
> 
> That in BNF would be?
> 
> 	bundle = signature prereq* ref* pack

more like

	bundle = header pack
	header = signature prereq* ref* nl

> I suspect that we might want to possibly:
> 
>  - have checksum protection over the part before pack data?

Possibly. But would the sha1's not be sufficient? I.e. if the header is 
corrupted, chances are that the commits are no longer verified correctly.

>  - give descriptive name to prereq, so that an error message can
>    say "you need v1.4.4" instead of "you need e267c2f6"?

My idea was to allow --since=<date>. You don't have a name there.

>  - make the fields extensible without updating "v2"?

You mean, like, warn about unknown header lines? Yes, that's really easy.

> > diff --git a/Documentation/cmd-list.perl b/Documentation/cmd-list.perl
> > index a2d6268..f61c77a 100755
> > --- a/Documentation/cmd-list.perl
> > +++ b/Documentation/cmd-list.perl
> > @@ -70,6 +70,7 @@ git-archive                             mainporcelain
> >  git-bisect                              mainporcelain
> >  git-blame                               ancillaryinterrogators
> >  git-branch                              mainporcelain
> > +git-bundle                              mainporcelain
> >  git-cat-file                            plumbinginterrogators
> >  git-checkout-index                      plumbingmanipulators
> >  git-checkout                            mainporcelain
> 
> Is this really a mainporcelain?

git bundle create <file>?

> > diff --git a/Documentation/git-bundle.txt b/Documentation/git-bundle.txt
> > new file mode 100644
> > index 0000000..4ea9e85
> > ...
> > +Author
> > +------
> > +Written by Mark Levedahl <mdl123@verizon.net>
> 
> ... and Dscho.

... not the Documentation (only patched a little to reflect the "--" 
change).

> > diff --git a/builtin-bundle.c b/builtin-bundle.c
> > new file mode 100644
> > index 0000000..4bd596a
> > --- /dev/null
> > +++ b/builtin-bundle.c
> > ...
> > +static const char *bundle_usage="git-bundle (--create <bundle> <git-rev-list args> | --verify <bundle> | --list-heads <bundle> [refname]... | --unbundle <bundle> [refname]... )";
> 
> I thought you removed "--" prefixes from subcommands.

Yes, and I forgot it there. Will fix.

> > +struct bundle_header {
> > +	struct ref_list prerequisites, references;
> > +};
> 
> (minor style) I find these two members each on its own line
> easier to read, as in:
> 
>         struct bundle_header {
>                 struct ref_list prerequisites;
>                 struct ref_list references;
>         };

Will fix.

> > +/* this function returns the length of the string */
> > +static int read_string(int fd, char *buffer, int size)
> > +{
> > +	int i;
> > +	for (i = 0; i < size - 1; i++) {
> > +		int count = read(fd, buffer + i, 1);
> > +		if (count < 0)
> > +			return error("Read error: %s", strerror(errno));
> > +		if (count == 0) {
> > +			i--;
> > +			break;
> > +		}
> > +		if (buffer[i] == '\n')
> > +			break;
> > +	}
> > +	buffer[i + 1] = '\0';
> > +	return i + 1;
> > +}
> 
> At least xread() please.  I know the reason you read one byte at
> a time is because you cannot over-read into pack area, but
> somehow this makes me go "hmmmmmm".  It's not performance
> critical so let's leave it that way.  Is bundle supposed to be
> streamable?  Otherwise we could probably seek back.

I wanted unbundle to be able to read from stdin (I did not yet allow "git 
fetch - master:master"). But I somehow forgot about xread(). D'oh. Will 
fix.

> > +/* returns an fd */
> > +static int read_header(const char *path, struct bundle_header *header) {
> > +	char buffer[1024];
> > +	int fd = open(path, O_RDONLY);
> > +
> > +	if (fd < 0)
> > +		return error("could not open '%s'", path);
> > +	if (read_string(fd, buffer, sizeof(buffer)) < 0 ||
> > +			strcmp(buffer, bundle_signature)) {
> > +		close(fd);
> > +		return error("'%s' does not look like a v2 bundle file", path);
> > +	}
> > +	while (read_string(fd, buffer, sizeof(buffer)) > 0
> > +			&& buffer[0] != '\n') {
> > +		int offset = buffer[0] == '-' ? 1 : 0;
> > +		int len = strlen(buffer);
> > +		unsigned char sha1[20];
> > +		struct ref_list *list = offset ? &header->prerequisites
> > +			: &header->references;
> > +		if (get_sha1_hex(buffer + offset, sha1)) {
> > +			close(fd);
> > +			return error("invalid SHA1: %s", buffer);
> > +		}
> > +		if (buffer[len - 1] == '\n')
> > +			buffer[len - 1] = '\0';
> > +		add_to_ref_list(sha1, buffer + 41 + offset, list);
> > +	}
> > +	return fd;
> > +}
> 
> Don't you want to look at and validate buffer[40+offset] in any
> way, other than what get_sha1_hex() does (which is issspace())?

Right. Will fix. (Although I will only write two "s" instead of three...)

> > +/* if in && *in >= 0, take that as input file descriptor instead */
> > +static int fork_with_pipe(const char **argv, int *in, int *out)
> > +{
> > +	int needs_in, needs_out;
> > +	int fdin[2], fdout[2], pid;
> > +
> > +	needs_in = in && *in < 0;
> > +	if (needs_in) {
> > +		if (pipe(fdin) < 0)
> > +			return error("could not setup pipe");
> > +		*in = fdin[1];
> > +	}
> > +
> > +	needs_out = out && *out < 0;
> > +	if (needs_out) {
> > +		if (pipe(fdout) < 0)
> > +			return error("could not setup pipe");
> > +		*out = fdout[0];
> > +	}
> > +
> > +	if ((pid = fork()) < 0) {
> > +		if (needs_in) {
> > +			close(fdin[0]);
> > +			close(fdin[1]);
> > +		}
> > +		if (needs_out) {
> > +			close(fdout[0]);
> > +			close(fdout[1]);
> > +		}
> > +		return error("could not fork");
> > +	}
> > +	if (!pid) {
> > +		if (needs_in) {
> > +			dup2(fdin[0], 0);
> > +			close(fdin[0]);
> > +			close(fdin[1]);
> > +		} else if (in)
> > +			dup2(*in, 0);
> > +		if (needs_out) {
> > +			dup2(fdout[1], 1);
> > +			close(fdout[0]);
> > +			close(fdout[1]);
> > +		} else if (out)
> > +			dup2(*out, 1);
> > +		exit(execv_git_cmd(argv));
> > +	}
> 
> Do you need to close *in and *out that are given by the caller
> after dup2() in the child?

I probably should. Will fix.

> > +static int verify_bundle(struct bundle_header *header)
> > +{
> > +	/*
> > +	 * Do fast check, then if any prereqs are missing then go line by line
> > +	 * to be verbose about the errors
> > +	 */
> > +	struct ref_list *p = &header->prerequisites;
> > +	const char *argv[5] = {"rev-list", "--stdin", "--not", "--all", NULL};
> > +	int pid, in, out, i, ret = 0;
> > +	char buffer[1024];
> > +
> > +	in = out = -1;
> > +	pid = fork_with_pipe(argv, &in, &out);
> > +	if (pid < 0)
> > +		return error("Could not fork rev-list");
> > +	if (!fork()) {
> > +		for (i = 0; i < p->nr; i++) {
> > +			write(in, sha1_to_hex(p->list[i].sha1), 40);
> > +			write(in, "\n", 1);
> > +		}
> > +		close(in);
> > +		exit(0);
> > +	}
> > +	close(in);
> 
> What if write() fails?  That can happen when one of the objects
> you feed here, or its parent objects, is missing from your
> repository -- receiving rev-list would die() and the writing
> child would sigpipe.

The error is caught afterwards, with WEXITSTATUS(), no? It might be 
cleaner to check, but is it really necessary?

> I also wonder who waits for this child...

rev-list. It only exits when that child closes the input.

> > +	while (read_string(out, buffer, sizeof(buffer)) > 0) {
> > +		if (ret++ == 0)
> > +			error ("The bundle requires the following commits you lack:");
> > +		fprintf(stderr, "%s", buffer);
> > +	}
> > +	close(out);
> 
> I do not think this error message would buy you anything.  If
> you say:
> 
> 	echo commit | rev-list --stdin --not --all
> 
> and you got commit back, that means you _do_ have that commit,
> and that commit reaches some ref you have (objects associated to
> that commit may be still missing).  If commit is truly missing,
> then you do not get commit output from rev-list as it cannot
> even determine if it is "--not --all" or not -- it would error
> out, which is the more important check to see if prereqs are
> lacking.

Yes, I realized that rev-list die()s, and does not output the missing 
revs -- as git-bundle.sh expected it to. When I found out about that, I 
failed to remove that bogus error. Will fix.

> > +	while (waitpid(pid, &i, 0) < 0)
> > +		if (errno != EINTR)
> > +			return -1;
> > +	if (!ret && (!WIFEXITED(i) || WEXITSTATUS(i)))
> > +		return error("At least one prerequisite is lacking.");
> 
> So you would want to keep this error, perhaps even make it more
> helpful by suggesting which ones are missing.

rev-list die()s with the first missing. It does not continue, printing 
other missing revs.

> > +static int list_heads(struct bundle_header *header, int argc, const char **argv)
> > +{
> > +	int i;
> > +	struct ref_list *r = &header->references;
> > +
> > +	for (i = 0; i < r->nr; i++) {
> > +		if (argc > 1) {
> > +			int j;
> > +			for (j = 1; j < argc; j++)
> > +				if (!strcmp(r->list[i].name, argv[j]))
> > +					break;
> > +			if (j == argc)
> > +				continue;
> > +		}
> > +		printf("%s %s\n", sha1_to_hex(r->list[i].sha1),
> > +				r->list[i].name);
> > +	}
> > +	return 0;
> > +}
> 
> I wonder if we want to have a way to list prereqs in similar way.

That is what verify should be for. Maybe add "-v" to the "verify" 
subcommand, make it properly builtin, and output non-missing prerequisites 
only with "-v"?

> > +static void show_commit(struct commit *commit)
> > +{
> > +	write(1, sha1_to_hex(commit->object.sha1), 40);
> > +	write(1, "\n", 1);
> > +	if (commit->parents) {
> > +		free_commit_list(commit->parents);
> > +		commit->parents = NULL;
> > +	}
> > +}
> 
> (everywhere) We would want write_in_full() with error handling
> that dies even on EPIPE.

Or write_or_die()? I mean, if write() fails, we cannot sanely continue 
anyway, right?

> > +static void show_object(struct object_array_entry *p)
> > +{
> > +	/* An object with name "foo\n0000000..." can be used to
> > +	 *          * confuse downstream git-pack-objects very badly.
> > +	 *                   */
> 
> An interesting way to wrap comments.

Bad, bad vi. Will fix.

> > +	/* write prerequisites */
> > +	memcpy(argv_boundary + 2, argv + 1, argc * sizeof(const char *));
> > +	argv_boundary[0] = "rev-list";
> > +	argv_boundary[1] = "--boundary";
> > +	argv_boundary[argc + 1] = NULL;
> > +	out = -1;
> > +	pid = fork_with_pipe(argv_boundary, NULL, &out);
> > +	if (pid < 0)
> > +		return -1;
> > +	while ((i = read_string(out, buffer, sizeof(buffer))) > 0)
> > +		if (buffer[0] == '-')
> > +			write(bundle_fd, buffer, i);
> 
> It would be helpful for the recipient if you can append output
> >from git-describe (or name-rev) when the buffer lacks "name".

Hmm. This could take a long time, as -describe is not really fast. ATM 
name-rev is not fast either, but I have plans to make it so. So obiously, 
I would prefer name-rev output. Comments?

> > +static int unbundle(struct bundle_header *header, int bundle_fd,
> > +		int argc, const char **argv)
> > +{
> > +	const char *argv_index_pack[] = {"index-pack", "--stdin", NULL};
> > +	int pid, status, dev_null;
> > +
> > +	if (verify_bundle(header))
> > +		return -1;
> > +	dev_null = open("/dev/null", O_WRONLY);
> > +	pid = fork_with_pipe(argv_index_pack, &bundle_fd, &dev_null);
> > +	if (pid < 0)
> > +		return error("Could not spawn index-pack");
> > +	close(bundle_fd);
> > +	while (waitpid(pid, &status, 0) < 0)
> > +		if (errno != EINTR)
> > +			return error("index-pack died");
> > +	if (!WIFEXITED(status) || WEXITSTATUS(status))
> > +		return error("index-pack exited with status %d",
> > +				WEXITSTATUS(status));
> > +	return list_heads(header, argc, argv);
> > +}
> 
> We might want to later use --keep for this as well...

Yes, later. It is such a low-hanging fruit that I'd prefer somebody else 
to get involved with the code.

> > diff --git a/index-pack.c b/index-pack.c
> > index fa9a0e7..5ccf4c4 100644
> > --- a/index-pack.c
> > +++ b/index-pack.c
> > @@ -457,8 +457,8 @@ static void parse_pack_objects(unsigned char *sha1)
> >  	/* If input_fd is a file, we should have reached its end now. */
> >  	if (fstat(input_fd, &st))
> >  		die("cannot fstat packfile: %s", strerror(errno));
> > -	if (S_ISREG(st.st_mode) && st.st_size != consumed_bytes)
> > -		die("pack has junk at the end");
> > +	if (input_fd && S_ISREG(st.st_mode) && st.st_size != consumed_bytes)
> > +		die("pack has junk at the end: 0%o, %d, %d %d", st.st_mode, (int)st.st_size, (int)consumed_bytes, input_fd);
> >  
> >  	if (!nr_deltas)
> >  		return;
> 
> ??

As I said in my reply to Nico, if input_fd is 0, but really comes from a 
file, this check will fail (the bundle is _strictly_ larger than the 
pack).

Would you like the fixes on top of the big commit, or a replacement patch?

Ciao,
Dscho

Re: [PATCH] Add git-bundle: move objects and references by archive

[Johannes Schindelin]

Hi,

On Wed, 21 Feb 2007, Junio C Hamano wrote:

> Junio C Hamano <junkio@cox.net> writes:
> 
> >> +static int verify_bundle(struct bundle_header *header)
> >> +{
> >> +	/*
> >> +	 * Do fast check, then if any prereqs are missing then go line by line
> >> +	 * to be verbose about the errors
> >> +	 */
> >> +	struct ref_list *p = &header->prerequisites;
> >> +	const char *argv[5] = {"rev-list", "--stdin", "--not", "--all", NULL};
> >> +	int pid, in, out, i, ret = 0;
> >> +	char buffer[1024];
> >> +
> >> +	in = out = -1;
> >> +	pid = fork_with_pipe(argv, &in, &out);
> >> +	if (pid < 0)
> >> +		return error("Could not fork rev-list");
> >> +	if (!fork()) {
> >> +		for (i = 0; i < p->nr; i++) {
> >> +			write(in, sha1_to_hex(p->list[i].sha1), 40);
> >> +			write(in, "\n", 1);
> >> +		}
> >> +		close(in);
> >> +		exit(0);
> >> +	}
> >> +	close(in);
> >
> > What if write() fails?  That can happen when one of the objects
> > you feed here, or its parent objects, is missing from your
> > repository -- receiving rev-list would die() and the writing
> > child would sigpipe.
> >
> > I also wonder who waits for this child...
> 
> In general, fork() to avoid bidirectional pipe deadlock is a
> good discipline, but in this particular case I think it would be
> easier to handle errors if you don't (and it would save another
> process).  The other side "rev-list --stdin --not --all" is
> running a limited traversal, and would not emit anything until
> you stop feeding it from --stdin, or until it dies because you
> fed it a commit that does not exist.  So as long as you check
> the error condition from write() for EPIPE to notice the other
> end died, I think you are Ok.

Thinking about this deeper, I have to say I find my decision to use 
"--stdin" rather silly, given that I know the exact number of revisions, 
and their SHA1s.

But it might make more sense to rewrite the checking part, instead of 
fork()ing it.

Ciao,
Dscho

Re: [PATCH] Add git-bundle: move objects and references by archive

[Nicolas Pitre]

On Thu, 22 Feb 2007, Johannes Schindelin wrote:

> Hi,
> 
> On Wed, 21 Feb 2007, Nicolas Pitre wrote:
> 
> > On Thu, 22 Feb 2007, Johannes Schindelin wrote:
> > 
> > > diff --git a/index-pack.c b/index-pack.c
> > > index fa9a0e7..5ccf4c4 100644
> > > --- a/index-pack.c
> > > +++ b/index-pack.c
> > > @@ -457,8 +457,8 @@ static void parse_pack_objects(unsigned char *sha1)
> > >  	/* If input_fd is a file, we should have reached its end now. */
> > >  	if (fstat(input_fd, &st))
> > >  		die("cannot fstat packfile: %s", strerror(errno));
> > > -	if (S_ISREG(st.st_mode) && st.st_size != consumed_bytes)
> > > -		die("pack has junk at the end");
> > > +	if (input_fd && S_ISREG(st.st_mode) && st.st_size != consumed_bytes)
> > > +		die("pack has junk at the end: 0%o, %d, %d %d", st.st_mode, (int)st.st_size, (int)consumed_bytes, input_fd);
> > >  
> > >  	if (!nr_deltas)
> > >  		return;
> > 
> > What is this supposed to mean?
> 
> The funny thing is, if you stream part of the bundle file to index-pack, 
> S_ISREG(st.st_mode) is true, even if input_fd == 0.

Hmmmm. indeed..

Could you please make the test, including the call to fstat(), 
conditional on !from_stdin instead?

Also I don't see the point of displaying the mode since we know that 
S_ISREG(st.st_mode) is true, and input_fd is of little interest as well.


Nicolas

Re: [PATCH] Add git-bundle: move objects and references by archive

[Nicolas Pitre]

On Thu, 22 Feb 2007, Simon 'corecode' Schubert wrote:

> Johannes Schindelin wrote:
> > Hi,
> > 
> > On Wed, 21 Feb 2007, Nicolas Pitre wrote:
> > 
> > > On Thu, 22 Feb 2007, Johannes Schindelin wrote:
> > > 
> > > > diff --git a/index-pack.c b/index-pack.c
> > > > index fa9a0e7..5ccf4c4 100644
> > > > --- a/index-pack.c
> > > > +++ b/index-pack.c
> > > > @@ -457,8 +457,8 @@ static void parse_pack_objects(unsigned char *sha1)
> > > >  	/* If input_fd is a file, we should have reached its end now. */
> > > >  	if (fstat(input_fd, &st))
> > > >  		die("cannot fstat packfile: %s", strerror(errno));
> > > > -	if (S_ISREG(st.st_mode) && st.st_size != consumed_bytes)
> > > > -		die("pack has junk at the end");
> > > > +	if (input_fd && S_ISREG(st.st_mode) && st.st_size != consumed_bytes)
> > > > +		die("pack has junk at the end: 0%o, %d, %d %d", st.st_mode,
> > > > (int)st.st_size, (int)consumed_bytes, input_fd);
> > > >   	if (!nr_deltas)
> > > >  		return;
> > > What is this supposed to mean?
> > 
> > The funny thing is, if you stream part of the bundle file to index-pack,
> > S_ISREG(st.st_mode) is true, even if input_fd == 0.
> 
> Well, of course:  you opened a regular file and pass this as stdin to
> index-pack.
> 
> Maybe something like this would be cleaner:
> 
> if (IS_REF(st.st_mode) && lseek(input_fd, 0, SEEK_CUR) != st.st_size)

No that won't work because the input is buffered in the fill() function.


Nicolas

Re: [PATCH] Add git-bundle: move objects and references by archive

[Johannes Schindelin]

Hi,

On Thu, 22 Feb 2007, Simon 'corecode' Schubert wrote:

> Johannes Schindelin wrote:
> > Hi,
> > 
> > On Wed, 21 Feb 2007, Nicolas Pitre wrote:
> > 
> > > On Thu, 22 Feb 2007, Johannes Schindelin wrote:
> > > 
> > > > diff --git a/index-pack.c b/index-pack.c
> > > > index fa9a0e7..5ccf4c4 100644
> > > > --- a/index-pack.c
> > > > +++ b/index-pack.c
> > > > @@ -457,8 +457,8 @@ static void parse_pack_objects(unsigned char *sha1)
> > > >  	/* If input_fd is a file, we should have reached its end now. */
> > > >  	if (fstat(input_fd, &st))
> > > >  		die("cannot fstat packfile: %s", strerror(errno));
> > > > -	if (S_ISREG(st.st_mode) && st.st_size != consumed_bytes)
> > > > -		die("pack has junk at the end");
> > > > +	if (input_fd && S_ISREG(st.st_mode) && st.st_size != consumed_bytes)
> > > > +		die("pack has junk at the end: 0%o, %d, %d %d", st.st_mode,
> > > > (int)st.st_size, (int)consumed_bytes, input_fd);
> > > >   	if (!nr_deltas)
> > > >  		return;
> > > What is this supposed to mean?
> > 
> > The funny thing is, if you stream part of the bundle file to index-pack,
> > S_ISREG(st.st_mode) is true, even if input_fd == 0.
> 
> Well, of course:  you opened a regular file and pass this as stdin to
> index-pack.
> 
> Maybe something like this would be cleaner:
> 
> if (IS_REF(st.st_mode) && lseek(input_fd, 0, SEEK_CUR) != st.st_size)
> 	die("...");

The lseek would fail whenever the input is _not_ a file, dying. Since 
index-pack is called from fetch-pack, with a socket instead of a file, it 
would fail for the most common user.

My patch was only a minimal fixup to allow "--stdin" even starting in the 
middle of a file, and it does not break fetching.

Ciao,
Dscho

Re: [PATCH] Add git-bundle: move objects and references by archive

[Simon 'corecode' Schubert]

[-- Attachment #1: Type: text/plain, Size: 2048 bytes --]

Johannes Schindelin wrote:
> Hi,
> 
> On Thu, 22 Feb 2007, Simon 'corecode' Schubert wrote:
> 
>> Johannes Schindelin wrote:
>>> Hi,
>>>
>>> On Wed, 21 Feb 2007, Nicolas Pitre wrote:
>>>
>>>> On Thu, 22 Feb 2007, Johannes Schindelin wrote:
>>>>
>>>>> diff --git a/index-pack.c b/index-pack.c
>>>>> index fa9a0e7..5ccf4c4 100644
>>>>> --- a/index-pack.c
>>>>> +++ b/index-pack.c
>>>>> @@ -457,8 +457,8 @@ static void parse_pack_objects(unsigned char *sha1)
>>>>>  	/* If input_fd is a file, we should have reached its end now. */
>>>>>  	if (fstat(input_fd, &st))
>>>>>  		die("cannot fstat packfile: %s", strerror(errno));
>>>>> -	if (S_ISREG(st.st_mode) && st.st_size != consumed_bytes)
>>>>> -		die("pack has junk at the end");
>>>>> +	if (input_fd && S_ISREG(st.st_mode) && st.st_size != consumed_bytes)
>>>>> +		die("pack has junk at the end: 0%o, %d, %d %d", st.st_mode,
>>>>> (int)st.st_size, (int)consumed_bytes, input_fd);
>>>>>   	if (!nr_deltas)
>>>>>  		return;
>>>> What is this supposed to mean?
>>> The funny thing is, if you stream part of the bundle file to index-pack,
>>> S_ISREG(st.st_mode) is true, even if input_fd == 0.
>> Well, of course:  you opened a regular file and pass this as stdin to
>> index-pack.
>>
>> Maybe something like this would be cleaner:
>>
>> if (IS_REF(st.st_mode) && lseek(input_fd, 0, SEEK_CUR) != st.st_size)
>> 	die("...");
> 
> The lseek would fail whenever the input is _not_ a file, dying. Since 
> index-pack is called from fetch-pack, with a socket instead of a file, it 
> would fail for the most common user.

that's why i put IS_REF (should read IS_REG) there.

but as nicolas pointed out, this won't work for read-ahead.

cheers
  simon

-- 
Serve - BSD     +++  RENT this banner advert  +++    ASCII Ribbon   /"\
Work - Mac      +++  space for low €€€ NOW!1  +++      Campaign     \ /
Party Enjoy Relax   |   http://dragonflybsd.org      Against  HTML   \
Dude 2c 2 the max   !   http://golden-apple.biz       Mail + News   / \


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: [PATCH] Add git-bundle: move objects and references by archive

[Johannes Schindelin]

Hi,

On Thu, 22 Feb 2007, Simon 'corecode' Schubert wrote:

> Johannes Schindelin wrote:
> > 
> > On Thu, 22 Feb 2007, Simon 'corecode' Schubert wrote:
> > 
> > > Maybe something like this would be cleaner:
> > > 
> > > if (IS_REF(st.st_mode) && lseek(input_fd, 0, SEEK_CUR) != st.st_size)
> > > 	die("...");
> > 
> > The lseek would fail whenever the input is _not_ a file, dying. Since
> > index-pack is called from fetch-pack, with a socket instead of a file, it
> > would fail for the most common user.
> 
> that's why i put IS_REF (should read IS_REG) there.

Ah, yes, I understand.

> but as nicolas pointed out, this won't work for read-ahead.

Well, you could do this:

if (S_ISREG(st.st_mode) &&
		lseek(input_fd, 0, SEEK_CUR) - input_len != st.st_size)
	die("...");

Of course, this suggests that a file containing a pack must not have 
_anything_ after the pack. But AFAIR the reading part chokes on "garbage 
after pack" anyway.

Ciao,
Dscho

Re: [PATCH] Add git-bundle: move objects and references by archive

[Johannes Schindelin]

Hi,

On Thu, 22 Feb 2007, Nicolas Pitre wrote:

> On Thu, 22 Feb 2007, Johannes Schindelin wrote:
> 
> > Hi,
> > 
> > On Wed, 21 Feb 2007, Nicolas Pitre wrote:
> > 
> > > On Thu, 22 Feb 2007, Johannes Schindelin wrote:
> > > 
> > > > diff --git a/index-pack.c b/index-pack.c
> > > > index fa9a0e7..5ccf4c4 100644
> > > > --- a/index-pack.c
> > > > +++ b/index-pack.c
> > > > @@ -457,8 +457,8 @@ static void parse_pack_objects(unsigned char *sha1)
> > > >  	/* If input_fd is a file, we should have reached its end now. */
> > > >  	if (fstat(input_fd, &st))
> > > >  		die("cannot fstat packfile: %s", strerror(errno));
> > > > -	if (S_ISREG(st.st_mode) && st.st_size != consumed_bytes)
> > > > -		die("pack has junk at the end");
> > > > +	if (input_fd && S_ISREG(st.st_mode) && st.st_size != consumed_bytes)
> > > > +		die("pack has junk at the end: 0%o, %d, %d %d", st.st_mode, (int)st.st_size, (int)consumed_bytes, input_fd);
> > > >  
> > > >  	if (!nr_deltas)
> > > >  		return;
> > > 
> > > What is this supposed to mean?
> > 
> > The funny thing is, if you stream part of the bundle file to index-pack, 
> > S_ISREG(st.st_mode) is true, even if input_fd == 0.
> 
> Hmmmm. indeed..
> 
> Could you please make the test, including the call to fstat(), 
> conditional on !from_stdin instead?

How about using Simon's idea instead (subtracting input_len)?

> Also I don't see the point of displaying the mode since we know that 
> S_ISREG(st.st_mode) is true, and input_fd is of little interest as well.

As you probably guessed, these are debug remnants. Will fix.

Ciao,
Dscho

Re: [PATCH] Add git-bundle: move objects and references by archive

[Nicolas Pitre]

On Thu, 22 Feb 2007, Johannes Schindelin wrote:

> On Thu, 22 Feb 2007, Nicolas Pitre wrote:
> 
> > Could you please make the test, including the call to fstat(), 
> > conditional on !from_stdin instead?
> 
> How about using Simon's idea instead (subtracting input_len)?

Yes it would work.


Nicolas

[PATCH] git-bundle: assorted fixes

[Johannes Schindelin]

This patch fixes issues mentioned by Junio, Nico and Simon:

- I forgot to convert the usage string when removing the "--" from
  the subcommands,
- a style fix in the bundle_header,
- use xread() instead of read(),
- use write_or_die() instead of write(),
- make the bundle header extensible,
- fail if the whitespace after a sha1 of a reference is missing,
- close() the fds passed to a subprocess,
- in verify_bundle(), do not use "rev-list --stdin", but rather
  pass the revs directly (avoiding a fork()),
- fix a corrupted comment in show_object(), and
- fix the size check in index_pack.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin-bundle.c |  107 ++++++++++++++++++++++++++++++------------------------
 index-pack.c     |    5 ++-
 2 files changed, 63 insertions(+), 49 deletions(-)

diff --git a/builtin-bundle.c b/builtin-bundle.c
index 4bd596a..521bbda 100644
--- a/builtin-bundle.c
+++ b/builtin-bundle.c
@@ -13,7 +13,7 @@
  * bundle supporting git-fetch, git-pull, and git-ls-remote
  */
 
-static const char *bundle_usage="git-bundle (--create <bundle> <git-rev-list args> | --verify <bundle> | --list-heads <bundle> [refname]... | --unbundle <bundle> [refname]... )";
+static const char *bundle_usage="git-bundle (create <bundle> <git-rev-list args> | verify <bundle> | list-heads <bundle> [refname]... | unbundle <bundle> [refname]... )";
 
 static const char bundle_signature[] = "# v2 git bundle\n";
 
@@ -39,7 +39,8 @@ static void add_to_ref_list(const unsigned char *sha1, const char *name,
 }
 
 struct bundle_header {
-	struct ref_list prerequisites, references;
+	struct ref_list prerequisites;
+	struct ref_list references;
 };
 
 /* this function returns the length of the string */
@@ -47,7 +48,7 @@ static int read_string(int fd, char *buffer, int size)
 {
 	int i;
 	for (i = 0; i < size - 1; i++) {
-		int count = read(fd, buffer + i, 1);
+		int count = xread(fd, buffer + i, 1);
 		if (count < 0)
 			return error("Read error: %s", strerror(errno));
 		if (count == 0) {
@@ -75,18 +76,25 @@ static int read_header(const char *path, struct bundle_header *header) {
 	}
 	while (read_string(fd, buffer, sizeof(buffer)) > 0
 			&& buffer[0] != '\n') {
-		int offset = buffer[0] == '-' ? 1 : 0;
+		int is_prereq = buffer[0] == '-';
+		int offset = is_prereq ? 1 : 0;
 		int len = strlen(buffer);
 		unsigned char sha1[20];
-		struct ref_list *list = offset ? &header->prerequisites
+		struct ref_list *list = is_prereq ? &header->prerequisites
 			: &header->references;
-		if (get_sha1_hex(buffer + offset, sha1)) {
-			close(fd);
-			return error("invalid SHA1: %s", buffer);
-		}
+		char delim;
+
 		if (buffer[len - 1] == '\n')
 			buffer[len - 1] = '\0';
-		add_to_ref_list(sha1, buffer + 41 + offset, list);
+		if (get_sha1_hex(buffer + offset, sha1)) {
+			warn("unrecognized header: %s", buffer);
+			continue;
+		}
+		delim = buffer[40 + offset];
+		if (!isspace(delim) && (delim != '\0' || !is_prereq))
+			die ("invalid header: %s", buffer);
+		add_to_ref_list(sha1, isspace(delim) ?
+				buffer + 41 + offset : "", list);
 	}
 	return fd;
 }
@@ -127,20 +135,28 @@ static int fork_with_pipe(const char **argv, int *in, int *out)
 			dup2(fdin[0], 0);
 			close(fdin[0]);
 			close(fdin[1]);
-		} else if (in)
+		} else if (in) {
 			dup2(*in, 0);
+			close(*in);
+		}
 		if (needs_out) {
 			dup2(fdout[1], 1);
 			close(fdout[0]);
 			close(fdout[1]);
-		} else if (out)
+		} else if (out) {
 			dup2(*out, 1);
+			close(*out);
+		}
 		exit(execv_git_cmd(argv));
 	}
 	if (needs_in)
 		close(fdin[0]);
+	else if (in)
+		close(*in);
 	if (needs_out)
 		close(fdout[1]);
+	else if (out)
+		close(*out);
 	return pid;
 }
 
@@ -151,29 +167,28 @@ static int verify_bundle(struct bundle_header *header)
 	 * to be verbose about the errors
 	 */
 	struct ref_list *p = &header->prerequisites;
-	const char *argv[5] = {"rev-list", "--stdin", "--not", "--all", NULL};
-	int pid, in, out, i, ret = 0;
+	char **argv;
+	int pid, out, i, ret = 0;
 	char buffer[1024];
 
-	in = out = -1;
-	pid = fork_with_pipe(argv, &in, &out);
+	argv = xmalloc((p->nr + 4) * sizeof(const char *));
+	argv[0] = "rev-list";
+	argv[1] = "--not";
+	argv[2] = "--all";
+	for (i = 0; i < p->nr; i++)
+		argv[i + 3] = xstrdup(sha1_to_hex(p->list[i].sha1));
+	argv[p->nr + 3] = NULL;
+	out = -1;
+	pid = fork_with_pipe((const char **)argv, NULL, &out);
 	if (pid < 0)
 		return error("Could not fork rev-list");
-	if (!fork()) {
-		for (i = 0; i < p->nr; i++) {
-			write(in, sha1_to_hex(p->list[i].sha1), 40);
-			write(in, "\n", 1);
-		}
-		close(in);
-		exit(0);
-	}
-	close(in);
-	while (read_string(out, buffer, sizeof(buffer)) > 0) {
-		if (ret++ == 0)
-			error ("The bundle requires the following commits you lack:");
-		fprintf(stderr, "%s", buffer);
-	}
+	while (read_string(out, buffer, sizeof(buffer)) > 0)
+		; /* do nothing */
 	close(out);
+	for (i = 0; i < p->nr; i++)
+		free(argv[i + 3]);
+	free(argv);
+
 	while (waitpid(pid, &i, 0) < 0)
 		if (errno != EINTR)
 			return -1;
@@ -205,8 +220,8 @@ static int list_heads(struct bundle_header *header, int argc, const char **argv)
 
 static void show_commit(struct commit *commit)
 {
-	write(1, sha1_to_hex(commit->object.sha1), 40);
-	write(1, "\n", 1);
+	write_or_die(1, sha1_to_hex(commit->object.sha1), 40);
+	write_or_die(1, "\n", 1);
 	if (commit->parents) {
 		free_commit_list(commit->parents);
 		commit->parents = NULL;
@@ -216,15 +231,15 @@ static void show_commit(struct commit *commit)
 static void show_object(struct object_array_entry *p)
 {
 	/* An object with name "foo\n0000000..." can be used to
-	 *          * confuse downstream git-pack-objects very badly.
-	 *                   */
+	 * confuse downstream git-pack-objects very badly.
+	 */
 	const char *ep = strchr(p->name, '\n');
 	int len = ep ? ep - p->name : strlen(p->name);
-	write(1, sha1_to_hex(p->item->sha1), 40);
-	write(1, " ", 1);
+	write_or_die(1, sha1_to_hex(p->item->sha1), 40);
+	write_or_die(1, " ", 1);
 	if (len)
-		write(1, p->name, len);
-	write(1, "\n", 1);
+		write_or_die(1, p->name, len);
+	write_or_die(1, "\n", 1);
 }
 
 static int create_bundle(struct bundle_header *header, const char *path,
@@ -243,7 +258,7 @@ static int create_bundle(struct bundle_header *header, const char *path,
 		return error("Could not write to '%s'", path);
 
 	/* write signature */
-	write(bundle_fd, bundle_signature, strlen(bundle_signature));
+	write_or_die(bundle_fd, bundle_signature, strlen(bundle_signature));
 
 	/* write prerequisites */
 	memcpy(argv_boundary + 2, argv + 1, argc * sizeof(const char *));
@@ -256,7 +271,7 @@ static int create_bundle(struct bundle_header *header, const char *path,
 		return -1;
 	while ((i = read_string(out, buffer, sizeof(buffer))) > 0)
 		if (buffer[0] == '-')
-			write(bundle_fd, buffer, i);
+			write_or_die(bundle_fd, buffer, i);
 	while ((i = waitpid(pid, &status, 0)) < 0)
 		if (errno != EINTR)
 			return error("rev-list died");
@@ -279,16 +294,16 @@ static int create_bundle(struct bundle_header *header, const char *path,
 			char *ref;
 			if (dwim_ref(e->name, strlen(e->name), sha1, &ref) != 1)
 				continue;
-			write(bundle_fd, sha1_to_hex(e->item->sha1), 40);
-			write(bundle_fd, " ", 1);
-			write(bundle_fd, ref, strlen(ref));
-			write(bundle_fd, "\n", 1);
+			write_or_die(bundle_fd, sha1_to_hex(e->item->sha1), 40);
+			write_or_die(bundle_fd, " ", 1);
+			write_or_die(bundle_fd, ref, strlen(ref));
+			write_or_die(bundle_fd, "\n", 1);
 			free(ref);
 		}
 	}
 
 	/* end header */
-	write(bundle_fd, "\n", 1);
+	write_or_die(bundle_fd, "\n", 1);
 
 	/* write pack */
 	argv_pack[0] = "pack-objects";
@@ -301,7 +316,6 @@ static int create_bundle(struct bundle_header *header, const char *path,
 	if (pid < 0)
 		return error("Could not spawn pack-objects");
 	close(1);
-	close(bundle_fd);
 	dup2(in, 1);
 	close(in);
 	prepare_revision_walk(&revs);
@@ -327,7 +341,6 @@ static int unbundle(struct bundle_header *header, int bundle_fd,
 	pid = fork_with_pipe(argv_index_pack, &bundle_fd, &dev_null);
 	if (pid < 0)
 		return error("Could not spawn index-pack");
-	close(bundle_fd);
 	while (waitpid(pid, &status, 0) < 0)
 		if (errno != EINTR)
 			return error("index-pack died");
diff --git a/index-pack.c b/index-pack.c
index 5ccf4c4..365f8f3 100644
--- a/index-pack.c
+++ b/index-pack.c
@@ -457,8 +457,9 @@ static void parse_pack_objects(unsigned char *sha1)
 	/* If input_fd is a file, we should have reached its end now. */
 	if (fstat(input_fd, &st))
 		die("cannot fstat packfile: %s", strerror(errno));
-	if (input_fd && S_ISREG(st.st_mode) && st.st_size != consumed_bytes)
-		die("pack has junk at the end: 0%o, %d, %d %d", st.st_mode, (int)st.st_size, (int)consumed_bytes, input_fd);
+	if (S_ISREG(st.st_mode) &&
+			lseek(input_fd, 0, SEEK_CUR) - input_len != st.st_size)
+		die("pack has junk at the end");
 
 	if (!nr_deltas)
 		return;
-- 
1.5.0.1.2213.g94bc-dirty

Re: [PATCH] Add git-bundle: move objects and references by archive

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Thinking about this deeper, I have to say I find my decision to use 
> "--stdin" rather silly, given that I know the exact number of revisions, 
> and their SHA1s.

The only worry I would have is if that exact number is too large
that you cannot pass them sensibly in argv[].

Re: [PATCH] Add git-bundle: move objects and references by archive

[Johannes Schindelin]

Hi,

On Thu, 22 Feb 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Thinking about this deeper, I have to say I find my decision to use 
> > "--stdin" rather silly, given that I know the exact number of revisions, 
> > and their SHA1s.
> 
> The only worry I would have is if that exact number is too large
> that you cannot pass them sensibly in argv[].

I thought there is only a limitation in bash?

Well, anyway, I think that it makes sense writing just a little loop to 
find if we have the revs or not, to be able to warn about them properly.

I'll do that.

Ciao,
Dscho

Re: [PATCH] Add git-bundle: move objects and references by archive

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> The only worry I would have is if that exact number is too large
>> that you cannot pass them sensibly in argv[].
>
> I thought there is only a limitation in bash?

I am sure kernel folks on the list would correct me, but my
understanding is that is execve(2) limitation and you would get
E2BIG.

Re: [PATCH] Add git-bundle: move objects and references by archive

[Johannes Schindelin]

Hi,

On Thu, 22 Feb 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> The only worry I would have is if that exact number is too large
> >> that you cannot pass them sensibly in argv[].
> >
> > I thought there is only a limitation in bash?
> 
> I am sure kernel folks on the list would correct me, but my
> understanding is that is execve(2) limitation and you would get
> E2BIG.

Okay. So how about this (on top of my previous fixup):

-- snipsnap --
[PATCH] git-bundle: avoid fork() in verify_bundle()

We can use the revision walker easily for checking if the
prerequisites are met, instead of fork()ing off a rev-list,
which would list only the first unmet prerequisite.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin-bundle.c |   75 ++++++++++++++++++++++++++++++++++-------------------
 1 files changed, 48 insertions(+), 27 deletions(-)

diff --git a/builtin-bundle.c b/builtin-bundle.c
index 521bbda..4ac06f9 100644
--- a/builtin-bundle.c
+++ b/builtin-bundle.c
@@ -19,7 +19,7 @@ static const char bundle_signature[] = "# v2 git bundle\n";
 
 struct ref_list {
 	unsigned int nr, alloc;
-	struct {
+	struct ref_list_entry {
 		unsigned char sha1[20];
 		char *name;
 	} *list;
@@ -167,33 +167,54 @@ static int verify_bundle(struct bundle_header *header)
 	 * to be verbose about the errors
 	 */
 	struct ref_list *p = &header->prerequisites;
-	char **argv;
-	int pid, out, i, ret = 0;
-	char buffer[1024];
+	struct rev_info revs;
+	const char *argv[] = {NULL, "--all"};
+	struct object_array refs;
+	struct commit *commit;
+	int i, ret = 0, req_nr;
+	const char *message = "The bundle requires these lacking revs:";
 
-	argv = xmalloc((p->nr + 4) * sizeof(const char *));
-	argv[0] = "rev-list";
-	argv[1] = "--not";
-	argv[2] = "--all";
-	for (i = 0; i < p->nr; i++)
-		argv[i + 3] = xstrdup(sha1_to_hex(p->list[i].sha1));
-	argv[p->nr + 3] = NULL;
-	out = -1;
-	pid = fork_with_pipe((const char **)argv, NULL, &out);
-	if (pid < 0)
-		return error("Could not fork rev-list");
-	while (read_string(out, buffer, sizeof(buffer)) > 0)
-		; /* do nothing */
-	close(out);
-	for (i = 0; i < p->nr; i++)
-		free(argv[i + 3]);
-	free(argv);
-
-	while (waitpid(pid, &i, 0) < 0)
-		if (errno != EINTR)
-			return -1;
-	if (!ret && (!WIFEXITED(i) || WEXITSTATUS(i)))
-		return error("At least one prerequisite is lacking.");
+	init_revisions(&revs, NULL);
+	for (i = 0; i < p->nr; i++) {
+		struct ref_list_entry *e = p->list + i;
+		struct object *o = parse_object(e->sha1);
+		if (o) {
+			o->flags |= BOUNDARY_SHOW;
+			add_pending_object(&revs, o, e->name);
+			continue;
+		}
+		if (++ret == 1)
+			error(message);
+		error("%s %s", sha1_to_hex(e->sha1), e->name);
+	}
+	if (revs.pending.nr == 0)
+		return ret;
+	req_nr = revs.pending.nr;
+	setup_revisions(2, argv, &revs, NULL);
+
+	memset(&refs, 0, sizeof(struct object_array));
+	for (i = 0; i < revs.pending.nr; i++) {
+		struct object_array_entry *e = revs.pending.objects + i;
+		add_object_array(e->item, e->name, &refs);
+	}
+
+	prepare_revision_walk(&revs);
+
+	i = req_nr;
+	while (i && (commit = get_revision(&revs)))
+		if (commit->object.flags & BOUNDARY_SHOW)
+			i--;
+
+	for (i = 0; i < req_nr; i++)
+		if (!(refs.objects[i].item->flags & SHOWN)) {
+			if (++ret == 1)
+				error(message);
+			error("%s %s", sha1_to_hex(refs.objects[i].item->sha1),
+				refs.objects[i].name);
+		}
+
+	for (i = 0; i < refs.nr; i++)
+		clear_commit_marks((struct commit *)refs.objects[i].item, -1);
 
 	return ret;
 }
-- 
1.5.0.1.2214.gf22e-dirty

Re: [PATCH] git-bundle: assorted fixes

[Mark Levedahl]

Johannes Schindelin wrote:

I built this under cygwin, it is running but I find two regressions 
compared to my git-bundle.sh:

1) git bundle create --all  <whatever> does not record any references, 
while it should record all references (heads and tags). From shell, this 
requires use of both git-rev-parse and git-show-ref:
    gitrevargs=$(git-rev-parse --symbolic --revs-only $args) || exit 1

    # find the refs to carry along and get sha1s for each.
    refs=
    fullrevargs=
    for arg in $gitrevargs ; do
        #ignore options and basis refs, get full ref name for things
        # we will transport rejecting anything ambiguous (e.g., user
        # gives master, have heads/master and remotes/origin/master, we
        # keep the former).
        case "$arg" in
            -* | ^*) fullrevargs="$fullrevargs $arg";;
            *)  ref=$(git-show-ref "$arg")
                test "$(echo $ref | wc -w)" = "2" || die "Ambigous 
reference: $arg
$ref"
                fullrevargs="$fullrevargs ${ref#* }"
                refs="$refs $ref";;
        esac
    done

2) git bundle verify  reports only a single sha1 if prerequisites are 
not met. git-bundle.sh would loop through finding each missing one, 
annotating with the one line commit message. The prerequisites were 
stored using:

    (for p in $prereqs ; do
        git-rev-list --pretty=one --max-count=1 $p
    done) > "$prerequisites"

and then verify does:
    test -z "$prereqs" && return 0
    bad=$(echo "$prereqs" | cut -b-40 | git-rev-list --stdin --not --all 
2>&1)
    if test -n "$bad" ; then
        test "$1" = "--silent" && return 1
        echo "error: $bfile requires the following commits you lack:"
        echo "$prereqs" |
        while read sha1 comment ; do
            missing=$(git-rev-list $sha1 --not --all 2>&1)
            test -n "$missing" && echo "$sha1 $comment"
        done
        exit 1
    fi
    return 0

The difference is

    fatal: bad object 59e4aa84d4b4f7c6393317c68649ef7db3c4440c
    error: At least one prerequisite is lacking.

vs.

    error: /home/mlevedahl/bundle2 requires the following commits you lack:
    59e4aa84d4b4f7c6393317c68649ef7db3c4440c Merge remote branch 
'rvaas05/gem.418' into lcb.418
    8eb8c8274c28be9435da8534f8bb598503f4f85a Merge remote branch 
'rvaas05/gem.418' into lcd.419

This was Junio's suggestion originally, and I think it a very good one. 
The latter error message is far more helpful to some poor soul trying to 
fix a problem.


Also,  I did track down the issue that forced me to use tar (or at least 
*some* archiver) in cygwin: it is a known bug without a planned fix that 
precludes saving mixed text and binary in one file from bash, and seems 
to be tied deeply into bash, fork, and pipes. Basically, doing
    echo "some text data" > file
    echo "some binary data" >>file
is totally confused as the underlying pipe / fork mechanism thinks file 
is text mode on the second operation. This happens regardless of mount 
mode or CYGWIN=binmode. The end result is under bash the binary data 
unconditionally suffers crlf->lf translation. You can look at at 
http://www.mail-archive.com/cygwin@cygwin.com/msg76319.html for more 
information.

Mark

Re: [PATCH] git-bundle: assorted fixes

[Johannes Schindelin]

Hi,

On Thu, 22 Feb 2007, Mark Levedahl wrote:

> Johannes Schindelin wrote:
> 
> I built this under cygwin, it is running but I find two regressions compared
> to my git-bundle.sh:
> 
> 1) git bundle create --all  <whatever>

Are you sure you did not provide a bundle filename?

But indeed, my test shows that "--all" does not leave any refs. Bad.

This fixes it:

-- snip --
 revision.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/revision.c b/revision.c
index 4cf697e..823bbd1 100644
--- a/revision.c
+++ b/revision.c
@@ -480,7 +480,7 @@ static int handle_one_ref(const char *path, const unsigned char *sha1, int flag,
 	struct all_refs_cb *cb = cb_data;
 	struct object *object = get_reference(cb->all_revs, path, sha1,
 					      cb->all_flags);
-	add_pending_object(cb->all_revs, object, "");
+	add_pending_object(cb->all_revs, object, path);
 	return 0;
 }
-- snap --

but I think this adds a memory leak, and I don't know what else is 
affected by it.
 
> 2) git bundle verify  reports only a single sha1 if prerequisites are not
> met.

With the two follow-up patches I sent, this issue should be resolved, no?

Ciao,
Dscho

Re: [PATCH] git-bundle: assorted fixes

[Mark Levedahl]

Johannes Schindelin wrote:
> Hi,
>   
>> 1) git bundle create --all  <whatever>
>>     
>
> Are you sure you did not provide a bundle filename?
>   
I did, you *have* to give a bundle filename..
> But indeed, my test shows that "--all" does not leave any refs. Bad.
>
> This fixes it:
>   
Confirmed - that works.
>  
>   
>> 2) git bundle verify  reports only a single sha1 if prerequisites are not
>> met.
>>     
>
> With the two follow-up patches I sent, this issue should be resolved, no?
>
> Ciao,
> Dscho
>
>   
I missed one of your patches, adding that, the problem is incompletely 
fixed: all missing commits are shown, but only as raw hashes (e.g., the 
1-line commit message is not there to describe what is missing).

Mark

Re: [PATCH] git-bundle: assorted fixes

[Johannes Schindelin]

On Thu, 22 Feb 2007, Mark Levedahl wrote:

> Johannes Schindelin wrote:
>
> > Mark wrote:
> >
> > > 2) git bundle verify reports only a single sha1 if prerequisites are 
> > > not met.
> > 
> > With the two follow-up patches I sent, this issue should be resolved, 
> > no?
> > 
>   I missed one of your patches, adding that, the problem is incompletely 
> fixed: all missing commits are shown, but only as raw hashes (e.g., the 
> 1-line commit message is not there to describe what is missing).

Okay, how about this on top:

-- snipsnap --

 builtin-bundle.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/builtin-bundle.c b/builtin-bundle.c
index 4ac06f9..d11435b 100644
--- a/builtin-bundle.c
+++ b/builtin-bundle.c
@@ -267,7 +267,7 @@ static int create_bundle(struct bundle_header *header, const char *path,
 		int argc, const char **argv)
 {
 	int bundle_fd = -1;
-	const char **argv_boundary = xmalloc((argc + 3) * sizeof(const char *));
+	const char **argv_boundary = xmalloc((argc + 4) * sizeof(const char *));
 	const char **argv_pack = xmalloc(4 * sizeof(const char *));
 	int pid, in, out, i, status;
 	char buffer[1024];
@@ -282,10 +282,11 @@ static int create_bundle(struct bundle_header *header, const char *path,
 	write_or_die(bundle_fd, bundle_signature, strlen(bundle_signature));
 
 	/* write prerequisites */
-	memcpy(argv_boundary + 2, argv + 1, argc * sizeof(const char *));
+	memcpy(argv_boundary + 3, argv + 1, argc * sizeof(const char *));
 	argv_boundary[0] = "rev-list";
 	argv_boundary[1] = "--boundary";
-	argv_boundary[argc + 1] = NULL;
+	argv_boundary[2] = "--pretty=oneline";
+	argv_boundary[argc + 2] = NULL;
 	out = -1;
 	pid = fork_with_pipe(argv_boundary, NULL, &out);
 	if (pid < 0)

Re: [PATCH] Add git-bundle: move objects and references by archive

[Mark Levedahl]

Junio C Hamano wrote:
>> diff --git a/Documentation/cmd-list.perl b/Documentation/cmd-list.perl
>> index a2d6268..f61c77a 100755
>> --- a/Documentation/cmd-list.perl
>> +++ b/Documentation/cmd-list.perl
>> @@ -70,6 +70,7 @@ git-archive                             mainporcelain
>>  git-bisect                              mainporcelain
>>  git-blame                               ancillaryinterrogators
>>  git-branch                              mainporcelain
>> +git-bundle                              mainporcelain
>>  git-cat-file                            plumbinginterrogators
>>  git-checkout-index                      plumbingmanipulators
>>  git-checkout                            mainporcelain
>>     
>
> Is this really a mainporcelain?
> I would say ancillarymanipulators (or perhaps synchelpers).
>
>   
git bundle has four commands: create, verify, list-heads, and unbundle. 
The last two are pure helper functions, basically plumbing. Verify is 
questionable as to where it lies. But, create is the only way to create 
a bundle, is logically equivalent to git push as a user command to move 
data,  so I called it mainporcelain because that is how git push is 
classified.


>> +	/* write prerequisites */
>> +	memcpy(argv_boundary + 2, argv + 1, argc * sizeof(const char *));
>> +	argv_boundary[0] = "rev-list";
>> +	argv_boundary[1] = "--boundary";
>> +	argv_boundary[argc + 1] = NULL;
>> +	out = -1;
>> +	pid = fork_with_pipe(argv_boundary, NULL, &out);
>> +	if (pid < 0)
>> +		return -1;
>> +	while ((i = read_string(out, buffer, sizeof(buffer))) > 0)
>> +		if (buffer[0] == '-')
>> +			write(bundle_fd, buffer, i);
>>     
>
> It would be helpful for the recipient if you can append output
> from git-describe (or name-rev) when the buffer lacks "name".
>   
I found the actual commit summary message (i.e., git-rev-list 
--pretty=one --max-count=1 sha1) the most useful of the various 
summaries available.



Mark

Re: [PATCH] git-bundle: assorted fixes

[Mark Levedahl]

Johannes Schindelin wrote:
> 
> On Thu, 22 Feb 2007, Mark Levedahl wrote:
> 
>> Johannes Schindelin wrote:
>>
>>> Mark wrote:
>>>
>>>> 2) git bundle verify reports only a single sha1 if prerequisites are 
>>>> not met.
>>> With the two follow-up patches I sent, this issue should be resolved, 
>>> no?
>>>
>>   I missed one of your patches, adding that, the problem is incompletely 
>> fixed: all missing commits are shown, but only as raw hashes (e.g., the 
>> 1-line commit message is not there to describe what is missing).
> 
> Okay, how about this on top:
> 
>

That works. I would change your error message slightly, perhaps...

diff --git a/builtin-bundle.c b/builtin-bundle.c
index d74afaa..e115feb 100644
--- a/builtin-bundle.c
+++ b/builtin-bundle.c
@@ -172,7 +172,7 @@ static int verify_bundle(struct bundle_header *header)
     struct object_array refs;
     struct commit *commit;
     int i, ret = 0, req_nr;
-   const char *message = "The bundle requires these lacking revs:";
+   const char *message = "Your repository lacks these prerequisite 
commits:";

     init_revisions(&revs, NULL);
     for (i = 0; i < p->nr; i++) {

Mark

Re: [PATCH] Add git-bundle: move objects and references by archive

[Junio C Hamano]

Mark Levedahl <mdl123@verizon.net> writes:

> I found the actual commit summary message (i.e., git-rev-list
> --pretty=one --max-count=1 sha1) the most useful of the various
> summaries available.

I would agree that would be the case, and Johannes's follow-up
seems to do that.