Re: What's cooking in git.git (Mar 2014, #07; Fri, 28)

[Michael Haggerty <mhagger@alum.mit.edu>]

On 03/31/2014 07:56 PM, Ronnie Sahlberg wrote:
> I am new to git, so sorry If I overlooked something.
> 
> I think there might be a race in ref_transaction_commit() when
> deleting references.
> 
> 	/* Perform deletes now that updates are safely completed */
> 	for (i = 0; i < n; i++) {
> 		struct ref_update *update = updates[i];
> 
> 		if (update->lock) {
> 			delnames[delnum++] = update->lock->ref_name;
> 			ret |= delete_ref_loose(update->lock, update->type);
> 		}
> 	}
> 
> 	ret |= repack_without_refs(delnames, delnum);
> 	for (i = 0; i < delnum; i++)
> 		unlink_or_warn(git_path("logs/%s", delnames[i]));
> 
> These two blocks should be reordered so that you first delete the
> actual refs first, while holding the lock and then release the lock
> afterward ?

I think what you suggest is what is already being done.  The locks of
references that are being deleted are not released until a few lines
after the code that you quoted:

> 	for (i = 0; i < n; i++)
> 		if (updates[i]->lock)
> 			unlock_ref(updates[i]->lock);

Before the code that you quoted, some locks are released, but only for
references being updated (not those being deleted).

But maybe I misunderstand your critique.

By the way, there *is* a race here, but it is a subtler one involving
the interaction between packed and loose references when references are
deleted.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/