Re: [PATCH] upload-pack: reject shallow requests that would return nothing

[Duy Nguyen <pclouds@gmail.com>]

On Mon, May 28, 2018 at 8:48 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Mon, May 28, 2018 at 7:55 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>>
>>> To avoid this, if rev-list returns nothing, we abort the clone/fetch.
>>> The user could adjust their request (e.g. --shallow-since further back
>>> in the past) and retry.
>>
>> Yeah, that makes sense.
>>
>>> Another possible option for this case is to fall back to a default
>>> depth (like depth 1). But I don't like too much magic that way because
>>> we may return something unexpected to the user.
>>
>> I agree that it would be a horrible fallback.  I actually am
>> wondering if we should just silently return no objects without even
>> telling the user there is something unexpected happening.  After
>> all, the user may well be expecting with --shallow-since that is too
>> recent that the fetch may not result in pulling anything new, and
>> giving a "die" message, which now needs to be distinguished from
>> other forms of die's like network connectivity or auth failures, is
>> not all that helpful.
>
> An empty fetch is probably ok (though I would need to double check if
> anything bad would happen or git-fetch would give some helpful
> suggestion). git-clone on the other hand should actually clean this up
> with a good advice. I'll need to check and come back with v2 later.

It turns out harder than I thought. Cutting history and want/have
negotiation are separate but must be consistent. If during the
negotiation you said "I'm giving you this ref at this SHA-1" then you
send nothing back, the client will complain at connectivity check. It
expects all the objects that lead to said SHA-1.

Part of the problem is we advertise refs very early, before accepting
shallow requests and it's kinda hard to tell the user "ok with this
set of shallow requests, only these refs are actually valid and could
be sent back to you" before the want/have negotiation starts. At the
current state, the only thing we could do is tell the user "nak you
can't have that ref" even if we advertise the ref. This might confuse
clients and does not sound great.

I think for now die() may be a good enough quick fix. I'm not sure if
it's worth messing with the want/have negotiation just to cover this
rare edge case.
-- 
Duy