{"thread":{"id":"61379","subject":"[PATCH 0/8] tightening ref handling outside of refs/","startedAt":"2024-04-29T08:15:14Z","lastAt":"2024-04-30T10:45:31Z","messageCount":7,"participants":["Jeff King","Patrick Steinhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"493611","messageId":"20240429081512.GA4130242@coredump.intra.peff.net","threadId":"61379","inReplyTo":null,"subject":"[PATCH 0/8] tightening ref handling outside of refs/","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-29T08:15:12Z","receivedAt":"2024-04-29T08:15:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is picking up the discussion from:\n\n https://lore.kernel.org/git/20240426211529.GD13703@coredump.intra.peff.net/\n\nThe basic issue is that we don't really enforce pseudoref syntax for\nnames outside of \"refs/\", so you update random files in .git like:\n\n git update-ref objects/info/commit-graphs/commit-graph-chain HEAD\n\nThis is mitigated a bit by:\n\n 1. You can't usually _overwrite_ files unless they look vaguely\n sha1-ish (in this case the chain file contains hashes of graph\n files, which is enough). So high-ticket items like \"config\" should\n be immune.\n\n 2. Receive-pack is a bit more careful here, and refuses anything\n outside of \"refs/\". So you can't get up to any mischief via \"git\n push\".\n\nBut I still find it a bit scary/weird in general. And as noted in that\nthread, there's some attempt to enforce this that is done\ninconsistently. So you can update and read such refs, but are forbidden\nto delete them.\n\nOf course all of this becomes a non-issue with reftables, where those\nnames are not used in the filesystem at all. But even there I think we'd\nprobably want to consistently enforce the syntax rules (both between\ndelete/update/read, but also consistency with the files backend).\n\nThis series teaches check_refname_format() to enforce these rules (when\ninstructed; see patch 6 for a discussion of all sorts of complications).\n\nThese changes are not backwards-compatible! But that is kind of the\npoint. This is stuff that was never supposed to work. My concern would\njust be that somehow somebody is relying on it. Pay attention\nspecifically to patches 4, 7, and 8, which are where the behavior\nchanges are.\n\n [1/8]: t0600: don't create ref outside of refs/\n [2/8]: t5619: use fully qualified refname for branch\n [3/8]: refs: move is_pseudoref_syntax() definition earlier\n [4/8]: refs: disallow dash in pseudoref syntax\n [5/8]: refs: use is_pseudoref_syntax() in refname_is_safe()\n [6/8]: check_refname_format(): add FULLY_QUALIFIED flag\n [7/8]: refs: check refnames as fully qualified when writing\n [8/8]: refs: check refnames as fully qualified when resolving\n\n refs.c | 59 ++++++++++++----------\n refs.h | 1 +\n t/t0600-reffiles-backend.sh | 2 +-\n t/t1430-bad-ref-name.sh | 20 ++++++++\n t/t5619-clone-local-ambiguous-transport.sh | 4 +-\n 5 files changed, 57 insertions(+), 29 deletions(-)\n\n-Peff\n"},{"id":"493612","messageId":"20240429081652.GA228803@coredump.intra.peff.net","threadId":"61379","inReplyTo":"20240429081512.GA4130242@coredump.intra.peff.net","subject":"[PATCH 1/8] t0600: don't create ref outside of refs/","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-29T08:16:52Z","receivedAt":"2024-04-29T08:16:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We have a test that tries to manipulate the branch refs/heads/referrent,\nmostly using either the fully qualified refname or git-branch (which\nimplies refs/heads/). However, the call to update-ref uses the\nunqualified name, meaning we were quietly creating \".git/referrent\",\nwhich was otherwise unused by the test.\n\nFix this to specify refs/heads/referrent.\n\nI _think_ it actually doesn't affect the test outcome either way. The\npoint of the test is that expiring reflogs for \"the_symref\" should not\nfail when we cannot lock \"refs/heads/referrent\" that it points to\n(because we have created a fake .lock file for it). And that is true\neven if the \"referrent\" file does not even exist. After all, the process\nholding the lock could be in the act of creating it.\n\nSo I suspect this \"update-ref\" line could just be dropped entirely.\nWhich you can verify by going back to its origin in 41d796ed5c (refs:\non symref reflog expire, lock symref not referrent, 2016-04-07) and\nremoving it. The test fails without the matching code change and passes\nwith it.\n\nBut I think it's worth keeping the update-ref call, as it creates a\nsituation which is more likely to match what we'd see in the real world.\nEven if it does not matter now, it's possible it could in the future.\n\nSigned-off-by: Jeff King \n---\n t/t0600-reffiles-backend.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh\nindex 64214340e7..41db9e30d4 100755\n--- a/t/t0600-reffiles-backend.sh\n+++ b/t/t0600-reffiles-backend.sh\n@@ -343,7 +343,7 @@ test_expect_success SHA1 'parsing reverse reflogs at BUFSIZ boundaries' '\n test_expect_success 'reflog expire operates on symref not referrent' '\n \tgit branch --create-reflog the_symref &&\n \tgit branch --create-reflog referrent &&\n-\tgit update-ref referrent HEAD &&\n+\tgit update-ref refs/heads/referrent HEAD &&\n \tgit symbolic-ref refs/heads/the_symref refs/heads/referrent &&\n \ttest_when_finished \"rm -f .git/refs/heads/referrent.lock\" &&\n \ttouch .git/refs/heads/referrent.lock &&\n-- \n2.45.0.rc1.416.gbe2a76c799\n\n"},{"id":"493620","messageId":"20240429083659.GA233736@coredump.intra.peff.net","threadId":"61379","inReplyTo":"20240429081512.GA4130242@coredump.intra.peff.net","subject":"Re: [PATCH 0/8] tightening ref handling outside of refs/","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-29T08:36:59Z","receivedAt":"2024-04-29T08:37:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 29, 2024 at 04:15:13AM -0400, Jeff King wrote:\n\n> [1/8]: t0600: don't create ref outside of refs/\n> [2/8]: t5619: use fully qualified refname for branch\n> [3/8]: refs: move is_pseudoref_syntax() definition earlier\n> [4/8]: refs: disallow dash in pseudoref syntax\n> [5/8]: refs: use is_pseudoref_syntax() in refname_is_safe()\n> [6/8]: check_refname_format(): add FULLY_QUALIFIED flag\n> [7/8]: refs: check refnames as fully qualified when writing\n> [8/8]: refs: check refnames as fully qualified when resolving\n\nUgh, sorry, I managed to break the threading due to some too-clever use\nof mutt. ;)\n\nThe other messages are on the list, and hopefully shouldn't be too hard\nto find by date.\n\n-Peff\n"},{"id":"493621","messageId":"20240429084238.GA242085@coredump.intra.peff.net","threadId":"61379","inReplyTo":"20240429081512.GA4130242@coredump.intra.peff.net","subject":"Re: [PATCH 0/8] tightening ref handling outside of refs/","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-29T08:42:38Z","receivedAt":"2024-04-29T08:42:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 29, 2024 at 04:15:13AM -0400, Jeff King wrote:\n\n> [1/8]: t0600: don't create ref outside of refs/\n> [2/8]: t5619: use fully qualified refname for branch\n\nYou can probably guess that I found these test cleanups only after\nwriting the rest of the series and seeing them fail. :)\n\nIt turns out there is one more spot that is run only with reftables (so\nCI caught it, but my local testing did not):\n\ndiff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\nindex 178791e086..c6dbd2b5c4 100755\n--- a/t/t0610-reftable-basics.sh\n+++ b/t/t0610-reftable-basics.sh\n@@ -343,11 +343,11 @@ test_expect_success 'ref transaction: env var disables compaction' '\n \tfor i in $(test_seq $iterations)\n \tdo\n \t\tGIT_TEST_REFTABLE_AUTOCOMPACTION=false \\\n-\t\tgit -C repo update-ref branch-$i HEAD || return 1\n+\t\tgit -C repo update-ref refs/heads/branch-$i HEAD || return 1\n \tdone &&\n \ttest_line_count = $expected repo/.git/reftable/tables.list &&\n \n-\tgit -C repo update-ref foo HEAD &&\n+\tgit -C repo update-ref refs/heads/foo HEAD &&\n \ttest_line_count -lt $expected repo/.git/reftable/tables.list\n '\n \n\nI'll wait for comments before re-rolling, but I'll make sure that gets\nadded in.\n\n-Peff\n"},{"id":"493622","messageId":"Zi9oRuknlhfG1-WX@tanuki","threadId":"61379","inReplyTo":"20240429084238.GA242085@coredump.intra.peff.net","subject":"Re: [PATCH 0/8] tightening ref handling outside of refs/","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-29T09:28:38Z","receivedAt":"2024-04-29T09:28:44Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Apr 29, 2024 at 04:42:38AM -0400, Jeff King wrote:\n> On Mon, Apr 29, 2024 at 04:15:13AM -0400, Jeff King wrote:\n> \n> > [1/8]: t0600: don't create ref outside of refs/\n> > [2/8]: t5619: use fully qualified refname for branch\n> \n> You can probably guess that I found these test cleanups only after\n> writing the rest of the series and seeing them fail. :)\n> \n> It turns out there is one more spot that is run only with reftables (so\n> CI caught it, but my local testing did not):\n\nYeah, that's an issue by itself in my opinion. It's ultimately the\nreason why I change this to always run the backend-specific tests in\n[1].\n\n> diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\n> index 178791e086..c6dbd2b5c4 100755\n> --- a/t/t0610-reftable-basics.sh\n> +++ b/t/t0610-reftable-basics.sh\n> @@ -343,11 +343,11 @@ test_expect_success 'ref transaction: env var disables compaction' '\n> \tfor i in $(test_seq $iterations)\n> \tdo\n> \t\tGIT_TEST_REFTABLE_AUTOCOMPACTION=false \\\n> -\t\tgit -C repo update-ref branch-$i HEAD || return 1\n> +\t\tgit -C repo update-ref refs/heads/branch-$i HEAD || return 1\n> \tdone &&\n> \ttest_line_count = $expected repo/.git/reftable/tables.list &&\n> \n> -\tgit -C repo update-ref foo HEAD &&\n> +\tgit -C repo update-ref refs/heads/foo HEAD &&\n> \ttest_line_count -lt $expected repo/.git/reftable/tables.list\n> '\n> \n> I'll wait for comments before re-rolling, but I'll make sure that gets\n> added in.\n\nThe fix looks reasonable.\n\nPatrick\n\n[1]: https://lore.kernel.org/git/acf0c285506fe7ba275b08cdaf6b2245ec66b565.1712896869.git.ps@pks.im/\n"},{"id":"493637","messageId":"xmqq5xw01acv.fsf@gitster.g","threadId":"61379","inReplyTo":"20240429081512.GA4130242@coredump.intra.peff.net","subject":"Re: [PATCH 0/8] tightening ref handling outside of refs/","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-29T15:01:36Z","receivedAt":"2024-04-29T15:01:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> This series teaches check_refname_format() to enforce these rules (when\n> instructed; see patch 6 for a discussion of all sorts of complications).\n>\n> These changes are not backwards-compatible! But that is kind of the\n> point. This is stuff that was never supposed to work. My concern would\n> just be that somehow somebody is relying on it.\n\nI would of course be worried about the same, but these all look like\nreasonable \"I wish we did them in this way from the beginning\"\nchanges.\n"},{"id":"493751","messageId":"20240430104530.GG1279403@coredump.intra.peff.net","threadId":"61379","inReplyTo":"Zi9oRuknlhfG1-WX@tanuki","subject":"Re: [PATCH 0/8] tightening ref handling outside of refs/","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-30T10:45:30Z","receivedAt":"2024-04-30T10:45:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 29, 2024 at 11:28:38AM +0200, Patrick Steinhardt wrote:\n\n> On Mon, Apr 29, 2024 at 04:42:38AM -0400, Jeff King wrote:\n> > On Mon, Apr 29, 2024 at 04:15:13AM -0400, Jeff King wrote:\n> > \n> > > [1/8]: t0600: don't create ref outside of refs/\n> > > [2/8]: t5619: use fully qualified refname for branch\n> > \n> > You can probably guess that I found these test cleanups only after\n> > writing the rest of the series and seeing them fail. :)\n> > \n> > It turns out there is one more spot that is run only with reftables (so\n> > CI caught it, but my local testing did not):\n> \n> Yeah, that's an issue by itself in my opinion. It's ultimately the\n> reason why I change this to always run the backend-specific tests in\n> [1].\n\nAh, I hadn't seen that series. Yes, I'd be very much in favor of that\nchange.\n\n-Peff\n"}]}