Subject: Re: [PATCH v2 1/9] bundle-uri: short-circuit capability parsing

From: Derrick Stolee <[email-masked]>
Date: Mon, 19 Sep 2022 13:55:32 -0400

On 9/9/2022 1:24 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <[email-masked]> writes:
> 
>> From: Derrick Stolee <[email-masked]>
>>
>> When parsing the capability lines from the 'git remote-https' process,
>> we can stop reading the lines once we notice the 'get' capability.
>>
>> Reported-by: Teng Long <[email-masked]>
>> Signed-off-by: Derrick Stolee <[email-masked]>
>> ---
>>  bundle-uri.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/bundle-uri.c b/bundle-uri.c
>> index 4a8cc74ed05..7173ed065e9 100644
>> --- a/bundle-uri.c
>> +++ b/bundle-uri.c
>> @@ -56,8 +56,10 @@ static int download_https_uri_to_file(const char *file, const char *uri)
>>  	while (!strbuf_getline(&line, child_out)) {
>>  		if (!line.len)
>>  			break;
>> -		if (!strcmp(line.buf, "get"))
>> +		if (!strcmp(line.buf, "get")) {
>>  			found_get = 1;
>> +			break;
>> +		}
>>  	}
> 
> Hmph, is this safe to do?  Who is feeding child_out?  Aren't they
> get upset if we do not slurp what they write to us?  Are we
> expecting to read more from them after this part?  Aren't we get
> upset if we leave some other stuff when we read from child_out after
> we saw "get"?  If we respond to child_in without reading all from
> them, do we not get into a deadlock?
> 
> Perhaps these are all silly questions, but the description above
> does not quite answer them.

In my testing, this has not been a problem, but that does not mean
that it is safe to do. I'll drop this patch in v3.

Thanks,
-Stolee