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 is

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 are

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. True. As

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 writing

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 care

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 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 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 for

Re: another packed-refs race

2013-05-03 Thread Johan Herland
On Fri, May 3, 2013 at 10:38 AM, Jeff King p...@peff.net 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

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 always

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, load all loose

Re: another packed-refs race

2013-05-03 Thread Johan Herland
On Fri, May 3, 2013 at 8:26 PM, Jeff King p...@peff.net 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 for

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 git commit