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@xxxxxxxxx>:
> On Fri, Mar 13, 2015 at 8:04 PM, Dongcan Jiang <dongcan.jiang@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html