bug: git-bundle create foo --stdin -> segfault

bug: git-bundle create foo --stdin -> segfault

[Joey Hess]

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

joey@gnu:~/tmp/new>echo master | git bundle create ../my.bundle --stdin
zsh: segmentation fault  git bundle create ../my.bundle --stdin

I noticed that git bundle --stdin actually attempts to read from stdin
past EOF. You can see this if you manually type into its stdin.

% git-bundle create ../bundle --stdin
master
^D
master
^D
fatal: Refusing to create empty bundle.

The first stdin read is done by the internal call to rev-list --stdin.
The second stdin read is done by the call to setup_revisions(),
which has its own handler for --stdin.

Bug seen with git version 1.6.5.7 / 1.6.6.243.gff6d2

I also tried going back to 22568f0a336ac37ae7329c917857b455839d1d09, but
still see a bug with Adam Brewster's initial code to add --stdin to
git-bundle. That code still tries to read stdin twice. If it sees
"master" both times, it does create a bundle.

-- 
see shy jo

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: bug: git-bundle create foo --stdin -> segfault

[Johannes Schindelin]

Hi,

On Mon, 18 Jan 2010, Joey Hess wrote:

> joey@gnu:~/tmp/new>echo master | git bundle create ../my.bundle --stdin
> zsh: segmentation fault  git bundle create ../my.bundle --stdin

Current 'next' fails, too.

Some previous Git versions failed with a message like this:

error: unrecognized argument: --stdin'

Other previous Git versions failed at least with a message such as this:

fatal: exec rev-list failed.
error: rev-list died

Something similar happens to me when I run the initial official revision 
of git-bundle. The reason is that "git rev-list --boundary 
--pretty=oneline --stdin" refuses to run.

The bad versions either segfault, or "refuse to create empty bundles".

And while 8b3dce5 purports to clean things up (even acknowledging that 
support code for --stdin was removed from bundle.c!), at that 
time git-bundle was obviously not tested/fixed.

Now, I invested a lot of time into the new Git wiki, and into trying to 
bisect this (it took many, many more steps than the suggested 13, and 
somewhere in between, the number of commits to be tested even increased!).

If you want to fix it, I suggest requiring --stdin to be the only 
parameter after the bundle file name, and adding a function using 
strbuf_getline() to parse the stdin into a string_list.  Once that is 
done, you can substitute the argv for the rev-list call with that list 
(for that, you need to prepopulate with "rev-list", "--boundary" and 
"--pretty=oneline").  You can reuse that list for the call to 
setup_revisions().

Alternatively, you can try to implement the rev-list --boundary by hand 
(the --pretty=oneline is only needed to get a boundary marker IIRC), 
taking care to reset the commit flags that were set in the process.  (We 
need to know the boundary commits before actually starting to write the 
pack, because the bundle file format dictates that the boundary commits 
are listed as prerequsites in the bundle header.)

If you want to go that route (which is arguably more elegant anyway), I 
suggest having a look at the merge_bases() and get_merge_bases() functions 
in commit.c, which do something similar (i.e. a revwalk without using 
revision.c's functions -- because you cannot tell what flags they will use 
in the future, and they have to be reset after the walk).

Ciao,
Dscho

[PATCH 0/2] test bundle --stdin, fix objects_array_remove_duplicates()

[Jonathan Nieder]

This series does not make ‘git bundle --stdin’ work.  In fact,
the bug that it does fix would not be visible if bundle --stdin
worked as it was supposed to.

Instead, this fixes a segfault that the bug triggers.  The
patches have been sitting in my git tree for a while.  They fix
a real problem regardless, and maybe they would help someone
to get bundle --stdin working properly.

Thoughts welcome, as always.
Jonathan Nieder (2):
  t5704 (bundle): add tests for bundle --stdin
  fix "bundle --stdin" segfault

 object.c          |    4 ++--
 t/t5704-bundle.sh |   16 ++++++++++++++++
 2 files changed, 18 insertions(+), 2 deletions(-)

As for how to fix bundle --stdin, Joey Hess wrote:

> I also tried going back to 22568f0a336ac37ae7329c917857b455839d1d09, but
> still see a bug with Adam Brewster's initial code to add --stdin to
> git-bundle. That code still tries to read stdin twice. If it sees
> "master" both times, it does create a bundle.

and Johannes Schindelin suggested slurping up the input and explicitly
using it twice, or:

> Alternatively, you can try to implement the rev-list --boundary by
> hand (the --pretty=oneline is only needed to get a boundary marker
> IIRC), taking care to reset the commit flags that were set in the
> process.
[...]
> If you want to go that route (which is arguably more elegant anyway),
> I suggest having a look at the merge_bases() and get_merge_bases()
> functions in commit.c

