[PATCH 1/3] Move bundle specific stuff into bundle.[ch]

[PATCH 1/3] Move bundle specific stuff into bundle.[ch]

[Johannes Schindelin]

The transport specific stuff was moved into libgit.a, and the
bundle specific stuff will not be left behind.

This is a big code move, with one exception: the function
unbundle() no longer outputs the list of refs.  You have to call
list_bundle_refs() yourself for that.

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

	FWIW this patch series is based on the bf_series branch of
	git://iabervon.org/~barkalow/git.git, revision a204e4b4.

 Makefile         |    2 +-
 builtin-bundle.c |  330 +-----------------------------------------------------
 bundle.c         |  310 ++++++++++++++++++++++++++++++++++++++++++++++++++
 bundle.h         |   25 ++++
 4 files changed, 342 insertions(+), 325 deletions(-)
 create mode 100644 bundle.c
 create mode 100644 bundle.h

diff --git a/Makefile b/Makefile
index 335be8f..a389ee7 100644
--- a/Makefile
+++ b/Makefile
@@ -318,7 +318,7 @@ LIB_OBJS = \
 	alloc.o merge-file.o path-list.o help.o unpack-trees.o $(DIFF_OBJS) \
 	color.o wt-status.o archive-zip.o archive-tar.o shallow.o utf8.o \
 	convert.o attr.o decorate.o progress.o mailmap.o symlinks.o remote.o \
-	transport.o
+	transport.o bundle.o
 
 BUILTIN_OBJS = \
 	builtin-add.o \
diff --git a/builtin-bundle.c b/builtin-bundle.c
index 306ad29..22d03a3 100644
--- a/builtin-bundle.c
+++ b/builtin-bundle.c
@@ -1,10 +1,5 @@
 #include "cache.h"
