Merge branch 'jc/boundary'

* jc/boundary:
  git-bundle: prevent overwriting existing bundles
  git-bundle: die if a given ref is not included in bundle
  git-bundle: handle thin packs in subcommand "unbundle"
  git-bundle: Make thin packs
  git-bundle: avoid packing objects which are in the prerequisites
  bundle: fix wrong check of read_header()'s return value & add tests
  revision --boundary: fix uncounted case.
  revision --boundary: fix stupid typo
  git-bundle: make verify a bit more chatty.
  revision traversal: SHOWN means shown
  git-bundle: various fixups
  revision traversal: retire BOUNDARY_SHOW
  revision walker: Fix --boundary when limited

4 files changed, 243 insertions, 197 deletions

diff --git a/builtin-bundle.c b/builtin-bundle.c
index 279b8f8e5..55f6d0abc 100644
--- a/builtin-bundle.c
+++ b/builtin-bundle.c
@@ -160,7 +160,28 @@ static int fork_with_pipe(const char **argv, int *in, int *out)
 	return pid;
 }
 
-static int verify_bundle(struct bundle_header *header)
+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
@@ -179,7 +200,7 @@ static int verify_bundle(struct bundle_header *header)
 		struct ref_list_entry *e = p->list + i;
 		struct object *o = parse_object(e->sha1);
 		if (o) {
-			o->flags |= BOUNDARY_SHOW;
+			o->flags |= PREREQ_MARK;
 			add_pending_object(&revs, o, e->name);
 			continue;
 		}
@@ -187,7 +208,7 @@ static int verify_bundle(struct bundle_header *header)
 			error(message);
 		error("%s %s", sha1_to_hex(e->sha1), e->name);
 	}
-	if (revs.pending.nr == 0)
+	if (revs.pending.nr != p->nr)
 		return ret;
 	req_nr = revs.pending.nr;
 	setup_revisions(2, argv, &revs, NULL);
@@ -202,7 +223,7 @@ static int verify_bundle(struct bundle_header *header)
 
 	i = req_nr;
 	while (i && (commit = get_revision(&revs)))
-		if (commit->object.flags & BOUNDARY_SHOW)
+		if (commit->object.flags & PREREQ_MARK)
 			i--;
 
 	for (i = 0; i < req_nr; i++)
@@ -216,56 +237,24 @@ static int verify_bundle(struct bundle_header *header)
 	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)
 {
-	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_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;
-	}
-}
-
-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_or_die(1, sha1_to_hex(p->item->sha1), 40);
-	write_or_die(1, " ", 1);
-	if (len)
-		write_or_die(1, p->name, len);
-	write_or_die(1, "\n", 1);
-}
-
-static void show_edge(struct commit *commit)
-{
-	; /* nothing to do */
+	return list_refs(&header->references, argc, argv);
 }
 
 static int create_bundle(struct bundle_header *header, const char *path,
@@ -273,19 +262,23 @@ static int create_bundle(struct bundle_header *header, const char *path,
 {
 	int bundle_fd = -1;
 	const char **argv_boundary = xmalloc((argc + 4) * sizeof(const char *));
-	const char **argv_pack = xmalloc(4 * sizeof(const char *));
+	const char **argv_pack = xmalloc(5 * 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));
+			open(path, O_CREAT | O_EXCL | O_WRONLY, 0666));
 	if (bundle_fd < 0)
-		return error("Could not write to '%s'", path);
+		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";
@@ -296,9 +289,20 @@ static int create_bundle(struct bundle_header *header, const char *path,
 	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] == '-')
+	while ((i = read_string(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;
+		}
+	}
 	while ((i = waitpid(pid, &status, 0)) < 0)
 		if (errno != EINTR)
 			return error("rev-list died");
@@ -306,27 +310,32 @@ static int create_bundle(struct bundle_header *header, const char *path,
 		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_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);
-		}
+		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))
+			die("ref '%s' is excluded by the rev-list options",
+				e->name);
+		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 */
@@ -336,36 +345,42 @@ static int create_bundle(struct bundle_header *header, const char *path,
 	argv_pack[0] = "pack-objects";
 	argv_pack[1] = "--all-progress";
 	argv_pack[2] = "--stdout";
-	argv_pack[3] = NULL;
+	argv_pack[3] = "--thin";
+	argv_pack[4] = 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);
-	dup2(in, 1);
+	for (i = 0; i < revs.pending.nr; i++) {
+		struct object *object = revs.pending.objects[i].item;
+		if (object->flags & UNINTERESTING)
+			write(in, "^", 1);
+		write(in, sha1_to_hex(object->sha1), 40);
+		write(in, "\n", 1);
+	}
 	close(in);
