threads / bug / 22278

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

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

## tl;dr

19 messages between Jan 19, 2010 and Jun 30, 2010.

replies: 18people: 4as markdown or json

Joey Hess· Jan 19, 2010, 00:26 UTC · lore

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
Johannes Schindelin· Jan 19, 2010, 23:52 UTC · re: Joey Hess · lore

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

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

Jonathan Nieder· Apr 19, 2010, 07:14 UTC · re: Joey Hess · lore

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

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

Jonathan Nieder· Apr 19, 2010, 08:03 UTC · re: Jonathan Nieder · lore

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

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
Jonathan Nieder· Apr 19, 2010, 08:03 UTC · re: Jonathan Nieder · lore

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

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
Jonathan Nieder· Jun 26, 2010, 06:17 UTC · re: Joey Hess · lore

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

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(-)
Jonathan Nieder· Jun 26, 2010, 06:19 UTC · re: Jonathan Nieder · lore

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

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
Jonathan Nieder· Jun 26, 2010, 06:20 UTC · re: Jonathan Nieder · lore

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

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
Junio C Hamano· Jun 30, 2010, 17:57 UTC · re: Jonathan Nieder · lore

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

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?

Jonathan Nieder· Jun 30, 2010, 20:34 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
Show 5 quoted lines
>> 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
Jonathan Nieder· Jun 26, 2010, 06:20 UTC · re: Jonathan Nieder · lore

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

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
Junio C Hamano· Jun 30, 2010, 18:04 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 59 quoted lines
> 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...
Jonathan Nieder· Jun 30, 2010, 20:37 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
Show 19 quoted lines
> 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.
Jonathan Nieder· Jun 26, 2010, 06:21 UTC · re: Jonathan Nieder · lore

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

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
Jonathan Nieder· Jun 26, 2010, 06:22 UTC · re: Jonathan Nieder · lore

[PATCH 5/8] bundle: reuse setup_revisions result

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
Jonathan Nieder· Jun 26, 2010, 06:28 UTC · re: Jonathan Nieder · lore

[PATCH 6/8] Fix bundle --stdin

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
Jonathan Nieder· Jun 26, 2010, 06:29 UTC · re: Jonathan Nieder · lore

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

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
Jonathan Nieder· Jun 26, 2010, 06:31 UTC · re: Jonathan Nieder · lore

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

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

← back to recent threads