-#include "object.h"
-#include "commit.h"
-#include "diff.h"
-#include "revision.h"
-#include "list-objects.h"
-#include "run-command.h"
+#include "bundle.h"
 
 /*
  * Basic handler for bundle files to connect repositories via sneakernet.
@@ -15,320 +10,6 @@
 
 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 ref_list_entry {
-		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;
-	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++) {
-		ssize_t count = xread(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 is_prereq = buffer[0] == '-';
-		int offset = is_prereq ? 1 : 0;
-		int len = strlen(buffer);
-		unsigned char sha1[20];
-		struct ref_list *list = is_prereq ? &header->prerequisites
-			: &header->references;
-		char delim;
-
-		if (buffer[len - 1] == '\n')
-			buffer[len - 1] = '\0';
-		if (get_sha1_hex(buffer + offset, sha1)) {
-			warning("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;
-}
-
-static int list_refs(struct ref_list *r, int argc, const char **argv)
-{
-	int i;
-
-	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;
-}
-
-#define PREREQ_MARK (1u<<16)
-
-static int verify_bundle(struct bundle_header *header, int verbose)
-{
-	/*
-	 * 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;
-	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 = "Repository lacks these prerequisite commits:";
-
-	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 |= PREREQ_MARK;
-			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 != p->nr)
-		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 & PREREQ_MARK)
-			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);
-
-	if (verbose) {
-		struct ref_list *r;
-
-		r = &header->references;
-		printf("The bundle contains %d ref%s\n",
-		       r->nr, (1 < r->nr) ? "s" : "");
-		list_refs(r, 0, NULL);
-		r = &header->prerequisites;
-		printf("The bundle requires these %d ref%s\n",
-		       r->nr, (1 < r->nr) ? "s" : "");
-		list_refs(r, 0, NULL);
-	}
-	return ret;
-}
-
-static int list_heads(struct bundle_header *header, int argc, const char **argv)
-{
-	return list_refs(&header->references, argc, argv);
-}
-
-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 + 4) * sizeof(const char *));
-	const char **argv_pack = xmalloc(5 * sizeof(const char *));
-	int i, ref_count = 0;
-	char buffer[1024];
-	struct rev_info revs;
-	struct child_process rls;
-
-	bundle_fd = (!strcmp(path, "-") ? 1 :
-			open(path, O_CREAT | O_EXCL | O_WRONLY, 0666));
-	if (bundle_fd < 0)
-		return error("Could not create '%s': %s", path, strerror(errno));
-
-	/* write signature */
-	write_or_die(bundle_fd, bundle_signature, strlen(bundle_signature));
-
-	/* init revs to list objects for pack-objects later */
-	save_commit_buffer = 0;
-	init_revisions(&revs, NULL);
-
-	/* write prerequisites */
-	memcpy(argv_boundary + 3, argv + 1, argc * sizeof(const char *));
-	argv_boundary[0] = "rev-list";
-	argv_boundary[1] = "--boundary";
-	argv_boundary[2] = "--pretty=oneline";
-	argv_boundary[argc + 2] = NULL;
-	memset(&rls, 0, sizeof(rls));
-	rls.argv = argv_boundary;
-	rls.out = -1;
-	rls.git_cmd = 1;
-	if (start_command(&rls))
-		return -1;
-	while ((i = read_string(rls.out, buffer, sizeof(buffer))) > 0) {
-		unsigned char sha1[20];
-		if (buffer[0] == '-') {
-			write_or_die(bundle_fd, buffer, i);
-			if (!get_sha1_hex(buffer + 1, sha1)) {
-				struct object *object = parse_object(sha1);
-				object->flags |= UNINTERESTING;
-				add_pending_object(&revs, object, buffer);
-			}
-		} else if (!get_sha1_hex(buffer, sha1)) {
-			struct object *object = parse_object(sha1);
-			object->flags |= SHOWN;
-		}
-	}
-	if (finish_command(&rls))
-		return error("rev-list died");
-
-	/* write references */
-	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;
-		unsigned char sha1[20];
-		char *ref;
-
-		if (e->item->flags & UNINTERESTING)
-			continue;
-		if (dwim_ref(e->name, strlen(e->name), sha1, &ref) != 1)
-			continue;
-		/*
-		 * Make sure the refs we wrote out is correct; --max-count and
-		 * other limiting options could have prevented all the tips
-		 * from getting output.
-		 */
-		if (!(e->item->flags & SHOWN)) {
-			warning("ref '%s' is excluded by the rev-list options",
-				e->name);
-			continue;
-		}
-		ref_count++;
-		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);
-	}
-	if (!ref_count)
-		die ("Refusing to create empty bundle.");
-
-	/* end header */
-	write_or_die(bundle_fd, "\n", 1);
-
-	/* write pack */
-	argv_pack[0] = "pack-objects";
-	argv_pack[1] = "--all-progress";
-	argv_pack[2] = "--stdout";
-	argv_pack[3] = "--thin";
-	argv_pack[4] = NULL;
-	memset(&rls, 0, sizeof(rls));
-	rls.argv = argv_pack;
-	rls.in = -1;
-	rls.out = bundle_fd;
-	rls.git_cmd = 1;
-	if (start_command(&rls))
-		return error("Could not spawn pack-objects");
-	for (i = 0; i < revs.pending.nr; i++) {
-		struct object *object = revs.pending.objects[i].item;
-		if (object->flags & UNINTERESTING)
-			write(rls.in, "^", 1);
-		write(rls.in, sha1_to_hex(object->sha1), 40);
-		write(rls.in, "\n", 1);
-	}
-	if (finish_command(&rls))
-		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",
-		"--fix-thin", "--stdin", NULL};
-	struct child_process ip;
-
-	if (verify_bundle(header, 0))
-		return -1;
-	memset(&ip, 0, sizeof(ip));
-	ip.argv = argv_index_pack;
-	ip.in = bundle_fd;
-	ip.no_stdout = 1;
-	ip.git_cmd = 1;
-	if (run_command(&ip))
-		return error("index-pack died");
-	return list_heads(header, argc, argv);
-}
-
 int cmd_bundle(int argc, const char **argv, const char *prefix)
 {
 	struct bundle_header header;
@@ -352,8 +33,8 @@ int cmd_bundle(int argc, const char **argv, const char *prefix)
 	}
 
 	memset(&header, 0, sizeof(header));
