{"thread":{"id":"29986","subject":"Clone fails on a repo with too many heads/tags","startedAt":"2012-03-18T08:14:30Z","lastAt":"2012-03-28T00:14:45Z","messageCount":54,"participants":["Ivan Todoroski","Nguyen Thai Ngoc Duy","Jakub Narebski","Jeff King","Shawn Pearce","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"187201","messageId":"loom.20120318T083216-96@post.gmane.org","threadId":"29986","inReplyTo":null,"subject":"Clone fails on a repo with too many heads/tags","fromName":"Ivan Todoroski","fromEmail":"grnch_lists@gmx.net","sentAt":"2012-03-18T08:14:30Z","receivedAt":"2012-03-18T08:14:30Z","isPatch":false,"sender":{"key":"grnch_lists@gmx.net","avatar":null},"body":"I recently tried cloning a fresh copy of a large repo (converted from CVS, \nnearly 10 years of history) and to my surprise \"git clone\" failed with the \nfollowing message:\n\n error: cannot spawn git: No such file or directory\n\nThe problem is only reproduced using the Smart HTTP transport.\n\nI used msysGit on Windows so my first instinct was to contact them, but after \nsome poking around I discovered that the problem is present in the Linux \nversion too, although harder to trigger.\n\nTry executing this script:\n\n-------------------------------\ngit init too-many-refs\ncd too-many-refs\necho bla > bla.txt\ngit add .\ngit commit -m test\nsha=$(git rev-parse HEAD)\nfor ((i=0; i<100000; i++)); do \n\techo $sha refs/tags/artificially-long-tag-name-to-more-easily-\ndemonstrate-the-problem-$i >> .git/packed-refs\ndone\n-------------------------------\n\nNow share this repo using the Smart HTTP transport (git-http-backend) and then \ntry cloning it in a different directory. This is what you would get:\n\n$ git clone http://localhost/.../too-many-refs/.git\nCloning into 'too-many-refs'...\nfatal: cannot exec 'fetch-pack': Argument list too long\n\nSo we come to the real reason for the failure: somewhere inside Git a \nsubcommand is invoked with all the tags/heads on the command line and if you \nhave enough of them it overflows the command line length limit of the OS.\n\nObviously the number of tags in the \"too-many-refs\" repo above is absurd (100k) \nbecause the cmdline length in Linux is much more generous, but on Windows the \nclone fails with as little as 500 tags in the above loop! I am already hitting \nthis problem with msysGit on real repos, not just artificial test cases.\n\nI tracked down the problem to remote-curl.c:fetch_git(). That's where the \n\"fetch-pack\" command line is being constructed with all the refs on one line:\n\ngit fetch-pack --stateless-rpc --lock-pack ......\n\nThe solution is conceptually simple: if the list of refs results in a too long \ncommand line, split the refs in batches and call fetch-pack multiple times such \nthat each call is under the cmdline limit:\n\ngit fetch-pack --stateless-rpc --lock-pack ......\ngit fetch-pack --stateless-rpc --lock-pack ......\n...\ngit fetch-pack --stateless-rpc --lock-pack ......\n"},{"id":"187204","messageId":"loom.20120318T121808-344@post.gmane.org","threadId":"29986","inReplyTo":"loom.20120318T083216-96@post.gmane.org","subject":"Re: Clone fails on a repo with too many heads/tags","fromName":"Ivan Todoroski","fromEmail":"grnch_lists@gmx.net","sentAt":"2012-03-18T11:37:22Z","receivedAt":"2012-03-18T11:37:22Z","isPatch":false,"sender":{"key":"grnch_lists@gmx.net","avatar":null},"body":"Ivan Todoroski gmx.net> writes:\n> Now share this repo using the Smart HTTP transport (git-http-backend) and \nthen \n> try cloning it in a different directory. This is what you would get:\n> \n> $ git clone http://localhost/.../too-many-refs/.git\n> Cloning into 'too-many-refs'...\n> fatal: cannot exec 'fetch-pack': Argument list too long\n> \n> [...]\n> \n> The solution is conceptually simple: if the list of refs results in a too \nlong \n> command line, split the refs in batches and call fetch-pack multiple times \nsuch \n> that each call is under the cmdline limit:\n> \n> git fetch-pack --stateless-rpc --lock-pack ......\n> git fetch-pack --stateless-rpc --lock-pack ......\n> ...\n> git fetch-pack --stateless-rpc --lock-pack ......\n\n\nBTW, I didn't want to sound like I am expecting or demanding a fix. If the \nexperienced Git devs lack the time or inclination to work on this bug \n(understandable), I am certainly willing to try it myself. My C skills are a \nbit rusty and I'm not very familiar with the Git codebase, but I will do my \nbest to follow Documentation/SubmittingPatches as well as the existing code \nstructure.\n\nI will need a few pointers to get me started in the right direction though...\n\n\n1) Is splitting the cmdline in batches and executing fetch-pack multiple times \nthe right approach? If you have another solution please suggest.\n\n\n2) Should I add the test case for this bug to existing scripts like t/t5551-\nhttp-fetch.sh and t/t5561-http-backend.sh, or should I create a new test script \nunder t/ following their example? There will probably be only one test case for \nthis bug, basically the script I pasted in the original email to reproduce it.\n\n\n3) What would be the most portable way to get the cmdline length limit between \nPOSIX and Windows? Would something like this be acceptable:\n\n#ifder _WIN32\n\tint cmdline_limit = 32767;\n#else\n\tint cmdline_limit = sysconf(_SC_ARG_MAX);\n#endif\n\nI couldn't actually find a Windows API to get the cmdline limit, but this blog \npost by one of the Windows people tells the value:\n\nhttp://blogs.msdn.com/b/oldnewthing/archive/2003/12/10/56028.aspx\n\n\n4) Should this problem be fixed only in remote-curl.c:fetch_git() or should it \nbe solved more generally in run-command.c:start_command(), which is used by \nfetch_git() for the actual invocation?\n\nIf this is fixed only in remote-curl:fetch_git(), then the same logic would \nneed to be open coded in any other such place that might be found. Are you \naware of any other internal sub-commands that put all refs on the command line \nand could be susceptible to the same issue?\n\n\nIf it's fixed at a lower level in run-command.c:start_command(), the logic \nwould become available to any other sub-command that needs it.\n\nHowever, this would mean that struct child_process as well as struct rpc_state \nwould need an additional field that would tell whether the command is safe to \nexecute in multiple batches and how many of the arguments at the beginning of \nchild_process.argv must be preserved on every invocation (the switches and \nsuch).\n\nSomething like child_process.split_after, which if non-zero would mean that \nstart_command() is free to invoke the command multiple times when argv exceeds \nthe cmdline limit, by grouping any arguments after argv[split_after] in smaller \nbatches for each invocation.\n"},{"id":"187206","messageId":"CACsJy8DRLQiXE83vNu2J5_jUfYxa-StBVYwzn2eoraXUE2fEEg@mail.gmail.com","threadId":"29986","inReplyTo":"loom.20120318T121808-344@post.gmane.org","subject":"Re: Clone fails on a repo with too many heads/tags","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-03-18T12:04:18Z","receivedAt":"2012-03-18T12:04:18Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"I had a look but could not come up with a simple solution for this. CC\nShawn who knows smart-http really well.\n\nOn Sun, Mar 18, 2012 at 6:37 PM, Ivan Todoroski wrote:\n> Ivan Todoroski gmx.net> writes:\n>> Now share this repo using the Smart HTTP transport (git-http-backend) and\n> then\n>> try cloning it in a different directory. This is what you would get:\n>>\n>> $ git clone http://localhost/.../too-many-refs/.git\n>> Cloning into 'too-many-refs'...\n>> fatal: cannot exec 'fetch-pack': Argument list too long\n>>\n>> [...]\n>>\n>> The solution is conceptually simple: if the list of refs results in a too\n> long\n>> command line, split the refs in batches and call fetch-pack multiple times\n> such\n>> that each call is under the cmdline limit:\n>>\n>> git fetch-pack --stateless-rpc --lock-pack ......\n>> git fetch-pack --stateless-rpc --lock-pack ......\n>> ...\n>> git fetch-pack --stateless-rpc --lock-pack ......\n>\n>\n> BTW, I didn't want to sound like I am expecting or demanding a fix. If the\n> experienced Git devs lack the time or inclination to work on this bug\n> (understandable), I am certainly willing to try it myself. My C skills are a\n> bit rusty and I'm not very familiar with the Git codebase, but I will do my\n> best to follow Documentation/SubmittingPatches as well as the existing code\n> structure.\n>\n> I will need a few pointers to get me started in the right direction though...\n>\n>\n> 1) Is splitting the cmdline in batches and executing fetch-pack multiple times\n> the right approach? If you have another solution please suggest.\n>\n>\n> 2) Should I add the test case for this bug to existing scripts like t/t5551-\n> http-fetch.sh and t/t5561-http-backend.sh, or should I create a new test script\n> under t/ following their example? There will probably be only one test case for\n> this bug, basically the script I pasted in the original email to reproduce it.\n>\n>\n> 3) What would be the most portable way to get the cmdline length limit between\n> POSIX and Windows? Would something like this be acceptable:\n>\n> #ifder _WIN32\n>        int cmdline_limit = 32767;\n> #else\n>        int cmdline_limit = sysconf(_SC_ARG_MAX);\n> #endif\n>\n> I couldn't actually find a Windows API to get the cmdline limit, but this blog\n> post by one of the Windows people tells the value:\n>\n> http://blogs.msdn.com/b/oldnewthing/archive/2003/12/10/56028.aspx\n>\n>\n> 4) Should this problem be fixed only in remote-curl.c:fetch_git() or should it\n> be solved more generally in run-command.c:start_command(), which is used by\n> fetch_git() for the actual invocation?\n>\n> If this is fixed only in remote-curl:fetch_git(), then the same logic would\n> need to be open coded in any other such place that might be found. Are you\n> aware of any other internal sub-commands that put all refs on the command line\n> and could be susceptible to the same issue?\n>\n>\n> If it's fixed at a lower level in run-command.c:start_command(), the logic\n> would become available to any other sub-command that needs it.\n>\n> However, this would mean that struct child_process as well as struct rpc_state\n> would need an additional field that would tell whether the command is safe to\n> execute in multiple batches and how many of the arguments at the beginning of\n> child_process.argv must be preserved on every invocation (the switches and\n> such).\n>\n> Something like child_process.split_after, which if non-zero would mean that\n> start_command() is free to invoke the command multiple times when argv exceeds\n> the cmdline limit, by grouping any arguments after argv[split_after] in smaller\n> batches for each invocation.\n>\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n\n\n\n-- \nDuy\n"},{"id":"187209","messageId":"m3fwd550j3.fsf@localhost.localdomain","threadId":"29986","inReplyTo":"loom.20120318T083216-96@post.gmane.org","subject":"Re: Clone fails on a repo with too many heads/tags","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-03-18T16:36:24Z","receivedAt":"2012-03-18T16:36:24Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Ivan Todoroski writes:\n\n> I recently tried cloning a fresh copy of a large repo (converted from CVS, \n> nearly 10 years of history) and to my surprise \"git clone\" failed with the \n> following message:\n> \n> error: cannot spawn git: No such file or directory\n> \n> The problem is only reproduced using the Smart HTTP transport.\n\n[...]\n> I tracked down the problem to remote-curl.c:fetch_git(). That's where the \n> \"fetch-pack\" command line is being constructed with all the refs on one line:\n> \n> git fetch-pack --stateless-rpc --lock-pack ......\n> \n> The solution is conceptually simple: if the list of refs results in a too long \n> command line, split the refs in batches and call fetch-pack multiple times such \n> that each call is under the cmdline limit:\n> \n> git fetch-pack --stateless-rpc --lock-pack ......\n> git fetch-pack --stateless-rpc --lock-pack ......\n> ...\n> git fetch-pack --stateless-rpc --lock-pack ......\n\nThat, or implement --stdin / --revs in git-fetch-pach (perhaps\nfollowing git-pack-objects that implements --revs).\n\n-- \nJakub Narebski\n"},{"id":"187214","messageId":"20120318190659.GA24829@sigill.intra.peff.net","threadId":"29986","inReplyTo":"m3fwd550j3.fsf@localhost.localdomain","subject":"Re: Clone fails on a repo with too many heads/tags","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-18T19:07:00Z","receivedAt":"2012-03-18T19:07:00Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 18, 2012 at 09:36:24AM -0700, Jakub Narebski wrote:\n\n> > The solution is conceptually simple: if the list of refs results in a too long \n> > command line, split the refs in batches and call fetch-pack multiple times such \n> > that each call is under the cmdline limit:\n> > \n> > git fetch-pack --stateless-rpc --lock-pack ......\n> > git fetch-pack --stateless-rpc --lock-pack ......\n> > ...\n> > git fetch-pack --stateless-rpc --lock-pack ......\n> \n> That, or implement --stdin / --revs in git-fetch-pach (perhaps\n> following git-pack-objects that implements --revs).\n\nI don't think that will work, as stateless-rpc fetch-pack already uses\nstdin to receive the list of advertised refs from the remote. Nor would\nyou want to have multiple invocations of fetch-pack, since that would\nmean multiple http requests and multiple pack responses (which could not\ndelta between themselves).\n\nAnd you can't condense the list in the general case. It is the set of\nrefs that we actually want to fetch. We could try passing just the\noriginal refspecs (not the expansion) and letting fetch-pack try to do\nthe expansion, but in the worst case, you might really just have a\ngigantic list of refs.\n\nI think the only sane solution is to write the values to a temporary\nfile, and do something like:\n\n git fetch-pack --stateless-rpc --refs-from=$tmpfile\n\nEven if you put the tmpfile in $GIT_DIR, I don't think this should run\nafoul of any read-only repositories, since by definition you are\nfetching into the repository (but you could also just put it in /tmp).\n\n-Peff\n"},{"id":"187217","messageId":"201203182307.43599.jnareb@gmail.com","threadId":"29986","inReplyTo":"20120318190659.GA24829@sigill.intra.peff.net","subject":"Re: Clone fails on a repo with too many heads/tags","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-03-18T22:07:42Z","receivedAt":"2012-03-18T22:07:42Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Jeff King wrote:\n[...]\n> I think the only sane solution is to write the values to a temporary\n> file, and do something like:\n> \n>   git fetch-pack --stateless-rpc --refs-from=$tmpfile\n> \n> Even if you put the tmpfile in $GIT_DIR, I don't think this should run\n> afoul of any read-only repositories, since by definition you are\n> fetching into the repository (but you could also just put it in /tmp).\n\nOr\n\n git fetch-pack --stateless-rpc --refs-fd=$n\n\nand there would be no need for temporary file.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"187229","messageId":"4F668652.5090705@gmx.net","threadId":"29986","inReplyTo":"20120318190659.GA24829@sigill.intra.peff.net","subject":"Re: Clone fails on a repo with too many heads/tags","fromName":"Ivan Todoroski","fromEmail":"grnch_lists@gmx.net","sentAt":"2012-03-19T01:05:22Z","receivedAt":"2012-03-19T01:05:22Z","isPatch":false,"sender":{"key":"grnch_lists@gmx.net","avatar":null},"body":"On 18.03.2012 20:07, Jeff King wrote:\n> Nor would you want to have multiple invocations of fetch-pack, since\n> that would mean multiple http requests and multiple pack responses\n> (which could not delta between themselves).\n\nSure, but if fetch-pack is called with so many refs that it overflows \nthe command line, it's looking at a lot of work ahead of it anyway, it's \nnot going to be a fast operation. So the overhead of multiple HTTP \nrequests and lack of delta compression between huge batches of refs \nwouldn't be all that significant in practice.\n\nThat said, I really like your temp file idea below. It's the best \nsolution so far. Simple, efficient, non-intrusive.\n\n\n> I think the only sane solution is to write the values to a temporary \n> file, and do something like:\n> \n> git fetch-pack --stateless-rpc --refs-from=$tmpfile\n> \n> Even if you put the tmpfile in $GIT_DIR, I don't think this should\n> run afoul of any read-only repositories, since by definition you are \n> fetching into the repository (but you could also just put it in\n> /tmp).\n\nOK, if nobody beats me to it I will try to code this next weekend. Maybe \nsooner, but I don't usually have much free time over the week (day job, \nfamily, etc.).\n"},{"id":"187232","messageId":"CACsJy8BNT-dY+wDONY_TgLnv0135RZ-47BEVMzX6c3ddH=83Zw@mail.gmail.com","threadId":"29986","inReplyTo":"20120318190659.GA24829@sigill.intra.peff.net","subject":"Re: Clone fails on a repo with too many heads/tags","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-03-19T01:30:38Z","receivedAt":"2012-03-19T01:30:38Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Mar 19, 2012 at 2:07 AM, Jeff King wrote:\n> I don't think that will work, as stateless-rpc fetch-pack already uses\n> stdin to receive the list of advertised refs from the remote. Nor would\n> you want to have multiple invocations of fetch-pack, since that would\n> mean multiple http requests and multiple pack responses (which could not\n> delta between themselves).\n\nremote-curl functions as middle man between http client and\nfetch-pack. Can we just send ref list over fetch-pack.stdin first,\nfollowed by maybe empty line, then normal stuff that remote-curl feeds\nfetch-pack?\n-- \nDuy\n"},{"id":"187234","messageId":"20120319023231.GA10426@sigill.intra.peff.net","threadId":"29986","inReplyTo":"201203182307.43599.jnareb@gmail.com","subject":"Re: Clone fails on a repo with too many heads/tags","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-19T02:32:31Z","receivedAt":"2012-03-19T02:32:31Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 18, 2012 at 11:07:42PM +0100, Jakub Narebski wrote:\n\n> > I think the only sane solution is to write the values to a temporary\n> > file, and do something like:\n> > \n> >   git fetch-pack --stateless-rpc --refs-from=$tmpfile\n> > \n> > Even if you put the tmpfile in $GIT_DIR, I don't think this should run\n> > afoul of any read-only repositories, since by definition you are\n> > fetching into the repository (but you could also just put it in /tmp).\n> \n> git fetch-pack --stateless-rpc --refs-fd=$n\n> \n> and there would be no need for temporary file.\n\nYeah. That is a slightly more awkward interface for command-line users,\nbut this is really meant to be an internal-to-git thing (just like\nstateless-rpc is in the first place). And avoiding a tempfile is a good\nthing.\n\nHowever, I think you would need to teach the run-command API how to open\nextra pipes to a child beyond stdout/stdin/stderr.\n\n-Peff\n"},{"id":"187235","messageId":"CACsJy8DsuQNEwLb_vSPTN_Zrh6o9OK3V_R3xZDMQsFCaOafUXw@mail.gmail.com","threadId":"29986","inReplyTo":"20120319023231.GA10426@sigill.intra.peff.net","subject":"Re: Clone fails on a repo with too many heads/tags","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-03-19T02:43:05Z","receivedAt":"2012-03-19T02:43:05Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Mar 19, 2012 at 9:32 AM, Jeff King wrote:\n> However, I think you would need to teach the run-command API how to open\n> extra pipes to a child beyond stdout/stdin/stderr.\n\nThat might be a problem on Windows. I think Windows exec* has special\nsupport for stdin/stdout/stderr and only those.\n-- \nDuy\n"},{"id":"187236","messageId":"20120319024436.GB10426@sigill.intra.peff.net","threadId":"29986","inReplyTo":"CACsJy8BNT-dY+wDONY_TgLnv0135RZ-47BEVMzX6c3ddH=83Zw@mail.gmail.com","subject":"Re: Clone fails on a repo with too many heads/tags","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-19T02:44:36Z","receivedAt":"2012-03-19T02:44:36Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 19, 2012 at 08:30:38AM +0700, Nguyen Thai Ngoc Duy wrote:\n\n> On Mon, Mar 19, 2012 at 2:07 AM, Jeff King wrote:\n> > I don't think that will work, as stateless-rpc fetch-pack already uses\n> > stdin to receive the list of advertised refs from the remote. Nor would\n> > you want to have multiple invocations of fetch-pack, since that would\n> > mean multiple http requests and multiple pack responses (which could not\n> > delta between themselves).\n> \n> remote-curl functions as middle man between http client and\n> fetch-pack. Can we just send ref list over fetch-pack.stdin first,\n> followed by maybe empty line, then normal stuff that remote-curl feeds\n> fetch-pack?\n\nYes, I think that would work. You'd just have to take care to pass the\nresidual buffer (i.e., what is left in your input buffer after you\nnotice that reading the list of wanted refs is finished) along to\nthe git-protocol code. So it would require a little refactoring of\nget_remote_heads, I think.\n\n-Peff\n"},{"id":"187237","messageId":"20120319024551.GC10426@sigill.intra.peff.net","threadId":"29986","inReplyTo":"CACsJy8DsuQNEwLb_vSPTN_Zrh6o9OK3V_R3xZDMQsFCaOafUXw@mail.gmail.com","subject":"Re: Clone fails on a repo with too many heads/tags","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-19T02:45:51Z","receivedAt":"2012-03-19T02:45:51Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 19, 2012 at 09:43:05AM +0700, Nguyen Thai Ngoc Duy wrote:\n\n> On Mon, Mar 19, 2012 at 9:32 AM, Jeff King wrote:\n> > However, I think you would need to teach the run-command API how to open\n> > extra pipes to a child beyond stdout/stdin/stderr.\n> \n> That might be a problem on Windows. I think Windows exec* has special\n> support for stdin/stdout/stderr and only those.\n\nThen your sneak-it-across-stdin trick is probably the best bet. It's a\nlittle ugly, but this is for stateless-rpc, so it's not like anything\noutside of git is going to actually see this.\n\n-Peff\n"},{"id":"187384","messageId":"4F69B5F0.2060605@gmx.net","threadId":"29986","inReplyTo":"20120319024436.GB10426@sigill.intra.peff.net","subject":"Re: Clone fails on a repo with too many heads/tags","fromName":"Ivan Todoroski","fromEmail":"grnch_lists@gmx.net","sentAt":"2012-03-21T11:05:20Z","receivedAt":"2012-03-21T11:05:20Z","isPatch":false,"sender":{"key":"grnch_lists@gmx.net","avatar":null},"body":"On 19.03.2012 03:44, Jeff King wrote:\n> On Mon, Mar 19, 2012 at 08:30:38AM +0700, Nguyen Thai Ngoc Duy wrote:\n> \n>> On Mon, Mar 19, 2012 at 2:07 AM, Jeff King wrote:\n>>> I don't think that will work, as stateless-rpc fetch-pack already uses\n>>> stdin to receive the list of advertised refs from the remote. Nor would\n>>> you want to have multiple invocations of fetch-pack, since that would\n>>> mean multiple http requests and multiple pack responses (which could not\n>>> delta between themselves).\n>> remote-curl functions as middle man between http client and\n>> fetch-pack. Can we just send ref list over fetch-pack.stdin first,\n>> followed by maybe empty line, then normal stuff that remote-curl feeds\n>> fetch-pack?\n> \n> Yes, I think that would work. You'd just have to take care to pass the\n> residual buffer (i.e., what is left in your input buffer after you\n> notice that reading the list of wanted refs is finished) along to\n> the git-protocol code. So it would require a little refactoring of\n> get_remote_heads, I think.\n\nWould it be OK for fetch-pack.c to use the packetized format \n(pkt-line.h) for reading the list of refs from stdin?\n\nThis way we can read() the exact number of bytes needed for the refs \nfrom the fd and there would be no need to refactor get_remote_heads() to \npass a residual buffer, it could just continue reading straight from the \nsame fd.\n"},{"id":"187397","messageId":"CAJo=hJu0H5wfXB_y5XQ6=S0VJ9t4pxHWkuy_=rehJL_6psf00g@mail.gmail.com","threadId":"29986","inReplyTo":"4F69B5F0.2060605@gmx.net","subject":"Re: Clone fails on a repo with too many heads/tags","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2012-03-21T14:28:24Z","receivedAt":"2012-03-21T14:28:24Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Wed, Mar 21, 2012 at 04:05, Ivan Todoroski wrote:\n> On 19.03.2012 03:44, Jeff King wrote:\n>> Yes, I think that would work. You'd just have to take care to pass the\n>> residual buffer (i.e., what is left in your input buffer after you\n>> notice that reading the list of wanted refs is finished) along to\n>> the git-protocol code.  So it would require a little refactoring of\n>> get_remote_heads, I think.\n>\n>\n> Would it be OK for fetch-pack.c to use the packetized format (pkt-line.h)\n> for reading the list of refs from stdin?\n\nThis is probably the easiest way to implement the sneak-into-stdin\npatch. Use a pkt-line for each argument that should have been in the\nargv array from the command line, and a flush pkt to terminate the\nlist.\n"},{"id":"187407","messageId":"20120321171423.GA13140@sigill.intra.peff.net","threadId":"29986","inReplyTo":"CAJo=hJu0H5wfXB_y5XQ6=S0VJ9t4pxHWkuy_=rehJL_6psf00g@mail.gmail.com","subject":"Re: Clone fails on a repo with too many heads/tags","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-21T17:14:23Z","receivedAt":"2012-03-21T17:14:23Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 21, 2012 at 07:28:24AM -0700, Shawn O. Pearce wrote:\n\n> > Would it be OK for fetch-pack.c to use the packetized format (pkt-line.h)\n> > for reading the list of refs from stdin?\n> \n> This is probably the easiest way to implement the sneak-into-stdin\n> patch. Use a pkt-line for each argument that should have been in the\n> argv array from the command line, and a flush pkt to terminate the\n> list.\n\nSomething in me feels slightly uncomfortable with that, just because\nsimple newline-delimited formats make it easy for people to hack on the\ntool and feed input from unexpected sources.\n\nBut stateless-rpc is already a pretty deep internal interface anyway,\nand the format is already weird (the second half is already packetized\ninput from a remote anyway). So it's probably not worth caring about\nhackability here.\n\n-Peff\n"},{"id":"187410","messageId":"201203211859.26923.jnareb@gmail.com","threadId":"29986","inReplyTo":"20120321171423.GA13140@sigill.intra.peff.net","subject":"Re: Clone fails on a repo with too many heads/tags","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-03-21T17:59:26Z","receivedAt":"2012-03-21T17:59:26Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Jeff King wrote:\n> On Wed, Mar 21, 2012 at 07:28:24AM -0700, Shawn O. Pearce wrote:\n> \n> > > Would it be OK for fetch-pack.c to use the packetized format (pkt-line.h)\n> > > for reading the list of refs from stdin?\n> > \n> > This is probably the easiest way to implement the sneak-into-stdin\n> > patch. Use a pkt-line for each argument that should have been in the\n> > argv array from the command line, and a flush pkt to terminate the\n> > list.\n> \n> Something in me feels slightly uncomfortable with that, just because\n> simple newline-delimited formats make it easy for people to hack on the\n> tool and feed input from unexpected sources.\n\nWell, give people \"pkt-line\" command line tool which would transform\nordinary textual output on input into pkt-line (which is almost pure\ntext anyway) output and vice versa.\n\n> But stateless-rpc is already a pretty deep internal interface anyway,\n> and the format is already weird (the second half is already packetized\n> input from a remote anyway). So it's probably not worth caring about\n> hackability here.\n\nRight.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"187425","messageId":"4F6A33C5.2080909@gmx.net","threadId":"29986","inReplyTo":"20120321171423.GA13140@sigill.intra.peff.net","subject":"Re: Clone fails on a repo with too many heads/tags","fromName":"Ivan Todoroski","fromEmail":"grnch_lists@gmx.net","sentAt":"2012-03-21T20:02:13Z","receivedAt":"2012-03-21T20:02:13Z","isPatch":false,"sender":{"key":"grnch_lists@gmx.net","avatar":null},"body":"On 21.03.2012 18:14, Jeff King wrote:\n> On Wed, Mar 21, 2012 at 07:28:24AM -0700, Shawn O. Pearce wrote:\n> \n>>> Would it be OK for fetch-pack.c to use the packetized format (pkt-line.h)\n>>> for reading the list of refs from stdin?\n>> This is probably the easiest way to implement the sneak-into-stdin\n>> patch. Use a pkt-line for each argument that should have been in the\n>> argv array from the command line, and a flush pkt to terminate the\n>> list.\n> \n> Something in me feels slightly uncomfortable with that, just because\n> simple newline-delimited formats make it easy for people to hack on the\n> tool and feed input from unexpected sources.\n\nI understand what you mean. How about this:\n\nIf both --stdin and --stateless-rpc are specified to fetch-pack, it will \nuse pkt-line to read the refs from stdin before handing off stdin to \nget_remote_heads().\n\nHowever, if only --stdin is specified, it will read refs from stdin in a \nscript-friendly newline delimited format, one ref per line. This is okay \nbecause when --stateless-rpc is not specified get_remote_heads() reads \nfrom an fd different from stdin so there is no issue with residual \nbuffers in this case.\n\nThis way you preserve scriptability for any other callers who don't use \n--stateless-rpc.\n\nHow does this sound?\n"},{"id":"187429","messageId":"20120321201722.GA15021@sigill.intra.peff.net","threadId":"29986","inReplyTo":"4F6A33C5.2080909@gmx.net","subject":"Re: Clone fails on a repo with too many heads/tags","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-21T20:17:23Z","receivedAt":"2012-03-21T20:17:23Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 21, 2012 at 09:02:13PM +0100, Ivan Todoroski wrote:\n\n> >Something in me feels slightly uncomfortable with that, just because\n> >simple newline-delimited formats make it easy for people to hack on the\n> >tool and feed input from unexpected sources.\n> \n> I understand what you mean. How about this:\n> \n> If both --stdin and --stateless-rpc are specified to fetch-pack, it\n> will use pkt-line to read the refs from stdin before handing off\n> stdin to get_remote_heads().\n> \n> However, if only --stdin is specified, it will read refs from stdin\n> in a script-friendly newline delimited format, one ref per line. This\n> is okay because when --stateless-rpc is not specified\n> get_remote_heads() reads from an fd different from stdin so there is\n> no issue with residual buffers in this case.\n> \n> This way you preserve scriptability for any other callers who don't\n> use --stateless-rpc.\n> \n> How does this sound?\n\nI think that sounds quite reasonable, and shouldn't be more than a few\nextra lines to implement.\n\n-Peff\n"},{"id":"187655","messageId":"4F6E3373.7090500@gmx.net","threadId":"29986","inReplyTo":"20120321201722.GA15021@sigill.intra.peff.net","subject":"Re: Clone fails on a repo with too many heads/tags","fromName":"Ivan Todoroski","fromEmail":"grnch_lists@gmx.net","sentAt":"2012-03-24T20:49:55Z","receivedAt":"2012-03-24T20:49:55Z","isPatch":false,"sender":{"key":"grnch_lists@gmx.net","avatar":null},"body":"On 21.03.2012 21:17, Jeff King wrote:\n> On Wed, Mar 21, 2012 at 09:02:13PM +0100, Ivan Todoroski wrote:\n> \n>>> Something in me feels slightly uncomfortable with that, just because\n>>> simple newline-delimited formats make it easy for people to hack on the\n>>> tool and feed input from unexpected sources.\n>> I understand what you mean. How about this:\n>>\n>> If both --stdin and --stateless-rpc are specified to fetch-pack, it\n>> will use pkt-line to read the refs from stdin before handing off\n>> stdin to get_remote_heads().\n>>\n>> However, if only --stdin is specified, it will read refs from stdin\n>> in a script-friendly newline delimited format, one ref per line. This\n>> is okay because when --stateless-rpc is not specified\n>> get_remote_heads() reads from an fd different from stdin so there is\n>> no issue with residual buffers in this case.\n>>\n>> This way you preserve scriptability for any other callers who don't\n>> use --stateless-rpc.\n>>\n>> How does this sound?\n> \n> I think that sounds quite reasonable, and shouldn't be more than a few\n> extra lines to implement.\n> \n> -Peff\n\n\nI wrote the code for this and went to write the test cases, but the test \nsuite is not cooperating. :(\n\nI'll send just the code changes for comments while I'm figuring out the \ntest suite. I'll include the test cases along with better commit \nmessages in the next version of the patches.\n\nSo, back to the test suite problem. To get familiar with it I first ran \nthe test suite against the vanilla \"maint\" branch without any of my \nchanges, just to see what I should expect on properly running code.\n\nUnfortunately it failed, and to make matters worse it failed exactly on \nthe parts dealing with smart HTTP, which is what I need to test in the \nfirst place. Talk about Murphy's law...\n\nIs it failing for anyone else on the vanilla \"maint\" branch? I would \nappreciate any help I could get here.\n\nMy machine is CentOS 5.8 with latest updates and I have the httpd \npackage installed. Here is the command that is failing (you can find the \nfull output in the attachment):\n\n\n$ GIT_TEST_HTTPD=yes ./t5551-http-fetch.sh -i -v\n[... skip successful tests ...]\nok 5 - used upload-pack service\n\nexpecting success:\n git clone $HTTPD_URL/smart-redir-perm/repo.git --quiet repo-p\n\nerror: RPC failed; result=22, HTTP code = 405\nfatal: The remote end hung up unexpectedly\nnot ok - 6 follow redirects (301)\n#\n# git clone $HTTPD_URL/smart-redir-perm/repo.git --quiet \nrepo-p\n#\n\n\n\nInitialized empty Git repository in /home/itodoroski/git/t/trash directory.t5551-http-fetch/.git/\nexpecting success: \n\techo content >file &&\n\tgit add file &&\n\tgit commit -m one\n\n[master (root-commit) ba36540] one\n Author: A U Thor \n 1 file changed, 1 insertion(+)\n create mode 100644 file\nok 1 - setup repository\n\nexpecting success: \n\tmkdir \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n\t(cd \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n\t git --bare init\n\t) &&\n\tgit remote add public \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n\tgit push public master:master\n\nInitialized empty Git repository in /home/itodoroski/git/t/trash directory.t5551-http-fetch/httpd/www/repo.git/\nTo /home/itodoroski/git/t/trash directory.t5551-http-fetch/httpd/www/repo.git\n * [new branch] master -> master\nok 2 - create http-accessible bare repository\n\nexpecting success: \n\tGIT_CURL_VERBOSE=1 git clone --quiet $HTTPD_URL/smart/repo.git clone 2>err &&\n\ttest_cmp file clone/file &&\n\ttr '\\015' Q <]/{\n\t\t\ts/^/> /\n\t\t}\n\n\t\t/^> User-Agent: /d\n\t\t/^> Host: /d\n\t\t/^> POST /,$ {\n\t\t\t/^> Accept: [*]\\\\/[*]/d\n\t\t}\n\t\ts/^> Content-Length: .*/> Content-Length: xxx/\n\t\t/^> 00..want /d\n\t\t/^> 00.*done/d\n\n\t\t/^< Server: /d\n\t\t/^< Expires: /d\n\t\t/^< Date: /d\n\t\t/^< Content-Length: /d\n\t\t/^< Transfer-Encoding: /d\n\t\" >act &&\n\ttest_cmp exp act\n\nok 3 - clone http repository\n\nexpecting success: \n\techo content >>file &&\n\tgit commit -a -m two &&\n\tgit push public\n\t(cd clone && git pull) &&\n\ttest_cmp file clone/file\n\n[master ace4881] two\n Author: A U Thor \n 1 file changed, 1 insertion(+)\nTo /home/itodoroski/git/t/trash directory.t5551-http-fetch/httpd/www/repo.git\n ba36540..ace4881 master -> master\nFrom http://127.0.0.1:5551/smart/repo\n ba36540..ace4881 master -> origin/master\nUpdating ba36540..ace4881\nFast-forward\n file | 1 +\n 1 file changed, 1 insertion(+)\nok 4 - fetch changes via http\n\nexpecting success: \n\tsed -e \"\n\t\ts/^.* \\\"//\n\t\ts/\\\"//\n\t\ts/ [1-9][0-9]*\\$//\n\t\ts/^GET /GET /\n\t\" >act <\"$HTTPD_ROOT_PATH\"/access.log &&\n\ttest_cmp exp act\n\nok 5 - used upload-pack service\n\nexpecting success: \n\tgit clone $HTTPD_URL/smart-redir-perm/repo.git --quiet repo-p\n\nerror: RPC failed; result=22, HTTP code = 405\nfatal: The remote end hung up unexpectedly\nnot ok - 6 follow redirects (301)\n#\t\n#\t\tgit clone $HTTPD_URL/smart-redir-perm/repo.git --quiet repo-p\n#\t\n"},{"id":"187656","messageId":"4F6E3446.9080001@gmx.net","threadId":"29986","inReplyTo":"20120321201722.GA15021@sigill.intra.peff.net","subject":"[PATCH/RFC 1/2] fetch-pack: new option to read refs from stdin","fromName":"Ivan Todoroski","fromEmail":"grnch_lists@gmx.net","sentAt":"2012-03-24T20:53:26Z","receivedAt":"2012-03-24T20:53:26Z","isPatch":true,"sender":{"key":"grnch_lists@gmx.net","avatar":null},"body":"\n>From c4bb55f9f27569faa368d823ca6fe4b236e37cd6 Mon Sep 17 00:00:00 2001\nFrom: Ivan Todoroski \nDate: Sat, 24 Mar 2012 15:13:05 +0100\nSubject: [PATCH/RFC 1/2] fetch-pack: new option to read refs from stdin\n\n---\n Documentation/git-fetch-pack.txt | 9 ++++++++\n builtin/fetch-pack.c | 44 ++++++++++++++++++++++++++++++++++++--\n fetch-pack.h | 3 ++-\n 3 files changed, 53 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-fetch-pack.txt b/Documentation/git-fetch-pack.txt\nindex ed1bdaacd1..7cd7c785bc 100644\n--- a/Documentation/git-fetch-pack.txt\n+++ b/Documentation/git-fetch-pack.txt\n@@ -32,6 +32,15 @@ OPTIONS\n --all::\n \tFetch all remote refs.\n \n+--stdin::\n+\tTake the list of refs from stdin, one per line. If there\n+\tare refs specified on the command line in addition to this\n+\toption, then the refs from stdin are processed after those\n+\ton the command line.\n+\tIf '--stateless-rpc' is specified together with this option\n+\tthen the list of refs must be in packet format (pkt-line)\n+\twith a flush packet terminating the list.\n+\n -q::\n --quiet::\n \tPass '-q' flag to 'git unpack-objects'; this makes the\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex a4d3e90a86..3c4c193e45 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -23,7 +23,7 @@ static struct fetch_pack_args args = {\n };\n \n static const char fetch_pack_usage[] =\n-\"git fetch-pack [--all] [--quiet|-q] [--keep|-k] [--thin] [--include-tag] [--upload-pack=] [--depth=] [--no-progress] [-v] [:] [...]\";\n+\"git fetch-pack [--all] [--stdin] [--quiet|-q] [--keep|-k] [--thin] [--include-tag] [--upload-pack=] [--depth=] [--no-progress] [-v] [:] [...]\";\n \n #define COMPLETE\t(1U << 0)\n #define COMMON\t\t(1U << 1)\n@@ -896,7 +896,7 @@ static void fetch_pack_setup(void)\n \n int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n {\n-\tint i, ret, nr_heads;\n+\tint i, ret, nr_heads, alloc_heads;\n \tstruct ref *ref = NULL;\n \tchar *dest = NULL, **heads;\n \tint fd[2];\n@@ -941,6 +941,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t\t\t\targs.fetch_all = 1;\n \t\t\t\tcontinue;\n \t\t\t}\n+\t\t\tif (!strcmp(\"--stdin\", arg)) {\n+\t\t\t\targs.refs_from_stdin = 1;\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t\tif (!strcmp(\"-v\", arg)) {\n \t\t\t\targs.verbose = 1;\n \t\t\t\tcontinue;\n@@ -972,6 +976,42 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \tif (!dest)\n \t\tusage(fetch_pack_usage);\n \n+\tif (args.refs_from_stdin) {\n+\t\tchar ref[1000];\n+\t\t/* copy refs from cmdline to new growable list */\n+\t\tint size = nr_heads * sizeof(*heads);\n+\t\theads = memcpy(xmalloc(size), heads, size);\n+\t\talloc_heads = nr_heads;\n+\t\t/* append refs from stdin to ones from cmdline */\n+\t\tfor (;;) {\n+\t\t\tif (args.stateless_rpc) {\n+\t\t\t\t/* read using pkt-line in stateless RPC mode,\n+\t\t\t\t flush packet signifies end of refs */\n+\t\t\t\tint n = packet_read_line(0, ref, sizeof(ref));\n+\t\t\t\tif (!n)\n+\t\t\t\t\tbreak;\n+\t\t\t\tif (ref[n-1] == '\\n')\n+\t\t\t\t\tref[n-1] = '\\0';\n+\t\t\t}\n+\t\t\telse {\n+\t\t\t\tint n;\n+\t\t\t\tif (!fgets(ref, sizeof(ref), stdin)) {\n+\t\t\t\t\tif (ferror(stdin))\n+\t\t\t\t\t\tdie_errno(\"cannot read ref\");\n+\t\t\t\t\tif (feof(stdin))\n+\t\t\t\t\t\tbreak;\n+\t\t\t\t}\n+\t\t\t\tn = strlen(ref);\n+\t\t\t\tif (ref[n-1] == '\\n')\n+\t\t\t\t\tref[n-1] = '\\0';\n+\t\t\t\telse if (!feof(stdin))\n+\t\t\t\t\tdie(\"ref too long on stdin\");\n+\t\t\t}\n+\t\t\tALLOC_GROW(heads, nr_heads + 1, alloc_heads);\n+\t\t\theads[nr_heads++] = xstrdup(ref);\n+\t\t}\n+\t}\n+\n \tif (args.stateless_rpc) {\n \t\tconn = NULL;\n \t\tfd[0] = 0;\ndiff --git a/fetch-pack.h b/fetch-pack.h\nindex 0608edae3f..292d69389e 100644\n--- a/fetch-pack.h\n+++ b/fetch-pack.h\n@@ -13,7 +13,8 @@ struct fetch_pack_args {\n \t\tverbose:1,\n \t\tno_progress:1,\n \t\tinclude_tag:1,\n-\t\tstateless_rpc:1;\n+\t\tstateless_rpc:1,\n+\t\trefs_from_stdin:1;\n };\n \n struct ref *fetch_pack(struct fetch_pack_args *args,\n-- \n1.7.9.4.16.gd24fa.dirty\n"},{"id":"187657","messageId":"4F6E3478.3090505@gmx.net","threadId":"29986","inReplyTo":"20120321201722.GA15021@sigill.intra.peff.net","subject":"[PATCH/RFC 2/2] remote-curl: send the refs to fetch-pack on stdin","fromName":"Ivan Todoroski","fromEmail":"grnch_lists@gmx.net","sentAt":"2012-03-24T20:54:16Z","receivedAt":"2012-03-24T20:54:16Z","isPatch":true,"sender":{"key":"grnch_lists@gmx.net","avatar":null},"body":"\n>From 723a561946824ee367f57f0d9b9d336a6bc28d13 Mon Sep 17 00:00:00 2001\nFrom: Ivan Todoroski \nDate: Sat, 24 Mar 2012 15:53:36 +0100\nSubject: [PATCH/RFC 2/2] remote-curl: send the refs to fetch-pack on stdin\n\n---\n remote-curl.c | 18 +++++++++++++-----\n 1 file changed, 13 insertions(+), 5 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex d159fe7f34..9fdfca9f32 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -531,12 +531,13 @@ static int post_rpc(struct rpc_state *rpc)\n \treturn err;\n }\n \n-static int rpc_service(struct rpc_state *rpc, struct discovery *heads)\n+static int rpc_service(struct rpc_state *rpc, struct discovery *heads,\n+\t\tint nr_fetch, struct ref **to_fetch)\n {\n \tconst char *svc = rpc->service_name;\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct child_process client;\n-\tint err = 0;\n+\tint err = 0, i;\n \n \tmemset(&client, 0, sizeof(client));\n \tclient.in = -1;\n@@ -545,6 +546,13 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads)\n \tclient.argv = rpc->argv;\n \tif (start_command(&client))\n \t\texit(1);\n+\n+\t/* push the refs to fetch-pack via stdin, if requested */\n+\tif (to_fetch) {\n+\t\tfor (i = 0; i < nr_fetch; i++)\n+\t\t\tpacket_write(client.in, \"%s\\n\", to_fetch[i]->name);\n+\t\tpacket_flush(client.in);\n+\t}\n \tif (heads)\n \t\twrite_or_die(client.in, heads->buf, heads->len);\n \n@@ -633,6 +641,7 @@ static int fetch_git(struct discovery *heads,\n \targv = xmalloc((15 + nr_heads) * sizeof(char*));\n \targv[argc++] = \"fetch-pack\";\n \targv[argc++] = \"--stateless-rpc\";\n+\targv[argc++] = \"--stdin\";\n \targv[argc++] = \"--lock-pack\";\n \tif (options.followtags)\n \t\targv[argc++] = \"--include-tag\";\n@@ -655,7 +664,6 @@ static int fetch_git(struct discovery *heads,\n \t\tstruct ref *ref = to_fetch[i];\n \t\tif (!ref->name || !*ref->name)\n \t\t\tdie(\"cannot fetch by sha1 over smart http\");\n-\t\targv[argc++] = ref->name;\n \t}\n \targv[argc++] = NULL;\n \n@@ -664,7 +672,7 @@ static int fetch_git(struct discovery *heads,\n \trpc.argv = argv;\n \trpc.gzip_request = 1;\n \n-\terr = rpc_service(&rpc, heads);\n+\terr = rpc_service(&rpc, heads, nr_heads, to_fetch);\n \tif (rpc.result.len)\n \t\tsafe_write(1, rpc.result.buf, rpc.result.len);\n \tstrbuf_release(&rpc.result);\n@@ -783,7 +791,7 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)\n \trpc.service_name = \"git-receive-pack\",\n \trpc.argv = argv;\n \n-\terr = rpc_service(&rpc, heads);\n+\terr = rpc_service(&rpc, heads, 0, NULL);\n \tif (rpc.result.len)\n \t\tsafe_write(1, rpc.result.buf, rpc.result.len);\n \tstrbuf_release(&rpc.result);\n-- \n1.7.9.4.16.gd24fa.dirty\n"},{"id":"187662","messageId":"20120325010609.GB27651@sigill.intra.peff.net","threadId":"29986","inReplyTo":"4F6E3373.7090500@gmx.net","subject":"Re: Clone fails on a repo with too many heads/tags","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-25T01:06:09Z","receivedAt":"2012-03-25T01:06:09Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 24, 2012 at 09:49:55PM +0100, Ivan Todoroski wrote:\n\n> Unfortunately it failed, and to make matters worse it failed exactly\n> on the parts dealing with smart HTTP, which is what I need to test in\n> the first place. Talk about Murphy's law...\n> \n> Is it failing for anyone else on the vanilla \"maint\" branch? I would\n> appreciate any help I could get here.\n\nNo, it passes fine here (Debian unstable).\n\n> expecting success:\n> git clone $HTTPD_URL/smart-redir-perm/repo.git --quiet repo-p\n> \n> error: RPC failed; result=22, HTTP code = 405\n> fatal: The remote end hung up unexpectedly\n> not ok - 6 follow redirects (301)\n\nThat's weird. 405 is \"Method Not Allowed\". Clone shouldn't be doing\nanything more exotic than GET and POST. And the prior tests pass, so it\nmeans that it's working in general. The only thing different about this\ntest is that apache is configured to use mod_rewrite to issue a\nredirect.\n\nDoes your machine have mod_rewrite installed and enabled? I would think\napache would complain at startup if it wasn't. I wonder if there's\nsomething non-portable in the minimal apache config we ship.\n\nDoes httpd/error.log in the trash directory say anything interesting?\n\n-Peff\n"},{"id":"187663","messageId":"20120325011948.GC27651@sigill.intra.peff.net","threadId":"29986","inReplyTo":"4F6E3446.9080001@gmx.net","subject":"Re: [PATCH/RFC 1/2] fetch-pack: new option to read refs from stdin","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-25T01:19:48Z","receivedAt":"2012-03-25T01:19:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 24, 2012 at 09:53:26PM +0100, Ivan Todoroski wrote:\n\n> From c4bb55f9f27569faa368d823ca6fe4b236e37cd6 Mon Sep 17 00:00:00 2001\n> From: Ivan Todoroski \n> Date: Sat, 24 Mar 2012 15:13:05 +0100\n> Subject: [PATCH/RFC 1/2] fetch-pack: new option to read refs from stdin\n\nYou can drop these lines; they are redundant with the actual email\nheaders.\n\n> ---\n> Documentation/git-fetch-pack.txt | 9 ++++++++\n> builtin/fetch-pack.c | 44 ++++++++++++++++++++++++++++++++++++--\n> fetch-pack.h | 3 ++-\n> 3 files changed, 53 insertions(+), 3 deletions(-)\n\nGive more of a commit message. Why is this option useful (I know, of\ncourse, from our previous discussion. But keep in mind the audience of\ndevelopers reading \"git log\" a year from now).\n\n> +--stdin::\n> +\tTake the list of refs from stdin, one per line. If there\n> +\tare refs specified on the command line in addition to this\n> +\toption, then the refs from stdin are processed after those\n> +\ton the command line.\n> +\tIf '--stateless-rpc' is specified together with this option\n> +\tthen the list of refs must be in packet format (pkt-line)\n> +\twith a flush packet terminating the list.\n\nNice. Thanks for taking the time not just to solve your problem, but to\ngive sane semantics in the non-stateless-rpc case.\n\nI think there is a minor formatting bug in the above. Asciidoc will make\nyour two paragraphs into a single one, won't it? I think you need to do\nthe (horribly ugly):\n\n --stdin::\n First paragraph.\n +\n Second paragraph.\n\nto appease asciidoc.\n\n> --- a/builtin/fetch-pack.c\n> +++ b/builtin/fetch-pack.c\n> @@ -23,7 +23,7 @@ static struct fetch_pack_args args = {\n> };\n> \n> static const char fetch_pack_usage[] =\n> -\"git fetch-pack [--all] [--quiet|-q] [--keep|-k] [--thin] [--include-tag] [--upload-pack=] [--depth=] [--no-progress] [-v] [:] [...]\";\n> +\"git fetch-pack [--all] [--stdin] [--quiet|-q] [--keep|-k] [--thin] [--include-tag] [--upload-pack=] [--depth=] [--no-progress] [-v] [:] [...]\";\n\nNot a problem you introduced, but maybe it is time for us to cut down\nthis gigantic line (it's 180 characters even before your patch!). Even\nbreaking it across lines at 80 columns would help.\n\n> @@ -972,6 +976,42 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n> \tif (!dest)\n> \t\tusage(fetch_pack_usage);\n> \n> +\tif (args.refs_from_stdin) {\n> +\t\tchar ref[1000];\n\nIck. Is there any reason not to use a strbuf here? 1000 is probably\nplenty, but we are generally moving towards removing such limits where\npossible.\n\nYou'd also get to use strbuf_getline and strbuf_trim in the\nnewline-delimited case.\n\n-Peff\n"},{"id":"187664","messageId":"20120325012432.GD27651@sigill.intra.peff.net","threadId":"29986","inReplyTo":"4F6E3478.3090505@gmx.net","subject":"Re: [PATCH/RFC 2/2] remote-curl: send the refs to fetch-pack on stdin","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-25T01:24:33Z","receivedAt":"2012-03-25T01:24:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 24, 2012 at 09:54:16PM +0100, Ivan Todoroski wrote:\n\n> From 723a561946824ee367f57f0d9b9d336a6bc28d13 Mon Sep 17 00:00:00 2001\n> From: Ivan Todoroski \n> Date: Sat, 24 Mar 2012 15:53:36 +0100\n> Subject: [PATCH/RFC 2/2] remote-curl: send the refs to fetch-pack on stdin\n> \n> ---\n\nSame comments as patch 1. Drop this stuff, and add in more commit\nmessage. :)\n\n> remote-curl.c | 18 +++++++++++++-----\n> 1 file changed, 13 insertions(+), 5 deletions(-)\n\nOverall the code looks sane to me. One minor comment:\n\n> -static int rpc_service(struct rpc_state *rpc, struct discovery *heads)\n> +static int rpc_service(struct rpc_state *rpc, struct discovery *heads,\n> +\t\tint nr_fetch, struct ref **to_fetch)\n\nI was curious why you needed to add new arguments for this, since we\nsurely must be passing the information into rpc_service already (since\nit has to put them on the command line). And the answer is that they are\nalready in the argv member of the struct rpc_state. I wonder if it would\nfit the existing style of the code to pass the arguments through a\nmember in the rpc_state in the same way. I don't feel strongly about it,\nthough.\n\n-Peff\n"},{"id":"187666","messageId":"20120325023215.GA13958@sigill.intra.peff.net","threadId":"29986","inReplyTo":"20120325010609.GB27651@sigill.intra.peff.net","subject":"Re: Clone fails on a repo with too many heads/tags","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-25T02:32:15Z","receivedAt":"2012-03-25T02:32:15Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 24, 2012 at 09:06:09PM -0400, Jeff King wrote:\n\n> That's weird. 405 is \"Method Not Allowed\". Clone shouldn't be doing\n> anything more exotic than GET and POST. And the prior tests pass, so it\n> means that it's working in general. The only thing different about this\n> test is that apache is configured to use mod_rewrite to issue a\n> redirect.\n> \n> Does your machine have mod_rewrite installed and enabled? I would think\n> apache would complain at startup if it wasn't. I wonder if there's\n> something non-portable in the minimal apache config we ship.\n> \n> Does httpd/error.log in the trash directory say anything interesting?\n\nAlso, does it work any better with this patch?\n\ndiff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\nindex 3c12b05..714760d 100644\n--- a/t/lib-httpd/apache.conf\n+++ b/t/lib-httpd/apache.conf\n@@ -61,9 +61,8 @@ ScriptAlias /smart_noexport/ ${GIT_EXEC_PATH}/git-http-backend/\n \tOptions ExecCGI\n \n \n-RewriteEngine on\n-RewriteRule ^/smart-redir-perm/(.*)$ /smart/$1 [R=301]\n-RewriteRule ^/smart-redir-temp/(.*)$ /smart/$1 [R=302]\n+RedirectMatch 301 ^/smart-redir-perm/(.*)$ /smart/$1\n+RedirectMatch 302 ^/smart-redir-temp/(.*)$ /smart/$1\n \n \n LoadModule ssl_module modules/mod_ssl.so\n"},{"id":"187671","messageId":"4F6EE7E2.1020702@gmx.net","threadId":"29986","inReplyTo":"20120325011948.GC27651@sigill.intra.peff.net","subject":"Re: [PATCH/RFC 1/2] fetch-pack: new option to read refs from stdin","fromName":"Ivan Todoroski","fromEmail":"grnch_lists@gmx.net","sentAt":"2012-03-25T09:39:46Z","receivedAt":"2012-03-25T09:39:46Z","isPatch":true,"sender":{"key":"grnch_lists@gmx.net","avatar":null},"body":"On 25.03.2012 03:19, Jeff King wrote:\n> On Sat, Mar 24, 2012 at 09:53:26PM +0100, Ivan Todoroski wrote:\n> \n>> ---\n>> Documentation/git-fetch-pack.txt | 9 ++++++++\n>> builtin/fetch-pack.c | 44 ++++++++++++++++++++++++++++++++++++--\n>> fetch-pack.h | 3 ++-\n>> 3 files changed, 53 insertions(+), 3 deletions(-)\n> \n> Give more of a commit message. Why is this option useful (I know, of\n> course, from our previous discussion. But keep in mind the audience of\n> developers reading \"git log\" a year from now).\n\nDefinitely. This was just work in progress I posted while I figure out \nthe test suite problem. I will add full commit messages when I complete \nthe test cases. Thanks for reviewing, I will incorporate all your \nsuggestions in the next version.\n\n>> @@ -972,6 +976,42 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n>> \tif (!dest)\n>> \t\tusage(fetch_pack_usage);\n>> \n>> +\tif (args.refs_from_stdin) {\n>> +\t\tchar ref[1000];\n> \n> Ick. Is there any reason not to use a strbuf here? 1000 is probably\n> plenty, but we are generally moving towards removing such limits where\n> possible.\n> \n> You'd also get to use strbuf_getline and strbuf_trim in the\n> newline-delimited case.\n\nRight now that \"char ref[1000]\" code is rejecting refs on stdin if they \nare longer than 1000 chars or if they contain an ASCII NUL char in them. \nWhen I change this to strbuf_getline() should I be doing any similar \nchecks, or do I just pass on whatever I read?\n"},{"id":"187672","messageId":"4F6EEAE3.60204@gmx.net","threadId":"29986","inReplyTo":"20120325012432.GD27651@sigill.intra.peff.net","subject":"Re: [PATCH/RFC 2/2] remote-curl: send the refs to fetch-pack on stdin","fromName":"Ivan Todoroski","fromEmail":"grnch_lists@gmx.net","sentAt":"2012-03-25T09:52:35Z","receivedAt":"2012-03-25T09:52:35Z","isPatch":true,"sender":{"key":"grnch_lists@gmx.net","avatar":null},"body":"On 25.03.2012 03:24, Jeff King wrote:\n> On Sat, Mar 24, 2012 at 09:54:16PM +0100, Ivan Todoroski wrote:\n>> -static int rpc_service(struct rpc_state *rpc, struct discovery *heads)\n>> +static int rpc_service(struct rpc_state *rpc, struct discovery *heads,\n>> +\t\tint nr_fetch, struct ref **to_fetch)\n> \n> I was curious why you needed to add new arguments for this, since we\n> surely must be passing the information into rpc_service already (since\n> it has to put them on the command line). And the answer is that they are\n> already in the argv member of the struct rpc_state. I wonder if it would\n> fit the existing style of the code to pass the arguments through a\n> member in the rpc_state in the same way. I don't feel strongly about it,\n> though.\n\nYeah, I realized this was ugly after I sent it.... not only are new \narguments added to rpc_service(), but they seem completely arbitrary and \nspecific to fetch-pack. There is no reason other callers of \nrpc_service() would want args in the exact same format.\n\nOn the other hand I can't reuse rpc_state.argv because that is already \npassed to client.argv for start_command().\n\nWould it be OK if I add a new memeber in rpc_state, e.g. \"struct strbuf \n*stdin_preamble\"? If non-NULL, it would contain any data the caller of \nrpc_service() wants shoved into the stdin of the sub-command before \nanything else. That way the caller is free to format this data in \nwhatever format they need, not only in the pkt-line format needed by \nfetch-pack.\n"},{"id":"187678","messageId":"4F6F3683.7000900@gmx.net","threadId":"29986","inReplyTo":"4F6EE7E2.1020702@gmx.net","subject":"Re: [PATCH/RFC 1/2] fetch-pack: new option to read refs from stdin","fromName":"Ivan Todoroski","fromEmail":"grnch_lists@gmx.net","sentAt":"2012-03-25T15:15:15Z","receivedAt":"2012-03-25T15:15:15Z","isPatch":true,"sender":{"key":"grnch_lists@gmx.net","avatar":null},"body":"On 25.03.2012 11:39, Ivan Todoroski wrote:\n> On 25.03.2012 03:19, Jeff King wrote:\n>> On Sat, Mar 24, 2012 at 09:53:26PM +0100, Ivan Todoroski wrote:\n>>> @@ -972,6 +976,42 @@ int cmd_fetch_pack(int argc, const char **argv, \n>>> const char *prefix)\n>>> if (!dest)\n>>> usage(fetch_pack_usage);\n>>> \n>>> + if (args.refs_from_stdin) {\n>>> + char ref[1000];\n>>\n>> Ick. Is there any reason not to use a strbuf here? 1000 is probably\n>> plenty, but we are generally moving towards removing such limits where\n>> possible.\n>>\n>> You'd also get to use strbuf_getline and strbuf_trim in the\n>> newline-delimited case.\n> \n> Right now that \"char ref[1000]\" code is rejecting refs on stdin if they \n> are longer than 1000 chars or if they contain an ASCII NUL char in them. \n> When I change this to strbuf_getline() should I be doing any similar \n> checks, or do I just pass on whatever I read?\n\nNever mind, there is no checking necessary. I just saw that \nstrbuf_getline() grows the buffer automatically so there is no chance of \nbuffer overflows, plus it doesn't mind if NUL chars are present in the \nmiddle of the string. The subsequent code that reads the refs will stop \nat the first NUL char anyway so all is well.\n\nI didn't know about this cool strbuf infrastructure, thanks for the tip.\n"},{"id":"187679","messageId":"4F6F39FC.2070801@gmx.net","threadId":"29986","inReplyTo":"20120325010609.GB27651@sigill.intra.peff.net","subject":"Re: Clone fails on a repo with too many heads/tags","fromName":"Ivan Todoroski","fromEmail":"grnch_lists@gmx.net","sentAt":"2012-03-25T15:30:04Z","receivedAt":"2012-03-25T15:30:04Z","isPatch":false,"sender":{"key":"grnch_lists@gmx.net","avatar":null},"body":"On 25.03.2012 03:06, Jeff King wrote:\n> On Sat, Mar 24, 2012 at 09:49:55PM +0100, Ivan Todoroski wrote:\n> \n>> Is it failing for anyone else on the vanilla \"maint\" branch? I would\n>> appreciate any help I could get here.\n> \n> No, it passes fine here (Debian unstable).\n\nOK, thanks for checking, it's good to know.\n\n\n> Does your machine have mod_rewrite installed and enabled? I would think\n> apache would complain at startup if it wasn't. I wonder if there's\n> something non-portable in the minimal apache config we ship.\n\nLooks like it comes by default with the RPM. Maybe the Apache version is \ntoo old?\n\n$ rpm -q httpd\nhttpd-2.2.3-63.el5.centos.1\n\n$ rpm -ql httpd | grep rewrite\n/usr/lib64/httpd/modules/mod_rewrite.so\n\n$ ls -l /usr/lib64/httpd/modules/mod_rewrite.so\n-rwxr-xr-x 1 root root 60384 Feb 23 19:23 \n/usr/lib64/httpd/modules/mod_rewrite.so\n\n$ ls -l trash\\ directory.t5551-http-fetch/httpd/modules/mod_rewrite.so\n-rwxr-xr-x 1 root root 60384 Feb 23 19:23 trash \ndirectory.t5551-http-fetch/httpd/modules/mod_rewrite.so\n\n\nAlso, mod_rewrite is enabled in both the master config file \n/etc/httpd/conf/httpd.conf, as well as the minimal \ngit/t/lib-httpd/apache.conf.\n\n\n> Does httpd/error.log in the trash directory say anything interesting?\n\nNothing, it just gives the startup and shutdown message:\n\n$ cat trash\\ directory.t5551-http-fetch/httpd/error.log\n[Sun Mar 25 15:27:12 2012] [notice] Apache/2.2.3 (CentOS) configured -- \nresuming normal operations\n[Sun Mar 25 15:27:12 2012] [notice] caught SIGTERM, shutting down\n\n\nI will try your patch from the other message a bit later and let you know.\n"},{"id":"187685","messageId":"4F6F56D5.3000309@gmx.net","threadId":"29986","inReplyTo":"20120325023215.GA13958@sigill.intra.peff.net","subject":"Re: Clone fails on a repo with too many heads/tags","fromName":"Ivan Todoroski","fromEmail":"grnch_lists@gmx.net","sentAt":"2012-03-25T17:33:09Z","receivedAt":"2012-03-25T17:33:09Z","isPatch":false,"sender":{"key":"grnch_lists@gmx.net","avatar":null},"body":"On 25.03.2012 04:32, Jeff King wrote:\n> On Sat, Mar 24, 2012 at 09:06:09PM -0400, Jeff King wrote:\n> \n>> That's weird. 405 is \"Method Not Allowed\". Clone shouldn't be doing\n>> anything more exotic than GET and POST. And the prior tests pass, so it\n>> means that it's working in general. The only thing different about this\n>> test is that apache is configured to use mod_rewrite to issue a\n>> redirect.\n>>\n>> Does your machine have mod_rewrite installed and enabled? I would think\n>> apache would complain at startup if it wasn't. I wonder if there's\n>> something non-portable in the minimal apache config we ship.\n>>\n>> Does httpd/error.log in the trash directory say anything interesting?\n> \n> Also, does it work any better with this patch?\n> \n> diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\n> index 3c12b05..714760d 100644\n> --- a/t/lib-httpd/apache.conf\n> +++ b/t/lib-httpd/apache.conf\n> @@ -61,9 +61,8 @@ ScriptAlias /smart_noexport/ ${GIT_EXEC_PATH}/git-http-backend/\n> \tOptions ExecCGI\n> \n> \n> -RewriteEngine on\n> -RewriteRule ^/smart-redir-perm/(.*)$ /smart/$1 [R=301]\n> -RewriteRule ^/smart-redir-temp/(.*)$ /smart/$1 [R=302]\n> +RedirectMatch 301 ^/smart-redir-perm/(.*)$ /smart/$1\n> +RedirectMatch 302 ^/smart-redir-temp/(.*)$ /smart/$1\n> \n> \n> LoadModule ssl_module modules/mod_ssl.so\n\n\nYes! That patch did it, t5551-http-fetch.sh now passes fully. Thank you.\n"},{"id":"187687","messageId":"4F6F5BCB.9050406@gmx.net","threadId":"29986","inReplyTo":"4F6F56D5.3000309@gmx.net","subject":"Re: Clone fails on a repo with too many heads/tags","fromName":"Ivan Todoroski","fromEmail":"grnch_lists@gmx.net","sentAt":"2012-03-25T17:54:19Z","receivedAt":"2012-03-25T17:54:19Z","isPatch":false,"sender":{"key":"grnch_lists@gmx.net","avatar":null},"body":"On 25.03.2012 19:33, Ivan Todoroski wrote:\n> On 25.03.2012 04:32, Jeff King wrote:\n>> On Sat, Mar 24, 2012 at 09:06:09PM -0400, Jeff King wrote:\n>>\n>>> That's weird. 405 is \"Method Not Allowed\". Clone shouldn't be doing\n>>> anything more exotic than GET and POST. And the prior tests pass, so it\n>>> means that it's working in general. The only thing different about this\n>>> test is that apache is configured to use mod_rewrite to issue a\n>>> redirect.\n>>>\n>>> Does your machine have mod_rewrite installed and enabled? I would think\n>>> apache would complain at startup if it wasn't. I wonder if there's\n>>> something non-portable in the minimal apache config we ship.\n>>>\n>>> Does httpd/error.log in the trash directory say anything interesting?\n>>\n>> Also, does it work any better with this patch?\n>>\n>> diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\n>> index 3c12b05..714760d 100644\n>> --- a/t/lib-httpd/apache.conf\n>> +++ b/t/lib-httpd/apache.conf\n>> @@ -61,9 +61,8 @@ ScriptAlias /smart_noexport/ \n>> ${GIT_EXEC_PATH}/git-http-backend/\n>> Options ExecCGI\n>> \n>> \n>> -RewriteEngine on\n>> -RewriteRule ^/smart-redir-perm/(.*)$ /smart/$1 [R=301]\n>> -RewriteRule ^/smart-redir-temp/(.*)$ /smart/$1 [R=302]\n>> +RedirectMatch 301 ^/smart-redir-perm/(.*)$ /smart/$1\n>> +RedirectMatch 302 ^/smart-redir-temp/(.*)$ /smart/$1\n>> \n>> \n>> LoadModule ssl_module modules/mod_ssl.so\n> \n> \n> Yes! That patch did it, t5551-http-fetch.sh now passes fully. Thank you.\n\nAh, no... I spoke too soon.\n\nI didn't look closely enough, I wasn't logged in at the right machine, \nsorry. :)\n\nIt's still failing on CentOS 5.8 with Apache 2.2.3. I will just find \nanother machine where the test suite works.\n\nIf you'd still like to debug the problem on CentOS 5.8 I can run any \ncommands or test any patches for you.\n"},{"id":"187691","messageId":"4F6F7941.8060008@gmx.net","threadId":"29986","inReplyTo":"20120325011948.GC27651@sigill.intra.peff.net","subject":"Re: [PATCH/RFC 1/2] fetch-pack: new option to read refs from stdin","fromName":"Ivan Todoroski","fromEmail":"grnch_lists@gmx.net","sentAt":"2012-03-25T20:00:01Z","receivedAt":"2012-03-25T20:00:01Z","isPatch":true,"sender":{"key":"grnch_lists@gmx.net","avatar":null},"body":"On 25.03.2012 03:19, Jeff King wrote:\n> On Sat, Mar 24, 2012 at 09:53:26PM +0100, Ivan Todoroski wrote:\n> I think there is a minor formatting bug in the above. Asciidoc will make\n> your two paragraphs into a single one, won't it? I think you need to do\n> the (horribly ugly):\n> \n> --stdin::\n> First paragraph.\n> +\n> Second paragraph.\n\nApparently this works too (i.e. indent the \"+\" too):\n\n --stdin::\n First paragraph.\n +\n Second paragraph.\n"},{"id":"187743","messageId":"20120326172145.GC7942@sigill.intra.peff.net","threadId":"29986","inReplyTo":"4F6F7941.8060008@gmx.net","subject":"Re: [PATCH/RFC 1/2] fetch-pack: new option to read refs from stdin","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-26T17:21:45Z","receivedAt":"2012-03-26T17:21:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 25, 2012 at 10:00:01PM +0200, Ivan Todoroski wrote:\n\n> On 25.03.2012 03:19, Jeff King wrote:\n> >On Sat, Mar 24, 2012 at 09:53:26PM +0100, Ivan Todoroski wrote:\n> >I think there is a minor formatting bug in the above. Asciidoc will make\n> >your two paragraphs into a single one, won't it? I think you need to do\n> >the (horribly ugly):\n> >\n> > --stdin::\n> > First paragraph.\n> > +\n> > Second paragraph.\n> \n> Apparently this works too (i.e. indent the \"+\" too):\n> \n> --stdin::\n> First paragraph.\n> +\n> Second paragraph.\n\nSadly, it's not quite the same (because I consider the source of your\nversion much more readable). The diff of the resulting HTML between my\nversion and yours is:\n\n--- no-indent.html\t2012-03-26 13:19:26.206728013 -0400\n+++ indent.html\t2012-03-26 13:19:04.270727319 -0400\n@@ -5,7 +5,8 @@\n
\n

\n First paragraph.\n+
\n+ Second paragraph.\n

\n-

Second paragraph.

\n
\n \n\nSo in your case it is putting in a line break, but not actually starting\na new paragraph.\n\n-Peff\n"},{"id":"187744","messageId":"20120326172432.GD7942@sigill.intra.peff.net","threadId":"29986","inReplyTo":"4F6EEAE3.60204@gmx.net","subject":"Re: [PATCH/RFC 2/2] remote-curl: send the refs to fetch-pack on stdin","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-26T17:24:32Z","receivedAt":"2012-03-26T17:24:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 25, 2012 at 11:52:35AM +0200, Ivan Todoroski wrote:\n\n> >>-static int rpc_service(struct rpc_state *rpc, struct discovery *heads)\n> >>+static int rpc_service(struct rpc_state *rpc, struct discovery *heads,\n> >>+\t\tint nr_fetch, struct ref **to_fetch)\n> [...]\n> Yeah, I realized this was ugly after I sent it.... not only are new\n> arguments added to rpc_service(), but they seem completely arbitrary\n> and specific to fetch-pack. There is no reason other callers of\n> rpc_service() would want args in the exact same format.\n\nYeah, I think that is what was bugging me about it.\n\n> On the other hand I can't reuse rpc_state.argv because that is\n> already passed to client.argv for start_command().\n\nRight. You'd need a new member.\n\n> Would it be OK if I add a new memeber in rpc_state, e.g. \"struct\n> strbuf *stdin_preamble\"? If non-NULL, it would contain any data the\n> caller of rpc_service() wants shoved into the stdin of the\n> sub-command before anything else. That way the caller is free to\n> format this data in whatever format they need, not only in the\n> pkt-line format needed by fetch-pack.\n\nI think that is the cleanest solution. It's a little less efficient, in\nthat we build the whole buffer in memory instead of sending each packet\nas we form it. But I think we are talking about a few kilobytes at most.\n\n-Peff\n"},{"id":"187746","messageId":"20120326173301.GE7942@sigill.intra.peff.net","threadId":"29986","inReplyTo":"4F6F5BCB.9050406@gmx.net","subject":"Re: Clone fails on a repo with too many heads/tags","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-26T17:33:01Z","receivedAt":"2012-03-26T17:33:01Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 25, 2012 at 07:54:19PM +0200, Ivan Todoroski wrote:\n\n> >>>That's weird. 405 is \"Method Not Allowed\". Clone shouldn't be doing\n> >>>anything more exotic than GET and POST. And the prior tests pass, so it\n> [...]\n> It's still failing on CentOS 5.8 with Apache 2.2.3. I will just find\n> another machine where the test suite works.\n\nI'm running the tests with apache 2.2.22. So I wonder if there is some\nconfig syntax or behavior that is simply different in the much older\nversion.\n\n> If you'd still like to debug the problem on CentOS 5.8 I can run any\n> commands or test any patches for you.\n\nA few more things to try:\n\n - check httpd/access.log to confirm that apache really is returning a\n 405 (I can't imagine that curl would not be reporting it accurately,\n but it's worth a shot)\n\n - try running with GIT_CURL_VERBOSE=1 to get debug output from curl\n\n - try adding these to the apache config:\n\n RewriteLog \"httpd/rewrite.log\"\n RewriteLogLevel 9\n\n which might yield more information.\n\nI have no idea what you're looking for in any of those, but maybe\nsomething useful will be obvious. I don't have a lot of apache\nexperience, so my next step in your shoes would be just trying to get\nmore information on what's happening.\n\n-Peff\n"},{"id":"187748","messageId":"4F70AC47.1010405@gmx.net","threadId":"29986","inReplyTo":"20120326172145.GC7942@sigill.intra.peff.net","subject":"Re: [PATCH/RFC 1/2] fetch-pack: new option to read refs from stdin","fromName":"Ivan Todoroski","fromEmail":"grnch_lists@gmx.net","sentAt":"2012-03-26T17:49:59Z","receivedAt":"2012-03-26T17:49:59Z","isPatch":true,"sender":{"key":"grnch_lists@gmx.net","avatar":null},"body":"On 26.03.2012 19:21, Jeff King wrote:\n> On Sun, Mar 25, 2012 at 10:00:01PM +0200, Ivan Todoroski wrote:\n> \n>> On 25.03.2012 03:19, Jeff King wrote:\n>>> On Sat, Mar 24, 2012 at 09:53:26PM +0100, Ivan Todoroski wrote:\n>>> I think there is a minor formatting bug in the above. Asciidoc will make\n>>> your two paragraphs into a single one, won't it? I think you need to do\n>>> the (horribly ugly):\n>>>\n>>> --stdin::\n>>> First paragraph.\n>>> +\n>>> Second paragraph.\n>> Apparently this works too (i.e. indent the \"+\" too):\n>>\n>> --stdin::\n>> First paragraph.\n>> +\n>> Second paragraph.\n> \n> Sadly, it's not quite the same (because I consider the source of your\n> version much more readable). The diff of the resulting HTML between my\n> version and yours is:\n> \n> --- no-indent.html\t2012-03-26 13:19:26.206728013 -0400\n> +++ indent.html\t2012-03-26 13:19:04.270727319 -0400\n> @@ -5,7 +5,8 @@\n>
\n>

\n> First paragraph.\n> +
\n> + Second paragraph.\n>

\n> -

Second paragraph.

\n>
\n> \n> \n> So in your case it is putting in a line break, but not actually starting\n> a new paragraph.\n> \n> -Peff\n\nAh, that's too bad. OK, I guess we have two options:\n\n1) Unindent the second paragraph - looks ugly in source but good in \nfinal output.\n\n2) Just merge the two paragraphs into a single paragraph. It's not that \nmuch text anyway, it doesn't really *have* to be in two paragraphs.\n\nLet me know which option you prefer.\n"},{"id":"187749","messageId":"20120326175125.GA9503@sigill.intra.peff.net","threadId":"29986","inReplyTo":"4F70AC47.1010405@gmx.net","subject":"Re: [PATCH/RFC 1/2] fetch-pack: new option to read refs from stdin","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-26T17:51:25Z","receivedAt":"2012-03-26T17:51:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 26, 2012 at 07:49:59PM +0200, Ivan Todoroski wrote:\n\n> >So in your case it is putting in a line break, but not actually starting\n> >a new paragraph.\n> >\n> Ah, that's too bad. OK, I guess we have two options:\n> \n> 1) Unindent the second paragraph - looks ugly in source but good in\n> final output.\n> \n> 2) Just merge the two paragraphs into a single paragraph. It's not\n> that much text anyway, it doesn't really *have* to be in two\n> paragraphs.\n> \n> Let me know which option you prefer.\n\nI would do (1). While I (and I suspect other git devs) do just read the\nsource straight from Documentation/*.txt, the formatted output is read\nby many more people, and should be considered the final product. So\noptimize for that case.\n\n-Peff\n"},{"id":"187826","messageId":"4F715CF7.5070903@gmx.net","threadId":"29986","inReplyTo":"20120321171423.GA13140@sigill.intra.peff.net","subject":"[PATCH/RFC v2 0/4] Fix fetch-pack command line overflow during clone","fromName":"Ivan Todoroski","fromEmail":"grnch@gmx.net","sentAt":"2012-03-27T06:23:51Z","receivedAt":"2012-03-27T06:23:51Z","isPatch":true,"sender":{"key":"grnch@gmx.net","avatar":"https://avatars.githubusercontent.com/u/90292?v=4"},"body":"This patch series is against the latest \"maint\" branch. Please let me\nknow if I need to rebase this on top of some other branch.\n\nThe problem we are fixing is described in the first patch in the series.\n\n\nChanges since the original patch:\n\n* add test cases\n* add full commit messages\n* fix formatting problem in --stdin doc\n* split overly long fetch_pack_usage line\n* use strbuf_getline() instead of fgets() for reading refs from stdin\n* minor optimization of the pkt-line reading loop, it was using xstrdup()\n even though the string length was already known, use xmemdupz() instead\n* rework the remote-curl.c patch to not add new parameters to rpc_service(),\n instead add a new strbuf member to rpc_state to pass the info around\n\n\nIvan Todoroski (4):\n fetch-pack: new --stdin option to read refs from stdin\n remote-curl: send the refs to fetch-pack on stdin\n fetch-pack: test cases for the new --stdin option\n remote-curl: main test case for the OS command line overflow\n\n Documentation/git-fetch-pack.txt | 10 ++++\n builtin/fetch-pack.c | 45 ++++++++++++++++-\n fetch-pack.h | 3 +-\n remote-curl.c | 15 +++++-\n t/t5500-fetch-pack.sh | 100 ++++++++++++++++++++++++++++++++++++++\n t/t5551-http-fetch.sh | 32 ++++++++++++\n 6 files changed, 201 insertions(+), 4 deletions(-)\n\n-- \n1.7.9.5.4.g4f508\n"},{"id":"187829","messageId":"4F715D55.8020109@gmx.net","threadId":"29986","inReplyTo":"4F715CF7.5070903@gmx.net","subject":"[PATCH/RFC v2 1/4] fetch-pack: new --stdin option to read refs from stdin","fromName":"Ivan Todoroski","fromEmail":"grnch@gmx.net","sentAt":"2012-03-27T06:25:25Z","receivedAt":"2012-03-27T06:25:25Z","isPatch":true,"sender":{"key":"grnch@gmx.net","avatar":"https://avatars.githubusercontent.com/u/90292?v=4"},"body":"If a remote repo has too many tags (or branches), cloning it over the\nsmart HTTP transport can fail because remote-curl.c puts all the refs\nfrom the remote repo on the fetch-pack command line. This can make the\ncommand line longer than the global OS command line limit, causing\nfetch-pack to fail.\n\nThis is especially a problem on Windows where the command line limit is\norders of magnitude shorter than Linux. There are already real repos out\nthere that msysGit cannot clone over smart HTTP due to this problem.\n\nHere is an easy way to trigger this problem:\n\ngit init too-many-refs\ncd too-many-refs\necho bla > bla.txt\ngit add .\ngit commit -m test\nsha=$(git rev-parse HEAD)\nfor ((i=0; i<50000; i++)); do\n\techo $sha refs/tags/artificially-long-tag-name-to-more-easily-\\\ndemonstrate-the-problem-$i >> .git/packed-refs\ndone\n\nThen share this repo over the smart HTTP protocol and try cloning it:\n\n\t$ git clone http://localhost/.../too-many-refs/.git\n\tCloning into 'too-many-refs'...\n\tfatal: cannot exec 'fetch-pack': Argument list too long\n\n50k tags is obviously an absurd number, but it is required to\ndemonstrate the problem on Linux because it has a much more generous\ncommand line limit. On Windows the clone fails with as little as 500\ntags in the above loop, which is getting uncomfortably close to the\nnumber of tags you might see in real long lived repos.\n\nThis is not just theoretical, msysGit is already failing to clone our\ncompany repo due to this. It's a large repo converted from CVS, nearly\n10 years of history.\n\nFour possible solutions were discussed on the Git mailing list (in no\nparticular order):\n\n1) Call fetch-pack multiple times with smaller batches of refs.\n\nThis was dismissed as inefficient and inelegant.\n\n2) Add option --refs-fd=$n to pass a an fd from where to read the refs.\n\nThis was rejected because inheriting descriptors other than\nstdin/stdout/stderr through exec() is apparently problematic on Windows,\nplus it would require changes to the run-command API to open extra\npipes.\n\n3) Add option --refs-from=$tmpfile to pass the refs using a temp file.\n\nThis was not favored because of the temp file requirement.\n\n4) Add option --stdin to pass the refs on stdin, one per line.\n\nIn the end this option was chosen as the most efficient and most\ndesirable from scripting perspective.\n\nThere was however a small complication when using stdin to pass refs to\nfetch-pack. The --stateless-rpc option to fetch-pack also uses stdin for\ncommunication with the remote server.\n\nIf we were going to sneak refs on stdin line by line in the presence of\n--stateless-rpc it would have to be done very carefully, because when\nreading refs one by line we might buffer too much data ahead and eat\nsome of the remote protocol data also coming on stdin.\n\nOne way to solve this would be refactor get_remote_heads() in\nfetch-pack.c to accept a residual buffer from our stdin line parsing\nabove, but this function is used in several places so other callers\nwould be burdened by this residual buffer interface even when most of\nthem don't need it.\n\nIn the end we settled on the following solution:\n\nIf --stdin is specified without --stateless-rpc, fetch-pack would read\nthe refs from stdin one per line, in a script friendly format.\n\nHowever if --stdin is specified together with --stateless-rpc,\nfetch-pack would read the refs from stdin in packetized format\n(pkt-line) with a flush packet terminating the list of refs. This way we\ncan read the exact number of bytes that we need from stdin, and then\nget_remote_heads() can continue reading from the same fd without losing\na single byte of remote protocol data.\n\nThis way the --stdin option only loses generality and scriptability when\nused together with --stateless-rpc, which is not easily scriptable\nanyway because it also uses pkt-line when talking to the remote server.\n---\n Documentation/git-fetch-pack.txt | 10 +++++++++\n builtin/fetch-pack.c | 45 +++++++++++++++++++++++++++++++++++++-\n fetch-pack.h | 3 ++-\n 3 files changed, 56 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-fetch-pack.txt b/Documentation/git-fetch-pack.txt\nindex ed1bdaacd1..1dd44fd348 100644\n--- a/Documentation/git-fetch-pack.txt\n+++ b/Documentation/git-fetch-pack.txt\n@@ -32,6 +32,16 @@ OPTIONS\n --all::\n \tFetch all remote refs.\n \n+--stdin::\n+\tTake the list of refs from stdin, one per line. If there\n+\tare refs specified on the command line in addition to this\n+\toption, then the refs from stdin are processed after those\n+\ton the command line.\n++\n+If '--stateless-rpc' is specified together with this option then\n+the list of refs must be in packet format (pkt-line) with a flush\n+packet terminating the list.\n+\n -q::\n --quiet::\n \tPass '-q' flag to 'git unpack-objects'; this makes the\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex a4d3e90a86..1a90fa852f 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -23,7 +23,9 @@ static struct fetch_pack_args args = {\n };\n \n static const char fetch_pack_usage[] =\n-\"git fetch-pack [--all] [--quiet|-q] [--keep|-k] [--thin] [--include-tag] [--upload-pack=] [--depth=] [--no-progress] [-v] [:] [...]\";\n+\"git fetch-pack [--all] [--stdin] [--quiet|-q] [--keep|-k] [--thin] \"\n+\"[--include-tag] [--upload-pack=] [--depth=] \"\n+\"[--no-progress] [-v] [:] [...]\";\n \n #define COMPLETE\t(1U << 0)\n #define COMMON\t\t(1U << 1)\n@@ -941,6 +943,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t\t\t\targs.fetch_all = 1;\n \t\t\t\tcontinue;\n \t\t\t}\n+\t\t\tif (!strcmp(\"--stdin\", arg)) {\n+\t\t\t\targs.refs_from_stdin = 1;\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t\tif (!strcmp(\"-v\", arg)) {\n \t\t\t\targs.verbose = 1;\n \t\t\t\tcontinue;\n@@ -972,6 +978,43 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \tif (!dest)\n \t\tusage(fetch_pack_usage);\n \n+\tif (args.refs_from_stdin) {\n+\t\t/* copy refs from cmdline to new growable list,\n+\t\t then append the refs from stdin */\n+\t\tint alloc_heads = nr_heads;\n+\t\tint size = nr_heads * sizeof(*heads);\n+\t\theads = memcpy(xmalloc(size), heads, size);\n+\t\tif (args.stateless_rpc) {\n+\t\t\t/* in stateless RPC mode we use pkt-line to read\n+\t\t\t from stdin, until we get a flush packet */\n+\t\t\tstatic char line[1000];\n+\t\t\tfor (;;) {\n+\t\t\t\tint n = packet_read_line(0, line, sizeof(line));\n+\t\t\t\tif (!n)\n+\t\t\t\t\tbreak;\n+\t\t\t\tif (line[n-1] == '\\n')\n+\t\t\t\t\tline[--n] = '\\0';\n+\t\t\t\tALLOC_GROW(heads, nr_heads + 1, alloc_heads);\n+\t\t\t\theads[nr_heads++] = xmemdupz(line, n);\n+\t\t\t}\n+\t\t}\n+\t\telse {\n+\t\t\t/* read from stdin one ref per line, until EOF */\n+\t\t\tstruct strbuf line;\n+\t\t\tstrbuf_init(&line, 0);\n+\t\t\tfor (;;) {\n+\t\t\t\tif (strbuf_getline(&line, stdin, '\\n') == EOF)\n+\t\t\t\t\tbreak;\n+\t\t\t\tstrbuf_trim(&line);\n+\t\t\t\tif (!line.len)\n+\t\t\t\t\tcontinue; /* skip empty lines */\n+\t\t\t\tALLOC_GROW(heads, nr_heads + 1, alloc_heads);\n+\t\t\t\theads[nr_heads++] = strbuf_detach(&line, NULL);\n+\t\t\t}\n+\t\t\tstrbuf_release(&line);\n+\t\t}\n+\t}\n+\n \tif (args.stateless_rpc) {\n \t\tconn = NULL;\n \t\tfd[0] = 0;\ndiff --git a/fetch-pack.h b/fetch-pack.h\nindex 0608edae3f..292d69389e 100644\n--- a/fetch-pack.h\n+++ b/fetch-pack.h\n@@ -13,7 +13,8 @@ struct fetch_pack_args {\n \t\tverbose:1,\n \t\tno_progress:1,\n \t\tinclude_tag:1,\n-\t\tstateless_rpc:1;\n+\t\tstateless_rpc:1,\n+\t\trefs_from_stdin:1;\n };\n \n struct ref *fetch_pack(struct fetch_pack_args *args,\n-- \n1.7.9.5.4.g4f508\n"},{"id":"187830","messageId":"4F715D91.5070901@gmx.net","threadId":"29986","inReplyTo":"4F715CF7.5070903@gmx.net","subject":"[PATCH/RFC v2 2/4] remote-curl: send the refs to fetch-pack on stdin","fromName":"Ivan Todoroski","fromEmail":"grnch@gmx.net","sentAt":"2012-03-27T06:26:25Z","receivedAt":"2012-03-27T06:26:25Z","isPatch":true,"sender":{"key":"grnch@gmx.net","avatar":"https://avatars.githubusercontent.com/u/90292?v=4"},"body":"If a remote repo has too many tags (or branches), cloning it over the\nsmart HTTP transport can fail because remote-curl.c puts all the refs\nfrom the remote repo on the fetch-pack command line. This can make the\ncommand line longer than the global OS command line limit, causing\nfetch-pack to fail.\n\nThis is especially a problem on Windows where the command line limit is\norders of magnitude shorter than Linux. There are already real repos out\nthere that msysGit cannot clone over smart HTTP due to this problem.\n\nTo solve this problem we teach remote-curl.c to pipe the refs to\nfetch-pack using the new --stdin option, instead of on the fetch-pack\ncommand line.\n\nFor a more detailed discussion of the problem see the parent of this\ncommit, titled \"fetch-pack: new --stdin option to read refs from stdin\".\n---\n remote-curl.c | 15 +++++++++++++--\n 1 file changed, 13 insertions(+), 2 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex d159fe7f34..52c21433c7 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -290,6 +290,7 @@ static void output_refs(struct ref *refs)\n struct rpc_state {\n \tconst char *service_name;\n \tconst char **argv;\n+\tstruct strbuf *stdin_preamble;\n \tchar *service_url;\n \tchar *hdr_content_type;\n \tchar *hdr_accept;\n@@ -535,6 +536,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads)\n {\n \tconst char *svc = rpc->service_name;\n \tstruct strbuf buf = STRBUF_INIT;\n+\tstruct strbuf *preamble = rpc->stdin_preamble;\n \tstruct child_process client;\n \tint err = 0;\n \n@@ -545,6 +547,8 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads)\n \tclient.argv = rpc->argv;\n \tif (start_command(&client))\n \t\texit(1);\n+\tif (preamble)\n+\t\twrite_or_die(client.in, preamble->buf, preamble->len);\n \tif (heads)\n \t\twrite_or_die(client.in, heads->buf, heads->len);\n \n@@ -626,6 +630,7 @@ static int fetch_git(struct discovery *heads,\n \tint nr_heads, struct ref **to_fetch)\n {\n \tstruct rpc_state rpc;\n+\tstruct strbuf preamble;\n \tchar *depth_arg = NULL;\n \tconst char **argv;\n \tint argc = 0, i, err;\n@@ -633,6 +638,7 @@ static int fetch_git(struct discovery *heads,\n \targv = xmalloc((15 + nr_heads) * sizeof(char*));\n \targv[argc++] = \"fetch-pack\";\n \targv[argc++] = \"--stateless-rpc\";\n+\targv[argc++] = \"--stdin\";\n \targv[argc++] = \"--lock-pack\";\n \tif (options.followtags)\n \t\targv[argc++] = \"--include-tag\";\n@@ -651,23 +657,28 @@ static int fetch_git(struct discovery *heads,\n \t\targv[argc++] = depth_arg;\n \t}\n \targv[argc++] = url;\n+\targv[argc++] = NULL;\n+\n+\tstrbuf_init(&preamble, 4);\n \tfor (i = 0; i < nr_heads; i++) {\n \t\tstruct ref *ref = to_fetch[i];\n \t\tif (!ref->name || !*ref->name)\n \t\t\tdie(\"cannot fetch by sha1 over smart http\");\n-\t\targv[argc++] = ref->name;\n+\t\tpacket_buf_write(&preamble, \"%s\\n\", ref->name);\n \t}\n-\targv[argc++] = NULL;\n+\tpacket_buf_flush(&preamble);\n \n \tmemset(&rpc, 0, sizeof(rpc));\n \trpc.service_name = \"git-upload-pack\",\n \trpc.argv = argv;\n+\trpc.stdin_preamble = &preamble;\n \trpc.gzip_request = 1;\n \n \terr = rpc_service(&rpc, heads);\n \tif (rpc.result.len)\n \t\tsafe_write(1, rpc.result.buf, rpc.result.len);\n \tstrbuf_release(&rpc.result);\n+\tstrbuf_release(&preamble);\n \tfree(argv);\n \tfree(depth_arg);\n \treturn err;\n-- \n1.7.9.5.4.g4f508\n"},{"id":"187831","messageId":"4F715DBF.6030909@gmx.net","threadId":"29986","inReplyTo":"4F715CF7.5070903@gmx.net","subject":"[PATCH/RFC v2 3/4] fetch-pack: test cases for the new --stdin option","fromName":"Ivan Todoroski","fromEmail":"grnch@gmx.net","sentAt":"2012-03-27T06:27:11Z","receivedAt":"2012-03-27T06:27:11Z","isPatch":true,"sender":{"key":"grnch@gmx.net","avatar":"https://avatars.githubusercontent.com/u/90292?v=4"},"body":"These test cases focus only on testing the parsing of refs on stdin,\nwithout bothering with the rest of the fetch-pack machinery. We pass in\nthe refs using different combinations of command line and stdin and then\nwe watch fetch-pack's stdout to see whether it prints all the refs we\nspecified and in the same order as we specified them.\n\nFor the --stateless-rpc tests we cannot easily execute fetch-pack to the\nend because that would require simulating the remote protocol. We settle\nfor only checking 2 cases:\n\n1) Whether fetch-pack correctly fails to parse the refs if they are not\nterminated by a flush packet\n\n2) Whether fetch-pack finishes parsing the refs without error when they\nare correctly terminated by a flush packet\n\nThe fetch-pack invocation fails in both cases due to the missing remote\nside of the protocol, but it fails in different ways which allows us to\ndetermine how the refs parsing ended by inspecting the different error\nmessages.\n---\n t/t5500-fetch-pack.sh | 100 +++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 100 insertions(+)\n\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 9bf69e9a0f..f4de7d07c1 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -248,4 +248,104 @@ test_expect_success 'clone shallow object count' '\n \tgrep \"^count: 52\" count.shallow\n '\n \n+\n+cat >stdin.exp < ../stdin.act &&\n+\tcd .. &&\n+\ttest_cmp stdin.exp stdin.act\n+'\n+\n+test_expect_success 'fetch refs from stdin' '\n+\tcd client &&\n+\tcat ../stdin.exp |\n+\tgit fetch-pack --stdin --no-progress .. |\n+\tcut -d \" \" -f 2 > ../stdin.act &&\n+\tcd .. &&\n+\ttest_cmp stdin.exp stdin.act\n+'\n+\n+test_expect_success 'fetch mixed refs from cmdline and stdin in right order' '\n+\tcd client &&\n+\ttail -n +5 ../stdin.exp |\n+\tgit fetch-pack --stdin --no-progress .. $(head -n 4 ../stdin.exp) |\n+\tcut -d \" \" -f 2 > ../stdin.act &&\n+\tcd .. &&\n+\ttest_cmp stdin.exp stdin.act\n+'\n+\n+\n+# remove final newline and insert random spaces, NULs, and empty lines\n+head -c -1 stdin.exp | sed -e '\n+\t1i\n+\t2s/^\\|$/ /g\n+\t4s/^/ /\n+\t6s/$/ /\n+\t8s/$/\\n /\n+\t9s/$/Ztrailing garbage/\n+\t9s/^\\|$/ /g\n+' | tr \"Z\" \"\\000\" > stdin.spaces\n+\n+test_expect_success 'ignore leading/trailing spaces, empty lines and NULs' '\n+\tcd client &&\n+\tcat ../stdin.spaces |\n+\tgit fetch-pack --stdin --no-progress .. |\n+\tcut -d \" \" -f 2 > ../stdin.act &&\n+\tcd .. &&\n+\ttest_cmp stdin.exp stdin.act\n+'\n+\n+\n+packet_write() { printf \"%04x%s\\n\" $((${#1} + 5)) \"$1\"; }\n+packet_flush() { printf \"0000\"; }\n+\n+cat stdin.exp | while read ref; do\n+\tpacket_write $ref\n+done | tee stdin.pkt.bad |\n+(packet_flush; packet_write garbage) > stdin.pkt.good\n+\n+echo \"fatal: protocol error: expected sha/ref, got 'garbage'\" > stdin.pkt.good.exp\n+echo \"fatal: The remote end hung up unexpectedly\" > stdin.pkt.bad.exp\n+\n+test_expect_success 'refs on stdin in packetized format' '\n+\tcd client &&\n+\tcat ../stdin.pkt.good |\n+\ttest_must_fail git fetch-pack --stdin --stateless-rpc .. \\\n+\t2> ../stdin.pkt.good.act &&\n+\tcd .. &&\n+\ttest_cmp stdin.pkt.good.exp stdin.pkt.good.act\n+'\n+\n+test_expect_success 'fail if flush packet is missing' '\n+\tcd client &&\n+\tcat ../stdin.pkt.bad |\n+\ttest_must_fail git fetch-pack --stdin --stateless-rpc .. \\\n+\t2> ../stdin.pkt.bad.act &&\n+\tcd .. &&\n+\ttest_cmp stdin.pkt.bad.exp stdin.pkt.bad.act\n+'\n+\n test_done\n-- \n1.7.9.5.4.g4f508\n"},{"id":"187832","messageId":"4F715DF6.7080401@gmx.net","threadId":"29986","inReplyTo":"4F715CF7.5070903@gmx.net","subject":"[PATCH/RFC v2 4/4] remote-curl: main test case for the OS command line overflow","fromName":"Ivan Todoroski","fromEmail":"grnch@gmx.net","sentAt":"2012-03-27T06:28:06Z","receivedAt":"2012-03-27T06:28:06Z","isPatch":true,"sender":{"key":"grnch@gmx.net","avatar":"https://avatars.githubusercontent.com/u/90292?v=4"},"body":"This is main test case for the original problem that triggered this\npatch series. We create a repo with 50k tags and then test whether\ngit-clone over the smart HTTP protocol succeeds.\n\nNote that we construct the repo in a slightly different way than the\noriginal script used to reproduce the problem. This is because the\noriginal script just created 50k tags all pointing to the same commit,\nso if there was a bug where remote-curl.c was not passing all the refs\nto fetch-pack we wouldn't know. The clone would succed even if only one\ntag was passed, because all the other tags were pointing at the same SHA\nand would be considered present.\n\nInstead we create a repo with 50k independent (dangling) commits and\nthen tag each of those commits with a unique tag. This way if one of the\ntags is not given to fetch-pack, later stages of the clone would\ncomplain about it.\n\nThis allows us to test both that the command line overflow was fixed, as\nwell as that it was fixed in a way that doesn't leave out any of the\nrefs.\n---\n t/t5551-http-fetch.sh | 32 ++++++++++++++++++++++++++++++++\n 1 file changed, 32 insertions(+)\n\ndiff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh\nindex 26d355725f..fb970bda22 100755\n--- a/t/t5551-http-fetch.sh\n+++ b/t/t5551-http-fetch.sh\n@@ -109,5 +109,37 @@ test_expect_success 'follow redirects (302)' '\n \tgit clone $HTTPD_URL/smart-redir-temp/repo.git --quiet repo-t\n '\n \n+\n+test -n \"$GIT_TEST_LONG\" && test_set_prereq EXPENSIVE\n+\n+test_expect_success EXPENSIVE 'create 50,000 tags in the repo' '\n+\t(\n+\tcd \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n+\tN=50000 &&\n+\tfor ((i=1; i<=$N; i++)); do\n+\t\techo \"commit refs/heads/too-many-refs\"\n+\t\techo \"mark :$i\"\n+\t\techo \"committer git $i +0000\"\n+\t\techo \"data 0\"\n+\t\techo \"M 644 inline bla.txt\"\n+\t\techo \"data 4\"\n+\t\techo \"bla\"\n+\t\t# make every commit dangling by always\n+\t\t# rewinding the branch after each commit\n+\t\techo \"reset refs/heads/too-many-refs\"\n+\t\techo \"from :1\"\n+\tdone | git fast-import --export-marks=marks &&\n+\n+\t# now assign tags to all the dangling commits we created above\n+\texport tag=refs/tags/artificially-long-tag-name-to-more-easily-demonstrate-the-problem &&\n+\tperl -nle '\\''/:(.+) (.+)/; print \"$2 $ENV{tag}-$1\"'\\'' < marks >> packed-refs\n+\t)\n+'\n+\n+test_expect_success EXPENSIVE 'clone the 50,000 tag repo to check OS command line overflow' '\n+\tgit clone $HTTPD_URL/smart/repo.git too-many-refs 2> too-many-refs.err &&\n+\ttest_line_count = 0 too-many-refs.err\n+'\n+\n stop_httpd\n test_done\n-- \n1.7.9.5.4.g4f508\n"},{"id":"187836","messageId":"4F71672F.7090202@gmx.net","threadId":"29986","inReplyTo":"20120326173301.GE7942@sigill.intra.peff.net","subject":"Re: Clone fails on a repo with too many heads/tags","fromName":"Ivan Todoroski","fromEmail":"grnch_lists@gmx.net","sentAt":"2012-03-27T07:07:27Z","receivedAt":"2012-03-27T07:07:27Z","isPatch":false,"sender":{"key":"grnch_lists@gmx.net","avatar":null},"body":"On 26.03.2012 19:33, Jeff King wrote:\n> On Sun, Mar 25, 2012 at 07:54:19PM +0200, Ivan Todoroski wrote:\n> \n>>>>> That's weird. 405 is \"Method Not Allowed\". Clone shouldn't be doing\n>>>>> anything more exotic than GET and POST. And the prior tests pass, so it\n>> [...]\n>> It's still failing on CentOS 5.8 with Apache 2.2.3. I will just find\n>> another machine where the test suite works.\n> \n> I'm running the tests with apache 2.2.22. So I wonder if there is some\n> config syntax or behavior that is simply different in the much older\n> version.\n\nMost likely.\n\n>> If you'd still like to debug the problem on CentOS 5.8 I can run any\n>> commands or test any patches for you.\n> \n> A few more things to try:\n> \n> - check httpd/access.log to confirm that apache really is returning a\n> 405 (I can't imagine that curl would not be reporting it accurately,\n> but it's worth a shot)\n\nYeah, it has 405 in access log too.\n\n> - try running with GIT_CURL_VERBOSE=1 to get debug output from curl\n> \n> - try adding these to the apache config:\n> \n> RewriteLog \"httpd/rewrite.log\"\n> RewriteLogLevel 9\n> \n> which might yield more information.\n> \n> I have no idea what you're looking for in any of those, but maybe\n> something useful will be obvious. I don't have a lot of apache\n> experience, so my next step in your shoes would be just trying to get\n> more information on what's happening.\n\nThanks for the tips. I'll keep tinkering with this as time allows and \npost a patch if I find something.\n"},{"id":"187865","messageId":"7vhaxaj7yi.fsf@alter.siamese.dyndns.org","threadId":"29986","inReplyTo":"4F715D55.8020109@gmx.net","subject":"Re: [PATCH/RFC v2 1/4] fetch-pack: new --stdin option to read refs from stdin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-27T16:59:33Z","receivedAt":"2012-03-27T16:59:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ivan Todoroski writes:\n\n> sha=$(git rev-parse HEAD)\n> for ((i=0; i<50000; i++)); do\n> \techo $sha refs/tags/artificially-long-tag-name-to-more-easily-\\\n> demonstrate-the-problem-$i >> .git/packed-refs\n> done\n\nThis comment does not have much to the issue, but the above is better\nwritten as\n\n\tcommit=$(git rev-parse HEAD) i=0\n while test $i -le 50000\n do\n \techo $commit refs/tags/....-$i\n\tdone >>.git/packed.refs\n\nto stay within the usual Bourne shell.\n\n> 4) Add option --stdin to pass the refs on stdin, one per line.\n> ...\n> In the end we settled on the following solution:\n>\n> If --stdin is specified without --stateless-rpc, fetch-pack would read\n> the refs from stdin one per line, in a script friendly format.\n>\n> However if --stdin is specified together with --stateless-rpc,\n> fetch-pack would read the refs from stdin in packetized format\n> (pkt-line) with a flush packet terminating the list of refs. This way we\n> can read the exact number of bytes that we need from stdin, and then\n> get_remote_heads() can continue reading from the same fd without losing\n> a single byte of remote protocol data.\n\nThat sounds like the right way to use pkt-line machinery. Send stuff you\nneed to send, and mark it with a flush to tell the receiver that you have\nfinished giving one logical collection of information.\n\n> This way the --stdin option only loses generality and scriptability when\n> used together with --stateless-rpc, which is not easily scriptable\n> anyway because it also uses pkt-line when talking to the remote server.\n> ---\n\nJust a gentle reminder; the final submission will need your sign off here.\n\n> diff --git a/Documentation/git-fetch-pack.txt b/Documentation/git-fetch-pack.txt\n> index ed1bdaacd1..1dd44fd348 100644\n> --- a/Documentation/git-fetch-pack.txt\n> +++ b/Documentation/git-fetch-pack.txt\n> @@ -32,6 +32,16 @@ OPTIONS\n> --all::\n> \tFetch all remote refs.\n> \n> +--stdin::\n> +\tTake the list of refs from stdin, one per line. If there\n> +\tare refs specified on the command line in addition to this\n> +\toption, then the refs from stdin are processed after those\n> +\ton the command line.\n> ++\n> +If '--stateless-rpc' is specified together with this option then\n> +the list of refs must be in packet format (pkt-line) with a flush\n> +packet terminating the list.\n> +\n\nIt is not clear from this description alone if this is a single (possibly\ngiant) packet with multiple lines, each of which describes a ref, or a\nseries of one packet per ref with a flush at the end of the sequence.\n\n> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\n> index a4d3e90a86..1a90fa852f 100644\n> --- a/builtin/fetch-pack.c\n> +++ b/builtin/fetch-pack.c\n> @@ -972,6 +978,43 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n> \tif (!dest)\n> \t\tusage(fetch_pack_usage);\n> \n> +\tif (args.refs_from_stdin) {\n> +\t\t/* copy refs from cmdline to new growable list,\n> +\t\t then append the refs from stdin */\n\n /*\n * We tend to format our multi-line\n * comments like this\n */\n\n> +\t\tint alloc_heads = nr_heads;\n> +\t\tint size = nr_heads * sizeof(*heads);\n> +\t\theads = memcpy(xmalloc(size), heads, size);\n> +\t\tif (args.stateless_rpc) {\n> +\t\t\t/* in stateless RPC mode we use pkt-line to read\n> +\t\t\t from stdin, until we get a flush packet */\n> +\t\t\tstatic char line[1000];\n\nWe will never have a refname that is longer than this limit?\n\n> +\t\t\tfor (;;) {\n> +\t\t\t\tint n = packet_read_line(0, line, sizeof(line));\n> +\t\t\t\tif (!n)\n> +\t\t\t\t\tbreak;\n> +\t\t\t\tif (line[n-1] == '\\n')\n> +\t\t\t\t\tline[--n] = '\\0';\n> +\t\t\t\tALLOC_GROW(heads, nr_heads + 1, alloc_heads);\n> +\t\t\t\theads[nr_heads++] = xmemdupz(line, n);\n\nMicronit. The use of xmemdupz() here means you do not have to replace LF\nwith NUL above; decrementing 'n' should be sufficient.\n\n> +\t\t\t}\n> +\t\t}\n> +\t\telse {\n> +\t\t\t/* read from stdin one ref per line, until EOF */\n> +\t\t\tstruct strbuf line;\n> +\t\t\tstrbuf_init(&line, 0);\n> +\t\t\tfor (;;) {\n> +\t\t\t\tif (strbuf_getline(&line, stdin, '\\n') == EOF)\n> +\t\t\t\t\tbreak;\n> +\t\t\t\tstrbuf_trim(&line);\n> +\t\t\t\tif (!line.len)\n> +\t\t\t\t\tcontinue; /* skip empty lines */\n\nCurious. \"stop at EOF\", \"trim\" and \"skip empty\" imply that you are\ncatering to people who debug this from the terminal by typing (or copy\npasting). Is that the expected use case?\n\nOtherwise we may want to tighten this part a bit to forbid cruft (e.g. a\nline with leading or trailing whitespaces).\n\n> +\t\t\t\tALLOC_GROW(heads, nr_heads + 1, alloc_heads);\n> +\t\t\t\theads[nr_heads++] = strbuf_detach(&line, NULL);\n> +\t\t\t}\n> +\t\t\tstrbuf_release(&line);\n> +\t\t}\n> +\t}\n> +\n> \tif (args.stateless_rpc) {\n> \t\tconn = NULL;\n> \t\tfd[0] = 0;\n> diff --git a/fetch-pack.h b/fetch-pack.h\n> index 0608edae3f..292d69389e 100644\n> --- a/fetch-pack.h\n> +++ b/fetch-pack.h\n> @@ -13,7 +13,8 @@ struct fetch_pack_args {\n> \t\tverbose:1,\n> \t\tno_progress:1,\n> \t\tinclude_tag:1,\n> -\t\tstateless_rpc:1;\n> +\t\tstateless_rpc:1,\n> +\t\trefs_from_stdin:1;\n> };\n> \n> struct ref *fetch_pack(struct fetch_pack_args *args,\n"},{"id":"187867","messageId":"7v8vimj73n.fsf@alter.siamese.dyndns.org","threadId":"29986","inReplyTo":"4F715D91.5070901@gmx.net","subject":"Re: [PATCH/RFC v2 2/4] remote-curl: send the refs to fetch-pack on stdin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-27T17:18:04Z","receivedAt":"2012-03-27T17:18:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ivan Todoroski writes:\n\n> If a remote repo has too many tags (or branches), cloning it over the\n> smart HTTP transport can fail because remote-curl.c puts all the refs\n> from the remote repo on the fetch-pack command line. This can make the\n> command line longer than the global OS command line limit, causing\n> fetch-pack to fail.\n>\n> This is especially a problem on Windows where the command line limit is\n> orders of magnitude shorter than Linux. There are already real repos out\n> there that msysGit cannot clone over smart HTTP due to this problem.\n>\n> To solve this problem we teach remote-curl.c to pipe the refs to\n> fetch-pack using the new --stdin option, instead of on the fetch-pack\n> command line.\n>\n> For a more detailed discussion of the problem see the parent of this\n> commit, titled \"fetch-pack: new --stdin option to read refs from stdin\".\n\nThanks. As there is no way for this single commit to be applied without\nthe previous step, you could have just said:\n\n\tNow we can throw arbitrary number of refs at fetch-pack using\n its --stdin option, use it in remote-curl helper to lift the\n command line length limit.\n\nor something ;-).\n\n> @@ -626,6 +630,7 @@ static int fetch_git(struct discovery *heads,\n> \tint nr_heads, struct ref **to_fetch)\n> {\n> \tstruct rpc_state rpc;\n> +\tstruct strbuf preamble;\n> \tchar *depth_arg = NULL;\n> \tconst char **argv;\n> \tint argc = 0, i, err;\n> @@ -633,6 +638,7 @@ static int fetch_git(struct discovery *heads,\n> \targv = xmalloc((15 + nr_heads) * sizeof(char*));\n> \targv[argc++] = \"fetch-pack\";\n> \targv[argc++] = \"--stateless-rpc\";\n> +\targv[argc++] = \"--stdin\";\n> \targv[argc++] = \"--lock-pack\";\n> \tif (options.followtags)\n> \t\targv[argc++] = \"--include-tag\";\n> @@ -651,23 +657,28 @@ static int fetch_git(struct discovery *heads,\n> \t\targv[argc++] = depth_arg;\n> \t}\n> \targv[argc++] = url;\n> +\targv[argc++] = NULL;\n> +\n> +\tstrbuf_init(&preamble, 4);\n\nCurious.\n\nIf \"4\" does not really matter, I would drop this and use STRBUF_INIT at\nthe beginning instead.\n\n> \tfor (i = 0; i < nr_heads; i++) {\n> \t\tstruct ref *ref = to_fetch[i];\n> \t\tif (!ref->name || !*ref->name)\n> \t\t\tdie(\"cannot fetch by sha1 over smart http\");\n> -\t\targv[argc++] = ref->name;\n> +\t\tpacket_buf_write(&preamble, \"%s\\n\", ref->name);\n> \t}\n> -\targv[argc++] = NULL;\n> +\tpacket_buf_flush(&preamble);\n> \n> \tmemset(&rpc, 0, sizeof(rpc));\n> \trpc.service_name = \"git-upload-pack\",\n> \trpc.argv = argv;\n> +\trpc.stdin_preamble = &preamble;\n> \trpc.gzip_request = 1;\n> \n> \terr = rpc_service(&rpc, heads);\n> \tif (rpc.result.len)\n> \t\tsafe_write(1, rpc.result.buf, rpc.result.len);\n> \tstrbuf_release(&rpc.result);\n> +\tstrbuf_release(&preamble);\n> \tfree(argv);\n> \tfree(depth_arg);\n> \treturn err;\n"},{"id":"187869","messageId":"7v4ntaj61t.fsf@alter.siamese.dyndns.org","threadId":"29986","inReplyTo":"4F715DBF.6030909@gmx.net","subject":"Re: [PATCH/RFC v2 3/4] fetch-pack: test cases for the new --stdin option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-27T17:40:46Z","receivedAt":"2012-03-27T17:40:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ivan Todoroski writes:\n\n> These test cases focus only on testing the parsing of refs on stdin,\n> without bothering with the rest of the fetch-pack machinery. We pass in\n> the refs using different combinations of command line and stdin and then\n> we watch fetch-pack's stdout to see whether it prints all the refs we\n> specified and in the same order as we specified them.\n\nThanks for writing what it does concisely and clearly. It makes it very\npleasant to review the patch.\n\nIt is sensible to expect that we see all the refs we told it to fetch, but\nI do not think it is sensible to require they come in the same order as we\nhave given them to the command.\n\n> For the --stateless-rpc tests we cannot easily execute fetch-pack to the\n> end because that would require simulating the remote protocol. We settle\n> for only checking 2 cases:\n>\n> 1) Whether fetch-pack correctly fails to parse the refs if they are not\n> terminated by a flush packet\n>\n> 2) Whether fetch-pack finishes parsing the refs without error when they\n> are correctly terminated by a flush packet\n>\n> The fetch-pack invocation fails in both cases due to the missing remote\n> side of the protocol, but it fails in different ways which allows us to\n> determine how the refs parsing ended by inspecting the different error\n> messages.\n\nIck.\n\n> ---\n> t/t5500-fetch-pack.sh | 100 +++++++++++++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 100 insertions(+)\n>\n> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n> index 9bf69e9a0f..f4de7d07c1 100755\n> --- a/t/t5500-fetch-pack.sh\n> +++ b/t/t5500-fetch-pack.sh\n> @@ -248,4 +248,104 @@ test_expect_success 'clone shallow object count' '\n> \tgrep \"^count: 52\" count.shallow\n> '\n> \n> +\n> +cat >stdin.exp < +refs/heads/C\n> +refs/heads/A\n> +refs/heads/D\n> +refs/tags/C\n> +refs/heads/B\n> +refs/tags/A\n> +refs/heads/E\n> +refs/tags/B\n> +refs/tags/E\n> +refs/tags/D\n> +EOF\n> +\n> +test_expect_success 'setup tests for --stdin' '\n> +\tfor head in C D E F; do\n> +\t\tadd $head\n> +\tdone &&\n> +\tfor head in A B C D E F; do\n> +\t\tgit tag $head $head\n> +\tdone\n> +'\n\nDrop \";\" and write \"do\" on its own line with proper indentation.\n\n> +\n> +test_expect_success 'fetch refs from cmdline, make sure it still works OK' '\n> +\tcd client &&\n> +\tgit fetch-pack --no-progress .. $(cat ../stdin.exp) |\n> +\tcut -d \" \" -f 2 > ../stdin.act &&\n> +\tcd .. &&\n> +\ttest_cmp stdin.exp stdin.act\n> +'\n\n - Do not chdir around without being in a subprocess ();\n\n - Do not place the command you are testing that might crash on the\n upstream of the pipe;\n\n - style;\n\ni.e.\n\n\t(\n\t\tcd client &&\n git fetch-pack --no-progress .. $(cat ../stdin.exp) >../stdin.raw\n\t) &&\n cut -d \" \" -f 2 stdin.act &&\n\ttest_cmp stdin.exp stdin.act\n\nNote that I lifted the \"in the same order\" requirement, which should not\nbe there. You may need to adjust the hardcoded stdin.exp to be sorted.\n\n> +test_expect_success 'fetch refs from stdin' '\n> +\tcd client &&\n> +\tcat ../stdin.exp |\n> +\tgit fetch-pack --stdin --no-progress .. |\n> +\tcut -d \" \" -f 2 > ../stdin.act &&\n> +\tcd .. &&\n> +\ttest_cmp stdin.exp stdin.act\n> +'\n\nIn addition to the comments on the previous one:\n\n - Do not pipe output from cat.\n\ni.e.\n\n\t(\n\t\tcd client &&\n git fetch-pack ... <../stdin.exp >stdin.raw\n\t) &&\n\tcut -d \" \" -f 2 stdin.act &&\n\ttest_cmp stdin.exp stdin.act\n\nBy the way, why are these not called \"expect\" and \"actual\" like most other\ntests?\n\n> +test_expect_success 'fetch mixed refs from cmdline and stdin in right order' '\n> +\tcd client &&\n> +\ttail -n +5 ../stdin.exp |\n> +\tgit fetch-pack --stdin --no-progress .. $(head -n 4 ../stdin.exp) |\n> +\tcut -d \" \" -f 2 > ../stdin.act &&\n> +\tcd .. &&\n> +\ttest_cmp stdin.exp stdin.act\n> +'\n\nDitto.\n\nDo we want to have a separate test to see what happens when there are dups\nin the input?\n\n> +# remove final newline and insert random spaces, NULs, and empty lines\n> +head -c -1 stdin.exp | sed -e '\n> +\t1i\n> +\t2s/^\\|$/ /g\n> +\t4s/^/ /\n> +\t6s/$/ /\n> +\t8s/$/\\n /\n> +\t9s/$/Ztrailing garbage/\n> +\t9s/^\\|$/ /g\n> +' | tr \"Z\" \"\\000\" > stdin.spaces\n\nSomebody may want to try this sed expression on Macs and BSDs, but more\nimportantly...\n\n> +test_expect_success 'ignore leading/trailing spaces, empty lines and NULs' '\n> +\tcd client &&\n> +\tcat ../stdin.spaces |\n> +\tgit fetch-pack --stdin --no-progress .. |\n> +\tcut -d \" \" -f 2 > ../stdin.act &&\n> +\tcd .. &&\n> +\ttest_cmp stdin.exp stdin.act\n> +'\n\n... it is unclear why it is a good thing to let the input with garbage\nproduce a result instead of erroring out.\n\nThe rest of the patch skimmed but not thoroughly reviewed; seems to have\nsimilar issues as already have been pointed out above.\n"},{"id":"187870","messageId":"7vzkb2hrd6.fsf@alter.siamese.dyndns.org","threadId":"29986","inReplyTo":"4F715DF6.7080401@gmx.net","subject":"Re: [PATCH/RFC v2 4/4] remote-curl: main test case for the OS command line overflow","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-27T17:43:17Z","receivedAt":"2012-03-27T17:43:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ivan Todoroski writes:\n\n> +test_expect_success EXPENSIVE 'create 50,000 tags in the repo' '\n> +\t(\n> +\tcd \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n> +\tN=50000 &&\n> +\tfor ((i=1; i<=$N; i++)); do\n\nThat's bash-ism isn't it?\n\n\tN=50000 I=1\n while test $I -le $N\n do\n\t\t...\n I=$(( $I + 1 ))\n\tdone\n\nThe body of the test itself looked reasonable.\n\nThanks.\n"},{"id":"187901","messageId":"4F724ADC.2030909@gmx.net","threadId":"29986","inReplyTo":"7vhaxaj7yi.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH/RFC v2 1/4] fetch-pack: new --stdin option to read refs from stdin","fromName":"Ivan Todoroski","fromEmail":"grnch@gmx.net","sentAt":"2012-03-27T23:18:52Z","receivedAt":"2012-03-27T23:18:52Z","isPatch":true,"sender":{"key":"grnch@gmx.net","avatar":"https://avatars.githubusercontent.com/u/90292?v=4"},"body":"On 27.03.2012 18:59, Junio C Hamano wrote:\n> Ivan Todoroski writes:\n> \n>> +\t\tint alloc_heads = nr_heads;\n>> +\t\tint size = nr_heads * sizeof(*heads);\n>> +\t\theads = memcpy(xmalloc(size), heads, size);\n>> +\t\tif (args.stateless_rpc) {\n>> +\t\t\t/* in stateless RPC mode we use pkt-line to read\n>> +\t\t\t from stdin, until we get a flush packet */\n>> +\t\t\tstatic char line[1000];\n> \n> We will never have a refname that is longer than this limit?\n\nI don't know. I grepped the code for existing usages of packet_read_line \nand that seemed to be a common idiom everywhere. Should I just bump up \nthe size or is there some accepted way to read arbitrary length packets?\n\n>> +\t\t\t}\n>> +\t\t}\n>> +\t\telse {\n>> +\t\t\t/* read from stdin one ref per line, until EOF */\n>> +\t\t\tstruct strbuf line;\n>> +\t\t\tstrbuf_init(&line, 0);\n>> +\t\t\tfor (;;) {\n>> +\t\t\t\tif (strbuf_getline(&line, stdin, '\\n') == EOF)\n>> +\t\t\t\t\tbreak;\n>> +\t\t\t\tstrbuf_trim(&line);\n>> +\t\t\t\tif (!line.len)\n>> +\t\t\t\t\tcontinue; /* skip empty lines */\n> \n> Curious. \"stop at EOF\", \"trim\" and \"skip empty\" imply that you are\n> catering to people who debug this from the terminal by typing (or copy\n> pasting). Is that the expected use case?\n\nThe expected use case is people using this from shell scripts that could \nbe getting refs by slicing and dicing output of other commands with \nregexps and what not which could leave some whitespace here and there, \nso a more liberal interface might be more friendly to such script writers.\n\nCurrently you would pass a list of generated refs to fetch-pack using \nsomething like this:\n\n generate-refs | xargs fetch-pack\n\nor this:\n\n fetch-pack $(generate-refs)\n\nBoth of these commands will ignore any extra whitespace produced by \n\"generate-refs\".\n\nSince --stdin is meant to be a spiritual replacement for the two \ncommands above, I thought it should behave in a similar spirit.\n\nAt least that was the reasoning... if you're not swayed by it I can just \nremove those lines and not tolerate any extra whitespace.\n"},{"id":"187902","messageId":"4F724B3C.2070702@gmx.net","threadId":"29986","inReplyTo":"7v8vimj73n.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH/RFC v2 2/4] remote-curl: send the refs to fetch-pack on stdin","fromName":"Ivan Todoroski","fromEmail":"grnch@gmx.net","sentAt":"2012-03-27T23:20:28Z","receivedAt":"2012-03-27T23:20:28Z","isPatch":true,"sender":{"key":"grnch@gmx.net","avatar":"https://avatars.githubusercontent.com/u/90292?v=4"},"body":"On 27.03.2012 19:18, Junio C Hamano wrote:\n> Ivan Todoroski writes:\n> \n>> +\tstrbuf_init(&preamble, 4);\n> \n> Curious.\n> \n> If \"4\" does not really matter, I would drop this and use STRBUF_INIT at\n> the beginning instead.\n\nI only put 4 because I know the strbuf will always have at least 4 \nbytes, because it always includes a flush packet. It doesn't matter at \nall otherwise, I will do as you suggest.\n"},{"id":"187903","messageId":"7vwr65y6b6.fsf@alter.siamese.dyndns.org","threadId":"29986","inReplyTo":"4F724ADC.2030909@gmx.net","subject":"Re: [PATCH/RFC v2 1/4] fetch-pack: new --stdin option to read refs from stdin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-27T23:26:05Z","receivedAt":"2012-03-27T23:26:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ivan Todoroski writes:\n\n> On 27.03.2012 18:59, Junio C Hamano wrote:\n>> Ivan Todoroski writes:\n>>\n>>> +\t\tint alloc_heads = nr_heads;\n>>> +\t\tint size = nr_heads * sizeof(*heads);\n>>> +\t\theads = memcpy(xmalloc(size), heads, size);\n>>> +\t\tif (args.stateless_rpc) {\n>>> +\t\t\t/* in stateless RPC mode we use pkt-line to read\n>>> +\t\t\t from stdin, until we get a flush packet */\n>>> +\t\t\tstatic char line[1000];\n>>\n>> We will never have a refname that is longer than this limit?\n>\n> I don't know. I grepped the code for existing usages of packet_read_line\n> and that seemed to be a common idiom everywhere. Should I just bump up\n> the size or is there some accepted way to read arbitrary length packets?\n\nAs you may have already guessed, you can have up to (64k - slop) bytes in\na packet. But it probably does not matter, as a loong ref that bust your\nlimit will not fit in the packet used in the normal codepath anyway.\n\n>> Curious. \"stop at EOF\", \"trim\" and \"skip empty\" imply that you are\n>> catering to people who debug this from the terminal by typing (or copy\n>> pasting). Is that the expected use case?\n>\n> The expected use case is people using this from shell scripts that\n> could be getting refs by slicing and dicing output of other commands\n> with regexps and what not\n\nThat kind of use would not have leading or trailing whitespaces, and if\nthe target audience is scripts, I would prefer to force them to be\nstrict.\n"},{"id":"187904","messageId":"4F724F0D.7000602@gmx.net","threadId":"29986","inReplyTo":"7v4ntaj61t.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH/RFC v2 3/4] fetch-pack: test cases for the new --stdin option","fromName":"Ivan Todoroski","fromEmail":"grnch@gmx.net","sentAt":"2012-03-27T23:36:45Z","receivedAt":"2012-03-27T23:36:45Z","isPatch":true,"sender":{"key":"grnch@gmx.net","avatar":"https://avatars.githubusercontent.com/u/90292?v=4"},"body":"On 27.03.2012 19:40, Junio C Hamano wrote:\n> \n> It is sensible to expect that we see all the refs we told it to fetch, but\n> I do not think it is sensible to require they come in the same order as we\n> have given them to the command.\n\nOh, OK. I was not aware that fetch-pack is free to reorder commits (at \nleast in principle). I will adjust the tests to be order-independent.\n\n>> For the --stateless-rpc tests we cannot easily execute fetch-pack to the\n>> end because that would require simulating the remote protocol. We settle\n>> for only checking 2 cases:\n>>\n>> 1) Whether fetch-pack correctly fails to parse the refs if they are not\n>> terminated by a flush packet\n>>\n>> 2) Whether fetch-pack finishes parsing the refs without error when they\n>> are correctly terminated by a flush packet\n>>\n>> The fetch-pack invocation fails in both cases due to the missing remote\n>> side of the protocol, but it fails in different ways which allows us to\n>> determine how the refs parsing ended by inspecting the different error\n>> messages.\n> \n> Ick.\n\nYeah... I couldn't figure out a way to do an isolated test of the \npacketized version of --stdin when --stateless-rpc is also in effect. \nAny guidance here would be welcome.\n\nOn second thought, maybe we can just drop these two --stateless-rpc \ntests from this patch? The \"git clone\" test in the next patch also \nexercises the packetized refs in --stateless-rpc mode and if there was \nanything wrong with them it would fail.\n\n>> +\n>> +test_expect_success 'fetch refs from cmdline, make sure it still works OK' '\n>> +\tcd client &&\n>> +\tgit fetch-pack --no-progress .. $(cat ../stdin.exp) |\n>> +\tcut -d \" \" -f 2 > ../stdin.act &&\n>> +\tcd .. &&\n>> +\ttest_cmp stdin.exp stdin.act\n>> +'\n> \n> - Do not chdir around without being in a subprocess ();\n\nSorry, I didn't realize the tests were eval-ed in the current \nenvironment. I will correct all such problems in the next version.\n\n> - Do not place the command you are testing that might crash on the\n> upstream of the pipe;\n> \n> - style;\n\nNoted.\n\n> \t(\n> \t\tcd client &&\n> git fetch-pack ... <../stdin.exp >stdin.raw\n> \t) &&\n> \tcut -d \" \" -f 2 stdin.act &&\n> \ttest_cmp stdin.exp stdin.act\n> \n> By the way, why are these not called \"expect\" and \"actual\" like most other\n> tests?\n\nThe test files I worked with used the shorter exp/act convention so I \nfollowed that.\n\nOr are you wondering about the \"stdin.\" prefix I added? That was because \nI didn't want to overwrite any exp/act files that might be created by \nthe previous tests, or is that not a concern?\n\nSorry, my inexperience with the Git test framework is showing...\n\n> Do we want to have a separate test to see what happens when there are dups\n> in the input?\n\nGood idea. I will check what happens in the current fetch-pack if \nduplicate refs are passed on the command line and will write a test to \nensure that the --stdin version does the same thing.\n\n\nP.S.\nIt goes without saying that I implicitly accept all the corrections that \nI did not reply to and will include them in the next version.\n"},{"id":"187905","messageId":"7vsjgty5ir.fsf@alter.siamese.dyndns.org","threadId":"29986","inReplyTo":"4F724F0D.7000602@gmx.net","subject":"Re: [PATCH/RFC v2 3/4] fetch-pack: test cases for the new --stdin option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-27T23:43:08Z","receivedAt":"2012-03-27T23:43:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ivan Todoroski writes:\n\n> On second thought, maybe we can just drop these two --stateless-rpc\n> tests from this patch? The \"git clone\" test in the next patch also\n> exercises the packetized refs in --stateless-rpc mode and if there was\n> anything wrong with them it would fail.\n\nYeah, I was thinking about the same.\n\n>>> +\n>>> +test_expect_success 'fetch refs from cmdline, make sure it still works OK' '\n>>> +\tcd client &&\n>>> +\tgit fetch-pack --no-progress .. $(cat ../stdin.exp) |\n>>> +\tcut -d \" \" -f 2 > ../stdin.act &&\n>>> +\tcd .. &&\n>>> +\ttest_cmp stdin.exp stdin.act\n>>> +'\n>>\n>> - Do not chdir around without being in a subprocess ();\n>\n> Sorry, I didn't realize the tests were eval-ed in the current\n> environment. I will correct all such problems in the next version.\n>\n>> - Do not place the command you are testing that might crash on the\n>> upstream of the pipe;\n>>\n>> - style;\n>\n> Noted.\n>\n>> \t(\n>> \t\tcd client &&\n>> git fetch-pack ... <../stdin.exp >stdin.raw\n>> \t) &&\n>> \tcut -d \" \" -f 2 stdin.act &&\n>> \ttest_cmp stdin.exp stdin.act\n>>\n>> By the way, why are these not called \"expect\" and \"actual\" like most other\n>> tests?\n>\n> The test files I worked with used the shorter exp/act convention so I\n> followed that.\n>\n> Or are you wondering about the \"stdin.\" prefix I added?\n\nNo. I was referring to just that these two files were not literally named\n\"expect\"/\"actual\", and I was lazy to look beyond what was in the patch\ncontext ;-). If the surrounding tests uses exp/act, mimicking them in\nthis patch is a good idea (we may want to fix them later but that is a\nseparate topic, and should not be done in this patch).\n\nThanks.\n"},{"id":"187906","messageId":"4F7251BB.5010406@gmx.net","threadId":"29986","inReplyTo":"7vwr65y6b6.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH/RFC v2 1/4] fetch-pack: new --stdin option to read refs from stdin","fromName":"Ivan Todoroski","fromEmail":"grnch@gmx.net","sentAt":"2012-03-27T23:48:11Z","receivedAt":"2012-03-27T23:48:11Z","isPatch":true,"sender":{"key":"grnch@gmx.net","avatar":"https://avatars.githubusercontent.com/u/90292?v=4"},"body":"On 28.03.2012 01:26, Junio C Hamano wrote:\n> Ivan Todoroski writes:\n> \n>>> Curious. \"stop at EOF\", \"trim\" and \"skip empty\" imply that you are\n>>> catering to people who debug this from the terminal by typing (or copy\n>>> pasting). Is that the expected use case?\n>> The expected use case is people using this from shell scripts that\n>> could be getting refs by slicing and dicing output of other commands\n>> with regexps and what not\n> \n> That kind of use would not have leading or trailing whitespaces, and if\n> the target audience is scripts, I would prefer to force them to be\n> strict.\n\nDepends on how good the script writer is. :)\n\nBut you're right, they can always change the script to tighten up what \nit generates.\n\nAlso, I just checked what other commands with --stdin flags do (like \nrev-list, checkout-index, etc). They all expect an exact format like you \nsaid and they don't tolerate extra whitespace, making my patch the odd \none out. It is important that similar flags behave consistently across \ndifferent commands.\n\nSorry for not checking for precedent before deciding to tolerate \nwhitespace. I will tighten it up.\n"},{"id":"187907","messageId":"4F7257F5.6070709@gmx.net","threadId":"29986","inReplyTo":"4F724F0D.7000602@gmx.net","subject":"Re: [PATCH/RFC v2 3/4] fetch-pack: test cases for the new --stdin option","fromName":"Ivan Todoroski","fromEmail":"grnch_lists@gmx.net","sentAt":"2012-03-28T00:14:45Z","receivedAt":"2012-03-28T00:14:45Z","isPatch":true,"sender":{"key":"grnch_lists@gmx.net","avatar":null},"body":"On 28.03.2012 01:36, Ivan Todoroski wrote:\n> On 27.03.2012 19:40, Junio C Hamano wrote:\n>> \n>> It is sensible to expect that we see all the refs we told it to\n>> fetch, but I do not think it is sensible to require they come in\n>> the same order as we have given them to the command.\n> \n> Oh, OK. I was not aware that fetch-pack is free to reorder commits\n> (at least in principle). I will adjust the tests to be\n> order-independent.\n\ns/reorder commits/reorder refs for fetching/\n\nI didn't mean actually reordering commits in history, of course.\n"}]}