{"thread":{"id":"13797","subject":"[RFC] http clone does not checkout working tree","startedAt":"2008-06-04T18:38:58Z","lastAt":"2008-06-04T20:10:08Z","messageCount":7,"participants":["Jeff King","Linus Torvalds","Junio C Hamano","Daniel Barkalow"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"78647","messageId":"20080604183858.GA7095@sigill.intra.peff.net","threadId":"13797","inReplyTo":null,"subject":"[RFC] http clone does not checkout working tree","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-06-04T18:38:58Z","receivedAt":"2008-06-04T18:38:58Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If I try cloning a repository by http with the current 'master', I end\nup with a proper git directory, but nothing gets checked out in the\nworking tree [I see the problem with any repository; I randomly picked\nthe one in the example below because it is small, and http clone can be\npainfully slow]:\n\n  $ git clone http://repo.or.cz/r/bigint.git\n  [...]\n  $ cd bigint\n  $ ls\n  $\n\nI think the problem is that the builtin-clone now does the http commit\nwalking and the tree unpacking in the same process, and the commit\nwalker leaves the in-core objects in a funny state.\n\nSpecifically, the top-level tree object has its \"parsed\" flag set to 1,\nbut the buffer and size members are set to NULL and 0, respectively.\nThis happens in walker.c:process_tree, where we free the tree buffer\nafter walking it. It seems like we are violating the meaning of \"parsed\"\nhere, which implies that those other members are valid.\n\nThe following patch fixes it for me, but I really have no idea if there\nisn't something more subtle at work. Sending to Linus, since \"git blame\"\npoints the surrounding code to you, and to Daniel, since the new clone\nand the commit walker are your areas.\n\n---\ndiff --git a/walker.c b/walker.c\nindex 31de6c1..0e68ee6 100644\n--- a/walker.c\n+++ b/walker.c\n@@ -59,6 +59,7 @@ static int process_tree(struct walker *walker, struct tree *tree)\n \tfree(tree->buffer);\n \ttree->buffer = NULL;\n \ttree->size = 0;\n+\ttree->object.parsed = 0;\n \treturn 0;\n }\n \n"},{"id":"78649","messageId":"alpine.LFD.1.10.0806041145170.3473@woody.linux-foundation.org","threadId":"13797","inReplyTo":"20080604183858.GA7095@sigill.intra.peff.net","subject":"Re: [RFC] http clone does not checkout working tree","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-06-04T18:59:10Z","receivedAt":"2008-06-04T18:59:10Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Wed, 4 Jun 2008, Jeff King wrote:\n> \n> The following patch fixes it for me, but I really have no idea if there\n> isn't something more subtle at work. Sending to Linus, since \"git blame\"\n> points the surrounding code to you, and to Daniel, since the new clone\n> and the commit walker are your areas.\n\nAck. This is correct.\n\nThat said, a *lot* of code does this pattern (freeing the tree buffer \nafter use, without marking the tree non-parsed), and I suspect the only \nreason I'm blamed is because this got copied from some other use of that \nsame model. \n\nNormally it's fine, because the whole object use is temporary, but as you \npoint out, doing things in the same process will re-use old object info. \nIt's one of the subtler implications of doing built-ins without fork/exec.\n\nIt is possible that we should clean this up by adding some kind of\n\n\tstatic void forget_tree(struct tree *tree)\n\t{\n\t\tfree(tree->buffer);\n\t\ttree->buffer = NULL;\n\t\ttree->size = 0;\n\t\ttree->parsed = 0;\n\t}\n\nto make this more robust and obvious. That said, a lot of the users are \nbasically the type\n\n\tif (parse_tree(tree) < 0)\n\t\tdie(..);\n\tinit_tree_desc(&desc, tree->buffer, tree->size);\n\twhile (tree_entry(&desc, &entry)) {\n\t\t...\n\t}\n\tforget_tree();\n\nand quite frankly, it's rather possible that we should get rid of the \n\"void *buffer\" and \"unsigned long size\" in the tree *entirely*, because \nthe above would likely be better written as\n\n\tbuffer = read_tree_desc(&desc);\n\twhile (tree_entry(&desc, &entry)) {\n\t\t...\n\t}\n\tfree(buffer);\n\nand make \"struct tree\" smaller, and not ever need parsing at all!\n\nI think that realisitcially, all tree users are of that format, and nobody \nreally wants to save the buffer (because buffer re-use is fairly unlikely, \nan re-generating it isn't all that expensive).\n\nBut that would be a much bigger patch. And maybe I'm wrong, and some uses \nreally do want the longer-term caching because they end up re-using the \ntree a lot. So it would need more thinking about.\n\n\t\tLinus\n"},{"id":"78650","messageId":"7vwsl5vxa5.fsf@gitster.siamese.dyndns.org","threadId":"13797","inReplyTo":"20080604183858.GA7095@sigill.intra.peff.net","subject":"Re: [RFC] http clone does not checkout working tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-04T19:02:26Z","receivedAt":"2008-06-04T19:02:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> The following patch fixes it for me, but I really have no idea if there\n> isn't something more subtle at work. Sending to Linus, since \"git blame\"\n> points the surrounding code to you, and to Daniel, since the new clone\n> and the commit walker are your areas.\n>\n> ---\n> diff --git a/walker.c b/walker.c\n> index 31de6c1..0e68ee6 100644\n> --- a/walker.c\n> +++ b/walker.c\n> @@ -59,6 +59,7 @@ static int process_tree(struct walker *walker, struct tree *tree)\n>  \tfree(tree->buffer);\n>  \ttree->buffer = NULL;\n>  \ttree->size = 0;\n> +\ttree->object.parsed = 0;\n>  \treturn 0;\n>  }\n>  \n\nThe patch looks good to me.\n\nAnd blaming Linus for this is slightly unfair, as the context the original\n\"process_tree()\" taken out of was that tree object was used once and then\nnever used after this codepath is done with it, even though he _could_\nhave a perfect foresight to anticipate that someday somebody might want to\ncall the routine from elsewhere without understanding the implications.\n"},{"id":"78653","messageId":"20080604191619.GA17327@sigill.intra.peff.net","threadId":"13797","inReplyTo":"7vwsl5vxa5.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFC] http clone does not checkout working tree","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-06-04T19:16:19Z","receivedAt":"2008-06-04T19:16:19Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 04, 2008 at 12:02:26PM -0700, Junio C Hamano wrote:\n\n> And blaming Linus for this is slightly unfair, as the context the original\n\nRight. I didn't mean to blame Linus so much as I had the notion that he\nwould have some clue that there wasn't something trickier going on.\n\nMaybe I should have said \"git annotate told me that...\" ;)\n\n-Peff\n"},{"id":"78655","messageId":"alpine.LFD.1.10.0806041222300.3473@woody.linux-foundation.org","threadId":"13797","inReplyTo":"alpine.LFD.1.10.0806041145170.3473@woody.linux-foundation.org","subject":"Re: [RFC] http clone does not checkout working tree","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-06-04T19:30:57Z","receivedAt":"2008-06-04T19:30:57Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Wed, 4 Jun 2008, Linus Torvalds wrote:\n> \n> and quite frankly, it's rather possible that we should get rid of the \n> \"void *buffer\" and \"unsigned long size\" in the tree *entirely*, because \n> the above would likely be better written as\n..\n\nSide note: the actual historical context here is that \"parse_tree()\" used \nto create that \"tree_entry_list\" of all the entries in the tree. So we \nused to do things like\n\n\tstruct tree_entry_list *list;\n\n\tif (parse_tree(tree))\n\t\tdie(..)\n\tlist = tree->entries;\n\twhile (list) {\n\t\t...\n\nso \"parse_tree()\" was something much bigger (and generated much slower and \nless dense data structures).\n\nThese days, parse_tree() basically just reads the object buffer and \nlength. So it boils down to just caching the result of \"read_sha1_file()\", \nbut we have all those legacy uses that come from the old historical thing. \nAnd to some degree it may have made sense to drop the buffer, but keep the \nactual list of entries in that old model.\n\nSee commit 2d9c58c69d1bab601e67b036d0546e85abcee7eb.\n\n\t\tLinus\n"},{"id":"78657","messageId":"alpine.LNX.1.00.0806041535230.19665@iabervon.org","threadId":"13797","inReplyTo":"alpine.LFD.1.10.0806041222300.3473@woody.linux-foundation.org","subject":"Re: [RFC] http clone does not checkout working tree","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-06-04T19:59:34Z","receivedAt":"2008-06-04T19:59:34Z","isPatch":false,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Wed, 4 Jun 2008, Linus Torvalds wrote:\n\n> On Wed, 4 Jun 2008, Linus Torvalds wrote:\n> > \n> > and quite frankly, it's rather possible that we should get rid of the \n> > \"void *buffer\" and \"unsigned long size\" in the tree *entirely*, because \n> > the above would likely be better written as\n> ..\n> \n> Side note: the actual historical context here is that \"parse_tree()\" used \n> to create that \"tree_entry_list\" of all the entries in the tree. So we \n> used to do things like\n> \n> \tstruct tree_entry_list *list;\n> \n> \tif (parse_tree(tree))\n> \t\tdie(..)\n> \tlist = tree->entries;\n> \twhile (list) {\n> \t\t...\n> \n> so \"parse_tree()\" was something much bigger (and generated much slower and \n> less dense data structures).\n> \n> These days, parse_tree() basically just reads the object buffer and \n> length. So it boils down to just caching the result of \"read_sha1_file()\", \n> but we have all those legacy uses that come from the old historical thing. \n> And to some degree it may have made sense to drop the buffer, but keep the \n> actual list of entries in that old model.\n> \n> See commit 2d9c58c69d1bab601e67b036d0546e85abcee7eb.\n\nI think the lineage of walker.c is to the version in fetch.c, which, in \nthat commit, you added the same \"drop the buffer\" optimization from \nrev-list. If the old style ever dropped the buffer, it would have been in \nparse_tree_buffer() and you'd have had to remove it to get the new style \nto work at all.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"78664","messageId":"alpine.LNX.1.00.0806041559390.19665@iabervon.org","threadId":"13797","inReplyTo":"alpine.LFD.1.10.0806041145170.3473@woody.linux-foundation.org","subject":"Re: [RFC] http clone does not checkout working tree","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-06-04T20:10:08Z","receivedAt":"2008-06-04T20:10:08Z","isPatch":false,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Wed, 4 Jun 2008, Linus Torvalds wrote:\n\n> On Wed, 4 Jun 2008, Jeff King wrote:\n> > \n> > The following patch fixes it for me, but I really have no idea if there\n> > isn't something more subtle at work. Sending to Linus, since \"git blame\"\n> > points the surrounding code to you, and to Daniel, since the new clone\n> > and the commit walker are your areas.\n> \n> Ack. This is correct.\n> \n> That said, a *lot* of code does this pattern (freeing the tree buffer \n> after use, without marking the tree non-parsed), and I suspect the only \n> reason I'm blamed is because this got copied from some other use of that \n> same model. \n> \n> Normally it's fine, because the whole object use is temporary, but as you \n> point out, doing things in the same process will re-use old object info. \n> It's one of the subtler implications of doing built-ins without fork/exec.\n> \n> It is possible that we should clean this up by adding some kind of\n> \n> \tstatic void forget_tree(struct tree *tree)\n> \t{\n> \t\tfree(tree->buffer);\n> \t\ttree->buffer = NULL;\n> \t\ttree->size = 0;\n> \t\ttree->parsed = 0;\n> \t}\n> \n> to make this more robust and obvious. That said, a lot of the users are \n> basically the type\n> \n> \tif (parse_tree(tree) < 0)\n> \t\tdie(..);\n> \tinit_tree_desc(&desc, tree->buffer, tree->size);\n> \twhile (tree_entry(&desc, &entry)) {\n> \t\t...\n> \t}\n> \tforget_tree();\n> \n> and quite frankly, it's rather possible that we should get rid of the \n> \"void *buffer\" and \"unsigned long size\" in the tree *entirely*, because \n> the above would likely be better written as\n> \n> \tbuffer = read_tree_desc(&desc);\n\nread_tree_desc(&desc, tree), which would use the hash in the tree object, \nwhich wouldn't need to be parse, I assume.\n\n> \twhile (tree_entry(&desc, &entry)) {\n> \t\t...\n> \t}\n> \tfree(buffer);\n> \n> and make \"struct tree\" smaller, and not ever need parsing at all!\n> \n> I think that realisitcially, all tree users are of that format, and nobody \n> really wants to save the buffer (because buffer re-use is fairly unlikely, \n> an re-generating it isn't all that expensive).\n>\n> But that would be a much bigger patch. And maybe I'm wrong, and some uses \n> really do want the longer-term caching because they end up re-using the \n> tree a lot. So it would need more thinking about.\n\nI think that your current unpack_trees() benefits from the fact that, if \nsome but not all of the trees are the same, you only uncompress the tree \nobjects once and store one copy of the resulting buffer. So, I don't think \nlong-term caching is worthwhile, but sharing between concurrent users \nmight be.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"}]}