{"thread":{"id":"29696","subject":"[PATCH 2/2] bundle: use a strbuf to scan the log for boundary commits","startedAt":"2012-02-22T19:34:22Z","lastAt":"2012-02-23T03:38:13Z","messageCount":11,"participants":["Thomas Rast","Junio C Hamano","Jeff King","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"185188","messageId":"a795f6dca5e7c3fc5f9212becda4a46116c502b7.1329939233.git.trast@student.ethz.ch","threadId":"29696","inReplyTo":"4F451259.7010304@codethink.co.uk","subject":"[PATCH 1/2] bundle: put strbuf_readline_fd in strbuf.c with adjustments","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2012-02-22T19:34:22Z","receivedAt":"2012-02-22T19:34:22Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"The comment even said that it should eventually go there.  While at\nit, match the calling convention and name of the function to the\nstrbuf_get*line family.  So it now is strbuf_getwholeline_fd.\n\nSigned-off-by: Thomas Rast <trast@student.ethz.ch>\n---\n\nI was left looking for strbuf_readline_fd() for a while, then tried to\nlook for the readline() from the libc that it would presumably match.\nHence this cleanup.\n\n bundle.c |   21 ++-------------------\n strbuf.c |   16 ++++++++++++++++\n strbuf.h |    1 +\n 3 files changed, 19 insertions(+), 19 deletions(-)\n\ndiff --git a/bundle.c b/bundle.c\nindex b8acf3c..313de42 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -23,23 +23,6 @@ static void add_to_ref_list(const unsigned char *sha1, const char *name,\n \tlist->nr++;\n }\n \n-/* Eventually this should go to strbuf.[ch] */\n-static int strbuf_readline_fd(struct strbuf *sb, int fd)\n-{\n-\tstrbuf_reset(sb);\n-\n-\twhile (1) {\n-\t\tchar ch;\n-\t\tssize_t len = xread(fd, &ch, 1);\n-\t\tif (len <= 0)\n-\t\t\treturn len;\n-\t\tstrbuf_addch(sb, ch);\n-\t\tif (ch == '\\n')\n-\t\t\tbreak;\n-\t}\n-\treturn 0;\n-}\n-\n static int parse_bundle_header(int fd, struct bundle_header *header,\n \t\t\t       const char *report_path)\n {\n@@ -47,7 +30,7 @@ static int parse_bundle_header(int fd, struct bundle_header *header,\n \tint status = 0;\n \n \t/* The bundle header begins with the signature */\n-\tif (strbuf_readline_fd(&buf, fd) ||\n+\tif (strbuf_getwholeline_fd(&buf, fd, '\\n') ||\n \t    strcmp(buf.buf, bundle_signature)) {\n \t\tif (report_path)\n \t\t\terror(\"'%s' does not look like a v2 bundle file\",\n@@ -57,7 +40,7 @@ static int parse_bundle_header(int fd, struct bundle_header *header,\n \t}\n \n \t/* The bundle header ends with an empty line */\n-\twhile (!strbuf_readline_fd(&buf, fd) &&\n+\twhile (!strbuf_getwholeline_fd(&buf, fd, '\\n') &&\n \t       buf.len && buf.buf[0] != '\\n') {\n \t\tunsigned char sha1[20];\n \t\tint is_prereq = 0;\ndiff --git a/strbuf.c b/strbuf.c\nindex ff0b96b..5135d59 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -383,6 +383,22 @@ int strbuf_getline(struct strbuf *sb, FILE *fp, int term)\n \treturn 0;\n }\n \n+int strbuf_getwholeline_fd(struct strbuf *sb, int fd, int term)\n+{\n+\tstrbuf_reset(sb);\n+\n+\twhile (1) {\n+\t\tchar ch;\n+\t\tssize_t len = xread(fd, &ch, 1);\n+\t\tif (len <= 0)\n+\t\t\treturn EOF;\n+\t\tstrbuf_addch(sb, ch);\n+\t\tif (ch == term)\n+\t\t\tbreak;\n+\t}\n+\treturn 0;\n+}\n+\n int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint)\n {\n \tint fd, len;\ndiff --git a/strbuf.h b/strbuf.h\nindex fbf059f..3effaa8 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -116,6 +116,7 @@ static inline void strbuf_complete_line(struct strbuf *sb)\n \n extern int strbuf_getwholeline(struct strbuf *, FILE *, int);\n extern int strbuf_getline(struct strbuf *, FILE *, int);\n+extern int strbuf_getwholeline_fd(struct strbuf *, int, int);\n \n extern void stripspace(struct strbuf *buf, int skip_comments);\n extern int launch_editor(const char *path, struct strbuf *buffer, const char *const *env);\n-- \n1.7.9.1.430.g4998543\n"},{"id":"185187","messageId":"fa1553d59714fd89fdab1bf54af19ac631a30a8c.1329939233.git.trast@student.ethz.ch","threadId":"29696","inReplyTo":"a795f6dca5e7c3fc5f9212becda4a46116c502b7.1329939233.git.trast@student.ethz.ch","subject":"[PATCH 2/2] bundle: use a strbuf to scan the log for boundary commits","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2012-02-22T19:34:23Z","receivedAt":"2012-02-22T19:34:23Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"The first part of the bundle header contains the boundary commits, and\ncould be approximated by\n\n  # v2 git bundle\n  $(git rev-list --pretty=oneline --boundary <ARGS> | grep ^-)\n\ngit-bundle actually spawns exactly this rev-list invocation, and does\nthe grepping internally.\n\nThere was a subtle bug in the latter step: it used fgets() with a\n1024-byte buffer.  If the user has sufficiently long subjects (e.g.,\nby not adhering to the git oneline-subject convention in the first\nplace), the 'oneline' format can easily overflow the buffer.  fgets()\nthen returns the rest of the line in the next call(s).  If one of\nthese remaining parts started with '-', git-bundle would mistakenly\ninsert it into the bundle thinking it was a boundary commit.\n\nFix it by using strbuf_getwholeline() instead, which handles arbitrary\nline lengths correctly.\n\nNote that on the receiving side in parse_bundle_header() we were\nalready using strbuf_getwholeline_fd(), so that part is safe.\n\nReported-by: Jannis Pohlmann <jannis.pohlmann@codethink.co.uk>\nSigned-off-by: Thomas Rast <trast@student.ethz.ch>\n---\n bundle.c          |   14 +++++++-------\n t/t5704-bundle.sh |   17 +++++++++++++++++\n 2 files changed, 24 insertions(+), 7 deletions(-)\n\ndiff --git a/bundle.c b/bundle.c\nindex 313de42..0dbd174 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -234,7 +234,7 @@ int create_bundle(struct bundle_header *header, const char *path,\n \tconst char **argv_boundary = xmalloc((argc + 4) * sizeof(const char *));\n \tconst char **argv_pack = xmalloc(6 * sizeof(const char *));\n \tint i, ref_count = 0;\n-\tchar buffer[1024];\n+\tstruct strbuf buf = STRBUF_INIT;\n \tstruct rev_info revs;\n \tstruct child_process rls;\n \tFILE *rls_fout;\n@@ -266,16 +266,16 @@ int create_bundle(struct bundle_header *header, const char *path,\n \tif (start_command(&rls))\n \t\treturn -1;\n \trls_fout = xfdopen(rls.out, \"r\");\n-\twhile (fgets(buffer, sizeof(buffer), rls_fout)) {\n+\twhile (strbuf_getwholeline(&buf, rls_fout, '\\n') != EOF) {\n \t\tunsigned char sha1[20];\n-\t\tif (buffer[0] == '-') {\n-\t\t\twrite_or_die(bundle_fd, buffer, strlen(buffer));\n-\t\t\tif (!get_sha1_hex(buffer + 1, sha1)) {\n+\t\tif (buf.len > 0 && buf.buf[0] == '-') {\n+\t\t\twrite_or_die(bundle_fd, buf.buf, buf.len);\n+\t\t\tif (!get_sha1_hex(buf.buf + 1, sha1)) {\n \t\t\t\tstruct object *object = parse_object(sha1);\n \t\t\t\tobject->flags |= UNINTERESTING;\n-\t\t\t\tadd_pending_object(&revs, object, buffer);\n+\t\t\t\tadd_pending_object(&revs, object, buf.buf);\n \t\t\t}\n-\t\t} else if (!get_sha1_hex(buffer, sha1)) {\n+\t\t} else if (!get_sha1_hex(buf.buf, sha1)) {\n \t\t\tstruct object *object = parse_object(sha1);\n \t\t\tobject->flags |= SHOWN;\n \t\t}\ndiff --git a/t/t5704-bundle.sh b/t/t5704-bundle.sh\nindex 4ae127d..7c2f307 100755\n--- a/t/t5704-bundle.sh\n+++ b/t/t5704-bundle.sh\n@@ -59,4 +59,21 @@ test_expect_success 'empty bundle file is rejected' '\n \n '\n \n+# If \"ridiculous\" is at least 1004 chars, this traps a bug in old\n+# versions where the resulting 1025-char line (with --pretty=oneline)\n+# was longer than a 1024-char buffer\n+test_expect_success 'ridiculously long subject in boundary' '\n+\n+\t: > file4 &&\n+\ttest_tick &&\n+\tgit add file4 &&\n+\tprintf \"abcdefghijkl %s\\n\" $(seq 1 100) | git commit -F - &&\n+\ttest_commit fifth &&\n+\tgit bundle create long-subject-bundle.bdl HEAD^..HEAD &&\n+\tgit fetch long-subject-bundle.bdl &&\n+\tsed -n \"/^-/{p;q}\" long-subject-bundle.bdl > boundary &&\n+\tgrep \"^-$_x40 \" boundary\n+\n+'\n+\n test_done\n-- \n1.7.9.1.430.g4998543\n"},{"id":"185193","messageId":"7vlinuaaab.fsf@alter.siamese.dyndns.org","threadId":"29696","inReplyTo":"fa1553d59714fd89fdab1bf54af19ac631a30a8c.1329939233.git.trast@student.ethz.ch","subject":"Re: [PATCH 2/2] bundle: use a strbuf to scan the log for boundary commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-22T20:22:04Z","receivedAt":"2012-02-22T20:22:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast <trast@student.ethz.ch> writes:\n\n> The first part of the bundle header contains the boundary commits, and\n> could be approximated by\n>\n>   # v2 git bundle\n>   $(git rev-list --pretty=oneline --boundary <ARGS> | grep ^-)\n>\n> git-bundle actually spawns exactly this rev-list invocation, and does\n> the grepping internally.\n>\n> There was a subtle bug in the latter step: it used fgets() with a\n> 1024-byte buffer.  If the user has sufficiently long subjects (e.g.,\n> by not adhering to the git oneline-subject convention in the first\n> place), the 'oneline' format can easily overflow the buffer.  fgets()\n> then returns the rest of the line in the next call(s).  If one of\n> these remaining parts started with '-', git-bundle would mistakenly\n> insert it into the bundle thinking it was a boundary commit.\n>\n> Fix it by using strbuf_getwholeline() instead, which handles arbitrary\n> line lengths correctly.\n>\n> Note that on the receiving side in parse_bundle_header() we were\n> already using strbuf_getwholeline_fd(), so that part is safe.\n\nThanks for diagnosing this, but I wonder if it even needs --pretty=oneline\nto begin with, except for debugging purposes.\n\nDo we ever use the subject string read from the rev-list output in any\nway?\n\nIn other words, I am wondering if the right patch to minimally fix the\nissue starting from older releases is something along this line instead:\n\ndiff --git a/bundle.c b/bundle.c\nindex b8acf3c..339dbb0 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -248,7 +248,7 @@ int create_bundle(struct bundle_header *header, const char *path,\n \tstatic struct lock_file lock;\n \tint bundle_fd = -1;\n \tint bundle_to_stdout;\n-\tconst char **argv_boundary = xmalloc((argc + 4) * sizeof(const char *));\n+\tconst char **argv_boundary = xmalloc((argc + 3) * sizeof(const char *));\n \tconst char **argv_pack = xmalloc(6 * sizeof(const char *));\n \tint i, ref_count = 0;\n \tchar buffer[1024];\n@@ -271,11 +271,10 @@ int create_bundle(struct bundle_header *header, const char *path,\n \tinit_revisions(&revs, NULL);\n \n \t/* write prerequisites */\n-\tmemcpy(argv_boundary + 3, argv + 1, argc * sizeof(const char *));\n+\tmemcpy(argv_boundary + 2, argv + 1, argc * sizeof(const char *));\n \targv_boundary[0] = \"rev-list\";\n \targv_boundary[1] = \"--boundary\";\n-\targv_boundary[2] = \"--pretty=oneline\";\n-\targv_boundary[argc + 2] = NULL;\n+\targv_boundary[argc + 1] = NULL;\n \tmemset(&rls, 0, sizeof(rls));\n \trls.argv = argv_boundary;\n \trls.out = -1;\n"},{"id":"185195","messageId":"87hayivcmm.fsf@thomas.inf.ethz.ch","threadId":"29696","inReplyTo":"7vlinuaaab.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] bundle: use a strbuf to scan the log for boundary commits","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2012-02-22T20:25:53Z","receivedAt":"2012-02-22T20:25:53Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Thomas Rast <trast@student.ethz.ch> writes:\n>\n>>   # v2 git bundle\n>>   $(git rev-list --pretty=oneline --boundary <ARGS> | grep ^-)\n>>\n>> git-bundle actually spawns exactly this rev-list invocation, and does\n>> the grepping internally.\n>>\n>> There was a subtle bug in the latter step: it used fgets() with a\n>> 1024-byte buffer.  If the user has sufficiently long subjects (e.g.,\n>> by not adhering to the git oneline-subject convention in the first\n>> place), the 'oneline' format can easily overflow the buffer.\n>\n> Thanks for diagnosing this, but I wonder if it even needs --pretty=oneline\n> to begin with, except for debugging purposes.\n>\n> Do we ever use the subject string read from the rev-list output in any\n> way?\n>\n> In other words, I am wondering if the right patch to minimally fix the\n> issue starting from older releases is something along this line instead:\n\nNot sure.  The only use I could think of would be to google for the\nsubjects, in the hope of finding some repository that has the commit you\nare looking for.  Other than that...\n\nIn any case the --pretty=oneline is very deliberate, as we can see from\nthe commit below.  It just doesn't give a reason :-)\n\ncommit 239296770dae75e21c179733785731ec6ffae1f5\nAuthor: Johannes Schindelin <Johannes.Schindelin@gmx.de>\nDate:   Fri Feb 23 03:17:51 2007 +0100\n\n    git-bundle: record commit summary in the prerequisite data\n\ndiff --git a/builtin-bundle.c b/builtin-bundle.c\nindex 191ec55..d74afaa 100644\n--- a/builtin-bundle.c\n+++ b/builtin-bundle.c\n@@ -267,7 +267,7 @@ static int create_bundle(struct bundle_header *header, const char *path,\n \t\tint argc, const char **argv)\n {\n \tint bundle_fd = -1;\n-\tconst char **argv_boundary = xmalloc((argc + 3) * sizeof(const char *));\n+\tconst char **argv_boundary = xmalloc((argc + 4) * sizeof(const char *));\n \tconst char **argv_pack = xmalloc(4 * sizeof(const char *));\n \tint pid, in, out, i, status;\n \tchar buffer[1024];\n@@ -282,10 +282,11 @@ static int create_bundle(struct bundle_header *header, const char *path,\n \twrite_or_die(bundle_fd, bundle_signature, strlen(bundle_signature));\n \n \t/* write prerequisites */\n-\tmemcpy(argv_boundary + 2, argv + 1, argc * sizeof(const char *));\n+\tmemcpy(argv_boundary + 3, argv + 1, argc * sizeof(const char *));\n \targv_boundary[0] = \"rev-list\";\n \targv_boundary[1] = \"--boundary\";\n-\targv_boundary[argc + 1] = NULL;\n+\targv_boundary[2] = \"--pretty=oneline\";\n+\targv_boundary[argc + 2] = NULL;\n \tout = -1;\n \tpid = fork_with_pipe(argv_boundary, NULL, &out);\n \tif (pid < 0)\n\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"185200","messageId":"7vhayia8yc.fsf@alter.siamese.dyndns.org","threadId":"29696","inReplyTo":"87hayivcmm.fsf@thomas.inf.ethz.ch","subject":"Re: [PATCH 2/2] bundle: use a strbuf to scan the log for boundary commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-22T20:50:51Z","receivedAt":"2012-02-22T20:50:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast <trast@inf.ethz.ch> writes:\n\n> In any case the --pretty=oneline is very deliberate, as we can see from\n> the commit below.  It just doesn't give a reason :-)\n\nThanks for digging and I hope everybody on this list who would ever send a\npatch learns the importance of describing _why_ in the log from this\nlesson.\n\nI'll amend your 2/2 with a note that keeping the subject material is not\njustified and we may want to remove (or at least reduce) it in the future.\n"},{"id":"185201","messageId":"20120222205108.GC6781@sigill.intra.peff.net","threadId":"29696","inReplyTo":"a795f6dca5e7c3fc5f9212becda4a46116c502b7.1329939233.git.trast@student.ethz.ch","subject":"Re: [PATCH 1/2] bundle: put strbuf_readline_fd in strbuf.c with adjustments","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-22T20:51:08Z","receivedAt":"2012-02-22T20:51:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 22, 2012 at 08:34:22PM +0100, Thomas Rast wrote:\n\n> The comment even said that it should eventually go there.  While at\n> it, match the calling convention and name of the function to the\n> strbuf_get*line family.  So it now is strbuf_getwholeline_fd.\n> [...]\n>  bundle.c |   21 ++-------------------\n>  strbuf.c |   16 ++++++++++++++++\n>  strbuf.h |    1 +\n\nNit: no update to Documentation/technical/api-strbuf.txt.\n\nYou might want to also mention that this will read() one byte at a time,\nand is therefore only a good idea if you really care about the position\nof the fd. Otherwise, fdopen() + strbuf_getwholeline() is much more\nefficient.\n\n-Peff\n"},{"id":"185202","messageId":"20120222205500.GD6781@sigill.intra.peff.net","threadId":"29696","inReplyTo":"fa1553d59714fd89fdab1bf54af19ac631a30a8c.1329939233.git.trast@student.ethz.ch","subject":"Re: [PATCH 2/2] bundle: use a strbuf to scan the log for boundary commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-22T20:55:00Z","receivedAt":"2012-02-22T20:55:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 22, 2012 at 08:34:23PM +0100, Thomas Rast wrote:\n\n> +# If \"ridiculous\" is at least 1004 chars, this traps a bug in old\n> +# versions where the resulting 1025-char line (with --pretty=oneline)\n> +# was longer than a 1024-char buffer\n> +test_expect_success 'ridiculously long subject in boundary' '\n> +\n> +\t: > file4 &&\n> +\ttest_tick &&\n> +\tgit add file4 &&\n> +\tprintf \"abcdefghijkl %s\\n\" $(seq 1 100) | git commit -F - &&\n\nSeq is not portable. I usually use either\n\n  perl -le \"print for (1..100)\"\n\nor just do:\n\n  z16=zzzzzzzzzzzzzzzz\n  z256=$z16$z16$z16$z16$z16$z16$z16$z16\n  z1024=$z256$z256$z256$z256$z256$z256$z256$z256\n\n-Peff\n"},{"id":"185204","messageId":"20120222210000.GE6781@sigill.intra.peff.net","threadId":"29696","inReplyTo":"87hayivcmm.fsf@thomas.inf.ethz.ch","subject":"Re: [PATCH 2/2] bundle: use a strbuf to scan the log for boundary commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-22T21:00:00Z","receivedAt":"2012-02-22T21:00:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 22, 2012 at 09:25:53PM +0100, Thomas Rast wrote:\n\n> > Thanks for diagnosing this, but I wonder if it even needs --pretty=oneline\n> > to begin with, except for debugging purposes.\n> >\n> > Do we ever use the subject string read from the rev-list output in any\n> > way?\n> >\n> > In other words, I am wondering if the right patch to minimally fix the\n> > issue starting from older releases is something along this line instead:\n> \n> Not sure.  The only use I could think of would be to google for the\n> subjects, in the hope of finding some repository that has the commit you\n> are looking for.  Other than that...\n\nOr because the bundle creator (which may even be you on a different day)\ndid not correctly guess at the negative refs when specifying a cutoff.\nAssuming you no longer have access to the original repo (not\nunreasonable, since you are using a bundle), the messages could help you\nunderstand where the error occurred.\n\nOf course, once you figure out \"aha! I expected the bundle to include\nfoo, but it is actually a cutoff point. I should have said XYZ^ as the\ncutoff\", I'm not sure what you do then. It's not like there is an easy\nway to salvage the data that is in the bundle.\n\nSo I think it is a debugging aid, but one that ultimately doesn't help\nyou that much.\n\n-Peff\n"},{"id":"185211","messageId":"4F4567C8.2030809@kdbg.org","threadId":"29696","inReplyTo":"20120222205500.GD6781@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] bundle: use a strbuf to scan the log for boundary commits","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2012-02-22T22:10:16Z","receivedAt":"2012-02-22T22:10:16Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 22.02.2012 21:55, schrieb Jeff King:\n> On Wed, Feb 22, 2012 at 08:34:23PM +0100, Thomas Rast wrote:\n>> +\tprintf \"abcdefghijkl %s\\n\" $(seq 1 100) | git commit -F - &&\n> \n> Seq is not portable.\n\nThanks for pointing this out.\n\n> I usually use either\n> \n>   perl -le \"print for (1..100)\"\n> \n> or just do:\n> \n>   z16=zzzzzzzzzzzzzzzz\n>   z256=$z16$z16$z16$z16$z16$z16$z16$z16\n>   z1024=$z256$z256$z256$z256$z256$z256$z256$z256\n\nIf a sequence of ASCII zeros is good enough, you can also do:\n\n  printf %02134d 0\n\n-- Hannes\n"},{"id":"185239","messageId":"7vy5ru5j7q.fsf@alter.siamese.dyndns.org","threadId":"29696","inReplyTo":"fa1553d59714fd89fdab1bf54af19ac631a30a8c.1329939233.git.trast@student.ethz.ch","subject":"Re: [PATCH 2/2] bundle: use a strbuf to scan the log for boundary commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-23T03:20:25Z","receivedAt":"2012-02-23T03:20:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast <trast@student.ethz.ch> writes:\n\n> +# If \"ridiculous\" is at least 1004 chars, this traps a bug in old\n> +# versions where the resulting 1025-char line (with --pretty=oneline)\n> +# was longer than a 1024-char buffer\n> +test_expect_success 'ridiculously long subject in boundary' '\n> +\n> +\t: > file4 &&\n> +\ttest_tick &&\n> +\tgit add file4 &&\n> +\tprintf \"abcdefghijkl %s\\n\" $(seq 1 100) | git commit -F - &&\n> +\ttest_commit fifth &&\n> +\tgit bundle create long-subject-bundle.bdl HEAD^..HEAD &&\n> +\tgit fetch long-subject-bundle.bdl &&\n> +\tsed -n \"/^-/{p;q}\" long-subject-bundle.bdl > boundary &&\n> +\tgrep \"^-$_x40 \" boundary\n> +\n> +'\n\nHmm, aside from the use of seq J6t mentioned, I am not sure what this is\ntesting.\n\nI thought the point is to make \"--oneline\" output produce a long line\nwhose 1024th character is '-' and make the fgets() based parser to mistake\nit as the beginning of a line that records a boundary commit, but you do\nnot seem to have any '-' there.\n"},{"id":"185240","messageId":"7vty2i5ie2.fsf@alter.siamese.dyndns.org","threadId":"29696","inReplyTo":"fa1553d59714fd89fdab1bf54af19ac631a30a8c.1329939233.git.trast@student.ethz.ch","subject":"Re: [PATCH 2/2] bundle: use a strbuf to scan the log for boundary commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-23T03:38:13Z","receivedAt":"2012-02-23T03:38:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast <trast@student.ethz.ch> writes:\n\n> diff --git a/bundle.c b/bundle.c\n> index 313de42..0dbd174 100644\n> --- a/bundle.c\n> +++ b/bundle.c\n> @@ -234,7 +234,7 @@ int create_bundle(struct bundle_header *header, const char *path,\n>  \tconst char **argv_boundary = xmalloc((argc + 4) * sizeof(const char *));\n>  \tconst char **argv_pack = xmalloc(6 * sizeof(const char *));\n>  \tint i, ref_count = 0;\n> -\tchar buffer[1024];\n> +\tstruct strbuf buf = STRBUF_INIT;\n>  \tstruct rev_info revs;\n>  \tstruct child_process rls;\n>  \tFILE *rls_fout;\n> @@ -266,16 +266,16 @@ int create_bundle(struct bundle_header *header, const char *path,\n>  \tif (start_command(&rls))\n>  \t\treturn -1;\n>  \trls_fout = xfdopen(rls.out, \"r\");\n> -\twhile (fgets(buffer, sizeof(buffer), rls_fout)) {\n> +\twhile (strbuf_getwholeline(&buf, rls_fout, '\\n') != EOF) {\n\nI'd add strbuf_release(&buf) after this loop.\n\nPerhaps we would want to squash something like this to the test to avoid\n\"seq\", using J6t's idea.  The issue is that we do not write the end of\nline for the long boundary (because it is hidden from us), keep reading\nand rejecting bogus letters, and then the tip is written without\nterminating the boundary line.  So checking boundary line is not a very\nuseful test, but \"fetch\" and \"list-heads\" are.\n\n t/t5704-bundle.sh |   13 +++++++------\n 1 file changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t5704-bundle.sh b/t/t5704-bundle.sh\nindex 7c2f307..5319b84 100755\n--- a/t/t5704-bundle.sh\n+++ b/t/t5704-bundle.sh\n@@ -59,19 +59,20 @@ test_expect_success 'empty bundle file is rejected' '\n \n '\n \n-# If \"ridiculous\" is at least 1004 chars, this traps a bug in old\n-# versions where the resulting 1025-char line (with --pretty=oneline)\n-# was longer than a 1024-char buffer\n+# This triggers a bug in older versions where the resulting line (with\n+# --pretty=oneline) was longer than a 1024-char buffer.\n test_expect_success 'ridiculously long subject in boundary' '\n \n-\t: > file4 &&\n+\t: >file4 &&\n \ttest_tick &&\n \tgit add file4 &&\n-\tprintf \"abcdefghijkl %s\\n\" $(seq 1 100) | git commit -F - &&\n+\tprintf \"%0982d\\n\" 0 | git commit -F - &&\n \ttest_commit fifth &&\n \tgit bundle create long-subject-bundle.bdl HEAD^..HEAD &&\n+\tgit bundle list-heads long-subject-bundle.bdl >heads &&\n+\ttest -s heads &&\n \tgit fetch long-subject-bundle.bdl &&\n-\tsed -n \"/^-/{p;q}\" long-subject-bundle.bdl > boundary &&\n+\tsed -n \"/^-/{p;q}\" long-subject-bundle.bdl >boundary &&\n \tgrep \"^-$_x40 \" boundary\n \n '\n-- \n1.7.9.2.258.g4407a\n"}]}