patch, 2 partsshow-ref --verify: accept pseudorefs

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

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

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

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

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.

On Wed, Feb 07, 2024 at 09:12:29AM -0800, Junio C Hamano wrote:

`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

Hi Junio

On 07/02/2024 17:12, Junio C Hamano wrote:

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.

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.

 >

Thanks

Phillip

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