[PATCH] Remove branch by putting a null sha1 into the ref file.

[PATCH] Remove branch by putting a null sha1 into the ref file.

[Christian Couder]

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

Re: [PATCH] Remove branch by putting a null sha1 into the ref file.

[Junio C Hamano]

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 ;-).

Re: [PATCH] Remove branch by putting a null sha1 into the ref file.

[Linus Torvalds]

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

Re: [PATCH] Remove branch by putting a null sha1 into the ref file.

[Junio C Hamano]

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 ,-).

Re: [PATCH] Remove branch by putting a null sha1 into the ref file.

[Christian Couder]

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.

Re: [PATCH] Remove branch by putting a null sha1 into the ref file.

[Junio C Hamano]

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

Re: [PATCH] Remove branch by putting a null sha1 into the ref file.

[Christian Couder]

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.

Re: [PATCH] Remove branch by putting a null sha1 into the ref file.

[Junio C Hamano]

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.

Re: [PATCH] Remove branch by putting a null sha1 into the ref file.

[Christian Couder]

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. 

Re: [PATCH] Remove branch by putting a null sha1 into the ref file.

[Junio C Hamano]

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

Re: [PATCH] Remove branch by putting a null sha1 into the ref file.

[Christian Couder]

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.

On ref locking

[Junio C Hamano]

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.

Re: On ref locking

[Daniel Barkalow]

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*

Re: On ref locking

[Junio C Hamano]

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.

[PATCH 1/3] Clean-up lock-ref implementation

[Junio C Hamano]

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

[PATCH 2/3] update-ref: -d flag and ref creation safety.

[Junio C Hamano]

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

[PATCH 3/3] update a few Porcelain-ish for ref lock safety.

[Junio C Hamano]

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

Re: [PATCH 3/3] update a few Porcelain-ish for ref lock safety.

[Andy Whitcroft]

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

Re: [PATCH 3/3] update a few Porcelain-ish for ref lock safety.

[Junio C Hamano]

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.