Re: [PATCH/RFC] upload-pack: ignore 'shallow' lines with unknown obj-ids

From: Duy Nguyen <pclouds@gmail.com>
Date: Apr 20, 2013, 23:56 UTC
Message-ID: <CACsJy8CT8r7ebE_q5ET+oicTY0NM4LAt1B61xwqfL8tLKc-4OA@mail.gmail.com>
In-Reply-To: <7v7gjxx6w3.fsf@alter.siamese.dyndns.org>

On Sun, Apr 21, 2013 at 6:51 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 23 quoted lines

> Duy Nguyen <pclouds@gmail.com> writes:
> But the shallow list is also used to compute the updated boundary
> (i.e. "this client does not have a valid history behind these
> commits")?  When we know their current shallow boundary, after
> sending history that crosses that boundary, we can tell them "before
> this fetch, you did not have any history behind this commit, but we
> know that you now have a bit more. Update your record with these new
> boundaries. You still do not have any history behind these commits."
> That is how deepening works.
>
> When you receive a shallow boundary unknown to you, it might not
> hurt if you keep it and parrot it at the end, so that the fetcher
> will still remember that it does not have any history behind the
> commit.  There may be reasons why doing so is not sufficient and we
> have to reject clients whose history is truncated at a place unknown
> to us.
>
> You would declare "now you have everything behind that unknown
> shallow boundary", if you ignore an unknown shallow boundary and do
> not send it back.
>
> That sounds ourright wrong to me. You simply do not know enough to
> make such a declaration.

It's a good point. But I think the receiver does not rely solely on "shallow " lines from the sender to create new shallow file. If it receives "shallow " line, it registers the received sha-1 as a cut point. If it receives "unshallow " line, it unregisters. If it receives neither, the current registered cutpoints in memory are simply written back to disk. So not sending it back should not be a big problem (at least for C Git).

> I do not seem to find the patch you are responding to, so I do not
> know how the patch handled the unshallowing part, but the impression
> I got from reading the log message quoted is that the patch was not
> even aware of the issue.

I can't find it on gmane.org either. Patch quoted below.

On Sat, Apr 20, 2013 at 8:05 PM, Michael Heemskerk <mheemskerk@atlassian.com> wrote:

Show 32 quoted lines

> diff --git a/Documentation/technical/pack-protocol.txt
> b/Documentation/technical/pack-protocol.txt
> index f1a51ed..b898e97 100644
> --- a/Documentation/technical/pack-protocol.txt
> +++ b/Documentation/technical/pack-protocol.txt
> @@ -228,8 +228,7 @@ obtained through ref discovery.
>  The client MUST write all obj-ids which it only has shallow copies
>  of (meaning that it does not have the parents of a commit) as
>  'shallow' lines so that the server is aware of the limitations of
> -the client's history. Clients MUST NOT mention an obj-id which
> -it does not know exists on the server.
> +the client's history.
>
>  The client now sends the maximum commit history depth it wants for
>  this transaction, which is the number of commits it wants from the
> diff --git a/upload-pack.c b/upload-pack.c
> index bfa6279..127e59a 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -592,7 +592,7 @@ static void receive_needs(void)
>                                 die("invalid shallow line: %s", line);
>                         object = parse_object(sha1);
>                         if (!object)
> -                               die("did not find object for %s", line);
> +                               continue;
>                         if (object->type != OBJ_COMMIT)
>                                 die("invalid shallow object %s",
> sha1_to_hex(sha1));
>                         if (!(object->flags & CLIENT_SHALLOW)) {
> --
> 1.8.0.2
>

-- Duy