* [PATCH] Remove branch by putting a null sha1 into the ref file. @ 2006-09-18 4:54 Christian Couder 2006-09-18 5:47 ` Junio C Hamano 2006-09-18 16:31 ` Linus Torvalds 0 siblings, 2 replies; 19+ messages in thread From: Christian Couder @ 2006-09-18 4:54 UTC (permalink / raw) To: Junio Hamano; +Cc: git With the new packed ref file format from Linus, this should be the new way to remove a branch. "refs.c" is fixed so that a null sha1 for a deleted branch does not result in "refs/head/deleted does not point to a valid commit object!" messages. "t/t3200-branch.sh" is fixed so that it uses git-show-ref instead of checking that the ref does not exist when a branch is deleted. Signed-off-by: Christian Couder <chriscool@tuxfamily.org> --- git-branch.sh | 6 +++++- refs.c | 2 ++ t/t3200-branch.sh | 2 +- 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/git-branch.sh b/git-branch.sh index 2600e9c..cb55880 100755 --- a/git-branch.sh +++ b/git-branch.sh @@ -10,6 +10,9 @@ SUBDIRECTORY_OK='Yes' headref=$(git-symbolic-ref HEAD | sed -e 's|^refs/heads/||') +# Fourty 0s. +NULL_SHA1="0000000000000000000000000000000000000000" + delete_branch () { option="$1" shift @@ -43,7 +46,8 @@ If you are sure you want to delete it, r ;; esac rm -f "$GIT_DIR/logs/refs/heads/$branch_name" - rm -f "$GIT_DIR/refs/heads/$branch_name" + echo $NULL_SHA1 > "$GIT_DIR/refs/heads/$branch_name" || \ + die "Failed to delete branch '$branch_name' !" echo "Deleted branch $branch_name." done exit 0 diff --git a/refs.c b/refs.c index 5e65314..76d8d0e 100644 --- a/refs.c +++ b/refs.c @@ -162,6 +162,8 @@ static int do_for_each_ref(const char *b error("%s points nowhere!", path); continue; } + if (is_null_sha1(sha1)) + continue; /* Ignore deleted refs. */ if (!has_sha1_file(sha1)) { error("%s does not point to a valid " "commit object!", path); diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index 5b04efc..150dfdc 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -47,7 +47,7 @@ test_expect_success \ test_expect_success \ 'git branch -d d/e/f should delete a branch and a log' \ 'git-branch -d d/e/f && - test ! -f .git/refs/heads/d/e/f && + ! git-show-ref --verify --quiet -- "refs/heads/d/e/f" && test ! -f .git/logs/refs/heads/d/e/f' cat >expect <<EOF -- 1.4.2.1.g4251-dirty ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] Remove branch by putting a null sha1 into the ref file. 2006-09-18 4:54 [PATCH] Remove branch by putting a null sha1 into the ref file Christian Couder @ 2006-09-18 5:47 ` Junio C Hamano 2006-09-18 16:31 ` Linus Torvalds 1 sibling, 0 replies; 19+ messages in thread From: Junio C Hamano @ 2006-09-18 5:47 UTC (permalink / raw) To: Christian Couder; +Cc: git Christian Couder <chriscool@tuxfamily.org> writes: > @@ -43,7 +46,8 @@ If you are sure you want to delete it, r > ;; > esac > rm -f "$GIT_DIR/logs/refs/heads/$branch_name" > - rm -f "$GIT_DIR/refs/heads/$branch_name" > + echo $NULL_SHA1 > "$GIT_DIR/refs/heads/$branch_name" || \ > + die "Failed to delete branch '$branch_name' !" Don't you need mkdir -p somewhere? > diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh > index 5b04efc..150dfdc 100755 > --- a/t/t3200-branch.sh > +++ b/t/t3200-branch.sh > @@ -47,7 +47,7 @@ test_expect_success \ > test_expect_success \ > 'git branch -d d/e/f should delete a branch and a log' \ > 'git-branch -d d/e/f && > - test ! -f .git/refs/heads/d/e/f && > + ! git-show-ref --verify --quiet -- "refs/heads/d/e/f" && > test ! -f .git/logs/refs/heads/d/e/f' I am old-fashioned and it makes me think twice when I see people do "! command" in shell. Bash and dash has support for it, and opengroup has it in its base specification, so probably it is Ok. As usual, Solaris /bin/sh does not grok it ;-). ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Remove branch by putting a null sha1 into the ref file. 2006-09-18 4:54 [PATCH] Remove branch by putting a null sha1 into the ref file Christian Couder 2006-09-18 5:47 ` Junio C Hamano @ 2006-09-18 16:31 ` Linus Torvalds 2006-09-18 18:43 ` Junio C Hamano ` (2 more replies) 1 sibling, 3 replies; 19+ messages in thread From: Linus Torvalds @ 2006-09-18 16:31 UTC (permalink / raw) To: Christian Couder; +Cc: Junio Hamano, git On Mon, 18 Sep 2006, Christian Couder wrote: > > With the new packed ref file format from Linus, this should be > the new way to remove a branch. Well, it's not really sufficient. Somebody should add this test-case git branch test git branch -d test git branch test/first which should work. It's entirely possible that the proper way to do branch deletion with packed branches is to simply re-pack without the old branch, rather than the negative branch model. I couldn't really decide. However: this part is definitely correct, considering that we allow the null sha1 in other places. > "refs.c" is fixed so that a null sha1 for a deleted branch does > not result in "refs/head/deleted does not point to a valid > commit object!" messages. And this last part is conceptually ok, but I think the implementaion is wrong: > "t/t3200-branch.sh" is fixed so that it uses git-show-ref > instead of checking that the ref does not exist when a branch > is deleted. I think you should change the ! git-show-ref --verify --quiet -- "refs/heads/d/e/f" && into git-show-ref --verify --quiet -- "refs/heads/d/e/f" || instead, no? Linus ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Remove branch by putting a null sha1 into the ref file. 2006-09-18 16:31 ` Linus Torvalds @ 2006-09-18 18:43 ` Junio C Hamano 2006-09-20 2:56 ` Christian Couder 2006-09-22 22:09 ` Junio C Hamano 2 siblings, 0 replies; 19+ messages in thread From: Junio C Hamano @ 2006-09-18 18:43 UTC (permalink / raw) To: Linus Torvalds; +Cc: git, Christian Couder Linus Torvalds <torvalds@osdl.org> writes: > On Mon, 18 Sep 2006, Christian Couder wrote: >> >> With the new packed ref file format from Linus, this should be >> the new way to remove a branch. > > Well, it's not really sufficient. > > Somebody should add this test-case > > git branch test > git branch -d test > git branch test/first > > which should work. Also this test-case needs to be added git branch test git pack-refs rm .git/refs/heads/test git branch test/second which should barf. Otherwise it would allow both test and test/second branches to exist, and trying to clone from such a repository would fail, at least by the existing tools (it might have been even nicer if we from day one allowed both test and test/second to exist, though, but it is too late now, or too early before we upgrade everybody). > It's entirely possible that the proper way to do branch deletion with > packed branches is to simply re-pack without the old branch, rather than > the negative branch model. I couldn't really decide. Or mkdir there ,-). ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Remove branch by putting a null sha1 into the ref file. 2006-09-18 16:31 ` Linus Torvalds 2006-09-18 18:43 ` Junio C Hamano @ 2006-09-20 2:56 ` Christian Couder 2006-09-22 22:09 ` Junio C Hamano 2 siblings, 0 replies; 19+ messages in thread From: Christian Couder @ 2006-09-20 2:56 UTC (permalink / raw) To: Linus Torvalds; +Cc: Junio Hamano, git Linus Torvalds wrote: > On Mon, 18 Sep 2006, Christian Couder wrote: > > With the new packed ref file format from Linus, this should be > > the new way to remove a branch. > > Well, it's not really sufficient. > > Somebody should add this test-case > > git branch test > git branch -d test > git branch test/first > > which should work. > > It's entirely possible that the proper way to do branch deletion with > packed branches is to simply re-pack without the old branch, rather than > the negative branch model. Yes, and anyway git-branch should probably use some other git command for this, like "git-update-ref <ref> 0" or "git-update-ref --delete <ref>". > I couldn't really decide. > > However: this part is definitely correct, considering that we allow the > null sha1 in other places. > > > "refs.c" is fixed so that a null sha1 for a deleted branch does > > not result in "refs/head/deleted does not point to a valid > > commit object!" messages. Well, it was in one of your patch already but I had not seen it. And I wrongly assumed that your patches were all merged into next but this one was not. > And this last part is conceptually ok, but I think the implementaion is > wrong: > > "t/t3200-branch.sh" is fixed so that it uses git-show-ref > > instead of checking that the ref does not exist when a branch > > is deleted. > > I think you should change the > > ! git-show-ref --verify --quiet -- "refs/heads/d/e/f" && > > into > > git-show-ref --verify --quiet -- "refs/heads/d/e/f" || > > instead, no? It seems Junio found it ok, and I still think it is ok, but my description of the change is bad. Maybe git-show-ref should have an option to check that a ref does not exist for example "git-show-ref --inexistant --quiet -- "refs/heads/d/e/f". Thanks for your comments. Christian. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Remove branch by putting a null sha1 into the ref file. 2006-09-18 16:31 ` Linus Torvalds 2006-09-18 18:43 ` Junio C Hamano 2006-09-20 2:56 ` Christian Couder @ 2006-09-22 22:09 ` Junio C Hamano 2006-09-23 4:45 ` Christian Couder 2 siblings, 1 reply; 19+ messages in thread From: Junio C Hamano @ 2006-09-22 22:09 UTC (permalink / raw) To: Linus Torvalds; +Cc: git, Christian Couder Linus Torvalds <torvalds@osdl.org> writes: > It's entirely possible that the proper way to do branch deletion with > packed branches is to simply re-pack without the old branch, rather than > the negative branch model. I couldn't really decide. After playing with branch deletion issues for some time, I started to think it would be a lot simpler if we do not mark a deleted branch with 0{40}. I started rewriting git-branch in C, mostly reusing Kristian Høgsberg's patch but taking care of the reflog issue still left in the original thread: Subject: [PATCH] branch as a builtin (again) Date: Sun, 20 Aug 2006 17:22:18 -0400 Message-ID: <59ad55d30608201422h4a6d40f7y7782212637380438@mail.gmail.com> We need to take care of two funny cases around packed-ref in lock_ref_sha1_basic() where a creation of a new ref takes place. It needs to deal with two issues. * We should not allow frotz branch to be created when frotz/nitfol branch already exists (packed and pruned). Being able to create .git/refs/heads/frotz.lock file and being able to rename it to .git/refs/heads/frotz is not enough. The other way around has the same problem. So we would need to have a for_each_ref() to check if the ref being created is a prefix of an existing one or there is an existing one that is a prefix of the one being created. * When creating frotz branch, there may be a leftover .git/refs/heads/frotz/ directory on the filesystem. If the directory is empty (or contains only empty subdirectories), we can rmdir recursively and make refs/heads/frotz file there. Otherwise it means that a branch frotz/something still exists there, but the above prefix check should have taken care of it. So in practice we should always be able to just do an equivalent of "rm -fr .git/refs/heads/frotz" when creating a frotz branch. But the latter falls apart if we use 0{40} convention to mark a deleted branch. Removing .git/refs/heads/frotz/nitfol file which has 0{40} and creating .git/refs/heads/frotz file resurrects frotz/nitfol branch that is still packed. Not allowing frotz branch to be created only because we had deleted frotz/nitfol previously is not what we want either, so at that point we need to repack without frotz/nitfol anyway. Which makes me think that we would better repack when removing any existing ref. BTW, the second issue exists without packed ref; currently we cannot do git branch foo/bar git branch -d foo/bar git branch foo ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Remove branch by putting a null sha1 into the ref file. 2006-09-22 22:09 ` Junio C Hamano @ 2006-09-23 4:45 ` Christian Couder 2006-09-23 8:04 ` Junio C Hamano 0 siblings, 1 reply; 19+ messages in thread From: Christian Couder @ 2006-09-23 4:45 UTC (permalink / raw) To: Junio C Hamano; +Cc: Linus Torvalds, git Junio C Hamano wrote: > Linus Torvalds <torvalds@osdl.org> writes: > > It's entirely possible that the proper way to do branch deletion with > > packed branches is to simply re-pack without the old branch, rather > > than the negative branch model. I couldn't really decide. > > After playing with branch deletion issues for some time, I > started to think it would be a lot simpler if we do not mark > a deleted branch with 0{40}. I also came to the same conclusion and I wonder if we could delete a branch by moving the file ".git/refs/heads/frotz" to ".git/deleted-refs/heads/frotz". If the branch is packed we could perhaps just create ".git/deleted-refs/heads/frotz". It means everytime we are looking up a ref we have to check in ".git/deleted-refs/" first to see if it exists. > But the latter falls apart if we use 0{40} convention to mark a > deleted branch. Removing .git/refs/heads/frotz/nitfol file which > has 0{40} and creating .git/refs/heads/frotz file resurrects > frotz/nitfol branch that is still packed. If we move ".git/refs/heads/frotz/nitfol" to ".git/deleted-refs/heads/frotz/nitfol" when we remove this ref, we only need to try to rmdir all subdirectories under ".git/refs/heads/frotz/" and then ".git/refs/heads/frotz/" to see if we can create ".git/refs/heads/frotz", and if we can, we won't resurect "frotz/nitfol" because ".git/deleted-refs/heads/frotz/nitfol" still exists. > Not allowing frotz > branch to be created only because we had deleted frotz/nitfol > previously is not what we want either, so at that point we need > to repack without frotz/nitfol anyway. > > Which makes me think that we would better repack when removing > any existing ref. Yes but it may not be fast and may be corruption prone. Christian. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Remove branch by putting a null sha1 into the ref file. 2006-09-23 4:45 ` Christian Couder @ 2006-09-23 8:04 ` Junio C Hamano 2006-09-23 11:22 ` Christian Couder 0 siblings, 1 reply; 19+ messages in thread From: Junio C Hamano @ 2006-09-23 8:04 UTC (permalink / raw) To: Christian Couder; +Cc: git Christian Couder <chriscool@tuxfamily.org> writes: > If we move ".git/refs/heads/frotz/nitfol" > to ".git/deleted-refs/heads/frotz/nitfol" when we remove this ref, we only > need to try to rmdir all subdirectories under ".git/refs/heads/frotz/" and > then ".git/refs/heads/frotz/" to see if we can > create ".git/refs/heads/frotz", and if we can, we won't > resurect "frotz/nitfol" because ".git/deleted-refs/heads/frotz/nitfol" > still exists. I am not sure if that would be workable. I suspect that you would need to do quite an involved sequence in "git branch" to make this sequence to work with .git/deleted-refs/ scheme: git branch frotz/nitfol git pack-refs --prune git branch -d frotz/nitfol git branch frotz git pack-refs git branch -d frotz After deleting frotz/nitfol you would create frotz/nitfol in deleted hierarchy. Then when you delete frotz you would need to create frotz in deleted hierarchy, but you cannot, without losing frotz/nitfol. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Remove branch by putting a null sha1 into the ref file. 2006-09-23 8:04 ` Junio C Hamano @ 2006-09-23 11:22 ` Christian Couder 2006-09-23 21:49 ` Junio C Hamano 0 siblings, 1 reply; 19+ messages in thread From: Christian Couder @ 2006-09-23 11:22 UTC (permalink / raw) To: Junio C Hamano; +Cc: git Junio C Hamano wrote: > Christian Couder <chriscool@tuxfamily.org> writes: > > If we move ".git/refs/heads/frotz/nitfol" > > to ".git/deleted-refs/heads/frotz/nitfol" when we remove this ref, we > > only need to try to rmdir all subdirectories under > > ".git/refs/heads/frotz/" and then ".git/refs/heads/frotz/" to see if we > > can > > create ".git/refs/heads/frotz", and if we can, we won't > > resurect "frotz/nitfol" because ".git/deleted-refs/heads/frotz/nitfol" > > still exists. > > I am not sure if that would be workable. I suspect that you > would need to do quite an involved sequence in "git branch" to > make this sequence to work with .git/deleted-refs/ scheme: > > git branch frotz/nitfol > git pack-refs --prune > git branch -d frotz/nitfol > git branch frotz > git pack-refs > git branch -d frotz > > After deleting frotz/nitfol you would create frotz/nitfol in > deleted hierarchy. Then when you delete frotz you would need to > create frotz in deleted hierarchy, but you cannot, without > losing frotz/nitfol. You are right, so what about moving ".git/refs/heads/frotz" to ".git/deleted-refs/heads/frotz.ref" or ".git/deleted-refs/heads/frotz~ref" (because "~" is forbidden in ref names). Christian. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Remove branch by putting a null sha1 into the ref file. 2006-09-23 11:22 ` Christian Couder @ 2006-09-23 21:49 ` Junio C Hamano 2006-09-24 4:45 ` Christian Couder 0 siblings, 1 reply; 19+ messages in thread From: Junio C Hamano @ 2006-09-23 21:49 UTC (permalink / raw) To: Christian Couder; +Cc: git Christian Couder <chriscool@tuxfamily.org> writes: > You are right, so what about moving ".git/refs/heads/frotz" > to ".git/deleted-refs/heads/frotz.ref" > or ".git/deleted-refs/heads/frotz~ref" (because "~" is forbidden in ref > names). But wouldn't it bring up the issue of locking among deleters, updaters/creators, and traversers? If we choose to use packed-refs.lock as the "set of all refs" lock, the whole sequence would become something like this. Note that this tries to make readers lockless but I am sure there are nasty race condition issues. I am not sure what we would want to do about them. = Looking up a ref $frotz. - check if .git/$frotz exists, and use it if it does. - check if .git/deleted-refs/$frotz~ref exists, and return "no such ref" if it does. - find $frotz in .git/packed-refs. = Looping over refs. - grab all .git/refs/ and subtract all .git/deleted-refs/ - walk .git/packed-refs and the result from the above in parallel as in the current code. = Storing a new value in ref $frotz. - acquire .git/packed-refs.lock - lock .git/$frotz.lock. - write into .git/$frotz.lock. - create or update .git/logs/$frotz as needed. - if .git/deleted-refs/$frotz~ref exists, unlink it. - rename .git/$frotz.lock to .git/$frotz to unlock it. - unlink .git/packed-refs.lock = Deleting a ref $frotz. - acquire .git/packed-refs.lock - look up $frotz; if it does not exist either barf or return silent (haven't thought it through yet). - create .git/deleted-refs/$frotz~ref. - remove .git/logs/$frotz - unlink .git/packed-refs.lock = Packing refs, with optional pruning. - lock .git/packed-refs.lock - loop over refs: - write it out to .git/packed-refs.lock unless a symref. - if it is a loose one (not a symref), remember it for pruning. - if pruning: - remove the entire .git/deleted-refs/ hierarchy - remove the remembered ones - rename .git/packed-refs.lock to .git/packed-refs ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Remove branch by putting a null sha1 into the ref file. 2006-09-23 21:49 ` Junio C Hamano @ 2006-09-24 4:45 ` Christian Couder 2006-09-25 9:26 ` On ref locking Junio C Hamano 0 siblings, 1 reply; 19+ messages in thread From: Christian Couder @ 2006-09-24 4:45 UTC (permalink / raw) To: Junio C Hamano; +Cc: git Junio C Hamano wrote: > Christian Couder <chriscool@tuxfamily.org> writes: > > You are right, so what about moving ".git/refs/heads/frotz" > > to ".git/deleted-refs/heads/frotz.ref" > > or ".git/deleted-refs/heads/frotz~ref" (because "~" is forbidden in ref > > names). > > But wouldn't it bring up the issue of locking among deleters, > updaters/creators, and traversers? Yes, it will get worse. > If we choose to use packed-refs.lock as the "set of all refs" > lock, the whole sequence would become something like this. Note > that this tries to make readers lockless but I am sure there are > nasty race condition issues. I am not sure what we would want > to do about them. > > = Looking up a ref $frotz. We could acquire .git/$frotz.lock here if we want to be sure that nothing will happen to it while we read it, but anyway something could happen to it just after we read it and before we use it. So the right thing to do is perhaps to let the caller acquire the lock if it needs to. We should just check in the other cases below that nothing can happen to the ref if someone acquired .git/$frotz.lock. > - check if .git/$frotz exists, and use it if it does. > > - check if .git/deleted-refs/$frotz~ref exists, and return "no > such ref" if it does. > > - find $frotz in .git/packed-refs. > > = Looping over refs. Same as above. Depending on what the caller wants to do, it could acquire .git/packed-refs.lock or some .git/$frotz.lock to make sure nothing happens to all or only some refs. > - grab all .git/refs/ and subtract all .git/deleted-refs/ > > - walk .git/packed-refs and the result from the above in > parallel as in the current code. > > = Storing a new value in ref $frotz. > > - acquire .git/packed-refs.lock > > - lock .git/$frotz.lock. > > - write into .git/$frotz.lock. > > - create or update .git/logs/$frotz as needed. > > - if .git/deleted-refs/$frotz~ref exists, unlink it. > > - rename .git/$frotz.lock to .git/$frotz to unlock it. > > - unlink .git/packed-refs.lock > > = Deleting a ref $frotz. > > - acquire .git/packed-refs.lock - acquire .git/$frotz.lock seems needed too > - look up $frotz; if it does not exist either barf or return > silent (haven't thought it through yet). > > - create .git/deleted-refs/$frotz~ref or move .git/$frotz to .git/deleted-refs/$frotz~ref if .git/$frotz exists > - remove .git/logs/$frotz - unlink .git/$frotz.lock > - unlink .git/packed-refs.lock > > = Packing refs, with optional pruning. > > - lock .git/packed-refs.lock > > - loop over refs: > - write it out to .git/packed-refs.lock unless a symref. > - if it is a loose one (not a symref), remember it for pruning. > > - if pruning: > - remove the entire .git/deleted-refs/ hierarchy > - remove the remembered ones Perhaps we should acquire .git/$frotz.lock for each $frotz that we remove when pruning. > - rename .git/packed-refs.lock to .git/packed-refs Thanks, Christian. ^ permalink raw reply [flat|nested] 19+ messages in thread
* On ref locking 2006-09-24 4:45 ` Christian Couder @ 2006-09-25 9:26 ` Junio C Hamano 2006-09-25 17:05 ` Daniel Barkalow ` (3 more replies) 0 siblings, 4 replies; 19+ messages in thread From: Junio C Hamano @ 2006-09-25 9:26 UTC (permalink / raw) To: Christian Couder Cc: git, Linus Torvalds, Daniel Barkalow, Johannes Schindelin The comments you added to the strawman I sent suggested use of rather heavyweight locks, which made me feel we were somehow going in a wrong direction. Before going into the details of branch removing, let's first see if we can summarize what kind of guarantee we would want from ref updates. The current locking scheme is very carefully and nicely done by Linus and Daniel Barkalow around June last year, and I do not want to lose good property of it. - When reading and/or listing refs you do not need to acquire any lock. - When you are going to update an existing $ref, you create $ref.lock, and do a compare-and-swap. What the latter means is that an updater: (1) first learns the current value of the $ref, without locking; (2) decides based on the knowledge from (1) what the next value should be; (3) gets $ref.lock, makes sure $ref still is the value it learned in (1), updates it to the desired value and releases the lock. The above 3-step sequence prevents updater-updater races with an extremely short critical section. We only need to hold the lock while we do compare and swap. The mandatory "fast-forward" check by receive-pack introduced by Johannes (see receive-pack.c::update()) does exactly the above. The program reads the current value of refs involved very early, and verifies fast-forward-ness of the update here. After doing that, it gets the lock, makes sure the refs are still the same as we read earlier (meaning, nobody else updated the ref while we were doing other things, thereby invalidating the checks we've done earlier) and then finally unlocks it. Side note: we might want to move the call to run_update_hook() before creating the lock for the same reason as we do the new fast-forwards test before acquiring the lock. I am not sure if the current code in receive-pack is doing the right thing to prevent creator-creator race, though. While creating a new ref, verify_old_ref() is called with 0{40} as the old value (meaning, "we are creating this ref and setting it to this value, based on our previous knowledge that it did not exist"). It happily returns without checking anything in this case, but I think it should make sure nobody created the ref while we were looking the other way. With the current code, we would allow overwriting somebody else's push. And the race window is not so small -- the check that the ref is a new one we are about to create is performed during the initial protocol handshake, and then we can spend quite some time unpacking the pack stream until we get to verify_old_ref(). The same 3-step sequence is done by refs.c::lock_ref_sha1() and lock_any_ref_for_update() API and Porcelains are expected to use git-update-ref. You call them with the current value of the ref as you learned earlier, and you would get a lock. If you get a lock successfully, you write the new value out. Side note: I think the same issue of creator-creator race exists in verify_lock(), by the way. I think we can fix this by accepting 0{40} SHA1 to git-update-ref to mean "I am creating this based on the assumption that it does not exist yet -- that is my understanding from my earlier check". We are very relaxed about deleting refs right now, compared to the updates described above. "git branch -d" barfs if the ref it wanted to delete is not an ancestor of the current branch, but it does not have any lock between the time it checks and the time it actually deletes it. There are very loosely written ref updates in Porcelains. "git tag" and "git branch" barf if the ref they wanted to create exists, but both have rather large race window without the protection of any lock. Especially "git tag" race window is large -- it lets you open an editor to type the tag message and runs gpg to sign the message after checking if you are not overwriting an existing tag X-<. These should be fixed, and I think we can do so by the above fix to git-update-ref I mentioned in the above side note. We might need to update the ref locking to adjust to packed refs, and getting locks around ref deletion right becomes more important if we want to do the funky .git/deleted-refs/$ref~ref business. But we should try to stick to the same 3-step sequence. What this means is that a deleter: (1) first learns the current value of the $ref, without locking; (2) decides based on the knowledge from (1) it indeed wants to delete it; (3) gets the $ref.lock, makes sure $ref still has the value it learned in (1), and deletes it. As a special case, if $ref no longer exists, that does not have to be an error. Somebody else deleted it while we were looking the other way, but a delete is a delete is a delete, and we are simply happy that the ref is gone. This should protect us from deleter-updater race. $ref.lock would protect us from deleter-creator race. So I do not think we would need to take .git/packed-refs.lock while deleting a ref. Even if you implement "(3) ... deletes it" with the proposed .git/deleted-refs/$ref~ref file. I think the way Linus did git-pack-refs protects us from packer-packer race, and packer-updater, packer-creator, and packer-deleter race does not exist, because pack-refs: (0) takes .git/packed-refs.lock (1) learns the current value of all refs without any further locking; (2) writes out the current values; (3) releases .git/packed-refs.lock. If somebody else creates or updates a ref while the above is running, the new result from loose refs will be used by later user, making the entry in packed-refs obsolete. So there is no packer-updater or packer-creator race here. If we do the proposed .git/deleted-refs/$ref~ref to mark deletion, that would also override whatever is in the resulting packed-refs, so we do not have to worry about packer-deleter race either. I am reasonably sure there is no pruner-updater and pruner-creator races. "pack-refs --prune": (1) learns the current value of all refs without any locking, as part of the packing operation during the same run. For each ref: (2) takes $ref.lock, makes sure $ref still has the value it learned from (1), and deletes .git/$ref (because the same value is recorded in .git/packed-refs), and then unlocks $ref.lock I haven't thought through packer-pruner race (pruner needs to unlock .git/packed-refs.lock first -- otherwise if it dies in the middle of pruning we would lose refs that are stashed in the .git/packed-refs.lock file, that hasn't made to its final location), but I think we do not have any problem. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: On ref locking 2006-09-25 9:26 ` On ref locking Junio C Hamano @ 2006-09-25 17:05 ` Daniel Barkalow 2006-09-26 4:11 ` Junio C Hamano 2006-09-26 4:13 ` [PATCH 1/3] Clean-up lock-ref implementation Junio C Hamano ` (2 subsequent siblings) 3 siblings, 1 reply; 19+ messages in thread From: Daniel Barkalow @ 2006-09-25 17:05 UTC (permalink / raw) To: Junio C Hamano; +Cc: Christian Couder, git, Linus Torvalds, Johannes Schindelin On Mon, 25 Sep 2006, Junio C Hamano wrote: > The comments you added to the strawman I sent suggested use of > rather heavyweight locks, which made me feel we were somehow > going in a wrong direction. Before going into the details of > branch removing, let's first see if we can summarize what kind > of guarantee we would want from ref updates. The current > locking scheme is very carefully and nicely done by Linus and > Daniel Barkalow around June last year, and I do not want to lose > good property of it. > > - When reading and/or listing refs you do not need to acquire > any lock. > > - When you are going to update an existing $ref, you create > $ref.lock, and do a compare-and-swap. > > What the latter means is that an updater: > > (1) first learns the current value of the $ref, without > locking; > > (2) decides based on the knowledge from (1) what the next value > should be; > > (3) gets $ref.lock, makes sure $ref still is the value it > learned in (1), updates it to the desired value and > releases the lock. > > The above 3-step sequence prevents updater-updater races with an > extremely short critical section. We only need to hold the lock > while we do compare and swap. I remember having a certain amount of disagreement over whether it's better to actually take the lock in (1), and hold it through (2), or only verify that it didn't change in (3) after taking the lock for real. If there's nothing substantial going on in (2), it doesn't matter. If users are producing content (e.g., git tag), they may want to make sure that nobody else is in the middle of writing something that would conflict. I think I'd advocated getting the lock early if you're going to want it, and I don't remember what the convincing argument on the other side was for the cases under consideration at the time, beyond it not mattering significantly. Note that we make sure to remove the lock when aborting due to signals (assuming we get a chance), so the size of the critical section doesn't matter too much to the chance of it getting left locked inappropriately. Of course, this is harder to do with the core code for a shell script than for C code. -Daniel *This .sig left intentionally blank* ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: On ref locking 2006-09-25 17:05 ` Daniel Barkalow @ 2006-09-26 4:11 ` Junio C Hamano 0 siblings, 0 replies; 19+ messages in thread From: Junio C Hamano @ 2006-09-26 4:11 UTC (permalink / raw) To: Daniel Barkalow Cc: Christian Couder, git, Linus Torvalds, Johannes Schindelin Daniel Barkalow <barkalow@iabervon.org> writes: > On Mon, 25 Sep 2006, Junio C Hamano wrote: >... >> What the latter means is that an updater: >> >> (1) first learns the current value of the $ref, without >> locking; >> >> (2) decides based on the knowledge from (1) what the next value >> should be; >> >> (3) gets $ref.lock, makes sure $ref still is the value it >> learned in (1), updates it to the desired value and >> releases the lock. >> >> The above 3-step sequence prevents updater-updater races with an >> extremely short critical section. We only need to hold the lock >> while we do compare and swap. > > I remember having a certain amount of disagreement over whether it's > better to actually take the lock in (1), and hold it through (2), or only > verify that it didn't change in (3) after taking the lock for real. If > there's nothing substantial going on in (2), it doesn't matter. If users > are producing content (e.g., git tag), they may want to make sure that > nobody else is in the middle of writing something that would conflict. > > I think I'd advocated getting the lock early if you're going to want it, > and I don't remember what the convincing argument on the other side was > for the cases under consideration at the time, beyond it not mattering > significantly. > > Note that we make sure to remove the lock when aborting due to signals > (assuming we get a chance), so the size of the critical section doesn't > matter too much to the chance of it getting left locked inappropriately. > Of course, this is harder to do with the core code for a shell script than > for C code. I do not recall that arguments on the list but what you say makes sense. Taking a lock early makes implementation more cumbersome on the scripting side but reduces the annoyance factor. You will be annoyed if the tool tells you somebody else did something to the ref you were going to update and your intended operation is blocked after carefully editing your tag message or commit log message. However, I think the distributed nature of git makes it less of an issue for normal repositories. Shared distribution point repositories (where everybody pushes to and fetches from) needs to protect between updaters lest that we lose commits (fetching a rev older while a push is in progress is not an error -- re-feching would give you the final result), and that is what Johannes's recent patch does. Pushing into a repository with working tree does have a lot of issues (e.g. push to update the current branch head while a commit is in progress) but doing that as the workflow has problems for other reasons as well (i.e. push to update the current branch would make the index out of sync even when no commit is in progress). It is strongly discouraged to share $GIT_DIR/refs, just like in any halfway sane development workflow people do not share working tree files. So "locking before letting the user spend effort so that we would not later have to say 'sorry, we cannot let you do that because somebody else updated the ref in the meanwhile'" is technically not incorrect, and we should be able to make things work that way, but I think it pretty much is a "why bother" issue. "git tag" checks if the new tag being created does not exist to reduce the annoyance factor, and the check does not even attempt to avoid a race. If we revalidated that the tag is still not there while storing the tag object name to the ref in the final step, I think that is good enough, and I think having "lock during operation" buys us much real-life benefit. git-commit has the same attitude, and I think it is the right balance. It checks if the commit being created is an initial commit, a single-parent commit or a merge first, prepares "-p" arguments to the commit-tree that will happen much later, then lets the user prepare the log message. Theoretically somebody else could update the current HEAD in the meantime and the resulting commit can have ancestors that are different from what was intended. We do check and avoid it by making sure that the HEAD is still where we thought it would be before doing the final. ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/3] Clean-up lock-ref implementation 2006-09-25 9:26 ` On ref locking Junio C Hamano 2006-09-25 17:05 ` Daniel Barkalow @ 2006-09-26 4:13 ` Junio C Hamano 2006-09-26 4:13 ` [PATCH 2/3] update-ref: -d flag and ref creation safety Junio C Hamano 2006-09-26 18:05 ` [PATCH 3/3] update a few Porcelain-ish for ref lock safety Junio C Hamano 3 siblings, 0 replies; 19+ messages in thread From: Junio C Hamano @ 2006-09-26 4:13 UTC (permalink / raw) To: git This drops "mustexist" parameter lock_ref_sha1() and lock_any_ref_forupdate() functions take. Existing callers were always passing 0. Signed-off-by: Junio C Hamano <junkio@cox.net> --- * A bit of clean-up before the real changes... builtin-update-ref.c | 2 +- fetch.c | 2 +- refs.c | 20 +++++++++----------- refs.h | 4 ++-- 4 files changed, 13 insertions(+), 15 deletions(-) diff --git a/builtin-update-ref.c b/builtin-update-ref.c index 90a3da5..ab52833 100644 --- a/builtin-update-ref.c +++ b/builtin-update-ref.c @@ -48,7 +48,7 @@ int cmd_update_ref(int argc, const char if (oldval && get_sha1(oldval, oldsha1)) die("%s: not a valid old SHA1", oldval); - lock = lock_any_ref_for_update(refname, oldval ? oldsha1 : NULL, 0); + lock = lock_any_ref_for_update(refname, oldval ? oldsha1 : NULL); if (!lock) return 1; if (write_ref_sha1(lock, sha1, msg) < 0) diff --git a/fetch.c b/fetch.c index 34df8d3..74585c4 100644 --- a/fetch.c +++ b/fetch.c @@ -266,7 +266,7 @@ int pull(int targets, char **target, con if (!write_ref || !write_ref[i]) continue; - lock[i] = lock_ref_sha1(write_ref[i], NULL, 0); + lock[i] = lock_ref_sha1(write_ref[i], NULL); if (!lock[i]) { error("Can't lock ref %s", write_ref[i]); goto unlock_and_fail; diff --git a/refs.c b/refs.c index 5e65314..b4f6d4f 100644 --- a/refs.c +++ b/refs.c @@ -264,12 +264,11 @@ int check_ref_format(const char *ref) } } -static struct ref_lock *verify_lock(struct ref_lock *lock, - const unsigned char *old_sha1, int mustexist) +static struct ref_lock *verify_lock(struct ref_lock *lock, const unsigned char *old_sha1) { char buf[40]; int nr, fd = open(lock->ref_file, O_RDONLY); - if (fd < 0 && (mustexist || errno != ENOENT)) { + if (fd < 0 && errno != ENOENT) { error("Can't verify ref %s", lock->ref_file); unlock_ref(lock); return NULL; @@ -292,11 +291,12 @@ static struct ref_lock *verify_lock(stru static struct ref_lock *lock_ref_sha1_basic(const char *path, int plen, - const unsigned char *old_sha1, int mustexist) + const unsigned char *old_sha1) { const char *orig_path = path; struct ref_lock *lock; struct stat st; + int mustexist = 0; lock = xcalloc(1, sizeof(struct ref_lock)); lock->lock_fd = -1; @@ -321,23 +321,21 @@ static struct ref_lock *lock_ref_sha1_ba die("unable to create directory for %s", lock->ref_file); lock->lock_fd = hold_lock_file_for_update(lock->lk, lock->ref_file, 1); - return old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock; + return old_sha1 ? verify_lock(lock, old_sha1) : lock; } -struct ref_lock *lock_ref_sha1(const char *ref, - const unsigned char *old_sha1, int mustexist) +struct ref_lock *lock_ref_sha1(const char *ref, const unsigned char *old_sha1) { if (check_ref_format(ref)) return NULL; return lock_ref_sha1_basic(git_path("refs/%s", ref), - 5 + strlen(ref), old_sha1, mustexist); + 5 + strlen(ref), old_sha1); } -struct ref_lock *lock_any_ref_for_update(const char *ref, - const unsigned char *old_sha1, int mustexist) +struct ref_lock *lock_any_ref_for_update(const char *ref, const unsigned char *old_sha1) { return lock_ref_sha1_basic(git_path("%s", ref), - strlen(ref), old_sha1, mustexist); + strlen(ref), old_sha1); } void unlock_ref(struct ref_lock *lock) diff --git a/refs.h b/refs.h index 553155c..d3bc295 100644 --- a/refs.h +++ b/refs.h @@ -24,10 +24,10 @@ extern int for_each_remote_ref(int (*fn) extern int get_ref_sha1(const char *ref, unsigned char *sha1); /** Locks a "refs/" ref returning the lock on success and NULL on failure. **/ -extern struct ref_lock *lock_ref_sha1(const char *ref, const unsigned char *old_sha1, int mustexist); +extern struct ref_lock *lock_ref_sha1(const char *ref, const unsigned char *old_sha1); /** Locks any ref (for 'HEAD' type refs). */ -extern struct ref_lock *lock_any_ref_for_update(const char *ref, const unsigned char *old_sha1, int mustexist); +extern struct ref_lock *lock_any_ref_for_update(const char *ref, const unsigned char *old_sha1); /** Release any lock taken but not written. **/ extern void unlock_ref(struct ref_lock *lock); -- 1.4.2.1.g7a39b ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/3] update-ref: -d flag and ref creation safety. 2006-09-25 9:26 ` On ref locking Junio C Hamano 2006-09-25 17:05 ` Daniel Barkalow 2006-09-26 4:13 ` [PATCH 1/3] Clean-up lock-ref implementation Junio C Hamano @ 2006-09-26 4:13 ` Junio C Hamano 2006-09-26 18:05 ` [PATCH 3/3] update a few Porcelain-ish for ref lock safety Junio C Hamano 3 siblings, 0 replies; 19+ messages in thread From: Junio C Hamano @ 2006-09-26 4:13 UTC (permalink / raw) To: git This adds -d flag to update-ref to allow safe deletion of ref. Before deleting it, the command checks if the given <oldvalue> still matches the value the caller thought the ref contained. Similarly, it also accepts 0{40} as <oldvalue> to allow safe creation of a new ref. Signed-off-by: Junio C Hamano <junkio@cox.net> --- Documentation/git-update-ref.txt | 9 ++++++-- builtin-update-ref.c | 16 +++++++++++++-- cache.h | 1 + refs.c | 41 +++++++++++++++++++++++++++++++++++++- 4 files changed, 62 insertions(+), 5 deletions(-) diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt index e062030..723906d 100644 --- a/Documentation/git-update-ref.txt +++ b/Documentation/git-update-ref.txt @@ -7,7 +7,7 @@ git-update-ref - update the object name SYNOPSIS -------- -'git-update-ref' [-m <reason>] <ref> <newvalue> [<oldvalue>] +'git-update-ref' [-m <reason>] (-d <ref> <oldvalue> | <ref> <newvalue> [<oldvalue>]) DESCRIPTION ----------- @@ -20,7 +20,8 @@ possibly dereferencing the symbolic refs the current value of the <ref> matches <oldvalue>. E.g. `git-update-ref refs/heads/master <newvalue> <oldvalue>` updates the master branch head to <newvalue> only if its current -value is <oldvalue>. +value is <oldvalue>. You can specify 40 "0" as <oldvalue> to +make sure that the ref you are creating does not exist. It also allows a "ref" file to be a symbolic pointer to another ref file by starting with the four-byte header sequence of @@ -49,6 +50,10 @@ for reading but not for writing (so we'l ref symlink to some other tree, if you have copied a whole archive by creating a symlink tree). +With `-d` flag, it deletes the named <ref> after verifying it +still contains <oldvalue>. + + Logging Updates --------------- If config parameter "core.logAllRefUpdates" is true or the file diff --git a/builtin-update-ref.c b/builtin-update-ref.c index ab52833..486b8f2 100644 --- a/builtin-update-ref.c +++ b/builtin-update-ref.c @@ -3,15 +3,16 @@ #include "refs.h" #include "builtin.h" static const char git_update_ref_usage[] = -"git-update-ref <refname> <value> [<oldval>] [-m <reason>]"; +"git-update-ref [-m <reason>] (-d <refname> <value> | <refname> <value> [<oldval>])"; int cmd_update_ref(int argc, const char **argv, const char *prefix) { const char *refname=NULL, *value=NULL, *oldval=NULL, *msg=NULL; struct ref_lock *lock; unsigned char sha1[20], oldsha1[20]; - int i; + int i, delete; + delete = 0; setup_ident(); git_config(git_default_config); @@ -26,6 +27,10 @@ int cmd_update_ref(int argc, const char die("Refusing to perform update with \\n in message."); continue; } + if (!strcmp("-d", argv[i])) { + delete = 1; + continue; + } if (!refname) { refname = argv[i]; continue; @@ -44,6 +49,13 @@ int cmd_update_ref(int argc, const char if (get_sha1(value, sha1)) die("%s: not a valid SHA1", value); + + if (delete) { + if (oldval) + usage(git_update_ref_usage); + return delete_ref(refname, sha1); + } + hashclr(oldsha1); if (oldval && get_sha1(oldval, oldsha1)) die("%s: not a valid old SHA1", oldval); diff --git a/cache.h b/cache.h index 97debd0..851f4c0 100644 --- a/cache.h +++ b/cache.h @@ -179,6 +179,7 @@ struct lock_file { extern int hold_lock_file_for_update(struct lock_file *, const char *path, int); extern int commit_lock_file(struct lock_file *); extern void rollback_lock_file(struct lock_file *); +extern int delete_ref(const char *, unsigned char *sha1); /* Environment bits from configuration mechanism */ extern int use_legacy_headers; diff --git a/refs.c b/refs.c index b4f6d4f..4d499b0 100644 --- a/refs.c +++ b/refs.c @@ -212,6 +212,32 @@ int get_ref_sha1(const char *ref, unsign return read_ref(git_path("refs/%s", ref), sha1); } +int delete_ref(const char *refname, unsigned char *sha1) +{ + struct ref_lock *lock; + int err, i, ret = 0; + + lock = lock_any_ref_for_update(refname, sha1); + if (!lock) + return 1; + i = strlen(lock->lk->filename) - 5; /* .lock */ + lock->lk->filename[i] = 0; + err = unlink(lock->lk->filename); + if (err) { + ret = 1; + error("unlink(%s) failed: %s", + lock->lk->filename, strerror(errno)); + } + lock->lk->filename[i] = '.'; + + err = unlink(lock->log_file); + if (err && errno != ENOENT) + fprintf(stderr, "warning: unlink(%s) failed: %s", + lock->log_file, strerror(errno)); + + return ret; +} + /* * Make sure "ref" is something reasonable to have under ".git/refs/"; * We do not like it if: @@ -268,6 +294,19 @@ static struct ref_lock *verify_lock(stru { char buf[40]; int nr, fd = open(lock->ref_file, O_RDONLY); + + if (is_null_sha1(old_sha1)) { + /* we wanted to make sure it did not exist */ + if (fd < 0 && errno == ENOENT) + return lock; + if (0 <= fd) { + close(fd); + error("ref %s should be absent but it exists", + lock->ref_file); + unlock_ref(lock); + return NULL; + } + } if (fd < 0 && errno != ENOENT) { error("Can't verify ref %s", lock->ref_file); unlock_ref(lock); @@ -296,7 +335,7 @@ static struct ref_lock *lock_ref_sha1_ba const char *orig_path = path; struct ref_lock *lock; struct stat st; - int mustexist = 0; + int mustexist = old_sha1 && !is_null_sha1(old_sha1); lock = xcalloc(1, sizeof(struct ref_lock)); lock->lock_fd = -1; -- 1.4.2.1.g7a39b ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 3/3] update a few Porcelain-ish for ref lock safety. 2006-09-25 9:26 ` On ref locking Junio C Hamano ` (2 preceding siblings ...) 2006-09-26 4:13 ` [PATCH 2/3] update-ref: -d flag and ref creation safety Junio C Hamano @ 2006-09-26 18:05 ` Junio C Hamano 2006-09-26 18:08 ` Andy Whitcroft 3 siblings, 1 reply; 19+ messages in thread From: Junio C Hamano @ 2006-09-26 18:05 UTC (permalink / raw) To: git This updates the use of git-update-ref in git-branch, git-tag and git-commit to make them safer in a few corner cases as demonstration. - git-tag makes sure that the named tag does not exist, allows you to edit tag message and then creates the tag. If a tag with the same name was created by somebody else in the meantime, it used to happily overwrote it. Now it notices the situation. - git-branch -d and git-commit (for the initial commit) had the same issue but with smaller race window, which is plugged with this. Signed-off-by: Junio C Hamano <junkio@cox.net> --- * Obviously I would need to update this on top of Linus's packed-refs, but this 3-patch series applies on top of the current "master". git-branch.sh | 9 ++++++--- git-commit.sh | 2 +- git-tag.sh | 9 ++++++--- 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/git-branch.sh b/git-branch.sh index e0501ec..2b58d20 100755 --- a/git-branch.sh +++ b/git-branch.sh @@ -42,8 +42,7 @@ If you are sure you want to delete it, r esac ;; esac - rm -f "$GIT_DIR/logs/refs/heads/$branch_name" - rm -f "$GIT_DIR/refs/heads/$branch_name" + git update-ref -d "refs/heads/$branch_name" "$branch" echo "Deleted branch $branch_name." done exit 0 @@ -112,6 +111,7 @@ rev=$(git-rev-parse --verify "$head") || git-check-ref-format "heads/$branchname" || die "we do not like '$branchname' as a branch name." +prev=0000000000000000000000000000000000000000 if [ -e "$GIT_DIR/refs/heads/$branchname" ] then if test '' = "$force" @@ -121,10 +121,13 @@ then then die "cannot force-update the current branch." fi + prev=`git rev-parse --verify "refs/heads/$branchname"` fi if test "$create_log" = 'yes' then mkdir -p $(dirname "$GIT_DIR/logs/refs/heads/$branchname") touch "$GIT_DIR/logs/refs/heads/$branchname" fi -git update-ref -m "branch: Created from $head" "refs/heads/$branchname" $rev +git update-ref -m "branch: Created from $head" \ + "refs/heads/$branchname" $rev $prev + diff --git a/git-commit.sh b/git-commit.sh index 5a4c659..87b13ef 100755 --- a/git-commit.sh +++ b/git-commit.sh @@ -554,8 +554,8 @@ else exit 1 fi PARENTS="" - current= rloga='commit (initial)' + current=0000000000000000000000000000000000000000 fi if test -z "$no_edit" diff --git a/git-tag.sh b/git-tag.sh index a0afa25..2bde3c0 100755 --- a/git-tag.sh +++ b/git-tag.sh @@ -63,8 +63,11 @@ done name="$1" [ "$name" ] || usage -if [ -e "$GIT_DIR/refs/tags/$name" -a -z "$force" ]; then - die "tag '$name' already exists" +prev=0000000000000000000000000000000000000000 +if test -e "$GIT_DIR/refs/tags/$name" +then + test -n "$force" || die "tag '$name' already exists" + prev=`git rev-parse "refs/tags/$name"` fi shift git-check-ref-format "tags/$name" || @@ -109,4 +112,4 @@ fi leading=`expr "refs/tags/$name" : '\(.*\)/'` && mkdir -p "$GIT_DIR/$leading" && -echo $object > "$GIT_DIR/refs/tags/$name" +GIT_DIR="$GIT_DIR" git update-ref "refs/tags/$name" "$object" "$prev" -- 1.4.2.1.g7a39b ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] update a few Porcelain-ish for ref lock safety. 2006-09-26 18:05 ` [PATCH 3/3] update a few Porcelain-ish for ref lock safety Junio C Hamano @ 2006-09-26 18:08 ` Andy Whitcroft 2006-09-27 7:25 ` Junio C Hamano 0 siblings, 1 reply; 19+ messages in thread From: Andy Whitcroft @ 2006-09-26 18:08 UTC (permalink / raw) To: Junio C Hamano; +Cc: git Junio C Hamano wrote: > This updates the use of git-update-ref in git-branch, git-tag > and git-commit to make them safer in a few corner cases as > demonstration. > > - git-tag makes sure that the named tag does not exist, allows > you to edit tag message and then creates the tag. If a tag > with the same name was created by somebody else in the > meantime, it used to happily overwrote it. Now it notices > the situation. > > - git-branch -d and git-commit (for the initial commit) had the > same issue but with smaller race window, which is plugged > with this. > > Signed-off-by: Junio C Hamano <junkio@cox.net> > --- > > * Obviously I would need to update this on top of Linus's > packed-refs, but this 3-patch series applies on top of the > current "master". > > git-branch.sh | 9 ++++++--- > git-commit.sh | 2 +- > git-tag.sh | 9 ++++++--- > 3 files changed, 13 insertions(+), 7 deletions(-) > > diff --git a/git-branch.sh b/git-branch.sh > index e0501ec..2b58d20 100755 > --- a/git-branch.sh > +++ b/git-branch.sh > @@ -42,8 +42,7 @@ If you are sure you want to delete it, r > esac > ;; > esac > - rm -f "$GIT_DIR/logs/refs/heads/$branch_name" > - rm -f "$GIT_DIR/refs/heads/$branch_name" > + git update-ref -d "refs/heads/$branch_name" "$branch" > echo "Deleted branch $branch_name." > done > exit 0 > @@ -112,6 +111,7 @@ rev=$(git-rev-parse --verify "$head") || > git-check-ref-format "heads/$branchname" || > die "we do not like '$branchname' as a branch name." > > +prev=0000000000000000000000000000000000000000 > if [ -e "$GIT_DIR/refs/heads/$branchname" ] > then > if test '' = "$force" > @@ -121,10 +121,13 @@ then > then > die "cannot force-update the current branch." > fi > + prev=`git rev-parse --verify "refs/heads/$branchname"` > fi > if test "$create_log" = 'yes' > then > mkdir -p $(dirname "$GIT_DIR/logs/refs/heads/$branchname") > touch "$GIT_DIR/logs/refs/heads/$branchname" > fi > -git update-ref -m "branch: Created from $head" "refs/heads/$branchname" $rev > +git update-ref -m "branch: Created from $head" \ > + "refs/heads/$branchname" $rev $prev > + > diff --git a/git-commit.sh b/git-commit.sh > index 5a4c659..87b13ef 100755 > --- a/git-commit.sh > +++ b/git-commit.sh > @@ -554,8 +554,8 @@ else > exit 1 > fi > PARENTS="" > - current= > rloga='commit (initial)' > + current=0000000000000000000000000000000000000000 > fi > > if test -z "$no_edit" > diff --git a/git-tag.sh b/git-tag.sh > index a0afa25..2bde3c0 100755 > --- a/git-tag.sh > +++ b/git-tag.sh > @@ -63,8 +63,11 @@ done > > name="$1" > [ "$name" ] || usage > -if [ -e "$GIT_DIR/refs/tags/$name" -a -z "$force" ]; then > - die "tag '$name' already exists" > +prev=0000000000000000000000000000000000000000 It seems a little odd to need to use such a large 'none' thing. Will linus' updates start returning this when there is no tag? If so then it makes sense. Else perhaps it would be nice to have a short cut for it. Such as 'none'. -apw ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] update a few Porcelain-ish for ref lock safety. 2006-09-26 18:08 ` Andy Whitcroft @ 2006-09-27 7:25 ` Junio C Hamano 0 siblings, 0 replies; 19+ messages in thread From: Junio C Hamano @ 2006-09-27 7:25 UTC (permalink / raw) To: Andy Whitcroft; +Cc: git Andy Whitcroft <apw@shadowen.org> writes: >> +prev=0000000000000000000000000000000000000000 > > It seems a little odd to need to use such a large 'none' thing. Will > linus' updates start returning this when there is no tag? If so then it > makes sense. Else perhaps it would be nice to have a short cut for it. > Such as 'none'. True. It probably is better to accept something shorter, like an empty string, or token "none". But this is a plumbing command, and I was not too concerned about having to say 40 "0" in the Porcelain scripts. ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2006-09-27 7:25 UTC | newest] Thread overview: 19+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2006-09-18 4:54 [PATCH] Remove branch by putting a null sha1 into the ref file Christian Couder 2006-09-18 5:47 ` Junio C Hamano 2006-09-18 16:31 ` Linus Torvalds 2006-09-18 18:43 ` Junio C Hamano 2006-09-20 2:56 ` Christian Couder 2006-09-22 22:09 ` Junio C Hamano 2006-09-23 4:45 ` Christian Couder 2006-09-23 8:04 ` Junio C Hamano 2006-09-23 11:22 ` Christian Couder 2006-09-23 21:49 ` Junio C Hamano 2006-09-24 4:45 ` Christian Couder 2006-09-25 9:26 ` On ref locking Junio C Hamano 2006-09-25 17:05 ` Daniel Barkalow 2006-09-26 4:11 ` Junio C Hamano 2006-09-26 4:13 ` [PATCH 1/3] Clean-up lock-ref implementation Junio C Hamano 2006-09-26 4:13 ` [PATCH 2/3] update-ref: -d flag and ref creation safety Junio C Hamano 2006-09-26 18:05 ` [PATCH 3/3] update a few Porcelain-ish for ref lock safety Junio C Hamano 2006-09-26 18:08 ` Andy Whitcroft 2006-09-27 7:25 ` Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).