Dscho’s full message is pretty helpful:
http://thread.gmane.org/gmane.comp.version-control.git/137414/focus=137503

[PATCH 1/2] t5704 (bundle): add tests for bundle --stdin

[Jonathan Nieder]

As long as no rev-list arguments are supplied on the command line,
git bundle create --stdin currently segfaults.  With added rev-list
arguments, it does not segfault, but the revisions from stdin are
ignored.

Thanks to Joey Hess <joey@kitenet.net> for the report.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t5704-bundle.sh |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/t/t5704-bundle.sh b/t/t5704-bundle.sh
index a8f4419..1154a4e 100755
--- a/t/t5704-bundle.sh
+++ b/t/t5704-bundle.sh
@@ -30,4 +30,20 @@ test_expect_success 'tags can be excluded by rev-list options' '
 
 '
 
+test_expect_failure 'bundle --stdin' '
+
+	echo master | git bundle create stdin-bundle.bdl --stdin &&
+	git ls-remote stdin-bundle.bdl > output &&
+	grep master output
+
+'
+
+test_expect_failure 'bundle --stdin <rev-list options>' '
+
+	echo master | git bundle create hybrid-bundle.bdl --stdin tag &&
+	git ls-remote hybrid-bundle.bdl > output &&
+	grep master output
+
+'
+
 test_done
-- 
1.7.1.rc1

[PATCH 2/2] fix "bundle --stdin" segfault

[Jonathan Nieder]

When passed an empty list, objects_array_remove_duplicates() corrupts
it by changing the number of entries from 0 to 1.

