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 <rafa.almas@gmail.com> wrote:

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

Thanks Martin for the quick fix.

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

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 <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(-)

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:

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 <mhagger@alum.mit.edu> wrote:

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 <martin.agren@gmail.com> wrote:

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(-)

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 <martin.agren@gmail.com>
---
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(-)

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 <mhagger@alum.mit.edu>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 t/t1400-update-ref.sh | 60 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

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 <rafa.almas@gmail.com>
Helped-by: Rafael Ascensão <rafa.almas@gmail.com>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 t/t1400-update-ref.sh |  4 ++--
 refs.c                | 16 +++++++++++++---
 2 files changed, 15 insertions(+), 5 deletions(-)