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

From: Jeff King <peff@peff.net>
Date: Feb 11, 2015, 00:05 UTC
Message-ID: <20150211000520.GA30561@peff.net>
In-Reply-To: <CAGZ79kbrFpgF6_dLYdgT2D0JjWggu8edjV2sgXER5btpmyjDNw@mail.gmail.com>

On Tue, Feb 10, 2015 at 03:24:47PM -0800, Stefan Beller wrote:

Show 11 quoted lines

> 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