The problem lies in the condition of its main loop:

	for (ref = 0; ref < array->nr - 1; ref++) {

The loop body manipulates the supplied object array.  In the case of
an empty array, it should not be doing anything at all.  But array->nr
is an unsigned quantity, so the code enters the loop, in particular
increasing array->nr.  Fix this by comparing (ref + 1 < array->nr)
instead.

This bug can be triggered by git bundle --stdin:

	$ echo HEAD | git bundle create some.bundle --stdin’
	Segmentation fault (core dumped)

The list of commits to bundle appears to be empty because of another
bug: by the time the revision-walking machinery gets to look at it,
standard input has already been consumed by rev-list, so
...remove_duplicates() gets an empty list of revisions.

After this patch, git bundle --stdin still does not work; it just
doesn’t segfault any more.

Reported-by: Joey Hess <joey@kitenet.net>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 object.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/object.c b/object.c
index 3ca92c4..277b3dd 100644
--- a/object.c
+++ b/object.c
@@ -252,10 +252,10 @@ void add_object_array_with_mode(struct object *obj, const char *name, struct obj
 
 void object_array_remove_duplicates(struct object_array *array)
 {
-	int ref, src, dst;
+	unsigned int ref, src, dst;
 	struct object_array_entry *objects = array->objects;
 
-	for (ref = 0; ref < array->nr - 1; ref++) {
+	for (ref = 0; ref + 1 < array->nr; ref++) {
 		for (src = ref + 1, dst = src;
 		     src < array->nr;
 		     src++) {
-- 
1.7.1.rc1

Re: [PATCH 0/2] test bundle --stdin, fix objects_array_remove_duplicates()

[Junio C Hamano]

Thanks, will queue.

[RFC/PATCH 0/8] Re: bug: git-bundle create foo --stdin -> segfault

[Jonathan Nieder]

Joey Hess wrote:

> I noticed that git bundle --stdin actually attempts to read from stdin
> past EOF.

Patch 5 below fixes that.  Sadly it is not the whole story and I had
to introduce a memory leak for all users of --stdin to make
‘bundle --stdin’ actually work (patch 6).

In the case of bundle --stdin, I think the leak is unavoidable, while
in general, I suspect a parameter is needed to allow the caller to
indicate whether the leak is needed (analogous to save_commit_buffer).

The rest of the patches should be comparatively pleasant.

Patches 1, 3, and 4 give various parts of bundle creation their own
functions, to make the code easier to digest;

Patch 2 teaches the basis discovery code (which currently forks a
rev-list --boundary process) to use the revision walker directly, to
prepare for patch 5.

Patch 5 looked like it would be the main point of the series: it saves
the list of revs including those read from stdin and resets the
revision walker after the boundary is found.  This way, stdin is only
read once and the underlying logic of the revision walk does not need
to be changed significantly.

Even with patch 5, bundle --stdin still finds no revisions to read.
The problem: to write the table of contents, the name passed on the
command line for each ref is needed.  Unfortunately, names passed
through stdin are kept in a temporary buffer and then freed; the
random gibberish read instead does not look like a meaningful ref
name, so no valid toc entries are found.

Patch 6 fixes this, at the cost of a memory leak.  Suggestions for
avoiding the leak would of course be welcome.

Patch 7 adopts a similar “fix” in a context where it is not needed.
This is just for illustration.

Patch 8 is an unrelated enhancement that came along the way.

Thanks to Joey for the report and Johannes for suggestions for fixing
it.

Thoughts?  Tests to add?
Jonathan Nieder (8):
  bundle: split basis discovery into its own function
  bundle: use libified rev-list --boundary
  bundle: split body of list_prerequisites() loop into its own function
  bundle: split table of contents output into its own function
  bundle: reuse setup_revisions result
  revision: Keep ref names after reading them from stdin
  bundle: Keep names of basis refs after discovery
  bundle_create: Do not exit when given no revs to bundle

 bundle.c          |  172 ++++++++++++++++++++++++++++++++++-------------------
 revision.c        |    2 +-
 t/t5704-bundle.sh |    4 +-
 3 files changed, 113 insertions(+), 65 deletions(-)

[PATCH 1/8] bundle: split basis discovery into its own function

[Jonathan Nieder]

create_bundle() is getting a little long.  Isolate a small
piece to work on without distractions.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 bundle.c |   58 ++++++++++++++++++++++++++++++++++------------------------
 1 files changed, 34 insertions(+), 24 deletions(-)

diff --git a/bundle.c b/bundle.c
index ff97adc..66948f4 100644
--- a/bundle.c
+++ b/bundle.c
@@ -193,35 +193,14 @@ static int is_tag_in_date_range(struct object *tag, struct rev_info *revs)
 		(revs->min_age == -1 || revs->min_age > date);
 }
 
-int create_bundle(struct bundle_header *header, const char *path,
-		int argc, const char **argv)
+static int list_prerequisites(int bundle_fd, struct rev_info *revs,
+		int argc, const char * const *argv)
 {
-	static struct lock_file lock;
-	int bundle_fd = -1;
-	int bundle_to_stdout;
 	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;
 	FILE *rls_fout;
 
-	bundle_to_stdout = !strcmp(path, "-");
-	if (bundle_to_stdout)
-		bundle_fd = 1;
-	else
-		bundle_fd = hold_lock_file_for_update(&lock, path,
-						      LOCK_DIE_ON_ERROR);
-
-	/* 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";
@@ -241,7 +220,7 @@ int create_bundle(struct bundle_header *header, const char *path,
 			if (!get_sha1_hex(buffer + 1, sha1)) {
 				struct object *object = parse_object(sha1);
 				object->flags |= UNINTERESTING;
-				add_pending_object(&revs, object, buffer);
+				add_pending_object(revs, object, buffer);
 			}
 		} else if (!get_sha1_hex(buffer, sha1)) {
 			struct object *object = parse_object(sha1);
@@ -251,6 +230,37 @@ int create_bundle(struct bundle_header *header, const char *path,
 	fclose(rls_fout);
 	if (finish_command(&rls))
 		return error("rev-list died");
+	return 0;
+}
+
+int create_bundle(struct bundle_header *header, const char *path,
+		int argc, const char **argv)
+{
+	static struct lock_file lock;
+	int bundle_fd = -1;
+	int bundle_to_stdout;
+	const char **argv_pack = xmalloc(5 * sizeof(const char *));
+	int i, ref_count = 0;
+	struct rev_info revs;
+	struct child_process rls;
+
+	bundle_to_stdout = !strcmp(path, "-");
+	if (bundle_to_stdout)
+		bundle_fd = 1;
+	else
+		bundle_fd = hold_lock_file_for_update(&lock, path,
+						      LOCK_DIE_ON_ERROR);
+
+	/* 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 */
+	if (list_prerequisites(bundle_fd, &revs, argc, argv))
+		return -1;
 
 	/* write references */
 	argc = setup_revisions(argc, argv, &revs, NULL);
-- 
1.7.1.198.g8d802

[PATCH 2/8] bundle: use libified rev-list --boundary

[Jonathan Nieder]

The revision walker produces structured output, which should be a
little easier to work with than the text from rev-list.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 bundle.c |   69 ++++++++++++++++++++++++++++++++++---------------------------
 1 files changed, 38 insertions(+), 31 deletions(-)

diff --git a/bundle.c b/bundle.c
index 66948f4..0dd2acb 100644
--- a/bundle.c
+++ b/bundle.c
@@ -196,40 +196,47 @@ static int is_tag_in_date_range(struct object *tag, struct rev_info *revs)
 static int list_prerequisites(int bundle_fd, struct rev_info *revs,
 		int argc, const char * const *argv)
 {
-	const char **argv_boundary = xmalloc((argc + 4) * sizeof(const char *));
-	char buffer[1024];
-	struct child_process rls;
-	FILE *rls_fout;
+	const char **argv_boundary = xmalloc((argc + 1) * sizeof(const char *));
+	struct rev_info boundary_revs;
+	struct commit *rev;
 
-	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;
-	rls_fout = xfdopen(rls.out, "r");
-	while (fgets(buffer, sizeof(buffer), rls_fout)) {
-		unsigned char sha1[20];
-		if (buffer[0] == '-') {
-			write_or_die(bundle_fd, buffer, strlen(buffer));
-			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;
+	memcpy(argv_boundary, argv, (argc + 1) * sizeof(const char *));
+
+	init_revisions(&boundary_revs, NULL);
+	boundary_revs.boundary = 1;
+	if (setup_revisions(argc, argv_boundary, &boundary_revs, NULL) > 1)
+		return error("unrecognized argument: %s", argv_boundary[1]);
+	if (prepare_revision_walk(&boundary_revs))
+		return error("revision walk setup failed");
+
+	while ((rev = get_revision(&boundary_revs))) {
+		if (rev->object.flags & BOUNDARY) {
+			struct strbuf buf = STRBUF_INIT;
+			struct pretty_print_context ctx = {0};
+			enum object_type type;
+			unsigned long size;
+
+			/*
+			 * The commit buffer is needed
+			 * to pretty-print boundary commits.
+			 */
+			rev->buffer = read_sha1_file(rev->object.sha1,
+							&type, &size);
+
+			strbuf_addch(&buf, '-');
+			strbuf_add(&buf, sha1_to_hex(rev->object.sha1), 40);
+			strbuf_addch(&buf, ' ');
+			pretty_print_commit(CMIT_FMT_ONELINE, rev, &buf, &ctx);
+			strbuf_addch(&buf, '\n');
+			write_or_die(bundle_fd, buf.buf, buf.len);
+
+			rev->object.flags |= UNINTERESTING;
+			add_pending_object(revs, &rev->object, buf.buf);
+			strbuf_release(&buf);
+		} else {
+			rev->object.flags |= SHOWN;
 		}
 	}
-	fclose(rls_fout);
-	if (finish_command(&rls))
-		return error("rev-list died");
 	return 0;
 }
 
-- 
1.7.1.198.g8d802

[PATCH 3/8] bundle: give list_prerequisites() loop body its own function

[Jonathan Nieder]

No functional change intended.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 bundle.c |   57 +++++++++++++++++++++++++++++++--------------------------
 1 files changed, 31 insertions(+), 26 deletions(-)

diff --git a/bundle.c b/bundle.c
index 0dd2acb..e90b5c5 100644
--- a/bundle.c
+++ b/bundle.c
@@ -193,6 +193,33 @@ static int is_tag_in_date_range(struct object *tag, struct rev_info *revs)
 		(revs->min_age == -1 || revs->min_age > date);
 }
 
+static void list_prerequisite(int bundle_fd, struct rev_info *revs,
+		struct commit *rev)
+{
+	struct strbuf buf = STRBUF_INIT;
+	struct pretty_print_context ctx = {0};
+	enum object_type type;
+	unsigned long size;
+
+	/*
+	 * The commit buffer is needed
+	 * to pretty-print boundary commits.
+	 */
+	rev->buffer = read_sha1_file(rev->object.sha1, &type, &size);
+
+	strbuf_addch(&buf, '-');
+	strbuf_add(&buf, sha1_to_hex(rev->object.sha1), 40);
+	strbuf_addch(&buf, ' ');
+	pretty_print_commit(CMIT_FMT_ONELINE, rev, &buf, &ctx);
+	strbuf_addch(&buf, '\n');
+
+	write_or_die(bundle_fd, buf.buf, buf.len);
+
+	rev->object.flags |= UNINTERESTING;
+	add_pending_object(revs, &rev->object, buf.buf);
+	strbuf_release(&buf);
+}
+
 static int list_prerequisites(int bundle_fd, struct rev_info *revs,
 		int argc, const char * const *argv)
 {
@@ -209,33 +236,11 @@ static int list_prerequisites(int bundle_fd, struct rev_info *revs,
 	if (prepare_revision_walk(&boundary_revs))
 		return error("revision walk setup failed");
 
-	while ((rev = get_revision(&boundary_revs))) {
-		if (rev->object.flags & BOUNDARY) {
-			struct strbuf buf = STRBUF_INIT;
-			struct pretty_print_context ctx = {0};
-			enum object_type type;
-			unsigned long size;
-
-			/*
-			 * The commit buffer is needed
-			 * to pretty-print boundary commits.
-			 */
-			rev->buffer = read_sha1_file(rev->object.sha1,
-							&type, &size);
-
-			strbuf_addch(&buf, '-');
-			strbuf_add(&buf, sha1_to_hex(rev->object.sha1), 40);
-			strbuf_addch(&buf, ' ');
-			pretty_print_commit(CMIT_FMT_ONELINE, rev, &buf, &ctx);
-			strbuf_addch(&buf, '\n');
-			write_or_die(bundle_fd, buf.buf, buf.len);
-
-			rev->object.flags |= UNINTERESTING;
-			add_pending_object(revs, &rev->object, buf.buf);
-			strbuf_release(&buf);
-		} else {
+	while ((rev = get_revision(revs))) {
+		if (rev->object.flags & BOUNDARY)
+			list_prerequisite(bundle_fd, revs, rev);
+		else
 			rev->object.flags |= SHOWN;
-		}
 	}
 	return 0;
 }
-- 
1.7.1.198.g8d802

[PATCH 4/8] bundle: split table of contents output into its own function

[Jonathan Nieder]

Isolate another piece of create_bundle() to work on.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 bundle.c |   78 +++++++++++++++++++++++++++++++++++---------------------------
 1 files changed, 44 insertions(+), 34 deletions(-)

diff --git a/bundle.c b/bundle.c
index e90b5c5..8ba6479 100644
--- a/bundle.c
+++ b/bundle.c
@@ -245,45 +245,20 @@ static int list_prerequisites(int bundle_fd, struct rev_info *revs,
 	return 0;
 }
 
-int create_bundle(struct bundle_header *header, const char *path,
-		int argc, const char **argv)
+static int bundle_list_refs(int bundle_fd,
+		int argc, const char **argv, struct rev_info *revs)
 {
-	static struct lock_file lock;
-	int bundle_fd = -1;
-	int bundle_to_stdout;
-	const char **argv_pack = xmalloc(5 * sizeof(const char *));
 	int i, ref_count = 0;
-	struct rev_info revs;
-	struct child_process rls;
 
-	bundle_to_stdout = !strcmp(path, "-");
-	if (bundle_to_stdout)
-		bundle_fd = 1;
-	else
-		bundle_fd = hold_lock_file_for_update(&lock, path,
-						      LOCK_DIE_ON_ERROR);
-
-	/* 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 */
-	if (list_prerequisites(bundle_fd, &revs, argc, argv))
-		return -1;
-
-	/* write references */
-	argc = setup_revisions(argc, argv, &revs, NULL);
+	argc = setup_revisions(argc, argv, revs, NULL);
 
 	if (argc > 1)
 		return error("unrecognized argument: %s'", argv[1]);
 
-	object_array_remove_duplicates(&revs.pending);
+	object_array_remove_duplicates(&revs->pending);
 
-	for (i = 0; i < revs.pending.nr; i++) {
-		struct object_array_entry *e = revs.pending.objects + i;
+	for (i = 0; i < revs->pending.nr; i++) {
+		struct object_array_entry *e = revs->pending.objects + i;
 		unsigned char sha1[20];
 		char *ref;
 		const char *display_ref;
@@ -298,7 +273,7 @@ int create_bundle(struct bundle_header *header, const char *path,
 		display_ref = (flag & REF_ISSYMREF) ? e->name : ref;
 
 		if (e->item->type == OBJ_TAG &&
-				!is_tag_in_date_range(e->item, &revs)) {
+				!is_tag_in_date_range(e->item, revs)) {
 			e->item->flags |= UNINTERESTING;
 			continue;
 		}
@@ -344,7 +319,7 @@ int create_bundle(struct bundle_header *header, const char *path,
 				 */
 				obj = parse_object(sha1);
 				obj->flags |= SHOWN;
-				add_pending_object(&revs, obj, e->name);
+				add_pending_object(revs, obj, e->name);
 			}
 			free(ref);
 			continue;
@@ -358,7 +333,42 @@ int create_bundle(struct bundle_header *header, const char *path,
 		free(ref);
 	}
 	if (!ref_count)
-		die ("Refusing to create empty bundle.");
+		die("Refusing to create empty bundle.");
+	return 0;
+}
+
+int create_bundle(struct bundle_header *header, const char *path,
+		int argc, const char **argv)
+{
+	static struct lock_file lock;
+	int bundle_fd = -1;
+	int bundle_to_stdout;
+	const char **argv_pack = xmalloc(5 * sizeof(const char *));
+	struct rev_info revs;
+	struct child_process rls;
+	int i;
+
+	bundle_to_stdout = !strcmp(path, "-");
+	if (bundle_to_stdout)
+		bundle_fd = 1;
+	else
+		bundle_fd = hold_lock_file_for_update(&lock, path,
+						      LOCK_DIE_ON_ERROR);
+
+	/* 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 */
+	if (list_prerequisites(bundle_fd, &revs, argc, argv))
+		return -1;
+
+	/* write references */
+	if (bundle_list_refs(bundle_fd, argc, argv, &revs))
+		return -1;
 
 	/* end header */
 	write_or_die(bundle_fd, "\n", 1);
-- 
1.7.1.198.g8d802

[PATCH 5/8] bundle: reuse setup_revisions result

[Jonathan Nieder]

Avoid reading stdin twice for bundle --stdin.

Reported-by: Joey Hess <joey@kitenet.net>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 bundle.c |   57 +++++++++++++++++++++++++++++++++++++--------------------
 1 files changed, 37 insertions(+), 20 deletions(-)

diff --git a/bundle.c b/bundle.c
index 8ba6479..311c554 100644
--- a/bundle.c
+++ b/bundle.c
@@ -193,7 +193,17 @@ static int is_tag_in_date_range(struct object *tag, struct rev_info *revs)
 		(revs->min_age == -1 || revs->min_age > date);
 }
 
-static void list_prerequisite(int bundle_fd, struct rev_info *revs,
+static void object_array_copy(struct object_array *dest, const struct object_array *src)
+{
+	int i;
+
+	for (i = 0; i < src->nr; i++) {
+		struct object_array_entry *e = src->objects + i;
+		add_object_array_with_mode(e->item, e->name, dest, e->mode);
+	}
+}
+
+static void list_prerequisite(int bundle_fd, struct object_array *pending,
 		struct commit *rev)
 {
 	struct strbuf buf = STRBUF_INIT;
@@ -216,45 +226,52 @@ static void list_prerequisite(int bundle_fd, struct rev_info *revs,
 	write_or_die(bundle_fd, buf.buf, buf.len);
 
 	rev->object.flags |= UNINTERESTING;
-	add_pending_object(revs, &rev->object, buf.buf);
+	add_object_array(&rev->object, buf.buf, pending);
 	strbuf_release(&buf);
 }
 
 static int list_prerequisites(int bundle_fd, struct rev_info *revs,
-		int argc, const char * const *argv)
+		int argc, const char **argv)
 {
-	const char **argv_boundary = xmalloc((argc + 1) * sizeof(const char *));
-	struct rev_info boundary_revs;
 	struct commit *rev;
+	struct object_array pending_copy = { 0, 0, NULL };
 
-	memcpy(argv_boundary, argv, (argc + 1) * sizeof(const char *));
+	revs->simplify_history = 0;
+	argc = setup_revisions(argc, argv, revs, NULL);
+	if (argc > 1)
+		return error("unrecognized argument: %s", argv[1]);
 
-	init_revisions(&boundary_revs, NULL);
-	boundary_revs.boundary = 1;
-	if (setup_revisions(argc, argv_boundary, &boundary_revs, NULL) > 1)
-		return error("unrecognized argument: %s", argv_boundary[1]);
-	if (prepare_revision_walk(&boundary_revs))
+	if (revs->reflog_info)
+		return error("bundle does not support --walk-reflogs");
+	if (revs->no_walk)
+		return error("bundle does not support --no-walk");
+	if (revs->simplify_history)
+		return error("bundle and history simplification do not mix");
+
+	object_array_copy(&pending_copy, &revs->pending);
+
+	revs->boundary = 1;
+	if (prepare_revision_walk(revs))
 		return error("revision walk setup failed");
 
 	while ((rev = get_revision(revs))) {
 		if (rev->object.flags & BOUNDARY)
-			list_prerequisite(bundle_fd, revs, rev);
+			list_prerequisite(bundle_fd, &pending_copy, rev);
 		else
 			rev->object.flags |= SHOWN;
 	}
+
+	if (revs->pending.objects != NULL)
+		return error("%u revisions pending after boundary walk",
+						revs->pending.nr);
+	revs->pending = pending_copy;
 	return 0;
 }
 
-static int bundle_list_refs(int bundle_fd,
-		int argc, const char **argv, struct rev_info *revs)
+static int bundle_list_refs(int bundle_fd, struct rev_info *revs)
 {
 	int i, ref_count = 0;
 
-	argc = setup_revisions(argc, argv, revs, NULL);
-
-	if (argc > 1)
-		return error("unrecognized argument: %s'", argv[1]);
-
 	object_array_remove_duplicates(&revs->pending);
 
 	for (i = 0; i < revs->pending.nr; i++) {
@@ -367,7 +384,7 @@ int create_bundle(struct bundle_header *header, const char *path,
 		return -1;
 
 	/* write references */
-	if (bundle_list_refs(bundle_fd, argc, argv, &revs))
+	if (bundle_list_refs(bundle_fd, &revs))
 		return -1;
 
 	/* end header */
-- 
1.7.1.198.g8d802

[PATCH 6/8] Fix bundle --stdin

[Jonathan Nieder]

To write a bundle’s table of contents, the name passed on the command
line for each ref is needed.  Unfortunately, names passed through
stdin are put in a temporary buffer and then freed; the random
gibberish that tends to replace them does not look like a meaningful
ref name to bundle_list_refs(), so no valid toc entries are found and
‘git bundle --stdin’ thinks it has been asked to create an empty
bundle.

So teach the revision walker to keep rev names after reading them from
stdin.  This fixes ‘git bundle --stdin’ at the cost of a memory leak.

Reported-by: Joey Hess <joey@kitenet.net>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 revision.c        |    2 +-
 t/t5704-bundle.sh |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/revision.c b/revision.c
index f4b8b38..cf86af3 100644
--- a/revision.c
+++ b/revision.c
@@ -1022,7 +1022,7 @@ static void read_revisions_from_stdin(struct rev_info *revs, const char ***prune
 			}
 			die("options not supported in --stdin mode");
 		}
-		if (handle_revision_arg(sb.buf, revs, 0, 1))
+		if (handle_revision_arg(xstrdup(sb.buf), revs, 0, 1))
 			die("bad revision '%s'", sb.buf);
 	}
 	if (seen_dashdash)
diff --git a/t/t5704-bundle.sh b/t/t5704-bundle.sh
index ddc3dc5..cc463f3 100755
--- a/t/t5704-bundle.sh
+++ b/t/t5704-bundle.sh
@@ -30,7 +30,7 @@ test_expect_success 'tags can be excluded by rev-list options' '
 
 '
 
-test_expect_failure 'bundle --stdin' '
+test_expect_success 'bundle --stdin' '
 
 	echo master | git bundle create stdin-bundle.bdl --stdin &&
 	git ls-remote stdin-bundle.bdl >output &&
@@ -38,7 +38,7 @@ test_expect_failure 'bundle --stdin' '
 
 '
 
-test_expect_failure 'bundle --stdin <rev-list options>' '
+test_expect_success 'bundle --stdin <rev-list options>' '
 
 	echo master | git bundle create hybrid-bundle.bdl --stdin tag &&
 	git ls-remote hybrid-bundle.bdl >output &&
-- 
1.7.1.198.g8d802

[RFC/PATCH 7/8] bundle: Keep names of basis refs after discovery

[Jonathan Nieder]

Without this change, attempts to examine revs->pending.objects[i].name
when debugging produce random giberish.  On the other hand, it
introduces a small per-basis-ref memory leak.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
For illustration.

 bundle.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/bundle.c b/bundle.c
index 311c554..7aff369 100644
--- a/bundle.c
+++ b/bundle.c
@@ -226,8 +226,7 @@ static void list_prerequisite(int bundle_fd, struct object_array *pending,
 	write_or_die(bundle_fd, buf.buf, buf.len);
 
 	rev->object.flags |= UNINTERESTING;
-	add_object_array(&rev->object, buf.buf, pending);
-	strbuf_release(&buf);
+	add_object_array(&rev->object, strbuf_detach(&buf, NULL), pending);
 }
 
 static int list_prerequisites(int bundle_fd, struct rev_info *revs,
-- 
1.7.1.198.g8d802

[PATCH 8/8] bundle_create: Do not exit when given no revs to bundle

[Jonathan Nieder]

Return an error instead to help with the libification effort.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
That’s the end of the series.  Thanks for reading.

I was a bit torn about whether to present this as a request for
comment or a patch series ready for application.  On one hand it fixes
a bug; on the other hand, I have very little confidence that it works
well in the presence of arbitrary rev-list options.  Thoughts and
testing would be very welcome.

Good night,
Jonathan

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

diff --git a/bundle.c b/bundle.c
index 7aff369..7dd3f65 100644
--- a/bundle.c
+++ b/bundle.c
@@ -349,7 +349,7 @@ static int bundle_list_refs(int bundle_fd, struct rev_info *revs)
 		free(ref);
 	}
 	if (!ref_count)
-		die("Refusing to create empty bundle.");
+		return error("Refusing to create empty bundle.");
 	return 0;
 }
 
-- 
1.7.1.198.g8d802

Re: [PATCH 2/8] bundle: use libified rev-list --boundary

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> The revision walker produces structured output, which should be a
> little easier to work with than the text from rev-list.

Hmm, doesn't it negatively affect later traversal you would need to do if
you smudged the flag bits by running revision traversal like this?

Re: [PATCH 3/8] bundle: give list_prerequisites() loop body its own function

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> No functional change intended.
>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
>  bundle.c |   57 +++++++++++++++++++++++++++++++--------------------------
>  1 files changed, 31 insertions(+), 26 deletions(-)
>
> diff --git a/bundle.c b/bundle.c
> index 0dd2acb..e90b5c5 100644
> --- a/bundle.c
> +++ b/bundle.c
> @@ -193,6 +193,33 @@ static int is_tag_in_date_range(struct object *tag, struct rev_info *revs)
>  		(revs->min_age == -1 || revs->min_age > date);
>  }
>  
> +static void list_prerequisite(int bundle_fd, struct rev_info *revs,
> +		struct commit *rev)
> +{
> +	struct strbuf buf = STRBUF_INIT;
> +	struct pretty_print_context ctx = {0};
> +	enum object_type type;
> +	unsigned long size;
> +
> +	/*
> +	 * The commit buffer is needed
> +	 * to pretty-print boundary commits.
> +	 */
> +	rev->buffer = read_sha1_file(rev->object.sha1, &type, &size);
> +
> +	strbuf_addch(&buf, '-');
> +	strbuf_add(&buf, sha1_to_hex(rev->object.sha1), 40);
> +	strbuf_addch(&buf, ' ');
> +	pretty_print_commit(CMIT_FMT_ONELINE, rev, &buf, &ctx);
> +	strbuf_addch(&buf, '\n');
> +
> +	write_or_die(bundle_fd, buf.buf, buf.len);
> +
> +	rev->object.flags |= UNINTERESTING;
> +	add_pending_object(revs, &rev->object, buf.buf);
> +	strbuf_release(&buf);
> +}
> +
>  static int list_prerequisites(int bundle_fd, struct rev_info *revs,
>  		int argc, const char * const *argv)
>  {
> @@ -209,33 +236,11 @@ static int list_prerequisites(int bundle_fd, struct rev_info *revs,
>  	if (prepare_revision_walk(&boundary_revs))
>  		return error("revision walk setup failed");
>  
> -	while ((rev = get_revision(&boundary_revs))) {
> -		if (rev->object.flags & BOUNDARY) {
> -...
> -		} else {
> +	while ((rev = get_revision(revs))) {
> +		if (rev->object.flags & BOUNDARY)
> +			list_prerequisite(bundle_fd, revs, rev);
> +		else
>  			rev->object.flags |= SHOWN;
> -		}

You used to walk boundary_revs but now you walk revs that is given by the
caller, exhausting the revs.pending the caller wanted to use later to feed
pack_objects with?

Confused...

Re: [PATCH 2/8] bundle: use libified rev-list --boundary

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> The revision walker produces structured output, which should be a
>> little easier to work with than the text from rev-list.
>
> Hmm, doesn't it negatively affect later traversal you would need to do if
> you smudged the flag bits by running revision traversal like this?

I imagine so.  I fear this would be the first git command to use the
revision walker twice, and I am not sure whether we can really make that
work.

The revision walker uses object flags for the following purposes:

 - marking objects uninteresting/SYMMETRIC_LEFT.  Luckily for us, if
   an object is uninteresting or SYMMETRIC_LEFT for the first
   --boundary walk, it will be likewise for pack-objects, too.

 - history simplification (TREESAME), --cherry-pick (SHOWN),
   --merge (SYMMETRIC_LEFT).  There are already other reasons to
   disallow these features for bundle.

 - add_parents_to_list (ADDED, SEEN).  This one is really worrisome;
   should we walk through again to throw away the added parents?
   Should there be a pass through all revisions to clear the ADDED
   bit?

I’ll figure out a one-pass solution. :(

Jonathan

Re: [PATCH 3/8] bundle: give list_prerequisites() loop body its own function

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> @@ -209,33 +236,11 @@ static int list_prerequisites(int bundle_fd, struct rev_info *revs,
>>  	if (prepare_revision_walk(&boundary_revs))
>>  		return error("revision walk setup failed");
>>  
>> -	while ((rev = get_revision(&boundary_revs))) {
>> -		if (rev->object.flags & BOUNDARY) {
>> -...
>> -		} else {
>> +	while ((rev = get_revision(revs))) {
>> +		if (rev->object.flags & BOUNDARY)
>> +			list_prerequisite(bundle_fd, revs, rev);
>> +		else
>>  			rev->object.flags |= SHOWN;
>> -		}
>
> You used to walk boundary_revs but now you walk revs that is given by the
> caller

Agh!  Typo.  Thanks for catching it.