Problems with unrecognized headers in git bundles

Hi,

creating bundles from some repositories seems to lead to bundles with 
incorrectly formatted headers, at least with git >= 1.7.2. When cloning 
from such bundles, git prints the following error/warning:

   $ git clone perl-clone.bundle perl-clone
   Cloning into 'perl-clone'...
   warning: unrecognized header: --work around mangled archname on...

This can be reproduced easily with git from any version >= 1.7.2 or from 
master, using the following steps:

   git clone git://perl5.git.perl.org/perl.git perl
   GIT_DIR=perl/.git git bundle create perl-clone.bundle --all
   git clone perl-clone.bundle perl-clone

The content of the bundle is:

   # v2 git bundle
   -- work around mangled archname on win32 while finding...
   39ec54a59ce332fc44e553f4e5eeceef88e8369e refs/heads/blead
   39ec54a59ce332fc44e553f4e5eeceef88e8369e refs/remotes/origin/HEAD
   ...

The "--work around mangled archname..." line is rather long, so I've 
omitted most of it. What it contains is a series of commit messages all 
combined into a single line. It appears that this line is the problem 
because it's neither a comment (like '#v2 git bundle') nor a SHA1 
followed by a ref name.

Note that this problem does not occur with all repositories. A bundle 
created from a test repository with a single text file and just one 
commit does not have this problem.

Note also that "git clone <bundle> <dest>" does not fail with corrupted 
bundles if the git version is something like 1.7.2 or 1.7.7.6. Those 
version print the above warning but still succeed in cloning. "git 
clone" from master however treats this as an error and fails.

Without any knowledge of how git works internally, I would assume that 
this is either a bug in how bundles are created, or a hint at a slightly 
broken perl repository (other's have the same thing though, perhaps it 
is because of a conversion from another SCM software to git in the 
past). Does this sound reasonable?

Is there a way to work around this or fix it properly? I'm not sure what 
lead to the decision to no longer ignore unrecognized headers in git 
master, would it be sensible to revert this change if nothing can be 
done to solve this during bundle creation?

   - Jannis

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.

Reported-by: Jannis Pohlmann <redacted>
Signed-off-by: Thomas Rast <redacted>
---
 bundle.c          |   14 +++++++-------
 t/t5704-bundle.sh |   17 +++++++++++++++++
 2 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/bundle.c b/bundle.c
index 313de42..0dbd174 100644
--- a/bundle.c
+++ b/bundle.c

