Subject: Re: [PATCH v7 1/5] unpack-objects.c: add dry_run mode for get_data()

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Tue, 21 Dec 2021 16:04:23 +0100

On Tue, Dec 21 2021, René Scharfe wrote:

> Am 21.12.21 um 15:09 schrieb Ævar Arnfjörð Bjarmason:
>>
>> On Tue, Dec 21 2021, Han Xin wrote:
>>
>>> From: Han Xin <[email-masked]>
>>>
>>> In dry_run mode, "get_data()" is used to verify the inflation of data,
>>> and the returned buffer will not be used at all and will be freed
>>> immediately. Even in dry_run mode, it is dangerous to allocate a
>>> full-size buffer for a large blob object. Therefore, only allocate a
>>> low memory footprint when calling "get_data()" in dry_run mode.
>>>
>>> Suggested-by: Jiang Xin <[email-masked]>
>>> Signed-off-by: Han Xin <[email-masked]>
>>> ---
>>>  builtin/unpack-objects.c            | 23 +++++++++---
>>>  t/t5590-unpack-non-delta-objects.sh | 57 +++++++++++++++++++++++++++++
>>>  2 files changed, 74 insertions(+), 6 deletions(-)
>>>  create mode 100755 t/t5590-unpack-non-delta-objects.sh
>>>
>>> diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
>>> index 4a9466295b..9104eb48da 100644
>>> --- a/builtin/unpack-objects.c
>>> +++ b/builtin/unpack-objects.c
>>> @@ -96,15 +96,21 @@ static void use(int bytes)
>>>  	display_throughput(progress, consumed_bytes);
>>>  }
>>>
>>> -static void *get_data(unsigned long size)
>>> +static void *get_data(size_t size, int dry_run)
>>>  {
>>>  	git_zstream stream;
>>> -	void *buf = xmallocz(size);
>>> +	size_t bufsize;
>>> +	void *buf;
>>>
>>>  	memset(&stream, 0, sizeof(stream));
>>> +	if (dry_run && size > 8192)
>>> +		bufsize = 8192;
>>> +	else
>>> +		bufsize = size;
>>> +	buf = xmallocz(bufsize);
>>
>> Maybe I'm misunderstanding this, but the commit message says it would be
>> dangerous to allocate a very larger buffer, but here we only limit the
>> size under "dry_run".
>
> This patch reduces the memory usage of dry runs, as its commit message
> says.  The memory usage of one type of actual (non-dry) unpack is reduced
> by patch 5.
>
>> Removing that "&& size > 8192" makes all the tests pass still, so there
>> seems to be some missing coverage here in any case.
>
> How would you test that an 8KB buffer is allocated even though a smaller
> one would suffice?  And why?  Wasting a few KB shouldn't be noticeable.

That doesn't sound like it needs to be tested. I was just trying to grok
what this was all doing. Thanks!