Re: [PATCH/GSoC/RFC] fetch: git fetch --deepen
- From
Dongcan Jiang <dongcan.jiang@gmail.com>
- Date
- Mar 17, 2015, 12:00 UTC
- Message-ID
- <CABwkPcq2AawTCQLwfvyR8gbx4Z7BhA8vKd_ON3LSCVcxFX9b4w@mail.gmail.com>
- In-Reply-To
- <CACsJy8AO1w0tYmYkOLjB6osw50gYpc01_6iUt5JZLqL0xtDPrw@mail.gmail.com>
Show 5 quoted lines
> 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>:
Show 49 quoted lines
> 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