-	if (strcmp(cmd, "create") &&
-			(bundle_fd = read_header(bundle_file, &header)) < 0)
+	if (strcmp(cmd, "create") && (bundle_fd =
+				read_bundle_header(bundle_file, &header)) < 0)
 		return 1;
 
 	if (!strcmp(cmd, "verify")) {
@@ -365,7 +46,7 @@ int cmd_bundle(int argc, const char **argv, const char *prefix)
 	}
 	if (!strcmp(cmd, "list-heads")) {
 		close(bundle_fd);
-		return !!list_heads(&header, argc, argv);
+		return !!list_bundle_refs(&header, argc, argv);
 	}
 	if (!strcmp(cmd, "create")) {
 		if (nongit)
@@ -374,7 +55,8 @@ int cmd_bundle(int argc, const char **argv, const char *prefix)
 	} else if (!strcmp(cmd, "unbundle")) {
 		if (nongit)
 			die("Need a repository to unbundle.");
-		return !!unbundle(&header, bundle_fd, argc, argv);
+		return !!unbundle(&header, bundle_fd) ||
+			list_bundle_refs(&header, argc, argv);
 	} else
 		usage(bundle_usage);
 }
diff --git a/bundle.c b/bundle.c
new file mode 100644
index 0000000..659b61d
--- /dev/null
+++ b/bundle.c
@@ -0,0 +1,310 @@
+#include "cache.h"
+#include "bundle.h"
+#include "object.h"
+#include "commit.h"
+#include "diff.h"
+#include "revision.h"
+#include "list-objects.h"
+#include "run-command.h"
+
+static const char bundle_signature[] = "# v2 git bundle\n";
+
+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++;
+}
+
+/* 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++) {
+		ssize_t count = xread(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 */
+int read_bundle_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 is_prereq = buffer[0] == '-';
+		int offset = is_prereq ? 1 : 0;
+		int len = strlen(buffer);
+		unsigned char sha1[20];
+		struct ref_list *list = is_prereq ? &header->prerequisites
+			: &header->references;
+		char delim;
+
+		if (buffer[len - 1] == '\n')
+			buffer[len - 1] = '\0';
+		if (get_sha1_hex(buffer + offset, sha1)) {
+			warning("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;
+}
+
+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 + 4) * sizeof(const char *));
+	const char **argv_pack = xmalloc(5 * sizeof(const char *));
+	int i, ref_count = 0;
+	char buffer[1024];
+	struct rev_info revs;
+	struct child_process rls;
+
+	bundle_fd = (!strcmp(path, "-") ? 1 :
+			open(path, O_CREAT | O_EXCL | O_WRONLY, 0666));
+	if (bundle_fd < 0)
+		return error("Could not create '%s': %s", path, strerror(errno));
+
+	/* write signature */
+	write_or_die(bundle_fd, bundle_signature, strlen(bundle_signature));
+
+	/* init revs to list objects for pack-objects later */
+	save_commit_buffer = 0;
+	init_revisions(&revs, NULL);
+
+	/* write prerequisites */
+	memcpy(argv_boundary + 3, argv + 1, argc * sizeof(const char *));
+	argv_boundary[0] = "rev-list";
+	argv_boundary[1] = "--boundary";
+	argv_boundary[2] = "--pretty=oneline";
+	argv_boundary[argc + 2] = NULL;
+	memset(&rls, 0, sizeof(rls));
+	rls.argv = argv_boundary;
+	rls.out = -1;
+	rls.git_cmd = 1;
+	if (start_command(&rls))
+		return -1;
+	while ((i = read_string(rls.out, buffer, sizeof(buffer))) > 0) {
+		unsigned char sha1[20];
+		if (buffer[0] == '-') {
+			write_or_die(bundle_fd, buffer, i);
+			if (!get_sha1_hex(buffer + 1, sha1)) {
+				struct object *object = parse_object(sha1);
+				object->flags |= UNINTERESTING;
+				add_pending_object(&revs, object, buffer);
+			}
+		} else if (!get_sha1_hex(buffer, sha1)) {
+			struct object *object = parse_object(sha1);
+			object->flags |= SHOWN;
+		}
+	}
+	if (finish_command(&rls))
+		return error("rev-list died");
+
+	/* write references */
+	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;
+		unsigned char sha1[20];
+		char *ref;
+
+		if (e->item->flags & UNINTERESTING)
+			continue;
+		if (dwim_ref(e->name, strlen(e->name), sha1, &ref) != 1)
+			continue;
+		/*
+		 * Make sure the refs we wrote out is correct; --max-count and
+		 * other limiting options could have prevented all the tips
+		 * from getting output.
+		 */
+		if (!(e->item->flags & SHOWN)) {
+			warning("ref '%s' is excluded by the rev-list options",
+				e->name);
+			continue;
+		}
+		ref_count++;
+		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);
+	}
+	if (!ref_count)
+		die ("Refusing to create empty bundle.");
+
+	/* end header */
+	write_or_die(bundle_fd, "\n", 1);
+
+	/* write pack */
+	argv_pack[0] = "pack-objects";
+	argv_pack[1] = "--all-progress";
+	argv_pack[2] = "--stdout";
+	argv_pack[3] = "--thin";
+	argv_pack[4] = NULL;
+	memset(&rls, 0, sizeof(rls));
+	rls.argv = argv_pack;
+	rls.in = -1;
+	rls.out = bundle_fd;
+	rls.git_cmd = 1;
+	if (start_command(&rls))
+		return error("Could not spawn pack-objects");
+	for (i = 0; i < revs.pending.nr; i++) {
+		struct object *object = revs.pending.objects[i].item;
+		if (object->flags & UNINTERESTING)
+			write(rls.in, "^", 1);
+		write(rls.in, sha1_to_hex(object->sha1), 40);
+		write(rls.in, "\n", 1);
+	}
+	if (finish_command(&rls))
+		return error ("pack-objects died");
+	return 0;
+}
+
+static int list_refs(struct ref_list *r, int argc, const char **argv)
+{
+	int i;
+
+	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;
+}
+
+int list_bundle_refs(struct bundle_header *header, int argc, const char **argv)
+{
+	return list_refs(&header->references, argc, argv);
+}
+
+#define PREREQ_MARK (1u<<16)
+
+int verify_bundle(struct bundle_header *header, int verbose)
+{
+	/*
+	 * 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;
+	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 = "Repository lacks these prerequisite commits:";
+
+	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 |= PREREQ_MARK;
+			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 != p->nr)
+		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 & PREREQ_MARK)
+			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);
+
+	if (verbose) {
+		struct ref_list *r;
+
+		r = &header->references;
+		printf("The bundle contains %d ref%s\n",
+		       r->nr, (1 < r->nr) ? "s" : "");
+		list_refs(r, 0, NULL);
+		r = &header->prerequisites;
+		printf("The bundle requires these %d ref%s\n",
+		       r->nr, (1 < r->nr) ? "s" : "");
+		list_refs(r, 0, NULL);
+	}
+	return ret;
+}
+
+int unbundle(struct bundle_header *header, int bundle_fd)
+{
+	const char *argv_index_pack[] = {"index-pack",
+		"--fix-thin", "--stdin", NULL};
+	struct child_process ip;
+
+	if (verify_bundle(header, 0))
+		return -1;
+	memset(&ip, 0, sizeof(ip));
+	ip.argv = argv_index_pack;
+	ip.in = bundle_fd;
+	ip.no_stdout = 1;
+	ip.git_cmd = 1;
+	if (run_command(&ip))
+		return error("index-pack died");
+	return 0;
+}
+
+
diff --git a/bundle.h b/bundle.h
new file mode 100644
index 0000000..e2aedd6
--- /dev/null
+++ b/bundle.h
@@ -0,0 +1,25 @@
+#ifndef BUNDLE_H
+#define BUNDLE_H
+
+struct ref_list {
+	unsigned int nr, alloc;
+	struct ref_list_entry {
+		unsigned char sha1[20];
+		char *name;
+	} *list;
+};
+
+struct bundle_header {
+	struct ref_list prerequisites;
+	struct ref_list references;
+};
+
+int read_bundle_header(const char *path, struct bundle_header *header);
+int create_bundle(struct bundle_header *header, const char *path,
+		int argc, const char **argv);
+int verify_bundle(struct bundle_header *header, int verbose);
+int unbundle(struct bundle_header *header, int bundle_fd);
+int list_bundle_refs(struct bundle_header *header,
+		int argc, const char **argv);
+
+#endif
-- 
1.5.3.rc1.16.g9d6f-dirty

Re: [PATCH 1/3] Move bundle specific stuff into bundle.[ch]

[Daniel Barkalow]

On Tue, 17 Jul 2007, Johannes Schindelin wrote:

> The transport specific stuff was moved into libgit.a, and the
> bundle specific stuff will not be left behind.
> 
> This is a big code move, with one exception: the function
> unbundle() no longer outputs the list of refs.  You have to call
> list_bundle_refs() yourself for that.

You should use -C on this sort of thing, so that the interesting aspects 
of the patch are easier to see. (It actually comes out longer in this 
case, but it's far easier to tell that the code in the new file is the 
same as the old code.) Can you tell I've been rearranging a lot of code 
lately and trying to make the patches not look really scary?

Aside from presentation, it looks good to me. Shall I stick the bundle 
changes into my series? I'd like to have them come before the patch to 
switch to builtin-fetch, so that there aren't any revisions where "git 
fetch" doesn't have bundle support.

And I think it would be best to take part 3 as a review fix to my final 
patch.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH 1/3] Move bundle specific stuff into bundle.[ch]

[Daniel Barkalow]

On Tue, 17 Jul 2007, Daniel Barkalow wrote:

> On Tue, 17 Jul 2007, Johannes Schindelin wrote:
> 
> > The transport specific stuff was moved into libgit.a, and the
> > bundle specific stuff will not be left behind.
> > 
> > This is a big code move, with one exception: the function
> > unbundle() no longer outputs the list of refs.  You have to call
> > list_bundle_refs() yourself for that.
> 
> You should use -C on this sort of thing, so that the interesting aspects 
> of the patch are easier to see. (It actually comes out longer in this 
> case, but it's far easier to tell that the code in the new file is the 
> same as the old code.) Can you tell I've been rearranging a lot of code 
> lately and trying to make the patches not look really scary?

Actually, I ended up touching this up a tiny bit, too: I ordered the 
functions in bundle.c the way they were in builtin-bundle.c (so that the 
patch is more trivial) and removed the blank lines at the end of the file. 
This makes the "git diff -C" output really obvious. 

(Someday, I'd like to have a diff that can show that a substantial block 
of '+' lines matches a block of lines from somewhere in the "before" 
content, so reviewers can verify that the patch reorders code but doesn't 
change it, or changes it in certain ways. But, of course, that's both hard 
to generate and hard to display usefully.)

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH 1/3] Move bundle specific stuff into bundle.[ch]

[Shawn O. Pearce]

Daniel Barkalow <barkalow@iabervon.org> wrote:
> (Someday, I'd like to have a diff that can show that a substantial block 
> of '+' lines matches a block of lines from somewhere in the "before" 
> content, so reviewers can verify that the patch reorders code but doesn't 
> change it, or changes it in certain ways. But, of course, that's both hard 
> to generate and hard to display usefully.)

I just moved a huge block in git-gui so I have a great example...

What about a patch format like this?  I doubt people move more than
26 blocks in the same patch of the same file, so using a single
character block prefix when the before/after images are identical
might work OK.

diff --git a/git-gui.sh b/git-gui.sh
index 0443129..f13fa80 100755
--- a/git-gui.sh
+++ b/git-gui.sh
a@@ -632,6 +632,43 a@@ You are using [git-version]:
  
  ######################################################################
  ##
a+## feature option selection
a+
a+if {[regexp {^git-(.+)$} [appname] _junk subcommand]} {
a+	unset _junk
a+} else {
a+	set subcommand gui
a+}
a+if {$subcommand eq {gui.sh}} {
a+	set subcommand gui
a+}
a+if {$subcommand eq {gui} && [llength $argv] > 0} {
a+	set subcommand [lindex $argv 0]
a+	set argv [lrange $argv 1 end]
a+}
a+
a+enable_option multicommit
a+enable_option branch
a+enable_option transport
a+
a+switch -- $subcommand {
a+browser -
a+blame {
a+	disable_option multicommit
a+	disable_option branch
a+	disable_option transport
a+}
a+citool {
a+	enable_option singlecommit
a+
a+	disable_option multicommit
a+	disable_option branch
a+	disable_option transport
a+}
a+}
a+
a+######################################################################
a+##
  ## repository setup
  
  if {[catch {
a@@ -1598,43 +1635,6 a@@ apply_config
  
  ######################################################################
  ##
a-## feature option selection
a-
a-if {[regexp {^git-(.+)$} [appname] _junk subcommand]} {
a-	unset _junk
a-} else {
a-	set subcommand gui
a-}
a-if {$subcommand eq {gui.sh}} {
a-	set subcommand gui
a-}
a-if {$subcommand eq {gui} && [llength $argv] > 0} {
a-	set subcommand [lindex $argv 0]
a-	set argv [lrange $argv 1 end]
a-}
a-
a-enable_option multicommit
a-enable_option branch
a-enable_option transport
a-
a-switch -- $subcommand {
a-browser -
a-blame {
a-	disable_option multicommit
a-	disable_option branch
a-	disable_option transport
a-}
a-citool {
a-	enable_option singlecommit
a-
a-	disable_option multicommit
a-	disable_option branch
a-	disable_option transport
a-}
a-}
a-
a-######################################################################
a-##
  ## ui construction
  
  set ui_comm {}
-- 
1.5.3.rc1.818.g84b7


-- 
Shawn.

Re: [PATCH 1/3] Move bundle specific stuff into bundle.[ch]

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> What about a patch format like this?  I doubt people move more than
> 26 blocks in the same patch of the same file, so using a single
> character block prefix when the before/after images are identical
> might work OK.
>
> diff --git a/git-gui.sh b/git-gui.sh
> index 0443129..f13fa80 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> a@@ -632,6 +632,43 a@@ You are using [git-version]:
>   
>   ######################################################################
>   ##
> a+## feature option selection
> ...
> a+
> a+######################################################################
> a+##
>   ## repository setup
>   
>   if {[catch {
> a@@ -1598,43 +1635,6 a@@ apply_config
>   
>   ######################################################################
>   ##
> a-## feature option selection
> ....
> a-##
>   ## ui construction
>   
>   set ui_comm {}

Gaah, my eyes, my *eyes*!!

	runs, stays in bathroom for 10 minutes and washes, and
        comes back...

It might not be actually so bad.  But I wonder if it would be
more obvious if you do not show the whole "a-" lines but leave
just a marker there.  That is (ugliness of "a@@" and "a-" that
made me wash my eyes needs to be fixed, though -- but that is
only the syntax):

a@@ -1598,43 +1635,6 a@@ apply_config
  
  ######################################################################
  ##
a-<<< Block a was originally here >>>
  ## ui construction
  
  set ui_comm {}

You are coming up with a new output format that is only used
when it is a straight move and nothing else, so by definition
there is really no need to show both removal and addition.

Re: [PATCH 1/3] Move bundle specific stuff into bundle.[ch]

[Shawn O. Pearce]

Junio C Hamano <gitster@pobox.com> wrote:
> It might not be actually so bad.  But I wonder if it would be
> more obvious if you do not show the whole "a-" lines but leave
> just a marker there.  That is (ugliness of "a@@" and "a-" that
> made me wash my eyes needs to be fixed, though -- but that is
> only the syntax):
> 
> a@@ -1598,43 +1635,6 a@@ apply_config
>   
>   ######################################################################
>   ##
> a-<<< Block a was originally here >>>
>   ## ui construction
>   
>   set ui_comm {}
> 
> You are coming up with a new output format that is only used
> when it is a straight move and nothing else, so by definition
> there is really no need to show both removal and addition.

Yea, this I like even better than what I posted.  Now we just need
a suck^H^H^H^Hprogrammer to implement a working prototype and see
how folks like more realistic diffs generated with it.  ;-)

-- 
Shawn.

Re: [PATCH 1/3] Move bundle specific stuff into bundle.[ch]

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> Yea, this I like even better than what I posted.  Now we just need
> a suck^H^H^H^Hprogrammer to implement a working prototype and see
> how folks like more realistic diffs generated with it.  ;-)

I have to disgree.

We on the git list *live* in pretty much git-only world (ok, I
am ignoring git-svn people for now ;-)), so it might feel it
beneficial to add a new output format to git-diff.

But many users of git generated patch needs to interact with
patches that are not generated by git.  I think an additional
postprocessor in patchutils/interdiff toolchest would probably
make much more sense.  Then a person who reviews a patch that is
supposed to be line movement can use the filter to verify the
parts that should be only line movements are indeed are
movements and nothing else.

Speaking of "diff" output, what I would like to see is a support
of 'copied context' (i.e. traditional 'context diff' format you
would get from "diff -c"), in addition to the 'unified context'
we support.  Generating copied context may help people who need
to give patches to others that accept only copied context format
patches (are there important projects that take only cpied
context format patches?).  Being able to accept copied context
format patches in 'git-apply' would also be handy.

Of course, we could use interdiff to mangle copied context
format into unified context format, so 'git-apply' is not a big
deal, though.

Re: [PATCH 1/3] Move bundle specific stuff into bundle.[ch]

[Johannes Schindelin]

Hi,

On Tue, 17 Jul 2007, Daniel Barkalow wrote:

> On Tue, 17 Jul 2007, Johannes Schindelin wrote:
> 
> > The transport specific stuff was moved into libgit.a, and the bundle 
> > specific stuff will not be left behind.
> > 
> > This is a big code move, with one exception: the function unbundle() 
> > no longer outputs the list of refs.  You have to call 
> > list_bundle_refs() yourself for that.
> 
> You should use -C on this sort of thing, so that the interesting aspects 
> of the patch are easier to see. (It actually comes out longer in this 
> case, but it's far easier to tell that the code in the new file is the 
> same as the old code.)

Okay, I wanted it to be kept short, since I really get lost easily in 
hundreds of "-" lines, with possibly one in the midst being a "+".

> Can you tell I've been rearranging a lot of code lately and trying to 
> make the patches not look really scary?

Sorry, I did not pay that close attention.

> Aside from presentation, it looks good to me. Shall I stick the bundle 
> changes into my series? I'd like to have them come before the patch to 
> switch to builtin-fetch, so that there aren't any revisions where "git 
> fetch" doesn't have bundle support.

Looks fine to me.  Seems like you should add a SOB line, too.

> And I think it would be best to take part 3 as a review fix to my final 
> patch.

Yes, definitely.  This shows again (to me, at least), that just looking at 
the code is not enough, you have to run it, too, to review patches.

Ciao,
Dscho

Re: [PATCH 1/3] Move bundle specific stuff into bundle.[ch]

[Johannes Schindelin]

Hi,

On Tue, 17 Jul 2007, Daniel Barkalow wrote:

> On Tue, 17 Jul 2007, Daniel Barkalow wrote:
> 
> > On Tue, 17 Jul 2007, Johannes Schindelin wrote:
> > 
> > > The transport specific stuff was moved into libgit.a, and the bundle 
> > > specific stuff will not be left behind.
> > > 
> > > This is a big code move, with one exception: the function unbundle() 
> > > no longer outputs the list of refs.  You have to call 
> > > list_bundle_refs() yourself for that.
> > 
> > You should use -C on this sort of thing, so that the interesting 
> > aspects of the patch are easier to see. (It actually comes out longer 
> > in this case, but it's far easier to tell that the code in the new 
> > file is the same as the old code.) Can you tell I've been rearranging 
> > a lot of code lately and trying to make the patches not look really 
> > scary?
> 
> Actually, I ended up touching this up a tiny bit, too: I ordered the 
> functions in bundle.c the way they were in builtin-bundle.c (so that the 
> patch is more trivial) and removed the blank lines at the end of the file. 
> This makes the "git diff -C" output really obvious. 

Makes sense.  Thanks.

> (Someday, I'd like to have a diff that can show that a substantial block 
> of '+' lines matches a block of lines from somewhere in the "before" 
> content, so reviewers can verify that the patch reorders code but 
> doesn't change it, or changes it in certain ways. But, of course, that's 
> both hard to generate and hard to display usefully.)

AFAIR from the thread after

http://thread.gmane.org/gmane.linux.kernel/484924/focus=37498

the problem was not so much the displaying as the applying.  You have a 
diff for builtin-bundle.c.  You want to move code to bundle.c.  git-diff 
has to rearrange the diff so that bundle.c can copy the code from 
builtin-bundle.c, and then it is deleted from builtin-bundle.c.  If you 
move code criss-cross, it is not possible.

However, if you do something like

diff --git a/builtin-bundle.c:10--20 b/bundle.c:0--30
code move
--- a/builtin-bundle.c:10--20
+++ b/bundle.c:1--11
diff --git ...

it should be possible.  We could allow generation of such a diff with 
--code-moves, which would detect if there is substantial evidence for a 
code move, and produce such a diff.

Note: this kind of code movement diff has to come before _both_ the diffs 
for builtin-bundle.c and bundle.c, since chances are that the code had to 
be touched a little here and there.  And probably you would want a little 
context, just to make sure it is the correct code when applying the patch.

Ciao,
Dscho

Re: [PATCH 1/3] Move bundle specific stuff into bundle.[ch]

[Daniel Barkalow]

On Wed, 18 Jul 2007, Johannes Schindelin wrote:

> Hi,
> 
> On Tue, 17 Jul 2007, Daniel Barkalow wrote:
> 
> > You should use -C on this sort of thing, so that the interesting aspects 
> > of the patch are easier to see. (It actually comes out longer in this 
> > case, but it's far easier to tell that the code in the new file is the 
> > same as the old code.)
> 
> Okay, I wanted it to be kept short, since I really get lost easily in 
> hundreds of "-" lines, with possibly one in the midst being a "+".

Actually, putting the functions in the original order made the -C diff 
shorter than without -C. In general, a hundred lines of '-' with maybe a 
'+' is hard to read, but I think whole functions of '-' without anything 
else are easy; scan the left column, and find that the whole thing goes 
away. If a patch is discarding blocks which are exactly whole top-level 
definitions, those changes are probably either correct or totally broken 
(depending on whether those blocks were actually used); you just have to 
get suspicious around preprocessor stuff. Your change ended up being 
trivially what the message described: bunch of blocks not in one or the 
other of the resulting files, some functions make not static, renamed, 
and/or had arguments changed, and the function you have to call after 
unbundle if you want it.

> > Aside from presentation, it looks good to me. Shall I stick the bundle 
> > changes into my series? I'd like to have them come before the patch to 
> > switch to builtin-fetch, so that there aren't any revisions where "git 
> > fetch" doesn't have bundle support.
> 
> Looks fine to me.  Seems like you should add a SOB line, too.

Ah, yes. I'll have to see if I'll be the first person in git development 
to have a SOB line that's neither first nor last. :)

> > And I think it would be best to take part 3 as a review fix to my final 
> > patch.
> 
> Yes, definitely.  This shows again (to me, at least), that just looking at 
> the code is not enough, you have to run it, too, to review patches.

You caught that by running it? I've been running this code, and I've never 
done anything with it which caused fetch_refs to fail and then checked the 
result. I thought you must have found it by looking for missing checks of 
return values. Or did you find it when you'd implemented half of bundle 
support and it didn't complain?

Re: [PATCH 1/3] Move bundle specific stuff into bundle.[ch]

[Johannes Schindelin]

Hi,

On Wed, 18 Jul 2007, Daniel Barkalow wrote:

> On Wed, 18 Jul 2007, Johannes Schindelin wrote:
> 
> > On Tue, 17 Jul 2007, Daniel Barkalow wrote:
> > 
> > > You should use -C on this sort of thing, so that the interesting aspects 
> > > of the patch are easier to see. (It actually comes out longer in this 
> > > case, but it's far easier to tell that the code in the new file is the 
> > > same as the old code.)
> > 
> > Okay, I wanted it to be kept short, since I really get lost easily in 
> > hundreds of "-" lines, with possibly one in the midst being a "+".
> 
> Actually, putting the functions in the original order made the -C diff 
> shorter than without -C.

True enough!

> > > Aside from presentation, it looks good to me. Shall I stick the 
> > > bundle changes into my series? I'd like to have them come before the 
> > > patch to switch to builtin-fetch, so that there aren't any revisions 
> > > where "git fetch" doesn't have bundle support.
> > 
> > Looks fine to me.  Seems like you should add a SOB line, too.
> 
> Ah, yes. I'll have to see if I'll be the first person in git development 
> to have a SOB line that's neither first nor last. :)

We have 28 commits ATM in next's history, having 3 SOB lines: 
1e76b702c1e754c7e6df1ced9ce6f1863cb7e092 is the most recent one.

> > > And I think it would be best to take part 3 as a review fix to my 
> > > final patch.
> > 
> > Yes, definitely.  This shows again (to me, at least), that just 
> > looking at the code is not enough, you have to run it, too, to review 
> > patches.
> 
> You caught that by running it? I've been running this code, and I've never 
> done anything with it which caused fetch_refs to fail and then checked the 
> result. I thought you must have found it by looking for missing checks of 
> return values. Or did you find it when you'd implemented half of bundle 
> support and it didn't complain?

Exactly.  It is the "bundle 1" test that failed.

Ciao,
Dscho