git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Duy Nguyen <pclouds@gmail.com>
Date
Jun 2, 2018, 06:06 UTC
Message-ID
<CACsJy8D_RDhJTbMp26gN291piDAr5pPp7zcSmY7RHc_o-anB9g@mail.gmail.com>
In-Reply-To
<CACsJy8B7g-J8XeWYpY9SG1p51-fBLE2-V5872uWbyBXY2aVoEA@mail.gmail.com>
On Mon, May 28, 2018 at 8:48 PM, Duy Nguyen <pclouds@gmail.com> wrote:
Show 26 quoted lines
> 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
Previous: Duy NguyenNext: Junio C Hamano
Message 6 of 8 in “bug: --shallow-since misbehaves on old branch heads”
  1. Andreas KreyMay 22, 2018
  2. Duy NguyenMay 23, 2018
  3. upload-pack: reject shallow requests that would return nothingNguyễn Thái Ngọc Duy, May 26, 2018
  4. Junio C HamanoMay 28, 2018
  5. Duy NguyenMay 28, 2018
  6. Duy NguyenJun 2, 2018
  7. Junio C HamanoJun 4, 2018
  8. Duy NguyenJun 4, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.