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

> 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.
>
> -Peff