-	prepare_revision_walk(&revs);
-	mark_edges_uninteresting(revs.commits, &revs, show_edge);
-	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;
+
+	return status;
 }
 
 static int unbundle(struct bundle_header *header, int bundle_fd,
 		int argc, const char **argv)
 {
-	const char *argv_index_pack[] = {"index-pack", "--stdin", NULL};
+	const char *argv_index_pack[] = {"index-pack",
+		"--fix-thin", "--stdin", NULL};
 	int pid, status, dev_null;
 
-	if (verify_bundle(header))
+	if (verify_bundle(header, 0))
 		return -1;
 	dev_null = open("/dev/null", O_WRONLY);
+	if (dev_null < 0)
+		return error("Could not open /dev/null");
 	pid = fork_with_pipe(argv_index_pack, &bundle_fd, &dev_null);
 	if (pid < 0)
 		return error("Could not spawn index-pack");
@@ -402,12 +417,12 @@ 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)))
+			(bundle_fd = read_header(bundle_file, &header)) < 0)
 		return 1;
 
 	if (!strcmp(cmd, "verify")) {
 		close(bundle_fd);
-		if (verify_bundle(&header))
+		if (verify_bundle(&header, 1))
 			return 1;
 		fprintf(stderr, "%s is okay\n", bundle_file);
 		return 0;
@@ -427,4 +442,3 @@ int cmd_bundle(int argc, const char **argv, const char *prefix)
 	} else
 		usage(bundle_usage);
 }
-
diff --git a/revision.c b/revision.c
index f5b8ae4f0..3c2eb125e 100644
--- a/revision.c
+++ b/revision.c
@@ -437,36 +437,6 @@ static void limit_list(struct rev_info *revs)
 			continue;
 		p = &commit_list_insert(commit, p)->next;
 	}
-	if (revs->boundary) {
-		/* mark the ones that are on the result list first */
-		for (list = newlist; list; list = list->next) {
-			struct commit *commit = list->item;
-			commit->object.flags |= TMP_MARK;
-		}
-		for (list = newlist; list; list = list->next) {
-			struct commit *commit = list->item;
-			struct object *obj = &commit->object;
-			struct commit_list *parent;
-			if (obj->flags & UNINTERESTING)
-				continue;
-			for (parent = commit->parents;
-			     parent;
-			     parent = parent->next) {
-				struct commit *pcommit = parent->item;
-				if (!(pcommit->object.flags & UNINTERESTING))
-					continue;
-				pcommit->object.flags |= BOUNDARY;
-				if (pcommit->object.flags & TMP_MARK)
-					continue;
-				pcommit->object.flags |= TMP_MARK;
-				p = &commit_list_insert(pcommit, p)->next;
-			}
-		}
-		for (list = newlist; list; list = list->next) {
-			struct commit *commit = list->item;
-			commit->object.flags &= ~TMP_MARK;
-		}
-	}
 	revs->commits = newlist;
 }
 
@@ -1193,17 +1163,6 @@ static void rewrite_parents(struct rev_info *revs, struct commit *commit)
 	}
 }
 
-static void mark_boundary_to_show(struct commit *commit)
-{
-	struct commit_list *p = commit->parents;
-	while (p) {
-		commit = p->item;
-		p = p->next;
-		if (commit->object.flags & BOUNDARY)
-			commit->object.flags |= BOUNDARY_SHOW;
-	}
-}
-
 static int commit_match(struct commit *commit, struct rev_info *opt)
 {
 	if (!opt->grep_filter)
@@ -1235,15 +1194,9 @@ static struct commit *get_revision_1(struct rev_info *revs)
 		 */
 		if (!revs->limited) {
 			if (revs->max_age != -1 &&
-			    (commit->date < revs->max_age)) {
-				if (revs->boundary)
-					commit->object.flags |=
-						BOUNDARY_SHOW | BOUNDARY;
-				else
-					continue;
-			} else
-				add_parents_to_list(revs, commit,
-						&revs->commits);
+			    (commit->date < revs->max_age))
+				continue;
+			add_parents_to_list(revs, commit, &revs->commits);
 		}
 		if (commit->object.flags & SHOWN)
 			continue;
@@ -1252,18 +1205,6 @@ static struct commit *get_revision_1(struct rev_info *revs)
 						    revs->ignore_packed))
 		    continue;
 
