{"thread":{"id":"10909","subject":"[PATCH] fetch-pack: Prepare for a side-band demultiplexer in a thread.","startedAt":"2007-11-17T22:09:28Z","lastAt":"2007-11-18T12:23:35Z","messageCount":7,"participants":["Johannes Sixt","Junio C Hamano","Alex Riesen","Robin Rosenberg"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"60179","messageId":"200711172309.28364.johannes.sixt@telecom.at","threadId":"10909","inReplyTo":null,"subject":"[PATCH] fetch-pack: Prepare for a side-band demultiplexer in a thread.","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2007-11-17T22:09:28Z","receivedAt":"2007-11-17T22:09:28Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"get_pack() receives a pair of file descriptors that communicate with\nupload-pack at the remote end. In order to support the case where the\nside-band demultiplexer runs in a thread, and, hence, in the same process\nas the main routine, we must not close the readable file descriptor early.\n\nThe handling of the readable fd is changed in the case where upload-pack\nsupports side-band communication: The old code closed the fd after it was\ninherited to the side-band demultiplexer process. Now we do not close it.\nThe caller (do_fetch_pack) will close it later anyway. The demultiplexer\nis the only reader, it does not matter that the fd remains open in the\nmain process as well as in unpack-objects/index-pack, which inherits it.\n\nThe writable fd is not needed in get_pack(), hence, the old code closed\nthe fd. For symmetry with the readable fd, we now do not close it; the\ncaller (do_fetch_pack) will close it later anyway. Therefore, the new\nbehavior is that the channel now remains open during the entire\nconversation, but this has no ill effects because upload-pack does not read\nfrom it once it has begun to send the pack data. For the same reason it\ndoes not matter that the writable fd is now inherited to the demultiplexer\nand unpack-objects/index-pack processes.\n\nSigned-off-by: Johannes Sixt \n---\n\tThis change again originates from the MinGW port. Since we don't\n\thave fork(2) on Windows, we must run the sideband demultiplexer\n\tin a thread.\n\n\t-- Hannes\n\n builtin-fetch-pack.c | 42 ++++++++++++++++--------------------------\n 1 files changed, 16 insertions(+), 26 deletions(-)\n\ndiff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c\nindex bb1742f..807fa93 100644\n--- a/builtin-fetch-pack.c\n+++ b/builtin-fetch-pack.c\n@@ -462,34 +462,12 @@ static int sideband_demux(int fd, void *data)\n {\n \tint *xd = data;\n \n-\tclose(xd[1]);\n \treturn recv_sideband(\"fetch-pack\", xd[0], fd, 2);\n }\n \n-static void setup_sideband(int fd[2], int xd[2], struct async *demux)\n-{\n-\tif (!use_sideband) {\n-\t\tfd[0] = xd[0];\n-\t\tfd[1] = xd[1];\n-\t\treturn;\n-\t}\n-\t/* xd[] is talking with upload-pack; subprocess reads from\n-\t * xd[0], spits out band#2 to stderr, and feeds us band#1\n-\t * through demux->out.\n-\t */\n-\tdemux->proc = sideband_demux;\n-\tdemux->data = xd;\n-\tif (start_async(demux))\n-\t\tdie(\"fetch-pack: unable to fork off sideband demultiplexer\");\n-\tclose(xd[0]);\n-\tfd[0] = demux->out;\n-\tfd[1] = xd[1];\n-}\n-\n static int get_pack(int xd[2], char **pack_lockfile)\n {\n \tstruct async demux;\n-\tint fd[2];\n \tconst char *argv[20];\n \tchar keep_arg[256];\n \tchar hdr_arg[256];\n@@ -497,7 +475,20 @@ static int get_pack(int xd[2], char **pack_lockfile)\n \tint do_keep = args.keep_pack;\n \tstruct child_process cmd;\n \n-\tsetup_sideband(fd, xd, &demux);\n+\tmemset(&demux, 0, sizeof(demux));\n+\tif (use_sideband) {\n+\t\t/* xd[] is talking with upload-pack; subprocess reads from\n+\t\t * xd[0], spits out band#2 to stderr, and feeds us band#1\n+\t\t * through demux->out.\n+\t\t */\n+\t\tdemux.proc = sideband_demux;\n+\t\tdemux.data = xd;\n+\t\tif (start_async(&demux))\n+\t\t\tdie(\"fetch-pack: unable to fork off sideband\"\n+\t\t\t \" demultiplexer\");\n+\t}\n+\telse\n+\t\tdemux.out = xd[0];\n \n \tmemset(&cmd, 0, sizeof(cmd));\n \tcmd.argv = argv;\n@@ -506,7 +497,7 @@ static int get_pack(int xd[2], char **pack_lockfile)\n \tif (!args.keep_pack && unpack_limit) {\n \t\tstruct pack_header header;\n \n-\t\tif (read_pack_header(fd[0], &header))\n+\t\tif (read_pack_header(demux.out, &header))\n \t\t\tdie(\"protocol error: bad pack header\");\n \t\tsnprintf(hdr_arg, sizeof(hdr_arg), \"--pack_header=%u,%u\",\n \t\t\t ntohl(header.hdr_version), ntohl(header.hdr_entries));\n@@ -542,11 +533,10 @@ static int get_pack(int xd[2], char **pack_lockfile)\n \t\t*av++ = hdr_arg;\n \t*av++ = NULL;\n \n-\tcmd.in = fd[0];\n+\tcmd.in = demux.out;\n \tcmd.git_cmd = 1;\n \tif (start_command(&cmd))\n \t\tdie(\"fetch-pack: unable to fork off %s\", argv[0]);\n-\tclose(fd[1]);\n \tif (do_keep && pack_lockfile)\n \t\t*pack_lockfile = index_pack_lockfile(cmd.out);\n \n-- \n1.5.3.5.721.g039b-dirty\n"},{"id":"60190","messageId":"7vtznkz8nw.fsf@gitster.siamese.dyndns.org","threadId":"10909","inReplyTo":"200711172309.28364.johannes.sixt@telecom.at","subject":"Re: [PATCH] fetch-pack: Prepare for a side-band demultiplexer in a thread.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-18T00:42:11Z","receivedAt":"2007-11-18T00:42:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> \tThis change again originates from the MinGW port. Since we don't\n> \thave fork(2) on Windows, we must run the sideband demultiplexer\n> \tin a thread.\n\nIf the rationale was \"running in a thread is more natural on the\nplatform\", I would understand it.\n\nBut \"_must_ run because there is no fork(2)\" solicits a \"Huh?\nHow does Cygwin does it then?\" from me.\n"},{"id":"60192","messageId":"20071118010532.GC7664@steel.home","threadId":"10909","inReplyTo":"7vtznkz8nw.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] fetch-pack: Prepare for a side-band demultiplexer in a thread.","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-11-18T01:05:32Z","receivedAt":"2007-11-18T01:05:32Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Junio C Hamano, Sun, Nov 18, 2007 01:42:11 +0100:\n> Johannes Sixt writes:\n> \n> > \tThis change again originates from the MinGW port. Since we don't\n> > \thave fork(2) on Windows, we must run the sideband demultiplexer\n> > \tin a thread.\n> \n> If the rationale was \"running in a thread is more natural on the\n> platform\", I would understand it.\n> \n> But \"_must_ run because there is no fork(2)\" solicits a \"Huh?\n> How does Cygwin does it then?\" from me.\n> \n\nYou wont believe it: they start the currently running program again\nand copy parents memory over into the child. Sometimes it fails.\nIf you want something scary:\nhttp://cygwin.com/cgi-bin/cvsweb.cgi/src/winsup/cygwin/fork.cc?rev=1.193&content-type=text/x-cvsweb-markup&cvsroot=src\n"},{"id":"60216","messageId":"200711181036.14565.johannes.sixt@telecom.at","threadId":"10909","inReplyTo":"7vtznkz8nw.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] fetch-pack: Prepare for a side-band demultiplexer in a thread.","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2007-11-18T09:36:14Z","receivedAt":"2007-11-18T09:36:14Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Sunday 18 November 2007 01:42, Junio C Hamano wrote:\n> Johannes Sixt writes:\n> > \tThis change again originates from the MinGW port. Since we don't\n> > \thave fork(2) on Windows, we must run the sideband demultiplexer\n> > \tin a thread.\n>\n> If the rationale was \"running in a thread is more natural on the\n> platform\", I would understand it.\n\nPlease take it as such.\n\n> But \"_must_ run because there is no fork(2)\" solicits a \"Huh?\n> How does Cygwin does it then?\" from me.\n\nAlex has answered this. We are not going to copy Cygwin's fork() into git.\n\n-- Hannes\n"},{"id":"60219","messageId":"7v3av3yi1x.fsf@gitster.siamese.dyndns.org","threadId":"10909","inReplyTo":"200711181036.14565.johannes.sixt@telecom.at","subject":"Re: [PATCH] fetch-pack: Prepare for a side-band demultiplexer in a thread.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-18T10:16:58Z","receivedAt":"2007-11-18T10:16:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n>> If the rationale was \"running in a thread is more natural on the\n>> platform\", I would understand it.\n>\n> Please take it as such.\n>\n>> But \"_must_ run because there is no fork(2)\" solicits a \"Huh?\n>> How does Cygwin does it then?\" from me.\n>\n> Alex has answered this. We are not going to copy Cygwin's fork() into git.\n\nWholeheartedly agree, and I feel dirty after looking at that\nthing. Nothing against Cygwin folks, of course.\n\nThe reason I wanted to close pipes as early as possible was\nbecause I did not want to get bitten by \"read() waiting forever\nfor EOF due to an extra unclosed fd on the writer end of the\npipe() without the writer not writing anything to it\" problem,\nwhich is an often-seen mistake.\n\nAs long as you did not introduce that problem the change is fine\nby me.\n"},{"id":"60229","messageId":"200711181203.37450.robin.rosenberg.lists@dewire.com","threadId":"10909","inReplyTo":"200711172309.28364.johannes.sixt@telecom.at","subject":"Re: [PATCH] fetch-pack: Prepare for a side-band demultiplexer in a thread.","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg.lists@dewire.com","sentAt":"2007-11-18T11:03:37Z","receivedAt":"2007-11-18T11:03:37Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"lördag 17 november 2007 skrev Johannes Sixt:\n> Signed-off-by: Johannes Sixt \n> ---\n> \tThis change again originates from the MinGW port. Since we don't\n> \thave fork(2) on Windows, we must run the sideband demultiplexer\n> \tin a thread.\n\nI think this explanation deserves a place in the commit too.\n\n-- robin\n"},{"id":"60232","messageId":"200711181323.35219.johannes.sixt@telecom.at","threadId":"10909","inReplyTo":"7v3av3yi1x.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] fetch-pack: Prepare for a side-band demultiplexer in a thread.","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2007-11-18T12:23:35Z","receivedAt":"2007-11-18T12:23:35Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Sunday 18 November 2007 11:16, Junio C Hamano wrote:\n> Johannes Sixt writes:\n> >> If the rationale was \"running in a thread is more natural on the\n> >> platform\", I would understand it.\n> >\n> > Please take it as such.\n> >\n> >> But \"_must_ run because there is no fork(2)\" solicits a \"Huh?\n> >> How does Cygwin does it then?\" from me.\n> >\n> > Alex has answered this. We are not going to copy Cygwin's fork() into\n> > git.\n>\n> Wholeheartedly agree, and I feel dirty after looking at that\n> thing. Nothing against Cygwin folks, of course.\n>\n> The reason I wanted to close pipes as early as possible was\n> because I did not want to get bitten by \"read() waiting forever\n> for EOF due to an extra unclosed fd on the writer end of the\n> pipe() without the writer not writing anything to it\" problem,\n> which is an often-seen mistake.\n\nThat was my concern, too, and I spent quite a few brain cycles to verify that \nthis is not the case here.\n\n-- Hannes\n"}]}