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

On Tue, Aug 16, 2016 at 3:16 PM, Jeff King [off-list ref] wrote:

On Tue, Aug 16, 2016 at 10:17:01AM +0200, Christian Couder wrote:

quoted

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.

quoted

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