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@xxxxxxxxx>
> Helped-by: Rafael Ascensão <rafa.almas@xxxxxxxxx>
> Signed-off-by: Martin Ågren <martin.agren@xxxxxxxxx>
> ---
> (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;
> }
