Re: [PATCH 10/23] files_ref_store: put the packed files lock directly in this struct

[Brandon Williams <bmwill@google.com>]

On 05/17, Stefan Beller wrote:
> On Wed, May 17, 2017 at 6:17 AM, Jeff King <peff@peff.net> wrote:
> > On Wed, May 17, 2017 at 02:05:33PM +0200, Michael Haggerty wrote:
> >
> >> Instead of using a global `lock_file` instance for the main
> >> "packed-refs" file and using a pointer in `files_ref_store` to keep
> >> track of whether it is locked, embed the `lock_file` instance directly
> >> in the `files_ref_store` struct and use the new
> >> `is_lock_file_locked()` function to keep track of whether it is
> >> locked. This keeps related data together and makes the main reference
> >> store less of a special case.
> >
> > This made me wonder how we handle the locking for ref_stores besides the
> > main one (e.g., for submodules). The lockfile structs have to remain
> > valid for the length of the program. Previously those stores could have
> > xcalloc()'d a lockfile and just leaked it. Now they'll need to xcalloc()
> > and leak their whole structs.
> 
> +cc Brandon, who is eager to go down that road.

I'm probably too eager haha.  But I still think its something to slowly
work towards.

> 
> > I suspect the answer is "we don't ever lock anything except the main ref
> > store because that is the only one we write to", so it doesn't matter
> > anyway.
> >
> > -Peff
> 
> 
> > @@ -102,7 +98,7 @@ static void clear_packed_ref_cache(struct files_ref_store *refs)
> >        if (refs->packed) {
> >                struct packed_ref_cache *packed_refs = refs->packed;
> >
> > -               if (refs->packlock)
> > +               if (is_lock_file_locked(&refs->packlock))
> >                        die("internal error: packed-ref cache cleared while locked");
> 
> I think the error message needs adjustment here as well? Maybe
> 
>      die("internal error: packed refs locked in cleanup");
> 
> Thanks,
> Stefan

-- 
Brandon Williams