git update-ref fails to create reference. (bug)

While trying to create a pseudo reference named REF pointing to the
empty tree iff it doesn't exist, I stumbled on the following:

I assume both are valid ways to create such reference:
 	a) $ echo -e option no-deref\\nupdate REF $(git hash-object -t tree /dev/null) 0000000000000000000000000000000000000000 | git update-ref --stdin
 	b) $ git update-ref --no-deref REF $(git hash-object -t tree /dev/null) 0000000000000000000000000000000000000000

While a) works, b) will throw:
	fatal: could not read ref 'REF'

Bisect seems to point to:
2c3aed138 (pseudoref: check return values from read_ref(), 2015-07-15)

Thanks,
Rafael Ascensão

Hi Rafael,

On 4 May 2018 at 18:28, Rafael Ascensão [off-list ref] wrote:

While trying to create a pseudo reference named REF pointing to the
empty tree iff it doesn't exist, I stumbled on the following:

I assume both are valid ways to create such reference:
        a) $ echo -e option no-deref\\nupdate REF $(git hash-object -t
        tree /dev/null) 0000000000000000000000000000000000000000 | git
        update-ref --stdin
        b) $ git update-ref --no-deref REF $(git hash-object -t tree
        /dev/null) 0000000000000000000000000000000000000000

While a) works, b) will throw:
        fatal: could not read ref 'REF'

I can reproduce this and I agree with your understanding of what should
happen here. The patch below makes this work according to my and your
expectations, at least in my command-line testing.

The die("... already exists") could instead be a no-op, trusting that
the backend discovers the problem. "die" could also be strbuf_addf(...),
I'm just following 2c3aed138 here.

Anyway, that's not where I'm stuck... Regardless of how I try to write
tests (in t1400), they just pass beautifully even before this patch. I
might be able to look into that more on the weekend. If anyone has
ideas, I am all ears. Or if someone feels like picking this up and
running with it, feel free.

Martin

diff --git a/refs.c b/refs.c
index 8b7a77fe5e..cdb0a5ab29 100644
--- a/refs.c
+++ b/refs.c

