{"thread":{"id":"48410","subject":"git update-ref fails to create reference. (bug)","startedAt":"2018-05-04T16:28:19Z","lastAt":"2018-05-10T19:30:27Z","messageCount":11,"participants":["Rafael Ascensão","Martin Ågren","David Turner","Michael Haggerty"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"346681","messageId":"cced9e2c-7d02-47fd-109a-1185eed13e63@gmail.com","threadId":"48410","inReplyTo":null,"subject":"git update-ref fails to create reference. (bug)","fromName":"Rafael Ascensão","fromEmail":"rafa.almas@gmail.com","sentAt":"2018-05-04T16:28:12Z","receivedAt":"2018-05-04T16:28:19Z","isPatch":false,"sender":{"key":"rafa.almas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1923789?v=4"},"body":"While trying to create a pseudo reference named REF pointing to the\nempty tree iff it doesn't exist, I stumbled on the following:\n\nI assume both are valid ways to create such reference:\n \ta) $ echo -e option no-deref\\\\nupdate REF $(git hash-object -t tree /dev/null) 0000000000000000000000000000000000000000 | git update-ref --stdin\n \tb) $ git update-ref --no-deref REF $(git hash-object -t tree /dev/null) 0000000000000000000000000000000000000000\n\nWhile a) works, b) will throw:\n\tfatal: could not read ref 'REF'\n\nBisect seems to point to:\n2c3aed138 (pseudoref: check return values from read_ref(), 2015-07-15)\n\nThanks,\nRafael Ascensão\n"},{"id":"346687","messageId":"20180504182646.7738-1-martin.agren@gmail.com","threadId":"48410","inReplyTo":"cced9e2c-7d02-47fd-109a-1185eed13e63@gmail.com","subject":"Re: git update-ref fails to create reference. (bug)","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-05-04T18:26:46Z","receivedAt":"2018-05-04T18:27:07Z","isPatch":false,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Hi Rafael,\n\nOn 4 May 2018 at 18:28, Rafael Ascensão wrote:\n> While trying to create a pseudo reference named REF pointing to the\n> empty tree iff it doesn't exist, I stumbled on the following:\n>\n> I assume both are valid ways to create such reference:\n> a) $ echo -e option no-deref\\\\nupdate REF $(git hash-object -t\n> tree /dev/null) 0000000000000000000000000000000000000000 | git\n> update-ref --stdin\n> b) $ git update-ref --no-deref REF $(git hash-object -t tree\n> /dev/null) 0000000000000000000000000000000000000000\n>\n> While a) works, b) will throw:\n> fatal: could not read ref 'REF'\n\n\nI can reproduce this and I agree with your understanding of what should\nhappen here. The patch below makes this work according to my and your\nexpectations, at least in my command-line testing.\n\nThe die(\"... already exists\") could instead be a no-op, trusting that\nthe backend discovers the problem. \"die\" could also be strbuf_addf(...),\nI'm just following 2c3aed138 here.\n\nAnyway, that's not where I'm stuck... Regardless of how I try to write\ntests (in t1400), they just pass beautifully even before this patch. I\nmight be able to look into that more on the weekend. If anyone has\nideas, I am all ears. Or if someone feels like picking this up and\nrunning with it, feel free.\n\nMartin\n\ndiff --git a/refs.c b/refs.c\nindex 8b7a77fe5e..cdb0a5ab29 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -666,9 +666,12 @@ static int write_pseudoref(const char *pseudoref, const struct object_id *oid,\n \tif (old_oid) {\n \t\tstruct object_id actual_old_oid;\n \n-\t\tif (read_ref(pseudoref, &actual_old_oid))\n-\t\t\tdie(\"could not read ref '%s'\", pseudoref);\n-\t\tif (oidcmp(&actual_old_oid, old_oid)) {\n+\t\tif (read_ref(pseudoref, &actual_old_oid)) {\n+\t\t\tif (!is_null_oid(old_oid))\n+\t\t\t\tdie(\"could not read ref '%s'\", pseudoref);\n+\t\t} else if (is_null_oid(old_oid)) {\n+\t\t\tdie(\"reference '%s' already exists\", pseudoref);\n+\t\t} else if (oidcmp(&actual_old_oid, old_oid)) {\n \t\t\tstrbuf_addf(err, \"unexpected sha1 when writing '%s'\", pseudoref);\n \t\t\trollback_lock_file(&lock);\n \t\t\tgoto done;\n-- \n2.17.0.392.g7fa371e468\n\n"},{"id":"346727","messageId":"CACUQV5-9PagVhE5YY=Z3721YRiBwSZykT3ZjtzmD3o-c6O6ddQ@mail.gmail.com","threadId":"48410","inReplyTo":"20180504182646.7738-1-martin.agren@gmail.com","subject":"Re: git update-ref fails to create reference. (bug)","fromName":"Rafael Ascensão","fromEmail":"rafa.almas@gmail.com","sentAt":"2018-05-05T19:08:01Z","receivedAt":"2018-05-05T19:08:41Z","isPatch":false,"sender":{"key":"rafa.almas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1923789?v=4"},"body":"Thanks Martin for the quick fix.\n\nOn Fri, May 04, 2018 at 08:26:46PM +0200, Martin �gren wrote:\n> Anyway, that's not where I'm stuck... Regardless of how I try to write\n> tests (in t1400), they just pass beautifully even before this patch. I\n> might be able to look into that more on the weekend. If anyone has\n> ideas, I am all ears. Or if someone feels like picking this up and\n> running with it, feel free.\n\nIn t1400 `m=refs/heads/master` is used in the majority of tests. And\nthis issue doesn't manifest itself if refs are being written under refs/\n\nIt also seems particular about having the \"old sha\" set to 40 zeros or\nthe empty string.\n\nSo I guess we should add some extra tests to cover the variations of\nthese two cases.\n\ne.g.\n test_expect_success \"create PSEUDOREF\" '\n git update-ref PSEUDOREF $A\n0000000000000000000000000000000000000000 &&\n test $A = $(cat .git/PSEUDOREF)\n '\n\nfails/succeeds appropriately in my limited testing.\n\nI am busy this weekend, but can try to write some if no one writes it\nuntil after the weekend.\n\nCumprimentos,\nRafael Ascensão\n"},{"id":"346760","messageId":"20180506133549.8536-1-martin.agren@gmail.com","threadId":"48410","inReplyTo":"CACUQV5-9PagVhE5YY=Z3721YRiBwSZykT3ZjtzmD3o-c6O6ddQ@mail.gmail.com","subject":"[PATCH] refs: handle null-oid for pseudorefs","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-05-06T13:35:49Z","receivedAt":"2018-05-06T13:36:46Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"According to the documentation on `git update-ref`, it is possible to\n\"specify 40 '0' or an empty string as to make sure that the\nref you are creating does not exist.\" But in the code for pseudorefs, we\ndo not implement this. If we fail to read the old ref, we immediately\ndie. A failure to read would actually be a good thing if we have been\ngiven the null-oid.\n\nWith the null-oid, allow -- and even require -- the ref-reading to fail.\nThis implements the \"make sure that the ref ... does not exist\" part of\nthe documentation.\n\nSince we have a `strbuf err` for collecting errors, let's use it and\nsignal an error to the caller instead of dying hard.\n\nReported-by: Rafael Ascensão \nHelped-by: Rafael Ascensão \nSigned-off-by: Martin Ågren \n---\n(David's twopensource-address bounced, so I'm trying instead the one he\nmost recently posted from.)\n\n t/t1400-update-ref.sh | 7 +++++++\n refs.c | 19 +++++++++++++++----\n 2 files changed, 22 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex 664a3a4e4e..bd41f86f22 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -457,6 +457,13 @@ test_expect_success 'git cat-file blob master@{2005-05-26 23:42}:F (expect OTHER\n \ttest OTHER = $(git cat-file blob \"master@{2005-05-26 23:42}:F\")\n '\n \n+test_expect_success 'create pseudoref with old oid null, but do not overwrite' '\n+\tgit update-ref PSEUDOREF $A $Z &&\n+\ttest_when_finished \"git update-ref -d PSEUDOREF\" &&\n+\ttest $A = $(cat .git/PSEUDOREF) &&\n+\ttest_must_fail git update-ref PSEUDOREF $A $Z\n+'\n+\n a=refs/heads/a\n b=refs/heads/b\n c=refs/heads/c\ndiff --git a/refs.c b/refs.c\nindex 8b7a77fe5e..3669190499 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -666,10 +666,21 @@ static int write_pseudoref(const char *pseudoref, const struct object_id *oid,\n \tif (old_oid) {\n \t\tstruct object_id actual_old_oid;\n \n-\t\tif (read_ref(pseudoref, &actual_old_oid))\n-\t\t\tdie(\"could not read ref '%s'\", pseudoref);\n-\t\tif (oidcmp(&actual_old_oid, old_oid)) {\n-\t\t\tstrbuf_addf(err, \"unexpected sha1 when writing '%s'\", pseudoref);\n+\t\tif (read_ref(pseudoref, &actual_old_oid)) {\n+\t\t\tif (!is_null_oid(old_oid)) {\n+\t\t\t\tstrbuf_addf(err, \"could not read ref '%s'\",\n+\t\t\t\t\t pseudoref);\n+\t\t\t\trollback_lock_file(&lock);\n+\t\t\t\tgoto done;\n+\t\t\t}\n+\t\t} else if (is_null_oid(old_oid)) {\n+\t\t\tstrbuf_addf(err, \"ref '%s' already exists\",\n+\t\t\t\t pseudoref);\n+\t\t\trollback_lock_file(&lock);\n+\t\t\tgoto done;\n+\t\t} else if (oidcmp(&actual_old_oid, old_oid)) {\n+\t\t\tstrbuf_addf(err, \"unexpected sha1 when writing '%s'\",\n+\t\t\t\t pseudoref);\n \t\t\trollback_lock_file(&lock);\n \t\t\tgoto done;\n \t\t}\n-- \n2.17.0.411.g9fd64c8e46\n\n"},{"id":"346770","messageId":"1525621052.16035.4.camel@novalis.org","threadId":"48410","inReplyTo":"20180506133549.8536-1-martin.agren@gmail.com","subject":"Re: [PATCH] refs: handle null-oid for pseudorefs","fromName":"David Turner","fromEmail":"novalis@novalis.org","sentAt":"2018-05-06T15:37:32Z","receivedAt":"2018-05-06T15:37:37Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"LGTM. \n\n(This is the current best address to reach me, but do not expect fast\nresponses over the next few days as I'm out of town)\n\n\n\nOn Sun, 2018-05-06 at 15:35 +0200, Martin Ågren wrote:\n> According to the documentation on `git update-ref`, it is possible to\n> \"specify 40 '0' or an empty string as to make sure that\n> the\n> ref you are creating does not exist.\" But in the code for pseudorefs,\n> we\n> do not implement this. If we fail to read the old ref, we immediately\n> die. A failure to read would actually be a good thing if we have been\n> given the null-oid.\n> \n> With the null-oid, allow -- and even require -- the ref-reading to\n> fail.\n> This implements the \"make sure that the ref ... does not exist\" part\n> of\n> the documentation.\n> \n> Since we have a `strbuf err` for collecting errors, let's use it and\n> signal an error to the caller instead of dying hard.\n> \n> Reported-by: Rafael Ascensão \n> Helped-by: Rafael Ascensão \n> Signed-off-by: Martin Ågren \n> ---\n> (David's twopensource-address bounced, so I'm trying instead the one\n> he\n> most recently posted from.)\n> \n> t/t1400-update-ref.sh | 7 +++++++\n> refs.c | 19 +++++++++++++++----\n> 2 files changed, 22 insertions(+), 4 deletions(-)\n> \n> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\n> index 664a3a4e4e..bd41f86f22 100755\n> --- a/t/t1400-update-ref.sh\n> +++ b/t/t1400-update-ref.sh\n> @@ -457,6 +457,13 @@ test_expect_success 'git cat-file blob\n> master@{2005-05-26 23:42}:F (expect OTHER\n> \ttest OTHER = $(git cat-file blob \"master@{2005-05-26\n> 23:42}:F\")\n> '\n> \n> +test_expect_success 'create pseudoref with old oid null, but do not\n> overwrite' '\n> +\tgit update-ref PSEUDOREF $A $Z &&\n> +\ttest_when_finished \"git update-ref -d PSEUDOREF\" &&\n> +\ttest $A = $(cat .git/PSEUDOREF) &&\n> +\ttest_must_fail git update-ref PSEUDOREF $A $Z\n> +'\n> +\n> a=refs/heads/a\n> b=refs/heads/b\n> c=refs/heads/c\n> diff --git a/refs.c b/refs.c\n> index 8b7a77fe5e..3669190499 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -666,10 +666,21 @@ static int write_pseudoref(const char\n> *pseudoref, const struct object_id *oid,\n> \tif (old_oid) {\n> \t\tstruct object_id actual_old_oid;\n> \n> -\t\tif (read_ref(pseudoref, &actual_old_oid))\n> -\t\t\tdie(\"could not read ref '%s'\", pseudoref);\n> -\t\tif (oidcmp(&actual_old_oid, old_oid)) {\n> -\t\t\tstrbuf_addf(err, \"unexpected sha1 when\n> writing '%s'\", pseudoref);\n> +\t\tif (read_ref(pseudoref, &actual_old_oid)) {\n> +\t\t\tif (!is_null_oid(old_oid)) {\n> +\t\t\t\tstrbuf_addf(err, \"could not read ref\n> '%s'\",\n> +\t\t\t\t\t pseudoref);\n> +\t\t\t\trollback_lock_file(&lock);\n> +\t\t\t\tgoto done;\n> +\t\t\t}\n> +\t\t} else if (is_null_oid(old_oid)) {\n> +\t\t\tstrbuf_addf(err, \"ref '%s' already exists\",\n> +\t\t\t\t pseudoref);\n> +\t\t\trollback_lock_file(&lock);\n> +\t\t\tgoto done;\n> +\t\t} else if (oidcmp(&actual_old_oid, old_oid)) {\n> +\t\t\tstrbuf_addf(err, \"unexpected sha1 when\n> writing '%s'\",\n> +\t\t\t\t pseudoref);\n> \t\t\trollback_lock_file(&lock);\n> \t\t\tgoto done;\n> \t\t}\n"},{"id":"346849","messageId":"1291dbc0-31f9-213a-ae30-1a6349ea1626@alum.mit.edu","threadId":"48410","inReplyTo":"20180506133549.8536-1-martin.agren@gmail.com","subject":"Re: [PATCH] refs: handle null-oid for pseudorefs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2018-05-07T07:39:06Z","receivedAt":"2018-05-07T07:39:14Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/06/2018 03:35 PM, Martin Ågren wrote:\n> According to the documentation on `git update-ref`, it is possible to\n> \"specify 40 '0' or an empty string as to make sure that the\n> ref you are creating does not exist.\" But in the code for pseudorefs, we\n> do not implement this. If we fail to read the old ref, we immediately\n> die. A failure to read would actually be a good thing if we have been\n> given the null-oid.\n> \n> With the null-oid, allow -- and even require -- the ref-reading to fail.\n> This implements the \"make sure that the ref ... does not exist\" part of\n> the documentation.\n> \n> Since we have a `strbuf err` for collecting errors, let's use it and\n> signal an error to the caller instead of dying hard.\n> \n> Reported-by: Rafael Ascensão \n> Helped-by: Rafael Ascensão \n> Signed-off-by: Martin Ågren \n\nThanks for the patch. This looks good to me. But it it seems that the\ntest coverage related to pseudorefs is still not great. Ideally, all of\nthe following combinations should be tested:\n\nPre-update value | ref-update old OID | Expected result\n-------------------|----------------------|----------------\nmissing | missing | accept *\nmissing | value | reject\nset | missing | reject *\nset | correct value | accept\nset | wrong value | reject\n\nI think your test only covers the lines with asterisks. Are the other\nscenarios already covered by other tests? If not, how about adding them?\nThat would give us confidence that the new code works in all circumstances.\n\nMichael\n"},{"id":"346856","messageId":"CAN0heSp-rxqAVJ3Q1KMD=eYqPUkcDP8xBTVTDtGfom6v5WpBLQ@mail.gmail.com","threadId":"48410","inReplyTo":"1291dbc0-31f9-213a-ae30-1a6349ea1626@alum.mit.edu","subject":"Re: [PATCH] refs: handle null-oid for pseudorefs","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-05-07T10:05:42Z","receivedAt":"2018-05-07T10:05:46Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 7 May 2018 at 09:39, Michael Haggerty wrote:\n> Thanks for the patch. This looks good to me. But it it seems that the\n> test coverage related to pseudorefs is still not great. Ideally, all of\n> the following combinations should be tested:\n>\n> Pre-update value | ref-update old OID | Expected result\n> -------------------|----------------------|----------------\n> missing | missing | accept *\n> missing | value | reject\n> set | missing | reject *\n> set | correct value | accept\n> set | wrong value | reject\n>\n> I think your test only covers the lines with asterisks. Are the other\n> scenarios already covered by other tests? If not, how about adding them?\n> That would give us confidence that the new code works in all circumstances.\n\nThank you for your comments. I was not able to find much\npseudoref-testing. I think what I should do is a patch 1/2 adding the\ntests you outlined (some will be expected failures), then turn this\npatch into a patch 2/2.\n\nMartin\n"},{"id":"347296","messageId":"cover.1525979881.git.martin.agren@gmail.com","threadId":"48410","inReplyTo":"CAN0heSp-rxqAVJ3Q1KMD=eYqPUkcDP8xBTVTDtGfom6v5WpBLQ@mail.gmail.com","subject":"[PATCH v2 0/3] refs: handle zero oid for pseudorefs","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-05-10T19:29:53Z","receivedAt":"2018-05-10T19:30:12Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 7 May 2018 at 12:05, Martin Ågren wrote:\n> On 7 May 2018 at 09:39, Michael Haggerty wrote:\n>> Thanks for the patch. This looks good to me. But it it seems that the\n>> test coverage related to pseudorefs is still not great. Ideally, all of\n>> the following combinations should be tested:\n>\n> Thank you for your comments. I was not able to find much\n> pseudoref-testing. I think what I should do is a patch 1/2 adding the\n> tests you outlined (some will be expected failures), then turn this\n> patch into a patch 2/2.\n\nWell, it turned into three patches. One to move away from \"sha1\" in some\nerror messages before spreading them to the test suite, then one to add\nthe tests, then one for the actual fix.\n\nMartin\n\nMartin Ågren (3):\n refs.c: refer to \"object ID\", not \"sha1\", in error messages\n t1400: add tests around adding/deleting pseudorefs\n refs: handle zero oid for pseudorefs\n\n t/t1400-update-ref.sh | 60 +++++++++++++++++++++++++++++++++++++++++++\n refs.c | 22 ++++++++++++----\n 2 files changed, 77 insertions(+), 5 deletions(-)\n\n-- \n2.17.0\n\n"},{"id":"347297","messageId":"076c6847d4f533103a8be5d9b6dd4dc3dfb8c83a.1525979881.git.martin.agren@gmail.com","threadId":"48410","inReplyTo":"cover.1525979881.git.martin.agren@gmail.com","subject":"[PATCH v2 1/3] refs.c: refer to \"object ID\", not \"sha1\", in error messages","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-05-10T19:29:54Z","receivedAt":"2018-05-10T19:30:20Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"We have two error messages that complain about the \"sha1\". Because we\nare about to touch one of these sites and add some tests, let's first\nmodernize the messages to say \"object ID\" instead.\n\nWhile at it, make the second one use `error()` instead of `warning()`.\nAfter printing the message, we do not continue, but actually drop the\nlock and return -1 without deleting the pseudoref.\n\nSigned-off-by: Martin Ågren \n---\nWe could make error-reporting more consistent in general in this file,\nbut I'd rather not lose track of the original goal of this series.\n\n refs.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 64aadd14c9..7820a52c4f 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -684,7 +684,8 @@ static int write_pseudoref(const char *pseudoref, const struct object_id *oid,\n \t\tif (read_ref(pseudoref, &actual_old_oid))\n \t\t\tdie(\"could not read ref '%s'\", pseudoref);\n \t\tif (oidcmp(&actual_old_oid, old_oid)) {\n-\t\t\tstrbuf_addf(err, \"unexpected sha1 when writing '%s'\", pseudoref);\n+\t\t\tstrbuf_addf(err, \"unexpected object ID when writing '%s'\",\n+\t\t\t\t pseudoref);\n \t\t\trollback_lock_file(&lock);\n \t\t\tgoto done;\n \t\t}\n@@ -722,7 +723,8 @@ static int delete_pseudoref(const char *pseudoref, const struct object_id *old_o\n \t\tif (read_ref(pseudoref, &actual_old_oid))\n \t\t\tdie(\"could not read ref '%s'\", pseudoref);\n \t\tif (oidcmp(&actual_old_oid, old_oid)) {\n-\t\t\twarning(\"Unexpected sha1 when deleting %s\", pseudoref);\n+\t\t\terror(\"unexpected object ID when deleting '%s'\",\n+\t\t\t pseudoref);\n \t\t\trollback_lock_file(&lock);\n \t\t\treturn -1;\n \t\t}\n-- \n2.17.0\n\n"},{"id":"347298","messageId":"5daaf948da0575ee95cfa1499a04d6cced4e3454.1525979881.git.martin.agren@gmail.com","threadId":"48410","inReplyTo":"cover.1525979881.git.martin.agren@gmail.com","subject":"[PATCH v2 2/3] t1400: add tests around adding/deleting pseudorefs","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-05-10T19:29:55Z","receivedAt":"2018-05-10T19:30:23Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"I have not been able to find any tests around adding pseudorefs using\n`git update-ref`. Add some as outlined in this table (original design by\nMichael Haggerty; modified and extended by me):\n\nPre-update value | ref-update old OID | Expected result\n-------------------|----------------------|----------------\nmissing | value | reject\nmissing | none given | accept\nset | none given | accept\nset | correct value | accept\nset | wrong value | reject\nmissing | zero | accept *\nset | zero | reject *\n\nThe tests marked with a * currently fail, despite git-update-ref(1)\nclaiming that it is possible to \"specify 40 '0' or an empty string as\n to make sure that the ref you are creating does not exist.\"\nThese failing tests will be fixed in the next commit.\n\nIt is only natural to test deletion as well. Test deletion without an\nold OID, with a correct one and with an incorrect one.\n\nSuggested-by: Michael Haggerty \nSigned-off-by: Martin Ågren \n---\n t/t1400-update-ref.sh | 60 +++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 60 insertions(+)\n\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex 664a3a4e4e..3996109ba4 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -457,6 +457,66 @@ test_expect_success 'git cat-file blob master@{2005-05-26 23:42}:F (expect OTHER\n \ttest OTHER = $(git cat-file blob \"master@{2005-05-26 23:42}:F\")\n '\n \n+# Test adding and deleting pseudorefs\n+\n+test_expect_success 'given old value for missing pseudoref, do not create' '\n+\ttest_must_fail git update-ref PSEUDOREF $A $B 2>err &&\n+\ttest_path_is_missing .git/PSEUDOREF &&\n+\tgrep \"could not read ref\" err\n+'\n+\n+test_expect_success 'create pseudoref' '\n+\tgit update-ref PSEUDOREF $A &&\n+\ttest $A = $(cat .git/PSEUDOREF)\n+'\n+\n+test_expect_success 'overwrite pseudoref with no old value given' '\n+\tgit update-ref PSEUDOREF $B &&\n+\ttest $B = $(cat .git/PSEUDOREF)\n+'\n+\n+test_expect_success 'overwrite pseudoref with correct old value' '\n+\tgit update-ref PSEUDOREF $C $B &&\n+\ttest $C = $(cat .git/PSEUDOREF)\n+'\n+\n+test_expect_success 'do not overwrite pseudoref with wrong old value' '\n+\ttest_must_fail git update-ref PSEUDOREF $D $E 2>err &&\n+\ttest $C = $(cat .git/PSEUDOREF) &&\n+\tgrep \"unexpected object ID\" err\n+'\n+\n+test_expect_success 'delete pseudoref' '\n+\tgit update-ref -d PSEUDOREF &&\n+\ttest_path_is_missing .git/PSEUDOREF\n+'\n+\n+test_expect_success 'do not delete pseudoref with wrong old value' '\n+\tgit update-ref PSEUDOREF $A &&\n+\ttest_must_fail git update-ref -d PSEUDOREF $B 2>err &&\n+\ttest $A = $(cat .git/PSEUDOREF) &&\n+\tgrep \"unexpected object ID\" err\n+'\n+\n+test_expect_success 'delete pseudoref with correct old value' '\n+\tgit update-ref -d PSEUDOREF $A &&\n+\ttest_path_is_missing .git/PSEUDOREF\n+'\n+\n+test_expect_failure 'create pseudoref with old OID zero' '\n+\tgit update-ref PSEUDOREF $A $Z &&\n+\ttest $A = $(cat .git/PSEUDOREF)\n+'\n+\n+test_expect_failure 'do not overwrite pseudoref with old OID zero' '\n+\ttest_when_finished git update-ref -d PSEUDOREF &&\n+\ttest_must_fail git update-ref PSEUDOREF $B $Z 2>err &&\n+\ttest $A = $(cat .git/PSEUDOREF) &&\n+\tgrep \"already exists\" err\n+'\n+\n+# Test --stdin\n+\n a=refs/heads/a\n b=refs/heads/b\n c=refs/heads/c\n-- \n2.17.0\n\n"},{"id":"347299","messageId":"1b6b1a25acf42747df479b90b1cc28e4c31eabe0.1525979881.git.martin.agren@gmail.com","threadId":"48410","inReplyTo":"cover.1525979881.git.martin.agren@gmail.com","subject":"[PATCH v2 3/3] refs: handle zero oid for pseudorefs","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-05-10T19:29:56Z","receivedAt":"2018-05-10T19:30:27Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"According to the documentation, it is possible to \"specify 40 '0' or an\nempty string as to make sure that the ref you are creating\ndoes not exist.\" But in the code for pseudorefs, we do not implement\nthis, as demonstrated by the failing tests added in the previous commit.\nIf we fail to read the old ref, we immediately die. But a failure to\nread would actually be a good thing if we have been given the zero oid.\n\nWith the zero oid, allow -- and even require -- the ref-reading to fail.\nThis implements the \"make sure that the ref ... does not exist\" part of\nthe documentation and fixes both failing tests from the previous commit.\n\nSince we have a `strbuf err` for collecting errors, let's use it and\nsignal an error to the caller instead of dying hard.\n\nReported-by: Rafael Ascensão \nHelped-by: Rafael Ascensão \nSigned-off-by: Martin Ågren \n---\n t/t1400-update-ref.sh | 4 ++--\n refs.c | 16 +++++++++++++---\n 2 files changed, 15 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex 3996109ba4..faf0dfe993 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -503,12 +503,12 @@ test_expect_success 'delete pseudoref with correct old value' '\n \ttest_path_is_missing .git/PSEUDOREF\n '\n \n-test_expect_failure 'create pseudoref with old OID zero' '\n+test_expect_success 'create pseudoref with old OID zero' '\n \tgit update-ref PSEUDOREF $A $Z &&\n \ttest $A = $(cat .git/PSEUDOREF)\n '\n \n-test_expect_failure 'do not overwrite pseudoref with old OID zero' '\n+test_expect_success 'do not overwrite pseudoref with old OID zero' '\n \ttest_when_finished git update-ref -d PSEUDOREF &&\n \ttest_must_fail git update-ref PSEUDOREF $B $Z 2>err &&\n \ttest $A = $(cat .git/PSEUDOREF) &&\ndiff --git a/refs.c b/refs.c\nindex 7820a52c4f..26af07fc51 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -681,9 +681,19 @@ static int write_pseudoref(const char *pseudoref, const struct object_id *oid,\n \tif (old_oid) {\n \t\tstruct object_id actual_old_oid;\n \n-\t\tif (read_ref(pseudoref, &actual_old_oid))\n-\t\t\tdie(\"could not read ref '%s'\", pseudoref);\n-\t\tif (oidcmp(&actual_old_oid, old_oid)) {\n+\t\tif (read_ref(pseudoref, &actual_old_oid)) {\n+\t\t\tif (!is_null_oid(old_oid)) {\n+\t\t\t\tstrbuf_addf(err, \"could not read ref '%s'\",\n+\t\t\t\t\t pseudoref);\n+\t\t\t\trollback_lock_file(&lock);\n+\t\t\t\tgoto done;\n+\t\t\t}\n+\t\t} else if (is_null_oid(old_oid)) {\n+\t\t\tstrbuf_addf(err, \"ref '%s' already exists\",\n+\t\t\t\t pseudoref);\n+\t\t\trollback_lock_file(&lock);\n+\t\t\tgoto done;\n+\t\t} else if (oidcmp(&actual_old_oid, old_oid)) {\n \t\t\tstrbuf_addf(err, \"unexpected object ID when writing '%s'\",\n \t\t\t\t pseudoref);\n \t\t\trollback_lock_file(&lock);\n-- \n2.17.0\n\n"}]}