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

[Christian Couder <christian.couder@gmail.com>]

On Tue, Aug 16, 2016 at 3:16 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Aug 16, 2016 at 10:17:01AM +0200, Christian Couder wrote:
>>
>> Cleaning up what has already been written to disk is a
>> related problem that is not addressed by this patch.
>>
>> The problem is that git-gc can clean up tmp_pack_* files
>> after their grace time expired, but that may not be
>> enough if someone tries to "git push" in a loop.
>>
>> A simple fix is to call register_tempfile() in
>> open_pack_file(), and just have index-pack clean up the
>> file on its way out.
>>
>> But there are harder cases. For instance, if somebody
>> pushes a 500MB file, and there is a pre-receive hook that
>> says "too big; I won't accept this". And then they push in
>> a loop, the incoming pack has already been accepted into
>> the repository by the time the pre-receive hook runs.
>> It's not possible to just delete it, because we don't know
>> if other simultaneous processes have started to depend on
>> the objects.
>
> IMHO, everything after "not addressed by this patch" doesn't really add
> anything. This commit has value on its own, and the discussion about the
> next steps is already documented on the list (and hopefully will be
> fixed with other patches!).

Ok, I will remove that in the next iteration.

>> +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`.
>> +
>> +while read unpacklimit filesize filename
>> +do
>> [...]
>> +done <<\EOF
>> +1 1024 one-k-file
>> +10 2048 two-k-file
>> +EOF
>
> Is there any reason to use different filenames and sizes for the two
> runs? They should behave identically, so it would make more sense to me
> to subject them to identical inputs.

About the sizes, I thought that some people might want to try sizes
closer to the limit and also that it is good anyway in general to add
a bit of "randomness", or at least variability, in the tests.

> I know you need different files and filenames to continue pushing into
> the same "dest", but the problem is easily solved by bumping the "git
> init" into the loop.

I thought that it would be a bit less wasteful to use the same "dest"
and also it would make the test more realistic as people often push in
non empty repos.