Re: [PATCH 1/2] http: advertise capabilities when cloning empty repos

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> So I think in this hunk:
>
>> @@ -1379,6 +1381,8 @@ void upload_pack(const int advertise_refs, const int stateless_rpc,
>>  			data.no_done = 1;
>>  		head_ref_namespaced(send_ref, &data);
>>  		for_each_namespaced_ref(send_ref, &data);
>> +		if (!data.sent_capabilities && advertise_refs)
>> +			send_ref("capabilities^{}", null_oid(), 0, &data);
>>  		/*
>>  		 * fflush stdout before calling advertise_shallow_grafts because send_ref
>>  		 * uses stdio.
>
> you would want to drop the "&& advertise_refs" bit, after which both of
> the cases above would yield a sha256 repository.

Good suggestion.

>> +test_expect_success 'clone empty SHA-256 repository with protocol v0' '
>> +	rm -fr sha256 &&
>> +	echo sha256 >expected &&
>> +	GIT_TRACE=1 GIT_TRACE_PACKET=1 git -c protocol.version=0 clone "$HTTPD_URL/smart/sha256.git" &&
>> +	git -C sha256 rev-parse --show-object-format >actual &&
>> +	test_cmp actual expected &&
>> +	git ls-remote "$HTTPD_URL/smart/sha256.git" >actual &&
>> +	test_must_be_empty actual
>> +'
>
> This looks reasonable, though I think if we do not need HTTP to
> demonstrate the issue (and I don't think we do), then we should probably
> avoid it, just to get test coverage on platforms that don't support
> HTTP.

HTTP tests tend to be more cumbersome to set up and harder to debug
than the plain vanilla "over the pipe on the same machine"
transport, so I tend to agree with the statement.

They however represent a more common use case, so having HTTP tests
in addition to non-HTTP tests would be nicer, if we can afford to.

Thanks.