Re: [PATCH] refs: handle null-oid for pseudorefs

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: May 7, 2018, 07:39 UTC
Message-ID: <1291dbc0-31f9-213a-ae30-1a6349ea1626@alum.mit.edu>
In-Reply-To: <20180506133549.8536-1-martin.agren@gmail.com>

On 05/06/2018 03:35 PM, Martin Ågren wrote:

Show 17 quoted lines

> According to the documentation on `git update-ref`, it is possible to
> "specify 40 '0' or an empty string as <oldvalue> to make sure that the
> ref you are creating does not exist." But in the code for pseudorefs, we
> do not implement this. If we fail to read the old ref, we immediately
> die. A failure to read would actually be a good thing if we have been
> given the null-oid.
> 
> With the null-oid, allow -- and even require -- the ref-reading to fail.
> This implements the "make sure that the ref ... does not exist" part of
> the documentation.
> 
> Since we have a `strbuf err` for collecting errors, let's use it and
> signal an error to the caller instead of dying hard.
> 
> Reported-by: Rafael Ascensão <rafa.almas@gmail.com>
> Helped-by: Rafael Ascensão <rafa.almas@gmail.com>
> Signed-off-by: Martin Ågren <martin.agren@gmail.com>

Thanks for the patch. This looks good to me. But it it seems that the test coverage related to pseudorefs is still not great. Ideally, all of the following combinations should be tested:

Pre-update value | ref-update old OID | Expected result -------------------|----------------------|---------------- missing | missing | accept * missing | value | reject set | missing | reject * set | correct value | accept set | wrong value | reject

I think your test only covers the lines with asterisks. Are the other scenarios already covered by other tests? If not, how about adding them? That would give us confidence that the new code works in all circumstances.

Michael