{"thread":{"id":"7117","subject":"[PATCH] bundle: fix wrong check of read_header()'s return value & add tests","startedAt":"2007-03-06T21:57:07Z","lastAt":"2007-03-08T20:43:23Z","messageCount":7,"participants":["Johannes Schindelin","Junio C Hamano","Alex Riesen","Mark Levedahl"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"36476","messageId":"Pine.LNX.4.63.0703062256200.22628@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"7117","inReplyTo":null,"subject":"[PATCH] bundle: fix wrong check of read_header()'s return value & add tests","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-03-06T21:57:07Z","receivedAt":"2007-03-06T21:57:07Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nIf read_header() fails, it returns <0, not 0. Further, an open(/dev/null)\nwas not checked for errors.\n\nAlso, this adds two tests to make sure that the bundle file looks\ncorrect, by checking if it has the header has the expected form, and that\nthe pack contains the right amount of objects.\n\nSigned-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>\n---\n builtin-bundle.c |    4 +++-\n t/t5510-fetch.sh |   14 ++++++++++++++\n 2 files changed, 17 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-bundle.c b/builtin-bundle.c\nindex d0c3617..3b3bc25 100644\n--- a/builtin-bundle.c\n+++ b/builtin-bundle.c\n@@ -404,6 +404,8 @@ static int unbundle(struct bundle_header *header, int bundle_fd,\n \tif (verify_bundle(header, 0))\n \t\treturn -1;\n \tdev_null = open(\"/dev/null\", O_WRONLY);\n+\tif (dev_null < 0)\n+\t\treturn error(\"Could not open /dev/null\");\n \tpid = fork_with_pipe(argv_index_pack, &bundle_fd, &dev_null);\n \tif (pid < 0)\n \t\treturn error(\"Could not spawn index-pack\");\n@@ -440,7 +442,7 @@ int cmd_bundle(int argc, const char **argv, const char *prefix)\n \n \tmemset(&header, 0, sizeof(header));\n \tif (strcmp(cmd, \"create\") &&\n-\t\t\t!(bundle_fd = read_header(bundle_file, &header)))\n+\t\t\t(bundle_fd = read_header(bundle_file, &header)) < 0)\n \t\treturn 1;\n \n \tif (!strcmp(cmd, \"verify\")) {\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex fa76662..ce96b4b 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -90,6 +90,13 @@ test_expect_success 'create bundle 1' '\n \tgit bundle create bundle1 master^..master\n '\n \n+test_expect_success 'header of bundle looks right' '\n+\thead -n 1 \"$D\"/bundle1 | grep \"^#\" &&\n+\thead -n 2 \"$D\"/bundle1 | grep \"^-[0-9a-f]\\{40\\} \" &&\n+\thead -n 3 \"$D\"/bundle1 | grep \"^[0-9a-f]\\{40\\} \" &&\n+\thead -n 4 \"$D\"/bundle1 | grep \"^$\"\n+'\n+\n test_expect_success 'create bundle 2' '\n \tcd \"$D\" &&\n \tgit bundle create bundle2 master~2..master\n@@ -101,6 +108,13 @@ test_expect_failure 'unbundle 1' '\n \tgit fetch \"$D/bundle1\" master:master\n '\n \n+test_expect_success 'bundle 1 has only 3 files ' '\n+\tcd \"$D\" &&\n+\tsed \"1,4d\" < bundle1 > bundle.pack &&\n+\tgit index-pack bundle.pack &&\n+\ttest 4 = $(git verify-pack -v bundle.pack | wc -l)\n+'\n+\n test_expect_success 'unbundle 2' '\n \tcd \"$D/bundle\" &&\n \tgit fetch ../bundle2 master:master &&\n-- \n1.5.0.3.2559.g30d3b\n"},{"id":"36529","messageId":"Pine.LNX.4.63.0703070613530.22628@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"7117","inReplyTo":"Pine.LNX.4.63.0703062256200.22628@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH] bundle: fix wrong check of read_header()'s return value & add tests","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-03-07T05:15:12Z","receivedAt":"2007-03-07T05:15:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 6 Mar 2007, Johannes Schindelin wrote:\n\n> +test_expect_success 'bundle 1 has only 3 files ' '\n> +\tcd \"$D\" &&\n> +\tsed \"1,4d\" < bundle1 > bundle.pack &&\n\nI fear this would suffer the same fate as t8001, namely that some sed \nwould add a newline, which is plain wrong here. This is a workaround:\n\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex ce96b4b..f895072 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -110,7 +110,7 @@ test_expect_failure 'unbundle 1' '\n \n test_expect_success 'bundle 1 has only 3 files ' '\n \tcd \"$D\" &&\n-\tsed \"1,4d\" < bundle1 > bundle.pack &&\n+\tdd bs=136 skip=1 if=bundle1 of=bundle.pack &&\n \tgit index-pack bundle.pack &&\n \ttest 4 = $(git verify-pack -v bundle.pack | wc -l)\n '\n\nCiao,\nDscho\n"},{"id":"36532","messageId":"7vslchio4f.fsf@assigned-by-dhcp.cox.net","threadId":"7117","inReplyTo":"Pine.LNX.4.63.0703070613530.22628@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH] bundle: fix wrong check of read_header()'s return value & add tests","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-03-07T06:06:24Z","receivedAt":"2007-03-07T06:06:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> I fear this would suffer the same fate as t8001, namely that some sed \n> would add a newline, which is plain wrong here. This is a workaround:\n>\n> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n> index ce96b4b..f895072 100755\n> --- a/t/t5510-fetch.sh\n> +++ b/t/t5510-fetch.sh\n> @@ -110,7 +110,7 @@ test_expect_failure 'unbundle 1' '\n>  \n>  test_expect_success 'bundle 1 has only 3 files ' '\n>  \tcd \"$D\" &&\n> -\tsed \"1,4d\" < bundle1 > bundle.pack &&\n> +\tdd bs=136 skip=1 if=bundle1 of=bundle.pack &&\n\nWe might want to reword or enhance the headers later, and 136 is\na horrible workaround.\n\nWould this work?\n\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex ce96b4b..ad589dd 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -110,9 +110,16 @@ test_expect_failure 'unbundle 1' '\n \n test_expect_success 'bundle 1 has only 3 files ' '\n \tcd \"$D\" &&\n-\tsed \"1,4d\" < bundle1 > bundle.pack &&\n+\t(\n+\t\twhile read x && test -n \"$x\"\n+\t\tdo\n+\t\t\t:;\n+\t\tdone\n+\t\tcat\n+\t) <bundle1 >bundle.pack &&\n \tgit index-pack bundle.pack &&\n-\ttest 4 = $(git verify-pack -v bundle.pack | wc -l)\n+\tverify=$(git verify-pack -v bundle.pack) &&\n+\ttest 4 = $(echo \"$verify\" | wc -l)\n '\n \n test_expect_success 'unbundle 2' '\n"},{"id":"36620","messageId":"Pine.LNX.4.63.0703081405310.22628@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"7117","inReplyTo":"7vslchio4f.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] bundle: fix wrong check of read_header()'s return value & add tests","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-03-08T13:06:05Z","receivedAt":"2007-03-08T13:06:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 6 Mar 2007, Junio C Hamano wrote:\n\n> Would this work?\n> \n> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n> index ce96b4b..ad589dd 100755\n> --- a/t/t5510-fetch.sh\n> +++ b/t/t5510-fetch.sh\n> @@ -110,9 +110,16 @@ test_expect_failure 'unbundle 1' '\n>  \n>  test_expect_success 'bundle 1 has only 3 files ' '\n>  \tcd \"$D\" &&\n> -\tsed \"1,4d\" < bundle1 > bundle.pack &&\n> +\t(\n> +\t\twhile read x && test -n \"$x\"\n> +\t\tdo\n> +\t\t\t:;\n> +\t\tdone\n> +\t\tcat\n> +\t) <bundle1 >bundle.pack &&\n\nI tried to avoid that, because it was mentioned that this does not work on \nCygwin for some reason I forgot.\n\nCiao,\nDscho\n"},{"id":"36622","messageId":"81b0412b0703080558ye4cff44l88eb88d4911222a0@mail.gmail.com","threadId":"7117","inReplyTo":"Pine.LNX.4.63.0703081405310.22628@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH] bundle: fix wrong check of read_header()'s return value & add tests","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-03-08T13:58:01Z","receivedAt":"2007-03-08T13:58:01Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 3/8/07, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:\n> On Tue, 6 Mar 2007, Junio C Hamano wrote:\n> > +     (\n> > +             while read x && test -n \"$x\"\n> > +             do\n> > +                     :;\n> > +             done\n> > +             cat\n> > +     ) <bundle1 >bundle.pack &&\n>\n> I tried to avoid that, because it was mentioned that this does not work on\n> Cygwin for some reason I forgot.\n>\n\nCan't think of a reason why it would not. Just tried: works.\nIt works even with \\r\\n line endings (which I don't understand).\n"},{"id":"36638","messageId":"Pine.LNX.4.63.0703081811360.22628@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"7117","inReplyTo":"81b0412b0703080558ye4cff44l88eb88d4911222a0@mail.gmail.com","subject":"Re: [PATCH] bundle: fix wrong check of read_header()'s return value & add tests","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-03-08T17:12:14Z","receivedAt":"2007-03-08T17:12:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 8 Mar 2007, Alex Riesen wrote:\n\n> On 3/8/07, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:\n> > On Tue, 6 Mar 2007, Junio C Hamano wrote:\n> > > +     (\n> > > +             while read x && test -n \"$x\"\n> > > +             do\n> > > +                     :;\n> > > +             done\n> > > +             cat\n> > > +     ) <bundle1 >bundle.pack &&\n> > \n> > I tried to avoid that, because it was mentioned that this does not work on\n> > Cygwin for some reason I forgot.\n> > \n> \n> Can't think of a reason why it would not. Just tried: works.\n> It works even with \\r\\n line endings (which I don't understand).\n\nIIRC there was a problem when a file was detected to be text, but \ncontinued to be binary. Mark?\n\nCiao,\nDscho\n"},{"id":"36651","messageId":"45F0756B.5080808@verizon.net","threadId":"7117","inReplyTo":"Pine.LNX.4.63.0703081811360.22628@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH] bundle: fix wrong check of read_header()'s return value & add tests","fromName":"Mark Levedahl","fromEmail":"mdl123@verizon.net","sentAt":"2007-03-08T20:43:23Z","receivedAt":"2007-03-08T20:43:23Z","isPatch":true,"sender":{"key":"mdl123@verizon.net","avatar":"https://avatars.githubusercontent.com/u/5302462?v=4"},"body":"Johannes Schindelin wrote:\n> IIRC there was a problem when a file was detected to be text, but \n> continued to be binary. Mark?\n>\n> Ciao,\n> Dscho\n>\n>   \nThe problem occurs with constructs like\n\necho \"some text stuff\"  > file\necho \"some binary stuff\" >> file\n\nThe second write, being an append, ends up executed in a forked process \nwhere file was opened by the parent, and unfortunately auto-detected as \na text file, such that the write from the child process ends up mangling \nany crlf in the stream. This occurs regardless of the defined mount type \nand other cygwin flags. It is definitely a bug, but is attributed to \nlooseness in POSIX with noone claiming ownership to fix.\n\nHowever, the above bug is not triggered in the construct mentioned by Junio.\n\nMark\n"}]}