Re: [PATCH] refs: handle null-oid for pseudorefs

[Martin Ågren]

On 7 May 2018 at 09:39, Michael Haggerty  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

Re: [PATCH] refs: handle null-oid for pseudorefs

[Michael Haggerty]

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  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 
> Helped-by: Rafael Ascensão 
> Signed-off-by: Martin Ågren 

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

Re: [PATCH] refs: handle null-oid for pseudorefs

[David Turner]

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  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 
> Helped-by: Rafael Ascensão 
> Signed-off-by: Martin Ågren 
> ---
> (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, _old_oid))
> - die("could not read ref '%s'", pseudoref);
> - if (oidcmp(_old_oid, old_oid)) {
> - strbuf_addf(err, "unexpected sha1 when
> writing '%s'", pseudoref);
> + if (read_ref(pseudoref, _old_oid)) {
> + if (!is_null_oid(old_oid)) {
> + strbuf_addf(err, "could not read ref
> '%s'",
> + pseudoref);
> + rollback_lock_file();
> + goto done;
> + }
> + } else if (is_null_oid(old_oid)) {
> + strbuf_addf(err, "ref '%s' already exists",
> + pseudoref);
> + rollback_lock_file();
> + goto done;
> + } else if (oidcmp(_old_oid, old_oid)) {
> + strbuf_addf(err, "unexpected sha1 when
> writing '%s'",
> + pseudoref);
>   rollback_lock_file();
>   goto done;
>   }

[PATCH] refs: handle null-oid for pseudorefs

[Martin Ågren]

According to the documentation on `git update-ref`, 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. 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 
Helped-by: Rafael Ascensão 
Signed-off-by: Martin Ågren 
---
(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, _old_oid))
-   die("could not read ref '%s'", pseudoref);
-   if (oidcmp(_old_oid, old_oid)) {
-   strbuf_addf(err, "unexpected sha1 when writing '%s'", 
pseudoref);
+   if (read_ref(pseudoref, _old_oid)) {
+   if (!is_null_oid(old_oid)) {
+   strbuf_addf(err, "could not read ref '%s'",
+   pseudoref);
+   rollback_lock_file();
+   goto done;
+   }
+   } else if (is_null_oid(old_oid)) {
+   strbuf_addf(err, "ref '%s' already exists",
+   pseudoref);
+   rollback_lock_file();
+   goto done;
+   } else if (oidcmp(_old_oid, old_oid)) {
+   strbuf_addf(err, "unexpected sha1 when writing '%s'",
+   pseudoref);
rollback_lock_file();
goto done;
}
-- 
2.17.0.411.g9fd64c8e46