From: Junio C Hamano <gitster@pobox.com>
Date: Thu, 12 Feb 2015 17:32:11 GMT
Subject: Re: [PATCH v2 04/12] ref_transaction_update(): remove "have_old" parameter
Message-ID: <CAPc5daX80Mk2cRNAijTckHZ-EFJhiF4GHWMxBK4wYJYSwaEe0w@mail.gmail.com>
In-Reply-To: <1423739543-1025-5-git-send-email-mhagger@alum.mit.edu>

On Thu, Feb 12, 2015 at 3:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> Instead, verify the reference's old value if and only if old_sha1 is
> non-NULL.
>
>...
> @@ -3664,9 +3664,6 @@ int ref_transaction_update(struct ref_transaction *transaction,
>         if (transaction->state != REF_TRANSACTION_OPEN)
>                 die("BUG: update called for transaction that is not open");
>
> -       if (have_old && !old_sha1)
> -               die("BUG: have_old is true but old_sha1 is NULL");
> -

In the old world, old_sha1 here used to be one of
 (1) NULL, (2) null_sha1[], or (3) a real object name.
What is the rule in the new world?

Does it still allow NULL, and if so what does NULL mean?
The same thing as null_sha1[] or something else?

Or is it an error to pass NULL to this function after this change
(which I think is a very sensible rule, by the way)?

If the former, perhaps "if (!old_sha1) old_sha1 = null_sha1;"
here might be a prudent measure to simplify the code.

If the latter, "assert(old_sha1);" may be appropriate.

>         if (!is_null_sha1(new_sha1) &&
>             check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
>                 strbuf_addf(err, "refusing to update ref with bad name %s",