@@ -666,9 +666,12 @@ 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)) {
+		if (read_ref(pseudoref, &actual_old_oid)) {
+			if (!is_null_oid(old_oid))
+				die("could not read ref '%s'", pseudoref);
+		} else if (is_null_oid(old_oid)) {
+			die("reference '%s' already exists", pseudoref);
+		} else if (oidcmp(&actual_old_oid, old_oid)) {
 			strbuf_addf(err, "unexpected sha1 when writing '%s'", pseudoref);
 			rollback_lock_file(&lock);
 			goto done;

-- 
2.17.0.392.g7fa371e468

Thanks Martin for the quick fix.

On Fri, May 04, 2018 at 08:26:46PM +0200, Martin �gren wrote:

Anyway, that's not where I'm stuck... Regardless of how I try to write
tests (in t1400), they just pass beautifully even before this patch. I
might be able to look into that more on the weekend. If anyone has
ideas, I am all ears. Or if someone feels like picking this up and
running with it, feel free.

In t1400 `m=refs/heads/master` is used in the majority of tests. And
this issue doesn't manifest itself if refs are being written under refs/

It also seems particular about having the "old sha" set to 40 zeros or
the empty string.

So I guess we should add some extra tests to cover the variations of
these two cases.

e.g.
     test_expect_success "create PSEUDOREF" '
         git update-ref PSEUDOREF $A
0000000000000000000000000000000000000000 &&
         test $A = $(cat .git/PSEUDOREF)
     '

fails/succeeds appropriately in my limited testing.

I am busy this weekend, but can try to write some if no one writes it
until after the weekend.

Cumprimentos,
Rafael Ascensão

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 <redacted>
Helped-by: Rafael Ascensão [off-list ref]
Signed-off-by: Martin Ågren <redacted>
---
(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;
 		}

-- 
2.17.0.411.g9fd64c8e46

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:

On 05/06/2018 03:35 PM, 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 <redacted>
Helped-by: Rafael Ascensão [off-list ref]
Signed-off-by: Martin Ågren <redacted>

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

On 7 May 2018 at 09:39, Michael Haggerty [off-list ref] wrote:

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.

Thank you for your comments. I was not able to find much
pseudoref-testing. I think what I should do is a patch 1/2 adding the
tests you outlined (some will be expected failures), then turn this
patch into a patch 2/2.

Martin

On 7 May 2018 at 12:05, Martin Ågren [off-list ref] wrote:

On 7 May 2018 at 09:39, Michael Haggerty [off-list ref] wrote:

quoted

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:

Thank you for your comments. I was not able to find much
pseudoref-testing. I think what I should do is a patch 1/2 adding the
tests you outlined (some will be expected failures), then turn this
patch into a patch 2/2.

Well, it turned into three patches. One to move away from "sha1" in some
error messages before spreading them to the test suite, then one to add
the tests, then one for the actual fix.

Martin

Martin Ågren (3):
  refs.c: refer to "object ID", not "sha1", in error messages
  t1400: add tests around adding/deleting pseudorefs
  refs: handle zero oid for pseudorefs

 t/t1400-update-ref.sh | 60 +++++++++++++++++++++++++++++++++++++++++++
 refs.c                | 22 ++++++++++++----
 2 files changed, 77 insertions(+), 5 deletions(-)

-- 
2.17.0

We have two error messages that complain about the "sha1". Because we
are about to touch one of these sites and add some tests, let's first
modernize the messages to say "object ID" instead.

While at it, make the second one use `error()` instead of `warning()`.
After printing the message, we do not continue, but actually drop the
lock and return -1 without deleting the pseudoref.

Signed-off-by: Martin Ågren <redacted>
---
We could make error-reporting more consistent in general in this file,
but I'd rather not lose track of the original goal of this series.

 refs.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index 64aadd14c9..7820a52c4f 100644
--- a/refs.c
+++ b/refs.c

@@ -684,7 +684,8 @@ static int write_pseudoref(const char *pseudoref, const struct object_id *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);
+			strbuf_addf(err, "unexpected object ID when writing '%s'",
+				    pseudoref);
 			rollback_lock_file(&lock);
 			goto done;
 		}

@@ -722,7 +723,8 @@ static int delete_pseudoref(const char *pseudoref, const struct object_id *old_o
 		if (read_ref(pseudoref, &actual_old_oid))
 			die("could not read ref '%s'", pseudoref);
 		if (oidcmp(&actual_old_oid, old_oid)) {
-			warning("Unexpected sha1 when deleting %s", pseudoref);
+			error("unexpected object ID when deleting '%s'",
+			      pseudoref);
 			rollback_lock_file(&lock);
 			return -1;
 		}

-- 
2.17.0

I have not been able to find any tests around adding pseudorefs using
`git update-ref`. Add some as outlined in this table (original design by
Michael Haggerty; modified and extended by me):

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

The tests marked with a * currently fail, despite git-update-ref(1)
claiming that 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."
These failing tests will be fixed in the next commit.

It is only natural to test deletion as well. Test deletion without an
old OID, with a correct one and with an incorrect one.

Suggested-by: Michael Haggerty <redacted>
Signed-off-by: Martin Ågren <redacted>
---
 t/t1400-update-ref.sh | 60 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 664a3a4e4e..3996109ba4 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh

@@ -457,6 +457,66 @@ 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 adding and deleting pseudorefs
+
+test_expect_success 'given old value for missing pseudoref, do not create' '
+	test_must_fail git update-ref PSEUDOREF $A $B 2>err &&
+	test_path_is_missing .git/PSEUDOREF &&
+	grep "could not read ref" err
+'
+
+test_expect_success 'create pseudoref' '
+	git update-ref PSEUDOREF $A &&
+	test $A = $(cat .git/PSEUDOREF)
+'
+
+test_expect_success 'overwrite pseudoref with no old value given' '
+	git update-ref PSEUDOREF $B &&
+	test $B = $(cat .git/PSEUDOREF)
+'
+
+test_expect_success 'overwrite pseudoref with correct old value' '
+	git update-ref PSEUDOREF $C $B &&
+	test $C = $(cat .git/PSEUDOREF)
+'
+
+test_expect_success 'do not overwrite pseudoref with wrong old value' '
+	test_must_fail git update-ref PSEUDOREF $D $E 2>err &&
+	test $C = $(cat .git/PSEUDOREF) &&
+	grep "unexpected object ID" err
+'
+
+test_expect_success 'delete pseudoref' '
+	git update-ref -d PSEUDOREF &&
+	test_path_is_missing .git/PSEUDOREF
+'
+
+test_expect_success 'do not delete pseudoref with wrong old value' '
+	git update-ref PSEUDOREF $A &&
+	test_must_fail git update-ref -d PSEUDOREF $B 2>err &&
+	test $A = $(cat .git/PSEUDOREF) &&
+	grep "unexpected object ID" err
+'
+
+test_expect_success 'delete pseudoref with correct old value' '
+	git update-ref -d PSEUDOREF $A &&
+	test_path_is_missing .git/PSEUDOREF
+'
+
+test_expect_failure 'create pseudoref with old OID zero' '
+	git update-ref PSEUDOREF $A $Z &&
+	test $A = $(cat .git/PSEUDOREF)
+'
+
+test_expect_failure 'do not overwrite pseudoref with old OID zero' '
+	test_when_finished git update-ref -d PSEUDOREF &&
+	test_must_fail git update-ref PSEUDOREF $B $Z 2>err &&
+	test $A = $(cat .git/PSEUDOREF) &&
+	grep "already exists" err
+'
+
+# Test --stdin
+
 a=refs/heads/a
 b=refs/heads/b
 c=refs/heads/c

-- 
2.17.0

According to the documentation, 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, as demonstrated by the failing tests added in the previous commit.
If we fail to read the old ref, we immediately die. But a failure to
read would actually be a good thing if we have been given the zero oid.

With the zero 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 and fixes both failing tests from the previous commit.

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 <redacted>
Helped-by: Rafael Ascensão [off-list ref]
Signed-off-by: Martin Ågren <redacted>
---
 t/t1400-update-ref.sh |  4 ++--
 refs.c                | 16 +++++++++++++---
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 3996109ba4..faf0dfe993 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh

@@ -503,12 +503,12 @@ test_expect_success 'delete pseudoref with correct old value' '
 	test_path_is_missing .git/PSEUDOREF
 '
 
-test_expect_failure 'create pseudoref with old OID zero' '
+test_expect_success 'create pseudoref with old OID zero' '
 	git update-ref PSEUDOREF $A $Z &&
 	test $A = $(cat .git/PSEUDOREF)
 '
 
-test_expect_failure 'do not overwrite pseudoref with old OID zero' '
+test_expect_success 'do not overwrite pseudoref with old OID zero' '
 	test_when_finished git update-ref -d PSEUDOREF &&
 	test_must_fail git update-ref PSEUDOREF $B $Z 2>err &&
 	test $A = $(cat .git/PSEUDOREF) &&

diff --git a/refs.c b/refs.c
index 7820a52c4f..26af07fc51 100644
--- a/refs.c
+++ b/refs.c

@@ -681,9 +681,19 @@ 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)) {
+		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 object ID when writing '%s'",
 				    pseudoref);
 			rollback_lock_file(&lock);

-- 
2.17.0