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

[Dongcan Jiang <dongcan.jiang@gmail.com>]

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

Actually, I mean that the upload-pack in this patch will not crash,
whatever it receives.
e.g. "depth N", "depth_deepen", "depth N"+"depth_deepen"

Because "depth_deepen" is just a boolean signal to tell the upload-pack
that "depth N" means "deepen N", it takes effect only when "depth N"
is given. Otherwise, "depth_deepen" will be ignored.


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

Oh, thank you for pointing out this. And sorry Junio. I misunderstood
what you said. I haven't realized the problem of existing git server.
You've reminded me of the importance of compatibility. Thank you!

2015-03-17 18:44 GMT+08:00 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



-- 
江东灿（Dongcan Jiang）
Team of Search Engine & Web Mining
School of Electronic Engineering & Computer Science
Peking University, Beijing, 100871, P.R.China