@@ -234,7 +234,7 @@ int create_bundle(struct bundle_header *header, const char *path,
 	const char **argv_boundary = xmalloc((argc + 4) * sizeof(const char *));
 	const char **argv_pack = xmalloc(6 * sizeof(const char *));
 	int i, ref_count = 0;
-	char buffer[1024];
+	struct strbuf buf = STRBUF_INIT;
 	struct rev_info revs;
 	struct child_process rls;
 	FILE *rls_fout;

@@ -266,16 +266,16 @@ int create_bundle(struct bundle_header *header, const char *path,
 	if (start_command(&rls))
 		return -1;
 	rls_fout = xfdopen(rls.out, "r");
-	while (fgets(buffer, sizeof(buffer), rls_fout)) {
+	while (strbuf_getwholeline(&buf, rls_fout, '\n') != EOF) {
 		unsigned char sha1[20];
-		if (buffer[0] == '-') {
-			write_or_die(bundle_fd, buffer, strlen(buffer));
-			if (!get_sha1_hex(buffer + 1, sha1)) {
+		if (buf.len > 0 && buf.buf[0] == '-') {
+			write_or_die(bundle_fd, buf.buf, buf.len);
+			if (!get_sha1_hex(buf.buf + 1, sha1)) {
 				struct object *object = parse_object(sha1);
 				object->flags |= UNINTERESTING;
-				add_pending_object(&revs, object, buffer);
+				add_pending_object(&revs, object, buf.buf);
 			}
-		} else if (!get_sha1_hex(buffer, sha1)) {
+		} else if (!get_sha1_hex(buf.buf, sha1)) {
 			struct object *object = parse_object(sha1);
 			object->flags |= SHOWN;
 		}

diff --git a/t/t5704-bundle.sh b/t/t5704-bundle.sh
index 4ae127d..7c2f307 100755
--- a/t/t5704-bundle.sh
+++ b/t/t5704-bundle.sh

@@ -59,4 +59,21 @@ test_expect_success 'empty bundle file is rejected' '
 
 '
 
+# If "ridiculous" is at least 1004 chars, this traps a bug in old
+# versions where the resulting 1025-char line (with --pretty=oneline)
+# was longer than a 1024-char buffer
+test_expect_success 'ridiculously long subject in boundary' '
+
+	: > file4 &&
+	test_tick &&
+	git add file4 &&
+	printf "abcdefghijkl %s\n" $(seq 1 100) | git commit -F - &&
+	test_commit fifth &&
+	git bundle create long-subject-bundle.bdl HEAD^..HEAD &&
+	git fetch long-subject-bundle.bdl &&
+	sed -n "/^-/{p;q}" long-subject-bundle.bdl > boundary &&
+	grep "^-$_x40 " boundary
+
+'
+
 test_done

-- 
1.7.9.1.430.g4998543

The comment even said that it should eventually go there.  While at
it, match the calling convention and name of the function to the
strbuf_get*line family.  So it now is strbuf_getwholeline_fd.

Signed-off-by: Thomas Rast <redacted>
---

I was left looking for strbuf_readline_fd() for a while, then tried to
look for the readline() from the libc that it would presumably match.
Hence this cleanup.

 bundle.c |   21 ++-------------------
 strbuf.c |   16 ++++++++++++++++
 strbuf.h |    1 +
 3 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/bundle.c b/bundle.c
index b8acf3c..313de42 100644
--- a/bundle.c
+++ b/bundle.c

@@ -23,23 +23,6 @@ static void add_to_ref_list(const unsigned char *sha1, const char *name,
 	list->nr++;
 }
 
-/* Eventually this should go to strbuf.[ch] */
-static int strbuf_readline_fd(struct strbuf *sb, int fd)
-{
-	strbuf_reset(sb);
-
-	while (1) {
-		char ch;
-		ssize_t len = xread(fd, &ch, 1);
-		if (len <= 0)
-			return len;
-		strbuf_addch(sb, ch);
-		if (ch == '\n')
-			break;
-	}
-	return 0;
-}
-
 static int parse_bundle_header(int fd, struct bundle_header *header,
 			       const char *report_path)
 {

@@ -47,7 +30,7 @@ static int parse_bundle_header(int fd, struct bundle_header *header,
 	int status = 0;
 
 	/* The bundle header begins with the signature */
-	if (strbuf_readline_fd(&buf, fd) ||
+	if (strbuf_getwholeline_fd(&buf, fd, '\n') ||
 	    strcmp(buf.buf, bundle_signature)) {
 		if (report_path)
 			error("'%s' does not look like a v2 bundle file",

@@ -57,7 +40,7 @@ static int parse_bundle_header(int fd, struct bundle_header *header,
 	}
 
 	/* The bundle header ends with an empty line */
-	while (!strbuf_readline_fd(&buf, fd) &&
+	while (!strbuf_getwholeline_fd(&buf, fd, '\n') &&
 	       buf.len && buf.buf[0] != '\n') {
 		unsigned char sha1[20];
 		int is_prereq = 0;

diff --git a/strbuf.c b/strbuf.c
index ff0b96b..5135d59 100644
--- a/strbuf.c
+++ b/strbuf.c

@@ -383,6 +383,22 @@ int strbuf_getline(struct strbuf *sb, FILE *fp, int term)
 	return 0;
 }
 
+int strbuf_getwholeline_fd(struct strbuf *sb, int fd, int term)
+{
+	strbuf_reset(sb);
+
+	while (1) {
+		char ch;
+		ssize_t len = xread(fd, &ch, 1);
+		if (len <= 0)
+			return EOF;
+		strbuf_addch(sb, ch);
+		if (ch == term)
+			break;
+	}
+	return 0;
+}
+
 int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint)
 {
 	int fd, len;

diff --git a/strbuf.h b/strbuf.h
index fbf059f..3effaa8 100644
--- a/strbuf.h
+++ b/strbuf.h

@@ -116,6 +116,7 @@ static inline void strbuf_complete_line(struct strbuf *sb)
 
 extern int strbuf_getwholeline(struct strbuf *, FILE *, int);
 extern int strbuf_getline(struct strbuf *, FILE *, int);
+extern int strbuf_getwholeline_fd(struct strbuf *, int, int);
 
 extern void stripspace(struct strbuf *buf, int skip_comments);
 extern int launch_editor(const char *path, struct strbuf *buffer, const char *const *env);

-- 
1.7.9.1.430.g4998543

On 22 February 2012 17:05, Jannis Pohlmann wrote:

Hi,

creating bundles from some repositories seems to lead to bundles with
incorrectly formatted headers, at least with git >= 1.7.2. When
cloning from such bundles, git prints the following error/warning:

 $ git clone perl-clone.bundle perl-clone
 Cloning into 'perl-clone'...
 warning: unrecognized header: --work around mangled archname on...

This can be reproduced easily with git from any version >= 1.7.2 or
from master, using the following steps:

 git clone git://perl5.git.perl.org/perl.git perl
 GIT_DIR=perl/.git git bundle create perl-clone.bundle --all
 git clone perl-clone.bundle perl-clone

The content of the bundle is:

 # v2 git bundle
 -- work around mangled archname on win32 while finding...
 39ec54a59ce332fc44e553f4e5eeceef88e8369e refs/heads/blead
 39ec54a59ce332fc44e553f4e5eeceef88e8369e refs/remotes/origin/HEAD

Have researched this a bit, and I've found that all git versions back to
when git-bundle was introduced (around v1.5.4) produces the same invalid
line. The culprit is commit 3e8148feadabd0d0b1869fcc4d218a6475a5b0bc in
perl.git, branch 'maint-5.005'. The log message of that commit contains
email headers, maybe that's the reason git bundle gets confused?

        Øyvind

On 22 February 2012 21:25, Øyvind A. Holm [off-list ref] wrote:

On 22 February 2012 17:05, Jannis Pohlmann wrote:

quoted

creating bundles from some repositories seems to lead to bundles
with incorrectly formatted headers, at least with git >= 1.7.2. When
cloning from such bundles, git prints the following error/warning:

 $ git clone perl-clone.bundle perl-clone
 Cloning into 'perl-clone'...
 warning: unrecognized header: --work around mangled archname on...

Have researched this a bit, and I've found that all git versions back
to when git-bundle was introduced (around v1.5.4) produces the same
invalid line. The culprit is commit
3e8148feadabd0d0b1869fcc4d218a6475a5b0bc in perl.git, branch
'maint-5.005'. The log message of that commit contains email headers,
maybe that's the reason git bundle gets confused?

...or maybe because the log message doesn't contain any empty lines, so
they're joined together into an insanely long line. I've seen this
behaviour before, in git-am or git-apply, I think. Anyway, when the
bundle doesn't contain this commit, the line is not present.

  Øyvind

On Wed, Feb 22, 2012 at 08:34:22PM +0100, Thomas Rast wrote:

The comment even said that it should eventually go there.  While at
it, match the calling convention and name of the function to the
strbuf_get*line family.  So it now is strbuf_getwholeline_fd.
[...]
 bundle.c |   21 ++-------------------
 strbuf.c |   16 ++++++++++++++++
 strbuf.h |    1 +

Nit: no update to Documentation/technical/api-strbuf.txt.

You might want to also mention that this will read() one byte at a time,
and is therefore only a good idea if you really care about the position
of the fd. Otherwise, fdopen() + strbuf_getwholeline() is much more
efficient.

-Peff

On Wed, Feb 22, 2012 at 08:34:23PM +0100, Thomas Rast wrote:

+# If "ridiculous" is at least 1004 chars, this traps a bug in old
+# versions where the resulting 1025-char line (with --pretty=oneline)
+# was longer than a 1024-char buffer
+test_expect_success 'ridiculously long subject in boundary' '
+
+	: > file4 &&
+	test_tick &&
+	git add file4 &&
+	printf "abcdefghijkl %s\n" $(seq 1 100) | git commit -F - &&

Seq is not portable. I usually use either

  perl -le "print for (1..100)"

or just do:

  z16=zzzzzzzzzzzzzzzz
  z256=$z16$z16$z16$z16$z16$z16$z16$z16
  z1024=$z256$z256$z256$z256$z256$z256$z256$z256

-Peff

Am 22.02.2012 21:55, schrieb Jeff King:

On Wed, Feb 22, 2012 at 08:34:23PM +0100, Thomas Rast wrote:

quoted

+	printf "abcdefghijkl %s\n" $(seq 1 100) | git commit -F - &&

Seq is not portable.

Thanks for pointing this out.

I usually use either

  perl -le "print for (1..100)"

or just do:

  z16=zzzzzzzzzzzzzzzz
  z256=$z16$z16$z16$z16$z16$z16$z16$z16
  z1024=$z256$z256$z256$z256$z256$z256$z256$z256

If a sequence of ASCII zeros is good enough, you can also do:

  printf %02134d 0

-- Hannes

On Wed, Feb 22, 2012 at 9:25 PM, Øyvind A. Holm [off-list ref] wrote:

On 22 February 2012 17:05, Jannis Pohlmann wrote:

quoted

Hi,

creating bundles from some repositories seems to lead to bundles with
incorrectly formatted headers, at least with git >= 1.7.2. When
cloning from such bundles, git prints the following error/warning:

 $ git clone perl-clone.bundle perl-clone
 Cloning into 'perl-clone'...
 warning: unrecognized header: --work around mangled archname on...

This can be reproduced easily with git from any version >= 1.7.2 or
from master, using the following steps:

 git clone git://perl5.git.perl.org/perl.git perl
 GIT_DIR=perl/.git git bundle create perl-clone.bundle --all
 git clone perl-clone.bundle perl-clone

The content of the bundle is:

 # v2 git bundle
 -- work around mangled archname on win32 while finding...
 39ec54a59ce332fc44e553f4e5eeceef88e8369e refs/heads/blead
 39ec54a59ce332fc44e553f4e5eeceef88e8369e refs/remotes/origin/HEAD

Have researched this a bit, and I've found that all git versions back to
when git-bundle was introduced (around v1.5.4) produces the same invalid
line. The culprit is commit 3e8148feadabd0d0b1869fcc4d218a6475a5b0bc in
perl.git, branch 'maint-5.005'. The log message of that commit contains
email headers, maybe that's the reason git bundle gets confused?

For the lazy, the commit can be found here:

http://perl5.git.perl.org/perl.git/commit/3e8148feadabd0d0b1869fcc4d218a6475a5b0bc