{"thread":{"id":"33551","subject":"Re: [PATCH/RFC] upload-pack: ignore 'shallow' lines with unknown obj-ids","startedAt":"2013-04-20T10:34:41Z","lastAt":"2013-04-29T05:39:50Z","messageCount":7,"participants":["Duy Nguyen","Junio C Hamano","Michael Heemskerk"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"214909","messageId":"CACsJy8B6OYC-Qcwc53BsVtUSHw1ag6LWF2rBSP6agO6yTXQo+g@mail.gmail.com","threadId":"33551","inReplyTo":"CAJDSCnN6Ekp3wF9hX9Dbt3+CLNg1_aBz8nPGUuCu0WS9MF8aXA@mail.gmail.com","subject":"Re: [PATCH/RFC] upload-pack: ignore 'shallow' lines with unknown obj-ids","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-04-20T10:34:41Z","receivedAt":"2013-04-20T10:34:41Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Apr 20, 2013 at 8:05 PM, Michael Heemskerk\n wrote:\n> When the client sends a 'shallow' line for an object that the server does\n> not have, the server currently dies with the error: \"did not find object\n> for shallow \". The client may have received the object from a\n> different server, or the object may have been garbage collected by the\n> server. In either case, the object is not relevant for calculating the pack\n> that is to be sent and can be safely ignored.\n>\n> The documentation in technical/pack-protocol.txt has been updated to\n> remove the restriction that \"Clients MUST NOT mention an obj-id which it\n> does not know exists on the server\". This requirement is not realistic\n> because clients cannot know whether an object has been garbage collected\n> by the server.\n>\n> Signed-off-by: Michael Heemskerk \n\nI'm not an expert in git protocol (or shallow extension in particular)\nbut the reasoning sounds right and the implemention looks correct.\n\nAcked-by: Duy Nguyen \n--\nDuy\n"},{"id":"214942","messageId":"7v7gjxx6w3.fsf@alter.siamese.dyndns.org","threadId":"33551","inReplyTo":"CACsJy8B6OYC-Qcwc53BsVtUSHw1ag6LWF2rBSP6agO6yTXQo+g@mail.gmail.com","subject":"Re: [PATCH/RFC] upload-pack: ignore 'shallow' lines with unknown obj-ids","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-20T20:51:40Z","receivedAt":"2013-04-20T20:51:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Sat, Apr 20, 2013 at 8:05 PM, Michael Heemskerk\n> wrote:\n>> When the client sends a 'shallow' line for an object that the server does\n>> not have, the server currently dies with the error: \"did not find object\n>> for shallow \". The client may have received the object from a\n>> different server, or the object may have been garbage collected by the\n>> server. In either case, the object is not relevant for calculating the pack\n>> that is to be sent and can be safely ignored.\n\nFor the purpose of generating a pack, these shallow boundaries are\nirrelevant. The above describes only that part.\n\nBut the shallow list is also used to compute the updated boundary\n(i.e. \"this client does not have a valid history behind these\ncommits\")? When we know their current shallow boundary, after\nsending history that crosses that boundary, we can tell them \"before\nthis fetch, you did not have any history behind this commit, but we\nknow that you now have a bit more. Update your record with these new\nboundaries. You still do not have any history behind these commits.\"\nThat is how deepening works.\n\nWhen you receive a shallow boundary unknown to you, it might not\nhurt if you keep it and parrot it at the end, so that the fetcher\nwill still remember that it does not have any history behind the\ncommit. There may be reasons why doing so is not sufficient and we\nhave to reject clients whose history is truncated at a place unknown\nto us.\n\nYou would declare \"now you have everything behind that unknown\nshallow boundary\", if you ignore an unknown shallow boundary and do\nnot send it back.\n\nThat sounds ourright wrong to me. You simply do not know enough to\nmake such a declaration.\n\nI do not seem to find the patch you are responding to, so I do not\nknow how the patch handled the unshallowing part, but the impression\nI got from reading the log message quoted is that the patch was not\neven aware of the issue.\n"},{"id":"214949","messageId":"CACsJy8CT8r7ebE_q5ET+oicTY0NM4LAt1B61xwqfL8tLKc-4OA@mail.gmail.com","threadId":"33551","inReplyTo":"7v7gjxx6w3.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH/RFC] upload-pack: ignore 'shallow' lines with unknown obj-ids","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-04-20T23:56:15Z","receivedAt":"2013-04-20T23:56:15Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Apr 21, 2013 at 6:51 AM, Junio C Hamano wrote:\n> Duy Nguyen writes:\n> But the shallow list is also used to compute the updated boundary\n> (i.e. \"this client does not have a valid history behind these\n> commits\")? When we know their current shallow boundary, after\n> sending history that crosses that boundary, we can tell them \"before\n> this fetch, you did not have any history behind this commit, but we\n> know that you now have a bit more. Update your record with these new\n> boundaries. You still do not have any history behind these commits.\"\n> That is how deepening works.\n>\n> When you receive a shallow boundary unknown to you, it might not\n> hurt if you keep it and parrot it at the end, so that the fetcher\n> will still remember that it does not have any history behind the\n> commit. There may be reasons why doing so is not sufficient and we\n> have to reject clients whose history is truncated at a place unknown\n> to us.\n>\n> You would declare \"now you have everything behind that unknown\n> shallow boundary\", if you ignore an unknown shallow boundary and do\n> not send it back.\n>\n> That sounds ourright wrong to me. You simply do not know enough to\n> make such a declaration.\n\nIt's a good point. But I think the receiver does not rely solely on\n\"shallow \" lines from the sender to create new shallow file. If it\nreceives \"shallow \" line, it registers the received sha-1 as a cut\npoint. If it receives \"unshallow \" line, it unregisters. If it\nreceives neither, the current registered cutpoints in memory are\nsimply written back to disk. So not sending it back should not be a\nbig problem (at least for C Git).\n\n> I do not seem to find the patch you are responding to, so I do not\n> know how the patch handled the unshallowing part, but the impression\n> I got from reading the log message quoted is that the patch was not\n> even aware of the issue.\n\nI can't find it on gmane.org either. Patch quoted below.\n\nOn Sat, Apr 20, 2013 at 8:05 PM, Michael Heemskerk\n wrote:\n> diff --git a/Documentation/technical/pack-protocol.txt\n> b/Documentation/technical/pack-protocol.txt\n> index f1a51ed..b898e97 100644\n> --- a/Documentation/technical/pack-protocol.txt\n> +++ b/Documentation/technical/pack-protocol.txt\n> @@ -228,8 +228,7 @@ obtained through ref discovery.\n> The client MUST write all obj-ids which it only has shallow copies\n> of (meaning that it does not have the parents of a commit) as\n> 'shallow' lines so that the server is aware of the limitations of\n> -the client's history. Clients MUST NOT mention an obj-id which\n> -it does not know exists on the server.\n> +the client's history.\n>\n> The client now sends the maximum commit history depth it wants for\n> this transaction, which is the number of commits it wants from the\n> diff --git a/upload-pack.c b/upload-pack.c\n> index bfa6279..127e59a 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -592,7 +592,7 @@ static void receive_needs(void)\n> die(\"invalid shallow line: %s\", line);\n> object = parse_object(sha1);\n> if (!object)\n> - die(\"did not find object for %s\", line);\n> + continue;\n> if (object->type != OBJ_COMMIT)\n> die(\"invalid shallow object %s\",\n> sha1_to_hex(sha1));\n> if (!(object->flags & CLIENT_SHALLOW)) {\n> --\n> 1.8.0.2\n>\n--\nDuy\n"},{"id":"214951","messageId":"CAJDSCnN_Va7r2BMLYSbsSzoUNs6YfYfihatb-u4jJw25w8qvZg@mail.gmail.com","threadId":"33551","inReplyTo":"CAJDSCnPt=wgT0UQirahvTyen9w_v4XTn0ABHJVCLeKLmC8BsHQ@mail.gmail.com","subject":"Fwd: [PATCH/RFC] upload-pack: ignore 'shallow' lines with unknown obj-ids","fromName":"Michael Heemskerk","fromEmail":"mheemskerk@atlassian.com","sentAt":"2013-04-21T01:08:27Z","receivedAt":"2013-04-21T01:08:27Z","isPatch":true,"sender":{"key":"mheemskerk@atlassian.com","avatar":"https://gravatar.com/avatar/c3ebe086c3c55529246291e9879e5ef7af3c0ffcadbd81b5508b5fcdf66aa065?d=mp&s=160"},"body":"Re-sent to the mailing list because the original was bounced (HTML subpart):\n\nOn 21 April 2013 09:56, Duy Nguyen wrote:\n>\n> On Sun, Apr 21, 2013 at 6:51 AM, Junio C Hamano wrote:\n> > Duy Nguyen writes:\n> > But the shallow list is also used to compute the updated boundary\n> > (i.e. \"this client does not have a valid history behind these\n> > commits\")? When we know their current shallow boundary, after\n> > sending history that crosses that boundary, we can tell them \"before\n> > this fetch, you did not have any history behind this commit, but we\n> > know that you now have a bit more. Update your record with these new\n> > boundaries. You still do not have any history behind these commits.\"\n> > That is how deepening works.\n> >\n> > When you receive a shallow boundary unknown to you, it might not\n> > hurt if you keep it and parrot it at the end, so that the fetcher\n> > will still remember that it does not have any history behind the\n> > commit. There may be reasons why doing so is not sufficient and we\n> > have to reject clients whose history is truncated at a place unknown\n> > to us.\n> >\n> > You would declare \"now you have everything behind that unknown\n> > shallow boundary\", if you ignore an unknown shallow boundary and do\n> > not send it back.\n> >\n> > That sounds ourright wrong to me. You simply do not know enough to\n> > make such a declaration.\n>\n> It's a good point. But I think the receiver does not rely solely on\n> \"shallow \" lines from the sender to create new shallow file. If it\n> receives \"shallow \" line, it registers the received sha-1 as a cut\n> point. If it receives \"unshallow \" line, it unregisters. If it\n> receives neither, the current registered cutpoints in memory are\n> simply written back to disk. So not sending it back should not be a\n> big problem (at least for C Git).\n\n\nWith the patch applied, the server ignores the shallow line mentioned by\nthe server and will not send a \"shallow\" or \"unshallow\" line for it back to\nthe client. This scenario is not explicitly described in pack-protocol.txt\nbut I'd be happy to add it to. I'll also update the comment to cover this\naspect.\n\nAs Duy pointed out, it doesn't cause problems in the current C Git\nimplementation: the client adds a new entry to the shallow file for each\n\"shallow\" line it receives from the server and removes an entry for each\n\"unshallow\" line it receives. Any current shallow object that is not\nmentioned by the server is still marked as shallow after the fetch.\n\nI think that's how it should be: it should be the client's\nresponsibility to track\nthe list of objects it only has in shallow form. It should not rely on\nthe server\nto tell it what that list is. Again, an extra line or two in pack-protocol.txt\nwould help to clear this up.\n\nIn testing this patch, I also discovered that the shallow file is not updated\nas part of a prune operation. Arguably, any pruned commits should also\nbe removed from the shallow file. I haven't included a fix for this in my\npatch because it's an existing issue that is best fixed in a separate patch\n(in progress).\n\n>\n> > I do not seem to find the patch you are responding to, so I do not\n> > know how the patch handled the unshallowing part, but the impression\n> > I got from reading the log message quoted is that the patch was not\n> > even aware of the issue.\n>\n> I can't find it on gmane.org either. Patch quoted below.\n>\n> On Sat, Apr 20, 2013 at 8:05 PM, Michael Heemskerk\n> wrote:\n> > diff --git a/Documentation/technical/pack-protocol.txt\n> > b/Documentation/technical/pack-protocol.txt\n> > index f1a51ed..b898e97 100644\n> > --- a/Documentation/technical/pack-protocol.txt\n> > +++ b/Documentation/technical/pack-protocol.txt\n> > @@ -228,8 +228,7 @@ obtained through ref discovery.\n> > The client MUST write all obj-ids which it only has shallow copies\n> > of (meaning that it does not have the parents of a commit) as\n> > 'shallow' lines so that the server is aware of the limitations of\n> > -the client's history. Clients MUST NOT mention an obj-id which\n> > -it does not know exists on the server.\n> > +the client's history.\n> >\n> > The client now sends the maximum commit history depth it wants for\n> > this transaction, which is the number of commits it wants from the\n> > diff --git a/upload-pack.c b/upload-pack.c\n> > index bfa6279..127e59a 100644\n> > --- a/upload-pack.c\n> > +++ b/upload-pack.c\n> > @@ -592,7 +592,7 @@ static void receive_needs(void)\n> > die(\"invalid shallow line: %s\", line);\n> > object = parse_object(sha1);\n> > if (!object)\n> > - die(\"did not find object for %s\", line);\n> > + continue;\n> > if (object->type != OBJ_COMMIT)\n> > die(\"invalid shallow object %s\",\n> > sha1_to_hex(sha1));\n> > if (!(object->flags & CLIENT_SHALLOW)) {\n> > --\n> > 1.8.0.2\n> >\n> --\n> Duy\n"},{"id":"215816","messageId":"7v7gjmow6o.fsf@alter.siamese.dyndns.org","threadId":"33551","inReplyTo":"CAJDSCnN_Va7r2BMLYSbsSzoUNs6YfYfihatb-u4jJw25w8qvZg@mail.gmail.com","subject":"Re: Fwd: [PATCH/RFC] upload-pack: ignore 'shallow' lines with unknown obj-ids","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-29T03:25:03Z","receivedAt":"2013-04-29T03:25:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Heemskerk writes:\n\n> Re-sent to the mailing list because the original was bounced (HTML subpart):\n> ...\n>\n> With the patch applied, the server ignores the shallow line mentioned by\n> the server and will not send a \"shallow\" or \"unshallow\" line for it back to\n> the client. This scenario is not explicitly described in pack-protocol.txt\n> but I'd be happy to add it to. I'll also update the comment to cover this\n> aspect.\n>\n> As Duy pointed out, it doesn't cause problems in the current C Git\n> implementation: the client adds a new entry to the shallow file for each\n> \"shallow\" line it receives from the server and removes an entry for each\n> \"unshallow\" line it receives. Any current shallow object that is not\n> mentioned by the server is still marked as shallow after the fetch.\n>\n> I think that's how it should be: it should be the client's\n> responsibility to track\n> the list of objects it only has in shallow form. It should not rely on\n> the server\n> to tell it what that list is. Again, an extra line or two in pack-protocol.txt\n> would help to clear this up.\n> ...\n>> > I do not seem to find the patch you are responding to, so I do not\n>> > know how the patch handled the unshallowing part, but the impression\n>> > I got from reading the log message quoted is that the patch was not\n>> > even aware of the issue.\n>>\n>> I can't find it on gmane.org either. Patch quoted below.\n\nOK, the change the patch text shows looks sensible to me. Can you\nresend it in full, with the log message, your sign-off, and Duy's\n\"Reviewed-by:\", so that it can be applied?\n\nThanks.\n\n>> On Sat, Apr 20, 2013 at 8:05 PM, Michael Heemskerk\n>> wrote:\n>> > diff --git a/Documentation/technical/pack-protocol.txt\n>> > b/Documentation/technical/pack-protocol.txt\n>> > index f1a51ed..b898e97 100644\n>> > --- a/Documentation/technical/pack-protocol.txt\n>> > +++ b/Documentation/technical/pack-protocol.txt\n>> > @@ -228,8 +228,7 @@ obtained through ref discovery.\n>> ...\n"},{"id":"215827","messageId":"7v7gjloqaz.fsf@alter.siamese.dyndns.org","threadId":"33551","inReplyTo":"7v7gjmow6o.fsf@alter.siamese.dyndns.org","subject":"Re: Fwd: [PATCH/RFC] upload-pack: ignore 'shallow' lines with unknown obj-ids","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-29T05:32:04Z","receivedAt":"2013-04-29T05:32:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> OK, the change the patch text shows looks sensible to me. Can you\n> resend it in full, with the log message, your sign-off, and Duy's\n> \"Reviewed-by:\", so that it can be applied?\n\nI managed to reassemble what I _think_ is close to the original from\na few messages by dequoting and fixing MUA whitespace breakages in\nthem.\n\nHere is with a slight tweak to the log message. Please holler if I\ngrabbed a wrong version of the patch or made any silly mistakes.\n\nThanks.\n\n-- >8 --\nFrom: Michael Heemskerk \nSubject: [PATCH] upload-pack: ignore 'shallow' lines with unknown obj-ids\n\nWhen the client sends a 'shallow' line for an object that the server does\nnot have, the server currently dies with the error: \"did not find object\nfor shallow \". The client may have truncated the history at\nthe commit by fetching shallowly from a different server, or the commit\nmay have been garbage collected by the server. In either case, this\nunknown commit is not relevant for calculating the pack that is to be\nsent and can be safely ignored, and it is not used when recomputing where\nthe updated history of the client is cauterised.\n\nThe documentation in technical/pack-protocol.txt has been updated to\nremove the restriction that \"Clients MUST NOT mention an obj-id which it\ndoes not know exists on the server\". This requirement is not realistic\nbecause clients cannot know whether an object has been garbage collected\nby the server.\n\nSigned-off-by: Michael Heemskerk \nReviewed-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: Junio C Hamano \n---\n Documentation/technical/pack-protocol.txt | 3 +--\n upload-pack.c | 2 +-\n 2 files changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt\nindex f1a51ed..b898e97 100644\n--- a/Documentation/technical/pack-protocol.txt\n+++ b/Documentation/technical/pack-protocol.txt\n@@ -228,8 +228,7 @@ obtained through ref discovery.\n The client MUST write all obj-ids which it only has shallow copies\n of (meaning that it does not have the parents of a commit) as\n 'shallow' lines so that the server is aware of the limitations of\n-the client's history. Clients MUST NOT mention an obj-id which\n-it does not know exists on the server.\n+the client's history.\n \n The client now sends the maximum commit history depth it wants for\n this transaction, which is the number of commits it wants from the\ndiff --git a/upload-pack.c b/upload-pack.c\nindex bfa6279..127e59a 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -592,7 +592,7 @@ static void receive_needs(void)\n \t\t\t\tdie(\"invalid shallow line: %s\", line);\n \t\t\tobject = parse_object(sha1);\n \t\t\tif (!object)\n-\t\t\t\tdie(\"did not find object for %s\", line);\n+\t\t\t\tcontinue;\n \t\t\tif (object->type != OBJ_COMMIT)\n \t\t\t\tdie(\"invalid shallow object %s\", sha1_to_hex(sha1));\n \t\t\tif (!(object->flags & CLIENT_SHALLOW)) {\n-- \n1.8.3-rc0-117-g5915a95\n"},{"id":"215829","messageId":"CAJDSCnOw6=iD1UiwjB3fmsapKq9Ai3bCcQ1qr_dCucEehw9mYA@mail.gmail.com","threadId":"33551","inReplyTo":"7v7gjloqaz.fsf@alter.siamese.dyndns.org","subject":"Re: Fwd: [PATCH/RFC] upload-pack: ignore 'shallow' lines with unknown obj-ids","fromName":"Michael Heemskerk","fromEmail":"mheemskerk@atlassian.com","sentAt":"2013-04-29T05:39:50Z","receivedAt":"2013-04-29T05:39:50Z","isPatch":true,"sender":{"key":"mheemskerk@atlassian.com","avatar":"https://gravatar.com/avatar/c3ebe086c3c55529246291e9879e5ef7af3c0ffcadbd81b5508b5fcdf66aa065?d=mp&s=160"},"body":"Thanks Junio,\n\nThat looks fine to me. I'll try to find some time this week to create a\nfollow-up patch for removing pruned commits from the shallow file as\nwell.\n\nCheers,\nMichael\n\nOn 29 April 2013 15:32, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> OK, the change the patch text shows looks sensible to me. Can you\n>> resend it in full, with the log message, your sign-off, and Duy's\n>> \"Reviewed-by:\", so that it can be applied?\n>\n> I managed to reassemble what I _think_ is close to the original from\n> a few messages by dequoting and fixing MUA whitespace breakages in\n> them.\n>\n> Here is with a slight tweak to the log message. Please holler if I\n> grabbed a wrong version of the patch or made any silly mistakes.\n>\n> Thanks.\n>\n> -- >8 --\n> From: Michael Heemskerk \n> Subject: [PATCH] upload-pack: ignore 'shallow' lines with unknown obj-ids\n>\n> When the client sends a 'shallow' line for an object that the server does\n> not have, the server currently dies with the error: \"did not find object\n> for shallow \". The client may have truncated the history at\n> the commit by fetching shallowly from a different server, or the commit\n> may have been garbage collected by the server. In either case, this\n> unknown commit is not relevant for calculating the pack that is to be\n> sent and can be safely ignored, and it is not used when recomputing where\n> the updated history of the client is cauterised.\n>\n> The documentation in technical/pack-protocol.txt has been updated to\n> remove the restriction that \"Clients MUST NOT mention an obj-id which it\n> does not know exists on the server\". This requirement is not realistic\n> because clients cannot know whether an object has been garbage collected\n> by the server.\n>\n> Signed-off-by: Michael Heemskerk \n> Reviewed-by: Nguyễn Thái Ngọc Duy \n> Signed-off-by: Junio C Hamano \n> ---\n> Documentation/technical/pack-protocol.txt | 3 +--\n> upload-pack.c | 2 +-\n> 2 files changed, 2 insertions(+), 3 deletions(-)\n>\n> diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt\n> index f1a51ed..b898e97 100644\n> --- a/Documentation/technical/pack-protocol.txt\n> +++ b/Documentation/technical/pack-protocol.txt\n> @@ -228,8 +228,7 @@ obtained through ref discovery.\n> The client MUST write all obj-ids which it only has shallow copies\n> of (meaning that it does not have the parents of a commit) as\n> 'shallow' lines so that the server is aware of the limitations of\n> -the client's history. Clients MUST NOT mention an obj-id which\n> -it does not know exists on the server.\n> +the client's history.\n>\n> The client now sends the maximum commit history depth it wants for\n> this transaction, which is the number of commits it wants from the\n> diff --git a/upload-pack.c b/upload-pack.c\n> index bfa6279..127e59a 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -592,7 +592,7 @@ static void receive_needs(void)\n> die(\"invalid shallow line: %s\", line);\n> object = parse_object(sha1);\n> if (!object)\n> - die(\"did not find object for %s\", line);\n> + continue;\n> if (object->type != OBJ_COMMIT)\n> die(\"invalid shallow object %s\", sha1_to_hex(sha1));\n> if (!(object->flags & CLIENT_SHALLOW)) {\n> --\n> 1.8.3-rc0-117-g5915a95\n>\n"}]}