Re: [PATCH 2/2] builtin/receive-pack: add option to skip connectivity check

From: Justin Tobler <jltobler@gmail.com>
Date: May 20, 2025, 15:10 UTC
Message-ID: <vgqluphl4wdyzlkyoxndm2kspeylpeqhdi2chusytowjhvvqie@ya3yqorppdo6>
In-Reply-To: <aCwQU-SwlS8MR88l@pks.im>

On 25/05/20 07:17AM, Patrick Steinhardt wrote:

Show 25 quoted lines

> On Mon, May 19, 2025 at 08:49:20PM -0500, Justin Tobler wrote:
> > diff --git a/Documentation/git-receive-pack.adoc b/Documentation/git-receive-pack.adoc
> > index 20aca92073..68427d93d9 100644
> > --- a/Documentation/git-receive-pack.adoc
> > +++ b/Documentation/git-receive-pack.adoc
> > @@ -46,6 +46,18 @@ OPTIONS
> >  	`$GIT_URL/info/refs?service=git-receive-pack` requests. See
> >  	`--http-backend-info-refs` in linkgit:git-upload-pack[1].
> >  
> > +--skip-connectivity-check::
> > +	Bypasses the connectivity checks performed to validate incoming
> > +	objects. This option exists for server operators that may want to
> > +	implement their own object connectivity check outside of Git. This is
> > +	useful in such cases where the server-side knows additional information
> > +	about how Git is being used and thus can rely on guarantees to more
> > +	efficiently compute object connectivity that Git itself cannot make.
> > +	Usage of this option without a separate mechanism to validate and
> > +	ensure incoming objects connect properly to the references risks a
> > +	repository becoming corrupted and should not be used in the general
> > +	case.
> 
> Nit: the connectivity check doesn't only have to verify that objects
> connect to existing refs, but also that all objects part of the
> transitive closure of reachable objects exist. Might be worthwhile to
> point out here.

That's a good point, I'll teak the wording here so something like this:

	Bypasses the connectivity checks that validate the existence of all
	objects in the transitive closure of reachable objects. This option is
	intended for server operators that want to implement their own object
	connectivity validation outside of Git. This is useful in such cases
	where the server-side knows additional information about how Git is
	being used and thus can rely on certain guarantees to more efficiently
	compute object connectivity that Git itself cannot make. Usage of this
	option without a reliable external mechanism to ensure full reachable
	object connectivity risks corrupting the repository and should not be
	used in the general case.

Show 5 quoted lines

> > +	git -C remote.git cat-file -e $(git -C repo rev-parse HEAD)
> 
> And we do have the object now. Do we maybe also want to have a check
> though that the repository itself _isn't_ fully connected to ensure that
> the test setup isn't broken?

That makes sense. I'll do something like this in the next version:

diff --git a/t/t5410-receive-pack.sh b/t/t5410-receive-pack.sh
index 10c67c2bf8..f76a22943e 100755
--- a/t/t5410-receive-pack.sh
+++ b/t/t5410-receive-pack.sh
@@ -80,7 +80,8 @@ test_expect_success 'receive-pack missing objects bypasses connectivity check' '
 
 	test_grep ! "missing necessary objects" actual &&
 	test_must_be_empty err &&
-	git -C remote.git cat-file -e $(git -C repo rev-parse HEAD)
+	git -C remote.git cat-file -e $(git -C repo rev-parse HEAD) &&
+	test_must_fail git -C remote.git rev-list $(git -C repo rev-parse HEAD)
 '
 
 test_done

Thanks for the review!

-Justin