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

From: Duy Nguyen <pclouds@gmail.com>
Date: Mar 17, 2015, 10:44 UTC
Message-ID: <CACsJy8AO1w0tYmYkOLjB6osw50gYpc01_6iUt5JZLqL0xtDPrw@mail.gmail.com>
In-Reply-To: <CABwkPcrF9oFvNkbp6rFV28U3w-szyV9k4LFviYyFkoJvp8pjbA@mail.gmail.com>

On Mon, Mar 16, 2015 at 11:10 PM, Dongcan Jiang <dongcan.jiang@gmail.com> wrote:

Show 14 quoted lines

> 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:

Show 20 quoted lines

>>> -             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