{"thread":{"id":"11633","subject":"Be more careful about updating refs","startedAt":"2008-01-15T23:50:17Z","lastAt":"2008-01-23T22:53:05Z","messageCount":23,"participants":["Linus Torvalds","Junio C Hamano","Charles Bailey","Johannes Sixt","Johannes Schindelin","Shawn O. Pearce","Sam Vilain"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"65437","messageId":"alpine.LFD.1.00.0801151546560.2806@woody.linux-foundation.org","threadId":"11633","inReplyTo":null,"subject":"Be more careful about updating refs","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-01-15T23:50:17Z","receivedAt":"2008-01-15T23:50:17Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nThis makes write_ref_sha1() more careful: it actually checks the SHA1 of \nthe ref it is updating, and refuses to update a ref with an object that it \ncannot find.\n\nPerhaps more importantly, it also refuses to update a branch head with a \nnon-commit object. I don't quite know *how* the stable series maintainers \nwere able to corrupt their repository to have a HEAD that pointed to a tag \nrather than a commit object, but they did. Which results in a totally \nbroken repository that cannot be cloned or committed on.\n\nSo make it harder for people to shoot themselves in the foot like that.\n\nSigned-off-by: Linus Torvalds <torvalds@linux-foundation.org>\n---\n\nI'm signing off on this, but I hope people will double-check this: I \ndidn't actually test it very much.\n\n refs.c |   19 +++++++++++++++++++\n 1 files changed, 19 insertions(+), 0 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 58f6d17..c3ffe03 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1119,10 +1119,16 @@ static int log_ref_write(const char *ref_name, const unsigned char *old_sha1,\n \treturn 0;\n }\n \n+static int is_branch(const char *refname)\n+{\n+\treturn !strcmp(refname, \"HEAD\") || !prefixcmp(refname, \"refs/heads/\");\n+}\n+\n int write_ref_sha1(struct ref_lock *lock,\n \tconst unsigned char *sha1, const char *logmsg)\n {\n \tstatic char term = '\\n';\n+\tstruct object *o;\n \n \tif (!lock)\n \t\treturn -1;\n@@ -1130,6 +1136,19 @@ int write_ref_sha1(struct ref_lock *lock,\n \t\tunlock_ref(lock);\n \t\treturn 0;\n \t}\n+\to = parse_object(sha1);\n+\tif (!o) {\n+\t\terror(\"Trying to write ref %s with nonexistant object %s\",\n+\t\t\tlock->ref_name, sha1_to_hex(sha1));\n+\t\tunlock_ref(lock);\n+\t\treturn -1;\n+\t}\n+\tif (o->type != OBJ_COMMIT && is_branch(lock->ref_name)) {\n+\t\terror(\"Trying to write non-commit object %s to branch %s\",\n+\t\t\tsha1_to_hex(sha1), lock->ref_name);\n+\t\tunlock_ref(lock);\n+\t\treturn -1;\n+\t}\n \tif (write_in_full(lock->lock_fd, sha1_to_hex(sha1), 40) != 40 ||\n \t    write_in_full(lock->lock_fd, &term, 1) != 1\n \t\t|| close(lock->lock_fd) < 0) {\n"},{"id":"65438","messageId":"alpine.LFD.1.00.0801151600140.2806@woody.linux-foundation.org","threadId":"11633","inReplyTo":"alpine.LFD.1.00.0801151546560.2806@woody.linux-foundation.org","subject":"Re: Be more careful about updating refs","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-01-16T00:02:22Z","receivedAt":"2008-01-16T00:02:22Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 15 Jan 2008, Linus Torvalds wrote:\n> \n> This makes write_ref_sha1() more careful: it actually checks the SHA1 of \n> the ref it is updating, and refuses to update a ref with an object that it \n> cannot find.\n\nSide note: this breaks some tests, because those tests do things like\n\n\tgit update-ref refs/heads/master 1111111111111111111111111111111111111111 &&\n\t\ttest 1111111111111111111111111111111111111111 = $(cat .git/refs/heads/master)\n\nwhich obviously won't work. I didn't update the tests, because I'm an evil \nperson who just finds it very onerous to touch tests, and I particularly \nhate tests that turn out to be wrong.\n\n(Pet peeve on mine: people fixing assert()'s by changing the source-code, \nwithout ever asking themselves whether maybe the assert itself was the \nbug).\n\n\t\tLinus\n"},{"id":"65443","messageId":"7vd4s2d33x.fsf@gitster.siamese.dyndns.org","threadId":"11633","inReplyTo":"alpine.LFD.1.00.0801151546560.2806@woody.linux-foundation.org","subject":"Re: Be more careful about updating refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-16T00:29:54Z","receivedAt":"2008-01-16T00:29:54Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>\n> ---\n>\n> I'm signing off on this, but I hope people will double-check this: I \n> didn't actually test it very much.\n\nDoes \"Signed-off-by:\" line mean something different for you than\nfor other people these days?\n\nI thought that the rule that applies to you (and only you) on\nthis list was that all patches from you are DCO certified and I\nam free to forge your signature on them, and the other rule that\napplies to everybody including you is that Signed-off-by: is\nabout DCO certification and not about anything else\n(e.g. author's confidence level about the patch).\n\nI am asking because you did not have S-o-b and did not say\nanything about the other patch about commit object creation.\nAlthough both patches looked sane to me.\n"},{"id":"65445","messageId":"alpine.LFD.1.00.0801151640530.2806@woody.linux-foundation.org","threadId":"11633","inReplyTo":"7vd4s2d33x.fsf@gitster.siamese.dyndns.org","subject":"Re: Be more careful about updating refs","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-01-16T00:42:02Z","receivedAt":"2008-01-16T00:42:02Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 15 Jan 2008, Junio C Hamano wrote:\n> \n> Does \"Signed-off-by:\" line mean something different for you than\n> for other people these days?\n\nI intentionally don't tend to sign off stuff that I just send out as trial \nballoons, not meant to \"really\" be applied.\n\nOf course, then I *also* forget to sign off emails that I do intend to be \napplied, so that rule-of-thumb is pretty weak ;)\n\n\t\tLinus\n"},{"id":"65449","messageId":"7vzlv6bmm3.fsf@gitster.siamese.dyndns.org","threadId":"11633","inReplyTo":"alpine.LFD.1.00.0801151546560.2806@woody.linux-foundation.org","subject":"Re: Be more careful about updating refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-16T01:11:32Z","receivedAt":"2008-01-16T01:11:32Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> This makes write_ref_sha1() more careful: it actually checks the SHA1 of \n> the ref it is updating, and refuses to update a ref with an object that it \n> cannot find.\n>\n> Perhaps more importantly, it also refuses to update a branch head with a \n> non-commit object. I don't quite know *how* the stable series maintainers \n> were able to corrupt their repository to have a HEAD that pointed to a tag \n> rather than a commit object, but they did. Which results in a totally \n> broken repository that cannot be cloned or committed on.\n\nTwo questions and a comment:\n\n - Do we want to impose the same restriction on refs/remotes/?\n   I think that is a logical thing to do.\n\n - What should the receive-pack and git-fetch do if they trigger\n   the check in this codepath while updating the refs under the\n   affected hierarchies?  Fail the push and fetch?\n\n - I think !strcmp(refname, \"HEAD\") is not quite a right check\n   to do here.  In order to catch the detached head case, it\n   must be checked, but at the same time if the head is not\n   detached, it should look at where the symref points at\n   (i.e. a symref HEAD that points outside refs/heads is an\n   error, and we need to catch that).\n\n> +static int is_branch(const char *refname)\n> +{\n> +\treturn !strcmp(refname, \"HEAD\") || !prefixcmp(refname, \"refs/heads/\");\n> +}\n"},{"id":"65591","messageId":"7vodblpmyc.fsf@gitster.siamese.dyndns.org","threadId":"11633","inReplyTo":"alpine.LFD.1.00.0801151600140.2806@woody.linux-foundation.org","subject":"Re: Be more careful about updating refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-16T19:52:43Z","receivedAt":"2008-01-16T19:52:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> On Tue, 15 Jan 2008, Linus Torvalds wrote:\n>> \n>> This makes write_ref_sha1() more careful: it actually checks the SHA1 of \n>> the ref it is updating, and refuses to update a ref with an object that it \n>> cannot find.\n>\n> Side note: this breaks some tests, because those tests do things like\n>\n> \tgit update-ref refs/heads/master 1111111111111111111111111111111111111111 &&\n> \t\ttest 1111111111111111111111111111111111111111 = $(cat .git/refs/heads/master)\n>\n> ...\n> (Pet peeve on mine: people fixing assert()'s by changing the source-code, \n> without ever asking themselves whether maybe the assert itself was the \n> bug).\n\nThe rules for the plumbing used to be that refs can point at\nanything that get_sha1() accepts.  We did not even require it to\nbe parse_object() happy let alone it being parse_commit() kosher.\n\nYou changed the world order.  I agree that the world order was\nchanged in a good way, but saying that the original test did not\ncheck the right thing or it was a bug is not quite fair.  At\nworst, we can say that it was very sloppily written by assuming\nthat the commands involved in the particular test would not care\nabout corrupted repositories whose refs point at nonexistant\nbogus objects.\n\nI'll squash the following to your patch.\n\n---\n t/t1400-update-ref.sh |   19 +++++++++++++------\n 1 files changed, 13 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex a90824b..71ab2dd 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -7,12 +7,19 @@ test_description='Test git update-ref and basic ref logging'\n . ./test-lib.sh\n \n Z=0000000000000000000000000000000000000000\n-A=1111111111111111111111111111111111111111\n-B=2222222222222222222222222222222222222222\n-C=3333333333333333333333333333333333333333\n-D=4444444444444444444444444444444444444444\n-E=5555555555555555555555555555555555555555\n-F=6666666666666666666666666666666666666666\n+\n+test_expect_success setup '\n+\n+\tfor name in A B C D E F\n+\tdo\n+\t\ttest_tick &&\n+\t\tT=$(git write-tree) &&\n+\t\tsha1=$(echo $name | git commit-tree $T) &&\n+\t\teval $name=$sha1\n+\tdone\n+\n+'\n+\n m=refs/heads/master\n n_dir=refs/heads/gu\n n=$n_dir/fixes\n"},{"id":"65697","messageId":"20080117091558.GA8341@hashpling.org","threadId":"11633","inReplyTo":"7vodblpmyc.fsf@gitster.siamese.dyndns.org","subject":"Re: Be more careful about updating refs","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2008-01-17T09:15:58Z","receivedAt":"2008-01-17T09:15:58Z","isPatch":false,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Wed, Jan 16, 2008 at 11:52:43AM -0800, Junio C Hamano wrote:\n> Linus Torvalds <torvalds@linux-foundation.org> writes:\n> \n> > On Tue, 15 Jan 2008, Linus Torvalds wrote:\n> >> \n> >> This makes write_ref_sha1() more careful: it actually checks the SHA1 of \n> >> the ref it is updating, and refuses to update a ref with an object that it \n> >> cannot find.\n> >\n> > Side note: this breaks some tests, because those tests do things like\n> >\n> > \tgit update-ref refs/heads/master 1111111111111111111111111111111111111111 &&\n> > \t\ttest 1111111111111111111111111111111111111111 = $(cat .git/refs/heads/master)\n> >\n> > ...\n> > (Pet peeve on mine: people fixing assert()'s by changing the source-code, \n> > without ever asking themselves whether maybe the assert itself was the \n> > bug).\n> \n> The rules for the plumbing used to be that refs can point at\n> anything that get_sha1() accepts.  We did not even require it to\n> be parse_object() happy let alone it being parse_commit() kosher.\n> \n> You changed the world order.  I agree that the world order was\n> changed in a good way, but saying that the original test did not\n> check the right thing or it was a bug is not quite fair.  At\n> worst, we can say that it was very sloppily written by assuming\n> that the commands involved in the particular test would not care\n> about corrupted repositories whose refs point at nonexistant\n> bogus objects.\n> \n> I'll squash the following to your patch.\n\nI'm assuming that this original patch and the test update turned into\nthe following commit in master:\n\nc3b0dec509fe136c5417422f31898b5a4e2d5e02 is first bad commit\n\nI just thought I should warn you that this seems (git bisect tells me\nso) to have caused a failure in t9301-fast-export.sh on my Mac OS X\n10.4.11 machine although I haven't yet had the time to investigate\nwhy.\n\nCharles.\n"},{"id":"65702","messageId":"478F3367.8050307@viscovery.net","threadId":"11633","inReplyTo":"20080117091558.GA8341@hashpling.org","subject":"Re: Be more careful about updating refs","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-01-17T10:52:23Z","receivedAt":"2008-01-17T10:52:23Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Charles Bailey schrieb:\n> I'm assuming that this original patch and the test update turned into\n> the following commit in master:\n> \n> c3b0dec509fe136c5417422f31898b5a4e2d5e02 is first bad commit\n> \n> I just thought I should warn you that this seems (git bisect tells me\n> so) to have caused a failure in t9301-fast-export.sh on my Mac OS X\n> 10.4.11 machine although I haven't yet had the time to investigate\n> why.\n\nI observed the same (on Windows). The reason is that above-mentioned\ncommit introduces a call to parse_objects(). But by the time that\nfast-import calls write_ref_sha1() (and, hence, this new parse_objects())\nit has not yet written a pack file, and parse_objects() fails. I don't\nhave a clue how to fix this short of reverting the commit.\n\n-- Hannes\n"},{"id":"65703","messageId":"20080117105635.GA12285@hashpling.org","threadId":"11633","inReplyTo":"20080117091558.GA8341@hashpling.org","subject":"Re: Be more careful about updating refs","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2008-01-17T10:56:35Z","receivedAt":"2008-01-17T10:56:35Z","isPatch":false,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Thu, Jan 17, 2008 at 09:15:58AM +0000, Charles Bailey wrote:\n> \n> c3b0dec509fe136c5417422f31898b5a4e2d5e02 is first bad commit\n> \n> I just thought I should warn you that this seems (git bisect tells me\n> so) to have caused a failure in t9301-fast-export.sh on my Mac OS X\n> 10.4.11 machine although I haven't yet had the time to investigate\n> why.\n> \n\nFurther to this, the first problem is that the 'fast-export |\nfast-import' test is failing.\n\nAs far as I can tell, fast-export is behaving no differently, the\ngenerated export file looks identical in shape.  It seems that\nfast-import is creating a bad temporary packfile for itself.\n\nThe error that I'm getting is:\n\nfatal: unknown object type 0 in\n.git/objects/pack/pack-6be2b92c2d7485fd5cefbb0d9a68827e4c23d548.pack\nfast-import: dumping crash report to .git/fast_import_crash_12317\n\nwhich seems to come from the unpack_entry function in sha1_file.c.\n\nI haven't got much deeper into how the refs.c change has caused the\nissue, or why on earth it seems to be a Mac only issue.\n"},{"id":"65704","messageId":"20080117110132.GB12285@hashpling.org","threadId":"11633","inReplyTo":"478F3367.8050307@viscovery.net","subject":"Re: Be more careful about updating refs","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2008-01-17T11:01:32Z","receivedAt":"2008-01-17T11:01:32Z","isPatch":false,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Thu, Jan 17, 2008 at 11:52:23AM +0100, Johannes Sixt wrote:\n> \n> I observed the same (on Windows). The reason is that above-mentioned\n> commit introduces a call to parse_objects(). But by the time that\n> fast-import calls write_ref_sha1() (and, hence, this new parse_objects())\n> it has not yet written a pack file, and parse_objects() fails. I don't\n> have a clue how to fix this short of reverting the commit.\n> \n\nOK, so it's not just Mac OS X, then.  From your description and my\ninitial poke at the code,  I can't immediately see a good reason why\nthe test should succeed on Linux, though.\n"},{"id":"65710","messageId":"478F4CDC.2090703@viscovery.net","threadId":"11633","inReplyTo":"20080117110132.GB12285@hashpling.org","subject":"Re: Be more careful about updating refs","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-01-17T12:41:00Z","receivedAt":"2008-01-17T12:41:00Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Charles Bailey schrieb:\n> On Thu, Jan 17, 2008 at 11:52:23AM +0100, Johannes Sixt wrote:\n>> I observed the same (on Windows). The reason is that above-mentioned\n>> commit introduces a call to parse_objects(). But by the time that\n>> fast-import calls write_ref_sha1() (and, hence, this new parse_objects())\n>> it has not yet written a pack file, and parse_objects() fails. I don't\n>> have a clue how to fix this short of reverting the commit.\n>>\n> \n> OK, so it's not just Mac OS X, then.  From your description and my\n> initial poke at the code,  I can't immediately see a good reason why\n> the test should succeed on Linux, though.\n\nMy analysis is not correct. The pack file is present, but it seems to be\nincomplete.\n\nThe reason is the NO_MMAP build flag. If you compile with\nNO_MMAP=YesPlease on Linux, t9301 fails as well. Does this ring a bell?\n\n-- Hannes\n"},{"id":"65713","messageId":"alpine.LSU.1.00.0801171256280.17650@racer.site","threadId":"11633","inReplyTo":"478F4CDC.2090703@viscovery.net","subject":"Re: Be more careful about updating refs","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-01-17T12:58:07Z","receivedAt":"2008-01-17T12:58:07Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 17 Jan 2008, Johannes Sixt wrote:\n\n> Charles Bailey schrieb:\n> > On Thu, Jan 17, 2008 at 11:52:23AM +0100, Johannes Sixt wrote:\n> >> I observed the same (on Windows). The reason is that above-mentioned\n> >> commit introduces a call to parse_objects(). But by the time that\n> >> fast-import calls write_ref_sha1() (and, hence, this new parse_objects())\n> >> it has not yet written a pack file, and parse_objects() fails. I don't\n> >> have a clue how to fix this short of reverting the commit.\n> >>\n> > \n> > OK, so it's not just Mac OS X, then.  From your description and my\n> > initial poke at the code,  I can't immediately see a good reason why\n> > the test should succeed on Linux, though.\n> \n> My analysis is not correct. The pack file is present, but it seems to be\n> incomplete.\n> \n> The reason is the NO_MMAP build flag. If you compile with\n> NO_MMAP=YesPlease on Linux, t9301 fails as well. Does this ring a bell?\n\nJust a wild guess... Could it be that the mmap() happened before contents \nwere written to that fd?  The faked mmap() is just a malloc() && pread(), \nand would thus miss out on changes written after the mmap() call.\n\nCiao,\nDscho\n"},{"id":"65715","messageId":"20080117130740.GA30090@hashpling.org","threadId":"11633","inReplyTo":"alpine.LSU.1.00.0801171256280.17650@racer.site","subject":"Re: Be more careful about updating refs","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2008-01-17T13:07:40Z","receivedAt":"2008-01-17T13:07:40Z","isPatch":false,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Thu, Jan 17, 2008 at 12:58:07PM +0000, Johannes Schindelin wrote:\n> \n> Just a wild guess... Could it be that the mmap() happened before contents \n> were written to that fd?  The faked mmap() is just a malloc() && pread(), \n> and would thus miss out on changes written after the mmap() call.\n\nThis would be my guess too.  As far as I can tell fast-import is\ncreating objects directly in a packfile.  I'm guessing here, but the\nnew checking code is probably being triggered to check (for example)\nthe parent ref for commits that fast-import is creating and the check\nonly works if mmap can successfully examine objects in the\nstill-being-written packfile.\n"},{"id":"65805","messageId":"7vbq7jkixg.fsf@gitster.siamese.dyndns.org","threadId":"11633","inReplyTo":"478F4CDC.2090703@viscovery.net","subject":"Re: Be more careful about updating refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-18T01:43:07Z","receivedAt":"2008-01-18T01:43:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j.sixt@viscovery.net> writes:\n\n> Charles Bailey schrieb:\n>> On Thu, Jan 17, 2008 at 11:52:23AM +0100, Johannes Sixt wrote:\n>>> I observed the same (on Windows). The reason is that above-mentioned\n>>> commit introduces a call to parse_objects(). But by the time that\n>>> fast-import calls write_ref_sha1() (and, hence, this new parse_objects())\n>>> it has not yet written a pack file, and parse_objects() fails. I don't\n>>> have a clue how to fix this short of reverting the commit.\n>>>\n>> \n>> OK, so it's not just Mac OS X, then.  From your description and my\n>> initial poke at the code,  I can't immediately see a good reason why\n>> the test should succeed on Linux, though.\n>\n> My analysis is not correct. The pack file is present, but it seems to be\n> incomplete.\n>\n> The reason is the NO_MMAP build flag. If you compile with\n> NO_MMAP=YesPlease on Linux, t9301 fails as well. Does this ring a bell?\n\nI ran strace and found that fast-import retains three windows to\nthe same data that was opened while the pack was still being\nbuilt (i.e. the filename is still tmp_pack_XXXXXX) when it dies:\n\n    {next = 0x6f6d20, base = 0x6f77a0 \"PACK\", offset = 0, len = 907,\n     last_used = 9, inuse_cnt = 0}\n    {next = 0x728630, base = 0x6f7160 \"PACK\", offset = 0, len = 500, \n      last_used = 5, inuse_cnt = 0}\n    {next = 0x0, base = 0x6f6d50 \"PACK\", offset = 0, len = 261, \n      last_used = 1, inuse_cnt = 0}\n\nWhen it hits end_packfile() to install the built-pack to its\nfinal destination and makes it available to the rest of git,\nhowever, it has this clever code to reuse the still open window.\n\nThe object offset asked when it dies is at offset 887.  However,\ntmp_pack was written after that last window with length 907 has\nbeen (re)opened, and reusing the window results in reading a\nstale data.\n\nThe following patch seems to fix the issue for me, but this is\nprimarily meant for discussion, as I do not quite understand why\nthe same issue does not manifest itself when NO_MMAP is not\nused.\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 3609c24..bd0ddb1 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -926,7 +926,13 @@ static void end_packfile(void)\n \t\tnew_p = add_packed_git(idx_name, strlen(idx_name), 1);\n \t\tif (!new_p)\n \t\t\tdie(\"core git rejected index %s\", idx_name);\n-\t\tnew_p->windows = old_p->windows;\n+\t\twhile (old_p->windows) {\n+\t\t\tstruct pack_window *w = old_p->windows;\n+\t\t\tmunmap(w->base, w->len);\n+\t\t\told_p->windows = w->next;\n+\t\t\tfree(w);\n+\t\t}\n+\t\tnew_p->windows = NULL;\n \t\tall_packs[pack_id] = new_p;\n \t\tinstall_packed_git(new_p);\n \n"},{"id":"65806","messageId":"7v63xrki29.fsf@gitster.siamese.dyndns.org","threadId":"11633","inReplyTo":"7vbq7jkixg.fsf@gitster.siamese.dyndns.org","subject":"Re: Be more careful about updating refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-18T02:01:50Z","receivedAt":"2008-01-18T02:01:50Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> The following patch seems to fix the issue for me, but this is\n> primarily meant for discussion, as I do not quite understand why\n> the same issue does not manifest itself when NO_MMAP is not\n> used.\n> ...\n\nBTW, the lookup of the object that dies is in update_branch().\nThe call to write_ref_sha1() at the last tries to verify b->sha1\nis available and is a commit.\n\n        static int update_branch(struct branch *b)\n        {\n                static const char *msg = \"fast-import\";\n                struct ref_lock *lock;\n                unsigned char old_sha1[20];\n\n                if (read_ref(b->name, old_sha1))\n                        hashclr(old_sha1);\n                lock = lock_any_ref_for_update(b->name, old_sha1, 0);\n                if (!lock)\n                        return error(\"Unable to lock %s\", b->name);\n                if (!force_update && !is_null_sha1(old_sha1)) {\n                        struct commit *old_cmit, *new_cmit;\n\n                        old_cmit = lookup_commit_reference_gently(old_sha1, 0);\n                        new_cmit = lookup_commit_reference_gently(b->sha1, 0);\n                        if (!old_cmit || !new_cmit) {\n                                unlock_ref(lock);\n                                return error(\"Branch %s is missing commits.\", b->name);\n                        }\n\n                        if (!in_merge_bases(old_cmit, &new_cmit, 1)) {\n                                unlock_ref(lock);\n                                warning(\"Not updating %s\"\n                                        \" (new tip %s does not contain %s)\",\n                                        b->name, sha1_to_hex(b->sha1), sha1_to_hex(old_sha1));\n                                return -1;\n                        }\n                }\n                if (write_ref_sha1(lock, b->sha1, msg) < 0)\n                        return error(\"Unable to update %s\", b->name);\n                return 0;\n        }\n\nThe write_ref_sha1() function previously did not even check if\nthe object pointed at by b->sha1 actually existed, but now it\nparses the object and makes sure it is a commit, if the updated\nref is under ref/heads/ hierarchy.\n\nWhat I do not quite understand is how this can be a new issue.\n\nThe codepath to allow updating an existing branch shown above\n(i.e. \"if it is not force and old is not NULL\") uses the usual\nlookup_commit_reference_gently() interface to access b->sha1,\nand does not use gfi-aware gfi_unpack_entry() or anything\nmagical, which means it would have passed the same codepath down\nto trigger the same issue.  IOW, even before this tightening of\nwrite_ref_sha1(), we already should have had the issue of not\nbeing to able to grab the object b->sha1 refers to out of the\nnewly built packfile.\n\nIs it just nobody seriously exercised the codepath yet, or is\nthere a difference between these two calls that is more subtle\nthan that?\n\nQuite confused...\n"},{"id":"65808","messageId":"20080118021300.GR24004@spearce.org","threadId":"11633","inReplyTo":"7v63xrki29.fsf@gitster.siamese.dyndns.org","subject":"Re: Be more careful about updating refs","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-01-18T02:13:00Z","receivedAt":"2008-01-18T02:13:00Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano <gitster@pobox.com> wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > The following patch seems to fix the issue for me, but this is\n> > primarily meant for discussion, as I do not quite understand why\n> > the same issue does not manifest itself when NO_MMAP is not\n> > used.\n> > ...\n...\n> What I do not quite understand is how this can be a new issue.\n\nIt isn't.  It is indeed a pretty old issue, as far as git issues go.\nIts probably about as old as fast-import being accepted into your\ntree is.\n \n> The codepath to allow updating an existing branch shown above\n> (i.e. \"if it is not force and old is not NULL\") uses the usual\n> lookup_commit_reference_gently() interface to access b->sha1,\n> and does not use gfi-aware gfi_unpack_entry() or anything\n> magical, which means it would have passed the same codepath down\n> to trigger the same issue.  IOW, even before this tightening of\n> write_ref_sha1(), we already should have had the issue of not\n> being to able to grab the object b->sha1 refers to out of the\n> newly built packfile.\n> \n> Is it just nobody seriously exercised the codepath yet, or is\n> there a difference between these two calls that is more subtle\n> than that?\n\nI think the problem is nobody has tested fast-import updating an\nexisting ref while using NO_MMAP.  Or if they did, they didn't report\nthe problem as they didn't figure they needed fast-import that badly.\n\nUpdating an existing ref is not a common operation, but the test\nsuite does test for it.  So it must be the NO_MMAP configuration\nis simply not being tested well enough.\n\n-- \nShawn.\n"},{"id":"65809","messageId":"7v1w8fkgy9.fsf@gitster.siamese.dyndns.org","threadId":"11633","inReplyTo":"20080118021300.GR24004@spearce.org","subject":"Re: Be more careful about updating refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-18T02:25:50Z","receivedAt":"2008-01-18T02:25:50Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" <spearce@spearce.org> writes:\n\n> I think the problem is nobody has tested fast-import updating an\n> existing ref while using NO_MMAP.  Or if they did, they didn't report\n> the problem as they didn't figure they needed fast-import that badly.\n>\n> Updating an existing ref is not a common operation, but the test\n> suite does test for it.  So it must be the NO_MMAP configuration\n> is simply not being tested well enough.\n\nOk, thanks.\n\nNow a more important question is how we would properly fix this\nissue?\n\nI suspect that fast-import is the only one that opens windows\ninto an unfinalized pack, and if that is the case, it would be\nthe only program that may be hit by the issue of mmap emulation\ngetting stale data.\n\nI do not think the patch I posted was correct at all.\n\nEspecially, I am not sure if the issue only exists at the\nend_packfile() boundary.  Don't we have the same issue reading\nfrom the packfile being built, and isn't the only reason my hack\nworks it around is because access patterns of the testsuite\nhappens to not trigger it?\n"},{"id":"65810","messageId":"20080118023005.GS24004@spearce.org","threadId":"11633","inReplyTo":"7vbq7jkixg.fsf@gitster.siamese.dyndns.org","subject":"Re: Be more careful about updating refs","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-01-18T02:30:06Z","receivedAt":"2008-01-18T02:30:06Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano <gitster@pobox.com> wrote:\n> I ran strace and found that fast-import retains three windows to\n> the same data that was opened while the pack was still being\n> built (i.e. the filename is still tmp_pack_XXXXXX) when it dies:\n> \n>     {next = 0x6f6d20, base = 0x6f77a0 \"PACK\", offset = 0, len = 907,\n>      last_used = 9, inuse_cnt = 0}\n>     {next = 0x728630, base = 0x6f7160 \"PACK\", offset = 0, len = 500, \n>       last_used = 5, inuse_cnt = 0}\n>     {next = 0x0, base = 0x6f6d50 \"PACK\", offset = 0, len = 261, \n>       last_used = 1, inuse_cnt = 0}\n\nOuch.  Hmm, well, maybe it shouldn't keep three windows open to\nthe same part of the file, but with different lengths.  :)\n\nBut that's another issue.\n \n> The following patch seems to fix the issue for me, but this is\n> primarily meant for discussion,\n\nI agree the patch should fix this particular issue and the\nperformance difference it will cause is minor enough to not be\nworth discussing further.  But I have one minor comment (please\nsee it below).\n\n> as I do not quite understand why\n> the same issue does not manifest itself when NO_MMAP is not\n> used.\n\nWhen NO_MMAP is off (we are actually using the OS's true mmap)\nthe data is updated into the window when the file is written,\nas the window is backed by the OS's filesystem cache.\n\nIf the data access is outside of the window (past the offset of\nthe longest window, here 907) we would just open a new window to\nthe relevant region of the file.  But when it is inside the window,\nthe window being backed by the filesystem cache saved us.  In the\ncase of NO_MMAP this doesn't work, so we get a failure later during\nobject access.\n\nIn particular if you look at gfi_unpack_entry() (which is where\nwe cause windows to be opened on the file being created) we tell\nsha1_file.c it has 20 bytes available in the window *beyond* the\nactual end of the file.  This is due to an assumption within the\nwindowing code of sha1_file.c that we always have the packfile\nfooter at the end of the file, so all windowing code assumes it\nhas at least 20 bytes from the start of a window that it can access\nwithout needing to perform additional bounds checking.\n\nSo what's happening here is we are adding objects into the file,\nsome of which may have their data appearing in the trailing 20 bytes\nof some prior window.  If that happens the cached window looks like\nit can be used to access the data, but it really cannot be in the\nNO_MMAP case as those trailing 20 bytes of the window are all \\0's.\n\nThe astute reader may wonder how gfi_unpack_entry() works when\nNO_MMAP is being used.  It doesn't for that last 20 bytes of\nany window.  Which has me thinking that Junio's patch alone isn't\nenough to make fast-import work correctly under NO_MMAP.\n\n> diff --git a/fast-import.c b/fast-import.c\n> index 3609c24..bd0ddb1 100644\n> --- a/fast-import.c\n> +++ b/fast-import.c\n> @@ -926,7 +926,13 @@ static void end_packfile(void)\n>  \t\tnew_p = add_packed_git(idx_name, strlen(idx_name), 1);\n>  \t\tif (!new_p)\n>  \t\t\tdie(\"core git rejected index %s\", idx_name);\n> -\t\tnew_p->windows = old_p->windows;\n> +\t\twhile (old_p->windows) {\n> +\t\t\tstruct pack_window *w = old_p->windows;\n> +\t\t\tmunmap(w->base, w->len);\n> +\t\t\told_p->windows = w->next;\n> +\t\t\tfree(w);\n> +\t\t}\n> +\t\tnew_p->windows = NULL;\n\nThis assignment of NULL should not be necessary.  add_packed_git()\nalready does this for us on new_p so we do not need to repeat it\nhere in fast-import.\n\n>  \t\tall_packs[pack_id] = new_p;\n>  \t\tinstall_packed_git(new_p);\n>  \n\n-- \nShawn.\n"},{"id":"65811","messageId":"20080118023327.GT24004@spearce.org","threadId":"11633","inReplyTo":"7v1w8fkgy9.fsf@gitster.siamese.dyndns.org","subject":"Re: Be more careful about updating refs","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-01-18T02:33:27Z","receivedAt":"2008-01-18T02:33:27Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano <gitster@pobox.com> wrote:\n> \"Shawn O. Pearce\" <spearce@spearce.org> writes:\n> \n> > I think the problem is nobody has tested fast-import updating an\n> > existing ref while using NO_MMAP.  Or if they did, they didn't report\n> > the problem as they didn't figure they needed fast-import that badly.\n> >\n> > Updating an existing ref is not a common operation, but the test\n> > suite does test for it.  So it must be the NO_MMAP configuration\n> > is simply not being tested well enough.\n> \n> Now a more important question is how we would properly fix this\n> issue?\n> \n> I suspect that fast-import is the only one that opens windows\n> into an unfinalized pack, and if that is the case, it would be\n> the only program that may be hit by the issue of mmap emulation\n> getting stale data.\n\nYes, it is the only program that is foolish enough to access a\npartial packfile.  index-pack uses pread(), and only after it has\nthe entire packfile downloaded to the local system.  The only other\nreader of a partial packfile is unpack-objects, and obviously that\ndoesn't care about seeking backwards within the packfile.\n\nSo its probably the only user who suffers from the mmap emulation.\n \n> I do not think the patch I posted was correct at all.\n> \n> Especially, I am not sure if the issue only exists at the\n> end_packfile() boundary.  Don't we have the same issue reading\n> from the packfile being built, and isn't the only reason my hack\n> works it around is because access patterns of the testsuite\n> happens to not trigger it?\n\nYes, that's my take on it as well (see my other email).  The\ntestsuite must just be really lucky that its not hitting the\nboundary condition.\n\nI almost said gfi_unpack_entry() was immune from this bug, but\nI went back and read the code again and determined that it does\nin fact suffer from this under NO_MMAP, and we're just really\ndamn lucky nobody has caused it.\n\nI'll try to work up a patch this evening.\n\n-- \nShawn.\n"},{"id":"65812","messageId":"20080118025837.GU24004@spearce.org","threadId":"11633","inReplyTo":"20080118023327.GT24004@spearce.org","subject":"Re: Be more careful about updating refs","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-01-18T02:58:37Z","receivedAt":"2008-01-18T02:58:37Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"\"Shawn O. Pearce\" <spearce@spearce.org> wrote:\n> Junio C Hamano <gitster@pobox.com> wrote:\n> > \n> > Especially, I am not sure if the issue only exists at the\n> > end_packfile() boundary.  Don't we have the same issue reading\n> > from the packfile being built, and isn't the only reason my hack\n> > works it around is because access patterns of the testsuite\n> > happens to not trigger it?\n> \n> Yes, that's my take on it as well (see my other email).  The\n> testsuite must just be really lucky that its not hitting the\n> boundary condition.\n> \n> I almost said gfi_unpack_entry() was immune from this bug, but\n> I went back and read the code again and determined that it does\n> in fact suffer from this under NO_MMAP, and we're just really\n> damn lucky nobody has caused it.\n\nI think this solves the problem.  Its based on your first patch, but\nwould replace it.  The trick here is we close the cached windows if\nwe are accessing data from the packfile we are appending into and we\nhave increased the file length.  This way we don't blow away windows\nduring high read/low write periods, like during branch cache reloads.\n\n--8>--\ndiff --git a/fast-import.c b/fast-import.c\nindex 3609c24..8e7747c 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -907,6 +907,16 @@ static void unkeep_all_packs(void)\n \t}\n }\n \n+static void close_all_windows(struct packed_git *p)\n+{\n+\twhile (p->windows) {\n+\t\tstruct pack_window *w = p->windows;\n+\t\tmunmap(w->base, w->len);\n+\t\tp->windows = w->next;\n+\t\tfree(w);\n+\t}\n+}\n+\n static void end_packfile(void)\n {\n \tstruct packed_git *old_p = pack_data, *new_p;\n@@ -917,6 +927,7 @@ static void end_packfile(void)\n \t\tstruct branch *b;\n \t\tstruct tag *t;\n \n+\t\tclose_all_windows(pack_data);\n \t\tfixup_pack_header_footer(pack_data->pack_fd, pack_data->sha1,\n \t\t\t\t    pack_data->pack_name, object_count);\n \t\tclose(pack_data->pack_fd);\n@@ -926,7 +937,6 @@ static void end_packfile(void)\n \t\tnew_p = add_packed_git(idx_name, strlen(idx_name), 1);\n \t\tif (!new_p)\n \t\t\tdie(\"core git rejected index %s\", idx_name);\n-\t\tnew_p->windows = old_p->windows;\n \t\tall_packs[pack_id] = new_p;\n \t\tinstall_packed_git(new_p);\n \n@@ -1129,8 +1139,10 @@ static void *gfi_unpack_entry(\n {\n \tenum object_type type;\n \tstruct packed_git *p = all_packs[oe->pack_id];\n-\tif (p == pack_data)\n+\tif (p == pack_data && p->pack_size < (pack_size + 20)) {\n+\t\tclose_all_windows(p);\n \t\tp->pack_size = pack_size + 20;\n+\t}\n \treturn unpack_entry(p, oe->offset, &type, sizep);\n }\n \n\n-- \nShawn.\n"},{"id":"65815","messageId":"20080118031831.GW24004@spearce.org","threadId":"11633","inReplyTo":"20080118025837.GU24004@spearce.org","subject":"Re: Be more careful about updating refs","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-01-18T03:18:31Z","receivedAt":"2008-01-18T03:18:31Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"\"Shawn O. Pearce\" <spearce@spearce.org> wrote:\n> \"Shawn O. Pearce\" <spearce@spearce.org> wrote:\n> > Junio C Hamano <gitster@pobox.com> wrote:\n> > > \n> > > Especially, I am not sure if the issue only exists at the\n> > > end_packfile() boundary.  Don't we have the same issue reading\n> > > from the packfile being built, and isn't the only reason my hack\n> > > works it around is because access patterns of the testsuite\n> > > happens to not trigger it?\n> > \n> > Yes, that's my take on it as well (see my other email).  The\n> > testsuite must just be really lucky that its not hitting the\n> > boundary condition.\n> > \n> > I almost said gfi_unpack_entry() was immune from this bug, but\n> > I went back and read the code again and determined that it does\n> > in fact suffer from this under NO_MMAP, and we're just really\n> > damn lucky nobody has caused it.\n> \n> I think this solves the problem.  Its based on your first patch, but\n> would replace it.  The trick here is we close the cached windows if\n> we are accessing data from the packfile we are appending into and we\n> have increased the file length.  This way we don't blow away windows\n> during high read/low write periods, like during branch cache reloads.\n\nJunio pointed out my first attempt at this didn't update the\nmemory pressure values, so we could \"run out of memory\" even\nif we had plenty free.\n\nTry #2...\n \n--8>--\ndiff --git a/cache.h b/cache.h\nindex 24735bd..15388aa 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -561,6 +561,7 @@ extern struct packed_git *find_sha1_pack(const unsigned char *sha1,\n extern void pack_report(void);\n extern int open_pack_index(struct packed_git *);\n extern unsigned char* use_pack(struct packed_git *, struct pack_window **, off_t, unsigned int *);\n+extern void close_pack_windows(struct packed_git *, int);\n extern void unuse_pack(struct pack_window **);\n extern struct packed_git *add_packed_git(const char *, int, int);\n extern const unsigned char *nth_packed_object_sha1(struct packed_git *, uint32_t);\ndiff --git a/fast-import.c b/fast-import.c\nindex 3609c24..82c82ce 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -917,6 +917,7 @@ static void end_packfile(void)\n \t\tstruct branch *b;\n \t\tstruct tag *t;\n \n+\t\tclose_pack_windows(pack_data, 0);\n \t\tfixup_pack_header_footer(pack_data->pack_fd, pack_data->sha1,\n \t\t\t\t    pack_data->pack_name, object_count);\n \t\tclose(pack_data->pack_fd);\n@@ -926,7 +927,6 @@ static void end_packfile(void)\n \t\tnew_p = add_packed_git(idx_name, strlen(idx_name), 1);\n \t\tif (!new_p)\n \t\t\tdie(\"core git rejected index %s\", idx_name);\n-\t\tnew_p->windows = old_p->windows;\n \t\tall_packs[pack_id] = new_p;\n \t\tinstall_packed_git(new_p);\n \n@@ -1129,8 +1129,10 @@ static void *gfi_unpack_entry(\n {\n \tenum object_type type;\n \tstruct packed_git *p = all_packs[oe->pack_id];\n-\tif (p == pack_data)\n+\tif (p == pack_data && p->pack_size < (pack_size + 20)) {\n+\t\tclose_pack_windows(p, 0);\n \t\tp->pack_size = pack_size + 20;\n+\t}\n \treturn unpack_entry(p, oe->offset, &type, sizep);\n }\n \ndiff --git a/sha1_file.c b/sha1_file.c\nindex 6583797..50d1dea 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -611,6 +611,34 @@ void release_pack_memory(size_t need, int fd)\n \t\t; /* nothing */\n }\n \n+void close_pack_windows(struct packed_git *p, int retain_fd)\n+{\n+\tstruct pack_window **tail = NULL, *n = p->windows;\n+\twhile (n) {\n+\t\tstruct pack_window *w = p->windows;\n+\n+\t\tif (w->inuse_cnt) {\n+\t\t\t*tail = w;\n+\t\t\ttail = &w->next;\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tmunmap(w->base, w->len);\n+\t\tpack_mapped -= w->len;\n+\t\tpack_open_windows--;\n+\t\tn = w->next;\n+\t\tfree(w);\n+\t}\n+\n+\tp->windows = *tail;\n+\tif (p->windows)\n+\t\twarning(\"pack windows still in-use during close attempt\");\n+\telse if (!retain_fd && p->pack_fd != -1) {\n+\t\tclose(p->pack_fd);\n+\t\tp->pack_fd = -1;\n+\t}\n+}\n+\n void unuse_pack(struct pack_window **w_cursor)\n {\n \tstruct pack_window *w = *w_cursor;\n\n-- \nShawn.\n"},{"id":"65816","messageId":"20080118032222.GX24004@spearce.org","threadId":"11633","inReplyTo":"20080118031831.GW24004@spearce.org","subject":"Re: Be more careful about updating refs","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-01-18T03:22:22Z","receivedAt":"2008-01-18T03:22:22Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"\"Shawn O. Pearce\" <spearce@spearce.org> wrote:\n> \"Shawn O. Pearce\" <spearce@spearce.org> wrote:\n> > \"Shawn O. Pearce\" <spearce@spearce.org> wrote:\n> > > Junio C Hamano <gitster@pobox.com> wrote:\n> > > > \n> > > > Especially, I am not sure if the issue only exists at the\n> > > > end_packfile() boundary.  Don't we have the same issue reading\n> > > > from the packfile being built, and isn't the only reason my hack\n> > > > works it around is because access patterns of the testsuite\n> > > > happens to not trigger it?\n> > > \n> > > Yes, that's my take on it as well (see my other email).  The\n> > > testsuite must just be really lucky that its not hitting the\n> > > boundary condition.\n> > > \n> > > I almost said gfi_unpack_entry() was immune from this bug, but\n> > > I went back and read the code again and determined that it does\n> > > in fact suffer from this under NO_MMAP, and we're just really\n> > > damn lucky nobody has caused it.\n> > \n> > I think this solves the problem.  Its based on your first patch, but\n> > would replace it.  The trick here is we close the cached windows if\n> > we are accessing data from the packfile we are appending into and we\n> > have increased the file length.  This way we don't blow away windows\n> > during high read/low write periods, like during branch cache reloads.\n> \n> Junio pointed out my first attempt at this didn't update the\n> memory pressure values, so we could \"run out of memory\" even\n> if we had plenty free.\n> \n> Try #2...\n\nOK, that was crap.  Don't even try it.\n\nI'm holding off sending anything more until I get the test suite\nto actually run without telling me the bus crashed into the wall.\n\n-- \nShawn.\n"},{"id":"66432","messageId":"4797C551.8020909@vilain.net","threadId":"11633","inReplyTo":"alpine.LFD.1.00.0801151546560.2806@woody.linux-foundation.org","subject":"Re: Be more careful about updating refs","fromName":"Sam Vilain","fromEmail":"sam@vilain.net","sentAt":"2008-01-23T22:53:05Z","receivedAt":"2008-01-23T22:53:05Z","isPatch":false,"sender":{"key":"sam@vilain.net","avatar":"https://gravatar.com/avatar/8fc840ca854dbf6f7065b4335e3b934951c1dca3b11db688e95e471901f8f4a8?d=mp&s=160"},"body":"Linus Torvalds wrote:\n> Perhaps more importantly, it also refuses to update a branch head with a \n> non-commit object. I don't quite know *how* the stable series maintainers \n> were able to corrupt their repository to have a HEAD that pointed to a tag \n> rather than a commit object, but they did. Which results in a totally \n> broken repository that cannot be cloned or committed on.\n\nI actually used this for a prototype system to allow push over git://\nwith secure authentication via PGP; basically it used an update hook\nthat disallowed any ref change that wasn't a signed tag, and then in the\npost-update hook, moved the tag to an audit log refspace and put the\nreferant commit in the refs/heads/* location.\n\nThe hooks (probably racy etc) can be seen under\nhttp://git.utsl.gen.nz/Tangram/hooks\n\nSam.\n"}]}