{"thread":{"id":"35080","subject":"[PATCH] send-pack: don't send a thin pack when the server doesn't support it","startedAt":"2013-10-08T08:44:44Z","lastAt":"2013-10-19T15:19:47Z","messageCount":6,"participants":["Carlos Martín Nieto","Duy Nguyen","Carlos Martin Nieto","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"228603","messageId":"1381221884-27048-1-git-send-email-cmn@elego.de","threadId":"35080","inReplyTo":null,"subject":"[PATCH] send-pack: don't send a thin pack when the server doesn't support it","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2013-10-08T08:44:44Z","receivedAt":"2013-10-08T08:44:44Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"Not every server out there supports fixing thin packs, so let's send\nthem a full pack.\n\nSigned-off-by: Carlos Martín Nieto \n---\n\nIt's not always possible to support thin packs (sometimes there isn't\neven an object database to grab bases out of). And in any case git\nshouldn't create thin packs if the server hasn't said it knows how to\nfix them, as per the point of the extension.\n\n send-pack.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex 7d172ef..7b88ac8 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -205,6 +205,8 @@ int send_pack(struct send_pack_args *args,\n \t\tquiet_supported = 1;\n \tif (server_supports(\"agent\"))\n \t\tagent_supported = 1;\n+\tif (!server_supports(\"thin-pack\"))\n+\t\targs->use_thin_pack = 0;\n \n \tif (!remote_refs) {\n \t\tfprintf(stderr, \"No refs in common and none specified; doing nothing.\\n\"\n-- \n1.8.4.561.g1c3d45d\n"},{"id":"228604","messageId":"CACsJy8AQ-719sbUfJW98q_HYit7ePfB6oN3v7XTX6fvC7HnixA@mail.gmail.com","threadId":"35080","inReplyTo":"1381221884-27048-1-git-send-email-cmn@elego.de","subject":"Re: [PATCH] send-pack: don't send a thin pack when the server doesn't support it","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-10-08T09:44:49Z","receivedAt":"2013-10-08T09:44:49Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Oct 8, 2013 at 3:44 PM, Carlos Martín Nieto wrote:\n> diff --git a/send-pack.c b/send-pack.c\n> index 7d172ef..7b88ac8 100644\n> --- a/send-pack.c\n> +++ b/send-pack.c\n> @@ -205,6 +205,8 @@ int send_pack(struct send_pack_args *args,\n> quiet_supported = 1;\n> if (server_supports(\"agent\"))\n> agent_supported = 1;\n> + if (!server_supports(\"thin-pack\"))\n> + args->use_thin_pack = 0;\n\nHmm.. I think this introduces a regression in C Git because\nreceive-pack never advertises \"thin-pack\" and so \"git push\" from now\non will never send thin packs. It's too late now to add thin-pack to\nreceive-pack, perhaps a new extension \"no-thin-pack\" for those\nservers? Alternatively, just run git push --no-thin (you'll need\nf7c815c (push: respect --no-thin - 2013-08-12) though).\n\n> if (!remote_refs) {\n> fprintf(stderr, \"No refs in common and none specified; doing nothing.\\n\"\n-- \nDuy\n"},{"id":"228606","messageId":"87li242fmu.fsf@flaca.cmartin.tk","threadId":"35080","inReplyTo":"CACsJy8AQ-719sbUfJW98q_HYit7ePfB6oN3v7XTX6fvC7HnixA@mail.gmail.com","subject":"Re: [PATCH] send-pack: don't send a thin pack when the server doesn't support it","fromName":"Carlos Martin Nieto","fromEmail":"cmn@elego.de","sentAt":"2013-10-08T10:58:33Z","receivedAt":"2013-10-08T10:58:33Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"Duy Nguyen writes:\n\nOn Tue, 2013-10-08 at 16:44 +0700, Duy Nguyen wrote:\n>On Tue, Oct 8, 2013 at 3:44 PM, Carlos Martín Nieto wrote:\n> > diff --git a/send-pack.c b/send-pack.c\n> > index 7d172ef..7b88ac8 100644\n> > --- a/send-pack.c\n> > +++ b/send-pack.c\n> > @@ -205,6 +205,8 @@ int send_pack(struct send_pack_args *args,\n> > quiet_supported = 1;\n> > if (server_supports(\"agent\"))\n> > agent_supported = 1;\n> > + if (!server_supports(\"thin-pack\"))\n> > + args->use_thin_pack = 0;\n> \n> Hmm.. I think this introduces a regression in C Git because\n> receive-pack never advertises \"thin-pack\" and so \"git push\" from now\n> on will never send thin packs. It's too late now to add thin-pack to\n> \nOh, I'd never noticed that when looking though the network traffic. This seems like something that breaks the compatibility that git otherwise tries so hard to maintain. How did it happen that it's fine for the client to assume that the server supports thin packs?\n\nreceive-pack, perhaps a new extension \"no-thin-pack\" for those\n> servers? Alternatively, just run git push --no-thin (you'll need\n> f7c815c (push: respect --no-thin - 2013-08-12) though).\n> \nYeah, I had an older version in my PATH and was bitten by that when\ntesting. Having --no-thin and the server's index-pack fail with missing\nbases is quite worrying when you're the one who wrote most of the\nserver-side code.\n\nHaving to remember to run 'git push --no-thin' when pushing to one\nparticular project is pretty bad experience as a user and I was hoping\nto avoid this with newer gits. We could advertise a \"no-thin-pack\"\nextension if a patch to support that would be accepted into mainline\ngit.\n\nCheers,\n cmn\n"},{"id":"228607","messageId":"CACsJy8AHAbDuz3yhsjJfRQUGg1zzx6qqoAf=1oNhbX1xPVithg@mail.gmail.com","threadId":"35080","inReplyTo":"CACsJy8AQ-719sbUfJW98q_HYit7ePfB6oN3v7XTX6fvC7HnixA@mail.gmail.com","subject":"Re: [PATCH] send-pack: don't send a thin pack when the server doesn't support it","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-10-08T11:29:09Z","receivedAt":"2013-10-08T11:29:09Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Oct 8, 2013 at 4:44 PM, Duy Nguyen wrote:\n> On Tue, Oct 8, 2013 at 3:44 PM, Carlos Martín Nieto wrote:\n>> diff --git a/send-pack.c b/send-pack.c\n>> index 7d172ef..7b88ac8 100644\n>> --- a/send-pack.c\n>> +++ b/send-pack.c\n>> @@ -205,6 +205,8 @@ int send_pack(struct send_pack_args *args,\n>> quiet_supported = 1;\n>> if (server_supports(\"agent\"))\n>> agent_supported = 1;\n>> + if (!server_supports(\"thin-pack\"))\n>> + args->use_thin_pack = 0;\n>\n> Hmm.. I think this introduces a regression in C Git because\n> receive-pack never advertises \"thin-pack\" and so \"git push\" from now\n> on will never send thin packs. It's too late now to add thin-pack to\n> receive-pack\n\nOr maybe it's not that late. How about you go with your patch and add\nthin-pack capability to receive-pack too?\n\nWhen new \"git push\" is used against old server, thin pack is disabled.\nBut that's not a big deal (I hope). The difference is traffic\nincreases a bit more, but cpu consumption on the server side is also a\nbit less. Pushes are usually small, unless you do initial push, which\nis complete anyway. So a bit more or less does not really matter in\npractice.\n\nWe could mitigate the regression a bit by checking agent string and\nenable thin-pack for those versions even if thin-pack is not\nadvertises. That goes back to v1.7.12. We may do the same for some\nother popular git servers. And this hack is maintained like 3-4 years,\nenough time for new versions to be deployed, then removed.\n\nHmm?\n-- \nDuy\n"},{"id":"228632","messageId":"20131008222215.GI9464@google.com","threadId":"35080","inReplyTo":"CACsJy8AHAbDuz3yhsjJfRQUGg1zzx6qqoAf=1oNhbX1xPVithg@mail.gmail.com","subject":"Re: [PATCH] send-pack: don't send a thin pack when the server doesn't support it","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-10-08T22:22:15Z","receivedAt":"2013-10-08T22:22:15Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Duy Nguyen wrote:\n\n> Or maybe it's not that late. How about you go with your patch and add\n> thin-pack capability to receive-pack too?\n>\n> When new \"git push\" is used against old server, thin pack is disabled.\n> But that's not a big deal (I hope).\n\nCould we have separate patches to introduce the server-side capability\nand then to request it in the client? That way, people with old\nservers can apply the patch introducing the capability if they want.\n\nThe new meaning of the \"thin-pack\" capability should also be\ndocumented in Documentation/technical/protocol-capabilities.txt.\n\nDone that way and with enough time between the server advertising the\ncapability and the client looking for it, it seems like a good idea.\n\nMy two cents,\nJonathan\n"},{"id":"229220","messageId":"1382195987.17156.7.camel@centaur.cmartin.tk","threadId":"35080","inReplyTo":"20131008222215.GI9464@google.com","subject":"Re: [PATCH] send-pack: don't send a thin pack when the server doesn't support it","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2013-10-19T15:19:47Z","receivedAt":"2013-10-19T15:19:47Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"On Tue, 2013-10-08 at 15:22 -0700, Jonathan Nieder wrote:\n> Duy Nguyen wrote:\n> \n> > Or maybe it's not that late. How about you go with your patch and add\n> > thin-pack capability to receive-pack too?\n> >\n> > When new \"git push\" is used against old server, thin pack is disabled.\n> > But that's not a big deal (I hope).\n> \n> Could we have separate patches to introduce the server-side capability\n> and then to request it in the client? That way, people with old\n> servers can apply the patch introducing the capability if they want.\n\nThat could work.\n\n> \n> The new meaning of the \"thin-pack\" capability should also be\n> documented in Documentation/technical/protocol-capabilities.txt.\n\nOh, right; the capability as described is only about the server being\nable to generate a thin pack. Wouldn't his mean that git shouldn't\nassume that *any* remote can fix thin packs, though? (other than most\nservers you do talk to happen to).\n\nAnyway, facts on the ground and all that. I'll prepare some \n\n> \n> Done that way and with enough time between the server advertising the\n> capability and the client looking for it, it seems like a good idea.\n\n\nIf such patches would be accepted, that would be great. By the time this\nall gets merged, we might have thin pack fixing merged into libgit2, but\nthere will still be uses where fixing them isn't an issue due to other\nconstraints.\n\n\nCheers,\n cmn\n"}]}