Re: [PATCH 2/2] bundle: use a strbuf to scan the log for boundary commits

[Junio C Hamano <gitster@pobox.com>]

Thomas Rast <trast@student.ethz.ch> writes:

> The first part of the bundle header contains the boundary commits, and
> could be approximated by
>
>   # v2 git bundle
>   $(git rev-list --pretty=oneline --boundary <ARGS> | grep ^-)
>
> git-bundle actually spawns exactly this rev-list invocation, and does
> the grepping internally.
>
> There was a subtle bug in the latter step: it used fgets() with a
> 1024-byte buffer.  If the user has sufficiently long subjects (e.g.,
> by not adhering to the git oneline-subject convention in the first
> place), the 'oneline' format can easily overflow the buffer.  fgets()
> then returns the rest of the line in the next call(s).  If one of
> these remaining parts started with '-', git-bundle would mistakenly
> insert it into the bundle thinking it was a boundary commit.
>
> Fix it by using strbuf_getwholeline() instead, which handles arbitrary
> line lengths correctly.
>
> Note that on the receiving side in parse_bundle_header() we were
> already using strbuf_getwholeline_fd(), so that part is safe.

Thanks for diagnosing this, but I wonder if it even needs --pretty=oneline
to begin with, except for debugging purposes.

Do we ever use the subject string read from the rev-list output in any
way?

In other words, I am wondering if the right patch to minimally fix the
issue starting from older releases is something along this line instead:

diff --git a/bundle.c b/bundle.c
index b8acf3c..339dbb0 100644
--- a/bundle.c
+++ b/bundle.c
@@ -248,7 +248,7 @@ int create_bundle(struct bundle_header *header, const char *path,
 	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_boundary = xmalloc((argc + 3) * sizeof(const char *));
 	const char **argv_pack = xmalloc(6 * sizeof(const char *));
 	int i, ref_count = 0;
 	char buffer[1024];
@@ -271,11 +271,10 @@ int create_bundle(struct bundle_header *header, const char *path,
 	init_revisions(&revs, NULL);
 
 	/* write prerequisites */
-	memcpy(argv_boundary + 3, argv + 1, argc * sizeof(const char *));
+	memcpy(argv_boundary + 2, 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;
+	argv_boundary[argc + 1] = NULL;
 	memset(&rls, 0, sizeof(rls));
 	rls.argv = argv_boundary;
 	rls.out = -1;