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

From: Dongcan Jiang <dongcan.jiang@gmail.com>
Date: Mar 16, 2015, 16:10 UTC
Message-ID: <CABwkPcrF9oFvNkbp6rFV28U3w-szyV9k4LFviYyFkoJvp8pjbA@mail.gmail.com>
In-Reply-To: <CACsJy8AAsAO6gjWg0nsdiOXFEsZ4583pH9mkUx1Js3Yop5KHHQ@mail.gmail.com>

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.

> I was about to suggest you use "deepen" for both --depth and
> --deepen: --depth sends "deepen NUM" while --deepen sends "deepen
> +NUM". The protocol as described in pack-protocol.txt may allow this
> extension.

This suggestion looks neat! However, I'm afraid it may involves quite a bit changes as you've mentioned, which would be better to take in next stage.

Thanks, Dongcan

2015-03-14 18:56 GMT+08:00 Duy Nguyen <pclouds@gmail.com>:

Show 36 quoted lines

> On Fri, Mar 13, 2015 at 8:04 PM, Dongcan Jiang <dongcan.jiang@gmail.com> wrote:
>> @@ -317,7 +318,7 @@ static int find_common(struct fetch_pack_args *args,
>>         if (is_repository_shallow())
>>                 write_shallow_commits(&req_buf, 1, NULL);
>>         if (args->depth > 0)
>> -               packet_buf_write(&req_buf, "deepen %d", args->depth);
>> +               packet_buf_write(&req_buf, "depth %d", args->depth);
>>         packet_buf_flush(&req_buf);
>>         state_len = req_buf.len;
>
> Junio already questioned about your replacing starts_with("deepen "..)
> with starts_with("depth "..), this is part of that question. If you
> run "make test" you should find some breakages, or we need to improve
> our test suite.
>
> I think you just need to keep the old "if (args->depth > 0) send
> "depth" unchanged and add two new statements that check
> args->depth_deepen and sends "depth ". BTW, I think "deepen-more" or
> "deepen-relative" would be better than "depth" (*), which is a noun.
> But that's a minor point at this stage.
>
> And because --depth and --deepen are mutually exclusive, you need to
> make sure that the user must not specify that. The "user" includes the
> "client" from the server perspective, we need to make sure that
> upload-pack barf if some client sends both "deepen" and "depth".
>
> (*) I was about to suggest you use "deepen" for both --depth and
> --deepen: --depth sends "deepen NUM" while --deepen sends "deepen
> +NUM". The protocol as described in pack-protocol.txt may allow this
> extension. Unfortunately the current implementation is too relaxed. We
> use strtol() to parse "NUM" and it would accept the leading "+"
> instead of barfing out. So old clients would mistakes --deepen as
> --depth, not good. And it even accepts base 8 and 16!! We should fix
> this.
> --
> Duy

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