Re: [PATCH/GSoC/RFC] fetch: git fetch --deepen

[Duy Nguyen <pclouds@gmail.com>]

On Mon, Mar 16, 2015 at 11:10 PM, Dongcan Jiang <dongcan.jiang@gmail.com> wrote:
> Hi Duy,
>
> Sorry for my late response.
>
>> we need to make sure that upload-pack barf if some client sends both "deepen" and
>> "depth".
>
> Actually, in my current design, when client just wants "depth", it
> sends "depth N";
> when it want "deepen", it sends "depth N" as well as "depth_deepen".
> For the latter
> case, it tells upload-pack to collect objects for "deepen N".
>
> Thus, I'm not so sure whether upload-pack needs to check their exclusion.

C Git is not the only client that can talk to upload-pack. A buggy
client may send both. The least we need is make sure it would not
crash or break the server security in any way. But if we have to
consider that, we may as well reject with a clear message, which would
help the client writer. And speaking of clients..

On Mon, Mar 16, 2015 at 11:08 PM, Dongcan Jiang <dongcan.jiang@gmail.com> wrote:
>>> -             if (starts_with(line, "deepen ")) {
>>> +             if (starts_with(line, "depth ")) {
>>>                       char *end;
>>> -                     depth = strtol(line + 7, &end, 0);
>>> -                     if (end == line + 7 || depth <= 0)
>>> -                             die("Invalid deepen: %s", line);
>>> +                     depth = strtol(line + 6, &end, 0);
>>> +                     if (end == line + 6 || depth <= 0)
>>> +                             die("Invalid depth: %s", line);
>>>                       continue;
>>>               }
>>>               if (!starts_with(line, "want ") ||
>>
>> I do not quite understand this hunk.  What happens when this program
>> is talking to an existing fetch-pack that requested "deepen"?
>
> In this hunk, I actually just changed the one command name in upload-pack
> service from "deepen" to "depth". I made this change because the name
> "deepen" here is quite misleading, as it just means "depth". Of course,
> I've changed the caller's sent pack from "deepen" to "depth" too (See below).

You missed Junio's keyword "existing". Your new client will work well
with your new server. But it breaks "git clone --depth" (or "git fetch
--depth") for all existing git installations.
-- 
Duy