From 5e8617f560968567c285bc2e9b0674f8f9d535cb Mon Sep 17 00:00:00 2001 From: Thomas Rast Date: Wed, 22 Feb 2012 20:34:22 +0100 Subject: bundle: put strbuf_readline_fd in strbuf.c with adjustments 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 Signed-off-by: Junio C Hamano --- 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 8a1d53ba29..0542093ee8 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 3ad2cc0016..692cf1bd9a 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 46a33f8c46..e42dbe5ac8 100644 --- a/strbuf.h +++ b/strbuf.h @@ -108,6 +108,7 @@ extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint); 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); -- cgit v1.2.1 From bc2fed496baa54ae99dede7da23dec938adbf0eb Mon Sep 17 00:00:00 2001 From: Thomas Rast Date: Wed, 22 Feb 2012 20:34:23 +0100 Subject: bundle: use a strbuf to scan the log for boundary commits 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 | 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 Signed-off-by: Thomas Rast Signed-off-by: Junio C Hamano --- bundle.c | 15 ++++++++------- t/t5704-bundle.sh | 16 ++++++++++++++++ 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/bundle.c b/bundle.c index 0542093ee8..4497343e56 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,20 +266,21 @@ 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; } } + strbuf_release(&buf); fclose(rls_fout); if (finish_command(&rls)) return error("rev-list died"); diff --git a/t/t5704-bundle.sh b/t/t5704-bundle.sh index 4ae127d106..af19994aa7 100755 --- a/t/t5704-bundle.sh +++ b/t/t5704-bundle.sh @@ -59,4 +59,20 @@ test_expect_success 'empty bundle file is rejected' ' ' +# This triggers a bug in older versions where the resulting 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 "%01200d\n" 0 | git commit -F - && + test_commit fifth && + git bundle create long-subject-bundle.bdl HEAD^..HEAD && + git bundle list-heads long-subject-bundle.bdl >heads && + test -s heads && + git fetch long-subject-bundle.bdl && + sed -n "/^-/{p;q}" long-subject-bundle.bdl >boundary && + grep "^-$_x40 " boundary +' + test_done -- cgit v1.2.1 From 1c5f93b9a69f4c1c3d09853243562b9ae3bf79a6 Mon Sep 17 00:00:00 2001 From: Thomas Rast Date: Thu, 23 Feb 2012 10:42:21 +0100 Subject: strbuf: improve strbuf_get*line documentation Clarify strbuf_getline() documentation, and add the missing documentation for strbuf_getwholeline() and strbuf_getwholeline_fd(). Signed-off-by: Thomas Rast Signed-off-by: Junio C Hamano --- Documentation/technical/api-strbuf.txt | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt index afe2759951..95a8bf3846 100644 --- a/Documentation/technical/api-strbuf.txt +++ b/Documentation/technical/api-strbuf.txt @@ -255,8 +255,24 @@ same behaviour as well. `strbuf_getline`:: - Read a line from a FILE* pointer. The second argument specifies the line + Read a line from a FILE *, overwriting the existing contents + of the strbuf. The second argument specifies the line terminator character, typically `'\n'`. + Reading stops after the terminator or at EOF. The terminator + is removed from the buffer before returning. Returns 0 unless + there was nothing left before EOF, in which case it returns `EOF`. + +`strbuf_getwholeline`:: + + Like `strbuf_getline`, but keeps the trailing terminator (if + any) in the buffer. + +`strbuf_getwholeline_fd`:: + + Like `strbuf_getwholeline`, but operates on a file descriptor. + It reads one character at a time, so it is very slow. Do not + use it unless you need the correct position in the file + descriptor. `stripspace`:: -- cgit v1.2.1 From 8a557bb77fc009b00f7952f0d3d6ebd33079f70e Mon Sep 17 00:00:00 2001 From: Thomas Rast Date: Thu, 23 Feb 2012 10:42:23 +0100 Subject: t5704: match tests to modern style The test did not adhere to the current style on several counts: . empty lines around the test blocks, but within the test string . ': > file' or even just '> file' with an extra space . inconsistent indentation . hand-rolled commits instead of using test_commit Fix all of them. There's a catch to the last point: test_commit creates a tag, which the original test did not create. We still change it to test_commit, and explicitly delete the tags, so as to highlight that the test relies on not having them. Signed-off-by: Thomas Rast Signed-off-by: Junio C Hamano --- t/t5704-bundle.sh | 33 ++++++++------------------------- 1 file changed, 8 insertions(+), 25 deletions(-) diff --git a/t/t5704-bundle.sh b/t/t5704-bundle.sh index af19994aa7..a51c8b0560 100755 --- a/t/t5704-bundle.sh +++ b/t/t5704-bundle.sh @@ -4,59 +4,42 @@ test_description='some bundle related tests' . ./test-lib.sh test_expect_success 'setup' ' - - : > file && - git add file && - test_tick && - git commit -m initial && + test_commit initial && test_tick && git tag -m tag tag && - : > file2 && - git add file2 && - : > file3 && - test_tick && - git commit -m second && - git add file3 && - test_tick && - git commit -m third - + test_commit second && + test_commit third && + git tag -d initial && + git tag -d second && + git tag -d third ' test_expect_success 'tags can be excluded by rev-list options' ' - git bundle create bundle --all --since=7.Apr.2005.15:16:00.-0700 && git ls-remote bundle > output && ! grep tag output - ' test_expect_success 'die if bundle file cannot be created' ' - mkdir adir && test_must_fail git bundle create adir --all - ' 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 ' ' - echo master | git bundle create hybrid-bundle.bdl --stdin tag && git ls-remote hybrid-bundle.bdl >output && grep master output - ' test_expect_success 'empty bundle file is rejected' ' - - >empty-bundle && test_must_fail git fetch empty-bundle - + : >empty-bundle && + test_must_fail git fetch empty-bundle ' # This triggers a bug in older versions where the resulting line (with -- cgit v1.2.1