| Message ID | 20240429081512.GA4130242@coredump.intra.peff.net (mailing list archive) |
|---|---|
| Headers | show |
| Series | tightening ref handling outside of refs/ | expand |
On Mon, Apr 29, 2024 at 04:15:13AM -0400, Jeff King wrote: > [1/8]: t0600: don't create ref outside of refs/ > [2/8]: t5619: use fully qualified refname for branch > [3/8]: refs: move is_pseudoref_syntax() definition earlier > [4/8]: refs: disallow dash in pseudoref syntax > [5/8]: refs: use is_pseudoref_syntax() in refname_is_safe() > [6/8]: check_refname_format(): add FULLY_QUALIFIED flag > [7/8]: refs: check refnames as fully qualified when writing > [8/8]: refs: check refnames as fully qualified when resolving Ugh, sorry, I managed to break the threading due to some too-clever use of mutt. ;) The other messages are on the list, and hopefully shouldn't be too hard to find by date. -Peff
On Mon, Apr 29, 2024 at 04:15:13AM -0400, Jeff King wrote: > [1/8]: t0600: don't create ref outside of refs/ > [2/8]: t5619: use fully qualified refname for branch You can probably guess that I found these test cleanups only after writing the rest of the series and seeing them fail. :) It turns out there is one more spot that is run only with reftables (so CI caught it, but my local testing did not): diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh index 178791e086..c6dbd2b5c4 100755 --- a/t/t0610-reftable-basics.sh +++ b/t/t0610-reftable-basics.sh @@ -343,11 +343,11 @@ test_expect_success 'ref transaction: env var disables compaction' ' for i in $(test_seq $iterations) do GIT_TEST_REFTABLE_AUTOCOMPACTION=false \ - git -C repo update-ref branch-$i HEAD || return 1 + git -C repo update-ref refs/heads/branch-$i HEAD || return 1 done && test_line_count = $expected repo/.git/reftable/tables.list && - git -C repo update-ref foo HEAD && + git -C repo update-ref refs/heads/foo HEAD && test_line_count -lt $expected repo/.git/reftable/tables.list ' I'll wait for comments before re-rolling, but I'll make sure that gets added in. -Peff
On Mon, Apr 29, 2024 at 04:42:38AM -0400, Jeff King wrote: > On Mon, Apr 29, 2024 at 04:15:13AM -0400, Jeff King wrote: > > > [1/8]: t0600: don't create ref outside of refs/ > > [2/8]: t5619: use fully qualified refname for branch > > You can probably guess that I found these test cleanups only after > writing the rest of the series and seeing them fail. :) > > It turns out there is one more spot that is run only with reftables (so > CI caught it, but my local testing did not): Yeah, that's an issue by itself in my opinion. It's ultimately the reason why I change this to always run the backend-specific tests in [1]. > diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh > index 178791e086..c6dbd2b5c4 100755 > --- a/t/t0610-reftable-basics.sh > +++ b/t/t0610-reftable-basics.sh > @@ -343,11 +343,11 @@ test_expect_success 'ref transaction: env var disables compaction' ' > for i in $(test_seq $iterations) > do > GIT_TEST_REFTABLE_AUTOCOMPACTION=false \ > - git -C repo update-ref branch-$i HEAD || return 1 > + git -C repo update-ref refs/heads/branch-$i HEAD || return 1 > done && > test_line_count = $expected repo/.git/reftable/tables.list && > > - git -C repo update-ref foo HEAD && > + git -C repo update-ref refs/heads/foo HEAD && > test_line_count -lt $expected repo/.git/reftable/tables.list > ' > > I'll wait for comments before re-rolling, but I'll make sure that gets > added in. The fix looks reasonable. Patrick [1]: https://lore.kernel.org/git/acf0c285506fe7ba275b08cdaf6b2245ec66b565.1712896869.git.ps@pks.im/
Jeff King <peff@peff.net> writes: > This series teaches check_refname_format() to enforce these rules (when > instructed; see patch 6 for a discussion of all sorts of complications). > > These changes are not backwards-compatible! But that is kind of the > point. This is stuff that was never supposed to work. My concern would > just be that somehow somebody is relying on it. I would of course be worried about the same, but these all look like reasonable "I wish we did them in this way from the beginning" changes.
On Mon, Apr 29, 2024 at 11:28:38AM +0200, Patrick Steinhardt wrote: > On Mon, Apr 29, 2024 at 04:42:38AM -0400, Jeff King wrote: > > On Mon, Apr 29, 2024 at 04:15:13AM -0400, Jeff King wrote: > > > > > [1/8]: t0600: don't create ref outside of refs/ > > > [2/8]: t5619: use fully qualified refname for branch > > > > You can probably guess that I found these test cleanups only after > > writing the rest of the series and seeing them fail. :) > > > > It turns out there is one more spot that is run only with reftables (so > > CI caught it, but my local testing did not): > > Yeah, that's an issue by itself in my opinion. It's ultimately the > reason why I change this to always run the backend-specific tests in > [1]. Ah, I hadn't seen that series. Yes, I'd be very much in favor of that change. -Peff
This is picking up the discussion from: https://lore.kernel.org/git/20240426211529.GD13703@coredump.intra.peff.net/ The basic issue is that we don't really enforce pseudoref syntax for names outside of "refs/", so you update random files in .git like: git update-ref objects/info/commit-graphs/commit-graph-chain HEAD This is mitigated a bit by: 1. You can't usually _overwrite_ files unless they look vaguely sha1-ish (in this case the chain file contains hashes of graph files, which is enough). So high-ticket items like "config" should be immune. 2. Receive-pack is a bit more careful here, and refuses anything outside of "refs/". So you can't get up to any mischief via "git push". But I still find it a bit scary/weird in general. And as noted in that thread, there's some attempt to enforce this that is done inconsistently. So you can update and read such refs, but are forbidden to delete them. Of course all of this becomes a non-issue with reftables, where those names are not used in the filesystem at all. But even there I think we'd probably want to consistently enforce the syntax rules (both between delete/update/read, but also consistency with the files backend). This series teaches check_refname_format() to enforce these rules (when instructed; see patch 6 for a discussion of all sorts of complications). These changes are not backwards-compatible! But that is kind of the point. This is stuff that was never supposed to work. My concern would just be that somehow somebody is relying on it. Pay attention specifically to patches 4, 7, and 8, which are where the behavior changes are. [1/8]: t0600: don't create ref outside of refs/ [2/8]: t5619: use fully qualified refname for branch [3/8]: refs: move is_pseudoref_syntax() definition earlier [4/8]: refs: disallow dash in pseudoref syntax [5/8]: refs: use is_pseudoref_syntax() in refname_is_safe() [6/8]: check_refname_format(): add FULLY_QUALIFIED flag [7/8]: refs: check refnames as fully qualified when writing [8/8]: refs: check refnames as fully qualified when resolving refs.c | 59 ++++++++++++---------- refs.h | 1 + t/t0600-reffiles-backend.sh | 2 +- t/t1430-bad-ref-name.sh | 20 ++++++++ t/t5619-clone-local-ambiguous-transport.sh | 4 +- 5 files changed, 57 insertions(+), 29 deletions(-) -Peff