[PATCH 0/2] show-ref --verify: accept psuedorefs

[PATCH 0/2] show-ref --verify: accept psuedorefs

[Phillip Wood via GitGitGadget]

Running

git show-ref --verify CHERRY_PICK_HEAD


will always result in

fatal: 'CHERRY_PICK_HEAD' - not a valid ref


even when CHERRY_PICK_HEAD exists. The first patch in this series fixes that
and the second patch then replaces some instances of "git rev-parse " with
"git show-ref --verify " in our test suite.

Phillip Wood (2):
  show-ref --verify: accept pseudorefs
  t1400: use show-ref to check pseudorefs

 builtin/show-ref.c    |  2 +-
 t/t1400-update-ref.sh | 18 +++++++++---------
 t/t1403-show-ref.sh   |  8 ++++++++
 3 files changed, 18 insertions(+), 10 deletions(-)


base-commit: 2a540e432fe5dff3cfa9d3bf7ca56db2ad12ebb9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1654%2Fphillipwood%2Fshow-ref-pseudorefs-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1654/phillipwood/show-ref-pseudorefs-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1654
-- 
gitgitgadget

[PATCH 1/2] show-ref --verify: accept pseudorefs

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

"git show-ref --verify" is useful for scripts that want to look up a
fully qualified refname without falling back to the DWIM rules used by
"git rev-parse" rules when the ref does not exist. Currently it will
only accept "HEAD" or a refname beginning with "refs/". Running

    git show-ref --verify CHERRY_PICK_HEAD

will always result in

    fatal: 'CHERRY_PICK_HEAD' - not a valid ref

