{"thread":{"id":"19853","subject":"msysgit git-submodule: \"Unable to fetch in submodule path ...\"","startedAt":"2009-06-18T13:10:56Z","lastAt":"2009-07-11T10:58:24Z","messageCount":28,"participants":["Peter Krefting","Johan Herland","Johannes Sixt","Junio C Hamano","Alex Riesen","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"116566","messageId":"alpine.DEB.2.00.0906181310400.23400@ds9.cixit.se","threadId":"19853","inReplyTo":null,"subject":"msysgit git-submodule: \"Unable to fetch in submodule path ...\"","fromName":"Peter Krefting","fromEmail":"peter@softwolves.pp.se","sentAt":"2009-06-18T13:10:56Z","receivedAt":"2009-06-18T13:10:56Z","isPatch":false,"sender":{"key":"peter@softwolves.pp.se","avatar":"https://avatars.githubusercontent.com/u/990764?v=4"},"body":"Hi!\n\nThis is with msysGit 1.6.3.2.1299.gee46c\n\nI am having some problem when checking out a large project with many \nsubmodules. After cloning the superproject, doing a git submodule --init \nwill fail with a \"Unable to fetch in submodule path '...'\" for one specific \nmodule (does this on all machines that we have tried it on). I cannot see \nany error messages from git-fetch itself:\n\n   [...]\n   Initialized empty Git repository in c:/Users/peter/src/foo/bar/modules/foo/.git/\n   remote: Counting objects: 14752, done.\n   remote: Compressing objects: 100% (5036/5036), done.\n   remote: Total 14752 (delta 9278), reused 14752 (delta 9278)Receiving objects:  9\n\n   Receiving objects: 100% (14752/14752), 5.07 MiB | 3501 KiB/s, done.\n   Resolving deltas: 100% (9278/9278), done.\n   Unable to fetch in submodule path 'modules/foo'\n\nIf I enter the modules/foo manually and enter\n\n   git reset --hard\n\nor similar, everything works fine, and I do have the complete history.\n\nLooking at the code for git-submodule, it seems to suggest that git \nsubmodule is calling git-fetch without parameters, and checking the return \nvalue from it. It does, as indicated above, not seem to return any errors.\n\nI tried adding a \"-v -v\" to the git-fetch command line in git-submodule, and I \nsee that it does terminate early. With the other modules, I get a list of \nall the cloned branches and tags. But for this module, it stops as above. I \nget some additional debug output, but can't quite say it helps me much:\n\n   Initialized empty Git repository in c:/Users/peter/src/foo/baz/modules/foo/.git/\n   remote: Counting objects: 14752, done.\n   remote: Compressing objects: 100% (5036/5036), done.\n   emote: Total 14752 (delta 9278), reused 14752 (delta 9278)\n   Receiving objects: 100% (14752/14752), 5.07 MiB | 3629 KiB/s, done.\n   Resolving deltas: 100% (9278/9278), done.\n   Server supports multi_ack\n   Server supports side-band-64k\n   Server supports ofs-delta\n   Marking 76b96bfecc0d47013dd1fca1a555f12074eca814 as complete\n   Unable to fetch in submodule path 'modules/foo'\n\nThe string \"Marking %s as complete\" seems to stem from \nmark_recent_complete_commits() in builtin-fetch-pack.c. The other messages \nseems to stem from do_fetch_pack() in the same file, so it gets there and \nnot further. I cannot seem to find any exit point.\n\nDoes anyone know how to continue debugging, or know what might be going wrong?\n\n-- \n\\\\// Peter - http://www.softwolves.pp.se/\n"},{"id":"116760","messageId":"alpine.DEB.2.00.0906221342310.26061@ds9.cixit.se","threadId":"19853","inReplyTo":"alpine.DEB.2.00.0906181310400.23400@ds9.cixit.se","subject":"Re: msysgit git-submodule: \"Unable to fetch in submodule path ...\"","fromName":"Peter Krefting","fromEmail":"peter@softwolves.pp.se","sentAt":"2009-06-22T12:46:57Z","receivedAt":"2009-06-22T12:46:57Z","isPatch":false,"sender":{"key":"peter@softwolves.pp.se","avatar":"https://avatars.githubusercontent.com/u/990764?v=4"},"body":"Peter Krefting:\n\n>  Receiving objects: 100% (14752/14752), 5.07 MiB | 3629 KiB/s, done.\n>  Resolving deltas: 100% (9278/9278), done.\n>  Server supports multi_ack\n>  Server supports side-band-64k\n>  Server supports ofs-delta\n>  Marking 76b96bfecc0d47013dd1fca1a555f12074eca814 as complete\n>  Unable to fetch in submodule path 'modules/foo'\n\nI added some extra debugging output to the code in builtin-fetch.c and \nbuiltin-fetch-pack.c, and ended up with this:\n\n   Receiving objects: 100% (14752/14752), 5.07 MiB | 3383 KiB/s, done.\n   Resolving deltas: 100% (9278/9278), done.\n   cmd_fetch(): calling do_fetch()\n   do_fetch(): calling get_ref_map()\n   do_fetch(): get_ref_map() done\n   do_fetch(): check_not_current_branch() done\n   do_fetch(): read_ref() loop done\n   fetch_refs(): quickfetch() returned -10001\n   Server supports multi_ack\n   Server supports side-band-64k\n   Server supports ofs-delta\n   Entering everything_local()\n   everything_local() after 1st ref loop\n   Marking 76b96bfecc0d47013dd1fca1a555f12074eca814 as complete\n   everything_local() after mark_recent_complete_commits()\n   everything_local() after 2nd ref loop\n   everything_local() after filter_refs()\n   everything_local() done with retval = 1\n   everything_local() returned true\n   do_fetch_pack() done\n   fetch_pack(): calling reprepare_packed_git()\n   fetch_pack(): done\n   fetch_refs(): transport_fetch_refs() returned -1\n   do_fetch(): fetch_refs(transport, ref_map) returned non-zero\n   cmd_fetch(): do_fetch() return with exit_code = 1\n   Unable to fetch in submodule path 'modules/foo'\n\nThis seems to indicate that fetch_refs() seems to think that the fetch \n(which is done over ssh) fails, whereas the regular trace output (\"Receiving \nobjects\", etc.) indicates that it succeeds.\n\nIs there anything obvious that I should have a look at here?\n\nMy next step otherwise is adding trace output to the transport_fetch_refs() \nand whatever it is it calls and that calls the code in builtin-fetch-pack.c. \nAnywhere in particular I should have a look at?\n\n-- \n\\\\// Peter - http://www.softwolves.pp.se/\n"},{"id":"117615","messageId":"200907081558.51767.johan@herland.net","threadId":"19853","inReplyTo":"alpine.DEB.2.00.0906221342310.26061@ds9.cixit.se","subject":"[PATCH] quickfetch(): Prevent overflow of the rev-list command line","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2009-07-08T13:58:51Z","receivedAt":"2009-07-08T13:58:51Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"quickfetch() calls rev-list to check whether the objects we are about to\nfetch are already present in the repo (if so, we can skip the object fetch).\nHowever, when there are many (~1000) refs to be fetched, the rev-list\ncommand line grows larger than the maximum command line size on some systems\n(32K in Windows). This causes rev-list to fail, making quickfetch() return\nnon-zero, which unnecessarily triggers the transport machinery. This somehow\ncauses fetch to fail with an exit code.\n\nBy using the --stdin option to rev-list (and feeding the object list to its\nstandard input), we prevent the overflow of the rev-list command line,\nwhich causes quickfetch(), and subsequently the overall fetch, to succeed.\n\nHowever, using rev-list --stdin is not entirely straightforward: rev-list\nterminates immediately when encountering an unknown object, which can\ntrigger SIGPIPE if we are still writing object's to its standard input.\nWe therefore ignore SIGPIPE so that the fetch process is not terminated.\n\nSigned-off-by: Johan Herland <johan@herland.net>\nTested-by: Peter Krefting <peter@softwolves.pp.se>\n---\n\nHi,\n\nIt seems the git fetch failure described by Peter earlier in this thread\nis caused by a long ref list overflowing the command line buffer on\nWindows (32K I am told), when calling rev-list from quickfetch(). AFAICS\nthis overflow will trigger on any fetch from msysgit with more than ~800\n(32K / 40) refs.\n\nAccording to Peter, this patch fixes the submodule update failure.\n\nCC-ing Shawn since he is the original author of quickfetch().\n\n\nHave fun! :)\n\n...Johan\n\n builtin-fetch.c |   63 ++++++++++++++++++++++++++++++------------------------\n 1 files changed, 35 insertions(+), 28 deletions(-)\n\ndiff --git a/builtin-fetch.c b/builtin-fetch.c\nindex cd5eb9a..52febc6 100644\n--- a/builtin-fetch.c\n+++ b/builtin-fetch.c\n@@ -400,14 +400,14 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \n /*\n  * We would want to bypass the object transfer altogether if\n- * everything we are going to fetch already exists and connected\n+ * everything we are going to fetch already exists and is connected\n  * locally.\n  *\n- * The refs we are going to fetch are in to_fetch (nr_heads in\n- * total).  If running\n+ * The refs we are going to fetch are in ref_map.  If running\n  *\n- *  $ git rev-list --objects to_fetch[0] to_fetch[1] ... --not --all\n+ *  $ git rev-list --objects --stdin --not --all\n  *\n+ * (feeding all the refs in ref_map on its standard input)\n  * does not error out, that means everything reachable from the\n  * refs we are going to fetch exists and is connected to some of\n  * our existing refs.\n@@ -416,9 +416,10 @@ static int quickfetch(struct ref *ref_map)\n {\n \tstruct child_process revlist;\n \tstruct ref *ref;\n-\tchar **argv;\n-\tint i, err;\n-\n+\tint err;\n+\tconst char *argv[] = {\n+\t\t\"rev-list\", \"--quiet\", \"--objects\", \"--stdin\", \"--not\", \"--all\", NULL\n+\t};\n \t/*\n \t * If we are deepening a shallow clone we already have these\n \t * objects reachable.  Running rev-list here will return with\n@@ -429,34 +430,40 @@ static int quickfetch(struct ref *ref_map)\n \tif (depth)\n \t\treturn -1;\n \n-\tfor (i = 0, ref = ref_map; ref; ref = ref->next)\n-\t\ti++;\n-\tif (!i)\n+\tif (!ref_map)\n \t\treturn 0;\n \n-\targv = xmalloc(sizeof(*argv) * (i + 6));\n-\ti = 0;\n-\targv[i++] = xstrdup(\"rev-list\");\n-\targv[i++] = xstrdup(\"--quiet\");\n-\targv[i++] = xstrdup(\"--objects\");\n-\tfor (ref = ref_map; ref; ref = ref->next)\n-\t\targv[i++] = xstrdup(sha1_to_hex(ref->old_sha1));\n-\targv[i++] = xstrdup(\"--not\");\n-\targv[i++] = xstrdup(\"--all\");\n-\targv[i++] = NULL;\n-\n \tmemset(&revlist, 0, sizeof(revlist));\n-\trevlist.argv = (const char**)argv;\n+\trevlist.argv = argv;\n \trevlist.git_cmd = 1;\n-\trevlist.no_stdin = 1;\n \trevlist.no_stdout = 1;\n \trevlist.no_stderr = 1;\n-\terr = run_command(&revlist);\n+\trevlist.in = -1;\n+\n+\t/* If rev-list --stdin encounters an unknown commit, it terminates,\n+\t * which will cause SIGPIPE in the write loop below. */\n+\tsignal(SIGPIPE, SIG_IGN);\n+\n+\terr = start_command(&revlist);\n+\tif (err) {\n+\t\terror(\"could not run rev-list\");\n+\t\treturn err;\n+\t}\n \n-\tfor (i = 0; argv[i]; i++)\n-\t\tfree(argv[i]);\n-\tfree(argv);\n-\treturn err;\n+\tfor (ref = ref_map; ref; ref = ref->next) {\n+\t\tif (write_in_full(revlist.in, sha1_to_hex(ref->old_sha1), 40) < 0 ||\n+\t\t    write_in_full(revlist.in, \"\\n\", 1) < 0) {\n+\t\t\terror(\"failed write to rev-list\");\n+\t\t\terr = errno;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\n+\tif (close(revlist.in)) {\n+\t\terror(\"failed to close rev-list's stdin\");\n+\t\terr = errno;\n+\t}\n+\treturn finish_command(&revlist) || err;\n }\n \n static int fetch_refs(struct transport *transport, struct ref *ref_map)\n-- \n1.6.3.2.316.gda4e\n"},{"id":"117616","messageId":"4A54B755.4090100@viscovery.net","threadId":"19853","inReplyTo":"200907081558.51767.johan@herland.net","subject":"Re: [PATCH] quickfetch(): Prevent overflow of the rev-list command line","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-07-08T15:12:21Z","receivedAt":"2009-07-08T15:12:21Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Johan Herland schrieb:\n> +\t/* If rev-list --stdin encounters an unknown commit, it terminates,\n> +\t * which will cause SIGPIPE in the write loop below. */\n\nUnder the conditions you describe here...\n\n> +\tsignal(SIGPIPE, SIG_IGN);\n\n... and SIGPIPE being ignored...\n\n> +\n> +\terr = start_command(&revlist);\n> +\tif (err) {\n> +\t\terror(\"could not run rev-list\");\n> +\t\treturn err;\n> +\t}\n>  \n> -\tfor (i = 0; argv[i]; i++)\n> -\t\tfree(argv[i]);\n> -\tfree(argv);\n> -\treturn err;\n> +\tfor (ref = ref_map; ref; ref = ref->next) {\n> +\t\tif (write_in_full(revlist.in, sha1_to_hex(ref->old_sha1), 40) < 0 ||\n> +\t\t    write_in_full(revlist.in, \"\\n\", 1) < 0) {\n> +\t\t\terror(\"failed write to rev-list\");\n> +\t\t\terr = errno;\n\n... don't you get this error message with errno set to EPIPE? Previously,\nthere was no error message.\n\n-- Hannes\n"},{"id":"117618","messageId":"200907081801.36901.johan@herland.net","threadId":"19853","inReplyTo":"4A54B755.4090100@viscovery.net","subject":"Re: [PATCH] quickfetch(): Prevent overflow of the rev-list command line","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2009-07-08T16:01:36Z","receivedAt":"2009-07-08T16:01:36Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Wednesday 08 July 2009, Johannes Sixt wrote:\n> Johan Herland schrieb:\n> > +\t/* If rev-list --stdin encounters an unknown commit, it\n> > terminates, +\t * which will cause SIGPIPE in the write loop below.\n> > */\n>\n> Under the conditions you describe here...\n>\n> > +\tsignal(SIGPIPE, SIG_IGN);\n>\n> ... and SIGPIPE being ignored...\n>\n> > +\n> > +\terr = start_command(&revlist);\n> > +\tif (err) {\n> > +\t\terror(\"could not run rev-list\");\n> > +\t\treturn err;\n> > +\t}\n> >\n> > -\tfor (i = 0; argv[i]; i++)\n> > -\t\tfree(argv[i]);\n> > -\tfree(argv);\n> > -\treturn err;\n> > +\tfor (ref = ref_map; ref; ref = ref->next) {\n> > +\t\tif (write_in_full(revlist.in, sha1_to_hex(ref->old_sha1), 40) <\n> > 0 || +\t\t    write_in_full(revlist.in, \"\\n\", 1) < 0) {\n> > +\t\t\terror(\"failed write to rev-list\");\n> > +\t\t\terr = errno;\n>\n> ... don't you get this error message with errno set to EPIPE?\n> Previously, there was no error message.\n\nIndeed, you are correct. I guess the following should be added to the\npatch:\n\n \tif (write_in_full(revlist.in, sha1_to_hex(ref->old_sha1), 40) < 0 ||\n \t    write_in_full(revlist.in, \"\\n\", 1) < 0) {\n-\t\terror(\"failed write to rev-list\");\n-\t\terr = errno;\n+\t\tif (errno != EPIPE) {\n+\t\t\terror(\"failed write to rev-list\");\n+\t\t\terr = errno;\n+\t\t}\n \t\tbreak;\n \t}\n\nMaybe I need to do something to the close() call as well? What happens on close() after EPIPE?\n\n\nThanks,\n\n...Johan\n\n-- \nJohan Herland, <johan@herland.net>\nwww.herland.net\n"},{"id":"117622","messageId":"7vws6j6qed.fsf@alter.siamese.dyndns.org","threadId":"19853","inReplyTo":"200907081801.36901.johan@herland.net","subject":"Re: [PATCH] quickfetch(): Prevent overflow of the rev-list command line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-07-08T17:22:34Z","receivedAt":"2009-07-08T17:22:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland <johan@herland.net> writes:\n\n> Maybe I need to do something to the close() call as well? What happens\n> on close() after EPIPE?\n\nYou should be OK (you could try this).\n\n-- >8 --\n#include <stdio.h>\n#include <errno.h>\n#include <stdlib.h>\n#include <signal.h>\n#include <string.h>\n\nint main(int ac, char **av)\n{\n\tint pipefd[2];\n\tint child;\n\n\tif (pipe(pipefd) < 0) {\n\t\tfprintf(stderr, \"pipe failed: %s\\n\", strerror(errno));\n\t\texit(1);\n\t}\n\tchild = fork();\n\tif (child < 0) {\n\t\tfprintf(stderr, \"fork failed: %s\\n\", strerror(errno));\n\t\texit(1);\n\t} else if (child == 0) {\n\t\tchar buf[1024];\n\t\tssize_t sz;\n\n\t\t/* the child reads from the parent but does not talk back */\n\t\tclose(pipefd[1]);\n\n\t\t/* emulate reading a bit, then dying without cleaning up */\n\t\tsz = read(pipefd[0], buf, sizeof(buf));\n\t\tfprintf(stderr, \"read %lu bytes, and will die\\n\",\n\t\t\t(unsigned long) sz);\n\t\texit(1);\n\t} else {\n\t\tconst char data[] = \"abcdefg\";\n\t\tsize_t len = sizeof(data);\n\t\tsize_t written = 0;\n\n\t\t/* the parent writes to the child but does not listen */\n\t\tclose(pipefd[0]);\n\n\t\t/* we will rite to the pipe even after the child is gone */\n\t\tsignal(SIGPIPE, SIG_IGN);\n\n\t\t/* write, write, write, ... */\n\t\twhile (1) {\n\t\t\tssize_t sz = write(pipefd[1], data, len);\n\t\t\tif (sz < 0) {\n\t\t\t\t/* error */\n\t\t\t\tfprintf(stderr,\n\t\t\t\t\t\"write failed (%s) after writing\"\n\t\t\t\t\t\" %lu bytes\\n\",\n\t\t\t\t\tstrerror(errno),\n\t\t\t\t\t(unsigned long) written);\n\t\t\t\tbreak;\n\t\t\t}\n\t\t\twritten += sz;\n\t\t}\n\t\terrno = 0;\n\t\tif (close(pipefd[1]))\n\t\t\tfprintf(stderr, \"close failed: %s\\n\", strerror(errno));\n\t\telse\n\t\t\tfprintf(stderr, \"close ok\\n\");\n\t}\n\texit(0);\n}\n"},{"id":"117655","messageId":"81b0412b0907090101x7c8aa182o36687d67be3c5fb0@mail.gmail.com","threadId":"19853","inReplyTo":"200907081801.36901.johan@herland.net","subject":"Re: [PATCH] quickfetch(): Prevent overflow of the rev-list command line","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-07-09T08:01:45Z","receivedAt":"2009-07-09T08:01:45Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On Wed, Jul 8, 2009 at 18:01, Johan Herland<johan@herland.net> wrote:\n> On Wednesday 08 July 2009, Johannes Sixt wrote:\n>> ... don't you get this error message with errno set to EPIPE?\n>> Previously, there was no error message.\n>\n> Indeed, you are correct. I guess the following should be added to the\n> patch:\n>\n>        if (write_in_full(revlist.in, sha1_to_hex(ref->old_sha1), 40) < 0 ||\n>            write_in_full(revlist.in, \"\\n\", 1) < 0) {\n> -               error(\"failed write to rev-list\");\n> -               err = errno;\n> +               if (errno != EPIPE) {\n> +                       error(\"failed write to rev-list\");\n> +                       err = errno;\n\nYou'll loose errno this way: error() does not save it.\n"},{"id":"117659","messageId":"200907091037.41329.johan@herland.net","threadId":"19853","inReplyTo":"81b0412b0907090101x7c8aa182o36687d67be3c5fb0@mail.gmail.com","subject":"Re: [PATCH] quickfetch(): Prevent overflow of the rev-list command line","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2009-07-09T08:37:41Z","receivedAt":"2009-07-09T08:37:41Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Thursday 09 July 2009, Alex Riesen wrote:\n> On Wed, Jul 8, 2009 at 18:01, Johan Herland<johan@herland.net> wrote:\n> > On Wednesday 08 July 2009, Johannes Sixt wrote:\n> >> ... don't you get this error message with errno set to EPIPE?\n> >> Previously, there was no error message.\n> >\n> > Indeed, you are correct. I guess the following should be added to the\n> > patch:\n> >\n> >        if (write_in_full(revlist.in, sha1_to_hex(ref->old_sha1), 40) <\n> > 0 || write_in_full(revlist.in, \"\\n\", 1) < 0) {\n> > -               error(\"failed write to rev-list\");\n> > -               err = errno;\n> > +               if (errno != EPIPE) {\n> > +                       error(\"failed write to rev-list\");\n> > +                       err = errno;\n>\n> You'll loose errno this way: error() does not save it.\n\nNot sure what you mean here. Should I move \"err = errno;\" outside the \ninnermost \"if\"?\n\n>From my POV, if errno != EPIPE, we save it into err, and return that \n(overridden by finish_command()'s return value, if non-zero). If errno == \nEPIPE, we're not interested in saving it, because we expect finish_command() \nto return non-zero in any case.\n\n\n...Johan\n\n-- \nJohan Herland, <johan@herland.net>\nwww.herland.net\n"},{"id":"117661","messageId":"200907091043.03263.johan@herland.net","threadId":"19853","inReplyTo":"7vws6j6qed.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] quickfetch(): Prevent overflow of the rev-list command line","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2009-07-09T08:43:03Z","receivedAt":"2009-07-09T08:43:03Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Wednesday 08 July 2009, Junio C Hamano wrote:\n> Johan Herland <johan@herland.net> writes:\n> > Maybe I need to do something to the close() call as well? What happens\n> > on close() after EPIPE?\n>\n> You should be OK (you could try this).\n>\n> -- >8 --\n\nThanks! The programs works well on my Linux box (close() succeeds), but it \ndoes not run at all in Windows/MSYS (lacks pipe() and fork()).\n\nDoes anybody with Windows/MSYS experience know how this scenario (write() to \na terminated process, followed by close()) would play out in msysGit?\n\n\n...Johan\n\n-- \nJohan Herland, <johan@herland.net>\nwww.herland.net\n"},{"id":"117662","messageId":"81b0412b0907090143r7c894c6fh15db309b53b9c446@mail.gmail.com","threadId":"19853","inReplyTo":"200907091037.41329.johan@herland.net","subject":"Re: [PATCH] quickfetch(): Prevent overflow of the rev-list command line","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-07-09T08:43:38Z","receivedAt":"2009-07-09T08:43:38Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On Thu, Jul 9, 2009 at 10:37, Johan Herland<johan@herland.net> wrote:\n> On Thursday 09 July 2009, Alex Riesen wrote:\n>> On Wed, Jul 8, 2009 at 18:01, Johan Herland<johan@herland.net> wrote:\n>> > On Wednesday 08 July 2009, Johannes Sixt wrote:\n>> >> ... don't you get this error message with errno set to EPIPE?\n>> >> Previously, there was no error message.\n>> >\n>> > Indeed, you are correct. I guess the following should be added to the\n>> > patch:\n>> >\n>> >        if (write_in_full(revlist.in, sha1_to_hex(ref->old_sha1), 40) <\n>> > 0 || write_in_full(revlist.in, \"\\n\", 1) < 0) {\n>> > -               error(\"failed write to rev-list\");\n>> > -               err = errno;\n>> > +               if (errno != EPIPE) {\n>> > +                       error(\"failed write to rev-list\");\n>> > +                       err = errno;\n>>\n>> You'll loose errno this way: error() does not save it.\n>\n> Not sure what you mean here. Should I move \"err = errno;\" outside the\n> innermost \"if\"?\n\nput it before error(\"failed write to rev-list\"); or even before the\n\"if (err != EPIPE)\".\nOtherwise it is 0 after fprintf to stderr (which is the error() call).\n\n> From my POV, if errno != EPIPE, we save it into err, and return that\n> (overridden by finish_command()'s return value, if non-zero). If errno ==\n> EPIPE, we're not interested in saving it, because we expect finish_command()\n> to return non-zero in any case.\n\nAnd you think this expectation makes the code simpler to understand?\n"},{"id":"117664","messageId":"81b0412b0907090149l34450255ka77a2341e7ee966b@mail.gmail.com","threadId":"19853","inReplyTo":"200907091043.03263.johan@herland.net","subject":"Re: [PATCH] quickfetch(): Prevent overflow of the rev-list command line","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-07-09T08:49:07Z","receivedAt":"2009-07-09T08:49:07Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On Thu, Jul 9, 2009 at 10:43, Johan Herland<johan@herland.net> wrote:\n> On Wednesday 08 July 2009, Junio C Hamano wrote:\n>> Johan Herland <johan@herland.net> writes:\n>> > Maybe I need to do something to the close() call as well? What happens\n>> > on close() after EPIPE?\n>\n> Does anybody with Windows/MSYS experience know how this scenario (write() to\n> a terminated process, followed by close()) would play out in msysGit?\n>\n\nIt fails with ERROR_BROKEN_PIPE. See MSDN for WriteFile:\n\n http://msdn.microsoft.com/en-us/library/aa365747%28VS.85%29.aspx\n\n(look for the error above).\n\nWell, sometimes it just fails, so you can hardly use the error code to detect\nif the other process is truly gone or something broke in Windows.\n"},{"id":"117666","messageId":"4A55AF88.3090205@viscovery.net","threadId":"19853","inReplyTo":"200907091043.03263.johan@herland.net","subject":"Re: [PATCH] quickfetch(): Prevent overflow of the rev-list command line","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-07-09T08:51:20Z","receivedAt":"2009-07-09T08:51:20Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Johan Herland schrieb:\n> Does anybody with Windows/MSYS experience know how this scenario (write() to \n> a terminated process, followed by close()) would play out in msysGit?\n\nThe first write() sometimes fails with EPIPE, otherwise it fails with\nEINVAL. All subsequent write()s fail with EINVAL. The setting of SIGPIPE\nis irrelevant because it is unknown to Windows.\n\nThere's precedent already in write_or_die.c. You should not write the\nerror message for both EPIPE and EINVAL.\n\n-- Hannes\n"},{"id":"117672","messageId":"200907091107.46838.johan@herland.net","threadId":"19853","inReplyTo":"4A55AF88.3090205@viscovery.net","subject":"Re: [PATCH] quickfetch(): Prevent overflow of the rev-list command line","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2009-07-09T09:07:46Z","receivedAt":"2009-07-09T09:07:46Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Thursday 09 July 2009, Johannes Sixt wrote:\n> Johan Herland schrieb:\n> > Does anybody with Windows/MSYS experience know how this scenario\n> > (write() to a terminated process, followed by close()) would play out\n> > in msysGit?\n>\n> The first write() sometimes fails with EPIPE, otherwise it fails with\n> EINVAL. All subsequent write()s fail with EINVAL. The setting of SIGPIPE\n> is irrelevant because it is unknown to Windows.\n>\n> There's precedent already in write_or_die.c. You should not write the\n> error message for both EPIPE and EINVAL.\n\nThanks, but what about the subsequent close()? Will it fail with EINVAL? \nEBADF? or will is succeed (like on Linux)?\n\nI will send an updated patch with all fixes, as soon as I know what to do \nabout close().\n\n\n...Johan\n\n-- \nJohan Herland, <johan@herland.net>\nwww.herland.net\n"},{"id":"117675","messageId":"4A55B54D.4080601@viscovery.net","threadId":"19853","inReplyTo":"200907091107.46838.johan@herland.net","subject":"Re: [PATCH] quickfetch(): Prevent overflow of the rev-list command line","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-07-09T09:15:57Z","receivedAt":"2009-07-09T09:15:57Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Johan Herland schrieb:\n> Thanks, but what about the subsequent close()? Will it fail with EINVAL? \n> EBADF? or will is succeed (like on Linux)?\n\nI guess it succeeds, but I don't have a quick test. Send your patch, and\nwe'll see ;)\n\n-- Hannes\n"},{"id":"117677","messageId":"200907091134.45492.johan@herland.net","threadId":"19853","inReplyTo":"4A55B54D.4080601@viscovery.net","subject":"[PATCH] quickfetch(): Prevent overflow of the rev-list command line","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2009-07-09T09:34:45Z","receivedAt":"2009-07-09T09:34:45Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"quickfetch() calls rev-list to check whether the objects we are about to\nfetch are already present in the repo (if so, we can skip the object fetch).\nHowever, when there are many (~1000) refs to be fetched, the rev-list\ncommand line grows larger than the maximum command line size on some systems\n(32K in Windows). This causes rev-list to fail, making quickfetch() return\nnon-zero, which unnecessarily triggers the transport machinery. This somehow\ncauses fetch to fail with an exit code.\n\nBy using the --stdin option to rev-list (and feeding the object list to its\nstandard input), we prevent the overflow of the rev-list command line,\nwhich causes quickfetch(), and subsequently the overall fetch, to succeed.\n\nHowever, using rev-list --stdin is not entirely straightforward: rev-list\nterminates immediately when encountering an unknown object, which can\ntrigger SIGPIPE if we are still writing object's to its standard input.\nWe therefore ignore SIGPIPE so that the fetch process is not terminated.\n\nSigned-off-by: Johan Herland <johan@herland.net>\nImproved-by: Johannes Sixt <j.sixt@viscovery.net>\nImproved-by: Alex Riesen <raa.lkml@gmail.com>\nTested-by: Peter Krefting <peter@softwolves.pp.se>\n---\n builtin-fetch.c |   62 +++++++++++++++++++++++++++++++------------------------\n 1 files changed, 35 insertions(+), 27 deletions(-)\n\ndiff --git a/builtin-fetch.c b/builtin-fetch.c\nindex cd5eb9a..20bcbdd 100644\n--- a/builtin-fetch.c\n+++ b/builtin-fetch.c\n@@ -400,14 +400,14 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \n /*\n  * We would want to bypass the object transfer altogether if\n- * everything we are going to fetch already exists and connected\n+ * everything we are going to fetch already exists and is connected\n  * locally.\n  *\n- * The refs we are going to fetch are in to_fetch (nr_heads in\n- * total).  If running\n+ * The refs we are going to fetch are in ref_map.  If running\n  *\n- *  $ git rev-list --objects to_fetch[0] to_fetch[1] ... --not --all\n+ *  $ git rev-list --objects --stdin --not --all\n  *\n+ * (feeding all the refs in ref_map on its standard input)\n  * does not error out, that means everything reachable from the\n  * refs we are going to fetch exists and is connected to some of\n  * our existing refs.\n@@ -416,8 +416,9 @@ static int quickfetch(struct ref *ref_map)\n {\n \tstruct child_process revlist;\n \tstruct ref *ref;\n-\tchar **argv;\n-\tint i, err;\n+\tint err;\n+\tconst char *argv[] = {\"rev-list\",\n+\t\t\"--quiet\", \"--objects\", \"--stdin\", \"--not\", \"--all\", NULL};\n \n \t/*\n \t * If we are deepening a shallow clone we already have these\n@@ -429,34 +430,41 @@ static int quickfetch(struct ref *ref_map)\n \tif (depth)\n \t\treturn -1;\n \n-\tfor (i = 0, ref = ref_map; ref; ref = ref->next)\n-\t\ti++;\n-\tif (!i)\n+\tif (!ref_map)\n \t\treturn 0;\n \n-\targv = xmalloc(sizeof(*argv) * (i + 6));\n-\ti = 0;\n-\targv[i++] = xstrdup(\"rev-list\");\n-\targv[i++] = xstrdup(\"--quiet\");\n-\targv[i++] = xstrdup(\"--objects\");\n-\tfor (ref = ref_map; ref; ref = ref->next)\n-\t\targv[i++] = xstrdup(sha1_to_hex(ref->old_sha1));\n-\targv[i++] = xstrdup(\"--not\");\n-\targv[i++] = xstrdup(\"--all\");\n-\targv[i++] = NULL;\n-\n \tmemset(&revlist, 0, sizeof(revlist));\n-\trevlist.argv = (const char**)argv;\n+\trevlist.argv = argv;\n \trevlist.git_cmd = 1;\n-\trevlist.no_stdin = 1;\n \trevlist.no_stdout = 1;\n \trevlist.no_stderr = 1;\n-\terr = run_command(&revlist);\n+\trevlist.in = -1;\n+\n+\t/* If rev-list --stdin encounters an unknown commit, it terminates,\n+\t * which will cause SIGPIPE in the write loop below. */\n+\tsignal(SIGPIPE, SIG_IGN);\n+\n+\terr = start_command(&revlist);\n+\tif (err) {\n+\t\terror(\"could not run rev-list\");\n+\t\treturn err;\n+\t}\n \n-\tfor (i = 0; argv[i]; i++)\n-\t\tfree(argv[i]);\n-\tfree(argv);\n-\treturn err;\n+\tfor (ref = ref_map; ref; ref = ref->next) {\n+\t\tif (write_in_full(revlist.in, sha1_to_hex(ref->old_sha1), 40) < 0 ||\n+\t\t    write_in_full(revlist.in, \"\\n\", 1) < 0) {\n+\t\t\terr = errno;\n+\t\t\tif (err != EPIPE && err != EINVAL)\n+\t\t\t\terror(\"failed write to rev-list\");\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\n+\tif (close(revlist.in)) {\n+\t\terr = errno;\n+\t\terror(\"failed to close rev-list's stdin\");\n+\t}\n+\treturn finish_command(&revlist) || err;\n }\n \n static int fetch_refs(struct transport *transport, struct ref *ref_map)\n-- \n1.6.3.rc0.1.gf800\n"},{"id":"117685","messageId":"4A55E100.9010700@viscovery.net","threadId":"19853","inReplyTo":"200907091134.45492.johan@herland.net","subject":"Re: [PATCH] quickfetch(): Prevent overflow of the rev-list command line","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-07-09T12:22:24Z","receivedAt":"2009-07-09T12:22:24Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Johan Herland schrieb:\n> However, using rev-list --stdin is not entirely straightforward: rev-list\n> terminates immediately when encountering an unknown object, which can\n> trigger SIGPIPE if we are still writing object's to its standard input.\n> We therefore ignore SIGPIPE so that the fetch process is not terminated.\n\nI removed the \"signal(SIGPIPE, SIG_IGN)\", but the test suite still passes.\nIOW, there is no test case that has the configuration that you describe\nhere. Would you please add such a test (perhaps in t5502)? It would also\nhelp me verify the patch works as intended on Windows.\n\n> Signed-off-by: Johan Herland <johan@herland.net>\n> Improved-by: Johannes Sixt <j.sixt@viscovery.net>\n\nPlease make this <j6t@kdbg.org> despite the email address I'm using right now.\n\n> Improved-by: Alex Riesen <raa.lkml@gmail.com>\n> Tested-by: Peter Krefting <peter@softwolves.pp.se>\n\n> +\tfor (ref = ref_map; ref; ref = ref->next) {\n> +\t\tif (write_in_full(revlist.in, sha1_to_hex(ref->old_sha1), 40) < 0 ||\n> +\t\t    write_in_full(revlist.in, \"\\n\", 1) < 0) {\n> +\t\t\terr = errno;\n> +\t\t\tif (err != EPIPE && err != EINVAL)\n> +\t\t\t\terror(\"failed write to rev-list\");\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\tif (close(revlist.in)) {\n> +\t\terr = errno;\n> +\t\terror(\"failed to close rev-list's stdin\");\n> +\t}\n> +\treturn finish_command(&revlist) || err;\n\nThe call site of quickfetch() is not interested in the errno, only on\nwhether the return value is non-zero: You can just assign -1 to err\n(that's our convention for failure). OTOH, it would be helpful to include\nstrerror(errno) in the error message.\n\nShouldn't you reset signal(SIGPIPE) to its previous value?\n\n-- Hannes\n"},{"id":"117691","messageId":"200907091552.44545.johan@herland.net","threadId":"19853","inReplyTo":"4A55E100.9010700@viscovery.net","subject":"[PATCH v3] quickfetch(): Prevent overflow of the rev-list command line","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2009-07-09T13:52:44Z","receivedAt":"2009-07-09T13:52:44Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"quickfetch() calls rev-list to check whether the objects we are about to\nfetch are already present in the repo (if so, we can skip the object fetch).\nHowever, when there are many (~1000) refs to be fetched, the rev-list\ncommand line grows larger than the maximum command line size on some systems\n(32K in Windows). This causes rev-list to fail, making quickfetch() return\nnon-zero, which unnecessarily triggers the transport machinery. This somehow\ncauses fetch to fail with an exit code.\n\nBy using the --stdin option to rev-list (and feeding the object list to its\nstandard input), we prevent the overflow of the rev-list command line,\nwhich causes quickfetch(), and subsequently the overall fetch, to succeed.\n\nHowever, using rev-list --stdin is not entirely straightforward: rev-list\nterminates immediately when encountering an unknown object, which can\ntrigger SIGPIPE if we are still writing object's to its standard input.\nWe therefore temporarily ignore SIGPIPE so that the fetch process is not\nterminated.\n\nThe patch also contains a testcase to verify the fix (note that before\nthe patch, the testcase would only fail on msysGit).\n\nSigned-off-by: Johan Herland <johan@herland.net>\nImproved-by: Johannes Sixt <j6t@kdbg.org>\nImproved-by: Alex Riesen <raa.lkml@gmail.com>\nTested-by: Peter Krefting <peter@softwolves.pp.se>\n---\n\nOn Thursday 09 July 2009, Johannes Sixt wrote:\n> Would you please add such a test (perhaps in t5502)? It\n> would also help me verify the patch works as intended on Windows.\n\nDone (although somewhat naively). I don't have an msysgit setup to test\nthis, but faking the failure condition in quickfetch() (return -1 if\n#refs > 800) does trigger the selftest (the second git fetch fails).\n\nI could add a separate pre-patch introducing the selftest with\ntest_expect_failure, but that would only confuse non-msysgit users\nwhere the test succeeds both before and after the fix.\n\n> Please make this <j6t@kdbg.org> despite the email address I'm using right\n> now.\n\nOk.\n\n> The call site of quickfetch() is not interested in the errno, only on\n> whether the return value is non-zero: You can just assign -1 to err\n> (that's our convention for failure). OTOH, it would be helpful to include\n> strerror(errno) in the error message.\n\nFixed.\n\n> Shouldn't you reset signal(SIGPIPE) to its previous value?\n\nDone (provided that the sigchain_push/pop infrastructure works the way\nI expect).\n\nThanks a lot for your review and suggestions.\n\n\nHave fun! :)\n\n...Johan\n\n\n builtin-fetch.c       |   65 ++++++++++++++++++++++++++++--------------------\n t/t5502-quickfetch.sh |   20 +++++++++++++++\n 2 files changed, 58 insertions(+), 27 deletions(-)\n\ndiff --git a/builtin-fetch.c b/builtin-fetch.c\nindex cd5eb9a..2e3c609 100644\n--- a/builtin-fetch.c\n+++ b/builtin-fetch.c\n@@ -400,14 +400,14 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \n /*\n  * We would want to bypass the object transfer altogether if\n- * everything we are going to fetch already exists and connected\n+ * everything we are going to fetch already exists and is connected\n  * locally.\n  *\n- * The refs we are going to fetch are in to_fetch (nr_heads in\n- * total).  If running\n+ * The refs we are going to fetch are in ref_map.  If running\n  *\n- *  $ git rev-list --objects to_fetch[0] to_fetch[1] ... --not --all\n+ *  $ git rev-list --objects --stdin --not --all\n  *\n+ * (feeding all the refs in ref_map on its standard input)\n  * does not error out, that means everything reachable from the\n  * refs we are going to fetch exists and is connected to some of\n  * our existing refs.\n@@ -416,8 +416,9 @@ static int quickfetch(struct ref *ref_map)\n {\n \tstruct child_process revlist;\n \tstruct ref *ref;\n-\tchar **argv;\n-\tint i, err;\n+\tint err;\n+\tconst char *argv[] = {\"rev-list\",\n+\t\t\"--quiet\", \"--objects\", \"--stdin\", \"--not\", \"--all\", NULL};\n \n \t/*\n \t * If we are deepening a shallow clone we already have these\n@@ -429,34 +430,44 @@ static int quickfetch(struct ref *ref_map)\n \tif (depth)\n \t\treturn -1;\n \n-\tfor (i = 0, ref = ref_map; ref; ref = ref->next)\n-\t\ti++;\n-\tif (!i)\n+\tif (!ref_map)\n \t\treturn 0;\n \n-\targv = xmalloc(sizeof(*argv) * (i + 6));\n-\ti = 0;\n-\targv[i++] = xstrdup(\"rev-list\");\n-\targv[i++] = xstrdup(\"--quiet\");\n-\targv[i++] = xstrdup(\"--objects\");\n-\tfor (ref = ref_map; ref; ref = ref->next)\n-\t\targv[i++] = xstrdup(sha1_to_hex(ref->old_sha1));\n-\targv[i++] = xstrdup(\"--not\");\n-\targv[i++] = xstrdup(\"--all\");\n-\targv[i++] = NULL;\n-\n \tmemset(&revlist, 0, sizeof(revlist));\n-\trevlist.argv = (const char**)argv;\n+\trevlist.argv = argv;\n \trevlist.git_cmd = 1;\n-\trevlist.no_stdin = 1;\n \trevlist.no_stdout = 1;\n \trevlist.no_stderr = 1;\n-\terr = run_command(&revlist);\n+\trevlist.in = -1;\n+\n+\t/* If rev-list --stdin encounters an unknown commit, it terminates,\n+\t * which will cause SIGPIPE in the write loop below. */\n+\tsigchain_push(SIGPIPE, SIG_IGN);\n+\n+\terr = start_command(&revlist);\n+\tif (err) {\n+\t\terror(\"could not run rev-list\");\n+\t\treturn err;\n+\t}\n+\n+\tfor (ref = ref_map; ref; ref = ref->next) {\n+\t\tif (write_in_full(revlist.in, sha1_to_hex(ref->old_sha1), 40) < 0 ||\n+\t\t    write_in_full(revlist.in, \"\\n\", 1) < 0) {\n+\t\t\tif (err != EPIPE && err != EINVAL)\n+\t\t\t\terror(\"failed write to rev-list: %s\", strerror(errno));\n+\t\t\terr = -1;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\n+\tif (close(revlist.in)) {\n+\t\terror(\"failed to close rev-list's stdin: %s\", strerror(errno));\n+\t\terr = -1;\n+\t}\n+\n+\tsigchain_pop(SIGPIPE);\n \n-\tfor (i = 0; argv[i]; i++)\n-\t\tfree(argv[i]);\n-\tfree(argv);\n-\treturn err;\n+\treturn finish_command(&revlist) || err;\n }\n \n static int fetch_refs(struct transport *transport, struct ref *ref_map)\ndiff --git a/t/t5502-quickfetch.sh b/t/t5502-quickfetch.sh\nindex 16eadd6..1037a72 100755\n--- a/t/t5502-quickfetch.sh\n+++ b/t/t5502-quickfetch.sh\n@@ -119,4 +119,24 @@ test_expect_success 'quickfetch should not copy from alternate' '\n \n '\n \n+test_expect_success 'quickfetch should handle ~1000 refs (on Windows)' '\n+\n+\tgit gc &&\n+\thead=$(git rev-parse HEAD) &&\n+\tbranchprefix=\"$head refs/heads/branch\" &&\n+\tfor i in 0 1 2 3 4 5 6 7 8 9; do\n+\t\tfor j in 0 1 2 3 4 5 6 7 8 9; do\n+\t\t\tfor k in 0 1 2 3 4 5 6 7 8 9; do\n+\t\t\t\techo \"$branchprefix$i$j$k\" >> .git/packed-refs\n+\t\t\tdone\n+\t\tdone\n+\tdone &&\n+\t(\n+\t\tcd cloned &&\n+\t\tgit fetch &&\n+\t\tgit fetch\n+\t)\n+\n+'\n+\n test_done\n-- \n1.6.3.rc0.1.gf800\n"},{"id":"117692","messageId":"4A55FCD5.2020908@viscovery.net","threadId":"19853","inReplyTo":"200907091552.44545.johan@herland.net","subject":"Re: [PATCH v3] quickfetch(): Prevent overflow of the rev-list command line","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-07-09T14:21:09Z","receivedAt":"2009-07-09T14:21:09Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Johan Herland schrieb:\n> On Thursday 09 July 2009, Johannes Sixt wrote:\n>> Shouldn't you reset signal(SIGPIPE) to its previous value?\n> \n> Done (provided that the sigchain_push/pop infrastructure works the way\n> I expect).\n\nI'm not sure, either. Peff?\n\n> +test_expect_success 'quickfetch should handle ~1000 refs (on Windows)' '\n> +\n> +\tgit gc &&\n> +\thead=$(git rev-parse HEAD) &&\n> +\tbranchprefix=\"$head refs/heads/branch\" &&\n> +\tfor i in 0 1 2 3 4 5 6 7 8 9; do\n> +\t\tfor j in 0 1 2 3 4 5 6 7 8 9; do\n> +\t\t\tfor k in 0 1 2 3 4 5 6 7 8 9; do\n> +\t\t\t\techo \"$branchprefix$i$j$k\" >> .git/packed-refs\n> +\t\t\tdone\n> +\t\tdone\n> +\tdone &&\n> +\t(\n> +\t\tcd cloned &&\n> +\t\tgit fetch &&\n> +\t\tgit fetch\n> +\t)\n> +\n> +'\n\nThis test fails on Windows without the code change and passes with the\ncode change. So, it's a good test.\n\nBut actually I meant you to make a test that triggers the SIGPIPE that\nwould kill git-fetch if it were not ignored. This one doesn't trigger it,\neither.\n\n-- Hannes\n"},{"id":"117693","messageId":"20090709143203.GA31138@coredump.intra.peff.net","threadId":"19853","inReplyTo":"4A55FCD5.2020908@viscovery.net","subject":"Re: [PATCH v3] quickfetch(): Prevent overflow of the rev-list command line","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-07-09T14:32:03Z","receivedAt":"2009-07-09T14:32:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 09, 2009 at 04:21:09PM +0200, Johannes Sixt wrote:\n\n> Johan Herland schrieb:\n> > On Thursday 09 July 2009, Johannes Sixt wrote:\n> >> Shouldn't you reset signal(SIGPIPE) to its previous value?\n> > \n> > Done (provided that the sigchain_push/pop infrastructure works the way\n> > I expect).\n> \n> I'm not sure, either. Peff?\n\nI don't think I ever tried explicitly pushing SIG_IGN, but the\ninfrastructure was designed so that it would Just Work. So yes, I think\nit's right, but you may want to test it. :)\n\nThat being said, in the patch in question there is an early return after\nthe push that misses the corresponding pop. That should be fixed.\n\n-Peff\n"},{"id":"117694","messageId":"200907091642.05746.johan@herland.net","threadId":"19853","inReplyTo":"4A55FCD5.2020908@viscovery.net","subject":"Re: [PATCH v3] quickfetch(): Prevent overflow of the rev-list command line","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2009-07-09T14:42:05Z","receivedAt":"2009-07-09T14:42:05Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Thursday 09 July 2009, Johannes Sixt wrote:\n> But actually I meant you to make a test that triggers the SIGPIPE that\n> would kill git-fetch if it were not ignored. This one doesn't trigger it,\n> either.\n\nAFAIU from earlier in this thread (and a mail from Peter linking to \nhttp://markmail.org/message/dbgdj4csafen65ye), SIGPIPE _never_ triggers on \nWindows, thus ignoring SIGPIPE is not needed for the fix per se. However, as \na side-effect of the fix, we may now get SIGPIPE on Linux (and other POSIX \nplatforms), so although it never triggers on Windows, it's still needed.\n\n\n...Johan\n\n-- \nJohan Herland, <johan@herland.net>\nwww.herland.net\n"},{"id":"117695","messageId":"200907091649.26575.johan@herland.net","threadId":"19853","inReplyTo":"20090709143203.GA31138@coredump.intra.peff.net","subject":"[PATCH v4] quickfetch(): Prevent overflow of the rev-list command line","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2009-07-09T14:49:26Z","receivedAt":"2009-07-09T14:49:26Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"quickfetch() calls rev-list to check whether the objects we are about to\nfetch are already present in the repo (if so, we can skip the object fetch).\nHowever, when there are many (~1000) refs to be fetched, the rev-list\ncommand line grows larger than the maximum command line size on some systems\n(32K in Windows). This causes rev-list to fail, making quickfetch() return\nnon-zero, which unnecessarily triggers the transport machinery. This somehow\ncauses fetch to fail with an exit code.\n\nBy using the --stdin option to rev-list (and feeding the object list to its\nstandard input), we prevent the overflow of the rev-list command line,\nwhich causes quickfetch(), and subsequently the overall fetch, to succeed.\n\nHowever, using rev-list --stdin is not entirely straightforward: rev-list\nterminates immediately when encountering an unknown object, which can\ntrigger SIGPIPE if we are still writing object's to its standard input.\nWe therefore temporarily ignore SIGPIPE so that the fetch process is not\nterminated.\n\nThe patch also contains a testcase to verify the fix (note that before\nthe patch, the testcase would only fail on msysGit).\n\nSigned-off-by: Johan Herland <johan@herland.net>\nImproved-by: Johannes Sixt <j6t@kdbg.org>\nImproved-by: Alex Riesen <raa.lkml@gmail.com>\nTested-by: Peter Krefting <peter@softwolves.pp.se>\n---\n\nOn Thursday 09 July 2009, Jeff King wrote:\n> That being said, in the patch in question there is an early return after\n> the push that misses the corresponding pop. That should be fixed.\n\nThanks, fixed.\n\n...Johan\n\n-- \nJohan Herland, <johan@herland.net>\nwww.herland.net\n\n builtin-fetch.c       |   65 ++++++++++++++++++++++++++++--------------------\n t/t5502-quickfetch.sh |   20 +++++++++++++++\n 2 files changed, 58 insertions(+), 27 deletions(-)\n\ndiff --git a/builtin-fetch.c b/builtin-fetch.c\nindex cd5eb9a..34ba878 100644\n--- a/builtin-fetch.c\n+++ b/builtin-fetch.c\n@@ -400,14 +400,14 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \n /*\n  * We would want to bypass the object transfer altogether if\n- * everything we are going to fetch already exists and connected\n+ * everything we are going to fetch already exists and is connected\n  * locally.\n  *\n- * The refs we are going to fetch are in to_fetch (nr_heads in\n- * total).  If running\n+ * The refs we are going to fetch are in ref_map.  If running\n  *\n- *  $ git rev-list --objects to_fetch[0] to_fetch[1] ... --not --all\n+ *  $ git rev-list --objects --stdin --not --all\n  *\n+ * (feeding all the refs in ref_map on its standard input)\n  * does not error out, that means everything reachable from the\n  * refs we are going to fetch exists and is connected to some of\n  * our existing refs.\n@@ -416,8 +416,9 @@ static int quickfetch(struct ref *ref_map)\n {\n \tstruct child_process revlist;\n \tstruct ref *ref;\n-\tchar **argv;\n-\tint i, err;\n+\tint err;\n+\tconst char *argv[] = {\"rev-list\",\n+\t\t\"--quiet\", \"--objects\", \"--stdin\", \"--not\", \"--all\", NULL};\n \n \t/*\n \t * If we are deepening a shallow clone we already have these\n@@ -429,34 +430,44 @@ static int quickfetch(struct ref *ref_map)\n \tif (depth)\n \t\treturn -1;\n \n-\tfor (i = 0, ref = ref_map; ref; ref = ref->next)\n-\t\ti++;\n-\tif (!i)\n+\tif (!ref_map)\n \t\treturn 0;\n \n-\targv = xmalloc(sizeof(*argv) * (i + 6));\n-\ti = 0;\n-\targv[i++] = xstrdup(\"rev-list\");\n-\targv[i++] = xstrdup(\"--quiet\");\n-\targv[i++] = xstrdup(\"--objects\");\n-\tfor (ref = ref_map; ref; ref = ref->next)\n-\t\targv[i++] = xstrdup(sha1_to_hex(ref->old_sha1));\n-\targv[i++] = xstrdup(\"--not\");\n-\targv[i++] = xstrdup(\"--all\");\n-\targv[i++] = NULL;\n-\n \tmemset(&revlist, 0, sizeof(revlist));\n-\trevlist.argv = (const char**)argv;\n+\trevlist.argv = argv;\n \trevlist.git_cmd = 1;\n-\trevlist.no_stdin = 1;\n \trevlist.no_stdout = 1;\n \trevlist.no_stderr = 1;\n-\terr = run_command(&revlist);\n+\trevlist.in = -1;\n+\n+\terr = start_command(&revlist);\n+\tif (err) {\n+\t\terror(\"could not run rev-list\");\n+\t\treturn err;\n+\t}\n+\n+\t/* If rev-list --stdin encounters an unknown commit, it terminates,\n+\t * which will cause SIGPIPE in the write loop below. */\n+\tsigchain_push(SIGPIPE, SIG_IGN);\n+\n+\tfor (ref = ref_map; ref; ref = ref->next) {\n+\t\tif (write_in_full(revlist.in, sha1_to_hex(ref->old_sha1), 40) < 0 ||\n+\t\t    write_in_full(revlist.in, \"\\n\", 1) < 0) {\n+\t\t\tif (err != EPIPE && err != EINVAL)\n+\t\t\t\terror(\"failed write to rev-list: %s\", strerror(errno));\n+\t\t\terr = -1;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\n+\tif (close(revlist.in)) {\n+\t\terror(\"failed to close rev-list's stdin: %s\", strerror(errno));\n+\t\terr = -1;\n+\t}\n+\n+\tsigchain_pop(SIGPIPE);\n \n-\tfor (i = 0; argv[i]; i++)\n-\t\tfree(argv[i]);\n-\tfree(argv);\n-\treturn err;\n+\treturn finish_command(&revlist) || err;\n }\n \n static int fetch_refs(struct transport *transport, struct ref *ref_map)\ndiff --git a/t/t5502-quickfetch.sh b/t/t5502-quickfetch.sh\nindex 16eadd6..1037a72 100755\n--- a/t/t5502-quickfetch.sh\n+++ b/t/t5502-quickfetch.sh\n@@ -119,4 +119,24 @@ test_expect_success 'quickfetch should not copy from alternate' '\n \n '\n \n+test_expect_success 'quickfetch should handle ~1000 refs (on Windows)' '\n+\n+\tgit gc &&\n+\thead=$(git rev-parse HEAD) &&\n+\tbranchprefix=\"$head refs/heads/branch\" &&\n+\tfor i in 0 1 2 3 4 5 6 7 8 9; do\n+\t\tfor j in 0 1 2 3 4 5 6 7 8 9; do\n+\t\t\tfor k in 0 1 2 3 4 5 6 7 8 9; do\n+\t\t\t\techo \"$branchprefix$i$j$k\" >> .git/packed-refs\n+\t\t\tdone\n+\t\tdone\n+\tdone &&\n+\t(\n+\t\tcd cloned &&\n+\t\tgit fetch &&\n+\t\tgit fetch\n+\t)\n+\n+'\n+\n test_done\n-- \n1.6.3.rc0.1.gf800\n"},{"id":"117696","messageId":"4A560509.8060909@viscovery.net","threadId":"19853","inReplyTo":"200907091642.05746.johan@herland.net","subject":"Re: [PATCH v3] quickfetch(): Prevent overflow of the rev-list command line","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-07-09T14:56:09Z","receivedAt":"2009-07-09T14:56:09Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Johan Herland schrieb:\n> On Thursday 09 July 2009, Johannes Sixt wrote:\n>> But actually I meant you to make a test that triggers the SIGPIPE that\n>> would kill git-fetch if it were not ignored. This one doesn't trigger it,\n>> either.\n> \n> AFAIU from earlier in this thread (and a mail from Peter linking to \n> http://markmail.org/message/dbgdj4csafen65ye), SIGPIPE _never_ triggers on \n> Windows, thus ignoring SIGPIPE is not needed for the fix per se. However, as \n> a side-effect of the fix, we may now get SIGPIPE on Linux (and other POSIX \n> platforms), so although it never triggers on Windows, it's still needed.\n\nI know that, of course. But try this: Remove the signal(SIGPIPE, SIG_IGN)\nand run the test suite. There is not a single failure. IOW, we don't have\na single test case that verifies that the signal(SIGPIPE, SIG_IGN) is\nneeded. I would like to have that test case, and you seem to know how to\nconstruct it (otherwise there wouldn't be so much buzz about it).\n\n-- Hannes\n"},{"id":"117697","messageId":"200907091732.49708.johan@herland.net","threadId":"19853","inReplyTo":"4A560509.8060909@viscovery.net","subject":"Re: [PATCH v3] quickfetch(): Prevent overflow of the rev-list command line","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2009-07-09T15:32:49Z","receivedAt":"2009-07-09T15:32:49Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Thursday 09 July 2009, Johannes Sixt wrote:\n> Johan Herland schrieb:\n> > On Thursday 09 July 2009, Johannes Sixt wrote:\n> >> But actually I meant you to make a test that triggers the SIGPIPE that\n> >> would kill git-fetch if it were not ignored. This one doesn't trigger\n> >> it, either.\n> >\n> > AFAIU from earlier in this thread (and a mail from Peter linking to\n> > http://markmail.org/message/dbgdj4csafen65ye), SIGPIPE _never_ triggers\n> > on Windows, thus ignoring SIGPIPE is not needed for the fix per se.\n> > However, as a side-effect of the fix, we may now get SIGPIPE on Linux\n> > (and other POSIX platforms), so although it never triggers on Windows,\n> > it's still needed.\n>\n> I know that, of course. But try this: Remove the signal(SIGPIPE, SIG_IGN)\n> and run the test suite. There is not a single failure.\n\nThat's not what I'm seeing. When I don't ignore the signal, the testsuite \nfails intermittently for me (on Linux). I see the following tests fail:\n\n- t3409-rebase-preserve-merges.sh (subtest #2)\n- t5503-tagfollow.sh (subtests #4, #6, #7)\n- t5505-remote.sh (subtests #10, #12, #14 - #20, #27)\n- t5510-fetch.sh (subtest #6 or #25)\n- probably more (I seldom get this far...)\n\nI assume the intermittent failures are caused by git rev-list sometimes \nterminate before git fetch is finished writing objects to its standard input \n(because of scheduling differences).\n\nWhen i enable the signal handling, all selftests pass every time.\n\n\n...Johan\n\n-- \nJohan Herland, <johan@herland.net>\nwww.herland.net\n"},{"id":"117702","messageId":"4A561777.4080306@viscovery.net","threadId":"19853","inReplyTo":"200907091732.49708.johan@herland.net","subject":"Re: [PATCH v3] quickfetch(): Prevent overflow of the rev-list command line","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-07-09T16:14:47Z","receivedAt":"2009-07-09T16:14:47Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Johan Herland schrieb:\n> On Thursday 09 July 2009, Johannes Sixt wrote:\n>> But try this: Remove the signal(SIGPIPE, SIG_IGN)\n>> and run the test suite. There is not a single failure.\n> \n> That's not what I'm seeing. When I don't ignore the signal, the testsuite \n> fails intermittently for me (on Linux). I see the following tests fail:\n> \n> - t3409-rebase-preserve-merges.sh (subtest #2)\n> - t5503-tagfollow.sh (subtests #4, #6, #7)\n> - t5505-remote.sh (subtests #10, #12, #14 - #20, #27)\n> - t5510-fetch.sh (subtest #6 or #25)\n\nI see. If I insert sched_yield() in the for loop, I see many failures (not\n100% reproducible, but almost). On Windows, I have to insert Sleep(10) so\nthat the error exit is taken, and the error code is EINVAL :-/\n\n-- Hannes\n"},{"id":"117703","messageId":"4A5618C1.70106@viscovery.net","threadId":"19853","inReplyTo":"200907091649.26575.johan@herland.net","subject":"Re: [PATCH v4] quickfetch(): Prevent overflow of the rev-list command line","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-07-09T16:20:17Z","receivedAt":"2009-07-09T16:20:17Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"With this fixup the patch is good, I think.\n\n-- Hannes\n\ndiff --git a/builtin-fetch.c b/builtin-fetch.c\nindex 34ba878..7547631 100644\n--- a/builtin-fetch.c\n+++ b/builtin-fetch.c\n@@ -446,14 +446,16 @@ static int quickfetch(struct ref *ref_map)\n \t\treturn err;\n \t}\n\n-\t/* If rev-list --stdin encounters an unknown commit, it terminates,\n-\t * which will cause SIGPIPE in the write loop below. */\n+\t/*\n+\t * If rev-list --stdin encounters an unknown commit, it terminates,\n+\t * which would cause SIGPIPE in the write loop below.\n+\t */\n \tsigchain_push(SIGPIPE, SIG_IGN);\n\n \tfor (ref = ref_map; ref; ref = ref->next) {\n \t\tif (write_in_full(revlist.in, sha1_to_hex(ref->old_sha1), 40) < 0 ||\n \t\t    write_in_full(revlist.in, \"\\n\", 1) < 0) {\n-\t\t\tif (err != EPIPE && err != EINVAL)\n+\t\t\tif (errno != EPIPE && errno != EINVAL)\n \t\t\t\terror(\"failed write to rev-list: %s\", strerror(errno));\n \t\t\terr = -1;\n \t\t\tbreak;\n"},{"id":"117739","messageId":"200907100152.30683.johan@herland.net","threadId":"19853","inReplyTo":"4A5618C1.70106@viscovery.net","subject":"[PATCH v5] quickfetch(): Prevent overflow of the rev-list command line","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2009-07-09T23:52:30Z","receivedAt":"2009-07-09T23:52:30Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"quickfetch() calls rev-list to check whether the objects we are about to\nfetch are already present in the repo (if so, we can skip the object fetch).\nHowever, when there are many (~1000) refs to be fetched, the rev-list\ncommand line grows larger than the maximum command line size on some systems\n(32K in Windows). This causes rev-list to fail, making quickfetch() return\nnon-zero, which unnecessarily triggers the transport machinery. This somehow\ncauses fetch to fail with an exit code.\n\nBy using the --stdin option to rev-list (and feeding the object list to its\nstandard input), we prevent the overflow of the rev-list command line,\nwhich causes quickfetch(), and subsequently the overall fetch, to succeed.\n\nHowever, using rev-list --stdin is not entirely straightforward: rev-list\nterminates immediately when encountering an unknown object, which can\ntrigger SIGPIPE if we are still writing object's to its standard input.\nWe therefore temporarily ignore SIGPIPE so that the fetch process is not\nterminated.\n\nThe patch also contains a testcase to verify the fix (note that before\nthe patch, the testcase would only fail on msysGit).\n\nSigned-off-by: Johan Herland <johan@herland.net>\nImproved-by: Johannes Sixt <j6t@kdbg.org>\nImproved-by: Alex Riesen <raa.lkml@gmail.com>\nTested-by: Peter Krefting <peter@softwolves.pp.se>\n---\n\nOn Thursday 09 July 2009, Johannes Sixt wrote:\n> With this fixup the patch is good, I think.\n\nAgain, thanks for all your help!\n\nHere's the final (*crossing fingers*) iteration of the patch.\n\n\nHave fun! :)\n\n...Johan\n\n\n builtin-fetch.c       |   67 +++++++++++++++++++++++++++++-------------------\n t/t5502-quickfetch.sh |   20 ++++++++++++++\n 2 files changed, 60 insertions(+), 27 deletions(-)\n\ndiff --git a/builtin-fetch.c b/builtin-fetch.c\nindex cd5eb9a..817dd6b 100644\n--- a/builtin-fetch.c\n+++ b/builtin-fetch.c\n@@ -400,14 +400,14 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \n /*\n  * We would want to bypass the object transfer altogether if\n- * everything we are going to fetch already exists and connected\n+ * everything we are going to fetch already exists and is connected\n  * locally.\n  *\n- * The refs we are going to fetch are in to_fetch (nr_heads in\n- * total).  If running\n+ * The refs we are going to fetch are in ref_map.  If running\n  *\n- *  $ git rev-list --objects to_fetch[0] to_fetch[1] ... --not --all\n+ *  $ git rev-list --objects --stdin --not --all\n  *\n+ * (feeding all the refs in ref_map on its standard input)\n  * does not error out, that means everything reachable from the\n  * refs we are going to fetch exists and is connected to some of\n  * our existing refs.\n@@ -416,8 +416,9 @@ static int quickfetch(struct ref *ref_map)\n {\n \tstruct child_process revlist;\n \tstruct ref *ref;\n-\tchar **argv;\n-\tint i, err;\n+\tint err;\n+\tconst char *argv[] = {\"rev-list\",\n+\t\t\"--quiet\", \"--objects\", \"--stdin\", \"--not\", \"--all\", NULL};\n \n \t/*\n \t * If we are deepening a shallow clone we already have these\n@@ -429,34 +430,46 @@ static int quickfetch(struct ref *ref_map)\n \tif (depth)\n \t\treturn -1;\n \n-\tfor (i = 0, ref = ref_map; ref; ref = ref->next)\n-\t\ti++;\n-\tif (!i)\n+\tif (!ref_map)\n \t\treturn 0;\n \n-\targv = xmalloc(sizeof(*argv) * (i + 6));\n-\ti = 0;\n-\targv[i++] = xstrdup(\"rev-list\");\n-\targv[i++] = xstrdup(\"--quiet\");\n-\targv[i++] = xstrdup(\"--objects\");\n-\tfor (ref = ref_map; ref; ref = ref->next)\n-\t\targv[i++] = xstrdup(sha1_to_hex(ref->old_sha1));\n-\targv[i++] = xstrdup(\"--not\");\n-\targv[i++] = xstrdup(\"--all\");\n-\targv[i++] = NULL;\n-\n \tmemset(&revlist, 0, sizeof(revlist));\n-\trevlist.argv = (const char**)argv;\n+\trevlist.argv = argv;\n \trevlist.git_cmd = 1;\n-\trevlist.no_stdin = 1;\n \trevlist.no_stdout = 1;\n \trevlist.no_stderr = 1;\n-\terr = run_command(&revlist);\n+\trevlist.in = -1;\n+\n+\terr = start_command(&revlist);\n+\tif (err) {\n+\t\terror(\"could not run rev-list\");\n+\t\treturn err;\n+\t}\n+\n+\t/*\n+\t * If rev-list --stdin encounters an unknown commit, it terminates,\n+\t * which will cause SIGPIPE in the write loop below.\n+\t */\n+\tsigchain_push(SIGPIPE, SIG_IGN);\n+\n+\tfor (ref = ref_map; ref; ref = ref->next) {\n+\t\tif (write_in_full(revlist.in, sha1_to_hex(ref->old_sha1), 40) < 0 ||\n+\t\t    write_in_full(revlist.in, \"\\n\", 1) < 0) {\n+\t\t\tif (errno != EPIPE && errno != EINVAL)\n+\t\t\t\terror(\"failed write to rev-list: %s\", strerror(errno));\n+\t\t\terr = -1;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\n+\tif (close(revlist.in)) {\n+\t\terror(\"failed to close rev-list's stdin: %s\", strerror(errno));\n+\t\terr = -1;\n+\t}\n+\n+\tsigchain_pop(SIGPIPE);\n \n-\tfor (i = 0; argv[i]; i++)\n-\t\tfree(argv[i]);\n-\tfree(argv);\n-\treturn err;\n+\treturn finish_command(&revlist) || err;\n }\n \n static int fetch_refs(struct transport *transport, struct ref *ref_map)\ndiff --git a/t/t5502-quickfetch.sh b/t/t5502-quickfetch.sh\nindex 16eadd6..1037a72 100755\n--- a/t/t5502-quickfetch.sh\n+++ b/t/t5502-quickfetch.sh\n@@ -119,4 +119,24 @@ test_expect_success 'quickfetch should not copy from alternate' '\n \n '\n \n+test_expect_success 'quickfetch should handle ~1000 refs (on Windows)' '\n+\n+\tgit gc &&\n+\thead=$(git rev-parse HEAD) &&\n+\tbranchprefix=\"$head refs/heads/branch\" &&\n+\tfor i in 0 1 2 3 4 5 6 7 8 9; do\n+\t\tfor j in 0 1 2 3 4 5 6 7 8 9; do\n+\t\t\tfor k in 0 1 2 3 4 5 6 7 8 9; do\n+\t\t\t\techo \"$branchprefix$i$j$k\" >> .git/packed-refs\n+\t\t\tdone\n+\t\tdone\n+\tdone &&\n+\t(\n+\t\tcd cloned &&\n+\t\tgit fetch &&\n+\t\tgit fetch\n+\t)\n+\n+'\n+\n test_done\n-- \n1.6.3.rc0.1.gf800\n"},{"id":"117817","messageId":"7vy6qvn1ya.fsf@alter.siamese.dyndns.org","threadId":"19853","inReplyTo":"200907100152.30683.johan@herland.net","subject":"Re: [PATCH v5] quickfetch(): Prevent overflow of the rev-list command line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-07-11T06:55:25Z","receivedAt":"2009-07-11T06:55:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland <johan@herland.net> writes:\n\n> quickfetch() calls rev-list to check whether the objects we are about to\n> fetch are already present in the repo (if so, we can skip the object fetch).\n> However, when there are many (~1000) refs to be fetched, the rev-list\n> command line grows larger than the maximum command line size on some systems\n> (32K in Windows). This causes rev-list to fail, making quickfetch() return\n> non-zero, which unnecessarily triggers the transport machinery. This somehow\n> causes fetch to fail with an exit code.\n>\n> By using the --stdin option to rev-list (and feeding the object list to its\n> standard input), we prevent the overflow of the rev-list command line,\n> which causes quickfetch(), and subsequently the overall fetch, to succeed.\n\nI feel uneasy with that \"somehow\" at the end of the first paragraph, but\nnevertheless this is the right thing to do.  Since it is a very isolated\nchange, I'd queue this directly on 'master' and see if anybody notices a\nbreakage, as it would be relatively pain-free to revert if it turns out to\nbe necessary.\n\nThanks.\n"},{"id":"117824","messageId":"200907111258.25118.johan@herland.net","threadId":"19853","inReplyTo":"7vy6qvn1ya.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v5] quickfetch(): Prevent overflow of the rev-list command line","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2009-07-11T10:58:24Z","receivedAt":"2009-07-11T10:58:24Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Saturday 11 July 2009, Junio C Hamano wrote:\n> Johan Herland <johan@herland.net> writes:\n> > quickfetch() calls rev-list to check whether the objects we are about\n> > to fetch are already present in the repo (if so, we can skip the object\n> > fetch). However, when there are many (~1000) refs to be fetched, the\n> > rev-list command line grows larger than the maximum command line size\n> > on some systems (32K in Windows). This causes rev-list to fail, making\n> > quickfetch() return non-zero, which unnecessarily triggers the\n> > transport machinery. This somehow causes fetch to fail with an exit\n> > code.\n> >\n> > By using the --stdin option to rev-list (and feeding the object list to\n> > its standard input), we prevent the overflow of the rev-list command\n> > line, which causes quickfetch(), and subsequently the overall fetch, to\n> > succeed.\n>\n> I feel uneasy with that \"somehow\" at the end of the first paragraph, but\n> nevertheless this is the right thing to do.\n\nYes, it feels wrong that transport_fetch_refs() returns error when there are \nno objects to be fetched. After all, the quickfetch() routine is only meant \nto be an optimization (to skip the object fetching when not needed). Only if \nquickfetch() returned a false positive (indicating that all objects are \npresent when they're really not) would I expect it to have adverse effects \non the result of the fetch. But even then, as you indicate, fixing \nquickfetch() itself is the right thing to do, and looking into \ntransport_fetch_refs() is a separate issue.\n\n> Since it is a very isolated\n> change, I'd queue this directly on 'master' and see if anybody notices a\n> breakage, as it would be relatively pain-free to revert if it turns out\n> to be necessary.\n\nThanks.\n\n\nHave fun! :)\n\n...Johan\n\n-- \nJohan Herland, <johan@herland.net>\nwww.herland.net\n"}]}