threads / patch / 9082

patch, 3 partsMove bundle specific stuff into bundle.[ch]

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

## tl;dr

11 messages between Jul 17, 2007 and Jul 18, 2007. Diffs are folded; open one to read it.

replies: 10people: 4as markdown or json

Johannes Schindelin· Jul 17, 2007, 22:49 UTC · lore

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
Show changes to 4 files +342 −325

Makefile, builtin-bundle.c, bundle.c, 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
Daniel Barkalow· Jul 18, 2007, 02:48 UTC · re: Johannes Schindelin · lore

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

On Tue, 17 Jul 2007, Johannes Schindelin wrote:
Show 6 quoted lines
> 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*
Daniel Barkalow· Jul 18, 2007, 03:23 UTC · re: Daniel Barkalow · lore

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

On Tue, 17 Jul 2007, Daniel Barkalow wrote:
Show 14 quoted lines
> 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*
Shawn O. Pearce· Jul 18, 2007, 03:29 UTC · re: Daniel Barkalow · lore

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

Daniel Barkalow <barkalow@iabervon.org> wrote:
Show 5 quoted lines
> (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.

Show changes to git-gui.sh +0 −0
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.
Junio C Hamano· Jul 18, 2007, 06:13 UTC · re: Shawn O. Pearce · lore

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

"Shawn O. Pearce" <spearce@spearce.org> writes:
Show 31 quoted lines
> 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.

Shawn O. Pearce· Jul 18, 2007, 06:19 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> wrote:
Show 18 quoted lines
> 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.
Junio C Hamano· Jul 18, 2007, 07:26 UTC · re: Shawn O. Pearce · lore

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

"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.

Johannes Schindelin· Jul 18, 2007, 10:09 UTC · re: Daniel Barkalow · lore

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

Hi,
On Tue, 17 Jul 2007, Daniel Barkalow wrote:
Show 22 quoted lines
> 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.
Show 5 quoted lines
> (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
Show changes to 2 files +0 −1

builtin-bundle.c:10--20, bundle.c:1--11

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
Johannes Schindelin· Jul 18, 2007, 09:56 UTC · re: Daniel Barkalow · lore

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

Hi,
On Tue, 17 Jul 2007, Daniel Barkalow wrote:
Show 13 quoted lines
> 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

Daniel Barkalow· Jul 18, 2007, 16:25 UTC · re: Johannes Schindelin · lore

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

On Wed, 18 Jul 2007, Johannes Schindelin wrote:
Show 11 quoted lines
> 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.

Show 6 quoted lines
> > 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. :)

Show 5 quoted lines
> > 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?

Johannes Schindelin· Jul 18, 2007, 16:33 UTC · re: Daniel Barkalow · lore

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

Hi,
On Wed, 18 Jul 2007, Daniel Barkalow wrote:
Show 14 quoted lines
> 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!
Show 9 quoted lines
> > > 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.

Show 12 quoted lines
> > > 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

← back to recent threads