On 05/17/2017 07:18 PM, Stefan Beller wrote:
> On Wed, May 17, 2017 at 6:17 AM, Jeff King <peff@xxxxxxxx> 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 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");
Hmmm, I don't see what's wrong with the current error message (except
for s/internal error/BUG/). In any case, this is meant to be a sanity
check that users should never see, so I wouldn't worry too much about it
either way.
Michael