Re: [PATCH 3/8] lock_ref_sha1_basic(): do not set force_write for missing references

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

On 02/11/2015 01:05 AM, Jeff King wrote:
> On Tue, Feb 10, 2015 at 03:24:47PM -0800, Stefan Beller wrote:
> 
>> On Mon, Feb 9, 2015 at 1:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>>> If a reference is missing, its SHA-1 will be null_sha1, which can't
>>> possibly match a new value that ref_transaction_commit() is trying to
>>> update it to. So there is no need to set force_write in this scenario.
>>>
>>
>> This commit reverts half the lines of 5bdd8d4a3062a (2008-11, do not
>> force write of packed refs). And reading both commit messages, they
>> seem to contradict each other. (Both agree on  "If a reference is
>> missing, its SHA-1 will be null_sha1 as provided by resolve_ref", but
>> the conclusion seems to be different.)
> 
> Most of the lines of 5bdd8d4a3062a that are being reverted here are
> caching the is_null_sha1() check in the "missing" variable. And that's
> a cleanup in this patch that is not strictly necessary ("missing" would
> only be used once, so it becomes noise).
> 
> The interesting thing in the earlier commit was to use the null sha1 to
> cause a force-write, rather than lstat()ing the filesystem. And here we
> are saying the force-write is not necessary at all, no matter what
> storage scheme is used. So I don't think there is any contradiction
> between the two.
> 
> Is this patch correct that the force-write is not necessary? I think so.
> The force-write flag comes from:
> 
> commit 732232a123e1e61e38babb1c572722bb8a189ba3
> Author: Shawn Pearce <spearce@spearce.org>
> Date:   Fri May 19 03:29:05 2006 -0400
> 
>     Force writing ref if it doesn't exist.
>     
>     Normally we try to skip writing a ref if its value hasn't changed
>     but in the special case that the ref doesn't exist but the new
>     value is going to be 0{40} then force writing the ref anyway.
> 
> but I am not sure that logic still holds (if it ever did). We do not ever write
> 0{40} into a ref value.

I don't understand that old commit, either. Maybe there was an idea of
storing 0{40} in a loose ref file to mark a packed reference as deleted?
CC to Shawn Pearce in case he can shed some light on the situation.

I still think that my change is OK, because we definitely don't want to
write 0{40} to any loose reference file. The reference-reading code
can't deal with it, so this would break the repository.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu