{"thread":{"id":"5596","subject":"[PATCH] Remove branch by putting a null sha1 into the ref file.","startedAt":"2006-09-18T04:54:29Z","lastAt":"2006-09-27T07:25:45Z","messageCount":19,"participants":["Christian Couder","Junio C Hamano","Linus Torvalds","Daniel Barkalow","Andy Whitcroft"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"27106","messageId":"20060918065429.6f4de06e.chriscool@tuxfamily.org","threadId":"5596","inReplyTo":null,"subject":"[PATCH] Remove branch by putting a null sha1 into the ref file.","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2006-09-18T04:54:29Z","receivedAt":"2006-09-18T04:54:29Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"With the new packed ref file format from Linus, this should be\nthe new way to remove a branch.\n\n\"refs.c\" is fixed so that a null sha1 for a deleted branch does\nnot result in \"refs/head/deleted does not point to a valid\ncommit object!\" messages.\n\n\"t/t3200-branch.sh\" is fixed so that it uses git-show-ref\ninstead of checking that the ref does not exist when a branch\nis deleted.\n\nSigned-off-by: Christian Couder \n---\n git-branch.sh | 6 +++++-\n refs.c | 2 ++\n t/t3200-branch.sh | 2 +-\n 3 files changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/git-branch.sh b/git-branch.sh\nindex 2600e9c..cb55880 100755\n--- a/git-branch.sh\n+++ b/git-branch.sh\n@@ -10,6 +10,9 @@ SUBDIRECTORY_OK='Yes'\n \n headref=$(git-symbolic-ref HEAD | sed -e 's|^refs/heads/||')\n \n+# Fourty 0s.\n+NULL_SHA1=\"0000000000000000000000000000000000000000\" \n+\n delete_branch () {\n option=\"$1\"\n shift\n@@ -43,7 +46,8 @@ If you are sure you want to delete it, r\n \t ;;\n \tesac\n \trm -f \"$GIT_DIR/logs/refs/heads/$branch_name\"\n-\trm -f \"$GIT_DIR/refs/heads/$branch_name\"\n+\techo $NULL_SHA1 > \"$GIT_DIR/refs/heads/$branch_name\" || \\\n+\t die \"Failed to delete branch '$branch_name' !\"\n \techo \"Deleted branch $branch_name.\"\n done\n exit 0\ndiff --git a/refs.c b/refs.c\nindex 5e65314..76d8d0e 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -162,6 +162,8 @@ static int do_for_each_ref(const char *b\n \t\t\t\terror(\"%s points nowhere!\", path);\n \t\t\t\tcontinue;\n \t\t\t}\n+\t\t\tif (is_null_sha1(sha1))\n+\t\t\t\tcontinue; /* Ignore deleted refs. */\n \t\t\tif (!has_sha1_file(sha1)) {\n \t\t\t\terror(\"%s does not point to a valid \"\n \t\t\t\t \"commit object!\", path);\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 5b04efc..150dfdc 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -47,7 +47,7 @@ test_expect_success \\\n test_expect_success \\\n 'git branch -d d/e/f should delete a branch and a log' \\\n \t'git-branch -d d/e/f &&\n-\t test ! -f .git/refs/heads/d/e/f &&\n+\t ! git-show-ref --verify --quiet -- \"refs/heads/d/e/f\" &&\n \t test ! -f .git/logs/refs/heads/d/e/f'\n \n cat >expect < writes:\n\n> @@ -43,7 +46,8 @@ If you are sure you want to delete it, r\n> \t ;;\n> \tesac\n> \trm -f \"$GIT_DIR/logs/refs/heads/$branch_name\"\n> -\trm -f \"$GIT_DIR/refs/heads/$branch_name\"\n> +\techo $NULL_SHA1 > \"$GIT_DIR/refs/heads/$branch_name\" || \\\n> +\t die \"Failed to delete branch '$branch_name' !\"\n\nDon't you need mkdir -p somewhere?\n\n> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\n> index 5b04efc..150dfdc 100755\n> --- a/t/t3200-branch.sh\n> +++ b/t/t3200-branch.sh\n> @@ -47,7 +47,7 @@ test_expect_success \\\n> test_expect_success \\\n> 'git branch -d d/e/f should delete a branch and a log' \\\n> \t'git-branch -d d/e/f &&\n> -\t test ! -f .git/refs/heads/d/e/f &&\n> +\t ! git-show-ref --verify --quiet -- \"refs/heads/d/e/f\" &&\n> \t test ! -f .git/logs/refs/heads/d/e/f'\n\nI am old-fashioned and it makes me think twice when I see people\ndo \"! command\" in shell. Bash and dash has support for it, and\nopengroup has it in its base specification, so probably it is\nOk.\n\nAs usual, Solaris /bin/sh does not grok it ;-).\n"},{"id":"27133","messageId":"Pine.LNX.4.64.0609180926590.4388@g5.osdl.org","threadId":"5596","inReplyTo":"20060918065429.6f4de06e.chriscool@tuxfamily.org","subject":"Re: [PATCH] Remove branch by putting a null sha1 into the ref file.","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-09-18T16:31:21Z","receivedAt":"2006-09-18T16:31:21Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 18 Sep 2006, Christian Couder wrote:\n>\n> With the new packed ref file format from Linus, this should be\n> the new way to remove a branch.\n\nWell, it's not really sufficient.\n\nSomebody should add this test-case\n\n\tgit branch test\n\tgit branch -d test\n\tgit branch test/first\n\nwhich should work.\n\nIt's entirely possible that the proper way to do branch deletion with \npacked branches is to simply re-pack without the old branch, rather than \nthe negative branch model. I couldn't really decide.\n\nHowever: this part is definitely correct, considering that we allow the \nnull sha1 in other places.\n\n> \"refs.c\" is fixed so that a null sha1 for a deleted branch does\n> not result in \"refs/head/deleted does not point to a valid\n> commit object!\" messages.\n\nAnd this last part is conceptually ok, but I think the implementaion is \nwrong:\n\n> \"t/t3200-branch.sh\" is fixed so that it uses git-show-ref\n> instead of checking that the ref does not exist when a branch\n> is deleted.\n\nI think you should change the\n\n\t! git-show-ref --verify --quiet -- \"refs/heads/d/e/f\" &&\n\ninto\n\n\tgit-show-ref --verify --quiet -- \"refs/heads/d/e/f\" ||\n\ninstead, no?\n\n\t\tLinus\n"},{"id":"27141","messageId":"7vodtdow0v.fsf@assigned-by-dhcp.cox.net","threadId":"5596","inReplyTo":"Pine.LNX.4.64.0609180926590.4388@g5.osdl.org","subject":"Re: [PATCH] Remove branch by putting a null sha1 into the ref file.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-09-18T18:43:44Z","receivedAt":"2006-09-18T18:43:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> On Mon, 18 Sep 2006, Christian Couder wrote:\n>>\n>> With the new packed ref file format from Linus, this should be\n>> the new way to remove a branch.\n>\n> Well, it's not really sufficient.\n>\n> Somebody should add this test-case\n>\n> \tgit branch test\n> \tgit branch -d test\n> \tgit branch test/first\n>\n> which should work.\n\nAlso this test-case needs to be added\n\n\tgit branch test\n git pack-refs\n rm .git/refs/heads/test\n git branch test/second\n\nwhich should barf. Otherwise it would allow both test and\ntest/second branches to exist, and trying to clone from such a\nrepository would fail, at least by the existing tools (it might\nhave been even nicer if we from day one allowed both test and\ntest/second to exist, though, but it is too late now, or too\nearly before we upgrade everybody).\n\n> It's entirely possible that the proper way to do branch deletion with \n> packed branches is to simply re-pack without the old branch, rather than \n> the negative branch model. I couldn't really decide.\n\nOr mkdir there ,-).\n"},{"id":"27209","messageId":"200609200456.24714.chriscool@tuxfamily.org","threadId":"5596","inReplyTo":"Pine.LNX.4.64.0609180926590.4388@g5.osdl.org","subject":"Re: [PATCH] Remove branch by putting a null sha1 into the ref file.","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2006-09-20T02:56:24Z","receivedAt":"2006-09-20T02:56:24Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Linus Torvalds wrote:\n> On Mon, 18 Sep 2006, Christian Couder wrote:\n> > With the new packed ref file format from Linus, this should be\n> > the new way to remove a branch.\n>\n> Well, it's not really sufficient.\n>\n> Somebody should add this test-case\n>\n> \tgit branch test\n> \tgit branch -d test\n> \tgit branch test/first\n>\n> which should work.\n>\n> It's entirely possible that the proper way to do branch deletion with\n> packed branches is to simply re-pack without the old branch, rather than\n> the negative branch model. \n\nYes, and anyway git-branch should probably use some other git command for \nthis, like \"git-update-ref 0\" or \"git-update-ref --delete \".\n\n> I couldn't really decide.\n>\n> However: this part is definitely correct, considering that we allow the\n> null sha1 in other places.\n>\n> > \"refs.c\" is fixed so that a null sha1 for a deleted branch does\n> > not result in \"refs/head/deleted does not point to a valid\n> > commit object!\" messages.\n\nWell, it was in one of your patch already but I had not seen it. And I \nwrongly assumed that your patches were all merged into next but this one \nwas not.\n\n> And this last part is conceptually ok, but I think the implementaion is\n> wrong:\n> > \"t/t3200-branch.sh\" is fixed so that it uses git-show-ref\n> > instead of checking that the ref does not exist when a branch\n> > is deleted.\n>\n> I think you should change the\n>\n> \t! git-show-ref --verify --quiet -- \"refs/heads/d/e/f\" &&\n>\n> into\n>\n> \tgit-show-ref --verify --quiet -- \"refs/heads/d/e/f\" ||\n>\n> instead, no?\n\nIt seems Junio found it ok, and I still think it is ok, but my description \nof the change is bad.\n\nMaybe git-show-ref should have an option to check that a ref does not exist \nfor example \"git-show-ref --inexistant --quiet -- \"refs/heads/d/e/f\".\n\nThanks for your comments.\n\nChristian.\n"},{"id":"27418","messageId":"7v3baj365g.fsf@assigned-by-dhcp.cox.net","threadId":"5596","inReplyTo":"Pine.LNX.4.64.0609180926590.4388@g5.osdl.org","subject":"Re: [PATCH] Remove branch by putting a null sha1 into the ref file.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-09-22T22:09:31Z","receivedAt":"2006-09-22T22:09:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> It's entirely possible that the proper way to do branch deletion with \n> packed branches is to simply re-pack without the old branch, rather than \n> the negative branch model. I couldn't really decide.\n\nAfter playing with branch deletion issues for some time, I\nstarted to think it would be a lot simpler if we do not mark\na deleted branch with 0{40}.\n\nI started rewriting git-branch in C, mostly reusing Kristian\nHøgsberg's patch but taking care of the reflog issue still left\nin the original thread:\n\n Subject: [PATCH] branch as a builtin (again)\n Date: Sun, 20 Aug 2006 17:22:18 -0400\n Message-ID: <59ad55d30608201422h4a6d40f7y7782212637380438@mail.gmail.com>\n\nWe need to take care of two funny cases around packed-ref in\nlock_ref_sha1_basic() where a creation of a new ref takes\nplace. It needs to deal with two issues.\n\n * We should not allow frotz branch to be created when\n frotz/nitfol branch already exists (packed and pruned).\n Being able to create .git/refs/heads/frotz.lock file and\n being able to rename it to .git/refs/heads/frotz is not\n enough. The other way around has the same problem. So we\n would need to have a for_each_ref() to check if the ref being\n created is a prefix of an existing one or there is an\n existing one that is a prefix of the one being created.\n\n * When creating frotz branch, there may be a leftover\n .git/refs/heads/frotz/ directory on the filesystem. If the\n directory is empty (or contains only empty subdirectories),\n we can rmdir recursively and make refs/heads/frotz file\n there. Otherwise it means that a branch frotz/something\n still exists there, but the above prefix check should have\n taken care of it. So in practice we should always be able to\n just do an equivalent of \"rm -fr .git/refs/heads/frotz\" when\n creating a frotz branch.\n\nBut the latter falls apart if we use 0{40} convention to mark a\ndeleted branch. Removing .git/refs/heads/frotz/nitfol file which\nhas 0{40} and creating .git/refs/heads/frotz file resurrects\nfrotz/nitfol branch that is still packed. Not allowing frotz\nbranch to be created only because we had deleted frotz/nitfol\npreviously is not what we want either, so at that point we need\nto repack without frotz/nitfol anyway.\n\nWhich makes me think that we would better repack when removing\nany existing ref.\n\nBTW, the second issue exists without packed ref; currently we\ncannot do\n\n git branch foo/bar\n git branch -d foo/bar\n git branch foo\n"},{"id":"27447","messageId":"200609230645.37773.chriscool@tuxfamily.org","threadId":"5596","inReplyTo":"7v3baj365g.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Remove branch by putting a null sha1 into the ref file.","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2006-09-23T04:45:36Z","receivedAt":"2006-09-23T04:45:36Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Junio C Hamano wrote:\n> Linus Torvalds writes:\n> > It's entirely possible that the proper way to do branch deletion with\n> > packed branches is to simply re-pack without the old branch, rather\n> > than the negative branch model. I couldn't really decide.\n>\n> After playing with branch deletion issues for some time, I\n> started to think it would be a lot simpler if we do not mark\n> a deleted branch with 0{40}.\n\nI also came to the same conclusion and I wonder if we could delete a branch \nby moving the file \".git/refs/heads/frotz\" \nto \".git/deleted-refs/heads/frotz\".\nIf the branch is packed we could perhaps just create \n\".git/deleted-refs/heads/frotz\". \n\nIt means everytime we are looking up a ref we have to check \nin \".git/deleted-refs/\" first to see if it exists.\n\n> But the latter falls apart if we use 0{40} convention to mark a\n> deleted branch. Removing .git/refs/heads/frotz/nitfol file which\n> has 0{40} and creating .git/refs/heads/frotz file resurrects\n> frotz/nitfol branch that is still packed.\n\nIf we move \".git/refs/heads/frotz/nitfol\" \nto \".git/deleted-refs/heads/frotz/nitfol\" when we remove this ref, we only \nneed to try to rmdir all subdirectories under \".git/refs/heads/frotz/\" and \nthen \".git/refs/heads/frotz/\" to see if we can \ncreate \".git/refs/heads/frotz\", and if we can, we won't \nresurect \"frotz/nitfol\" because \".git/deleted-refs/heads/frotz/nitfol\" \nstill exists.\n\n> Not allowing frotz \n> branch to be created only because we had deleted frotz/nitfol\n> previously is not what we want either, so at that point we need\n> to repack without frotz/nitfol anyway.\n>\n> Which makes me think that we would better repack when removing\n> any existing ref.\n\nYes but it may not be fast and may be corruption prone.\n\nChristian.\n"},{"id":"27452","messageId":"7vu02zuhya.fsf@assigned-by-dhcp.cox.net","threadId":"5596","inReplyTo":"200609230645.37773.chriscool@tuxfamily.org","subject":"Re: [PATCH] Remove branch by putting a null sha1 into the ref file.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-09-23T08:04:45Z","receivedAt":"2006-09-23T08:04:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> If we move \".git/refs/heads/frotz/nitfol\" \n> to \".git/deleted-refs/heads/frotz/nitfol\" when we remove this ref, we only \n> need to try to rmdir all subdirectories under \".git/refs/heads/frotz/\" and \n> then \".git/refs/heads/frotz/\" to see if we can \n> create \".git/refs/heads/frotz\", and if we can, we won't \n> resurect \"frotz/nitfol\" because \".git/deleted-refs/heads/frotz/nitfol\" \n> still exists.\n\nI am not sure if that would be workable. I suspect that you\nwould need to do quite an involved sequence in \"git branch\" to\nmake this sequence to work with .git/deleted-refs/ scheme:\n\n\tgit branch frotz/nitfol\n git pack-refs --prune\n git branch -d frotz/nitfol\n git branch frotz\n git pack-refs\n git branch -d frotz\n\nAfter deleting frotz/nitfol you would create frotz/nitfol in\ndeleted hierarchy. Then when you delete frotz you would need to\ncreate frotz in deleted hierarchy, but you cannot, without\nlosing frotz/nitfol.\n"},{"id":"27465","messageId":"200609231322.30214.chriscool@tuxfamily.org","threadId":"5596","inReplyTo":"7vu02zuhya.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Remove branch by putting a null sha1 into the ref file.","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2006-09-23T11:22:30Z","receivedAt":"2006-09-23T11:22:30Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Junio C Hamano wrote:\n> Christian Couder writes:\n> > If we move \".git/refs/heads/frotz/nitfol\"\n> > to \".git/deleted-refs/heads/frotz/nitfol\" when we remove this ref, we\n> > only need to try to rmdir all subdirectories under\n> > \".git/refs/heads/frotz/\" and then \".git/refs/heads/frotz/\" to see if we\n> > can\n> > create \".git/refs/heads/frotz\", and if we can, we won't\n> > resurect \"frotz/nitfol\" because \".git/deleted-refs/heads/frotz/nitfol\"\n> > still exists.\n>\n> I am not sure if that would be workable. I suspect that you\n> would need to do quite an involved sequence in \"git branch\" to\n> make this sequence to work with .git/deleted-refs/ scheme:\n>\n> \tgit branch frotz/nitfol\n> git pack-refs --prune\n> git branch -d frotz/nitfol\n> git branch frotz\n> git pack-refs\n> git branch -d frotz\n>\n> After deleting frotz/nitfol you would create frotz/nitfol in\n> deleted hierarchy. Then when you delete frotz you would need to\n> create frotz in deleted hierarchy, but you cannot, without\n> losing frotz/nitfol.\n\nYou are right, so what about moving \".git/refs/heads/frotz\" \nto \".git/deleted-refs/heads/frotz.ref\" \nor \".git/deleted-refs/heads/frotz~ref\" (because \"~\" is forbidden in ref \nnames).\n\nChristian. \n"},{"id":"27516","messageId":"7veju2nthl.fsf@assigned-by-dhcp.cox.net","threadId":"5596","inReplyTo":"200609231322.30214.chriscool@tuxfamily.org","subject":"Re: [PATCH] Remove branch by putting a null sha1 into the ref file.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-09-23T21:49:42Z","receivedAt":"2006-09-23T21:49:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> You are right, so what about moving \".git/refs/heads/frotz\" \n> to \".git/deleted-refs/heads/frotz.ref\" \n> or \".git/deleted-refs/heads/frotz~ref\" (because \"~\" is forbidden in ref \n> names).\n\nBut wouldn't it bring up the issue of locking among deleters,\nupdaters/creators, and traversers?\n\nIf we choose to use packed-refs.lock as the \"set of all refs\"\nlock, the whole sequence would become something like this. Note\nthat this tries to make readers lockless but I am sure there are\nnasty race condition issues. I am not sure what we would want\nto do about them.\n\n= Looking up a ref $frotz.\n\n - check if .git/$frotz exists, and use it if it does.\n\n - check if .git/deleted-refs/$frotz~ref exists, and return \"no\n such ref\" if it does.\n\n - find $frotz in .git/packed-refs.\n\n= Looping over refs.\n\n - grab all .git/refs/ and subtract all .git/deleted-refs/\n\n - walk .git/packed-refs and the result from the above in\n parallel as in the current code.\n\n= Storing a new value in ref $frotz.\n\n - acquire .git/packed-refs.lock\n\n - lock .git/$frotz.lock.\n\n - write into .git/$frotz.lock.\n\n - create or update .git/logs/$frotz as needed.\n\n - if .git/deleted-refs/$frotz~ref exists, unlink it.\n\n - rename .git/$frotz.lock to .git/$frotz to unlock it.\n\n - unlink .git/packed-refs.lock\n\n= Deleting a ref $frotz.\n\n - acquire .git/packed-refs.lock\n\n - look up $frotz; if it does not exist either barf or return\n silent (haven't thought it through yet).\n\n - create .git/deleted-refs/$frotz~ref.\n\n - remove .git/logs/$frotz\n\n - unlink .git/packed-refs.lock\n\n= Packing refs, with optional pruning.\n\n - lock .git/packed-refs.lock\n\n - loop over refs:\n - write it out to .git/packed-refs.lock unless a symref.\n - if it is a loose one (not a symref), remember it for pruning.\n\n - if pruning:\n - remove the entire .git/deleted-refs/ hierarchy\n - remove the remembered ones\n\n - rename .git/packed-refs.lock to .git/packed-refs\n"},{"id":"27531","messageId":"200609240645.54467.chriscool@tuxfamily.org","threadId":"5596","inReplyTo":"7veju2nthl.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Remove branch by putting a null sha1 into the ref file.","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2006-09-24T04:45:53Z","receivedAt":"2006-09-24T04:45:53Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Junio C Hamano wrote:\n> Christian Couder writes:\n> > You are right, so what about moving \".git/refs/heads/frotz\"\n> > to \".git/deleted-refs/heads/frotz.ref\"\n> > or \".git/deleted-refs/heads/frotz~ref\" (because \"~\" is forbidden in ref\n> > names).\n>\n> But wouldn't it bring up the issue of locking among deleters,\n> updaters/creators, and traversers?\n\nYes, it will get worse. \n\n> If we choose to use packed-refs.lock as the \"set of all refs\"\n> lock, the whole sequence would become something like this. Note\n> that this tries to make readers lockless but I am sure there are\n> nasty race condition issues. I am not sure what we would want\n> to do about them.\n>\n> = Looking up a ref $frotz.\n\nWe could acquire .git/$frotz.lock here if we want to be sure that nothing \nwill happen to it while we read it, but anyway something could happen to it \njust after we read it and before we use it. So the right thing to do is \nperhaps to let the caller acquire the lock if it needs to. We should just \ncheck in the other cases below that nothing can happen to the ref if \nsomeone acquired .git/$frotz.lock.\n\n> - check if .git/$frotz exists, and use it if it does.\n>\n> - check if .git/deleted-refs/$frotz~ref exists, and return \"no\n> such ref\" if it does.\n>\n> - find $frotz in .git/packed-refs.\n>\n> = Looping over refs.\n\nSame as above. Depending on what the caller wants to do, it could \nacquire .git/packed-refs.lock or some .git/$frotz.lock to make sure nothing \nhappens to all or only some refs.\n\n> - grab all .git/refs/ and subtract all .git/deleted-refs/\n>\n> - walk .git/packed-refs and the result from the above in\n> parallel as in the current code.\n>\n> = Storing a new value in ref $frotz.\n>\n> - acquire .git/packed-refs.lock\n>\n> - lock .git/$frotz.lock.\n>\n> - write into .git/$frotz.lock.\n>\n> - create or update .git/logs/$frotz as needed.\n>\n> - if .git/deleted-refs/$frotz~ref exists, unlink it.\n>\n> - rename .git/$frotz.lock to .git/$frotz to unlock it.\n>\n> - unlink .git/packed-refs.lock\n>\n> = Deleting a ref $frotz.\n>\n> - acquire .git/packed-refs.lock\n\n- acquire .git/$frotz.lock seems needed too\n\n> - look up $frotz; if it does not exist either barf or return\n> silent (haven't thought it through yet).\n>\n> - create .git/deleted-refs/$frotz~ref\n\nor move .git/$frotz to .git/deleted-refs/$frotz~ref if .git/$frotz exists\n\n> - remove .git/logs/$frotz\n\n- unlink .git/$frotz.lock \n\n> - unlink .git/packed-refs.lock\n>\n> = Packing refs, with optional pruning.\n>\n> - lock .git/packed-refs.lock\n>\n> - loop over refs:\n> - write it out to .git/packed-refs.lock unless a symref.\n> - if it is a loose one (not a symref), remember it for pruning.\n>\n> - if pruning:\n> - remove the entire .git/deleted-refs/ hierarchy\n> - remove the remembered ones\n\nPerhaps we should acquire .git/$frotz.lock for each $frotz that we remove \nwhen pruning.\n\n> - rename .git/packed-refs.lock to .git/packed-refs\n\nThanks,\nChristian.\n"},{"id":"27613","messageId":"7vmz8o1em0.fsf_-_@assigned-by-dhcp.cox.net","threadId":"5596","inReplyTo":"200609240645.54467.chriscool@tuxfamily.org","subject":"On ref locking","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-09-25T09:26:31Z","receivedAt":"2006-09-25T09:26:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The comments you added to the strawman I sent suggested use of\nrather heavyweight locks, which made me feel we were somehow\ngoing in a wrong direction. Before going into the details of\nbranch removing, let's first see if we can summarize what kind\nof guarantee we would want from ref updates. The current\nlocking scheme is very carefully and nicely done by Linus and\nDaniel Barkalow around June last year, and I do not want to lose\ngood property of it.\n\n - When reading and/or listing refs you do not need to acquire\n any lock.\n\n - When you are going to update an existing $ref, you create\n $ref.lock, and do a compare-and-swap.\n\nWhat the latter means is that an updater:\n\n (1) first learns the current value of the $ref, without\n locking;\n\n (2) decides based on the knowledge from (1) what the next value\n should be;\n\n (3) gets $ref.lock, makes sure $ref still is the value it\n learned in (1), updates it to the desired value and\n releases the lock.\n\nThe above 3-step sequence prevents updater-updater races with an\nextremely short critical section. We only need to hold the lock\nwhile we do compare and swap.\n\nThe mandatory \"fast-forward\" check by receive-pack introduced by\nJohannes (see receive-pack.c::update()) does exactly the above.\nThe program reads the current value of refs involved very early,\nand verifies fast-forward-ness of the update here. After doing\nthat, it gets the lock, makes sure the refs are still the same\nas we read earlier (meaning, nobody else updated the ref while\nwe were doing other things, thereby invalidating the checks\nwe've done earlier) and then finally unlocks it.\n\nSide note: we might want to move the call to run_update_hook()\nbefore creating the lock for the same reason as we do the new\nfast-forwards test before acquiring the lock.\n\nI am not sure if the current code in receive-pack is doing the\nright thing to prevent creator-creator race, though. While\ncreating a new ref, verify_old_ref() is called with 0{40} as the\nold value (meaning, \"we are creating this ref and setting it to\nthis value, based on our previous knowledge that it did not\nexist\"). It happily returns without checking anything in this\ncase, but I think it should make sure nobody created the ref\nwhile we were looking the other way. With the current code, we\nwould allow overwriting somebody else's push. And the race\nwindow is not so small -- the check that the ref is a new one we\nare about to create is performed during the initial protocol\nhandshake, and then we can spend quite some time unpacking the\npack stream until we get to verify_old_ref().\n\nThe same 3-step sequence is done by refs.c::lock_ref_sha1() and\nlock_any_ref_for_update() API and Porcelains are expected to use\ngit-update-ref. You call them with the current value of the ref\nas you learned earlier, and you would get a lock. If you get a\nlock successfully, you write the new value out.\n\nSide note: I think the same issue of creator-creator race exists\nin verify_lock(), by the way. I think we can fix this by\naccepting 0{40} SHA1 to git-update-ref to mean \"I am creating\nthis based on the assumption that it does not exist yet -- that\nis my understanding from my earlier check\".\n\nWe are very relaxed about deleting refs right now, compared to\nthe updates described above. \"git branch -d\" barfs if the ref\nit wanted to delete is not an ancestor of the current branch,\nbut it does not have any lock between the time it checks and the\ntime it actually deletes it.\n\nThere are very loosely written ref updates in Porcelains. \"git\ntag\" and \"git branch\" barf if the ref they wanted to create\nexists, but both have rather large race window without the\nprotection of any lock. Especially \"git tag\" race window is\nlarge -- it lets you open an editor to type the tag message and\nruns gpg to sign the message after checking if you are not\noverwriting an existing tag X-<. These should be fixed, and I\nthink we can do so by the above fix to git-update-ref I\nmentioned in the above side note.\n\nWe might need to update the ref locking to adjust to packed\nrefs, and getting locks around ref deletion right becomes more\nimportant if we want to do the funky .git/deleted-refs/$ref~ref\nbusiness. But we should try to stick to the same 3-step\nsequence. What this means is that a deleter:\n\n (1) first learns the current value of the $ref, without\n locking;\n\n (2) decides based on the knowledge from (1) it indeed wants to\n delete it;\n\n (3) gets the $ref.lock, makes sure $ref still has the value it\n learned in (1), and deletes it. As a special case, if $ref\n no longer exists, that does not have to be an error.\n Somebody else deleted it while we were looking the other\n way, but a delete is a delete is a delete, and we are\n simply happy that the ref is gone.\n\nThis should protect us from deleter-updater race. $ref.lock\nwould protect us from deleter-creator race. So I do not think\nwe would need to take .git/packed-refs.lock while deleting a\nref. Even if you implement \"(3) ... deletes it\" with the\nproposed .git/deleted-refs/$ref~ref file.\n\nI think the way Linus did git-pack-refs protects us from\npacker-packer race, and packer-updater, packer-creator, and\npacker-deleter race does not exist, because pack-refs:\n\n (0) takes .git/packed-refs.lock\n\n (1) learns the current value of all refs without any further locking;\n\n (2) writes out the current values;\n\n (3) releases .git/packed-refs.lock.\n\nIf somebody else creates or updates a ref while the above is\nrunning, the new result from loose refs will be used by later\nuser, making the entry in packed-refs obsolete. So there is no\npacker-updater or packer-creator race here. If we do the\nproposed .git/deleted-refs/$ref~ref to mark deletion, that would\nalso override whatever is in the resulting packed-refs, so we do\nnot have to worry about packer-deleter race either.\n\nI am reasonably sure there is no pruner-updater and pruner-creator\nraces. \"pack-refs --prune\":\n\n (1) learns the current value of all refs without any locking,\n as part of the packing operation during the same run.\n\n For each ref:\n\n (2) takes $ref.lock, makes sure $ref still has the value it\n learned from (1), and deletes .git/$ref (because the same\n value is recorded in .git/packed-refs), and then unlocks\n $ref.lock\n\nI haven't thought through packer-pruner race (pruner needs to\nunlock .git/packed-refs.lock first -- otherwise if it dies in\nthe middle of pruning we would lose refs that are stashed in the\n.git/packed-refs.lock file, that hasn't made to its final\nlocation), but I think we do not have any problem.\n"},{"id":"27630","messageId":"Pine.LNX.4.64.0609251140550.9789@iabervon.org","threadId":"5596","inReplyTo":"7vmz8o1em0.fsf_-_@assigned-by-dhcp.cox.net","subject":"Re: On ref locking","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2006-09-25T17:05:42Z","receivedAt":"2006-09-25T17:05:42Z","isPatch":false,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Mon, 25 Sep 2006, Junio C Hamano wrote:\n\n> The comments you added to the strawman I sent suggested use of\n> rather heavyweight locks, which made me feel we were somehow\n> going in a wrong direction. Before going into the details of\n> branch removing, let's first see if we can summarize what kind\n> of guarantee we would want from ref updates. The current\n> locking scheme is very carefully and nicely done by Linus and\n> Daniel Barkalow around June last year, and I do not want to lose\n> good property of it.\n> \n> - When reading and/or listing refs you do not need to acquire\n> any lock.\n> \n> - When you are going to update an existing $ref, you create\n> $ref.lock, and do a compare-and-swap.\n> \n> What the latter means is that an updater:\n> \n> (1) first learns the current value of the $ref, without\n> locking;\n> \n> (2) decides based on the knowledge from (1) what the next value\n> should be;\n> \n> (3) gets $ref.lock, makes sure $ref still is the value it\n> learned in (1), updates it to the desired value and\n> releases the lock.\n> \n> The above 3-step sequence prevents updater-updater races with an\n> extremely short critical section. We only need to hold the lock\n> while we do compare and swap.\n\nI remember having a certain amount of disagreement over whether it's \nbetter to actually take the lock in (1), and hold it through (2), or only \nverify that it didn't change in (3) after taking the lock for real. If \nthere's nothing substantial going on in (2), it doesn't matter. If users \nare producing content (e.g., git tag), they may want to make sure that \nnobody else is in the middle of writing something that would conflict.\n\nI think I'd advocated getting the lock early if you're going to want it, \nand I don't remember what the convincing argument on the other side was \nfor the cases under consideration at the time, beyond it not mattering \nsignificantly.\n\nNote that we make sure to remove the lock when aborting due to signals \n(assuming we get a chance), so the size of the critical section doesn't \nmatter too much to the chance of it getting left locked inappropriately. \nOf course, this is harder to do with the core code for a shell script than \nfor C code.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"27647","messageId":"7virjbw9lc.fsf@assigned-by-dhcp.cox.net","threadId":"5596","inReplyTo":"Pine.LNX.4.64.0609251140550.9789@iabervon.org","subject":"Re: On ref locking","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-09-26T04:11:27Z","receivedAt":"2006-09-26T04:11:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Barkalow writes:\n\n> On Mon, 25 Sep 2006, Junio C Hamano wrote:\n>...\n>> What the latter means is that an updater:\n>> \n>> (1) first learns the current value of the $ref, without\n>> locking;\n>> \n>> (2) decides based on the knowledge from (1) what the next value\n>> should be;\n>> \n>> (3) gets $ref.lock, makes sure $ref still is the value it\n>> learned in (1), updates it to the desired value and\n>> releases the lock.\n>> \n>> The above 3-step sequence prevents updater-updater races with an\n>> extremely short critical section. We only need to hold the lock\n>> while we do compare and swap.\n>\n> I remember having a certain amount of disagreement over whether it's \n> better to actually take the lock in (1), and hold it through (2), or only \n> verify that it didn't change in (3) after taking the lock for real. If \n> there's nothing substantial going on in (2), it doesn't matter. If users \n> are producing content (e.g., git tag), they may want to make sure that \n> nobody else is in the middle of writing something that would conflict.\n>\n> I think I'd advocated getting the lock early if you're going to want it, \n> and I don't remember what the convincing argument on the other side was \n> for the cases under consideration at the time, beyond it not mattering \n> significantly.\n>\n> Note that we make sure to remove the lock when aborting due to signals \n> (assuming we get a chance), so the size of the critical section doesn't \n> matter too much to the chance of it getting left locked inappropriately. \n> Of course, this is harder to do with the core code for a shell script than \n> for C code.\n\nI do not recall that arguments on the list but what you say\nmakes sense. Taking a lock early makes implementation more\ncumbersome on the scripting side but reduces the annoyance\nfactor. You will be annoyed if the tool tells you somebody else\ndid something to the ref you were going to update and your\nintended operation is blocked after carefully editing your tag\nmessage or commit log message.\n\nHowever, I think the distributed nature of git makes it less of\nan issue for normal repositories.\n\nShared distribution point repositories (where everybody pushes\nto and fetches from) needs to protect between updaters lest that\nwe lose commits (fetching a rev older while a push is in\nprogress is not an error -- re-feching would give you the final\nresult), and that is what Johannes's recent patch does. Pushing\ninto a repository with working tree does have a lot of issues\n(e.g. push to update the current branch head while a commit is\nin progress) but doing that as the workflow has problems for\nother reasons as well (i.e. push to update the current branch\nwould make the index out of sync even when no commit is in\nprogress). It is strongly discouraged to share $GIT_DIR/refs,\njust like in any halfway sane development workflow people do not\nshare working tree files.\n\nSo \"locking before letting the user spend effort so that we\nwould not later have to say 'sorry, we cannot let you do that\nbecause somebody else updated the ref in the meanwhile'\" is\ntechnically not incorrect, and we should be able to make things\nwork that way, but I think it pretty much is a \"why bother\"\nissue. \"git tag\" checks if the new tag being created does not\nexist to reduce the annoyance factor, and the check does not\neven attempt to avoid a race. If we revalidated that the tag is\nstill not there while storing the tag object name to the ref in\nthe final step, I think that is good enough, and I think having\n\"lock during operation\" buys us much real-life benefit.\n\ngit-commit has the same attitude, and I think it is the right\nbalance. It checks if the commit being created is an initial\ncommit, a single-parent commit or a merge first, prepares \"-p\"\narguments to the commit-tree that will happen much later, then\nlets the user prepare the log message. Theoretically somebody\nelse could update the current HEAD in the meantime and the\nresulting commit can have ancestors that are different from what\nwas intended. We do check and avoid it by making sure that the\nHEAD is still where we thought it would be before doing the\nfinal.\n"},{"id":"27652","messageId":"7vlko7uuxp.fsf_-_@assigned-by-dhcp.cox.net","threadId":"5596","inReplyTo":"7vmz8o1em0.fsf_-_@assigned-by-dhcp.cox.net","subject":"[PATCH 1/3] Clean-up lock-ref implementation","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-09-26T04:13:22Z","receivedAt":"2006-09-26T04:13:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This drops \"mustexist\" parameter lock_ref_sha1() and lock_any_ref_forupdate()\nfunctions take. Existing callers were always passing 0.\n\nSigned-off-by: Junio C Hamano \n---\n\n * A bit of clean-up before the real changes...\n\n builtin-update-ref.c | 2 +-\n fetch.c | 2 +-\n refs.c | 20 +++++++++-----------\n refs.h | 4 ++--\n 4 files changed, 13 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin-update-ref.c b/builtin-update-ref.c\nindex 90a3da5..ab52833 100644\n--- a/builtin-update-ref.c\n+++ b/builtin-update-ref.c\n@@ -48,7 +48,7 @@ int cmd_update_ref(int argc, const char \n \tif (oldval && get_sha1(oldval, oldsha1))\n \t\tdie(\"%s: not a valid old SHA1\", oldval);\n \n-\tlock = lock_any_ref_for_update(refname, oldval ? oldsha1 : NULL, 0);\n+\tlock = lock_any_ref_for_update(refname, oldval ? oldsha1 : NULL);\n \tif (!lock)\n \t\treturn 1;\n \tif (write_ref_sha1(lock, sha1, msg) < 0)\ndiff --git a/fetch.c b/fetch.c\nindex 34df8d3..74585c4 100644\n--- a/fetch.c\n+++ b/fetch.c\n@@ -266,7 +266,7 @@ int pull(int targets, char **target, con\n \t\tif (!write_ref || !write_ref[i])\n \t\t\tcontinue;\n \n-\t\tlock[i] = lock_ref_sha1(write_ref[i], NULL, 0);\n+\t\tlock[i] = lock_ref_sha1(write_ref[i], NULL);\n \t\tif (!lock[i]) {\n \t\t\terror(\"Can't lock ref %s\", write_ref[i]);\n \t\t\tgoto unlock_and_fail;\ndiff --git a/refs.c b/refs.c\nindex 5e65314..b4f6d4f 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -264,12 +264,11 @@ int check_ref_format(const char *ref)\n \t}\n }\n \n-static struct ref_lock *verify_lock(struct ref_lock *lock,\n-\tconst unsigned char *old_sha1, int mustexist)\n+static struct ref_lock *verify_lock(struct ref_lock *lock, const unsigned char *old_sha1)\n {\n \tchar buf[40];\n \tint nr, fd = open(lock->ref_file, O_RDONLY);\n-\tif (fd < 0 && (mustexist || errno != ENOENT)) {\n+\tif (fd < 0 && errno != ENOENT) {\n \t\terror(\"Can't verify ref %s\", lock->ref_file);\n \t\tunlock_ref(lock);\n \t\treturn NULL;\n@@ -292,11 +291,12 @@ static struct ref_lock *verify_lock(stru\n \n static struct ref_lock *lock_ref_sha1_basic(const char *path,\n \tint plen,\n-\tconst unsigned char *old_sha1, int mustexist)\n+\tconst unsigned char *old_sha1)\n {\n \tconst char *orig_path = path;\n \tstruct ref_lock *lock;\n \tstruct stat st;\n+\tint mustexist = 0;\n \n \tlock = xcalloc(1, sizeof(struct ref_lock));\n \tlock->lock_fd = -1;\n@@ -321,23 +321,21 @@ static struct ref_lock *lock_ref_sha1_ba\n \t\tdie(\"unable to create directory for %s\", lock->ref_file);\n \tlock->lock_fd = hold_lock_file_for_update(lock->lk, lock->ref_file, 1);\n \n-\treturn old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;\n+\treturn old_sha1 ? verify_lock(lock, old_sha1) : lock;\n }\n \n-struct ref_lock *lock_ref_sha1(const char *ref,\n-\tconst unsigned char *old_sha1, int mustexist)\n+struct ref_lock *lock_ref_sha1(const char *ref, const unsigned char *old_sha1)\n {\n \tif (check_ref_format(ref))\n \t\treturn NULL;\n \treturn lock_ref_sha1_basic(git_path(\"refs/%s\", ref),\n-\t\t5 + strlen(ref), old_sha1, mustexist);\n+\t\t5 + strlen(ref), old_sha1);\n }\n \n-struct ref_lock *lock_any_ref_for_update(const char *ref,\n-\tconst unsigned char *old_sha1, int mustexist)\n+struct ref_lock *lock_any_ref_for_update(const char *ref, const unsigned char *old_sha1)\n {\n \treturn lock_ref_sha1_basic(git_path(\"%s\", ref),\n-\t\tstrlen(ref), old_sha1, mustexist);\n+\t\tstrlen(ref), old_sha1);\n }\n \n void unlock_ref(struct ref_lock *lock)\ndiff --git a/refs.h b/refs.h\nindex 553155c..d3bc295 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -24,10 +24,10 @@ extern int for_each_remote_ref(int (*fn)\n extern int get_ref_sha1(const char *ref, unsigned char *sha1);\n \n /** Locks a \"refs/\" ref returning the lock on success and NULL on failure. **/\n-extern struct ref_lock *lock_ref_sha1(const char *ref, const unsigned char *old_sha1, int mustexist);\n+extern struct ref_lock *lock_ref_sha1(const char *ref, const unsigned char *old_sha1);\n \n /** Locks any ref (for 'HEAD' type refs). */\n-extern struct ref_lock *lock_any_ref_for_update(const char *ref, const unsigned char *old_sha1, int mustexist);\n+extern struct ref_lock *lock_any_ref_for_update(const char *ref, const unsigned char *old_sha1);\n \n /** Release any lock taken but not written. **/\n extern void unlock_ref(struct ref_lock *lock);\n-- \n1.4.2.1.g7a39b\n"},{"id":"27653","messageId":"7vfyefuuxb.fsf_-_@assigned-by-dhcp.cox.net","threadId":"5596","inReplyTo":"7vmz8o1em0.fsf_-_@assigned-by-dhcp.cox.net","subject":"[PATCH 2/3] update-ref: -d flag and ref creation safety.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-09-26T04:13:36Z","receivedAt":"2006-09-26T04:13:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This adds -d flag to update-ref to allow safe deletion of ref.\nBefore deleting it, the command checks if the given \nstill matches the value the caller thought the ref contained.\n\nSimilarly, it also accepts 0{40} as to allow safe\ncreation of a new ref.\n\nSigned-off-by: Junio C Hamano \n---\n Documentation/git-update-ref.txt | 9 ++++++--\n builtin-update-ref.c | 16 +++++++++++++--\n cache.h | 1 +\n refs.c | 41 +++++++++++++++++++++++++++++++++++++-\n 4 files changed, 62 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt\nindex e062030..723906d 100644\n--- a/Documentation/git-update-ref.txt\n+++ b/Documentation/git-update-ref.txt\n@@ -7,7 +7,7 @@ git-update-ref - update the object name \n \n SYNOPSIS\n --------\n-'git-update-ref' [-m ] []\n+'git-update-ref' [-m ] (-d | [])\n \n DESCRIPTION\n -----------\n@@ -20,7 +20,8 @@ possibly dereferencing the symbolic refs\n the current value of the matches .\n E.g. `git-update-ref refs/heads/master `\n updates the master branch head to only if its current\n-value is .\n+value is . You can specify 40 \"0\" as to\n+make sure that the ref you are creating does not exist.\n \n It also allows a \"ref\" file to be a symbolic pointer to another\n ref file by starting with the four-byte header sequence of\n@@ -49,6 +50,10 @@ for reading but not for writing (so we'l\n ref symlink to some other tree, if you have copied a whole\n archive by creating a symlink tree).\n \n+With `-d` flag, it deletes the named after verifying it\n+still contains .\n+\n+\n Logging Updates\n ---------------\n If config parameter \"core.logAllRefUpdates\" is true or the file\ndiff --git a/builtin-update-ref.c b/builtin-update-ref.c\nindex ab52833..486b8f2 100644\n--- a/builtin-update-ref.c\n+++ b/builtin-update-ref.c\n@@ -3,15 +3,16 @@ #include \"refs.h\"\n #include \"builtin.h\"\n \n static const char git_update_ref_usage[] =\n-\"git-update-ref [] [-m ]\";\n+\"git-update-ref [-m ] (-d | [])\";\n \n int cmd_update_ref(int argc, const char **argv, const char *prefix)\n {\n \tconst char *refname=NULL, *value=NULL, *oldval=NULL, *msg=NULL;\n \tstruct ref_lock *lock;\n \tunsigned char sha1[20], oldsha1[20];\n-\tint i;\n+\tint i, delete;\n \n+\tdelete = 0;\n \tsetup_ident();\n \tgit_config(git_default_config);\n \n@@ -26,6 +27,10 @@ int cmd_update_ref(int argc, const char \n \t\t\t\tdie(\"Refusing to perform update with \\\\n in message.\");\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!strcmp(\"-d\", argv[i])) {\n+\t\t\tdelete = 1;\n+\t\t\tcontinue;\n+\t\t}\n \t\tif (!refname) {\n \t\t\trefname = argv[i];\n \t\t\tcontinue;\n@@ -44,6 +49,13 @@ int cmd_update_ref(int argc, const char \n \n \tif (get_sha1(value, sha1))\n \t\tdie(\"%s: not a valid SHA1\", value);\n+\n+\tif (delete) {\n+\t\tif (oldval)\n+\t\t\tusage(git_update_ref_usage);\n+\t\treturn delete_ref(refname, sha1);\n+\t}\n+\n \thashclr(oldsha1);\n \tif (oldval && get_sha1(oldval, oldsha1))\n \t\tdie(\"%s: not a valid old SHA1\", oldval);\ndiff --git a/cache.h b/cache.h\nindex 97debd0..851f4c0 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -179,6 +179,7 @@ struct lock_file {\n extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);\n extern int commit_lock_file(struct lock_file *);\n extern void rollback_lock_file(struct lock_file *);\n+extern int delete_ref(const char *, unsigned char *sha1);\n \n /* Environment bits from configuration mechanism */\n extern int use_legacy_headers;\ndiff --git a/refs.c b/refs.c\nindex b4f6d4f..4d499b0 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -212,6 +212,32 @@ int get_ref_sha1(const char *ref, unsign\n \treturn read_ref(git_path(\"refs/%s\", ref), sha1);\n }\n \n+int delete_ref(const char *refname, unsigned char *sha1)\n+{\n+\tstruct ref_lock *lock;\n+\tint err, i, ret = 0;\n+\n+\tlock = lock_any_ref_for_update(refname, sha1);\n+\tif (!lock)\n+\t\treturn 1;\n+\ti = strlen(lock->lk->filename) - 5; /* .lock */\n+\tlock->lk->filename[i] = 0;\n+\terr = unlink(lock->lk->filename);\n+\tif (err) {\n+\t\tret = 1;\n+\t\terror(\"unlink(%s) failed: %s\",\n+\t\t lock->lk->filename, strerror(errno));\n+\t}\n+\tlock->lk->filename[i] = '.';\n+\n+\terr = unlink(lock->log_file);\n+\tif (err && errno != ENOENT)\n+\t\tfprintf(stderr, \"warning: unlink(%s) failed: %s\",\n+\t\t\tlock->log_file, strerror(errno));\n+\n+\treturn ret;\n+}\n+\n /*\n * Make sure \"ref\" is something reasonable to have under \".git/refs/\";\n * We do not like it if:\n@@ -268,6 +294,19 @@ static struct ref_lock *verify_lock(stru\n {\n \tchar buf[40];\n \tint nr, fd = open(lock->ref_file, O_RDONLY);\n+\n+\tif (is_null_sha1(old_sha1)) {\n+\t\t/* we wanted to make sure it did not exist */\n+\t\tif (fd < 0 && errno == ENOENT)\n+\t\t\treturn lock;\n+\t\tif (0 <= fd) {\n+\t\t\tclose(fd);\n+\t\t\terror(\"ref %s should be absent but it exists\",\n+\t\t\t lock->ref_file);\n+\t\t\tunlock_ref(lock);\n+\t\t\treturn NULL;\n+\t\t}\n+\t}\n \tif (fd < 0 && errno != ENOENT) {\n \t\terror(\"Can't verify ref %s\", lock->ref_file);\n \t\tunlock_ref(lock);\n@@ -296,7 +335,7 @@ static struct ref_lock *lock_ref_sha1_ba\n \tconst char *orig_path = path;\n \tstruct ref_lock *lock;\n \tstruct stat st;\n-\tint mustexist = 0;\n+\tint mustexist = old_sha1 && !is_null_sha1(old_sha1);\n \n \tlock = xcalloc(1, sizeof(struct ref_lock));\n \tlock->lock_fd = -1;\n-- \n1.4.2.1.g7a39b\n"},{"id":"27689","messageId":"7vu02uqzaj.fsf@assigned-by-dhcp.cox.net","threadId":"5596","inReplyTo":"7vmz8o1em0.fsf_-_@assigned-by-dhcp.cox.net","subject":"[PATCH 3/3] update a few Porcelain-ish for ref lock safety.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-09-26T18:05:08Z","receivedAt":"2006-09-26T18:05:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This updates the use of git-update-ref in git-branch, git-tag\nand git-commit to make them safer in a few corner cases as\ndemonstration.\n\n - git-tag makes sure that the named tag does not exist, allows\n you to edit tag message and then creates the tag. If a tag\n with the same name was created by somebody else in the\n meantime, it used to happily overwrote it. Now it notices\n the situation.\n\n - git-branch -d and git-commit (for the initial commit) had the\n same issue but with smaller race window, which is plugged\n with this.\n\nSigned-off-by: Junio C Hamano \n---\n\n * Obviously I would need to update this on top of Linus's\n packed-refs, but this 3-patch series applies on top of the\n current \"master\". \n\n git-branch.sh | 9 ++++++---\n git-commit.sh | 2 +-\n git-tag.sh | 9 ++++++---\n 3 files changed, 13 insertions(+), 7 deletions(-)\n\ndiff --git a/git-branch.sh b/git-branch.sh\nindex e0501ec..2b58d20 100755\n--- a/git-branch.sh\n+++ b/git-branch.sh\n@@ -42,8 +42,7 @@ If you are sure you want to delete it, r\n \t esac\n \t ;;\n \tesac\n-\trm -f \"$GIT_DIR/logs/refs/heads/$branch_name\"\n-\trm -f \"$GIT_DIR/refs/heads/$branch_name\"\n+\tgit update-ref -d \"refs/heads/$branch_name\" \"$branch\"\n \techo \"Deleted branch $branch_name.\"\n done\n exit 0\n@@ -112,6 +111,7 @@ rev=$(git-rev-parse --verify \"$head\") ||\n git-check-ref-format \"heads/$branchname\" ||\n \tdie \"we do not like '$branchname' as a branch name.\"\n \n+prev=0000000000000000000000000000000000000000\n if [ -e \"$GIT_DIR/refs/heads/$branchname\" ]\n then\n \tif test '' = \"$force\"\n@@ -121,10 +121,13 @@ then\n \tthen\n \t\tdie \"cannot force-update the current branch.\"\n \tfi\n+\tprev=`git rev-parse --verify \"refs/heads/$branchname\"`\n fi\n if test \"$create_log\" = 'yes'\n then\n \tmkdir -p $(dirname \"$GIT_DIR/logs/refs/heads/$branchname\")\n \ttouch \"$GIT_DIR/logs/refs/heads/$branchname\"\n fi\n-git update-ref -m \"branch: Created from $head\" \"refs/heads/$branchname\" $rev\n+git update-ref -m \"branch: Created from $head\" \\\n+\t\"refs/heads/$branchname\" $rev $prev\n+\ndiff --git a/git-commit.sh b/git-commit.sh\nindex 5a4c659..87b13ef 100755\n--- a/git-commit.sh\n+++ b/git-commit.sh\n@@ -554,8 +554,8 @@ else\n \t\texit 1\n \tfi\n \tPARENTS=\"\"\n-\tcurrent=\n \trloga='commit (initial)'\n+\tcurrent=0000000000000000000000000000000000000000\n fi\n \n if test -z \"$no_edit\"\ndiff --git a/git-tag.sh b/git-tag.sh\nindex a0afa25..2bde3c0 100755\n--- a/git-tag.sh\n+++ b/git-tag.sh\n@@ -63,8 +63,11 @@ done\n \n name=\"$1\"\n [ \"$name\" ] || usage\n-if [ -e \"$GIT_DIR/refs/tags/$name\" -a -z \"$force\" ]; then\n- die \"tag '$name' already exists\"\n+prev=0000000000000000000000000000000000000000\n+if test -e \"$GIT_DIR/refs/tags/$name\"\n+then\n+ test -n \"$force\" || die \"tag '$name' already exists\"\n+ prev=`git rev-parse \"refs/tags/$name\"`\n fi\n shift\n git-check-ref-format \"tags/$name\" ||\n@@ -109,4 +112,4 @@ fi\n \n leading=`expr \"refs/tags/$name\" : '\\(.*\\)/'` &&\n mkdir -p \"$GIT_DIR/$leading\" &&\n-echo $object > \"$GIT_DIR/refs/tags/$name\"\n+GIT_DIR=\"$GIT_DIR\" git update-ref \"refs/tags/$name\" \"$object\" \"$prev\"\n-- \n1.4.2.1.g7a39b\n"},{"id":"27690","messageId":"45196CAB.6030903@shadowen.org","threadId":"5596","inReplyTo":"7vu02uqzaj.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 3/3] update a few Porcelain-ish for ref lock safety.","fromName":"Andy Whitcroft","fromEmail":"apw@shadowen.org","sentAt":"2006-09-26T18:08:43Z","receivedAt":"2006-09-26T18:08:43Z","isPatch":true,"sender":{"key":"apw@shadowen.org","avatar":"https://gravatar.com/avatar/d3088262854661a913ef35cc40fedcc270142d4461791142bc1ea0b2a4e2e147?d=mp&s=160"},"body":"Junio C Hamano wrote:\n> This updates the use of git-update-ref in git-branch, git-tag\n> and git-commit to make them safer in a few corner cases as\n> demonstration.\n> \n> - git-tag makes sure that the named tag does not exist, allows\n> you to edit tag message and then creates the tag. If a tag\n> with the same name was created by somebody else in the\n> meantime, it used to happily overwrote it. Now it notices\n> the situation.\n> \n> - git-branch -d and git-commit (for the initial commit) had the\n> same issue but with smaller race window, which is plugged\n> with this.\n> \n> Signed-off-by: Junio C Hamano \n> ---\n> \n> * Obviously I would need to update this on top of Linus's\n> packed-refs, but this 3-patch series applies on top of the\n> current \"master\". \n> \n> git-branch.sh | 9 ++++++---\n> git-commit.sh | 2 +-\n> git-tag.sh | 9 ++++++---\n> 3 files changed, 13 insertions(+), 7 deletions(-)\n> \n> diff --git a/git-branch.sh b/git-branch.sh\n> index e0501ec..2b58d20 100755\n> --- a/git-branch.sh\n> +++ b/git-branch.sh\n> @@ -42,8 +42,7 @@ If you are sure you want to delete it, r\n> \t esac\n> \t ;;\n> \tesac\n> -\trm -f \"$GIT_DIR/logs/refs/heads/$branch_name\"\n> -\trm -f \"$GIT_DIR/refs/heads/$branch_name\"\n> +\tgit update-ref -d \"refs/heads/$branch_name\" \"$branch\"\n> \techo \"Deleted branch $branch_name.\"\n> done\n> exit 0\n> @@ -112,6 +111,7 @@ rev=$(git-rev-parse --verify \"$head\") ||\n> git-check-ref-format \"heads/$branchname\" ||\n> \tdie \"we do not like '$branchname' as a branch name.\"\n> \n> +prev=0000000000000000000000000000000000000000\n> if [ -e \"$GIT_DIR/refs/heads/$branchname\" ]\n> then\n> \tif test '' = \"$force\"\n> @@ -121,10 +121,13 @@ then\n> \tthen\n> \t\tdie \"cannot force-update the current branch.\"\n> \tfi\n> +\tprev=`git rev-parse --verify \"refs/heads/$branchname\"`\n> fi\n> if test \"$create_log\" = 'yes'\n> then\n> \tmkdir -p $(dirname \"$GIT_DIR/logs/refs/heads/$branchname\")\n> \ttouch \"$GIT_DIR/logs/refs/heads/$branchname\"\n> fi\n> -git update-ref -m \"branch: Created from $head\" \"refs/heads/$branchname\" $rev\n> +git update-ref -m \"branch: Created from $head\" \\\n> +\t\"refs/heads/$branchname\" $rev $prev\n> +\n> diff --git a/git-commit.sh b/git-commit.sh\n> index 5a4c659..87b13ef 100755\n> --- a/git-commit.sh\n> +++ b/git-commit.sh\n> @@ -554,8 +554,8 @@ else\n> \t\texit 1\n> \tfi\n> \tPARENTS=\"\"\n> -\tcurrent=\n> \trloga='commit (initial)'\n> +\tcurrent=0000000000000000000000000000000000000000\n> fi\n> \n> if test -z \"$no_edit\"\n> diff --git a/git-tag.sh b/git-tag.sh\n> index a0afa25..2bde3c0 100755\n> --- a/git-tag.sh\n> +++ b/git-tag.sh\n> @@ -63,8 +63,11 @@ done\n> \n> name=\"$1\"\n> [ \"$name\" ] || usage\n> -if [ -e \"$GIT_DIR/refs/tags/$name\" -a -z \"$force\" ]; then\n> - die \"tag '$name' already exists\"\n> +prev=0000000000000000000000000000000000000000\n\nIt seems a little odd to need to use such a large 'none' thing. Will\nlinus' updates start returning this when there is no tag? If so then it\nmakes sense. Else perhaps it would be nice to have a short cut for it.\n Such as 'none'.\n\n-apw\n"},{"id":"27769","messageId":"7vbqp1g492.fsf@assigned-by-dhcp.cox.net","threadId":"5596","inReplyTo":"45196CAB.6030903@shadowen.org","subject":"Re: [PATCH 3/3] update a few Porcelain-ish for ref lock safety.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-09-27T07:25:45Z","receivedAt":"2006-09-27T07:25:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andy Whitcroft writes:\n\n>> +prev=0000000000000000000000000000000000000000\n>\n> It seems a little odd to need to use such a large 'none' thing. Will\n> linus' updates start returning this when there is no tag? If so then it\n> makes sense. Else perhaps it would be nice to have a short cut for it.\n> Such as 'none'.\n\nTrue. It probably is better to accept something shorter, like\nan empty string, or token \"none\". But this is a plumbing\ncommand, and I was not too concerned about having to say 40 \"0\"\nin the Porcelain scripts.\n"}]}