Re: [PATCH v2 3/3] receive-pack: allow a maximum input size to be specified

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 18, 2016, 20:25 UTC
Message-ID: <xmqqpop56czc.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160818131553.22580-4-chriscool@tuxfamily.org>

Christian Couder <christian.couder@gmail.com> writes:

Show 29 quoted lines

> From: Jeff King <peff@peff.net>
>
> Receive-pack feeds its input to either index-pack or
> unpack-objects, which will happily accept as many bytes as
> a sender is willing to provide. Let's allow an arbitrary
> cutoff point where we will stop writing bytes to disk.
>
> Cleaning up what has already been written to disk is a
> related problem that is not addressed by this patch.
>
> Signed-off-by: Jeff King <peff@peff.net>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  Documentation/config.txt           |  5 +++++
>  Documentation/git-receive-pack.txt |  3 +++
>  builtin/receive-pack.c             | 12 +++++++++++
>  t/t5546-push-limits.sh             | 42 ++++++++++++++++++++++++++++++++++++++
>  4 files changed, 62 insertions(+)
>  create mode 100755 t/t5546-push-limits.sh
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 0bcb679..f5b6061 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2517,6 +2517,11 @@ receive.unpackLimit::
>  	especially on slow filesystems.  If not set, the value of
>  	`transfer.unpackLimit` is used instead.
>  
> +receive.maxsize::

Shouldn't this be maxInputSize, not just maxSize? You are limiting the size of the input, not the size of the resulting pack, right?

> +	If the size of a pack file is larger than this limit, then

So, s/a pack file/the incoming pack stream/ or something?

> diff --git a/t/t5546-push-limits.sh b/t/t5546-push-limits.sh
> new file mode 100755
> index 0000000..09e958f

Technically, that's receive-limits, no? It is conceivable that the client side can also grow a feature to limit the size of an outgoing push, but that is not what this new script is about.

Show 12 quoted lines

> --- /dev/null
> +++ b/t/t5546-push-limits.sh
> @@ -0,0 +1,42 @@
> +#!/bin/sh
> +
> +test_description='check input limits for pushing'
> +. ./test-lib.sh
> +
> +test_expect_success 'create remote repository' '
> +	git init --bare dest
> +'
> +

> +# Let's run tests with different unpack limits: 1 and 10
> +# When the limit is 1, `git receive-pack` will call `git index-pack`.
> +# When the limit is 10, `git receive-pack` will call `git unpack-objects`.

I agree with Peff that these tests should push into an empty, pristine destination. Move the "create remote" step inside the while loop and prefix its "git init" with "rm -rf dest", or something like that.

It would make it crystal clear that the produced packstream for our transfer won't ever be affected by what is leftover in the destination repository.

Also, I think it would make it far more readable if you made the body of the while loop into a helper function that runs tests, keeping "1/10 corresponds to index/unpack" magic hidden inside the helper, i.e. something like

test_pack_input_limit () {
	size=$2
        case "$1" in
        unpack) unpack_limit=10000 ;;
        index) unpack_limit=1 ;;
	esac

	test_expect_success 'prepare destination repository' '
		rm -fr dest &&
		git --bare init dest
	'

	test_expect_success "set unpacklimit to $unpack_limit" '
		git --git-dir=dest config receive.unpacklimit "$unpack_limit"
	'

	test_expect_success 'setting receive.maxsize to 512 rejects push' '
		git --git-dir=dest config receive.maxsize 512 &&
		test_must_fail git push dest HEAD
	'

	test_expect_success 'bumping limit to 4k allows push' '
		git --git-dir=dest config receive.maxsize 4k &&
		git push dest HEAD
	'

	test_expect_success 'prepare destination repository (again)' '
		rm -fr dest &&
		git --bare init dest
	'

	test_expect_success 'lifting the limit allows push' '
		git --git-dir=dest config receive.maxsize 0 &&
		git push dest HEAD
	'
}

and have the body of the test more like this:

	test_expect_success 'setup' '
		test-genrandom foo 1024 >test-file &&
		git add test-file &&
		test_commit test-file
	'

	test_pack_input_limit unpack 1024 
	test_pack_input_limit index 1024

Perhaps?