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

[David Turner <novalis@novalis.org>]

LGTM.  

(This is the current best address to reach me, but do not expect fast
responses over the next few days as I'm out of town)



On Sun, 2018-05-06 at 15:35 +0200, Martin Ågren wrote:
> 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>
> ---
> (David's twopensource-address bounced, so I'm trying instead the one
> he
> most recently posted from.)
> 
>  t/t1400-update-ref.sh |  7 +++++++
>  refs.c                | 19 +++++++++++++++----
>  2 files changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> index 664a3a4e4e..bd41f86f22 100755
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -457,6 +457,13 @@ test_expect_success 'git cat-file blob
> master@{2005-05-26 23:42}:F (expect OTHER
>  	test OTHER = $(git cat-file blob "master@{2005-05-26
> 23:42}:F")
>  '
>  
> +test_expect_success 'create pseudoref with old oid null, but do not
> overwrite' '
> +	git update-ref PSEUDOREF $A $Z &&
> +	test_when_finished "git update-ref -d PSEUDOREF" &&
> +	test $A = $(cat .git/PSEUDOREF) &&
> +	test_must_fail git update-ref PSEUDOREF $A $Z
> +'
> +
>  a=refs/heads/a
>  b=refs/heads/b
>  c=refs/heads/c
> diff --git a/refs.c b/refs.c
> index 8b7a77fe5e..3669190499 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -666,10 +666,21 @@ static int write_pseudoref(const char
> *pseudoref, const struct object_id *oid,
>  	if (old_oid) {
>  		struct object_id actual_old_oid;
>  
> -		if (read_ref(pseudoref, &actual_old_oid))
> -			die("could not read ref '%s'", pseudoref);
> -		if (oidcmp(&actual_old_oid, old_oid)) {
> -			strbuf_addf(err, "unexpected sha1 when
> writing '%s'", pseudoref);
> +		if (read_ref(pseudoref, &actual_old_oid)) {
> +			if (!is_null_oid(old_oid)) {
> +				strbuf_addf(err, "could not read ref
> '%s'",
> +					    pseudoref);
> +				rollback_lock_file(&lock);
> +				goto done;
> +			}
> +		} else if (is_null_oid(old_oid)) {
> +			strbuf_addf(err, "ref '%s' already exists",
> +				    pseudoref);
> +			rollback_lock_file(&lock);
> +			goto done;
> +		} else if (oidcmp(&actual_old_oid, old_oid)) {
> +			strbuf_addf(err, "unexpected sha1 when
> writing '%s'",
> +				    pseudoref);
>  			rollback_lock_file(&lock);
>  			goto done;
>  		}