From c0bdd658bd2f06dc1c0d8fd578e80831f8d53b43 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Thu, 10 May 2018 21:29:54 +0200 Subject: [PATCH 1/3] refs.c: refer to "object ID", not "sha1", in error messages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Junio C Hamano --- 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; } From 65eb8fc344205a4039b989f07367f345101bbf28 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Thu, 10 May 2018 21:29:55 +0200 Subject: [PATCH 2/3] t1400: add tests around adding/deleting pseudorefs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 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 Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- 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 From db0210d445963e5d85f98e48d6a93b971779d449 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Thu, 10 May 2018 21:29:56 +0200 Subject: [PATCH 3/3] refs: handle zero oid for pseudorefs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit According to the documentation, it is possible to "specify 40 '0' or an empty string as 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 Helped-by: Rafael Ascensão Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- refs.c | 16 +++++++++++++--- t/t1400-update-ref.sh | 4 ++-- 2 files changed, 15 insertions(+), 5 deletions(-) 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); 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) &&