Re: another packed-refs race

2013-05-07 Thread Michael Haggerty
On 05/07/2013 06:44 AM, Jeff King wrote: > On Tue, May 07, 2013 at 06:32:12AM +0200, Michael Haggerty wrote: > >> Another potential problem caused by the non-atomicity of loose reference >> reading could be to confuse reachability tests if process 1 is reading >> loose references while process 2 i

Re: another packed-refs race

2013-05-06 Thread Jeff King
On Tue, May 07, 2013 at 06:32:12AM +0200, Michael Haggerty wrote: > Another potential problem caused by the non-atomicity of loose reference > reading could be to confuse reachability tests if process 1 is reading > loose references while process 2 is renaming a reference: > > 1. Process 1 looks

Re: another packed-refs race

2013-05-06 Thread Michael Haggerty
On 05/06/2013 08:41 PM, Jeff King wrote: > On Mon, May 06, 2013 at 02:03:40PM +0200, Michael Haggerty wrote: > [...] >> The loose refs cache is only used by the for_each_ref() functions, not >> for looking up individual references. Another approach would be to >> change the top-level for_each_ref(

Re: another packed-refs race

2013-05-06 Thread Jeff King
On Mon, May 06, 2013 at 02:41:22PM -0400, Jeff King wrote: > That is a weaker guarantee, and I think we can provide it with: > > 1. Load all loose refs into cache for a particular enumeration. > > 2. Make sure the packed-refs cache is up-to-date (by checking its > stat() information and

Re: another packed-refs race

2013-05-06 Thread Jeff King
On Mon, May 06, 2013 at 02:12:29PM +0200, Michael Haggerty wrote: > > I think that would be correct (modulo that step 1 cannot happen for > > enumeration). But we would like to avoid loading all loose refs if we > > can. Especially on a cold cache, it can be quite slow, and you may not > > even ca

Re: another packed-refs race

2013-05-06 Thread Jeff King
On Mon, May 06, 2013 at 02:03:40PM +0200, Michael Haggerty wrote: > > We could fix this by making sure our packed-refs file is up to date > > s/file/cache/ Yeah, I meant "our view of the packed-refs file", but I think "cache" says that more succinctly. I'll be sure to make it clear when I start

Re: another packed-refs race

2013-05-06 Thread Michael Haggerty
On 05/03/2013 07:28 PM, Jeff King wrote: > On Fri, May 03, 2013 at 11:26:11AM +0200, Johan Herland wrote: > >> You don't really need to be sure that packed-refs is up-to-date. You >> only need to make sure that don't rely on lazily loading loose refs >> _after_ you have loaded packed-refs. > > Tr

Re: another packed-refs race

2013-05-06 Thread Michael Haggerty
On 05/03/2013 10:38 AM, Jeff King wrote: > I found another race related to the packed-refs code. Consider for a > moment what happens when we are looking at refs and another process does > a simultaneous "git pack-refs --all --prune", updating packed-refs and > deleting the loose refs. > > If we a

Re: another packed-refs race

2013-05-03 Thread Jeff King
On Fri, May 03, 2013 at 04:38:47AM -0400, Jeff King wrote: > For reference, here's a script that demonstrates the problem during > enumeration (sometimes for-each-ref fails to realize that > refs/heads/master exists at all): > > # run this in one terminal > git init repo && > cd repo && >

Re: another packed-refs race

2013-05-03 Thread Johan Herland
On Fri, May 3, 2013 at 8:26 PM, Jeff King wrote: > On Fri, May 03, 2013 at 01:28:53PM -0400, Jeff King wrote: >> > The following solution might work in both the resolve-a-single-ref and >> > enumerating-refs case: >> > >> > 0. Look for ref already cached in memory. If found, OK. >> > >> > 1. Look

Re: another packed-refs race

2013-05-03 Thread Jeff King
On Fri, May 03, 2013 at 01:28:53PM -0400, Jeff King wrote: > > The following solution might work in both the resolve-a-single-ref and > > enumerating-refs case: > > > > 0. Look for ref already cached in memory. If found, OK. > > > > 1. Look for loose ref. If found, OK. > > > > 2. If not found,

Re: another packed-refs race

2013-05-03 Thread Jeff King
On Fri, May 03, 2013 at 11:26:11AM +0200, Johan Herland wrote: > You don't really need to be sure that packed-refs is up-to-date. You > only need to make sure that don't rely on lazily loading loose refs > _after_ you have loaded packed-refs. True. As long as you load them both together, and alwa

Re: another packed-refs race

2013-05-03 Thread Johan Herland
On Fri, May 3, 2013 at 10:38 AM, Jeff King wrote: > I found another race related to the packed-refs code. Consider for a > moment what happens when we are looking at refs and another process does > a simultaneous "git pack-refs --all --prune", updating packed-refs and > deleting the loose refs. >