{"thread":{"id":"10651","subject":"[PATCH] upload-pack: Use finish_{command,async}() instead of waitpid().","startedAt":"2007-11-04T19:46:48Z","lastAt":"2007-11-06T00:22:17Z","messageCount":5,"participants":["Johannes Sixt","Michael J. Cohen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"58300","messageId":"200711042046.48257.johannes.sixt@telecom.at","threadId":"10651","inReplyTo":null,"subject":"[PATCH] upload-pack: Use finish_{command,async}() instead of waitpid().","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2007-11-04T19:46:48Z","receivedAt":"2007-11-04T19:46:48Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"upload-pack spawns two processes, rev-list and pack-objects, and carefully\nmonitors their status so that it can report failure to the remote end.\nThis change removes the complicated procedures on the grounds of the\nfollowing observations:\n\n- If everything is OK, rev-list closes its output pipe end, upon which\n  pack-objects (which reads from the pipe) sees EOF and terminates itself,\n  closing its output (and error) pipes. upload-pack reads from both until\n  it sees EOF in both. It collects the exit codes of the child processes\n  (which indicate success) and terminates successfully.\n\n- If rev-list sees an error, it closes its output and terminates with\n  failure. pack-objects sees EOF in its input and terminates successfully.\n  Again upload-pack reads its inputs until EOF. When it now collects\n  the exit codes of its child processes, it notices the failure of rev-list\n  and signals failure to the remote end.\n\n- If pack-objects sees an error, it terminates with failure. Since this\n  breaks the pipe to rev-list, rev-list is killed with SIGPIPE.\n  upload-pack reads its input until EOF, then collects the exit codes of\n  the child processes, notices their failures, and signals failure to the\n  remote end.\n\n- If upload-pack itself dies unexpectedly, pack-objects is killed with\n  SIGPIPE, and subsequently also rev-list.\n\nThe upshot of this is that precise monitoring of child processes is not\nrequired because both terminate if either one of them dies unexpectedly.\nThis allows us to use finish_command() and finish_async() instead of\nan explicit waitpid(2) call.\n\nThe change is smaller than it looks because most of it only reduces the\nindentation of a large part of the inner loop.\n\nSigned-off-by: Johannes Sixt <johannes.sixt@telecom.at>\n---\n\tThis patch allows us to reduce the differences to the\n\tMinGW port even further. It goes on top of js/forkexec\n\t(which meanwhile is in master).\n\n\tThe test case checks for failures in rev-list (a missing\n\tobject). Any hints how to trigger a failure in pack-objects\n\tthat does not also trigger in rev-list would be welcome.\n\n\tBTW, I don't know what it means to process zombies if the\n\tparent does not waitpid(), but just terminates. Does this\n\twork as expected, ie. no zombies are left behind?\n\n\t-- Hannes\n\n t/t5530-upload-pack-error.sh |   49 +++++++++++\n upload-pack.c                |  192 +++++++++++++++++-------------------------\n 2 files changed, 126 insertions(+), 115 deletions(-)\n create mode 100755 t/t5530-upload-pack-error.sh\n\ndiff --git a/t/t5530-upload-pack-error.sh b/t/t5530-upload-pack-error.sh\nnew file mode 100755\nindex 0000000..9923ba0\n--- /dev/null\n+++ b/t/t5530-upload-pack-error.sh\n@@ -0,0 +1,49 @@\n+#!/bin/sh\n+\n+test_description='errors in upload-pack'\n+\n+. ./test-lib.sh\n+\n+D=`pwd`\n+\n+test_expect_success 'setup and corrupt repository' '\n+\n+\techo file >file &&\n+\tgit add file &&\n+\tgit rev-parse :file &&\n+\tgit commit -a -m original &&\n+\ttest_tick &&\n+\techo changed >file &&\n+\tgit commit -a -m changed &&\n+\trm -f .git/objects/f7/3f3093ff865c514c6c51f867e35f693487d0d3\n+\n+'\n+\n+test_expect_failure 'fsck fails' '\n+\n+\tgit fsck\n+'\n+\n+test_expect_failure 'upload pack fails due to error in rev-list' '\n+\n+\techo \"0032want $(git rev-parse HEAD)\n+00000009done\n+0000\" | git-upload-pack . > /dev/null\n+\n+'\n+\n+test_expect_success 'create empty repository' '\n+\n+\tmkdir foo &&\n+\tcd foo &&\n+\tgit init\n+\n+'\n+\n+test_expect_failure 'fetch fails' '\n+\n+\tgit fetch .. master\n+\n+'\n+\n+test_done\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 6799468..7e04311 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -144,6 +144,7 @@ static void create_pack_file(void)\n \tchar abort_msg[] = \"aborting due to possible repository \"\n \t\t\"corruption on the remote side.\";\n \tint buffered = -1;\n+\tssize_t sz;\n \tconst char *argv[10];\n \tint arg = 0;\n \n@@ -168,22 +169,15 @@ static void create_pack_file(void)\n \tpack_objects.git_cmd = 1;\n \tpack_objects.argv = argv;\n \n-\tif (start_command(&pack_objects)) {\n-\t\t/* daemon sets things up to ignore TERM */\n-\t\tkill(rev_list.pid, SIGKILL);\n+\tif (start_command(&pack_objects))\n \t\tdie(\"git-upload-pack: unable to fork git-pack-objects\");\n-\t}\n \n \t/* We read from pack_objects.err to capture stderr output for\n \t * progress bar, and pack_objects.out to capture the pack data.\n \t */\n \n \twhile (1) {\n-\t\tconst char *who;\n \t\tstruct pollfd pfd[2];\n-\t\tpid_t pid;\n-\t\tint status;\n-\t\tssize_t sz;\n \t\tint pe, pu, pollsize;\n \n \t\treset_timeout();\n@@ -204,123 +198,91 @@ static void create_pack_file(void)\n \t\t\tpollsize++;\n \t\t}\n \n-\t\tif (pollsize) {\n-\t\t\tif (poll(pfd, pollsize, -1) < 0) {\n-\t\t\t\tif (errno != EINTR) {\n-\t\t\t\t\terror(\"poll failed, resuming: %s\",\n-\t\t\t\t\t      strerror(errno));\n-\t\t\t\t\tsleep(1);\n-\t\t\t\t}\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (0 <= pu && (pfd[pu].revents & (POLLIN|POLLHUP))) {\n-\t\t\t\t/* Data ready; we keep the last byte\n-\t\t\t\t * to ourselves in case we detect\n-\t\t\t\t * broken rev-list, so that we can\n-\t\t\t\t * leave the stream corrupted.  This\n-\t\t\t\t * is unfortunate -- unpack-objects\n-\t\t\t\t * would happily accept a valid pack\n-\t\t\t\t * data with trailing garbage, so\n-\t\t\t\t * appending garbage after we pass all\n-\t\t\t\t * the pack data is not good enough to\n-\t\t\t\t * signal breakage to downstream.\n-\t\t\t\t */\n-\t\t\t\tchar *cp = data;\n-\t\t\t\tssize_t outsz = 0;\n-\t\t\t\tif (0 <= buffered) {\n-\t\t\t\t\t*cp++ = buffered;\n-\t\t\t\t\toutsz++;\n-\t\t\t\t}\n-\t\t\t\tsz = xread(pack_objects.out, cp,\n-\t\t\t\t\t  sizeof(data) - outsz);\n-\t\t\t\tif (0 < sz)\n-\t\t\t\t\t\t;\n-\t\t\t\telse if (sz == 0) {\n-\t\t\t\t\tclose(pack_objects.out);\n-\t\t\t\t\tpack_objects.out = -1;\n-\t\t\t\t}\n-\t\t\t\telse\n-\t\t\t\t\tgoto fail;\n-\t\t\t\tsz += outsz;\n-\t\t\t\tif (1 < sz) {\n-\t\t\t\t\tbuffered = data[sz-1] & 0xFF;\n-\t\t\t\t\tsz--;\n-\t\t\t\t}\n-\t\t\t\telse\n-\t\t\t\t\tbuffered = -1;\n-\t\t\t\tsz = send_client_data(1, data, sz);\n-\t\t\t\tif (sz < 0)\n-\t\t\t\t\tgoto fail;\n-\t\t\t}\n-\t\t\tif (0 <= pe && (pfd[pe].revents & (POLLIN|POLLHUP))) {\n-\t\t\t\t/* Status ready; we ship that in the side-band\n-\t\t\t\t * or dump to the standard error.\n-\t\t\t\t */\n-\t\t\t\tsz = xread(pack_objects.err, progress,\n-\t\t\t\t\t  sizeof(progress));\n-\t\t\t\tif (0 < sz)\n-\t\t\t\t\tsend_client_data(2, progress, sz);\n-\t\t\t\telse if (sz == 0) {\n-\t\t\t\t\tclose(pack_objects.err);\n-\t\t\t\t\tpack_objects.err = -1;\n-\t\t\t\t}\n-\t\t\t\telse\n-\t\t\t\t\tgoto fail;\n+\t\tif (!pollsize)\n+\t\t\tbreak;\n+\n+\t\tif (poll(pfd, pollsize, -1) < 0) {\n+\t\t\tif (errno != EINTR) {\n+\t\t\t\terror(\"poll failed, resuming: %s\",\n+\t\t\t\t      strerror(errno));\n+\t\t\t\tsleep(1);\n \t\t\t}\n+\t\t\tcontinue;\n \t\t}\n-\n-\t\t/* See if the children are still there */\n-\t\tif (rev_list.pid || pack_objects.pid) {\n-\t\t\tpid = waitpid(-1, &status, WNOHANG);\n-\t\t\tif (!pid)\n-\t\t\t\tcontinue;\n-\t\t\twho = ((pid == rev_list.pid) ? \"git-rev-list\" :\n-\t\t\t       (pid == pack_objects.pid) ? \"git-pack-objects\" :\n-\t\t\t       NULL);\n-\t\t\tif (!who) {\n-\t\t\t\tif (pid < 0) {\n-\t\t\t\t\terror(\"git-upload-pack: %s\",\n-\t\t\t\t\t      strerror(errno));\n-\t\t\t\t\tgoto fail;\n-\t\t\t\t}\n-\t\t\t\terror(\"git-upload-pack: we weren't \"\n-\t\t\t\t      \"waiting for %d\", pid);\n-\t\t\t\tcontinue;\n+\t\tif (0 <= pu && (pfd[pu].revents & (POLLIN|POLLHUP))) {\n+\t\t\t/* Data ready; we keep the last byte to ourselves\n+\t\t\t * in case we detect broken rev-list, so that we\n+\t\t\t * can leave the stream corrupted.  This is\n+\t\t\t * unfortunate -- unpack-objects would happily\n+\t\t\t * accept a valid packdata with trailing garbage,\n+\t\t\t * so appending garbage after we pass all the\n+\t\t\t * pack data is not good enough to signal\n+\t\t\t * breakage to downstream.\n+\t\t\t */\n+\t\t\tchar *cp = data;\n+\t\t\tssize_t outsz = 0;\n+\t\t\tif (0 <= buffered) {\n+\t\t\t\t*cp++ = buffered;\n+\t\t\t\toutsz++;\n+\t\t\t}\n+\t\t\tsz = xread(pack_objects.out, cp,\n+\t\t\t\t  sizeof(data) - outsz);\n+\t\t\tif (0 < sz)\n+\t\t\t\t\t;\n+\t\t\telse if (sz == 0) {\n+\t\t\t\tclose(pack_objects.out);\n+\t\t\t\tpack_objects.out = -1;\n \t\t\t}\n-\t\t\tif (!WIFEXITED(status) || WEXITSTATUS(status) > 0) {\n-\t\t\t\terror(\"git-upload-pack: %s died with error.\",\n-\t\t\t\t      who);\n+\t\t\telse\n \t\t\t\tgoto fail;\n+\t\t\tsz += outsz;\n+\t\t\tif (1 < sz) {\n+\t\t\t\tbuffered = data[sz-1] & 0xFF;\n+\t\t\t\tsz--;\n \t\t\t}\n-\t\t\tif (pid == rev_list.pid)\n-\t\t\t\trev_list.pid = 0;\n-\t\t\tif (pid == pack_objects.pid)\n-\t\t\t\tpack_objects.pid = 0;\n-\t\t\tif (rev_list.pid || pack_objects.pid)\n-\t\t\t\tcontinue;\n-\t\t}\n-\n-\t\t/* both died happily */\n-\t\tif (pollsize)\n-\t\t\tcontinue;\n-\n-\t\t/* flush the data */\n-\t\tif (0 <= buffered) {\n-\t\t\tdata[0] = buffered;\n-\t\t\tsz = send_client_data(1, data, 1);\n+\t\t\telse\n+\t\t\t\tbuffered = -1;\n+\t\t\tsz = send_client_data(1, data, sz);\n \t\t\tif (sz < 0)\n \t\t\t\tgoto fail;\n-\t\t\tfprintf(stderr, \"flushed.\\n\");\n \t\t}\n-\t\tif (use_sideband)\n-\t\t\tpacket_flush(1);\n-\t\treturn;\n+\t\tif (0 <= pe && (pfd[pe].revents & (POLLIN|POLLHUP))) {\n+\t\t\t/* Status ready; we ship that in the side-band\n+\t\t\t * or dump to the standard error.\n+\t\t\t */\n+\t\t\tsz = xread(pack_objects.err, progress,\n+\t\t\t\t  sizeof(progress));\n+\t\t\tif (0 < sz)\n+\t\t\t\tsend_client_data(2, progress, sz);\n+\t\t\telse if (sz == 0) {\n+\t\t\t\tclose(pack_objects.err);\n+\t\t\t\tpack_objects.err = -1;\n+\t\t\t}\n+\t\t\telse\n+\t\t\t\tgoto fail;\n+\t\t}\n+\t}\n+\n+\tif (finish_command(&pack_objects)) {\n+\t\terror(\"git-upload-pack: git-pack-objects died with error.\");\n+\t\tgoto fail;\n+\t}\n+\tif (finish_async(&rev_list))\n+\t\tgoto fail;\t/* error was already reported */\n+\n+\t/* flush the data */\n+\tif (0 <= buffered) {\n+\t\tdata[0] = buffered;\n+\t\tsz = send_client_data(1, data, 1);\n+\t\tif (sz < 0)\n+\t\t\tgoto fail;\n+\t\tfprintf(stderr, \"flushed.\\n\");\n \t}\n+\tif (use_sideband)\n+\t\tpacket_flush(1);\n+\treturn;\n+\n  fail:\n-\tif (pack_objects.pid)\n-\t\tkill(pack_objects.pid, SIGKILL);\n-\tif (rev_list.pid)\n-\t\tkill(rev_list.pid, SIGKILL);\n \tsend_client_data(3, abort_msg, sizeof(abort_msg));\n \tdie(\"git-upload-pack: %s\", abort_msg);\n }\n-- \n1.5.3.4.315.g2ce38\n"},{"id":"58333","messageId":"2040254C-5BC9-43C5-8146-5E34E9D65B95@mac.com","threadId":"10651","inReplyTo":"200711042046.48257.johannes.sixt@telecom.at","subject":"Re: [PATCH] upload-pack: Use finish_{command,async}() instead of waitpid().","fromName":"Michael J. Cohen","fromEmail":"michaeljosephcohen@mac.com","sentAt":"2007-11-05T01:42:24Z","receivedAt":"2007-11-05T01:42:24Z","isPatch":true,"sender":{"key":"michaeljosephcohen@mac.com","avatar":null},"body":"On Nov 4, 2007, at 2:46 PM, Johannes Sixt wrote:\n\n> The change is smaller than it looks because most of it only reduces  \n> the\n> indentation of a large part of the inner loop.\n\n\nmac-pro:git mjc$ git diff -u -b HEAD | diffstat\nt/t5530-upload-pack-error.sh |   49 ++++++++++++++++++++++++++++\nupload-pack.c                |   74 +++++++++ \n+---------------------------------\n2 files changed, 67 insertions(+), 56 deletions(-)\n\nagreed. :P\n\n-mjc\n"},{"id":"58409","messageId":"7v3avkqwyz.fsf@gitster.siamese.dyndns.org","threadId":"10651","inReplyTo":"200711042046.48257.johannes.sixt@telecom.at","subject":"Re: [PATCH] upload-pack: Use finish_{command,async}() instead of waitpid().","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-05T20:05:40Z","receivedAt":"2007-11-05T20:05:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <johannes.sixt@telecom.at> writes:\n\n> - If pack-objects sees an error, it terminates with failure. Since this\n>   breaks the pipe to rev-list, rev-list is killed with SIGPIPE.\n\nI was a bit uneasy about this part.  We do not explicitly tell\nrev-list not to ignore SIGPIPE, so it could spin fruitlessly\nlisting revs and calling write(2).  But the error from that\nwrite should already be handled correctly anyway, so this should\nbe Ok.\n\n> \tThe test case checks for failures in rev-list (a missing\n> \tobject). Any hints how to trigger a failure in pack-objects\n> \tthat does not also trigger in rev-list would be welcome.\n\nHow about removing a blob from the test repository  to corrupt\nit?  rev-list --objects I think would happily list the blob\nbecause it sees its name in its containing tree without checking\nits existence.\n"},{"id":"58426","messageId":"200711052240.12822.johannes.sixt@telecom.at","threadId":"10651","inReplyTo":"7v3avkqwyz.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] t5530-upload-pack-error: Check more carefully for failures.","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2007-11-05T21:40:12Z","receivedAt":"2007-11-05T21:40:12Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Previously, the test only triggered a failure in upload-pack's rev-list\nsubprocess. Here we also test for a failure of pack-objects, and make sure\nthat the right process failed.\n\nSigned-off-by: Johannes Sixt <johannes.sixt@telecom.at>\n---\n  On Monday 05 November 2007 21:05, Junio C Hamano wrote:\n  > > \tThe test case checks for failures in rev-list (a missing\n  > > \tobject). Any hints how to trigger a failure in pack-objects\n  > > \tthat does not also trigger in rev-list would be welcome.\n  >\n  > How about removing a blob from the test repository  to corrupt\n  > it?  rev-list --objects I think would happily list the blob\n  > because it sees its name in its containing tree without checking\n  > its existence.\n\n  That does it. This goes on top of my previous patch.\n\n  -- Hannes\n\n t/t5530-upload-pack-error.sh |   27 +++++++++++++++++++++++----\n 1 files changed, 23 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t5530-upload-pack-error.sh b/t/t5530-upload-pack-error.sh\nindex 9923ba0..70d4f86 100755\n--- a/t/t5530-upload-pack-error.sh\n+++ b/t/t5530-upload-pack-error.sh\n@@ -15,7 +15,7 @@ test_expect_success 'setup and corrupt repository' '\n \ttest_tick &&\n \techo changed >file &&\n \tgit commit -a -m changed &&\n-\trm -f .git/objects/f7/3f3093ff865c514c6c51f867e35f693487d0d3\n+\trm -f .git/objects/5e/a2ed416fbd4a4cbe227b75fe255dd7fa6bd4d6\n \n '\n \n@@ -24,14 +24,33 @@ test_expect_failure 'fsck fails' '\n \tgit fsck\n '\n \n-test_expect_failure 'upload pack fails due to error in rev-list' '\n+test_expect_success 'upload-pack fails due to error in pack-objects' '\n \n-\techo \"0032want $(git rev-parse HEAD)\n+\t! echo \"0032want $(git rev-parse HEAD)\n 00000009done\n-0000\" | git-upload-pack . > /dev/null\n+0000\" | git-upload-pack . > /dev/null 2> output.err &&\n+\tgrep \"pack-objects died\" output.err\n+'\n+\n+test_expect_success 'corrupt repo differently' '\n+\n+\tgit hash-object -w file &&\n+\trm -f .git/objects/be/c63e37d08c454ad3a60cde90b70f3f7d077852\n \n '\n \n+test_expect_failure 'fsck fails' '\n+\n+\tgit fsck\n+'\n+test_expect_success 'upload-pack fails due to error in rev-list' '\n+\n+\t! echo \"0032want $(git rev-parse HEAD)\n+00000009done\n+0000\" | git-upload-pack . > /dev/null 2> output.err &&\n+\tgrep \"waitpid (async) failed\" output.err\n+'\n+\n test_expect_success 'create empty repository' '\n \n \tmkdir foo &&\n-- \n1.5.3.4.315.g2ce38\n"},{"id":"58463","messageId":"7vve8gkyti.fsf@gitster.siamese.dyndns.org","threadId":"10651","inReplyTo":"200711052240.12822.johannes.sixt@telecom.at","subject":"Re: [PATCH] t5530-upload-pack-error: Check more carefully for failures.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-06T00:22:17Z","receivedAt":"2007-11-06T00:22:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <johannes.sixt@telecom.at> writes:\n\n>   On Monday 05 November 2007 21:05, Junio C Hamano wrote:\n>   > > \tThe test case checks for failures in rev-list (a missing\n>   > > \tobject). Any hints how to trigger a failure in pack-objects\n>   > > \tthat does not also trigger in rev-list would be welcome.\n>   >\n>   > How about removing a blob from the test repository  to corrupt\n>   > it?  rev-list --objects I think would happily list the blob\n>   > because it sees its name in its containing tree without checking\n>   > its existence.\n>\n>   That does it. This goes on top of my previous patch.\n\nThanks.  Will squash with further changes attached for readability.\n\n---\n t/t5530-upload-pack-error.sh |   11 +++++++++--\n 1 files changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5530-upload-pack-error.sh b/t/t5530-upload-pack-error.sh\nindex 70d4f86..cc8949e 100755\n--- a/t/t5530-upload-pack-error.sh\n+++ b/t/t5530-upload-pack-error.sh\n@@ -6,6 +6,13 @@ test_description='errors in upload-pack'\n \n D=`pwd`\n \n+corrupt_repo () {\n+\tobject_sha1=$(git rev-parse \"$1\") &&\n+\tob=$(expr \"$object_sha1\" : \"\\(..\\)\") &&\n+\tject=$(expr \"$object_sha1\" : \"..\\(..*\\)\") &&\n+\trm -f \".git/objects/$ob/$ject\"\n+}\n+\n test_expect_success 'setup and corrupt repository' '\n \n \techo file >file &&\n@@ -15,7 +22,7 @@ test_expect_success 'setup and corrupt repository' '\n \ttest_tick &&\n \techo changed >file &&\n \tgit commit -a -m changed &&\n-\trm -f .git/objects/5e/a2ed416fbd4a4cbe227b75fe255dd7fa6bd4d6\n+\tcorrupt_repo HEAD:file\n \n '\n \n@@ -35,7 +42,7 @@ test_expect_success 'upload-pack fails due to error in pack-objects' '\n test_expect_success 'corrupt repo differently' '\n \n \tgit hash-object -w file &&\n-\trm -f .git/objects/be/c63e37d08c454ad3a60cde90b70f3f7d077852\n+\tcorrupt_repo HEAD^^{tree}\n \n '\n \n"}]}