even when CHERRY_PICK_HEAD exists. By calling refname_is_safe() instead
of comparing the refname to "HEAD" we can accept all one-level refs that
contain only uppercase ascii letters and underscores.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/show-ref.c  | 2 +-
 t/t1403-show-ref.sh | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index 79955c2856e..1c15421e600 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -172,7 +172,7 @@ static int cmd_show_ref__verify(const struct show_one_options *show_one_opts,
 	while (*refs) {
 		struct object_id oid;
 
-		if ((starts_with(*refs, "refs/") || !strcmp(*refs, "HEAD")) &&
+		if ((starts_with(*refs, "refs/") || refname_is_safe(*refs)) &&
 		    !read_ref(*refs, &oid)) {
 			show_one(show_one_opts, *refs, &oid);
 		}
diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
index d0a8f7b121c..33fb7a38fff 100755
--- a/t/t1403-show-ref.sh
+++ b/t/t1403-show-ref.sh
@@ -174,6 +174,14 @@ test_expect_success 'show-ref --verify HEAD' '
 	test_must_be_empty actual
 '
 
+test_expect_success 'show-ref --verify pseudorefs' '
+	git update-ref CHERRY_PICK_HEAD HEAD $ZERO_OID &&
+	test_when_finished "git update-ref -d CHERRY_PICK_HEAD" &&
+	git show-ref -s --verify HEAD >actual &&
+	git show-ref -s --verify CHERRY_PICK_HEAD >expect &&
+	test_cmp actual expect
+'
+
 test_expect_success 'show-ref --verify with dangling ref' '
 	sha1_file() {
 		echo "$*" | sed "s#..#.git/objects/&/#"
-- 
gitgitgadget

[PATCH 2/2] t1400: use show-ref to check pseudorefs

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Now that "git show-ref --verify" accepts pseudorefs use that in
preference to "git rev-parse" when checking pseudorefs as we do when
checking branches etc.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t1400-update-ref.sh | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index f18843bf7aa..78a09abc35e 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -524,51 +524,51 @@ test_expect_success 'given old value for missing pseudoref, do not create' '
 
 test_expect_success 'create pseudoref' '
 	git update-ref PSEUDOREF $A &&
-	test $A = $(git rev-parse PSEUDOREF)
+	test $A = $(git show-ref -s --verify PSEUDOREF)
 '
 
 test_expect_success 'overwrite pseudoref with no old value given' '
 	git update-ref PSEUDOREF $B &&
-	test $B = $(git rev-parse PSEUDOREF)
+	test $B = $(git show-ref -s --verify PSEUDOREF)
 '
 
 test_expect_success 'overwrite pseudoref with correct old value' '
 	git update-ref PSEUDOREF $C $B &&
-	test $C = $(git rev-parse PSEUDOREF)
+	test $C = $(git show-ref -s --verify 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 = $(git rev-parse PSEUDOREF) &&
+	test $C = $(git show-ref -s --verify PSEUDOREF) &&
 	test_grep "cannot lock ref.*expected" err
 '
 
 test_expect_success 'delete pseudoref' '
 	git update-ref -d PSEUDOREF &&
-	test_must_fail git rev-parse PSEUDOREF
+	test_must_fail git show-ref -s --verify 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 = $(git rev-parse PSEUDOREF) &&
+	test $A = $(git show-ref -s --verify PSEUDOREF) &&
 	test_grep "cannot lock ref.*expected" err
 '
 
 test_expect_success 'delete pseudoref with correct old value' '
 	git update-ref -d PSEUDOREF $A &&
-	test_must_fail git rev-parse PSEUDOREF
+	test_must_fail git show-ref -s --verify PSEUDOREF
 '
 
 test_expect_success 'create pseudoref with old OID zero' '
 	git update-ref PSEUDOREF $A $Z &&
-	test $A = $(git rev-parse PSEUDOREF)
+	test $A = $(git show-ref -s --verify PSEUDOREF)
 '
 
 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 = $(git rev-parse PSEUDOREF) &&
+	test $A = $(git show-ref -s --verify PSEUDOREF) &&
 	test_grep "already exists" err
 '
 
-- 
gitgitgadget

Re: [PATCH 1/2] show-ref --verify: accept pseudorefs

[Junio C Hamano]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> ... when CHERRY_PICK_HEAD exists. By calling refname_is_safe() instead
> of comparing the refname to "HEAD" we can accept all one-level refs that
> contain only uppercase ascii letters and underscores.

Geez.  We have at least three implementations to determine if a ref
is a valid name?

> diff --git a/builtin/show-ref.c b/builtin/show-ref.c
> index 79955c2856e..1c15421e600 100644
> --- a/builtin/show-ref.c
> +++ b/builtin/show-ref.c
> @@ -172,7 +172,7 @@ static int cmd_show_ref__verify(const struct show_one_options *show_one_opts,
>  	while (*refs) {
>  		struct object_id oid;
>  
> -		if ((starts_with(*refs, "refs/") || !strcmp(*refs, "HEAD")) &&
> +		if ((starts_with(*refs, "refs/") || refname_is_safe(*refs)) &&

I think the helper you picked is the most sensible one, modulo a few
nits.

 - We would want to teach refname_is_safe() to honor is_pseudoref()
   from Karthik's series to make rules more consistent.

 - The refname_is_safe() helper is not just about the stuff at the
   root level.  While starts_with("refs/") is overly lenient, it
   rejects nonsense like "refs/../trash".  We would want to lose
   "starts_with() ||" part of the condition from here.

These are minor non-blocking nits that we should keep in mind only
for longer term maintenance, #leftoverbits after the dust settles.

Will queue.

Thanks.

Re: [PATCH 1/2] show-ref --verify: accept pseudorefs

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1163 bytes --]

On Wed, Feb 07, 2024 at 09:12:29AM -0800, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
> > ... when CHERRY_PICK_HEAD exists. By calling refname_is_safe() instead
> > of comparing the refname to "HEAD" we can accept all one-level refs that
> > contain only uppercase ascii letters and underscores.
> 
> Geez.  We have at least three implementations to determine if a ref
> is a valid name?

`check_refname_format()` and `refname_is_safe()` are often used in
tandem. `check_refname_format()` performs the first set of checks to
verify whether the pathname components are valid, whereas
`refname_is_safe()` checks for refs which are unsafe e.g. because they
try to escape out of "refs/".

I think that we really ought to merge `refname_is_safe()` into
`check_refname_format()`. It would require us to introduce a new flag
`REFNAME_ALLOW_BAD_NAME` to the latter function so that it would accept
refs with a bad name that are otherwise safe. But I think we shouldn't
ever allow unsafe names, so merging these two functions would overall
reduce the potential for security-relevant issues.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] show-ref --verify: accept pseudorefs

[phillip.wood123]

Hi Junio

On 07/02/2024 17:12, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> I think the helper you picked is the most sensible one, modulo a few
> nits.
> 
>   - We would want to teach refname_is_safe() to honor is_pseudoref()
>     from Karthik's series to make rules more consistent.

Yes, I held off sending this series waiting for a while but then got 
impatient. We may want to split out a helper from is_pseudoref() that 
just checks the name without trying to read the ref for callers like 
this which are going to read the ref anyway.

>   - The refname_is_safe() helper is not just about the stuff at the
>     root level.  While starts_with("refs/") is overly lenient, it
>     rejects nonsense like "refs/../trash".  We would want to lose
>     "starts_with() ||" part of the condition from here.

I left the "starts_with()" in as we check the refname when we look it up 
with read_ref() so it seemed like wasted effort to do it here as well.

> These are minor non-blocking nits that we should keep in mind only
> for longer term maintenance, #leftoverbits after the dust settles.
 >
> Will queue.

Thanks

Phillip