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

From: Thomas Rast <trast@inf.ethz.ch>
Date: Feb 22, 2012, 20:25 UTC
Message-ID: <87hayivcmm.fsf@thomas.inf.ethz.ch>
In-Reply-To: <7vlinuaaab.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

Show 21 quoted lines

> Thomas Rast <trast@student.ethz.ch> writes:
>
>>   # 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.
>
> 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:

Not sure. The only use I could think of would be to google for the subjects, in the hope of finding some repository that has the commit you are looking for. Other than that...

In any case the --pretty=oneline is very deliberate, as we can see from the commit below. It just doesn't give a reason :-)

commit 239296770dae75e21c179733785731ec6ffae1f5
Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Date:   Fri Feb 23 03:17:51 2007 +0100

    git-bundle: record commit summary in the prerequisite data

diff --git a/builtin-bundle.c b/builtin-bundle.c
index 191ec55..d74afaa 100644
--- a/builtin-bundle.c
+++ b/builtin-bundle.c
@@ -267,7 +267,7 @@ static int create_bundle(struct bundle_header *header, const char *path,
 		int argc, const char **argv)
 {
 	int bundle_fd = -1;
-	const char **argv_boundary = xmalloc((argc + 3) * sizeof(const char *));
+	const char **argv_boundary = xmalloc((argc + 4) * sizeof(const char *));
 	const char **argv_pack = xmalloc(4 * sizeof(const char *));
 	int pid, in, out, i, status;
 	char buffer[1024];
@@ -282,10 +282,11 @@ static int create_bundle(struct bundle_header *header, const char *path,
 	write_or_die(bundle_fd, bundle_signature, strlen(bundle_signature));
 
 	/* write prerequisites */
-	memcpy(argv_boundary + 2, argv + 1, argc * sizeof(const char *));
+	memcpy(argv_boundary + 3, argv + 1, argc * sizeof(const char *));
 	argv_boundary[0] = "rev-list";
 	argv_boundary[1] = "--boundary";
-	argv_boundary[argc + 1] = NULL;
+	argv_boundary[2] = "--pretty=oneline";
+	argv_boundary[argc + 2] = NULL;
 	out = -1;
 	pid = fork_with_pipe(argv_boundary, NULL, &out);
 	if (pid < 0)

-- 
Thomas Rast
trast@{inf,student}.ethz.ch