{"thread":{"id":"7502","subject":"git-index-pack really does suck..","startedAt":"2007-04-03T15:15:12Z","lastAt":"2007-04-06T22:55:10Z","messageCount":58,"participants":["Linus Torvalds","Nicolas Pitre","Chris Lee","Junio C Hamano","Shawn O. Pearce","Jeff King","Dana How","David Lang","Alex Riesen"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"38502","messageId":"Pine.LNX.4.64.0704030754020.6730@woody.linux-foundation.org","threadId":"7502","inReplyTo":null,"subject":"git-index-pack really does suck..","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-04-03T15:15:12Z","receivedAt":"2007-04-03T15:15:12Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nJunio, Nico,\n I think we need to do something about it.\n\nCLee was complaining about git-index-pack on #irc with the partial KDE \nrepo, and while I don't have the KDE repo, I decided to investigate a bit.\n\nEven with just the kernel repo (with a single 170MB pack-file), I can do\n\n\tgit index-pack --stdin --fix-thin new.pack < .git/objects/pack/pack-*.pack\n\nand it uses 52s of CPU-time, and on my 4GB machine it actually started \ndoing IO and swapping, because git-index-pack grew to 4.8GB in size. So \nwhile I initially thought I'd want a bigger test-case to see the problem, \nI sure as heck don't.\n\nThe 52s of CPU time exploded into almost three minutes of actual \nreal-time:\n\n\t47.33user 5.79system 2:41.65elapsed 32%CPU\n\t2117major+1245763minor\n\nAnd that's on a good system with a powerful CPU, \"enough memory\" for any \nreasonable development, and good disks! Very much ungood-plus-plus.\n\nI haven't looked into exactly why yet, but I bet it's just that we keep \nevery single object expanded in memory. We do need to keep the objects \naround, so that we can resolve delta's, but we can certainly do it other \nways. \n\nTwo suggestion for other ways:\n\n - simple one: don't keep unexploded objects around, just keep the deltas, \n and spend tons of CPU-time just re-expanding them if required.\n\n We *should* be able to do it with just keeping the original 170MB \n pack-file in memory, not expanding it to 3.8GB! \n\n Still, even this will be painful once you have a big pack-file, and the \n CPU waste is nasty (although a delta-base cache like we do in \n sha1_file.c would probably fix it 99% - at that point it's getting \n less simple, and the \"best\" solution below looks more palatable)\n\n - best one: when writing out the pack-file, we incrementally keep a \n \"struct packed_git\" around, and update the index for it dynamically, \n and totally get rid of all objects that we've written out, because we \n can re-create them.\n\n This means that we should have _zero_ memory footprint except for the \n one object that we're working on right then and there, and any \n unresolved deltas where we've not seen the base at all (and the latter \n generally shouldn't happen any more with most pack-files)\n\nThe \"best one\" wouldn't seem to be *that* painful, but as mentioned, I \nhaven't even started looking at the code yet, I thought I'd try to rope \nNico into looking at this first ;)\n\n\t\tLinus\n"},{"id":"38504","messageId":"Pine.LNX.4.64.0704030913060.6730@woody.linux-foundation.org","threadId":"7502","inReplyTo":"Pine.LNX.4.64.0704030754020.6730@woody.linux-foundation.org","subject":"Re: git-index-pack really does suck..","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-04-03T16:21:26Z","receivedAt":"2007-04-03T16:21:26Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 3 Apr 2007, Linus Torvalds wrote:\n> \n> and it uses 52s of CPU-time, and on my 4GB machine it actually started \n> doing IO and swapping, because git-index-pack grew to 4.8GB in size.\n\nAhh. False alarm.\n\nThe problem is actually largely a really stupid memory leak in the SHA1 \ncollision checking (which wouldn't trigger on a normal pull, but obviously \ndoes trigger for every single object when testing!)\n\nThis trivial patch fixes most of it. git-index-pack still uses too much \nmemory, but it does a *lot* better.\n\nJunio, please get this into 1.5.1 (I *think* the SHA1 checking is new, but \nif it exists in 1.5.0 too, it obviously needs the same fix).\n\nIt still grows, but it grew to just 287M in size now for the 170M kernel \nobject:\n\n\t41.59user 1.39system 0:43.64elapsed\n\t0major+73552minor\n\nwhich is quite a lot better.\n\nDuh.\n\n\t\tLinus\n\n---\n index-pack.c | 1 +\n 1 files changed, 1 insertions(+), 0 deletions(-)\n\ndiff --git a/index-pack.c b/index-pack.c\nindex 6284fe3..3c768fb 100644\n--- a/index-pack.c\n+++ b/index-pack.c\n@@ -358,6 +358,7 @@ static void sha1_object(const void *data, unsigned long size,\n \t\tif (size != has_size || type != has_type ||\n \t\t memcmp(data, has_data, size) != 0)\n \t\t\tdie(\"SHA1 COLLISION FOUND WITH %s !\", sha1_to_hex(sha1));\n+\t\tfree(has_data);\n \t}\n }\n \n"},{"id":"38507","messageId":"alpine.LFD.0.98.0704031220470.28181@xanadu.home","threadId":"7502","inReplyTo":"Pine.LNX.4.64.0704030754020.6730@woody.linux-foundation.org","subject":"Re: git-index-pack really does suck..","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-04-03T16:33:46Z","receivedAt":"2007-04-03T16:33:46Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 3 Apr 2007, Linus Torvalds wrote:\n\n> \n> Junio, Nico,\n> I think we need to do something about it.\n\nSure. Mea culpa.\n\n> CLee was complaining about git-index-pack on #irc with the partial KDE \n> repo, and while I don't have the KDE repo, I decided to investigate a bit.\n> \n> Even with just the kernel repo (with a single 170MB pack-file), I can do\n> \n> \tgit index-pack --stdin --fix-thin new.pack < .git/objects/pack/pack-*.pack\n> \n> and it uses 52s of CPU-time, and on my 4GB machine it actually started \n> doing IO and swapping, because git-index-pack grew to 4.8GB in size.\n\nRight.\n\n> Two suggestion for other ways:\n> \n> - simple one: don't keep unexploded objects around, just keep the deltas, \n> and spend tons of CPU-time just re-expanding them if required.\n> \n> We *should* be able to do it with just keeping the original 170MB \n> pack-file in memory, not expanding it to 3.8GB! \n> \n> Still, even this will be painful once you have a big pack-file, and the \n> CPU waste is nasty (although a delta-base cache like we do in \n> sha1_file.c would probably fix it 99% - at that point it's getting \n> less simple, and the \"best\" solution below looks more palatable)\n> \n> - best one: when writing out the pack-file, we incrementally keep a \n> \"struct packed_git\" around, and update the index for it dynamically, \n> and totally get rid of all objects that we've written out, because we \n> can re-create them.\n> \n> This means that we should have _zero_ memory footprint except for the \n> one object that we're working on right then and there, and any \n> unresolved deltas where we've not seen the base at all (and the latter \n> generally shouldn't happen any more with most pack-files)\n\nEven better:\n\n - Fix my own stupid mistake with a _single_ line of code:\n\ndiff --git a/index-pack.c b/index-pack.c\nindex 6284fe3..3c768fb 100644\n--- a/index-pack.c\n+++ b/index-pack.c\n@@ -358,6 +358,7 @@ static void sha1_object(const void *data, unsigned long size,\n \t\tif (size != has_size || type != has_type ||\n \t\t memcmp(data, has_data, size) != 0)\n \t\t\tdie(\"SHA1 COLLISION FOUND WITH %s !\", sha1_to_hex(sha1));\n+\t\tfree(has_data);\n \t}\n }\n\nThe thing is, that code path is executed _only_ when index-pack is \nencountering an object already in the repository in order to protect \nagainst possible SHA1 collision attacks. See commit 8685da42561d log \nfor the full story.\n\nNormally this should not happen in normal usage scenarios because the \nobjects you fetch are those that you don't already have. But if you \nmanually run index-pack inside an existing repository then you'll \nalready have _all_ those objects already explaining the high CPU usage.\n\nBut this is no excuse for not freeing the data though.\n\n\nNicolas\n"},{"id":"38508","messageId":"alpine.LFD.0.98.0704031235490.28181@xanadu.home","threadId":"7502","inReplyTo":"Pine.LNX.4.64.0704030913060.6730@woody.linux-foundation.org","subject":"Re: git-index-pack really does suck..","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-04-03T16:40:14Z","receivedAt":"2007-04-03T16:40:14Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 3 Apr 2007, Linus Torvalds wrote:\n\n> \n> \n> On Tue, 3 Apr 2007, Linus Torvalds wrote:\n> > \n> > and it uses 52s of CPU-time, and on my 4GB machine it actually started \n> > doing IO and swapping, because git-index-pack grew to 4.8GB in size.\n> \n> Ahh. False alarm.\n> \n> The problem is actually largely a really stupid memory leak in the SHA1 \n> collision checking (which wouldn't trigger on a normal pull, but obviously \n> does trigger for every single object when testing!)\n\nDamn!\n\nPlease don't report those things when I'm out for lunch so I could have \na chance to fix my own stupidities myself in time! :-)\n\n> This trivial patch fixes most of it. git-index-pack still uses too much \n> memory, but it does a *lot* better.\n> \n> Junio, please get this into 1.5.1 (I *think* the SHA1 checking is new, but \n> if it exists in 1.5.0 too, it obviously needs the same fix).\n\nNo, it is new to 1.5.1.\n\n> It still grows, but it grew to just 287M in size now for the 170M kernel \n> object:\n> \n> \t41.59user 1.39system 0:43.64elapsed\n> \t0major+73552minor\n> \n> which is quite a lot better.\n> \n> Duh.\n\nIndeed.\n\nAcked-by: Nicolas Pitre \n\n> index-pack.c | 1 +\n> 1 files changed, 1 insertions(+), 0 deletions(-)\n> \n> diff --git a/index-pack.c b/index-pack.c\n> index 6284fe3..3c768fb 100644\n> --- a/index-pack.c\n> +++ b/index-pack.c\n> @@ -358,6 +358,7 @@ static void sha1_object(const void *data, unsigned long size,\n> \t\tif (size != has_size || type != has_type ||\n> \t\t memcmp(data, has_data, size) != 0)\n> \t\t\tdie(\"SHA1 COLLISION FOUND WITH %s !\", sha1_to_hex(sha1));\n> +\t\tfree(has_data);\n> \t}\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\nNicolas\n"},{"id":"38519","messageId":"db69205d0704031227q1009eabfhdd82aa3636f25bb6@mail.gmail.com","threadId":"7502","inReplyTo":"Pine.LNX.4.64.0704030754020.6730@woody.linux-foundation.org","subject":"Re: git-index-pack really does suck..","fromName":"Chris Lee","fromEmail":"clee@kde.org","sentAt":"2007-04-03T19:27:05Z","receivedAt":"2007-04-03T19:27:05Z","isPatch":false,"sender":{"key":"clee@kde.org","avatar":"https://gravatar.com/avatar/c930bdc8cc6465094a5722188409ecb8955e0da2b188d7340137074b08f857e3?d=mp&s=160"},"body":"On 4/3/07, Linus Torvalds wrote:\n>\n> Junio, Nico,\n> I think we need to do something about it.\n>\n> CLee was complaining about git-index-pack on #irc with the partial KDE\n> repo, and while I don't have the KDE repo, I decided to investigate a bit.\n>\n> Even with just the kernel repo (with a single 170MB pack-file), I can do\n>\n> git index-pack --stdin --fix-thin new.pack < .git/objects/pack/pack-*.pack\n>\n> and it uses 52s of CPU-time, and on my 4GB machine it actually started\n> doing IO and swapping, because git-index-pack grew to 4.8GB in size. So\n> while I initially thought I'd want a bigger test-case to see the problem,\n> I sure as heck don't.\n\nThere's another issue here.\n\nI'm running git-index-pack as part of a workflow like so:\n\n$ git-verify-pack -v .git/objects/pack/*.idx > /tmp/all-objects\n$ grep 'blob' /tmp/all-objects > /tmp/blob-objects\n$ cat /tmp/blob-objects | awk '{print $1;}' | git-pack-objects\n--delta-base-offset --all-progress --stdout > blob.pack\n$ git-index-pack -v blob.pack\n\nNow, when I run 'git-index-pack' on blob.pack in the current\ndirectory, memory usage is pretty horrific (even with the applied\npatch to not leak all everything). Shawn tells me that index-pack\nshould only be decompressing the object twice - once from the repo and\nonce from blob.pack - iff I call git-index-pack with --stdin, which I\nam not.\n\nIf I move the blob.pack into /tmp, and run git-index-pack on it there,\nit completes much faster and the memory usage never exceeds 200MB.\n(Inside the repo, it takes up over 3G of RES according to top.)\n\nBy \"much faster\", I mean: the entire pack was indexed and completed in\n17:42.40, whereas I cancelled the inside-the-repo index because after\n56 minutes it was only at 46%.\n\n(And, as far as getting this huge repo published - I have it burned to\na DVD, and I'm going to drop it in the mail to hpa today on my lunch\nbreak, which I should be taking soon.)\n\n-clee\n"},{"id":"38525","messageId":"alpine.LFD.0.98.0704031540140.28181@xanadu.home","threadId":"7502","inReplyTo":"db69205d0704031227q1009eabfhdd82aa3636f25bb6@mail.gmail.com","subject":"Re: git-index-pack really does suck..","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-04-03T19:49:03Z","receivedAt":"2007-04-03T19:49:03Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 3 Apr 2007, Chris Lee wrote:\n\n> There's another issue here.\n> \n> I'm running git-index-pack as part of a workflow like so:\n> \n> $ git-verify-pack -v .git/objects/pack/*.idx > /tmp/all-objects\n> $ grep 'blob' /tmp/all-objects > /tmp/blob-objects\n> $ cat /tmp/blob-objects | awk '{print $1;}' | git-pack-objects\n> --delta-base-offset --all-progress --stdout > blob.pack\n> $ git-index-pack -v blob.pack\n\nInstead of using --stdout with git-pack-object, you should provide it \nwith a suitable base name for the resulting pack and the index will be \ncreated automatically along side the pack for you. No need to use \nindex-pack for that.\n\n> Now, when I run 'git-index-pack' on blob.pack in the current\n> directory, memory usage is pretty horrific (even with the applied\n> patch to not leak all everything). Shawn tells me that index-pack\n> should only be decompressing the object twice - once from the repo and\n> once from blob.pack - iff I call git-index-pack with --stdin, which I\n> am not.\n> \n> If I move the blob.pack into /tmp, and run git-index-pack on it there,\n> it completes much faster and the memory usage never exceeds 200MB.\n> (Inside the repo, it takes up over 3G of RES according to top.)\n\nThe 3G should definitely be fixed with the added free().\n\nThe CPU usage is explained by the fact that you're running index-pack on \nobjects that are all already found in your repo so the collision check \nis triggered. This is more or like the same issue as if you tried to \nrun unpack-objects on the same pack where none of your objects will \nactually be unpacked.\n\n\nNicolas\n"},{"id":"38527","messageId":"db69205d0704031254s23460558ycb9715362768be16@mail.gmail.com","threadId":"7502","inReplyTo":"alpine.LFD.0.98.0704031540140.28181@xanadu.home","subject":"Re: git-index-pack really does suck..","fromName":"Chris Lee","fromEmail":"clee@kde.org","sentAt":"2007-04-03T19:54:18Z","receivedAt":"2007-04-03T19:54:18Z","isPatch":false,"sender":{"key":"clee@kde.org","avatar":"https://gravatar.com/avatar/c930bdc8cc6465094a5722188409ecb8955e0da2b188d7340137074b08f857e3?d=mp&s=160"},"body":"On 4/3/07, Nicolas Pitre wrote:\n> On Tue, 3 Apr 2007, Chris Lee wrote:\n>\n> > There's another issue here.\n> >\n> > I'm running git-index-pack as part of a workflow like so:\n> >\n> > $ git-verify-pack -v .git/objects/pack/*.idx > /tmp/all-objects\n> > $ grep 'blob' /tmp/all-objects > /tmp/blob-objects\n> > $ cat /tmp/blob-objects | awk '{print $1;}' | git-pack-objects\n> > --delta-base-offset --all-progress --stdout > blob.pack\n> > $ git-index-pack -v blob.pack\n>\n> Instead of using --stdout with git-pack-object, you should provide it\n> with a suitable base name for the resulting pack and the index will be\n> created automatically along side the pack for you. No need to use\n> index-pack for that.\n\nRight. But then I wouldn't have discovered how much git-index-pack sucks. :)\n\n> > Now, when I run 'git-index-pack' on blob.pack in the current\n> > directory, memory usage is pretty horrific (even with the applied\n> > patch to not leak all everything). Shawn tells me that index-pack\n> > should only be decompressing the object twice - once from the repo and\n> > once from blob.pack - iff I call git-index-pack with --stdin, which I\n> > am not.\n> >\n> > If I move the blob.pack into /tmp, and run git-index-pack on it there,\n> > it completes much faster and the memory usage never exceeds 200MB.\n> > (Inside the repo, it takes up over 3G of RES according to top.)\n>\n> The 3G should definitely be fixed with the added free().\n\nNot really. This packfile is 2.6GB in size, and apparently it gets mmap'd.\n\n(Yesterday, my machine ran out of memory trying to do index-pack when\nthe memleak still existed; I have 4G of RAM and, normally, 4G of swap,\nbut I upped it to 32G of swap and it still ran out of memory.)\n\n> The CPU usage is explained by the fact that you're running index-pack on\n> objects that are all already found in your repo so the collision check\n> is triggered. This is more or like the same issue as if you tried to\n> run unpack-objects on the same pack where none of your objects will\n> actually be unpacked.\n\nRight, and if I was using --stdin, I would expect that. But I'm not.\nAnd, according to Shawn anyway, the current behaviour is not what was\nintended.\n\n-clee\n"},{"id":"38530","messageId":"Pine.LNX.4.64.0704031304420.6730@woody.linux-foundation.org","threadId":"7502","inReplyTo":"db69205d0704031227q1009eabfhdd82aa3636f25bb6@mail.gmail.com","subject":"Re: git-index-pack really does suck..","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-04-03T20:18:33Z","receivedAt":"2007-04-03T20:18:33Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 3 Apr 2007, Chris Lee wrote:\n> \n> There's another issue here.\n> \n> I'm running git-index-pack as part of a workflow like so:\n> \n> $ git-verify-pack -v .git/objects/pack/*.idx > /tmp/all-objects\n> $ grep 'blob' /tmp/all-objects > /tmp/blob-objects\n> $ cat /tmp/blob-objects | awk '{print $1;}' | git-pack-objects\n> --delta-base-offset --all-progress --stdout > blob.pack\n> $ git-index-pack -v blob.pack\n> \n> Now, when I run 'git-index-pack' on blob.pack in the current\n> directory, memory usage is pretty horrific (even with the applied\n> patch to not leak all everything). Shawn tells me that index-pack\n> should only be decompressing the object twice - once from the repo and\n> once from blob.pack - iff I call git-index-pack with --stdin, which I\n> am not.\n> \n> If I move the blob.pack into /tmp, and run git-index-pack on it there,\n> it completes much faster and the memory usage never exceeds 200MB.\n> (Inside the repo, it takes up over 3G of RES according to top.)\n\nYeah. What happens is that inside the repo, because we do all the \nduplicate object checks (verifying that there are no evil hash collisions) \neven after fixing the memory leak, we end up keeping *track* of all those \nobjects.\n\nAnd with a large repository, it's quite the expensive operation.\n\nThat whole \"verify no SHA1 hash collision\" code is really pretty damn \nparanoid. Maybe we shouldn't have it enabled by default.\n\nSo how about this updated patch? We could certainly make \"git pull\" imply \n\"--paranoid\" if we want to, but even that is likely pretty unnecessary. \nIt's not like anybody has ever shown a SHA1 collision, and if the *local* \nrepository is corrupt (and has an object with the wrong SHA1 - that's what \nthe testsuite checks for), then it's probably good to get the valid object \nfrom the remote..\n\nThis includes the previous one-liner, but also adds the \"--paranoid\" flag \nand fixes up the Documentation and tests to match.\n\nJunio, your choice, but regardless which one you choose:\n\n\tSigned-off-by: Linus Torvalds \n\nThanks,\n\n\t\tLinus\n\n---\n Documentation/git-index-pack.txt | 3 +--\n index-pack.c | 6 +++++-\n t/t5300-pack-object.sh | 4 ++--\n 3 files changed, 8 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt\nindex 2229ee8..7d8d33b 100644\n--- a/Documentation/git-index-pack.txt\n+++ b/Documentation/git-index-pack.txt\n@@ -8,8 +8,7 @@ git-index-pack - Build pack index file for an existing packed archive\n \n SYNOPSIS\n --------\n-'git-index-pack' [-v] [-o ] \n-'git-index-pack' --stdin [--fix-thin] [--keep] [-v] [-o ] []\n+'git-index-pack' [--stdin [--fix-thin] [--keep]] [-v] [--paranoid] [-o ] \n \n \n DESCRIPTION\ndiff --git a/index-pack.c b/index-pack.c\nindex 6284fe3..8a4c27a 100644\n--- a/index-pack.c\n+++ b/index-pack.c\n@@ -45,6 +45,7 @@ static int nr_resolved_deltas;\n \n static int from_stdin;\n static int verbose;\n+static int paranoid;\n \n static volatile sig_atomic_t progress_update;\n \n@@ -348,7 +349,7 @@ static void sha1_object(const void *data, unsigned long size,\n \t\t\tenum object_type type, unsigned char *sha1)\n {\n \thash_sha1_file(data, size, typename(type), sha1);\n-\tif (has_sha1_file(sha1)) {\n+\tif (paranoid && has_sha1_file(sha1)) {\n \t\tvoid *has_data;\n \t\tenum object_type has_type;\n \t\tunsigned long has_size;\n@@ -358,6 +359,7 @@ static void sha1_object(const void *data, unsigned long size,\n \t\tif (size != has_size || type != has_type ||\n \t\t memcmp(data, has_data, size) != 0)\n \t\t\tdie(\"SHA1 COLLISION FOUND WITH %s !\", sha1_to_hex(sha1));\n+\t\tfree(has_data);\n \t}\n }\n \n@@ -839,6 +841,8 @@ int main(int argc, char **argv)\n \t\tif (*arg == '-') {\n \t\t\tif (!strcmp(arg, \"--stdin\")) {\n \t\t\t\tfrom_stdin = 1;\n+\t\t\t} else if (!strcmp(arg, \"--paranoid\")) {\n+\t\t\t\tparanoid = 1;\n \t\t\t} else if (!strcmp(arg, \"--fix-thin\")) {\n \t\t\t\tfix_thin_pack = 1;\n \t\t\t} else if (!strcmp(arg, \"--keep\")) {\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex 35e036a..407c71e 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -262,7 +262,7 @@ test_expect_success \\\n \t\t.git/objects/c8/2de19312b6c3695c0c18f70709a6c535682a67'\n \n test_expect_failure \\\n- 'make sure index-pack detects the SHA1 collision' \\\n- 'git-index-pack -o bad.idx test-3.pack'\n+ 'make sure index-pack detects the SHA1 collision when paranoid' \\\n+ 'git-index-pack --paranoid -o bad.idx test-3.pack'\n \n test_done\n"},{"id":"38533","messageId":"alpine.LFD.0.98.0704031625050.28181@xanadu.home","threadId":"7502","inReplyTo":"Pine.LNX.4.64.0704031304420.6730@woody.linux-foundation.org","subject":"Re: git-index-pack really does suck..","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-04-03T20:32:25Z","receivedAt":"2007-04-03T20:32:25Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 3 Apr 2007, Linus Torvalds wrote:\n\n> \n> \n> On Tue, 3 Apr 2007, Chris Lee wrote:\n> > \n> > There's another issue here.\n> > \n> > I'm running git-index-pack as part of a workflow like so:\n> > \n> > $ git-verify-pack -v .git/objects/pack/*.idx > /tmp/all-objects\n> > $ grep 'blob' /tmp/all-objects > /tmp/blob-objects\n> > $ cat /tmp/blob-objects | awk '{print $1;}' | git-pack-objects\n> > --delta-base-offset --all-progress --stdout > blob.pack\n> > $ git-index-pack -v blob.pack\n> > \n> > Now, when I run 'git-index-pack' on blob.pack in the current\n> > directory, memory usage is pretty horrific (even with the applied\n> > patch to not leak all everything). Shawn tells me that index-pack\n> > should only be decompressing the object twice - once from the repo and\n> > once from blob.pack - iff I call git-index-pack with --stdin, which I\n> > am not.\n> > \n> > If I move the blob.pack into /tmp, and run git-index-pack on it there,\n> > it completes much faster and the memory usage never exceeds 200MB.\n> > (Inside the repo, it takes up over 3G of RES according to top.)\n> \n> Yeah. What happens is that inside the repo, because we do all the \n> duplicate object checks (verifying that there are no evil hash collisions) \n> even after fixing the memory leak, we end up keeping *track* of all those \n> objects.\n\nWhat do you mean?\n\n> And with a large repository, it's quite the expensive operation.\n> \n> That whole \"verify no SHA1 hash collision\" code is really pretty damn \n> paranoid. Maybe we shouldn't have it enabled by default.\n\nMaybe we shouldn't run index-pack on packs for which we _already_ have \nan index for which is the most likely reason for the collision check to \ntrigger in the first place.\n\nThis is in the same category as trying to run unpack-objects on a pack \nwithin a repository and wondering why it doesn't work.\n\n> So how about this updated patch? We could certainly make \"git pull\" imply \n> \"--paranoid\" if we want to, but even that is likely pretty unnecessary. \n\nI'm of the opinion that this patch is unnecessary. It only helps in \nbogus workflows to start with, and it makes the default behavior unsafe \n(unsafe from a paranoid pov, but still). And in the _normal_ workflow \nit should never trigger.\n\nSo I wouldn't merge it.\n\n\nNicolas\n"},{"id":"38537","messageId":"Pine.LNX.4.64.0704031322490.6730@woody.linux-foundation.org","threadId":"7502","inReplyTo":"Pine.LNX.4.64.0704031304420.6730@woody.linux-foundation.org","subject":"Re: git-index-pack really does suck..","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-04-03T20:33:24Z","receivedAt":"2007-04-03T20:33:24Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 3 Apr 2007, Linus Torvalds wrote:\n> \n> So how about this updated patch? We could certainly make \"git pull\" imply \n> \"--paranoid\" if we want to, but even that is likely pretty unnecessary. \n> It's not like anybody has ever shown a SHA1 collision, and if the *local* \n> repository is corrupt (and has an object with the wrong SHA1 - that's what \n> the testsuite checks for), then it's probably good to get the valid object \n> from the remote..\n\nSome trivial timings for indexing just the kernel pack..\n\nWithout --paranoid:\n\n\t24.61user 2.16system 0:27.04elapsed 99%CPU\n\t0major+14120minor pagefaults\n\nWith --paranoid:\n\n\t42.74user 3.04system 0:46.36elapsed 98%CPU\n\t0major+72768minor pagefaults\n\nso it's a noticeable CPU issue, but it's even more noticeable in memory \nusage (55MB vs 284MB - pagefaults give a good way to look at how much \nmemory really got allocated for the process).\n\nAll that extra memory is just for SHA1 commit ID information. \n\nNow, clearly the usage scenario here is a big odd (ie the case where we \nhave all the objects already), so in that sense this is very much a \nworst-case situation, and you simply shouldn't *do* something like this, \nbut at the same time, I'm just not convinced a very theoretical SHA1 \ncollision check is worth it. \n\nBtw, even if we don't have any of the objects, if you have tons and tons \nof objects and do a \"git pull\", just the *lookup* of the nonexistent \nobjects will be expensive: first we won't find it in any pack, then we'll \nlook at the loose objects, and then we'll look int he pack *again* due to \nthe race avoidance. So looking up nonexistent objects is actually pretty \nexpensive.\n\nIn fact, \"--paranoid\" takes one second more for me even totally outside of \na git repository, just because we waste so much time trying to look up \nnon-existent object files ;)\n\n\t\t\tLinus\n"},{"id":"38534","messageId":"7vbqi5w62c.fsf@assigned-by-dhcp.cox.net","threadId":"7502","inReplyTo":"Pine.LNX.4.64.0704031304420.6730@woody.linux-foundation.org","subject":"Re: git-index-pack really does suck..","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-04-03T20:34:19Z","receivedAt":"2007-04-03T20:34:19Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> That whole \"verify no SHA1 hash collision\" code is really pretty damn \n> paranoid. Maybe we shouldn't have it enabled by default.\n>\n> So how about this updated patch? We could certainly make \"git pull\" imply \n> \"--paranoid\" if we want to, but even that is likely pretty unnecessary. \n> It's not like anybody has ever shown a SHA1 collision, and if the *local* \n> repository is corrupt (and has an object with the wrong SHA1 - that's what \n> the testsuite checks for), then it's probably good to get the valid object \n> from the remote..\n\nI agree with that reasoning. We did not do paranoid in git-pull\nlong after we introduced the .keep thing anyway, so I do not\nthink the following patch is even needed, but I am throwing it\nout just for discussion.\n\n\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 06f4aec..c687f9f 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -522,6 +522,7 @@ static int get_pack(int xd[2])\n \n \tif (do_keep) {\n \t\t*av++ = \"index-pack\";\n+\t\t*av++ = \"--paranoid\";\n \t\t*av++ = \"--stdin\";\n \t\tif (!quiet && !no_progress)\n \t\t\t*av++ = \"-v\";\n"},{"id":"38536","messageId":"7vzm5pur7g.fsf@assigned-by-dhcp.cox.net","threadId":"7502","inReplyTo":"alpine.LFD.0.98.0704031625050.28181@xanadu.home","subject":"Re: git-index-pack really does suck..","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-04-03T20:40:35Z","receivedAt":"2007-04-03T20:40:35Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"[sorry, sent a message without finishing]\n\nNicolas Pitre writes:\n\n> Maybe we shouldn't run index-pack on packs for which we _already_ have \n> an index for which is the most likely reason for the collision check to \n> trigger in the first place.\n>\n> This is in the same category as trying to run unpack-objects on a pack \n> within a repository and wondering why it doesn't work.\n> ...\n> I'm of the opinion that this patch is unnecessary. It only helps in \n> bogus workflows to start with, and it makes the default behavior unsafe \n> (unsafe from a paranoid pov, but still). And in the _normal_ workflow \n> it should never trigger.\n\nHmmmm. You may have a point.\n\nSo maybe we should retitle this thread from \"git-index-pack\nreally does suck..\" to \"I used git-index-pack in a stupid way\"?\n \n"},{"id":"38538","messageId":"alpine.LFD.0.98.0704031639470.28181@xanadu.home","threadId":"7502","inReplyTo":"7vbqi5w62c.fsf@assigned-by-dhcp.cox.net","subject":"Re: git-index-pack really does suck..","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-04-03T20:53:57Z","receivedAt":"2007-04-03T20:53:57Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 3 Apr 2007, Junio C Hamano wrote:\n\n> Linus Torvalds writes:\n> \n> > That whole \"verify no SHA1 hash collision\" code is really pretty damn \n> > paranoid. Maybe we shouldn't have it enabled by default.\n> >\n> > So how about this updated patch? We could certainly make \"git pull\" imply \n> > \"--paranoid\" if we want to, but even that is likely pretty unnecessary. \n> > It's not like anybody has ever shown a SHA1 collision, and if the *local* \n> > repository is corrupt (and has an object with the wrong SHA1 - that's what \n> > the testsuite checks for), then it's probably good to get the valid object \n> > from the remote..\n> \n> I agree with that reasoning.\n\nFor the record, I don't agree. I stated why in my other email.\n\n> We did not do paranoid in git-pull long after we introduced the .keep \n> thing anyway,\n\nThat doesn't make it more \"correct\".\n\n> so I do not\n> think the following patch is even needed, but I am throwing it\n> out just for discussion.\n\n1) None of the objects in a pack should exist in the local repo when \n fetching, meaning that the paranoia code should not be executed \n normally.\n\n2) Running index-pack on a pack _inside_ a repository is a dubious thing \n to do with questionable usefulness already.\n\n3) It is unefficient to run pack-objects with --stdout just to feed the \n result to index-pack afterwards while repack-objects can create the \n index itself, which is the source of this discussion.\n \n4) I invite you to read the commit log for 8685da42561 where the \n _perception_ of GIT's security is discussed which led to the paranoia \n check, and sometimes the perception is more valuable than the \n reality, especially when it is free.\n\nTherefore Linus' patch and this one are working around the wrong issue \nas described in (3) IMHO.\n\nWhat could be done instead, if really really needed, is to have the \nparanoia test be made conditional on index-pack --stdin instead. But \nplease no bogus extra switches pretty please.\n\n\nNicolas\n"},{"id":"38539","messageId":"Pine.LNX.4.64.0704031346250.6730@woody.linux-foundation.org","threadId":"7502","inReplyTo":"alpine.LFD.0.98.0704031625050.28181@xanadu.home","subject":"Re: git-index-pack really does suck..","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-04-03T20:56:50Z","receivedAt":"2007-04-03T20:56:50Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 3 Apr 2007, Nicolas Pitre wrote:\n> > \n> > Yeah. What happens is that inside the repo, because we do all the \n> > duplicate object checks (verifying that there are no evil hash collisions) \n> > even after fixing the memory leak, we end up keeping *track* of all those \n> > objects.\n> \n> What do you mean?\n\nLook at what we have to do to look up a SHA1 object.. We create all the \nlookup infrastructure, we don't *just* read the object. The delta base \ncache is the most obvious one. \n\n> I'm of the opinion that this patch is unnecessary. It only helps in \n> bogus workflows to start with, and it makes the default behavior unsafe \n> (unsafe from a paranoid pov, but still). And in the _normal_ workflow \n> it should never trigger.\n\nActually, even in the normal workflow it will do all the extra unnecessary \nwork, if only because the lookup costs of *not* finding the entry.\n\nLookie here:\n\n - git index-pack of the *git* pack-file in the v2.6/linux directory (zero \n overlap of objects)\n\n With --paranoid:\n\n\t2.75user 0.37system 0:03.13elapsed 99%CPU\n\t0major+5583minor pagefaults\n\n Without --paranoid:\n\n\t2.55user 0.12system 0:02.68elapsed 99%CPU\n\t0major+2957minor pagefaults\n\nSee? That's the *normal* workflow. Zero objects found. 7% CPU overhead \nfrom just the unnecessary work, and almost twice as much memory used. Just \nfrom the index file lookup etc for a decent-sized project.\n\nNow, in the KDE situation, the *unnecessary* lookups will be about ten \ntimes more expensive, both on memory and CPU, just because the repository \nis about 20x the size. Even with no actual hits.\n\n\t\tLinus\n"},{"id":"38540","messageId":"Pine.LNX.4.64.0704031357470.6730@woody.linux-foundation.org","threadId":"7502","inReplyTo":"7vzm5pur7g.fsf@assigned-by-dhcp.cox.net","subject":"Re: git-index-pack really does suck..","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-04-03T21:00:55Z","receivedAt":"2007-04-03T21:00:55Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 3 Apr 2007, Junio C Hamano wrote:\n> \n> So maybe we should retitle this thread from \"git-index-pack\n> really does suck..\" to \"I used git-index-pack in a stupid way\"?\n\nSee my separate timing numbers, although I bet that Chris can give even \nbetter ones..\n\nChris, try applying my patch, and then inside the KDE repo you have, do\n\n\tgit index-pack --paranoid --stdin --fix-thin new.pack < ~/git/.git/objects/pack/pack-*.pack\n\n(ie index the objects of the *git* repository, not the KDE one). That \nshould approximate doing a fair-sized \"git pull\" - getting new objects. Do \nit with and without --paranoid, and time it.\n\nI bet that what I see as a 7% slowdown will be much bigger for you, just \nbecause the negative lookups will be all that much more expensive when you \nhave tons of objects.\n\n\t\t\tLinus\n"},{"id":"38541","messageId":"20070403210319.GH27706@spearce.org","threadId":"7502","inReplyTo":"Pine.LNX.4.64.0704031346250.6730@woody.linux-foundation.org","subject":"Re: git-index-pack really does suck..","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-04-03T21:03:19Z","receivedAt":"2007-04-03T21:03:19Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Linus Torvalds wrote:\n> Actually, even in the normal workflow it will do all the extra unnecessary \n> work, if only because the lookup costs of *not* finding the entry.\n> \n> Lookie here:\n> \n> - git index-pack of the *git* pack-file in the v2.6/linux directory (zero \n> overlap of objects)\n> \n> With --paranoid:\n> \n> \t2.75user 0.37system 0:03.13elapsed 99%CPU\n> \t0major+5583minor pagefaults\n> \n> Without --paranoid:\n> \n> \t2.55user 0.12system 0:02.68elapsed 99%CPU\n> \t0major+2957minor pagefaults\n> \n> See? That's the *normal* workflow. Zero objects found. 7% CPU overhead \n> from just the unnecessary work, and almost twice as much memory used. Just \n> from the index file lookup etc for a decent-sized project.\n\nOK, but what about that case with unpack-objects? Didn't we there\ndo all this work to also check for the object already existing?\nDuring update-index, write-tree and commit-tree don't we also do\na lot of work (per object anyway) to check for a non-existing object?\n\nSo even with --paranoid (aka what we have now) index-pack still\nshould be faster than unpack-objects for any sizeable transfer,\nand is just as \"safe\".\n\nIf its the missing-object lookup that is expensive, maybe we should\ntry to optimize that. We do it enough already in other parts of\nthe code...\n\n-- \nShawn.\n"},{"id":"38542","messageId":"alpine.LFD.0.98.0704031657130.28181@xanadu.home","threadId":"7502","inReplyTo":"Pine.LNX.4.64.0704031322490.6730@woody.linux-foundation.org","subject":"Re: git-index-pack really does suck..","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-04-03T21:05:19Z","receivedAt":"2007-04-03T21:05:19Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 3 Apr 2007, Linus Torvalds wrote:\n\n> All that extra memory is just for SHA1 commit ID information. \n\nI don't see where that might be. The only thing that the paranoia check \ntriggers is:\n\n\tfoo = read_sha1_file(blah);\n\tmemcmp(foo with bar);\n\tfree(foo);\n\nSo where is that commit ID information actually stored when using \nread_sha1_file()?\n\n> Btw, even if we don't have any of the objects, if you have tons and tons \n> of objects and do a \"git pull\", just the *lookup* of the nonexistent \n> objects will be expensive: first we won't find it in any pack, then we'll \n> look at the loose objects, and then we'll look int he pack *again* due to \n> the race avoidance. So looking up nonexistent objects is actually pretty \n> expensive.\n\nNot if you consider that it is performed _while_ receiving (and waiting \nfor) the pack data over the net in the normal case.\n\n\nNicolas\n"},{"id":"38543","messageId":"20070403211149.GI27706@spearce.org","threadId":"7502","inReplyTo":"alpine.LFD.0.98.0704031657130.28181@xanadu.home","subject":"Re: git-index-pack really does suck..","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-04-03T21:11:49Z","receivedAt":"2007-04-03T21:11:49Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Nicolas Pitre wrote:\n> On Tue, 3 Apr 2007, Linus Torvalds wrote:\n> \n> > All that extra memory is just for SHA1 commit ID information. \n> \n> I don't see where that might be. The only thing that the paranoia check \n> triggers is:\n> \n> \tfoo = read_sha1_file(blah);\n> \tmemcmp(foo with bar);\n> \tfree(foo);\n> \n> So where is that commit ID information actually stored when using \n> read_sha1_file()?\n\nThat might just be the mmap code in sha1_file.c. We are mmaping\nthe existing packfile, to fetch the object. That counts as virtual\nmemory. ;-)\n\n-- \nShawn.\n"},{"id":"38544","messageId":"Pine.LNX.4.64.0704031411320.6730@woody.linux-foundation.org","threadId":"7502","inReplyTo":"20070403210319.GH27706@spearce.org","subject":"Re: git-index-pack really does suck..","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-04-03T21:13:06Z","receivedAt":"2007-04-03T21:13:06Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 3 Apr 2007, Shawn O. Pearce wrote:\n> \n> So even with --paranoid (aka what we have now) index-pack still\n> should be faster than unpack-objects for any sizeable transfer,\n> and is just as \"safe\".\n\nI agree 100% that index-pack is much much MUCH better than unpack-objects \never was, so ..\n\n> If its the missing-object lookup that is expensive, maybe we should\n> try to optimize that. We do it enough already in other parts of\n> the code...\n\nWell, for all other cases it's really the \"object found\" case that is \nworth optimizing for, so I think optimizing for \"no object\" is actually \nwrong, unless it also speeds up (or at least doesn't make it worse) the \n\"real\" normal case.\n\n\t\tLinus\n"},{"id":"38545","messageId":"20070403211709.GJ27706@spearce.org","threadId":"7502","inReplyTo":"Pine.LNX.4.64.0704031411320.6730@woody.linux-foundation.org","subject":"Re: git-index-pack really does suck..","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-04-03T21:17:09Z","receivedAt":"2007-04-03T21:17:09Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Linus Torvalds wrote:\n> On Tue, 3 Apr 2007, Shawn O. Pearce wrote:\n> > If its the missing-object lookup that is expensive, maybe we should\n> > try to optimize that. We do it enough already in other parts of\n> > the code...\n> \n> Well, for all other cases it's really the \"object found\" case that is \n> worth optimizing for, so I think optimizing for \"no object\" is actually \n> wrong, unless it also speeds up (or at least doesn't make it worse) the \n> \"real\" normal case.\n\nRight. But maybe we shouldn't be scanning for packfiles every\ntime we don't find a loose object. Especially if the caller is in\na context where we actually *expect* to not find said object like\nhalf of the time... say in git-add/update-index. ;-)\n\n-- \nShawn.\n"},{"id":"38546","messageId":"alpine.LFD.0.98.0704031705440.28181@xanadu.home","threadId":"7502","inReplyTo":"Pine.LNX.4.64.0704031346250.6730@woody.linux-foundation.org","subject":"Re: git-index-pack really does suck..","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-04-03T21:21:12Z","receivedAt":"2007-04-03T21:21:12Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 3 Apr 2007, Linus Torvalds wrote:\n\n> \n> \n> On Tue, 3 Apr 2007, Nicolas Pitre wrote:\n> > > \n> > > Yeah. What happens is that inside the repo, because we do all the \n> > > duplicate object checks (verifying that there are no evil hash collisions) \n> > > even after fixing the memory leak, we end up keeping *track* of all those \n> > > objects.\n> > \n> > What do you mean?\n> \n> Look at what we have to do to look up a SHA1 object.. We create all the \n> lookup infrastructure, we don't *just* read the object. The delta base \n> cache is the most obvious one. \n\nIt is caped to 16MB, so we're far from the 200+ MB count.\n\n> > I'm of the opinion that this patch is unnecessary. It only helps in \n> > bogus workflows to start with, and it makes the default behavior unsafe \n> > (unsafe from a paranoid pov, but still). And in the _normal_ workflow \n> > it should never trigger.\n> \n> Actually, even in the normal workflow it will do all the extra unnecessary \n> work, if only because the lookup costs of *not* finding the entry.\n> \n> Lookie here:\n> \n> - git index-pack of the *git* pack-file in the v2.6/linux directory (zero \n> overlap of objects)\n> \n> With --paranoid:\n> \n> \t2.75user 0.37system 0:03.13elapsed 99%CPU\n> \t0major+5583minor pagefaults\n> \n> Without --paranoid:\n> \n> \t2.55user 0.12system 0:02.68elapsed 99%CPU\n> \t0major+2957minor pagefaults\n> \n> See? That's the *normal* workflow. Zero objects found. 7% CPU overhead \n> from just the unnecessary work, and almost twice as much memory used. Just \n> from the index file lookup etc for a decent-sized project.\n\n7% overhead over 2 second and a half of CPU which, _normally_, happens \nwhen cloning the whole thing over a network connection which, if you're \nlucky and have a 6mbps cable connection, will still be spread over 5 \nminutes of real time. And that is assuming that you're cloning a big \nproject inside itself which wouldn't work anyway. Otherwise a big clone \nwound run index-pack in an empty repo where the lookup of exinsting \nobject is zero. Remains git-fetch which should concern itself with much \nsmaller packs pushing this overhead in the noise.\n\n> Now, in the KDE situation, the *unnecessary* lookups will be about ten \n> times more expensive, both on memory and CPU, just because the repository \n> is about 20x the size. Even with no actual hits.\n\nSo? When would you really perform such an operation in a meaningful \nway?\n\nThe memory usage worries me. I still cannot explain nor justify it. \nBut the CPU overhead is certainly not of any concern in _normal_ usage \nscenarios, is it?\n\nIf anything that might be a good test case for the newton-raphson pack \nlookup idea.\n\n\nNicolas\n"},{"id":"38547","messageId":"Pine.LNX.4.64.0704031413200.6730@woody.linux-foundation.org","threadId":"7502","inReplyTo":"alpine.LFD.0.98.0704031657130.28181@xanadu.home","subject":"Re: git-index-pack really does suck..","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-04-03T21:24:45Z","receivedAt":"2007-04-03T21:24:45Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 3 Apr 2007, Nicolas Pitre wrote:\n>\n> I don't see where that might be. The only thing that the paranoia check \n> triggers is:\n> \n> \tfoo = read_sha1_file(blah);\n> \tmemcmp(foo with bar);\n> \tfree(foo);\n> \n> So where is that commit ID information actually stored when using \n> read_sha1_file()?\n\nI've got the numbers: it uses much more memory when doing even failing \nlookups, ie:\n\n\tWith --paranoid: 5583 minor pagefaults (21MB)\n\tWithout --paranoid: 2957 minor pagefaults (11MB)\n\n(remember, this was *just* the git pack, not the kernel pack)\n\nIt could be as simple as just the index file itself. That's 11MB for the \nkernel. Imagine if the index file was 20 times bigger - 200MB of memory \npaged in with bad access patterns just for unnecessary lookups.\n\nRunning valgrind shows no leak at all without --paranoid. With --paranoid, \nthere's some really trivial stuff (the \"packed_git\" structure etc, so I \nthink it's really just the index itself).\n\n> Not if you consider that it is performed _while_ receiving (and waiting \n> for) the pack data over the net in the normal case.\n\n..which is why I think it makes sense for \"pull\" to be paranoid. I just \ndon't think it makes sense to be paranoid *all* the time, since it's \nclearly expensive.\n\n\t\tLinus\n"},{"id":"38548","messageId":"Pine.LNX.4.64.0704031425220.6730@woody.linux-foundation.org","threadId":"7502","inReplyTo":"20070403211709.GJ27706@spearce.org","subject":"Re: git-index-pack really does suck..","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-04-03T21:26:41Z","receivedAt":"2007-04-03T21:26:41Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 3 Apr 2007, Shawn O. Pearce wrote:\n> \n> Right. But maybe we shouldn't be scanning for packfiles every\n> time we don't find a loose object. Especially if the caller is in\n> a context where we actually *expect* to not find said object like\n> half of the time... say in git-add/update-index. ;-)\n\nYes, we could definitely skip the re-lookup if we had a \"don't really \ncare, I can recreate the object myself\" flag (ie anybody who is going to \nwrite that object)\n\n\t\t\tLinus\n"},{"id":"38550","messageId":"Pine.LNX.4.64.0704031427050.6730@woody.linux-foundation.org","threadId":"7502","inReplyTo":"Pine.LNX.4.64.0704031425220.6730@woody.linux-foundation.org","subject":"Re: git-index-pack really does suck..","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-04-03T21:28:22Z","receivedAt":"2007-04-03T21:28:22Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 3 Apr 2007, Linus Torvalds wrote:\n> \n> Yes, we could definitely skip the re-lookup if we had a \"don't really \n> care, I can recreate the object myself\" flag (ie anybody who is going to \n> write that object)\n\nSide note: with \"alternates\" files, you might well *always* have the \nobjects. If you do\n\n\tgit clone -l -s ...\n\nto create various branches, and then pull between them, you'll actually \nend up in the situation that you'll always find the objects and get back \nto the really expensive case..\n\n\t\tLinus\n"},{"id":"38549","messageId":"alpine.LFD.0.98.0704031722590.28181@xanadu.home","threadId":"7502","inReplyTo":"Pine.LNX.4.64.0704031357470.6730@woody.linux-foundation.org","subject":"Re: git-index-pack really does suck..","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-04-03T21:28:26Z","receivedAt":"2007-04-03T21:28:26Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 3 Apr 2007, Linus Torvalds wrote:\n\n> See my separate timing numbers, although I bet that Chris can give even \n> better ones..\n> \n> Chris, try applying my patch, and then inside the KDE repo you have, do\n> \n> \tgit index-pack --paranoid --stdin --fix-thin new.pack < ~/git/.git/objects/pack/pack-*.pack\n> \n> (ie index the objects of the *git* repository, not the KDE one). That \n> should approximate doing a fair-sized \"git pull\" - getting new objects. Do \n> it with and without --paranoid, and time it.\n\nLike I said this is bogus since the index-pack is throttled by the \nnetwork making this overhead a non issue in real life.\n\nAnd like I said there should _not_ be such a memory usage difference \nwhich is probably showing potential problems *elsewhere*.\n\n> I bet that what I see as a 7% slowdown will be much bigger for you, just \n> because the negative lookups will be all that much more expensive when you \n> have tons of objects.\n\nAnd I bet your newton-raphson lookup idea would shine and bring that \noverhead down considerably in that case.\n\n\nNicolas\n"},{"id":"38551","messageId":"alpine.LFD.0.98.0704031730300.28181@xanadu.home","threadId":"7502","inReplyTo":"20070403211709.GJ27706@spearce.org","subject":"Re: git-index-pack really does suck..","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-04-03T21:34:41Z","receivedAt":"2007-04-03T21:34:41Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 3 Apr 2007, Shawn O. Pearce wrote:\n\n> Linus Torvalds wrote:\n> > On Tue, 3 Apr 2007, Shawn O. Pearce wrote:\n> > > If its the missing-object lookup that is expensive, maybe we should\n> > > try to optimize that. We do it enough already in other parts of\n> > > the code...\n> > \n> > Well, for all other cases it's really the \"object found\" case that is \n> > worth optimizing for, so I think optimizing for \"no object\" is actually \n> > wrong, unless it also speeds up (or at least doesn't make it worse) the \n> > \"real\" normal case.\n> \n> Right. But maybe we shouldn't be scanning for packfiles every\n> time we don't find a loose object. Especially if the caller is in\n> a context where we actually *expect* to not find said object like\n> half of the time... say in git-add/update-index. ;-)\n\nFirst, I truly believe we should have a 64-bit pack index and fewer \nlarger packs than many small packs.\n\nWhich leaves us with the actual pack index lookup. At that point the \ncost of finding an existing object and finding that a given object \ndoesn't exist is about the same thing, isn't it?\n\nOptimizing that lookup is going to benefit both cases.\n\n\nNicolas\n"},{"id":"38552","messageId":"20070403213710.GK27706@spearce.org","threadId":"7502","inReplyTo":"alpine.LFD.0.98.0704031730300.28181@xanadu.home","subject":"Re: git-index-pack really does suck..","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-04-03T21:37:10Z","receivedAt":"2007-04-03T21:37:10Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Nicolas Pitre wrote:\n> First, I truly believe we should have a 64-bit pack index and fewer \n> larger packs than many small packs.\n\nI'll buy that. ;-)\n \n> Which leaves us with the actual pack index lookup. At that point the \n> cost of finding an existing object and finding that a given object \n> doesn't exist is about the same thing, isn't it?\n\nHere's the rub: in the missing object case we didn't find it\nin the pack index, but it could be loose. That's one failed\nsyscall per object if the object isn't loose. If the object\nisn't loose, it could be that it was *just* removed by a\nrunning prune-packed, and the packfile that it was moved\nto was created after we scanned for packfiles, so time to\nrescan...\n\n-- \nShawn.\n"},{"id":"38553","messageId":"alpine.LFD.0.98.0704031735470.28181@xanadu.home","threadId":"7502","inReplyTo":"Pine.LNX.4.64.0704031413200.6730@woody.linux-foundation.org","subject":"Re: git-index-pack really does suck..","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-04-03T21:42:01Z","receivedAt":"2007-04-03T21:42:01Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 3 Apr 2007, Linus Torvalds wrote:\n\n> \n> \n> On Tue, 3 Apr 2007, Nicolas Pitre wrote:\n> >\n> > I don't see where that might be. The only thing that the paranoia check \n> > triggers is:\n> > \n> > \tfoo = read_sha1_file(blah);\n> > \tmemcmp(foo with bar);\n> > \tfree(foo);\n> > \n> > So where is that commit ID information actually stored when using \n> > read_sha1_file()?\n> \n> I've got the numbers: it uses much more memory when doing even failing \n> lookups, ie:\n> \n> \tWith --paranoid: 5583 minor pagefaults (21MB)\n> \tWithout --paranoid: 2957 minor pagefaults (11MB)\n> \n> (remember, this was *just* the git pack, not the kernel pack)\n> \n> It could be as simple as just the index file itself. That's 11MB for the \n> kernel. Imagine if the index file was 20 times bigger - 200MB of memory \n> paged in with bad access patterns just for unnecessary lookups.\n\nAgain that presumes you have to page in the whole index, which should \nnot happen when pulling (much smaller packs) and with a better lookup \nalgorithm.\n\n> > Not if you consider that it is performed _while_ receiving (and waiting \n> > for) the pack data over the net in the normal case.\n> \n> ..which is why I think it makes sense for \"pull\" to be paranoid. I just \n> don't think it makes sense to be paranoid *all* the time, since it's \n> clearly expensive.\n\nMake it conditionnal on --stdin then. This covers all cases where we \nreally want the secure thing to happen, and the --stdin case already \nperform the atomic rename-and-move thing when the pack is fully indexed.\n\n\nNicolas\n"},{"id":"38554","messageId":"7vvegduo9e.fsf@assigned-by-dhcp.cox.net","threadId":"7502","inReplyTo":"20070403213710.GK27706@spearce.org","subject":"Re: git-index-pack really does suck..","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-04-03T21:44:13Z","receivedAt":"2007-04-03T21:44:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n> Nicolas Pitre wrote:\n>> First, I truly believe we should have a 64-bit pack index and fewer \n>> larger packs than many small packs.\n>\n> I'll buy that. ;-)\n> \n>> Which leaves us with the actual pack index lookup. At that point the \n>> cost of finding an existing object and finding that a given object \n>> doesn't exist is about the same thing, isn't it?\n>\n> Here's the rub: in the missing object case we didn't find it\n> in the pack index, but it could be loose. That's one failed\n> syscall per object if the object isn't loose. If the object\n> isn't loose, it could be that it was *just* removed by a\n> running prune-packed, and the packfile that it was moved\n> to was created after we scanned for packfiles, so time to\n> rescan...\n\nIf that is the only reason we have these reprepare_packed_git()\nsprinkled all over in sha1_file.c (by 637cdd9d), perhaps we\nshould rethink that. Is there a cheap way to trigger these\nrescanning only when a prune-packed is in progress, I wonder...\n"},{"id":"38555","messageId":"20070403215342.GL27706@spearce.org","threadId":"7502","inReplyTo":"7vvegduo9e.fsf@assigned-by-dhcp.cox.net","subject":"Re: git-index-pack really does suck..","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-04-03T21:53:42Z","receivedAt":"2007-04-03T21:53:42Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano wrote:\n> \"Shawn O. Pearce\" writes:\n> > Here's the rub: in the missing object case we didn't find it\n> > in the pack index, but it could be loose. That's one failed\n> > syscall per object if the object isn't loose. If the object\n> > isn't loose, it could be that it was *just* removed by a\n> > running prune-packed, and the packfile that it was moved\n> > to was created after we scanned for packfiles, so time to\n> > rescan...\n> \n> If that is the only reason we have these reprepare_packed_git()\n> sprinkled all over in sha1_file.c (by 637cdd9d), perhaps we\n> should rethink that. Is there a cheap way to trigger these\n> rescanning only when a prune-packed is in progress, I wonder...\n\nYea, it is the only reason. So... if we could have some\nmagic to trigger that, it would be good. I just don't\nknow what magic that would be.\n\n-- \nShawn.\n"},{"id":"38556","messageId":"7vodm5un61.fsf@assigned-by-dhcp.cox.net","threadId":"7502","inReplyTo":"alpine.LFD.0.98.0704031735470.28181@xanadu.home","subject":"Re: git-index-pack really does suck..","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-04-03T22:07:50Z","receivedAt":"2007-04-03T22:07:50Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre writes:\n\n> Make it conditionnal on --stdin then. This covers all cases where we \n> really want the secure thing to happen, and the --stdin case already \n> perform the atomic rename-and-move thing when the pack is fully indexed.\n\nRepacking objects in a repository uses pack-objects without\nusing index-pack, as you suggested Chris. Is there a sane usage\nof index-pack that does not use --stdin? I do not think of any.\n\nIf there isn't, the \"conditional on --stdin\" suggestion means we\nunconditionally do the secure thing for all the sane usage, and\ngo unsecure for an insane usage that we do not really care about.\n\nIf so, it seems to me that it would be the simplest not to touch\nthe code at all, except that missing free().\n\nAm I missing something?\n"},{"id":"38557","messageId":"20070403221006.GA17948@coredump.intra.peff.net","threadId":"7502","inReplyTo":"20070403215342.GL27706@spearce.org","subject":"Re: git-index-pack really does suck..","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-04-03T22:10:07Z","receivedAt":"2007-04-03T22:10:07Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 03, 2007 at 05:53:42PM -0400, Shawn O. Pearce wrote:\n\n> > If that is the only reason we have these reprepare_packed_git()\n> > sprinkled all over in sha1_file.c (by 637cdd9d), perhaps we\n> > should rethink that. Is there a cheap way to trigger these\n> > rescanning only when a prune-packed is in progress, I wonder...\n> \n> Yea, it is the only reason. So... if we could have some\n> magic to trigger that, it would be good. I just don't\n> know what magic that would be.\n\nIt doesn't have to be in progress; it might have started and completed\nbetween pack-scanning and object lookup. Something like:\n\n 1. start git-repack -a -d\n 2. start git-rev-list, scan for packs\n 3. repack moves finished pack into place, starts git-prune-packed\n 4. git-prune-pack completes\n 5. git-rev-list looks up object\n\nSo you would need to have some sort of incremented counter that says\n\"there's a new pack available.\"\n\nPerhaps instead of rescanning unconditionally, it should simply stat()\nthe pack directory and look for a change? That should be much cheaper.\n\n-Jeff\n"},{"id":"38558","messageId":"20070403221126.GM27706@spearce.org","threadId":"7502","inReplyTo":"7vodm5un61.fsf@assigned-by-dhcp.cox.net","subject":"Re: git-index-pack really does suck..","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-04-03T22:11:26Z","receivedAt":"2007-04-03T22:11:26Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano wrote:\n> Nicolas Pitre writes:\n> \n> > Make it conditionnal on --stdin then. This covers all cases where we \n> > really want the secure thing to happen, and the --stdin case already \n> > perform the atomic rename-and-move thing when the pack is fully indexed.\n> \n> Repacking objects in a repository uses pack-objects without\n> using index-pack, as you suggested Chris. Is there a sane usage\n> of index-pack that does not use --stdin? I do not think of any.\n> \n> If there isn't, the \"conditional on --stdin\" suggestion means we\n> unconditionally do the secure thing for all the sane usage, and\n> go unsecure for an insane usage that we do not really care about.\n> \n> If so, it seems to me that it would be the simplest not to touch\n> the code at all, except that missing free().\n> \n> Am I missing something?\n\nNope. I agree with you completely.\n\n-- \nShawn.\n"},{"id":"38559","messageId":"Pine.LNX.4.64.0704031511580.6730@woody.linux-foundation.org","threadId":"7502","inReplyTo":"alpine.LFD.0.98.0704031735470.28181@xanadu.home","subject":"Re: git-index-pack really does suck..","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-04-03T22:14:28Z","receivedAt":"2007-04-03T22:14:28Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 3 Apr 2007, Nicolas Pitre wrote:\n> > \n> > It could be as simple as just the index file itself. That's 11MB for the \n> > kernel. Imagine if the index file was 20 times bigger - 200MB of memory \n> > paged in with bad access patterns just for unnecessary lookups.\n> \n> Again that presumes you have to page in the whole index, which should \n> not happen when pulling (much smaller packs) and with a better lookup \n> algorithm.\n\nActually, since SHA1's are randomly distributed, together with the binary \nsearch, you probably *will* pull in the bulk of the index, even with a \nfairly small set of SHA1's that you check.\n\n> Make it conditionnal on --stdin then. This covers all cases where we \n> really want the secure thing to happen, and the --stdin case already \n> perform the atomic rename-and-move thing when the pack is fully indexed.\n\nI don't care *what* it is conditional on, but your arguments suck. You \nclaim that it's not a normal case to already have the objects, when it \n*is* a normal case for alternates, etc.\n\nI don't understand why you argue against hard numbers. You have none of \nyour own.\n\n\t\tLinus\n"},{"id":"38560","messageId":"7vd52lum2f.fsf@assigned-by-dhcp.cox.net","threadId":"7502","inReplyTo":"Pine.LNX.4.64.0704031427050.6730@woody.linux-foundation.org","subject":"Re: git-index-pack really does suck..","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-04-03T22:31:36Z","receivedAt":"2007-04-03T22:31:36Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> Side note: with \"alternates\" files, you might well *always* have the \n> objects. If you do\n>\n> \tgit clone -l -s ...\n>\n> to create various branches, and then pull between them, you'll actually \n> end up in the situation that you'll always find the objects and get back \n> to the really expensive case..\n\nAh, that's true. If you \"git clone -l -s A B\", create new\nobjects in A and pull from B, the transfer would not exclude\nnew objects as they are not visible from B's refs.\n\nIn that scenario, the keep-pack behaviour is already worse than\nthe unpack-objects behaviour. The former creates a packfile\nthat duplicates objects that are in A while the latter, although\nexpensive, ends up doing nothing.\n\nI wonder if we can have a backdoor to avoid any object transfer\nin such a case to begin with...\n"},{"id":"38569","messageId":"Pine.LNX.4.63.0704031530040.21680@qynat.qvtvafvgr.pbz","threadId":"7502","inReplyTo":"Pine.LNX.4.64.0704031544110.6730@woody.linux-foundation.org","subject":"Re: git-index-pack really does suck..","fromName":"David Lang","fromEmail":"david.lang@digitalinsight.com","sentAt":"2007-04-03T22:31:45Z","receivedAt":"2007-04-03T22:31:45Z","isPatch":false,"sender":{"key":"david.lang@digitalinsight.com","avatar":null},"body":"On Tue, 3 Apr 2007, Linus Torvalds wrote:\n\n> On Tue, 3 Apr 2007, Dana How wrote:\n>>\n>> Larger and larger pack files make me nervous.\n>> They are expensive to manipulate,\n>> and >2GB requires a file format change.\n>\n> It sometimes also requires a new filesystem. There are a lot of\n> filesystems that can handle more than 4GB total, but not necessarily in a\n> single file.\n>\n> The only really useful such filesystem is probably FAT, which is still\n> quite useful for things like USB memory sticks. But that is probably\n> already worth supporting.\n>\n> So I think we want to support 64-bit (or at least something like 40+ bit)\n> pack-files, but yes, I think that even if/when we support it, we still\n> want to support the \"multiple smaller pack-files\" schenario exactly\n> because for some uses it's much *better* to have ten 2GB packfiles rather\n> than one 20GB pack-file.\n\nhowever, for historical archives you may end up with wanting to do a 2GB \npackfile of 'recent' stuff, and a 14GB packfile of 'ancient' stuff (with the \nlarge one built with all space-saving options turned up all the way, no matter \nhow much time it takes to build the pack)\n\nDavid Lang\n"},{"id":"38561","messageId":"alpine.LFD.0.98.0704031832380.28181@xanadu.home","threadId":"7502","inReplyTo":"7vodm5un61.fsf@assigned-by-dhcp.cox.net","subject":"Re: git-index-pack really does suck..","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-04-03T22:34:04Z","receivedAt":"2007-04-03T22:34:04Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 3 Apr 2007, Junio C Hamano wrote:\n\n> Nicolas Pitre writes:\n> \n> > Make it conditionnal on --stdin then. This covers all cases where we \n> > really want the secure thing to happen, and the --stdin case already \n> > perform the atomic rename-and-move thing when the pack is fully indexed.\n> \n> Repacking objects in a repository uses pack-objects without\n> using index-pack, as you suggested Chris. Is there a sane usage\n> of index-pack that does not use --stdin? I do not think of any.\n> \n> If there isn't, the \"conditional on --stdin\" suggestion means we\n> unconditionally do the secure thing for all the sane usage, and\n> go unsecure for an insane usage that we do not really care about.\n> \n> If so, it seems to me that it would be the simplest not to touch\n> the code at all, except that missing free().\n\nThat's exactly what I think as well.\n\n> Am I missing something?\n\nNot from my point of view.\n\n\nNicolas\n"},{"id":"38570","messageId":"Pine.LNX.4.63.0704031532390.21680@qynat.qvtvafvgr.pbz","threadId":"7502","inReplyTo":"alpine.LFD.0.98.0704031836350.28181@xanadu.home","subject":"Re: git-index-pack really does suck..","fromName":"David Lang","fromEmail":"david.lang@digitalinsight.com","sentAt":"2007-04-03T22:36:01Z","receivedAt":"2007-04-03T22:36:01Z","isPatch":false,"sender":{"key":"david.lang@digitalinsight.com","avatar":null},"body":"On Tue, 3 Apr 2007, Nicolas Pitre wrote:\n\n> On Tue, 3 Apr 2007, Linus Torvalds wrote:\n>\n>> I don't care *what* it is conditional on, but your arguments suck. You\n>> claim that it's not a normal case to already have the objects, when it\n>> *is* a normal case for alternates, etc.\n>>\n>> I don't understand why you argue against hard numbers. You have none of\n>> your own.\n>\n> Are hard numbers like 7% overhead (because right now that's all we have)\n> really worth it against bad _perceptions_?\n\nplus 1s overhead on what's otherwise a noop command.\n\n> The keeping of fetched packs broke that presumption of trust towards\n> local objects and it opened a real path for potential future attacks.\n> Those attacks are still fairly theoretical of course. But for how\n> _long_? Do we want GIT to be considered backdoor prone in a couple\n> years from now just because we were obsessed by a 7% CPU overhead?\n>\n> I think we have much more to gain by playing it safe and being more\n> secure and paranoid than trying to squeeze some CPU cycles out of an\n> operation that is likely to ever be bounded by network speed for most\n> people.\n\nthis is why -paranoid should be left on for network pulls, but having it on for \nthe local uses means that the cost isn't hidden in the network limits isn't \ngood.\n\nDavid Lang\n"},{"id":"38562","messageId":"20070403223840.GN27706@spearce.org","threadId":"7502","inReplyTo":"7vd52lum2f.fsf@assigned-by-dhcp.cox.net","subject":"Re: git-index-pack really does suck..","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-04-03T22:38:40Z","receivedAt":"2007-04-03T22:38:40Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano wrote:\n> Ah, that's true. If you \"git clone -l -s A B\", create new\n> objects in A and pull from B, the transfer would not exclude\n> new objects as they are not visible from B's refs.\n> \n> In that scenario, the keep-pack behaviour is already worse than\n> the unpack-objects behaviour. The former creates a packfile\n> that duplicates objects that are in A while the latter, although\n> expensive, ends up doing nothing.\n> \n> I wonder if we can have a backdoor to avoid any object transfer\n> in such a case to begin with...\n\nYea, symlink to the corresponding refs directory of the alternate\nODB. Then the refs will be visible. ;-)\n\n-- \nShawn.\n"},{"id":"38563","messageId":"56b7f5510704031540i4df918e6g5a82389b6759c50b@mail.gmail.com","threadId":"7502","inReplyTo":"alpine.LFD.0.98.0704031730300.28181@xanadu.home","subject":"Re: git-index-pack really does suck..","fromName":"Dana How","fromEmail":"danahow@gmail.com","sentAt":"2007-04-03T22:40:00Z","receivedAt":"2007-04-03T22:40:00Z","isPatch":false,"sender":{"key":"danahow@gmail.com","avatar":null},"body":"On 4/3/07, Nicolas Pitre wrote:\n> On Tue, 3 Apr 2007, Shawn O. Pearce wrote:\n> > Right. But maybe we shouldn't be scanning for packfiles every\n> > time we don't find a loose object. Especially if the caller is in\n> > a context where we actually *expect* to not find said object like\n> > half of the time... say in git-add/update-index. ;-)\n>\n> First, I truly believe we should have a 64-bit pack index and fewer\n> larger packs than many small packs.\n>\n> Which leaves us with the actual pack index lookup. At that point the\n> cost of finding an existing object and finding that a given object\n> doesn't exist is about the same thing, isn't it?\n>\n> Optimizing that lookup is going to benefit both cases.\n\nDo you get what you want if you move to fewer larger INDEX files\nbut not pack files -- in the extreme, one large index file?\nA \"super index\" could be built from multiple .idx files.\nThis would be a new file (format) unencumbered by the past,\nso it could be tried out more quickly.\nJust like objects are pruned when packed,\n.idx files could be pruned when the super index is built.\n\nPerhaps a number of (<2GB) packfiles and a large index\nfile could work out.\n\nLarger and larger pack files make me nervous.\nThey are expensive to manipulate,\nand >2GB requires a file format change.\n\nThanks,\n-- \nDana L. How danahow@gmail.com +1 650 804 5991 cell\n"},{"id":"38564","messageId":"7v8xd9ulm4.fsf@assigned-by-dhcp.cox.net","threadId":"7502","inReplyTo":"20070403223840.GN27706@spearce.org","subject":"Re: git-index-pack really does suck..","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-04-03T22:41:23Z","receivedAt":"2007-04-03T22:41:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n>> I wonder if we can have a backdoor to avoid any object transfer\n>> in such a case to begin with...\n>\n> Yea, symlink to the corresponding refs directory of the alternate\n> ODB. Then the refs will be visible. ;-)\n\nI was thinking about going the other way. When git-fetch is\nasked to fetch over a local transport, it could check if the\nsource is one of its alternate object stores.\n"},{"id":"38565","messageId":"db69205d0704031549g7273da53g817f885705735db2@mail.gmail.com","threadId":"7502","inReplyTo":"Pine.LNX.4.64.0704031357470.6730@woody.linux-foundation.org","subject":"Re: git-index-pack really does suck..","fromName":"Chris Lee","fromEmail":"clee@kde.org","sentAt":"2007-04-03T22:49:59Z","receivedAt":"2007-04-03T22:49:59Z","isPatch":false,"sender":{"key":"clee@kde.org","avatar":"https://gravatar.com/avatar/c930bdc8cc6465094a5722188409ecb8955e0da2b188d7340137074b08f857e3?d=mp&s=160"},"body":"On 4/3/07, Linus Torvalds wrote:\n> On Tue, 3 Apr 2007, Junio C Hamano wrote:\n> > So maybe we should retitle this thread from \"git-index-pack\n> > really does suck..\" to \"I used git-index-pack in a stupid way\"?\n\nWell, I never claimed to be a genius. :)\n\n> See my separate timing numbers, although I bet that Chris can give even\n> better ones..\n>\n> Chris, try applying my patch, and then inside the KDE repo you have, do\n>\n> git index-pack --paranoid --stdin --fix-thin new.pack < ~/git/.git/objects/pack/pack-*.pack\n>\n> (ie index the objects of the *git* repository, not the KDE one). That\n> should approximate doing a fair-sized \"git pull\" - getting new objects. Do\n> it with and without --paranoid, and time it.\n\n% time git-index-pack --paranoid --stdin --fix-thin paranoid.pack <\n/usr/local/src/git/.git/objects/pack/*pack\npack\tbf8ba7897da9c84d1981ecdc92c0b1979506a4b9\ngit-index-pack --paranoid --stdin --fix-thin paranoid.pack < 5.28s\nuser 0.24s system 98% cpu 5.592 total\n\n% time git-index-pack --stdin --fix-thin trusting.pack <\n/usr/local/src/git/.git/objects/pack/*pack\npack\tbf8ba7897da9c84d1981ecdc92c0b1979506a4b9\ngit-index-pack --stdin --fix-thin trusting.pack < 5.07s user 0.12s\nsystem 99% cpu 5.202 total\n\nSo, in my case, at least... not really much of a difference, which is puzzling.\n\n> I bet that what I see as a 7% slowdown will be much bigger for you, just\n> because the negative lookups will be all that much more expensive when you\n> have tons of objects.\n\nI applied exactly the patch you sent, and it applied perfectly cleanly\n- no failures.\n\nI also mailed out the DVD with the repo on it to hpa today, so\nhopefully by tomorrow he'll get it. (He's not even two cities over,\nand I suspect I could have just driven it to his place, but that might\nhave been a little awkward since I've never met him.)\n\nAnyway, so, hopefully once he gets it he can put it up somewhere that\nyou guys can grab it. For reference, the KDE repo is pretty big, but a\n\"real\" conversion of the repo would be bigger; the one that I've been\nplaying with only has the KDE svn trunk, and only the first 409k\nrevisions - there are, as of right now, over 650k revisions in KDE's\nsvn repo. So, realistically speaking, a fully-converted KDE git repo\nwould probably take up at least 6GB, packed, if not more. Subproject\nsupport would probably be *really* helpful to mitigate that.\n\n-clee\n"},{"id":"38566","messageId":"Pine.LNX.4.64.0704031544110.6730@woody.linux-foundation.org","threadId":"7502","inReplyTo":"56b7f5510704031540i4df918e6g5a82389b6759c50b@mail.gmail.com","subject":"Re: git-index-pack really does suck..","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-04-03T22:52:32Z","receivedAt":"2007-04-03T22:52:32Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 3 Apr 2007, Dana How wrote:\n> \n> Larger and larger pack files make me nervous.\n> They are expensive to manipulate,\n> and >2GB requires a file format change.\n\nIt sometimes also requires a new filesystem. There are a lot of \nfilesystems that can handle more than 4GB total, but not necessarily in a \nsingle file.\n\nThe only really useful such filesystem is probably FAT, which is still \nquite useful for things like USB memory sticks. But that is probably \nalready worth supporting.\n\nSo I think we want to support 64-bit (or at least something like 40+ bit) \npack-files, but yes, I think that even if/when we support it, we still \nwant to support the \"multiple smaller pack-files\" schenario exactly \nbecause for some uses it's much *better* to have ten 2GB packfiles rather \nthan one 20GB pack-file.\n\n\t\t\tLinus\n"},{"id":"38567","messageId":"alpine.LFD.0.98.0704031836350.28181@xanadu.home","threadId":"7502","inReplyTo":"Pine.LNX.4.64.0704031511580.6730@woody.linux-foundation.org","subject":"Re: git-index-pack really does suck..","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-04-03T22:55:05Z","receivedAt":"2007-04-03T22:55:05Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 3 Apr 2007, Linus Torvalds wrote:\n\n> I don't care *what* it is conditional on, but your arguments suck. You \n> claim that it's not a normal case to already have the objects, when it \n> *is* a normal case for alternates, etc.\n> \n> I don't understand why you argue against hard numbers. You have none of \n> your own.\n\nAre hard numbers like 7% overhead (because right now that's all we have) \nreally worth it against bad _perceptions_?\n\nSure, the SHA1 collision attack is paranoia. But it is becoming \nincreasingly *possible*.\n\nAnd when we only had unpack-objects on the receiving end of a fetch, you \nyourself bragged about the implied security of GIT in the presence of a \nSHA1 collision attack. Because let's admit it: when a SHA1 collision \nwill happen it is way more probable to come on purpose than from pure \naccident. But as you said at the time, it is not a problem because GIT \ntrusts local objects more than remote ones and incidentally \nunpack-objects doesn't overwrite existing objects.\n\nThe keeping of fetched packs broke that presumption of trust towards \nlocal objects and it opened a real path for potential future attacks. \nThose attacks are still fairly theoretical of course. But for how \n_long_? Do we want GIT to be considered backdoor prone in a couple \nyears from now just because we were obsessed by a 7% CPU overhead?\n\nI think we have much more to gain by playing it safe and being more \nsecure and paranoid than trying to squeeze some CPU cycles out of an \noperation that is likely to ever be bounded by network speed for most \npeople.\n\nAnd we _know_ that the operation can be optimized further anyway.\n\nSo IMHO in this case hard numbers alone aren't the end of it. Not as \nlong as they're reasonably low. And especially not for a command which \nis 1) rather infrequent and 2) not really interactive like git-log might \nbe.\n\n\nNicolas\n"},{"id":"38568","messageId":"alpine.LFD.0.98.0704031856110.28181@xanadu.home","threadId":"7502","inReplyTo":"56b7f5510704031540i4df918e6g5a82389b6759c50b@mail.gmail.com","subject":"Re: git-index-pack really does suck..","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-04-03T23:00:34Z","receivedAt":"2007-04-03T23:00:34Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 3 Apr 2007, Dana How wrote:\n\n> Do you get what you want if you move to fewer larger INDEX files\n> but not pack files -- in the extreme, one large index file?\n\nNo that doesn't solve the clone/fetch of large amount of data cleanly.\n\n> A \"super index\" could be built from multiple .idx files.\n> This would be a new file (format) unencumbered by the past,\n> so it could be tried out more quickly.\n> Just like objects are pruned when packed,\n> .idx files could be pruned when the super index is built.\n\nThis is an idea to consider independently of any other issues.\n\n> Perhaps a number of (<2GB) packfiles and a large index\n> file could work out.\n> \n> Larger and larger pack files make me nervous.\n> They are expensive to manipulate,\n> and >2GB requires a file format change.\n\nNo. Larger pack files are not more expensive than the same set of \nobjects spread into multiple packs. In fact I'd say it's quite the \nopposite. And larger pack files do not require a pack format change -- \nit's just the index that has to change and the index is a local matter.\n\n\nNicolas\n"},{"id":"38572","messageId":"Pine.LNX.4.64.0704031553080.6730@woody.linux-foundation.org","threadId":"7502","inReplyTo":"db69205d0704031549g7273da53g817f885705735db2@mail.gmail.com","subject":"Re: git-index-pack really does suck..","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-04-03T23:12:32Z","receivedAt":"2007-04-03T23:12:32Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 3 Apr 2007, Chris Lee wrote:\n>\n> git-index-pack --paranoid --stdin --fix-thin paranoid.pack < \n> 5.28s user 0.24s system 98% cpu 5.592 total\n> \n> git-index-pack --stdin --fix-thin trusting.pack < \n> 5.07s user 0.12s system 99% cpu 5.202 total\n\nOk, that's not a big enough of a difference to care.\n\n> So, in my case, at least... not really much of a difference, which is\n> puzzling.\n\nIt's entirely possible that the object lookup is good enough to not be a \nproblem even for huge packs, and it really only gets to be a problem when \nyou actually unpack all the objects.\n\nIn that case, the only real case to worry about is indeed the \"alternates\" \ncase (or if people actually use a shared git object directory, but I don't \nthink anybody really does - alternates just work well enough, and shared \nobject directories are painful enough that I doubt anybody *really* uses \nit).\n\n> I also mailed out the DVD with the repo on it to hpa today, so\n> hopefully by tomorrow he'll get it. (He's not even two cities over,\n> and I suspect I could have just driven it to his place, but that might\n> have been a little awkward since I've never met him.)\n\nHeh. Ok, good. I'll torrent it or something when it's up.\n\n> Anyway, so, hopefully once he gets it he can put it up somewhere that\n> you guys can grab it. For reference, the KDE repo is pretty big, but a\n> \"real\" conversion of the repo would be bigger; the one that I've been\n> playing with only has the KDE svn trunk, and only the first 409k\n> revisions - there are, as of right now, over 650k revisions in KDE's\n> svn repo. So, realistically speaking, a fully-converted KDE git repo\n> would probably take up at least 6GB, packed, if not more. Subproject\n> support would probably be *really* helpful to mitigate that.\n\nSure. I think subproject support is likely the big \"missing feature\" of \ngit right now. The rest is \"details\", even if they can be big and involved \ndetails.\n\nBut even at only 409k revisions, it's still going to be an order of \nmagnitude bigger than what the kernel is, exactly *because* it's such a \ndisaster from a maintenance setup standpoint, and it's going to be a \nuseful real-world test-case. So whether that is a \"good\" git archive or \nnot, it's going to be useful.\n\nLong ago we used to be able to look at the historic Linux archive as an \nexample of a \"big\" archive, but it's not actually all that much bigger \nthan the normal Linux archive any more, and we've pretty much fixed the \nproblems we used to have.\n\n[ The historical pack-file is actually smaller, but that's because it was \n done with a much deeper delta-chain to make it small: the historical \n archive still has more objects in it than the current active git kernel \n tree - but it's only in the 20% range, not \"20 *times* bigger\" ]\n\nThe Eclipse tree was useful (and I think we already improved performance \nfor you thanks to working with it - I don't know how much faster the \ndelta-base cache made things for you, but I'd assume it was at *least* by \nthe factor-of-2.5 that we saw on Eclipse), but the KDE is bigger *and* \ndeeper (the eclipse tree is 1.7GB, and 136k revisions in the main branch, \nso the KDE tree is more than twice the revisions).\n\n\t\tLinus\n"},{"id":"38575","messageId":"Pine.LNX.4.64.0704031624090.6730@woody.linux-foundation.org","threadId":"7502","inReplyTo":"alpine.LFD.0.98.0704031836350.28181@xanadu.home","subject":"Re: git-index-pack really does suck..","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-04-03T23:29:08Z","receivedAt":"2007-04-03T23:29:08Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 3 Apr 2007, Nicolas Pitre wrote:\n> \n> Are hard numbers like 7% overhead (because right now that's all we have) \n> really worth it against bad _perceptions_?\n\nIf it actually stays at just 7% even with large repos (and the numbers \nfrom Chris seem to say that it doesn't get worse - in fact, it may be that \nthe lookup gets relatively more efficient for a large repo thanks to the \nlog(n) costs), I agree that 7% probably isn't worth worrying about when \nweighed against \"guaranteed no SHA1 collision\". Especially as long as \nyou'd normally only hit it when your real performance issue is going to be \nthe network.\n\nSo especially if we can make sure that the *local* case is ok, where the \nnetwork isn't going to be the bottleneck, I think we can/should do the \nparanoia.\n\nThat's especially true as it is also the local case where the 7% has \nalready been shown to be just the best case, with the worst case being \nmany hundred percent (and memory use going up from 55M to 280M in one \nexample), thanks to us actually *finding* the objects.\n\n\t\t\tLinus\n"},{"id":"38603","messageId":"81b0412b0704040251j34b0bc5eh1518eadcfa2ed299@mail.gmail.com","threadId":"7502","inReplyTo":"Pine.LNX.4.63.0704031532390.21680@qynat.qvtvafvgr.pbz","subject":"Re: git-index-pack really does suck..","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-04-04T09:51:50Z","receivedAt":"2007-04-04T09:51:50Z","isPatch":false,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 4/4/07, David Lang wrote:\n>\n> > The keeping of fetched packs broke that presumption of trust towards\n> > local objects and it opened a real path for potential future attacks.\n> > Those attacks are still fairly theoretical of course. But for how\n> > _long_? Do we want GIT to be considered backdoor prone in a couple\n> > years from now just because we were obsessed by a 7% CPU overhead?\n> >\n> > I think we have much more to gain by playing it safe and being more\n> > secure and paranoid than trying to squeeze some CPU cycles out of an\n> > operation that is likely to ever be bounded by network speed for most\n> > people.\n>\n> this is why -paranoid should be left on for network pulls, but having it on for\n> the local uses means that the cost isn't hidden in the network limits isn't\n> good.\n\nYou never know what pull is networked (or should I say: remote enough\nto cause a collision).\n"},{"id":"38690","messageId":"7v7isrrugx.fsf@assigned-by-dhcp.cox.net","threadId":"7502","inReplyTo":"Pine.LNX.4.64.0704031427050.6730@woody.linux-foundation.org","subject":"[PATCH 1/2] git-fetch--tool pick-rref","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-04-05T10:22:54Z","receivedAt":"2007-04-05T10:22:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This script helper takes list of fully qualified refnames and\nresults from ls-remote and grabs only the lines for the named\nrefs from the latter.\n\nSigned-off-by: Junio C Hamano \n---\n\n Linus Torvalds writes:\n > On Tue, 3 Apr 2007, Linus Torvalds wrote:\n >> \n >> Yes, we could definitely skip the re-lookup if we had a \"don't really \n >> care, I can recreate the object myself\" flag (ie anybody who is going to \n >> write that object)\n >\n > Side note: with \"alternates\" files, you might well *always* have the \n > objects. If you do\n >\n > \tgit clone -l -s ...\n >\n > to create various branches, and then pull between them, you'll actually \n > end up in the situation that you'll always find the objects and get back \n > to the really expensive case..\n\n Ah, that's true. If you \"git clone -l -s A B\", create new\n objects in A and pull from B, the transfer would not exclude\n new objects as they are not visible from B's refs.\n\n In that scenario, the keep-pack behaviour is already worse than\n the unpack-objects behaviour. The former creates a packfile\n that duplicates objects that are in A while the latter, although\n expensive, ends up doing nothing.\n\n I wonder if we can have a backdoor to avoid any object transfer\n in such a case to begin with...\n\n ... and this two patch series does exactly that.\n\n builtin-fetch--tool.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 84 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c\nindex e9d6764..be341c1 100644\n--- a/builtin-fetch--tool.c\n+++ b/builtin-fetch--tool.c\n@@ -436,10 +436,87 @@ static int expand_refs_wildcard(const char *ls_remote_result, int numrefs,\n \treturn 0;\n }\n \n+static int pick_rref(int sha1_only, const char *rref, const char *ls_remote_result)\n+{\n+\tint err = 0;\n+\tint lrr_count = lrr_count, i, pass;\n+\tconst char *cp;\n+\tstruct lrr {\n+\t\tconst char *line;\n+\t\tconst char *name;\n+\t\tint namelen;\n+\t\tint shown;\n+\t} *lrr_list = lrr_list;\n+\n+\tfor (pass = 0; pass < 2; pass++) {\n+\t\t/* pass 0 counts and allocates, pass 1 fills... */\n+\t\tcp = ls_remote_result;\n+\t\ti = 0;\n+\t\twhile (1) {\n+\t\t\tconst char *np;\n+\t\t\twhile (*cp && isspace(*cp))\n+\t\t\t\tcp++;\n+\t\t\tif (!*cp)\n+\t\t\t\tbreak;\n+\t\t\tnp = strchr(cp, '\\n');\n+\t\t\tif (!np)\n+\t\t\t\tnp = cp + strlen(cp);\n+\t\t\tif (pass) {\n+\t\t\t\tlrr_list[i].line = cp;\n+\t\t\t\tlrr_list[i].name = cp + 41;\n+\t\t\t\tlrr_list[i].namelen = np - (cp + 41);\n+\t\t\t}\n+\t\t\ti++;\n+\t\t\tcp = np;\n+\t\t}\n+\t\tif (!pass) {\n+\t\t\tlrr_count = i;\n+\t\t\tlrr_list = xcalloc(lrr_count, sizeof(*lrr_list));\n+\t\t}\n+\t}\n+\n+\twhile (1) {\n+\t\tconst char *next;\n+\t\tint rreflen;\n+\t\tint i;\n+\n+\t\twhile (*rref && isspace(*rref))\n+\t\t\trref++;\n+\t\tif (!*rref)\n+\t\t\tbreak;\n+\t\tnext = strchr(rref, '\\n');\n+\t\tif (!next)\n+\t\t\tnext = rref + strlen(rref);\n+\t\trreflen = next - rref;\n+\n+\t\tfor (i = 0; i < lrr_count; i++) {\n+\t\t\tstruct lrr *lrr = &(lrr_list[i]);\n+\n+\t\t\tif (rreflen == lrr->namelen &&\n+\t\t\t !memcmp(lrr->name, rref, rreflen)) {\n+\t\t\t\tif (!lrr->shown)\n+\t\t\t\t\tprintf(\"%.*s\\n\",\n+\t\t\t\t\t sha1_only ? 40 : lrr->namelen + 41,\n+\t\t\t\t\t lrr->line);\n+\t\t\t\tlrr->shown = 1;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\t\tif (lrr_count <= i) {\n+\t\t\terror(\"pick-rref: %.*s not found\", rreflen, rref);\n+\t\t\terr = 1;\n+\t\t}\n+\t\trref = next;\n+\t}\n+\tfree(lrr_list);\n+\treturn err;\n+}\n+\n int cmd_fetch__tool(int argc, const char **argv, const char *prefix)\n {\n \tint verbose = 0;\n \tint force = 0;\n+\tint sopt = 0;\n \n \twhile (1 < argc) {\n \t\tconst char *arg = argv[1];\n@@ -447,6 +524,8 @@ int cmd_fetch__tool(int argc, const char **argv, const char *prefix)\n \t\t\tverbose = 1;\n \t\telse if (!strcmp(\"-f\", arg))\n \t\t\tforce = 1;\n+\t\telse if (!strcmp(\"-s\", arg))\n+\t\t\tsopt = 1;\n \t\telse\n \t\t\tbreak;\n \t\targc--;\n@@ -491,6 +570,11 @@ int cmd_fetch__tool(int argc, const char **argv, const char *prefix)\n \t\t\treflist = get_stdin();\n \t\treturn parse_reflist(reflist);\n \t}\n+\tif (!strcmp(\"pick-rref\", argv[1])) {\n+\t\tif (argc != 4)\n+\t\t\treturn error(\"pick-rref takes 2 args\");\n+\t\treturn pick_rref(sopt, argv[2], argv[3]);\n+\t}\n \tif (!strcmp(\"expand-refs-wildcard\", argv[1])) {\n \t\tconst char *reflist;\n \t\tif (argc < 4)\n-- \n1.5.1.45.g1ddb\n"},{"id":"38691","messageId":"7v1wizrugw.fsf@assigned-by-dhcp.cox.net","threadId":"7502","inReplyTo":"Pine.LNX.4.64.0704031427050.6730@woody.linux-foundation.org","subject":"[PATCH 2/2] git-fetch: use fetch--tool pick-rref to avoid local fetch from alternate","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-04-05T10:22:55Z","receivedAt":"2007-04-05T10:22:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When we are fetching from a repository that is on a local\nfilesystem, first check if we have all the objects that we are\ngoing to fetch available locally, by not just checking the tips\nof what we are fetching, but with a full reachability analysis\nto our existing refs. In such a case, we do not have to run\ngit-fetch-pack which would send many needless objects. This is\nespecially true when the other repository is an alternate of the\ncurrent repository (e.g. perhaps the repository was created by\nrunning \"git clone -l -s\" from there).\n\nThe useless objects transferred used to be discarded when they\nwere expanded by git-unpack-objects called from git-fetch-pack,\nbut recent git-fetch-pack prefers to keep the data it receives\nfrom the other end without exploding them into loose objects,\nresulting in a pack full of duplicated data when fetching from\nyour own alternate.\n\nThis also uses fetch--tool pick-rref on dumb transport side to\nremove a shell loop to do the same.\n\nSigned-off-by: Junio C Hamano \n---\n\n * Strictly speaking, there is no need to even check if $remote\n is a local directory for this to operate properly, as\n rev-list would barf and die as soon as it finds something\n unavailable, while limiting the traversal to stop immediately\n after it hits what are known to be reachable locally. On the\n other hand, if we really want to limit this to the case to a\n repository with an alternate to \"clone -l -s\" origin, we\n could add 'test -f \"$GIT_OBJECT_DIRECTORY/info/alternates\"',\n but I chose not to.\n\n git-fetch.sh | 41 ++++++++++++++++++++++++++++-------------\n 1 files changed, 28 insertions(+), 13 deletions(-)\n\ndiff --git a/git-fetch.sh b/git-fetch.sh\nindex fd70696..5dc3063 100755\n--- a/git-fetch.sh\n+++ b/git-fetch.sh\n@@ -173,9 +173,32 @@ fetch_all_at_once () {\n \t git-bundle unbundle \"$remote\" $rref ||\n \t echo failed \"$remote\"\n \telse\n-\t git-fetch-pack --thin $exec $keep $shallow_depth $no_progress \\\n-\t\t\"$remote\" $rref ||\n-\t echo failed \"$remote\"\n+\t\tif\ttest -d \"$remote\" &&\n+\n+\t\t\t# The remote might be our alternate. With\n+\t\t\t# this optimization we will bypass fetch-pack\n+\t\t\t# altogether, which means we cannot be doing\n+\t\t\t# the shallow stuff at all.\n+\t\t\ttest ! -f \"$GIT_DIR/shallow\" &&\n+\t\t\ttest -z \"$shallow_depth\" &&\n+\n+\t\t\t# See if all of what we are going to fetch are\n+\t\t\t# connected to our repository's tips, in which\n+\t\t\t# case we do not have to do any fetch.\n+\t\t\ttheirs=$(git-fetch--tool -s pick-rref \\\n+\t\t\t\t\t\"$rref\" \"$ls_remote_result\") &&\n+\n+\t\t\t# This will barf when $theirs reach an object that\n+\t\t\t# we do not have in our repository. Otherwise,\n+\t\t\t# we already have everything the fetch would bring in.\n+\t\t\tgit-rev-list --objects $theirs --not --all 2>/dev/null\n+\t\tthen\n+\t\t\tgit-fetch--tool pick-rref \"$rref\" \"$ls_remote_result\"\n+\t\telse\n+\t\t\tgit-fetch-pack --thin $exec $keep $shallow_depth \\\n+\t\t\t\t$no_progress \"$remote\" $rref ||\n+\t\t\techo failed \"$remote\"\n+\t\tfi\n \tfi\n ) |\n (\n@@ -235,16 +258,8 @@ fetch_per_ref () {\n \t fi\n \n \t # Find $remote_name from ls-remote output.\n-\t head=$(\n-\t\tIFS='\t'\n-\t\techo \"$ls_remote_result\" |\n-\t\twhile read sha1 name\n-\t\tdo\n-\t\t\ttest \"z$name\" = \"z$remote_name\" || continue\n-\t\t\techo \"$sha1\"\n-\t\t\tbreak\n-\t\tdone\n-\t )\n+\t head=$(git-fetch--tool -s pick-rref \\\n+\t\t\t\"$remote_name\" \"$ls_remote_result\")\n \t expr \"z$head\" : \"z$_x40\\$\" >/dev/null ||\n \t\tdie \"No such ref $remote_name at $remote\"\n \t echo >&2 \"Fetching $remote_name from $remote using $proto\"\n-- \n1.5.1.45.g1ddb\n"},{"id":"38718","messageId":"20070405161537.GJ5436@spearce.org","threadId":"7502","inReplyTo":"7v1wizrugw.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 2/2] git-fetch: use fetch--tool pick-rref to avoid local fetch from alternate","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-04-05T16:15:37Z","receivedAt":"2007-04-05T16:15:37Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano wrote:\n> +\t\t\t# This will barf when $theirs reach an object that\n> +\t\t\t# we do not have in our repository. Otherwise,\n> +\t\t\t# we already have everything the fetch would bring in.\n> +\t\t\tgit-rev-list --objects $theirs --not --all 2>/dev/null\n\nOK, I must be missing something here.\n\nThat rev-list is going to print out the SHA-1s for the objects we\nwould have copied, but didn't, isn't it? So fetch--tool native-store\nis going to get a whole lot of SHA-1s it doesn't want to see, right?\n\nOtherwise this is a nice trick. It doesn't assure us that after the\nfetch those objects are still in the alternate. Meaning someone\ncould run prune in the alternate between the rev-list and the\nnative-store, and whack these objects. Given how small of a window\nit is, and the improvements this brings to alternates, I say its\nworth that small downside. Just don't prune while fetching. ;-)\n\n-- \nShawn.\n"},{"id":"38738","messageId":"7v7isqo63t.fsf@assigned-by-dhcp.cox.net","threadId":"7502","inReplyTo":"20070405161537.GJ5436@spearce.org","subject":"Re: [PATCH 2/2] git-fetch: use fetch--tool pick-rref to avoid local fetch from alternate","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-04-05T21:37:26Z","receivedAt":"2007-04-05T21:37:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n> Junio C Hamano wrote:\n>> +\t\t\t# This will barf when $theirs reach an object that\n>> +\t\t\t# we do not have in our repository. Otherwise,\n>> +\t\t\t# we already have everything the fetch would bring in.\n>> +\t\t\tgit-rev-list --objects $theirs --not --all 2>/dev/null\n>\n> OK, I must be missing something here.\n>\n> That rev-list is going to print out the SHA-1s for the objects we\n> would have copied, but didn't, isn't it? So fetch--tool native-store\n> is going to get a whole lot of SHA-1s it doesn't want to see, right?\n\nTrue. We should send the standard output also to /dev/null.\n\n> Otherwise this is a nice trick. It doesn't assure us that after the\n> fetch those objects are still in the alternate. Meaning someone\n> could run prune in the alternate between the rev-list and the\n> native-store, and whack these objects. Given how small of a window\n> it is, and the improvements this brings to alternates, I say its\n> worth that small downside. Just don't prune while fetching. ;-)\n\nThat is \"don't prune after making an alternate that depends on\nyou\" in general. Without this patch, and without the keep\n(i.e. when the fetch is very small and the transfarred pack is\ngiven to unpack-objects) you would have depended on the\nalternates for those objects anyway.\n"},{"id":"38813","messageId":"Pine.LNX.4.63.0704061455380.24050@qynat.qvtvafvgr.pbz","threadId":"7502","inReplyTo":"81b0412b0704040251j34b0bc5eh1518eadcfa2ed299@mail.gmail.com","subject":"Re: git-index-pack really does suck..","fromName":"David Lang","fromEmail":"david.lang@digitalinsight.com","sentAt":"2007-04-06T21:56:51Z","receivedAt":"2007-04-06T21:56:51Z","isPatch":false,"sender":{"key":"david.lang@digitalinsight.com","avatar":null},"body":"On Wed, 4 Apr 2007, Alex Riesen wrote:\n\n> On 4/4/07, David Lang wrote:\n>> \n>> > The keeping of fetched packs broke that presumption of trust towards\n>> > local objects and it opened a real path for potential future attacks.\n>> > Those attacks are still fairly theoretical of course. But for how\n>> > _long_? Do we want GIT to be considered backdoor prone in a couple\n>> > years from now just because we were obsessed by a 7% CPU overhead?\n>> >\n>> > I think we have much more to gain by playing it safe and being more\n>> > secure and paranoid than trying to squeeze some CPU cycles out of an\n>> > operation that is likely to ever be bounded by network speed for most\n>> > people.\n>> \n>> this is why -paranoid should be left on for network pulls, but having it on \n>> for\n>> the local uses means that the cost isn't hidden in the network limits isn't\n>> good.\n>\n> You never know what pull is networked (or should I say: remote enough\n> to cause a collision).\n\nso leave it on for all pulls, but for other commands don't turn it on.\n\nremember that the command that linus ran into at the start of the thread wasn't \na pull.\n\nDavid Lang\n"},{"id":"38815","messageId":"Pine.LNX.4.63.0704061521320.24050@qynat.qvtvafvgr.pbz","threadId":"7502","inReplyTo":"7vlkh5i0e7.fsf@assigned-by-dhcp.cox.net","subject":"Re: git-index-pack really does suck..","fromName":"David Lang","fromEmail":"david.lang@digitalinsight.com","sentAt":"2007-04-06T22:22:19Z","receivedAt":"2007-04-06T22:22:19Z","isPatch":false,"sender":{"key":"david.lang@digitalinsight.com","avatar":null},"body":"On Fri, 6 Apr 2007, Junio C Hamano wrote:\n\n> Subject: Re: git-index-pack really does suck..\n> \n> Junio C Hamano writes:\n>\n>> David Lang writes:\n>>\n>>> On Wed, 4 Apr 2007, Alex Riesen wrote:\n>>> ...\n>>>> You never know what pull is networked (or should I say: remote enough\n>>>> to cause a collision).\n>>>\n>>> so leave it on for all pulls, but for other commands don't turn it on.\n>>>\n>>> remember that the command that linus ran into at the start of the\n>>> thread wasn't a pull.\n>>\n>> Are you referring to this command\n>>\n>> $ git index-pack --stdin --fix-thin new.pack < .git/objects/pack/pack-*.pack\n>>\n>> in this message?\n>\n> From: Linus Torvalds \n> Subject: git-index-pack really does suck..\n> Date: Tue, 3 Apr 2007 08:15:12 -0700 (PDT)\n> Message-ID: \n>\n> (sorry, chomped the message).\n\nprobably (I useually don't keep the mail after I read or reply to it)\n\nDavid Lang\n"},{"id":"38820","messageId":"Pine.LNX.4.63.0704061526280.24050@qynat.qvtvafvgr.pbz","threadId":"7502","inReplyTo":"7vhcrti04x.fsf@assigned-by-dhcp.cox.net","subject":"Re: git-index-pack really does suck..","fromName":"David Lang","fromEmail":"david.lang@digitalinsight.com","sentAt":"2007-04-06T22:28:31Z","receivedAt":"2007-04-06T22:28:31Z","isPatch":false,"sender":{"key":"david.lang@digitalinsight.com","avatar":null},"body":"On Fri, 6 Apr 2007, Junio C Hamano wrote:\n\n> David Lang writes:\n>\n>> On Fri, 6 Apr 2007, Junio C Hamano wrote:\n>>\n>>> Subject: Re: git-index-pack really does suck..\n>>>\n>>> Junio C Hamano writes:\n>>>\n>>>> David Lang writes:\n>>>>\n>>>>> On Wed, 4 Apr 2007, Alex Riesen wrote:\n>>>>> ...\n>>>>>> You never know what pull is networked (or should I say: remote enough\n>>>>>> to cause a collision).\n>>>>>\n>>>>> so leave it on for all pulls, but for other commands don't turn it on.\n>>>>>\n>>>>> remember that the command that linus ran into at the start of the\n>>>>> thread wasn't a pull.\n>>>>\n>>>> Are you referring to this command\n>>>>\n>>>> $ git index-pack --stdin --fix-thin new.pack < .git/objects/pack/pack-*.pack\n>>>>\n>>>> in this message?\n>>>\n>>> From: Linus Torvalds \n>>> Subject: git-index-pack really does suck..\n>>> Date: Tue, 3 Apr 2007 08:15:12 -0700 (PDT)\n>>> Message-ID: \n>>>\n>>> (sorry, chomped the message).\n>>\n>> probably (I useually don't keep the mail after I read or reply to it)\n>\n> Well, then you should remember that the command linus ran into\n> was pretty much about pull, nothing else.\n>\n> The quoted command was only to illustrate what 'git-pull'\n> invokes internally. I do not think of any reason to use that\n> command for cases other than 'git-pull'. What's the use case\n> you have in mind to run that command outside of the context of\n> git-pull?\n\nI guess I'm not remembering the thread accurately then (and/or am mising it up \nwith a different thread). I thought that Linus had identified other cases that \nwere impacted (something about proving that the object doesn't exist)\n\nDavid Lang\n"},{"id":"38808","messageId":"7vslbdi0hf.fsf@assigned-by-dhcp.cox.net","threadId":"7502","inReplyTo":"Pine.LNX.4.63.0704061455380.24050@qynat.qvtvafvgr.pbz","subject":"Re: git-index-pack really does suck..","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-04-06T22:47:40Z","receivedAt":"2007-04-06T22:47:40Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Lang writes:\n\n> On Wed, 4 Apr 2007, Alex Riesen wrote:\n> ...\n>> You never know what pull is networked (or should I say: remote enough\n>> to cause a collision).\n>\n> so leave it on for all pulls, but for other commands don't turn it on.\n>\n> remember that the command that linus ran into at the start of the\n> thread wasn't a pull.\n\nAre you referring to this command\n\n $ git index-pack --stdin --fix-thin new.pack < .git/objects/pack/pack-*.pack\n\nin this message?\n"},{"id":"38812","messageId":"7vlkh5i0e7.fsf@assigned-by-dhcp.cox.net","threadId":"7502","inReplyTo":"7vslbdi0hf.fsf@assigned-by-dhcp.cox.net","subject":"Re: git-index-pack really does suck..","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-04-06T22:49:36Z","receivedAt":"2007-04-06T22:49:36Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> David Lang writes:\n>\n>> On Wed, 4 Apr 2007, Alex Riesen wrote:\n>> ...\n>>> You never know what pull is networked (or should I say: remote enough\n>>> to cause a collision).\n>>\n>> so leave it on for all pulls, but for other commands don't turn it on.\n>>\n>> remember that the command that linus ran into at the start of the\n>> thread wasn't a pull.\n>\n> Are you referring to this command\n>\n> $ git index-pack --stdin --fix-thin new.pack < .git/objects/pack/pack-*.pack\n>\n> in this message?\n\n From: Linus Torvalds \n Subject: git-index-pack really does suck..\n Date: Tue, 3 Apr 2007 08:15:12 -0700 (PDT)\n Message-ID: \n\n(sorry, chomped the message).\n"},{"id":"38816","messageId":"7vhcrti04x.fsf@assigned-by-dhcp.cox.net","threadId":"7502","inReplyTo":"Pine.LNX.4.63.0704061521320.24050@qynat.qvtvafvgr.pbz","subject":"Re: git-index-pack really does suck..","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-04-06T22:55:10Z","receivedAt":"2007-04-06T22:55:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Lang writes:\n\n> On Fri, 6 Apr 2007, Junio C Hamano wrote:\n>\n>> Subject: Re: git-index-pack really does suck..\n>>\n>> Junio C Hamano writes:\n>>\n>>> David Lang writes:\n>>>\n>>>> On Wed, 4 Apr 2007, Alex Riesen wrote:\n>>>> ...\n>>>>> You never know what pull is networked (or should I say: remote enough\n>>>>> to cause a collision).\n>>>>\n>>>> so leave it on for all pulls, but for other commands don't turn it on.\n>>>>\n>>>> remember that the command that linus ran into at the start of the\n>>>> thread wasn't a pull.\n>>>\n>>> Are you referring to this command\n>>>\n>>> $ git index-pack --stdin --fix-thin new.pack < .git/objects/pack/pack-*.pack\n>>>\n>>> in this message?\n>>\n>> From: Linus Torvalds \n>> Subject: git-index-pack really does suck..\n>> Date: Tue, 3 Apr 2007 08:15:12 -0700 (PDT)\n>> Message-ID: \n>>\n>> (sorry, chomped the message).\n>\n> probably (I useually don't keep the mail after I read or reply to it)\n\nWell, then you should remember that the command linus ran into\nwas pretty much about pull, nothing else.\n\nThe quoted command was only to illustrate what 'git-pull'\ninvokes internally. I do not think of any reason to use that\ncommand for cases other than 'git-pull'. What's the use case\nyou have in mind to run that command outside of the context of\ngit-pull?\n"}]}