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

[Patrick Steinhardt <ps@pks.im>]

On Mon, May 19, 2025 at 08:49:20PM -0500, Justin Tobler wrote:
> During git-receive-pack(1), connectivity of the object graph is
> validated to ensure that the received packfile does not leave the
> repository in a broken state. This is done via git-rev-list(1) and
> walking the objects which can be expensive for large repositories.

s/objects/&,/

> 
> Generally, this check is critical to avoid an incomplete received
> packfile from corrupting a repository. Server operators may have
> additional knowledge though around exactly how Git is being used on the
> server-side which can be used to facilitate more efficient connectivity
> computatation of incoming objects.
> 
> For example, if it can be ensured that all objects in a repository are
> connected and do not depend on any missing objects, the connectivity of
> newly written objects can be checked by walking the object graph
> containing only the new objects from the updated tips and identifying
> the missing objects which represent the boundary between the new objects
> and the repository. These boundary objects can be checked in the
> canonical repository to ensure the new objects connect as expected and
> thus avoid walking the rest of the object graph.
> 
> Git itself cannot make the guarantees required for such an optimization
> as it is possible for a repository to contain an unreachable object that
> references a missing object without the repository being considered
> corrupt.

Yup, this reads very well to me now, and clearly lays out why it is an
assumption that _some_ setups can do, but others can't.

> Introduce the --skip-connectivity-check option for git-receive-pack(1)
> which bypasses this connectivity check to give more control to the
> server-side. Note that without proper server-side validation of newly
> received objects handled outside of Git, usage of this option risks
> corrupting a repository.
> 
> Signed-off-by: Justin Tobler <jltobler@gmail.com>
> ---
>  Documentation/git-receive-pack.adoc | 12 +++++++++
>  builtin/receive-pack.c              | 40 ++++++++++++++++-------------
>  t/t5410-receive-pack.sh             | 21 +++++++++++++++
>  3 files changed, 55 insertions(+), 18 deletions(-)
> 
> 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.

> diff --git a/t/t5410-receive-pack.sh b/t/t5410-receive-pack.sh
> index 9afea54a26..10c67c2bf8 100755
> --- a/t/t5410-receive-pack.sh
> +++ b/t/t5410-receive-pack.sh
> @@ -62,4 +62,25 @@ test_expect_success 'receive-pack missing objects fails connectivity check' '
>  	test_must_fail git -C remote.git cat-file -e $(git -C repo rev-parse HEAD)
>  '
>  
> +test_expect_success 'receive-pack missing objects bypasses connectivity check' '
> +	test_when_finished rm -rf repo remote.git setup.git &&
> +
> +	git init repo &&
> +	git -C repo commit --allow-empty -m 1 &&
> +	git clone --bare repo setup.git &&
> +	git -C repo commit --allow-empty -m 2 &&
> +
> +	# Capture git-send-pack(1) output sent to git-receive-pack(1).
> +	git -C repo send-pack ../setup.git --all \
> +		--receive-pack="tee ${SQ}$(pwd)/out${SQ} | git-receive-pack" &&
> +
> +	# Replay captured git-send-pack(1) output on new empty repository.
> +	git init --bare remote.git &&
> +	git receive-pack --skip-connectivity-check remote.git <out >actual 2>err &&
> +
> +	test_grep ! "missing necessary objects" actual &&
> +	test_must_be_empty err &&

Yup, the connectivity check shouldn't fail anymore.

> +	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?

Patrick