{"thread":{"id":"60865","subject":"[PATCH 1/2] show-ref --verify: accept pseudorefs","startedAt":"2024-02-07T16:50:03Z","lastAt":"2024-02-08T14:34:48Z","messageCount":6,"participants":["Phillip Wood via GitGitGadget","Junio C Hamano","Patrick Steinhardt","phillip.wood123@gmail.com"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"488155","messageId":"4dedc5704c3af6ab4ec8cc7503dc826480775b8e.1707324277.git.gitgitgadget@gmail.com","threadId":"60865","inReplyTo":"pull.1654.git.1707324277.gitgitgadget@gmail.com","subject":"[PATCH 1/2] show-ref --verify: accept pseudorefs","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-07T16:44:35Z","receivedAt":"2024-02-07T16:50:03Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\n\"git show-ref --verify\" is useful for scripts that want to look up a\nfully qualified refname without falling back to the DWIM rules used by\n\"git rev-parse\" rules when the ref does not exist. Currently it will\nonly accept \"HEAD\" or a refname beginning with \"refs/\". Running\n\n git show-ref --verify CHERRY_PICK_HEAD\n\nwill always result in\n\n fatal: 'CHERRY_PICK_HEAD' - not a valid ref\n\neven when CHERRY_PICK_HEAD exists. By calling refname_is_safe() instead\nof comparing the refname to \"HEAD\" we can accept all one-level refs that\ncontain only uppercase ascii letters and underscores.\n\nSigned-off-by: Phillip Wood \n---\n builtin/show-ref.c | 2 +-\n t/t1403-show-ref.sh | 8 ++++++++\n 2 files changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/show-ref.c b/builtin/show-ref.c\nindex 79955c2856e..1c15421e600 100644\n--- a/builtin/show-ref.c\n+++ b/builtin/show-ref.c\n@@ -172,7 +172,7 @@ static int cmd_show_ref__verify(const struct show_one_options *show_one_opts,\n \twhile (*refs) {\n \t\tstruct object_id oid;\n \n-\t\tif ((starts_with(*refs, \"refs/\") || !strcmp(*refs, \"HEAD\")) &&\n+\t\tif ((starts_with(*refs, \"refs/\") || refname_is_safe(*refs)) &&\n \t\t !read_ref(*refs, &oid)) {\n \t\t\tshow_one(show_one_opts, *refs, &oid);\n \t\t}\ndiff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh\nindex d0a8f7b121c..33fb7a38fff 100755\n--- a/t/t1403-show-ref.sh\n+++ b/t/t1403-show-ref.sh\n@@ -174,6 +174,14 @@ test_expect_success 'show-ref --verify HEAD' '\n \ttest_must_be_empty actual\n '\n \n+test_expect_success 'show-ref --verify pseudorefs' '\n+\tgit update-ref CHERRY_PICK_HEAD HEAD $ZERO_OID &&\n+\ttest_when_finished \"git update-ref -d CHERRY_PICK_HEAD\" &&\n+\tgit show-ref -s --verify HEAD >actual &&\n+\tgit show-ref -s --verify CHERRY_PICK_HEAD >expect &&\n+\ttest_cmp actual expect\n+'\n+\n test_expect_success 'show-ref --verify with dangling ref' '\n \tsha1_file() {\n \t\techo \"$*\" | sed \"s#..#.git/objects/&/#\"\n-- \ngitgitgadget\n\n"},{"id":"488156","messageId":"b828456329956e9a0a50a33df63486bb3a985474.1707324277.git.gitgitgadget@gmail.com","threadId":"60865","inReplyTo":"pull.1654.git.1707324277.gitgitgadget@gmail.com","subject":"[PATCH 2/2] t1400: use show-ref to check pseudorefs","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-07T16:44:36Z","receivedAt":"2024-02-07T16:51:12Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nNow that \"git show-ref --verify\" accepts pseudorefs use that in\npreference to \"git rev-parse\" when checking pseudorefs as we do when\nchecking branches etc.\n\nSigned-off-by: Phillip Wood \n---\n t/t1400-update-ref.sh | 18 +++++++++---------\n 1 file changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex f18843bf7aa..78a09abc35e 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -524,51 +524,51 @@ test_expect_success 'given old value for missing pseudoref, do not create' '\n \n test_expect_success 'create pseudoref' '\n \tgit update-ref PSEUDOREF $A &&\n-\ttest $A = $(git rev-parse PSEUDOREF)\n+\ttest $A = $(git show-ref -s --verify PSEUDOREF)\n '\n \n test_expect_success 'overwrite pseudoref with no old value given' '\n \tgit update-ref PSEUDOREF $B &&\n-\ttest $B = $(git rev-parse PSEUDOREF)\n+\ttest $B = $(git show-ref -s --verify PSEUDOREF)\n '\n \n test_expect_success 'overwrite pseudoref with correct old value' '\n \tgit update-ref PSEUDOREF $C $B &&\n-\ttest $C = $(git rev-parse PSEUDOREF)\n+\ttest $C = $(git show-ref -s --verify 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 = $(git rev-parse PSEUDOREF) &&\n+\ttest $C = $(git show-ref -s --verify PSEUDOREF) &&\n \ttest_grep \"cannot lock ref.*expected\" err\n '\n \n test_expect_success 'delete pseudoref' '\n \tgit update-ref -d PSEUDOREF &&\n-\ttest_must_fail git rev-parse PSEUDOREF\n+\ttest_must_fail git show-ref -s --verify 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 = $(git rev-parse PSEUDOREF) &&\n+\ttest $A = $(git show-ref -s --verify PSEUDOREF) &&\n \ttest_grep \"cannot lock ref.*expected\" err\n '\n \n test_expect_success 'delete pseudoref with correct old value' '\n \tgit update-ref -d PSEUDOREF $A &&\n-\ttest_must_fail git rev-parse PSEUDOREF\n+\ttest_must_fail git show-ref -s --verify PSEUDOREF\n '\n \n test_expect_success 'create pseudoref with old OID zero' '\n \tgit update-ref PSEUDOREF $A $Z &&\n-\ttest $A = $(git rev-parse PSEUDOREF)\n+\ttest $A = $(git show-ref -s --verify PSEUDOREF)\n '\n \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 = $(git rev-parse PSEUDOREF) &&\n+\ttest $A = $(git show-ref -s --verify PSEUDOREF) &&\n \ttest_grep \"already exists\" err\n '\n \n-- \ngitgitgadget\n"},{"id":"488157","messageId":"pull.1654.git.1707324277.gitgitgadget@gmail.com","threadId":"60865","inReplyTo":null,"subject":"[PATCH 0/2] show-ref --verify: accept psuedorefs","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-07T16:44:34Z","receivedAt":"2024-02-07T16:51:40Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Running\n\ngit show-ref --verify CHERRY_PICK_HEAD\n\n\nwill always result in\n\nfatal: 'CHERRY_PICK_HEAD' - not a valid ref\n\n\neven when CHERRY_PICK_HEAD exists. The first patch in this series fixes that\nand the second patch then replaces some instances of \"git rev-parse \" with\n\"git show-ref --verify \" in our test suite.\n\nPhillip Wood (2):\n show-ref --verify: accept pseudorefs\n t1400: use show-ref to check pseudorefs\n\n builtin/show-ref.c | 2 +-\n t/t1400-update-ref.sh | 18 +++++++++---------\n t/t1403-show-ref.sh | 8 ++++++++\n 3 files changed, 18 insertions(+), 10 deletions(-)\n\n\nbase-commit: 2a540e432fe5dff3cfa9d3bf7ca56db2ad12ebb9\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1654%2Fphillipwood%2Fshow-ref-pseudorefs-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1654/phillipwood/show-ref-pseudorefs-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1654\n-- \ngitgitgadget\n"},{"id":"488163","messageId":"xmqq5xz0b3lu.fsf@gitster.g","threadId":"60865","inReplyTo":"4dedc5704c3af6ab4ec8cc7503dc826480775b8e.1707324277.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] show-ref --verify: accept pseudorefs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-07T17:12:29Z","receivedAt":"2024-02-07T17:12:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" writes:\n\n> ... when CHERRY_PICK_HEAD exists. By calling refname_is_safe() instead\n> of comparing the refname to \"HEAD\" we can accept all one-level refs that\n> contain only uppercase ascii letters and underscores.\n\nGeez. We have at least three implementations to determine if a ref\nis a valid name?\n\n> diff --git a/builtin/show-ref.c b/builtin/show-ref.c\n> index 79955c2856e..1c15421e600 100644\n> --- a/builtin/show-ref.c\n> +++ b/builtin/show-ref.c\n> @@ -172,7 +172,7 @@ static int cmd_show_ref__verify(const struct show_one_options *show_one_opts,\n> \twhile (*refs) {\n> \t\tstruct object_id oid;\n> \n> -\t\tif ((starts_with(*refs, \"refs/\") || !strcmp(*refs, \"HEAD\")) &&\n> +\t\tif ((starts_with(*refs, \"refs/\") || refname_is_safe(*refs)) &&\n\nI think the helper you picked is the most sensible one, modulo a few\nnits.\n\n - We would want to teach refname_is_safe() to honor is_pseudoref()\n from Karthik's series to make rules more consistent.\n\n - The refname_is_safe() helper is not just about the stuff at the\n root level. While starts_with(\"refs/\") is overly lenient, it\n rejects nonsense like \"refs/../trash\". We would want to lose\n \"starts_with() ||\" part of the condition from here.\n\nThese are minor non-blocking nits that we should keep in mind only\nfor longer term maintenance, #leftoverbits after the dust settles.\n\nWill queue.\n\nThanks.\n"},{"id":"488219","messageId":"ZcSPpbL6AWAOkkCI@tanuki","threadId":"60865","inReplyTo":"xmqq5xz0b3lu.fsf@gitster.g","subject":"Re: [PATCH 1/2] show-ref --verify: accept pseudorefs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-08T08:24:05Z","receivedAt":"2024-02-08T08:24:12Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Feb 07, 2024 at 09:12:29AM -0800, Junio C Hamano wrote:\n> \"Phillip Wood via GitGitGadget\" writes:\n> \n> > ... when CHERRY_PICK_HEAD exists. By calling refname_is_safe() instead\n> > of comparing the refname to \"HEAD\" we can accept all one-level refs that\n> > contain only uppercase ascii letters and underscores.\n> \n> Geez. We have at least three implementations to determine if a ref\n> is a valid name?\n\n`check_refname_format()` and `refname_is_safe()` are often used in\ntandem. `check_refname_format()` performs the first set of checks to\nverify whether the pathname components are valid, whereas\n`refname_is_safe()` checks for refs which are unsafe e.g. because they\ntry to escape out of \"refs/\".\n\nI think that we really ought to merge `refname_is_safe()` into\n`check_refname_format()`. It would require us to introduce a new flag\n`REFNAME_ALLOW_BAD_NAME` to the latter function so that it would accept\nrefs with a bad name that are otherwise safe. But I think we shouldn't\never allow unsafe names, so merging these two functions would overall\nreduce the potential for security-relevant issues.\n\nPatrick\n"},{"id":"488230","messageId":"e137c365-13c6-4ae3-aaf8-0b9d7852e508@gmail.com","threadId":"60865","inReplyTo":"xmqq5xz0b3lu.fsf@gitster.g","subject":"Re: [PATCH 1/2] show-ref --verify: accept pseudorefs","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-02-08T14:34:43Z","receivedAt":"2024-02-08T14:34:48Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 07/02/2024 17:12, Junio C Hamano wrote:\n> \"Phillip Wood via GitGitGadget\" writes:\n> I think the helper you picked is the most sensible one, modulo a few\n> nits.\n> \n> - We would want to teach refname_is_safe() to honor is_pseudoref()\n> from Karthik's series to make rules more consistent.\n\nYes, I held off sending this series waiting for a while but then got \nimpatient. We may want to split out a helper from is_pseudoref() that \njust checks the name without trying to read the ref for callers like \nthis which are going to read the ref anyway.\n\n> - The refname_is_safe() helper is not just about the stuff at the\n> root level. While starts_with(\"refs/\") is overly lenient, it\n> rejects nonsense like \"refs/../trash\". We would want to lose\n> \"starts_with() ||\" part of the condition from here.\n\nI left the \"starts_with()\" in as we check the refname when we look it up \nwith read_ref() so it seemed like wasted effort to do it here as well.\n\n> These are minor non-blocking nits that we should keep in mind only\n> for longer term maintenance, #leftoverbits after the dust settles.\n >\n> Will queue.\n\nThanks\n\nPhillip\n"}]}