Re: [PATCH 1/2] http: advertise capabilities when cloning empty repos
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 27, 2023, 20:40 UTC
- Message-ID
- <xmqq8redjbyg.fsf@gitster.g>
- In-Reply-To
- <20230427053016.GD982277@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 14 quoted lines
> 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.
Show 14 quoted lines
>> +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.