-		/* We want to show boundary commits only when their
-		 * children are shown.  When path-limiter is in effect,
-		 * rewrite_parents() drops some commits from getting shown,
-		 * and there is no point showing boundary parents that
-		 * are not shown.  After rewrite_parents() rewrites the
-		 * parents of a commit that is shown, we mark the boundary
-		 * parents with BOUNDARY_SHOW.
-		 */
-		if (commit->object.flags & BOUNDARY_SHOW) {
-			commit->object.flags |= SHOWN;
-			return commit;
-		}
 		if (commit->object.flags & UNINTERESTING)
 			continue;
 		if (revs->min_age != -1 && (commit->date > revs->min_age))
@@ -1286,80 +1227,136 @@ static struct commit *get_revision_1(struct rev_info *revs)
 			if (revs->parents)
 				rewrite_parents(revs, commit);
 		}
-		if (revs->boundary)
-			mark_boundary_to_show(commit);
-		commit->object.flags |= SHOWN;
 		return commit;
 	} while (revs->commits);
 	return NULL;
 }
 
+static void gc_boundary(struct object_array *array)
+{
+	unsigned nr = array->nr;
+	unsigned alloc = array->alloc;
+	struct object_array_entry *objects = array->objects;
+
+	if (alloc <= nr) {
+		unsigned i, j;
+		for (i = j = 0; i < nr; i++) {
+			if (objects[i].item->flags & SHOWN)
+				continue;
+			if (i != j)
+				objects[j] = objects[i];
+			j++;
+		}
+		for (i = j; i < nr; i++)
+			objects[i].item = NULL;
+		array->nr = j;
+	}
+}
+
 struct commit *get_revision(struct rev_info *revs)
 {
 	struct commit *c = NULL;
+	struct commit_list *l;
+
+	if (revs->boundary == 2) {
+		unsigned i;
+		struct object_array *array = &revs->boundary_commits;
+		struct object_array_entry *objects = array->objects;
+		for (i = 0; i < array->nr; i++) {
+			c = (struct commit *)(objects[i].item);
+			if (!c)
+				continue;
+			if (!(c->object.flags & CHILD_SHOWN))
+				continue;
+			if (!(c->object.flags & SHOWN))
+				break;
+		}
+		if (array->nr <= i)
+			return NULL;
 
-	if (revs->reverse) {
-		struct commit_list *list;
+		c->object.flags |= SHOWN | BOUNDARY;
+		return c;
+	}
 
-		/*
-		 * rev_info.reverse is used to note the fact that we
-		 * want to output the list of revisions in reverse
-		 * order.  To accomplish this goal, reverse can have
-		 * different values:
-		 *
-		 *  0  do nothing
-		 *  1  reverse the list
-		 *  2  internal use:  we have already obtained and
-		 *     reversed the list, now we only need to yield
-		 *     its items.
-		 */
+	if (revs->reverse) {
+		int limit = -1;
 
-		if (revs->reverse == 1) {
-			revs->reverse = 0;
-			list = NULL;
-			while ((c = get_revision(revs)))
-				commit_list_insert(c, &list);
-			revs->commits = list;
-			revs->reverse = 2;
+		if (0 <= revs->max_count) {
+			limit = revs->max_count;
+			if (0 < revs->skip_count)
+				limit += revs->skip_count;
 		}
-
-		if (!revs->commits)
-			return NULL;
-		c = revs->commits->item;
-		list = revs->commits->next;
-		free(revs->commits);
-		revs->commits = list;
-		return c;
+		l = NULL;
+		while ((c = get_revision_1(revs))) {
+			commit_list_insert(c, &l);
+			if ((0 < limit) && !--limit)
+				break;
+		}
+		revs->commits = l;
+		revs->reverse = 0;
+		revs->max_count = -1;
+		c = NULL;
 	}
 
-	if (0 < revs->skip_count) {
-		while ((c = get_revision_1(revs)) != NULL) {
-			if (revs->skip_count-- <= 0)
+	/*
+	 * Now pick up what they want to give us
+	 */
+	c = get_revision_1(revs);
+	if (c) {
+		while (0 < revs->skip_count) {
+			revs->skip_count--;
+			c = get_revision_1(revs);
+			if (!c)
 				break;
 		}
 	}
 
-	/* Check the max_count ... */
+	/*
+	 * Check the max_count.
+	 */
 	switch (revs->max_count) {
 	case -1:
 		break;
 	case 0:
-		if (revs->boundary) {
-			struct commit_list *list = revs->commits;
-			while (list) {
-				list->item->object.flags |=
-					BOUNDARY_SHOW | BOUNDARY;
-				list = list->next;
-			}
-			/* all remaining commits are boundary commits */
-			revs->max_count = -1;
-			revs->limited = 1;
-		} else
-			return NULL;
+		c = NULL;
+		break;
 	default:
 		revs->max_count--;
 	}
+
 	if (c)
+		c->object.flags |= SHOWN;
+
+	if (!revs->boundary) {
 		return c;
-	return get_revision_1(revs);
+	}
+
+	if (!c) {
+		/*
+		 * get_revision_1() runs out the commits, and
+		 * we are done computing the boundaries.
+		 * switch to boundary commits output mode.
+		 */
+		revs->boundary = 2;
+		return get_revision(revs);
+	}
+
+	/*
+	 * boundary commits are the commits that are parents of the
+	 * ones we got from get_revision_1() but they themselves are
+	 * not returned from get_revision_1().  Before returning
+	 * 'c', we need to mark its parents that they could be boundaries.
+	 */
+
+	for (l = c->parents; l; l = l->next) {
+		struct object *p;
+		p = &(l->item->object);
+		if (p->flags & (CHILD_SHOWN | SHOWN))
+			continue;
+		p->flags |= CHILD_SHOWN;
+		gc_boundary(&revs->boundary_commits);
+		add_object_array(p, NULL, &revs->boundary_commits);
+	}
+
+	return c;
 }
diff --git a/revision.h b/revision.h
index cf3371360..6ae39e6be 100644
--- a/revision.h
+++ b/revision.h
@@ -7,7 +7,7 @@
 #define SHOWN		(1u<<3)
 #define TMP_MARK	(1u<<4) /* for isolated cases; clean after use */
 #define BOUNDARY	(1u<<5)
-#define BOUNDARY_SHOW	(1u<<6)
+#define CHILD_SHOWN	(1u<<6)
 #define ADDED		(1u<<7)	/* Parents already parsed and added? */
 #define SYMMETRIC_LEFT	(1u<<8)
 
@@ -21,6 +21,9 @@ struct rev_info {
 	struct commit_list *commits;
 	struct object_array pending;
 
+	/* Parents of shown commits */
+	struct object_array boundary_commits;
+
 	/* Basic information */
 	const char *prefix;
 	void *prune_data;
@@ -40,10 +43,10 @@ struct rev_info {
 			edge_hint:1,
 			limited:1,
 			unpacked:1, /* see also ignore_packed below */
-			boundary:1,
+			boundary:2,
 			left_right:1,
 			parents:1,
-			reverse:2;
+			reverse:1;
 
 	/* Diff flags */
 	unsigned int	diff:1,
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index fa76662dc..ee3f397a9 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -90,6 +90,13 @@ test_expect_success 'create bundle 1' '
 	git bundle create bundle1 master^..master
 '
 
+test_expect_success 'header of bundle looks right' '
+	head -n 1 "$D"/bundle1 | grep "^#" &&
+	head -n 2 "$D"/bundle1 | grep "^-[0-9a-f]\{40\} " &&
+	head -n 3 "$D"/bundle1 | grep "^[0-9a-f]\{40\} " &&
+	head -n 4 "$D"/bundle1 | grep "^$"
+'
+
 test_expect_success 'create bundle 2' '
 	cd "$D" &&
 	git bundle create bundle2 master~2..master
@@ -101,10 +108,35 @@ test_expect_failure 'unbundle 1' '
 	git fetch "$D/bundle1" master:master
 '
 
+test_expect_success 'bundle 1 has only 3 files ' '
+	cd "$D" &&
+	(
+		while read x && test -n "$x"
+		do
+			:;
+		done
+		cat
+	) <bundle1 >bundle.pack &&
+	git index-pack bundle.pack &&
+	verify=$(git verify-pack -v bundle.pack) &&
+	test 4 = $(echo "$verify" | wc -l)
+'
+
 test_expect_success 'unbundle 2' '
 	cd "$D/bundle" &&
 	git fetch ../bundle2 master:master &&
 	test "tip" = "$(git log -1 --pretty=oneline master | cut -b42-)"
 '
 
+test_expect_success 'bundle does not prerequisite objects' '
+	cd "$D" &&
+	touch file2 &&
+	git add file2 &&
+	git commit -m add.file2 file2 &&
+	git bundle create bundle3 -1 HEAD &&
+	sed "1,4d" < bundle3 > bundle.pack &&
+	git index-pack bundle.pack &&
+	test 4 = $(git verify-pack -v bundle.pack | wc -l)
+'
+
 test_done