{"thread":{"id":"27346","subject":"[PATCH 2/2] receive-pack: Add receive.denyObjectLimit to refuse push with too many objects","startedAt":"2011-05-13T16:54:31Z","lastAt":"2011-05-18T20:24:37Z","messageCount":53,"participants":["Johan Herland","Junio C Hamano","Johannes Sixt","Shawn Pearce","Ævar Arnfjörð Bjarmason","Jeff King","Sverre Rabbelier"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"167814","messageId":"201105131854.31540.johan@herland.net","threadId":"27346","inReplyTo":null,"subject":"[PATCH 2/2] receive-pack: Add receive.denyObjectLimit to refuse push with too many objects","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-05-13T16:54:31Z","receivedAt":"2011-05-13T16:54:31Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"The new receive.denyObjectLimit config variable defines an upper limit on the\nnumber of objects to accept in a single push. If the number of objects in a\npush exceeds this limit, the entire push is immediately aborted without\nstoring the pushed objects on the server at all.\n\nThis is meant to prevent an unintended large push (typically a result of the\nuser not being aware of exactly what is being pushed, e.g. pushing a large\nrewritten history) from entering the repo.\n\nUsually, this kind of limit could be imposed by a pre-receive or update hook,\nbut both of those run _after_ the pack has been received and stored by\nreceive-pack, so they cannot prevent the pack from being transferred in the\nfirst place.\n\nDocumentation and tests are included.\n\nSigned-off-by: Johan Herland \n---\n\nThere's something weird going on here: The included test case works as\nintended, but when I try to test this in \"real-world\" conditions,\nI don't get the expected error message in the output:\n\n # From inside my git.git repo:\n $ git init --bare foo.git\n $ (cd foo.git && git config receive.denyObjectLimit 100000)\n $ git push foo.git/ master\n Pushing to foo.git/\n Counting objects: 112383, done.\n Delta compression using up to 8 threads.\n Compressing objects: 100% (27658/27658), done.\n error: pack-objects died of signal 13\n error: failed to push some refs to 'foo.git/'\n $\n\nSo the pack-objects on the local side dies of a broken pipe (as\nexpected), but the error message from the remote side:\n\n error: unpack failed: received pack exceeds configured receive.denyObjectLimit\n\nis never printed, so the user gets no clue as to why the push failed.\nThis should be fixed before this patch can be considered for inclusion.\n\n\n...Johan\n\n\n Documentation/config.txt | 9 +++++++++\n builtin/receive-pack.c | 10 +++++++++-\n t/t5400-send-pack.sh | 44 ++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 62 insertions(+), 1 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 285c7f7..7c60146 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1591,6 +1591,15 @@ receive.unpackLimit::\n \tespecially on slow filesystems. If not set, the value of\n \t`transfer.unpackLimit` is used instead.\n \n+receive.denyObjectLimit::\n+\tIf the number of objects received in a push exceeds this limit,\n+\tthen the entire push will be refused. This is meant to prevent\n+\tan unintended large push (typically a result of the user not\n+\tbeing aware of exactly what is being pushed, e.g. pushing a\n+\tlarge rewritten history) from entering the repo. If not set,\n+\tthere is no upper limit on the number of objects transferred\n+\tin a single push.\n+\n receive.denyDeletes::\n \tIf set to true, git-receive-pack will deny a ref update that deletes\n \tthe ref. Use this to prevent such a ref deletion via a push.\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex e1ba4dc..856650f 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -25,6 +25,7 @@ static int deny_non_fast_forwards;\n static enum deny_action deny_current_branch = DENY_UNCONFIGURED;\n static enum deny_action deny_delete_current = DENY_UNCONFIGURED;\n static int receive_fsck_objects;\n+static int receive_deny_limit;\n static int receive_unpack_limit = -1;\n static int transfer_unpack_limit = -1;\n static int unpack_limit = 100;\n@@ -63,6 +64,11 @@ static int receive_pack_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n+\tif (strcmp(var, \"receive.denyobjectlimit\") == 0) {\n+\t\treceive_deny_limit = git_config_int(var, value);\n+\t\treturn 0;\n+\t}\n+\n \tif (strcmp(var, \"receive.unpacklimit\") == 0) {\n \t\treceive_unpack_limit = git_config_int(var, value);\n \t\treturn 0;\n@@ -648,7 +654,9 @@ static const char *unpack(void)\n \t\t\t\"--pack_header=%\"PRIu32\",%\"PRIu32,\n \t\t\tntohl(hdr.hdr_version), ntohl(hdr.hdr_entries));\n \n-\tif (ntohl(hdr.hdr_entries) < unpack_limit) {\n+\tif (receive_deny_limit > 0 && ntohl(hdr.hdr_entries) > receive_deny_limit)\n+\t\treturn \"received pack exceeds configured receive.denyObjectLimit\";\n+\telse if (ntohl(hdr.hdr_entries) < unpack_limit) {\n \t\tint code, i = 0;\n \t\tconst char *unpacker[4];\n \t\tunpacker[i++] = \"unpack-objects\";\ndiff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh\nindex 0eace37..2fbe34b 100755\n--- a/t/t5400-send-pack.sh\n+++ b/t/t5400-send-pack.sh\n@@ -222,4 +222,48 @@ test_expect_success 'deny pushing to delete current branch' '\n \t)\n '\n \n+test_expect_success 'deny pushing when receive.denyObjectLimit is exceeded' '\n+\trewound_push_setup &&\n+\t(\n+\t cd parent &&\n+\t git config receive.denyObjectLimit 1\n+\t) &&\n+\t(\n+\t cd child &&\n+\t git reset --hard origin/master &&\n+\t echo three > file && git commit -a -m three &&\n+\t test_must_fail git send-pack ../parent master 2>errs &&\n+\t grep -q \"receive\\\\.denyObjectLimit\" errs\n+\t) &&\n+\tparent_head=$(cd parent && git rev-parse --verify master) &&\n+\tchild_head=$(cd child && git rev-parse --verify master) &&\n+\ttest \"$parent_head\" != \"$child_head\"\n+'\n+\n+test_expect_success 'repeated push failure proves that objects were not stored remotely' '\n+\t(\n+\t cd child &&\n+\t test_must_fail git send-pack ../parent master 2>errs &&\n+\t grep -q \"receive\\\\.denyObjectLimit\" errs\n+\t) &&\n+\tparent_head=$(cd parent && git rev-parse --verify master) &&\n+\tchild_head=$(cd child && git rev-parse --verify master) &&\n+\ttest \"$parent_head\" != \"$child_head\"\n+'\n+\n+test_expect_success 'increasing receive.denyObjectLimit allows the push' '\n+\t(\n+\t cd parent &&\n+\t git config receive.denyObjectLimit 10\n+\t) &&\n+\t(\n+\t cd child &&\n+\t git send-pack ../parent master 2>errs &&\n+\t test_must_fail grep -q \"receive\\\\.denyObjectLimit\" errs\n+\t) &&\n+\tparent_head=$(cd parent && git rev-parse --verify master) &&\n+\tchild_head=$(cd child && git rev-parse --verify master) &&\n+\ttest \"$parent_head\" = \"$child_head\"\n+'\n+\n test_done\n-- \n1.7.5.1.354.g761178\n\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"167816","messageId":"7vpqnm7cim.fsf@alter.siamese.dyndns.org","threadId":"27346","inReplyTo":"201105131854.31540.johan@herland.net","subject":"Re: [PATCH 2/2] receive-pack: Add receive.denyObjectLimit to refuse push with too many objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-13T17:09:53Z","receivedAt":"2011-05-13T17:09:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> The new receive.denyObjectLimit config variable defines an upper limit on the\n> number of objects to accept in a single push. If the number of objects in a\n> push exceeds this limit, the entire push is immediately aborted without\n> storing the pushed objects on the server at all.\n\nWhere does the error message go? Can clients pushing over various\ntransports receive the reason without your server consuming the data from\nthem? Don't you want to \"receive-in-core-and-discard\" instead?\n\nFor the purpose of \"preventing an accidental push\", I suspect that people\nwould expect you to limit either by number of commits (i.e. depth of\nhistory) or by the total size of the data being transferred.\n\nThe name \"objectlimit\" sounds as if you are doing the latter and we can\nuse \"200MB\" there, but you are only limiting by count, so it is somewhat\nmisleading. We would want to see \"count\" or \"number\" somewhere in its\nname.\n"},{"id":"167820","messageId":"4DCD766D.2080801@kdbg.org","threadId":"27346","inReplyTo":"201105131854.31540.johan@herland.net","subject":"Re: [PATCH 2/2] receive-pack: Add receive.denyObjectLimit to refuse push with too many objects","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2011-05-13T18:20:29Z","receivedAt":"2011-05-13T18:20:29Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 13.05.2011 18:54, schrieb Johan Herland:\n> There's something weird going on here: The included test case works as\n> intended, but when I try to test this in \"real-world\" conditions,\n> I don't get the expected error message in the output:\n> \n> # From inside my git.git repo:\n> $ git init --bare foo.git\n> $ (cd foo.git && git config receive.denyObjectLimit 100000)\n> $ git push foo.git/ master\n> Pushing to foo.git/\n> Counting objects: 112383, done.\n> Delta compression using up to 8 threads.\n> Compressing objects: 100% (27658/27658), done.\n> error: pack-objects died of signal 13\n> error: failed to push some refs to 'foo.git/'\n> $\n> \n> So the pack-objects on the local side dies of a broken pipe (as\n> expected), but the error message from the remote side:\n> \n> error: unpack failed: received pack exceeds configured receive.denyObjectLimit\n> \n> is never printed, so the user gets no clue as to why the push failed.\n\nThe error message is printed by receive_status(), called around line 350\nin builtin/send-pack.c. But when pack-object fails, then the\npack_objects() call around line 340 signals an error and an early-exit\nbranch is taken, and receive_status() is never called.\n\nIn the test case, only a small amount of data is produced by\npack-objects, so that it can exit successfully and quickly enough\nbecause the data fits into the pipe buffer. If the pack-objects process\nwere scheduled differently, there is a chance that it dies from SIGPIPE\nas well. So, you are just being lucky that the test case succeeds.\n\n-- Hannes\n"},{"id":"167831","messageId":"201105140343.48741.johan@herland.net","threadId":"27346","inReplyTo":"7vpqnm7cim.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] receive-pack: Add receive.denyObjectLimit to refuse push with too many objects","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-05-14T01:43:48Z","receivedAt":"2011-05-14T01:43:48Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Friday 13 May 2011, Junio C Hamano wrote:\n> Johan Herland writes:\n> > The new receive.denyObjectLimit config variable defines an upper limit\n> > on the number of objects to accept in a single push. If the number of\n> > objects in a push exceeds this limit, the entire push is immediately\n> > aborted without storing the pushed objects on the server at all.\n> \n> Where does the error message go? Can clients pushing over various\n> transports receive the reason without your server consuming the data from\n> them? Don't you want to \"receive-in-core-and-discard\" instead?\n\nYes. Will be fixed in the re-roll.\n\n> For the purpose of \"preventing an accidental push\", I suspect that people\n> would expect you to limit either by number of commits (i.e. depth of\n> history) or by the total size of the data being transferred.\n\nYes, I agree that limiting by #commits, or by pack size would be more \nintuitive. However, neither of those values are available to me at the point \nwhere I have to decide what to do with the pack data (only the pack header \nis available, and that only contains the object count).\n\n> The name \"objectlimit\" sounds as if you are doing the latter and we can\n> use \"200MB\" there, but you are only limiting by count, so it is somewhat\n> misleading. We would want to see \"count\" or \"number\" somewhere in its\n> name.\n\nAgreed. Will be renamed to receive.objectCountLimit in the re-roll.\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"167832","messageId":"201105140349.59289.johan@herland.net","threadId":"27346","inReplyTo":"4DCD766D.2080801@kdbg.org","subject":"Re: [PATCH 2/2] receive-pack: Add receive.denyObjectLimit to refuse push with too many objects","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-05-14T01:49:59Z","receivedAt":"2011-05-14T01:49:59Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Friday 13 May 2011, Johannes Sixt wrote:\n> Am 13.05.2011 18:54, schrieb Johan Herland:\n> > So the pack-objects on the local side dies of a broken pipe (as\n> > expected), but the error message from the remote side:\n> > \n> > error: unpack failed: received pack exceeds configured\n> > receive.denyObjectLimit\n> > \n> > is never printed, so the user gets no clue as to why the push failed.\n> \n> The error message is printed by receive_status(), called around line 350\n> in builtin/send-pack.c. But when pack-object fails, then the\n> pack_objects() call around line 340 signals an error and an early-exit\n> branch is taken, and receive_status() is never called.\n> \n> In the test case, only a small amount of data is produced by\n> pack-objects, so that it can exit successfully and quickly enough\n> because the data fits into the pipe buffer. If the pack-objects process\n> were scheduled differently, there is a chance that it dies from SIGPIPE\n> as well. So, you are just being lucky that the test case succeeds.\n\nThanks for the helpful explanation!\n\nIndeed (as Junio already suggested) it seems I must swallow and discard all \nthe data sent by the client, and there's no way to easily abort the transfer \n_and_ get the error message printed on the client side. Still, I can at \nleast prevent the pack from being stored server-side.\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"167833","messageId":"201105140403.09981.johan@herland.net","threadId":"27346","inReplyTo":"201105140343.48741.johan@herland.net","subject":"[PATCHv2 2/2] receive-pack: Add receive.objectCountLimit to refuse push with too many objects","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-05-14T02:03:09Z","receivedAt":"2011-05-14T02:03:09Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"The new receive.objectCountLimit config variable defines an upper limit\non the number of objects to accept in a single push. If the number of\nobjects in a push exceeds this limit, the entire push is discarded\nwithout storing the pushed objects on the server at all.\n\nThis is meant to prevent an unintended large push (typically a result\nof the user not being aware of exactly what is being pushed,\ne.g. pushing a large rewritten history) from entering the repo.\n\nUsually, this kind of limit could be imposed by a pre-receive or update\nhook, but both of those run _after_ the pack has been received and\nstored by receive-pack, so they cannot prevent the pack from being\nstored on the server.\n\nDocumentation and tests are included.\n\nImproved-by: Junio C Hamano \nSigned-off-by: Johan Herland \n---\n\nHere is the re-roll with \"receive-in-core-and-discard\", plus the config\nvariable rename.\n\n\nHave fun! :)\n\n...Johan\n\n\n Documentation/config.txt | 9 +++++++++\n builtin/receive-pack.c | 14 +++++++++++++-\n t/t5400-send-pack.sh | 44 ++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 66 insertions(+), 1 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 285c7f7..b356956 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1591,6 +1591,15 @@ receive.unpackLimit::\n \tespecially on slow filesystems. If not set, the value of\n \t`transfer.unpackLimit` is used instead.\n \n+receive.objectCountLimit::\n+\tIf the number of objects received in a push exceeds this limit,\n+\tthen the entire push will be refused. This is meant to prevent\n+\tan unintended large push (typically a result of the user not\n+\tbeing aware of exactly what is being pushed, e.g. pushing a\n+\tlarge rewritten history) from entering the repo. If not set,\n+\tthere is no upper limit on the number of objects transferred\n+\tin a single push.\n+\n receive.denyDeletes::\n \tIf set to true, git-receive-pack will deny a ref update that deletes\n \tthe ref. Use this to prevent such a ref deletion via a push.\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex e1ba4dc..f521c83 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -25,6 +25,7 @@ static int deny_non_fast_forwards;\n static enum deny_action deny_current_branch = DENY_UNCONFIGURED;\n static enum deny_action deny_delete_current = DENY_UNCONFIGURED;\n static int receive_fsck_objects;\n+static int receive_object_count_limit;\n static int receive_unpack_limit = -1;\n static int transfer_unpack_limit = -1;\n static int unpack_limit = 100;\n@@ -63,6 +64,11 @@ static int receive_pack_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n+\tif (strcmp(var, \"receive.objectcountlimit\") == 0) {\n+\t\treceive_object_count_limit = git_config_int(var, value);\n+\t\treturn 0;\n+\t}\n+\n \tif (strcmp(var, \"receive.unpacklimit\") == 0) {\n \t\treceive_unpack_limit = git_config_int(var, value);\n \t\treturn 0;\n@@ -648,7 +654,13 @@ static const char *unpack(void)\n \t\t\t\"--pack_header=%\"PRIu32\",%\"PRIu32,\n \t\t\tntohl(hdr.hdr_version), ntohl(hdr.hdr_entries));\n \n-\tif (ntohl(hdr.hdr_entries) < unpack_limit) {\n+\tif (receive_object_count_limit > 0 &&\n+\t ntohl(hdr.hdr_entries) > receive_object_count_limit) {\n+\t\tchar buf[1024];\n+\t\twhile (xread(0, buf, 1024) > 0)\n+\t\t\t; /* nothing */\n+\t\treturn \"received pack exceeds configured receive.objectCountLimit\";\n+\t} else if (ntohl(hdr.hdr_entries) < unpack_limit) {\n \t\tint code, i = 0;\n \t\tconst char *unpacker[4];\n \t\tunpacker[i++] = \"unpack-objects\";\ndiff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh\nindex 0eace37..9198019 100755\n--- a/t/t5400-send-pack.sh\n+++ b/t/t5400-send-pack.sh\n@@ -222,4 +222,48 @@ test_expect_success 'deny pushing to delete current branch' '\n \t)\n '\n \n+test_expect_success 'deny pushing when receive.objectCountLimit is exceeded' '\n+\trewound_push_setup &&\n+\t(\n+\t cd parent &&\n+\t git config receive.objectCountLimit 1\n+\t) &&\n+\t(\n+\t cd child &&\n+\t git reset --hard origin/master &&\n+\t echo three > file && git commit -a -m three &&\n+\t test_must_fail git send-pack ../parent master 2>errs &&\n+\t grep -q \"receive\\\\.objectCountLimit\" errs\n+\t) &&\n+\tparent_head=$(cd parent && git rev-parse --verify master) &&\n+\tchild_head=$(cd child && git rev-parse --verify master) &&\n+\ttest \"$parent_head\" != \"$child_head\"\n+'\n+\n+test_expect_success 'repeated push failure proves that objects were not stored remotely' '\n+\t(\n+\t cd child &&\n+\t test_must_fail git send-pack ../parent master 2>errs &&\n+\t grep -q \"receive\\\\.objectCountLimit\" errs\n+\t) &&\n+\tparent_head=$(cd parent && git rev-parse --verify master) &&\n+\tchild_head=$(cd child && git rev-parse --verify master) &&\n+\ttest \"$parent_head\" != \"$child_head\"\n+'\n+\n+test_expect_success 'increasing receive.objectCountLimit allows the push' '\n+\t(\n+\t cd parent &&\n+\t git config receive.objectCountLimit 10\n+\t) &&\n+\t(\n+\t cd child &&\n+\t git send-pack ../parent master 2>errs &&\n+\t test_must_fail grep -q \"receive\\\\.objectCountLimit\" errs\n+\t) &&\n+\tparent_head=$(cd parent && git rev-parse --verify master) &&\n+\tchild_head=$(cd child && git rev-parse --verify master) &&\n+\ttest \"$parent_head\" = \"$child_head\"\n+'\n+\n test_done\n-- \n1.7.5.rc1.3.g4d7b\n"},{"id":"167834","messageId":"BANLkTik_taBK+=nh+0WEUjp3AV_fC7e_dg@mail.gmail.com","threadId":"27346","inReplyTo":"201105140403.09981.johan@herland.net","subject":"Re: [PATCHv2 2/2] receive-pack: Add receive.objectCountLimit to refuse push with too many objects","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2011-05-14T02:30:30Z","receivedAt":"2011-05-14T02:30:30Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Fri, May 13, 2011 at 19:03, Johan Herland wrote:\n> The new receive.objectCountLimit config variable defines an upper limit\n> on the number of objects to accept in a single push. If the number of\n> objects in a push exceeds this limit, the entire push is discarded\n> without storing the pushed objects on the server at all.\n...\n> +           ntohl(hdr.hdr_entries) > receive_object_count_limit) {\n> +               char buf[1024];\n> +               while (xread(0, buf, 1024) > 0)\n> +                       ; /* nothing */\n> +               return \"received pack exceeds configured receive.objectCountLimit\";\n\nDiscarding in core is painful. We are still consuming bandwidth to\ntoss away the data.\n\nI wonder... should we instead export the objectCountLimit as part of\nthe advertisement to the client, and teach send-pack to look at this\nand pass it down to pack-objects? If pack-objects winds up with more\nthan this limit, it aborts and the client doesn't even transmit data.\nNewer clients would abort cleanly with a nice error.\n\nI don't see it as a problem to advertise to a client \"This server will\nonly accept X objects from you, sending X + 1 is an error and will be\nrejected.\" If we are worried about an evil client using this\nadvertisement to try and DoS a server... he can easily do that with a\nsingle giant blob. Or a very long delta chain of a single blob and\nmany, many tiny deltas applied onto it. Knowing what the remote's\nobjectCountLimit is doesn't increase the risk of a DoS attack.\n\nFor older clients that don't know this new advertised capability, they\nshould fail hard and not transfer all of this data. In my experience\nwhen a user gets these strange errors from his Git client, he contacts\nhis server administrator with the screen output. At which point the\nadministrator can see the Counting objects line, check the repository\nconfiguration, and tell the user what the problem is... and encourage\nthem to upgrade their client to a newer version.\n\nIf we are going to put limits in, does it make sense to try and push\nthese limits back to pack-objects in a more detailed way? You talked\nabout depth of history, or size of pack. pack-objects could\napproximate both. If its depth of history, it might even be able to\ncut off before it enumerates too many commits. :-)\n\nThe remote side obviously cannot abort early with number of commits or\npack size limits, but if those were soft limits suggested to a client,\nwhile the object count was a hard limit, you might get a better\napproximation for what you want. A server administrator might\nconfigure a soft limit of 10 commits, but a hard limit of 5,000\nobjects. For most users, a bad push would abort very early on the soft\nlimit of 10 commits if they did an incorrect rebase. Meanwhile a user\nwho made 1 commit but changed every GPL header in the linux-2.6\nrepository (26,000+ files) would also be stopped for exceeding the\n(hard) 5000 object limit.\n\n-- \nShawn.\n"},{"id":"167841","messageId":"201105141517.45324.johan@herland.net","threadId":"27346","inReplyTo":"BANLkTik_taBK+=nh+0WEUjp3AV_fC7e_dg@mail.gmail.com","subject":"Re: [PATCHv2 2/2] receive-pack: Add receive.objectCountLimit to refuse push with too many objects","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-05-14T13:17:45Z","receivedAt":"2011-05-14T13:17:45Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Saturday 14 May 2011, Shawn Pearce wrote:\n> I wonder... should we instead export the objectCountLimit as part of\n> the advertisement to the client, and teach send-pack to look at this\n> and pass it down to pack-objects? If pack-objects winds up with more\n> than this limit, it aborts and the client doesn't even transmit data.\n> Newer clients would abort cleanly with a nice error.\n\nGood idea (although it grows the scope from the quick-fix I initially \nintended it to be...)\n\nI'm planning to add a new capability collection/namespace, called \"limit-*\", \nwhere the server can communicate capabilities to the client, like so:\n\n limit-object-count_100000\n limit-commit-count_1000\n limit-pack-size_500000000\n\n(I'd prefer to s/_/=/ or s/_/:/, but according to pack-protocol.txt, a \ncapability may not contain \"=\" or \":\")\n\nHowever, you say:\n\n> For older clients that don't know this new advertised capability, they\n> should fail hard and not transfer all of this data.\n\nAFAICS this is not the case. If a client does not understand a capability, \nit simply ignores it, and carries on doing its usual thing.\n\nIINM there are only two ways to prevent an older client from transferring \nall the data:\n\n1. Change the pack protocol in an incompatible way, that causes older client \nto abort with a pack format error prior to transmitting the pack.\n\n2. (as in initial patch) Abort receive-pack when the server detects a limit \nviolation, leaving the client with a broken pipe. I haven't read the pack \nprotocol closely, but I wouldn't be surprised if this behavior is strictly \nin violation of the protocol.\n\n> In my experience\n> when a user gets these strange errors from his Git client, he contacts\n> his server administrator with the screen output. At which point the\n> administrator can see the Counting objects line, check the repository\n> configuration, and tell the user what the problem is... and encourage\n> them to upgrade their client to a newer version.\n\nHmm... Not ideal, but I guess we can live with that. At least we should warn \nthe server administrator of this in the documentation of the config \nvariable(s).\n\n\nOtherwise, I agree with everything you wrote.\n\n\nHave fun! :)\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"167857","messageId":"7voc355fz9.fsf@alter.siamese.dyndns.org","threadId":"27346","inReplyTo":"BANLkTik_taBK+=nh+0WEUjp3AV_fC7e_dg@mail.gmail.com","subject":"Re: [PATCHv2 2/2] receive-pack: Add receive.objectCountLimit to refuse push with too many objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-14T17:50:18Z","receivedAt":"2011-05-14T17:50:18Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shawn Pearce writes:\n\n> If we are going to put limits in, does it make sense to try and push\n> these limits back to pack-objects in a more detailed way? You talked\n> about depth of history, or size of pack. pack-objects could\n> approximate both.\n\nI think the receiving end switches between index-pack and unpack-objects\nbased on the object count, but if we can add a protocol extension to carry\nthese estimates from the sender, we would be much better off. A large push\nwith a small number of objects would want to keep everything in a pack,\nfor example, but currently there is no way to do so.\n"},{"id":"167867","messageId":"BANLkTimQD5JFeOeF48cbMjDL3+WMfsmPtA@mail.gmail.com","threadId":"27346","inReplyTo":"201105141517.45324.johan@herland.net","subject":"Re: [PATCHv2 2/2] receive-pack: Add receive.objectCountLimit to refuse push with too many objects","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2011-05-14T22:17:24Z","receivedAt":"2011-05-14T22:17:24Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Sat, May 14, 2011 at 06:17, Johan Herland wrote:\n> I'm planning to add a new capability collection/namespace, called \"limit-*\",\n> where the server can communicate capabilities to the client, like so:\n>\n>  limit-object-count_100000\n>  limit-commit-count_1000\n>  limit-pack-size_500000000\n>\n> (I'd prefer to s/_/=/ or s/_/:/, but according to pack-protocol.txt, a\n> capability may not contain \"=\" or \":\")\n\nI forget why = and : are forbidden here. I think its just because we\nwanted the options to be \"simple\". I agree, I would prefer = here too,\nand probably would have written the patch that way myself. There\nshouldn't be a technical reason why = isn't allowed here. Its just\ndocumented as being not a good idea because at one time someone wrote\nthat down.\n\n> However, you say:\n>\n>> For older clients that don't know this new advertised capability, they\n>> should fail hard and not transfer all of this data.\n>\n> AFAICS this is not the case. If a client does not understand a capability,\n> it simply ignores it, and carries on doing its usual thing.\n\nBy this I meant #2 below (the initial patch).\n\n> IINM there are only two ways to prevent an older client from transferring\n> all the data:\n>\n> 1. Change the pack protocol in an incompatible way, that causes older client\n> to abort with a pack format error prior to transmitting the pack.\n\nThis is not a good idea. We still want the client to be able to talk\nto the server if it would be within the limits.\n\n> 2. (as in initial patch) Abort receive-pack when the server detects a limit\n> violation, leaving the client with a broken pipe. I haven't read the pack\n> protocol closely, but I wouldn't be surprised if this behavior is strictly\n> in violation of the protocol.\n\nIt is a violation of the protocol... sort of. Its allowed for the\nserver to up and die in the middle of a push. What happens if the\nremote system loses power due to a grid failure while you are writing\nto it? The remote system can't tell you \"I'm going away now\" first. It\njust freezes and stops ACK'ing the TCP packets. Or if the remote\nsystem gets overloaded and the Linux OOM killer kicks in... the remote\nmight get one of the processes elected for killing, and your TCP\nconnection breaks.\n\nI don't think its as bad as it sounds. Its not a great user\nexperience, sure. And we maybe should also look at changing the\nsend-pack code to check the pipe for received data from the remote\npeer if pack-objects dies (today it doesn't)... just in case the\nreason pack-objects died is because an error message was written and\nthen the stream was closed.\n\n-- \nShawn.\n"},{"id":"167869","messageId":"BANLkTikGPhYYGF190XqWkyc=LuJ3PwA4Jg@mail.gmail.com","threadId":"27346","inReplyTo":"7voc355fz9.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv2 2/2] receive-pack: Add receive.objectCountLimit to refuse push with too many objects","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2011-05-14T22:27:36Z","receivedAt":"2011-05-14T22:27:36Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Sat, May 14, 2011 at 10:50, Junio C Hamano wrote:\n> Shawn Pearce writes:\n>\n>> If we are going to put limits in, does it make sense to try and push\n>> these limits back to pack-objects in a more detailed way? You talked\n>> about depth of history, or size of pack. pack-objects could\n>> approximate both.\n>\n> I think the receiving end switches between index-pack and unpack-objects\n> based on the object count,\n\nIt does...\n\n> but if we can add a protocol extension to carry\n> these estimates from the sender, we would be much better off. A large push\n> with a small number of objects would want to keep everything in a pack,\n> for example, but currently there is no way to do so.\n\nYes, my thoughts exactly. A 600 MiB push of 3 objects (1 huge blob,\ntop level tree, commit) shouldn't be unpacked. Serving that huge blob\nback out to clients that want to fetch it is more efficient when its\ncoming from a pack than from a loose object, due to the object reuse\nthat is available from a pack. Unfortunately we don't have the data\nnecessary to make that decision.\n\nProbably the cleanest way to do this estimate transmission is to\nmodify the pack stream format. Introduce a new capability for\npack-stats. If advertised from receive-pack, send-pack can pass a\n--pack-stats flag to pack-objects. That process then writes out a PACK\nheader with version code 4 (rather than 2), and includes additional\ndata beyond the object count in an extended PACK header. receive-pack\ncan then read the PACK header and handle version 2 as it does today,\nand version 4 by also checking the additional stat fields and dumping\nwhat is left over into either unpack-objects or index-pack like it\ndoes now. We wouldn't even need to change index-pack or\nunpack-objects, receive-pack could translate the PACK version 4 header\ninto a version 2 command line flag when it invokes the sub process to\nhandle the stream.\n\nHmm, or maybe this should be pack v5, just so nobody confuses it with\nthe mythical pack v4 we haven't implemented.\n\nIf we do add this stats header to the PACK stream, we need to be a bit\nsmarter about it. I would lay out the header with an additional 4 byte\nfield telling us the size of the stats block, and then use repeating\nkey-value pairs within the stats block to transmit the data. This way\nclients can add additional stats to the stream and receive-pack can\nignore them if it doesn't understand them. We can start out with\nnumber of commits and estimated size of the packed data (based on\nsumming up the reused object sizes) but could add more later like date\nof oldest commit if we later decide it would help the remote handle\nthe pack stream, or reject it early.\n\n\nAs far as receive-pack rejecting the stream after seeing the header, I\nwould like to avoid an extra round-trip between the two peers. So we\nprobably need to modify send-pack like I suggest above to check the\nstream for an error message after pack-objects terminates, even if\npack-objects terminates with a non-zero exit status. Even over a POSIX\npipe we should have room for 512 bytes of error data to be crammed\ninto the stream from receive-pack before we deadlock the two peers. If\nwe wanted to be really paranoid, we would run pack-objects in the\nbackground and have send-pack constantly monitor the incoming half of\nthe pipe for a status result. Since pack-objects is already another\nprocess this shouldn't be that difficult. (But it is harder for me in\nJGit where its not a separate thread.)\n\n-- \nShawn.\n"},{"id":"167900","messageId":"201105151942.29219.johan@herland.net","threadId":"27346","inReplyTo":"BANLkTimQD5JFeOeF48cbMjDL3+WMfsmPtA@mail.gmail.com","subject":"Re: [PATCHv2 2/2] receive-pack: Add receive.objectCountLimit to refuse push with too many objects","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-05-15T17:42:29Z","receivedAt":"2011-05-15T17:42:29Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Sunday 15 May 2011, Shawn Pearce wrote:\n> On Sat, May 14, 2011 at 06:17, Johan Herland wrote:\n> > I'm planning to add a new capability collection/namespace, called\n> > \"limit-*\", where the server can communicate capabilities to the\n> > client, like so:\n> > \n> > limit-object-count_100000\n> > limit-commit-count_1000\n> > limit-pack-size_500000000\n> > \n> > (I'd prefer to s/_/=/ or s/_/:/, but according to pack-protocol.txt, a\n> > capability may not contain \"=\" or \":\")\n> \n> I forget why = and : are forbidden here. I think its just because we\n> wanted the options to be \"simple\". I agree, I would prefer = here too,\n> and probably would have written the patch that way myself. There\n> shouldn't be a technical reason why = isn't allowed here. Its just\n> documented as being not a good idea because at one time someone wrote\n> that down.\n\nOk. I'll use '='.\n\n> > However, you say:\n> >> For older clients that don't know this new advertised capability, they\n> >> should fail hard and not transfer all of this data.\n> > \n> > AFAICS this is not the case. If a client does not understand a\n> > capability, it simply ignores it, and carries on doing its usual\n> > thing.\n> \n> By this I meant #2 below (the initial patch).\n\nAh, I see. Will use that in the re-roll.\n\n> I don't think its as bad as it sounds. Its not a great user\n> experience, sure. And we maybe should also look at changing the\n> send-pack code to check the pipe for received data from the remote\n> peer if pack-objects dies (today it doesn't)... just in case the\n> reason pack-objects died is because an error message was written and\n> then the stream was closed.\n\nThis will be in the re-roll.\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"167926","messageId":"1305495440-30836-1-git-send-email-johan@herland.net","threadId":"27346","inReplyTo":"201105151942.29219.johan@herland.net","subject":"[PATCHv3 0/9] Push limits","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-05-15T21:37:11Z","receivedAt":"2011-05-15T21:37:11Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"Here's the next iteration of what started out as a simple patch to let\nthe server refuse pushes that exceeded a configurable limit on the\n#objects in a pack.\n\nThe current patch series allows limiting pushes by\n - size of pack\n - #objects in pack\n - #commits in pack\n\nThe limits are controlled by corresponding (new) config variables:\n - receive.packSizeLimit\n - receive.objectCountLimit\n - receive.commitCountLimit\n\nSetting one or more of these config variables causes receive-pack to\nadvertise the corresponding (new) capabilities:\n - limit-pack-size=\n - limit-object-count=\n - limit-commit-count=\n\nThese capabilities are parsed by the send-pack client, which pass them\non to pack-objects, using the corresponding (mostly new) pack-objects\noptions:\n --max-pack-size= (extended to be usable in this context)\n --max-object-count= (new)\n --max-commit-count= (new)\n\nWhen one or more of those options are given together with --stdout to\npack-objects, pack-objects will check the generated pack against those\nlimits, and abort the pack generation if any limit is exceeded.\n\nIn addition, the server will also verify the object count limit,\nif enabled, and abort the push if the pack exceeds the limit.\nCurrently, the server cannot easily check the other two limits without\nchanging the pack format (left as an exercise to the reader... ;-]),\nso exceeding the pack size limit or the commit count limit will not be\ncaught by the server.\n\n\nFinally, a quick run-through of the patches:\n\n - #1 is a very minor fix to the pack protocol docs.\n\n - #2 attempts to retrieve and display the remote status, even when\n pack-objects fail. This patch touches the same code as the recent\n send-pack deadlock fixes, so I'd like Peff or JSixt to review them.\n\n - #3 - #5 teaches pack-objects some new options to impose the above\n limits on the generated pack.\n\n - #6 contains some general preparation for the \"limit-*\" capabilities.\n\n - #7 - #9 adds the new limits to receive-pack, send-pack, and the\n corresponding protocol capabilites.\n\n\nHave fun! :)\n\n...Johan\n\n\nJohan Herland (9):\n Update technical docs to reflect side-band-64k capability in receive-pack\n send-pack: Attempt to retrieve remote status even if pack-objects fails\n pack-objects: Allow --max-pack-size to be used together with --stdout\n pack-objects: Teach new option --max-object-count, similar to --max-pack-size\n pack-objects: Teach new option --max-commit-count, limiting #commits in pack\n receive-pack: Prepare for addition of the new 'limit-*' family of capabilities\n send-pack/receive-pack: Allow server to refuse pushes with too many objects\n send-pack/receive-pack: Allow server to refuse pushing too large packs\n send-pack/receive-pack: Allow server to refuse pushes with too many commits\n\n Documentation/config.txt | 35 ++++\n Documentation/git-pack-objects.txt | 20 +++\n Documentation/git-repack.txt | 6 +\n Documentation/technical/pack-protocol.txt | 5 +-\n Documentation/technical/protocol-capabilities.txt | 30 +++-\n builtin/pack-objects.c | 53 +++++-\n builtin/receive-pack.c | 45 +++++-\n builtin/send-pack.c | 44 ++++--\n cache.h | 2 +-\n connect.c | 7 +-\n git-repack.sh | 27 ++--\n send-pack.h | 3 +\n t/t5300-pack-object.sh | 121 +++++++++++++\n t/t5400-send-pack.sh | 191 +++++++++++++++++++++\n 14 files changed, 543 insertions(+), 46 deletions(-)\n\n-- \n1.7.5.rc1.3.g4d7b\n"},{"id":"167925","messageId":"1305495440-30836-2-git-send-email-johan@herland.net","threadId":"27346","inReplyTo":"1305495440-30836-1-git-send-email-johan@herland.net","subject":"[PATCHv3 1/9] Update technical docs to reflect side-band-64k capability in receive-pack","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-05-15T21:37:12Z","receivedAt":"2011-05-15T21:37:12Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"Signed-off-by: Johan Herland \n---\n Documentation/technical/pack-protocol.txt | 3 ++-\n Documentation/technical/protocol-capabilities.txt | 4 ++--\n 2 files changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt\nindex 369f91d..4a68f0f 100644\n--- a/Documentation/technical/pack-protocol.txt\n+++ b/Documentation/technical/pack-protocol.txt\n@@ -391,7 +391,8 @@ The reference discovery phase is done nearly the same way as it is in the\n fetching protocol. Each reference obj-id and name on the server is sent\n in packet-line format to the client, followed by a flush-pkt. The only\n real difference is that the capability listing is different - the only\n-possible values are 'report-status', 'delete-refs' and 'ofs-delta'.\n+possible values are 'report-status', 'delete-refs', 'side-band-64k' and\n+'ofs-delta'.\n \n Reference Update Request and Packfile Transfer\n ----------------------------------------------\ndiff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt\nindex b15517f..b732e80 100644\n--- a/Documentation/technical/protocol-capabilities.txt\n+++ b/Documentation/technical/protocol-capabilities.txt\n@@ -21,8 +21,8 @@ NOT advertise capabilities it does not understand.\n The 'report-status' and 'delete-refs' capabilities are sent and\n recognized by the receive-pack (push to server) process.\n \n-The 'ofs-delta' capability is sent and recognized by both upload-pack\n-and receive-pack protocols.\n+The 'side-band-64k' and 'ofs-delta' capabilities are sent and\n+recognized by both upload-pack and receive-pack protocols.\n \n All other capabilities are only recognized by the upload-pack (fetch\n from server) process.\n-- \n1.7.5.rc1.3.g4d7b\n"},{"id":"167927","messageId":"1305495440-30836-3-git-send-email-johan@herland.net","threadId":"27346","inReplyTo":"1305495440-30836-1-git-send-email-johan@herland.net","subject":"[PATCHv3 2/9] send-pack: Attempt to retrieve remote status even if pack-objects fails","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-05-15T21:37:13Z","receivedAt":"2011-05-15T21:37:13Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"When pushing, send-pack uses pack-objects to write the pack data to the\nreceive-pack process running on the remote end. The scenarios where\npack-objects dies unexpectedly, can be roughly divided based whether the\nreason for the failure is _local_ (i.e. something in pack-objects caused\nit to fail of its own accord), or _remote_ (i.e. something in the remote\nreceive-pack process caused it to fail, leaving the local pack-objects\nprocess with a broken pipe)\n\nIf the reason for the failure is local, we expect pack-objects to report\nan appropriate error message to the user.\n\nHowever, if the reason for the failure is remote, pack-objects will merely\nabort because of the broken pipe, and the user is left with no clue as to\nthe reason why the remote receive-pack process died.\n\nIn certain cases, though, the receive-pack process on the other end may have\nproduced an error message immediately before exiting. This error message may\nbe currently waiting to be read by the local send-pack process.\n\nTherefore, we should try to read from the remote end, even when pack-objects\ndies unexepectedly. We accomplish this by _always_ calling receive_status()\nafter pack_objects(). If the remote end managed to produce a well-formed\nstatus report before exiting, then receive_status() simply presents that to\nthe user. Even if the data from the remote end cannot be understood by\nreceive_status(), it will print that data as part of its error message. In\nany case, we give the user as much information about the failure as possible.\n\nSigned-off-by: Johan Herland \n---\n\nI first wrote this patch on a base where e07fd15 (Peff's \"send-pack:\nunbreak push over stateless rpc\") was not present, and then resolved\na conflict when rebasing this patch onto current master. I hope Peff\nor Johannes (Sixt) can verify that my patch does not reintroduce the\ndeadlock they fixed.\n\n\n...Johan\n\n builtin/send-pack.c | 18 +++++-------------\n 1 files changed, 5 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex 4ac2ca9..f571917 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -251,7 +251,7 @@ int send_pack(struct send_pack_args *args,\n \tint status_report = 0;\n \tint use_sideband = 0;\n \tunsigned cmds_sent = 0;\n-\tint ret;\n+\tint ret = 0;\n \tstruct async demux;\n \n \t/* Does the other end support the reporting? */\n@@ -339,23 +339,15 @@ int send_pack(struct send_pack_args *args,\n \t}\n \n \tif (new_refs && cmds_sent) {\n-\t\tif (pack_objects(out, remote_refs, extra_have, args) < 0) {\n-\t\t\tfor (ref = remote_refs; ref; ref = ref->next)\n-\t\t\t\tref->status = REF_STATUS_NONE;\n-\t\t\tif (args->stateless_rpc)\n-\t\t\t\tclose(out);\n-\t\t\tif (use_sideband)\n-\t\t\t\tfinish_async(&demux);\n-\t\t\treturn -1;\n-\t\t}\n+\t\tret = pack_objects(out, remote_refs, extra_have, args);\n+\t\tif (ret && args->stateless_rpc)\n+\t\t\tclose(out);\n \t}\n \tif (args->stateless_rpc && cmds_sent)\n \t\tpacket_flush(out);\n \n \tif (status_report && cmds_sent)\n-\t\tret = receive_status(in, remote_refs);\n-\telse\n-\t\tret = 0;\n+\t\tret |= receive_status(in, remote_refs);\n \tif (args->stateless_rpc)\n \t\tpacket_flush(out);\n \n-- \n1.7.5.rc1.3.g4d7b\n"},{"id":"167928","messageId":"1305495440-30836-4-git-send-email-johan@herland.net","threadId":"27346","inReplyTo":"1305495440-30836-1-git-send-email-johan@herland.net","subject":"[PATCHv3 3/9] pack-objects: Allow --max-pack-size to be used together with --stdout","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-05-15T21:37:14Z","receivedAt":"2011-05-15T21:37:14Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"Currently we refuse combining --max-pack-size with --stdout since there's\nno way to make multiple packs when the pack is written to stdout. However,\nwe want to be able to limit the maximum size of the pack created by\n--stdout (and abort pack-objects if we are unable to meet that limit).\n\nTherefore, when used together with --stdout, we reinterpret --max-pack-size\nto indicate the maximum pack size which - if exceeded - will cause\npack-objects to abort with an error message.\n\nSigned-off-by: Johan Herland \n---\n Documentation/git-pack-objects.txt | 3 ++\n builtin/pack-objects.c | 9 ++++---\n t/t5300-pack-object.sh | 43 ++++++++++++++++++++++++++++++++++++\n 3 files changed, 51 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt\nindex 20c8551..ca97463 100644\n--- a/Documentation/git-pack-objects.txt\n+++ b/Documentation/git-pack-objects.txt\n@@ -112,6 +112,9 @@ base-name::\n \tIf specified, multiple packfiles may be created.\n \tThe default is unlimited, unless the config variable\n \t`pack.packSizeLimit` is set.\n++\n+When used together with --stdout, the command will fail with an error\n+message if the pack output exceeds the given limit.\n \n --honor-pack-keep::\n \tThis flag causes an object already in a local pack that\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex f402a84..69f1c51 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -229,7 +229,7 @@ static unsigned long write_object(struct sha1file *f,\n \n \tif (!entry->delta)\n \t\tusable_delta = 0;\t/* no delta */\n-\telse if (!pack_size_limit)\n+\telse if (!pack_size_limit || pack_to_stdout)\n \t usable_delta = 1;\t/* unlimited packfile */\n \telse if (entry->delta->idx.offset == (off_t)-1)\n \t\tusable_delta = 0;\t/* base was written to another pack */\n@@ -478,6 +478,9 @@ static void write_pack_file(void)\n \t\t * If so, rewrite it like in fast-import\n \t\t */\n \t\tif (pack_to_stdout) {\n+\t\t\tif (nr_written != nr_remaining)\n+\t\t\t\tdie(\"unable to make pack within the pack size\"\n+\t\t\t\t \" limit (%lu bytes)\", pack_size_limit);\n \t\t\tsha1close(f, sha1, CSUM_CLOSE);\n \t\t} else if (nr_written == nr_remaining) {\n \t\t\tsha1close(f, sha1, CSUM_FSYNC);\n@@ -2315,9 +2318,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \n \tif (!pack_to_stdout && !pack_size_limit)\n \t\tpack_size_limit = pack_size_limit_cfg;\n-\tif (pack_to_stdout && pack_size_limit)\n-\t\tdie(\"--max-pack-size cannot be used to build a pack for transfer.\");\n-\tif (pack_size_limit && pack_size_limit < 1024*1024) {\n+\tif (!pack_to_stdout && pack_size_limit && pack_size_limit < 1024*1024) {\n \t\twarning(\"minimum pack size limit is 1 MiB\");\n \t\tpack_size_limit = 1024*1024;\n \t}\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex 602806d..00f1bd8 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -396,6 +396,49 @@ test_expect_success 'verify resulting packs' '\n \tgit verify-pack test-11-*.pack\n '\n \n+test_expect_success '--stdout ignores pack.packSizeLimit' '\n+\tgit pack-objects --stdout test-12.pack &&\n+\tgit index-pack --strict test-12.pack\n+'\n+\n+test_expect_success 'verify resulting pack' '\n+\tgit verify-pack test-12.pack\n+'\n+\n+test_expect_success 'honor --max-pack-size' '\n+\tgit config --unset pack.packSizeLimit &&\n+\tpackname_13=$(git pack-objects --max-pack-size=3m test-13 test-15.pack &&\n+\tgit index-pack --strict test-15.pack\n+'\n+\n+test_expect_success 'verify resulting pack' '\n+\tgit verify-pack test-15.pack\n+'\n+\n+test_expect_success '--stdout fails when pack exceeds --max-pack-size' '\n+\ttest_must_fail git pack-objects --stdout --max-pack-size=1 test-16.pack 2>errs &&\n+\ttest_must_fail git index-pack --strict test-16.pack &&\n+\tgrep -q \"pack size limit\" errs\n+'\n+\n #\n # WARNING!\n #\n-- \n1.7.5.rc1.3.g4d7b\n"},{"id":"167929","messageId":"1305495440-30836-5-git-send-email-johan@herland.net","threadId":"27346","inReplyTo":"1305495440-30836-1-git-send-email-johan@herland.net","subject":"[PATCHv3 4/9] pack-objects: Teach new option --max-object-count, similar to --max-pack-size","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-05-15T21:37:15Z","receivedAt":"2011-05-15T21:37:15Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"The new --max-object-count option behaves similarly to --max-pack-size,\nexcept that the decision to split packs is determined by the number of\nobjects in the pack, and not by the size of the pack.\n\nThe new option also has a corresponding configuration variable, named\npack.objectCountLimit, which works similarly to pack.packSizeLimit,\nsubject to the difference mentioned above.\n\nAs with --max-pack-size, you can use --max-object-count together with\n--stdout to put a limit on the number of objects in the pack written to\nstdout. If the pack would exceed this limit, pack-objects will abort with\nan error message.\n\nFinally, for completeness, the new option is also added to git-repack,\nwhich simply forwards it to pack-objects\n\nDocumentation and tests are included.\n\nSigned-off-by: Johan Herland \n---\n Documentation/config.txt | 8 ++++++\n Documentation/git-pack-objects.txt | 9 +++++++\n Documentation/git-repack.txt | 6 +++++\n builtin/pack-objects.c | 20 ++++++++++++++++\n git-repack.sh | 27 +++++++++++----------\n t/t5300-pack-object.sh | 44 ++++++++++++++++++++++++++++++++++++\n 6 files changed, 101 insertions(+), 13 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 285c7f7..056e01f 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1523,6 +1523,14 @@ pack.packSizeLimit::\n \tCommon unit suffixes of 'k', 'm', or 'g' are\n \tsupported.\n \n+pack.objectCountLimit::\n+\tThe maximum number of objects in a pack. This setting only\n+\taffects packing to a file when repacking, i.e. the git://\n+\tprotocol is unaffected. It can be overridden by the\n+\t`\\--max-object-count` option of linkgit:git-repack[1].\n+\tThe default is unlimited. Common unit suffixes of 'k', 'm',\n+\tor 'g' are supported.\n+\n pager.::\n \tIf the value is boolean, turns on or off pagination of the\n \toutput of a particular git subcommand when writing to a tty.\ndiff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt\nindex ca97463..5ab1fe9 100644\n--- a/Documentation/git-pack-objects.txt\n+++ b/Documentation/git-pack-objects.txt\n@@ -116,6 +116,15 @@ base-name::\n When used together with --stdout, the command will fail with an error\n message if the pack output exceeds the given limit.\n \n+--max-object-count=::\n+\tMaximum number of objects in each output pack file. The number\n+\tcan be suffixed with \"k\", \"m\", or \"g\". If specified, multiple\n+\tpackfiles may be created. The default is unlimited, unless\n+\tthe config variable `pack.objectCountLimit` is set.\n++\n+When used together with --stdout, the command will fail with an error\n+message if the pack output exceeds the given limit.\n+\n --honor-pack-keep::\n \tThis flag causes an object already in a local pack that\n \thas a .keep file to be ignored, even if it would have\ndiff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt\nindex 0decee2..f9a44b8 100644\n--- a/Documentation/git-repack.txt\n+++ b/Documentation/git-repack.txt\n@@ -109,6 +109,12 @@ other objects in that pack they already have locally.\n \tThe default is unlimited, unless the config variable\n \t`pack.packSizeLimit` is set.\n \n+--max-object-count=::\n+\tMaximum number of objects in each output pack file. The number\n+\tcan be suffixed with \"k\", \"m\", or \"g\". If specified, multiple\n+\tpackfiles may be created. The default is unlimited, unless\n+\tthe config variable `pack.objectCountLimit` is set.\n+\n \n Configuration\n -------------\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 69f1c51..c4fbc54 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -74,6 +74,7 @@ static const char *base_name;\n static int progress = 1;\n static int window = 10;\n static unsigned long pack_size_limit, pack_size_limit_cfg;\n+static unsigned long object_count_limit, object_count_limit_cfg;\n static int depth = 50;\n static int delta_search_threads;\n static int pack_to_stdout;\n@@ -227,6 +228,10 @@ static unsigned long write_object(struct sha1file *f,\n \telse\n \t\tlimit = pack_size_limit - write_offset;\n \n+\t/* Trigger new pack when we reach object count limit */\n+\tif (object_count_limit && nr_written >= object_count_limit)\n+\t\treturn 0;\n+\n \tif (!entry->delta)\n \t\tusable_delta = 0;\t/* no delta */\n \telse if (!pack_size_limit || pack_to_stdout)\n@@ -1897,6 +1902,10 @@ static int git_pack_config(const char *k, const char *v, void *cb)\n \t\tpack_size_limit_cfg = git_config_ulong(k, v);\n \t\treturn 0;\n \t}\n+\tif (!strcmp(k, \"pack.objectcountlimit\")) {\n+\t\tobject_count_limit_cfg = git_config_ulong(k, v);\n+\t\treturn 0;\n+\t}\n \treturn git_default_config(k, v, cb);\n }\n \n@@ -2182,6 +2191,12 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t\t\tusage(pack_usage);\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!prefixcmp(arg, \"--max-object-count=\")) {\n+\t\t\tobject_count_limit_cfg = 0;\n+\t\t\tif (!git_parse_ulong(arg+19, &object_count_limit))\n+\t\t\t\tusage(pack_usage);\n+\t\t\tcontinue;\n+\t\t}\n \t\tif (!prefixcmp(arg, \"--window=\")) {\n \t\t\tchar *end;\n \t\t\twindow = strtoul(arg+9, &end, 0);\n@@ -2322,6 +2337,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\twarning(\"minimum pack size limit is 1 MiB\");\n \t\tpack_size_limit = 1024*1024;\n \t}\n+\tif (!pack_to_stdout && !object_count_limit)\n+\t\tobject_count_limit = object_count_limit_cfg;\n \n \tif (!pack_to_stdout && thin)\n \t\tdie(\"--thin cannot be used to build an indexable pack.\");\n@@ -2349,6 +2366,9 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \n \tif (non_empty && !nr_result)\n \t\treturn 0;\n+\tif (pack_to_stdout && object_count_limit && object_count_limit < nr_result)\n+\t\tdie(\"unable to make pack within the object count limit\"\n+\t\t\t\" (%lu objects)\", object_count_limit);\n \tif (nr_result)\n \t\tprepare_pack(window, depth);\n \twrite_pack_file();\ndiff --git a/git-repack.sh b/git-repack.sh\nindex 624feec..e8693a6 100755\n--- a/git-repack.sh\n+++ b/git-repack.sh\n@@ -7,19 +7,20 @@ OPTIONS_KEEPDASHDASH=\n OPTIONS_SPEC=\"\\\n git repack [options]\n --\n-a pack everything in a single pack\n-A same as -a, and turn unreachable objects loose\n-d remove redundant packs, and run git-prune-packed\n-f pass --no-reuse-delta to git-pack-objects\n-F pass --no-reuse-object to git-pack-objects\n-n do not run git-update-server-info\n-q,quiet be quiet\n-l pass --local to git-pack-objects\n+a pack everything in a single pack\n+A same as -a, and turn unreachable objects loose\n+d remove redundant packs, and run git-prune-packed\n+f pass --no-reuse-delta to git-pack-objects\n+F pass --no-reuse-object to git-pack-objects\n+n do not run git-update-server-info\n+q,quiet be quiet\n+l pass --local to git-pack-objects\n Packing constraints\n-window= size of the window used for delta compression\n-window-memory= same as the above, but limit memory size instead of entries count\n-depth= limits the maximum delta depth\n-max-pack-size= maximum size of each packfile\n+window= size of the window used for delta compression\n+window-memory= same as the above, but limit memory size instead of entries count\n+depth= limits the maximum delta depth\n+max-pack-size= maximum size of each packfile\n+max-object-count= maximum number of objects in each packfile\n \"\n SUBDIRECTORY_OK='Yes'\n . git-sh-setup\n@@ -38,7 +39,7 @@ do\n \t-f)\tno_reuse=--no-reuse-delta ;;\n \t-F)\tno_reuse=--no-reuse-object ;;\n \t-l)\tlocal=--local ;;\n-\t--max-pack-size|--window|--window-memory|--depth)\n+\t--max-pack-size|--max-object-count|--window|--window-memory|--depth)\n \t\textra=\"$extra $1=$2\"; shift ;;\n \t--) shift; break;;\n \t*)\tusage ;;\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex 00f1bd8..d133c28 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -439,6 +439,50 @@ test_expect_success '--stdout fails when pack exceeds --max-pack-size' '\n \tgrep -q \"pack size limit\" errs\n '\n \n+test_expect_success 'honor pack.objectCountLimit' '\n+\tgit config pack.objectCountLimit 5 &&\n+\tpackname_17=$(git pack-objects test-17 test-18.pack &&\n+\tgit index-pack --strict test-18.pack\n+'\n+\n+test_expect_success 'verify resulting pack' '\n+\tgit verify-pack test-18.pack\n+'\n+\n+test_expect_success 'honor --max-object-count' '\n+\tgit config --unset pack.objectCountLimit &&\n+\tpackname_19=$(git pack-objects --max-object-count=5 test-19 test-20.pack &&\n+\tgit index-pack --strict test-20.pack\n+'\n+\n+test_expect_success 'verify resulting pack' '\n+\tgit verify-pack test-20.pack\n+'\n+\n+test_expect_success '--stdout fails when pack exceeds --max-object-count' '\n+\ttest_must_fail git pack-objects --stdout --max-object-count=1 test-21.pack 2>errs &&\n+\ttest_must_fail git index-pack --strict test-21.pack &&\n+\tgrep -q \"object count limit\" errs\n+'\n+\n #\n # WARNING!\n #\n-- \n1.7.5.rc1.3.g4d7b\n"},{"id":"167930","messageId":"1305495440-30836-6-git-send-email-johan@herland.net","threadId":"27346","inReplyTo":"1305495440-30836-1-git-send-email-johan@herland.net","subject":"[PATCHv3 5/9] pack-objects: Teach new option --max-commit-count, limiting #commits in pack","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-05-15T21:37:16Z","receivedAt":"2011-05-15T21:37:16Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"The new --max-commit-count option behaves similarly to --max-object-count,\nwhen used together with --stdout: It limits the number of commits in the\npack written to stdout. If the pack would exceed this limit, pack-objects\nwill abort with an error message.\n\nUnlike --max-pack-size and --max-object-count, --max-commit-count must\nalways be used together with --stdout. This is because using the commit\ncount to split packs is not at all a good heuristic, since Git does not\nnecessarily distribute commit objects uniformly across packs.\n\nDocumentation and tests are included.\n\nSigned-off-by: Johan Herland \n---\n Documentation/git-pack-objects.txt | 8 ++++++++\n builtin/pack-objects.c | 24 +++++++++++++++++++++---\n t/t5300-pack-object.sh | 34 ++++++++++++++++++++++++++++++++++\n 3 files changed, 63 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt\nindex 5ab1fe9..21c30c0 100644\n--- a/Documentation/git-pack-objects.txt\n+++ b/Documentation/git-pack-objects.txt\n@@ -125,6 +125,14 @@ message if the pack output exceeds the given limit.\n When used together with --stdout, the command will fail with an error\n message if the pack output exceeds the given limit.\n \n+--max-commit-count=::\n+\tThis option is only useful together with --stdout.\n+\tSpecifies the maximum number of commits allowed in the created\n+\tpack. If the number of commits would exceed the given limit,\n+\tpack-objects will fail with an error message.\n+\tThe number can be suffixed with \"k\", \"m\", or \"g\".\n+\tThe default is unlimited.\n+\n --honor-pack-keep::\n \tThis flag causes an object already in a local pack that\n \thas a .keep file to be ignored, even if it would have\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex c4fbc54..6c6945e 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -61,7 +61,7 @@ struct object_entry {\n */\n static struct object_entry *objects;\n static struct pack_idx_entry **written_list;\n-static uint32_t nr_objects, nr_alloc, nr_result, nr_written;\n+static uint32_t nr_objects, nr_alloc, nr_result, nr_commits, nr_written;\n \n static int non_empty;\n static int reuse_delta = 1, reuse_object = 1;\n@@ -661,8 +661,11 @@ static int add_object_entry(const unsigned char *sha1, enum object_type type,\n \tif (ix >= 0) {\n \t\tif (exclude) {\n \t\t\tentry = objects + object_ix[ix] - 1;\n-\t\t\tif (!entry->preferred_base)\n+\t\t\tif (!entry->preferred_base) {\n \t\t\t\tnr_result--;\n+\t\t\t\tif (entry->type == OBJ_COMMIT)\n+\t\t\t\t\tnr_commits--;\n+\t\t\t}\n \t\t\tentry->preferred_base = 1;\n \t\t}\n \t\treturn 0;\n@@ -702,8 +705,11 @@ static int add_object_entry(const unsigned char *sha1, enum object_type type,\n \t\tentry->type = type;\n \tif (exclude)\n \t\tentry->preferred_base = 1;\n-\telse\n+\telse {\n \t\tnr_result++;\n+\t\tif (type == OBJ_COMMIT)\n+\t\t\tnr_commits++;\n+\t}\n \tif (found_pack) {\n \t\tentry->in_pack = found_pack;\n \t\tentry->in_pack_offset = found_offset;\n@@ -2137,6 +2143,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \tconst char **rp_av;\n \tint rp_ac_alloc = 64;\n \tint rp_ac;\n+\tunsigned long commit_count_limit = 0;\n \n \tread_replace_refs = 0;\n \n@@ -2197,6 +2204,11 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t\t\tusage(pack_usage);\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!prefixcmp(arg, \"--max-commit-count=\")) {\n+\t\t\tif (!git_parse_ulong(arg+19, &commit_count_limit))\n+\t\t\t\tusage(pack_usage);\n+\t\t\tcontinue;\n+\t\t}\n \t\tif (!prefixcmp(arg, \"--window=\")) {\n \t\t\tchar *end;\n \t\t\twindow = strtoul(arg+9, &end, 0);\n@@ -2340,6 +2352,9 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \tif (!pack_to_stdout && !object_count_limit)\n \t\tobject_count_limit = object_count_limit_cfg;\n \n+\tif (!pack_to_stdout && commit_count_limit)\n+\t\tdie(\"--max-commit-count is only useful together with --stdout.\");\n+\n \tif (!pack_to_stdout && thin)\n \t\tdie(\"--thin cannot be used to build an indexable pack.\");\n \n@@ -2369,6 +2384,9 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \tif (pack_to_stdout && object_count_limit && object_count_limit < nr_result)\n \t\tdie(\"unable to make pack within the object count limit\"\n \t\t\t\" (%lu objects)\", object_count_limit);\n+\tif (pack_to_stdout && commit_count_limit && commit_count_limit < nr_commits)\n+\t\tdie(\"unable to make pack within the commit count limit\"\n+\t\t\t\" (%lu commits)\", commit_count_limit);\n \tif (nr_result)\n \t\tprepare_pack(window, depth);\n \twrite_pack_file();\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex d133c28..919e2da 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -483,6 +483,40 @@ test_expect_success '--stdout fails when pack exceeds --max-object-count' '\n \tgrep -q \"object count limit\" errs\n '\n \n+test_expect_success 'make a few more commits' '\n+\tgit reset --hard $commit &&\n+\techo \"change\" > file &&\n+\tgit add file &&\n+\tgit commit -m second &&\n+\tcommit2=`git rev-parse --verify HEAD` &&\n+\techo \"more change\" >> file &&\n+\tgit commit -a -m third &&\n+\tcommit3=`git rev-parse --verify HEAD` &&\n+\techo \"even more change\" >> file &&\n+\tgit commit -a -m fourth &&\n+\tcommit4=`git rev-parse --verify HEAD` && {\n+\t\techo $commit &&\n+\t\techo $commit2 &&\n+\t\techo $commit3 &&\n+\t\techo $commit4\n+\t} >>commit-list\n+'\n+\n+test_expect_success '--stdout works with large enough --max-commit-count' '\n+\tgit pack-objects --revs --stdout --max-commit-count=4 test-22.pack &&\n+\tgit index-pack --strict test-22.pack\n+'\n+\n+test_expect_success 'verify resulting pack' '\n+\tgit verify-pack test-22.pack\n+'\n+\n+test_expect_success '--stdout fails when pack exceeds --max-commit-count' '\n+\ttest_must_fail git pack-objects --revs --stdout --max-commit-count=3 test-23.pack 2>errs &&\n+\ttest_must_fail git index-pack --strict test-23.pack &&\n+\tgrep -q \"commit count limit\" errs\n+'\n+\n #\n # WARNING!\n #\n-- \n1.7.5.rc1.3.g4d7b\n"},{"id":"167933","messageId":"1305495440-30836-7-git-send-email-johan@herland.net","threadId":"27346","inReplyTo":"1305495440-30836-1-git-send-email-johan@herland.net","subject":"[PATCHv3 6/9] receive-pack: Prepare for addition of the new 'limit-*' family of capabilities","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-05-15T21:37:17Z","receivedAt":"2011-05-15T21:37:17Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"This adds some technical documentation on the 'limit-*' family of\ncapabilities that will be added in the following commits.\n\nAlso refactor the generation of the capabilities declaration in receive-pack.\nThis will also be further expanded in the following commits.\n\nFinally, change the return type of server_supports() to allow the caller to\nmore closely examine the found capability, e.g. by calling\nserver_supports(\"limit-foo=\"), and then use the return value to parse the\nvalue following the '='.\n\nSigned-off-by: Johan Herland \n---\n Documentation/technical/pack-protocol.txt | 6 ++--\n Documentation/technical/protocol-capabilities.txt | 22 +++++++++++++++++++++\n builtin/receive-pack.c | 16 +++++++++++---\n cache.h | 2 +-\n connect.c | 7 +++--\n 5 files changed, 42 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt\nindex 4a68f0f..ddc0d0e 100644\n--- a/Documentation/technical/pack-protocol.txt\n+++ b/Documentation/technical/pack-protocol.txt\n@@ -167,7 +167,7 @@ MUST peel the ref if it's an annotated tag.\n other-peeled = obj-id SP refname \"^{}\" LF\n \n capability-list = capability *(SP capability)\n- capability = 1*(LC_ALPHA / DIGIT / \"-\" / \"_\")\n+ capability = 1*(LC_ALPHA / DIGIT / \"-\" / \"_\" / \"=\")\n LC_ALPHA = %x61-7A\n ----\n \n@@ -391,8 +391,8 @@ The reference discovery phase is done nearly the same way as it is in the\n fetching protocol. Each reference obj-id and name on the server is sent\n in packet-line format to the client, followed by a flush-pkt. The only\n real difference is that the capability listing is different - the only\n-possible values are 'report-status', 'delete-refs', 'side-band-64k' and\n-'ofs-delta'.\n+possible values are 'report-status', 'delete-refs', 'side-band-64k',\n+'ofs-delta' and 'limit-*'.\n \n Reference Update Request and Packfile Transfer\n ----------------------------------------------\ndiff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt\nindex b732e80..11849a3 100644\n--- a/Documentation/technical/protocol-capabilities.txt\n+++ b/Documentation/technical/protocol-capabilities.txt\n@@ -21,6 +21,9 @@ NOT advertise capabilities it does not understand.\n The 'report-status' and 'delete-refs' capabilities are sent and\n recognized by the receive-pack (push to server) process.\n \n+Any 'limit-*' capabilities may only be sent by the receive-pack\n+process. It is never requested by client.\n+\n The 'side-band-64k' and 'ofs-delta' capabilities are sent and\n recognized by both upload-pack and receive-pack protocols.\n \n@@ -185,3 +188,22 @@ it is capable of accepting a zero-id value as the target\n value of a reference update. It is not sent back by the client, it\n simply informs the client that it can be sent zero-id values\n to delete references.\n+\n+limit-*\n+-------\n+\n+If the server sends one or more capabilities that start with \"limit-\",\n+it means that there are certain limits to what kind of pack the server\n+will receive. More specifically, these capabilities must be of the form\n+\"limit-=\" where \"\" (a sequence of lower-case letters,\n+digits and \"-\") describes which property of the pack is limited, and\n+\"\" (a sequence of decimal digits) specifies the limit value.\n+Capabilities of this type are not sent back by the client; instead the\n+client must verify that the created packfile does not exceed the given\n+limits. This check should happen prior to transferring the packfile to\n+the server. If the check fails, the client must abort the upload, and\n+report the reason for the aborted push back to the user.\n+The following \"limit-*\" capabilites are recognized:\n+\n+More \"limit-*\" capabilities may be added in the future. The client\n+is free to ignore any \"limit-*\" capabilities it does not understand.\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex e1ba4dc..c55989d 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -106,15 +106,23 @@ static int receive_pack_config(const char *var, const char *value, void *cb)\n \treturn git_default_config(var, value, cb);\n }\n \n+static const char *capabilities()\n+{\n+\tstatic char buf[1024];\n+\tint ret = snprintf(buf, sizeof(buf),\n+\t\t\t \" report-status delete-refs side-band-64k%s\",\n+\t\t\t prefer_ofs_delta ? \" ofs-delta\" : \"\");\n+\tassert(ret < sizeof(buf));\n+\treturn buf;\n+}\n+\n static int show_ref(const char *path, const unsigned char *sha1, int flag, void *cb_data)\n {\n \tif (sent_capabilities)\n \t\tpacket_write(1, \"%s %s\\n\", sha1_to_hex(sha1), path);\n \telse\n-\t\tpacket_write(1, \"%s %s%c%s%s\\n\",\n-\t\t\t sha1_to_hex(sha1), path, 0,\n-\t\t\t \" report-status delete-refs side-band-64k\",\n-\t\t\t prefer_ofs_delta ? \" ofs-delta\" : \"\");\n+\t\tpacket_write(1, \"%s %s%c%s\\n\",\n+\t\t\t sha1_to_hex(sha1), path, 0, capabilities());\n \tsent_capabilities = 1;\n \treturn 0;\n }\ndiff --git a/cache.h b/cache.h\nindex 2b34116..db97097 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -981,7 +981,7 @@ struct extra_have_objects {\n \tunsigned char (*array)[20];\n };\n extern struct ref **get_remote_heads(int in, struct ref **list, int nr_match, char **match, unsigned int flags, struct extra_have_objects *);\n-extern int server_supports(const char *feature);\n+extern const char *server_supports(const char *feature);\n \n extern struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);\n \ndiff --git a/connect.c b/connect.c\nindex 57dc20c..015c570 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -102,10 +102,11 @@ struct ref **get_remote_heads(int in, struct ref **list,\n \treturn list;\n }\n \n-int server_supports(const char *feature)\n+const char *server_supports(const char *feature)\n {\n-\treturn server_capabilities &&\n-\t\tstrstr(server_capabilities, feature) != NULL;\n+\tif (server_capabilities)\n+\t\treturn strstr(server_capabilities, feature);\n+\treturn NULL;\n }\n \n int path_match(const char *path, int nr, char **match)\n-- \n1.7.5.rc1.3.g4d7b\n"},{"id":"167932","messageId":"1305495440-30836-8-git-send-email-johan@herland.net","threadId":"27346","inReplyTo":"1305495440-30836-1-git-send-email-johan@herland.net","subject":"[PATCHv3 7/9] send-pack/receive-pack: Allow server to refuse pushes with too many objects","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-05-15T21:37:18Z","receivedAt":"2011-05-15T21:37:18Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"Add a new receive.objectCountLimit config variable which defines an upper\nlimit on the number of objects to accept in a single push.\n\nThis limit is advertised to clients, using the new \"limit-object-count=\"\ncapability. The client side - aka. send-pack - parses this capability and\nforwards it to pack-objects, using the recently added --max-object-count\noption. pack-objects then checks the generated pack against the limit and\naborts the pack generation if the pack would have too many objects.\n\nAdditionally - for older clients that do not understand the capability - the\nserver aborts the transfer if the number of objects in the transferred pack\nexceeds the limit. This is a suboptimal fallback solution, as it leaves the\nclient with a broken pipe, and likely a confused user.\n\nServer administrators might want to use this config variable to prevent\nunintended large pushes from entering the repo (typically a result of the\nuser not being aware of exactly what is being pushed, e.g. pushing a large\nrewritten history). Note that this config variable is not intended to protect\nagainst DoS attacks, since there are countless other ways to attempt to DoS a\nserver without violating this limit.\n\nTraditionally, this kind of limit would be imposed by a pre-receive or update\nhook, but both of those run _after_ the pack has been received and stored by\nreceive-pack, so they cannot prevent the pack from being stored on the server.\n\nDocumentation and tests are included.\n\nSigned-off-by: Johan Herland \n---\n Documentation/config.txt | 9 +++\n Documentation/technical/protocol-capabilities.txt | 2 +\n builtin/receive-pack.c | 13 ++++-\n builtin/send-pack.c | 10 +++\n send-pack.h | 1 +\n t/t5400-send-pack.sh | 66 +++++++++++++++++++++\n 6 files changed, 100 insertions(+), 1 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 056e01f..fd6c130 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1599,6 +1599,15 @@ receive.unpackLimit::\n \tespecially on slow filesystems. If not set, the value of\n \t`transfer.unpackLimit` is used instead.\n \n+receive.objectCountLimit::\n+\tIf the number of objects received in a push exceeds this limit,\n+\tthen the entire push will be refused. This is meant to prevent\n+\tan unintended large push (typically a result of the user not\n+\tbeing aware of exactly what is being pushed, e.g. pushing a\n+\tlarge rewritten history) from entering the repo. If not set,\n+\tthere is no upper limit on the number of objects transferred\n+\tin a single push.\n+\n receive.denyDeletes::\n \tIf set to true, git-receive-pack will deny a ref update that deletes\n \tthe ref. Use this to prevent such a ref deletion via a push.\ndiff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt\nindex 11849a3..aac66af 100644\n--- a/Documentation/technical/protocol-capabilities.txt\n+++ b/Documentation/technical/protocol-capabilities.txt\n@@ -205,5 +205,7 @@ the server. If the check fails, the client must abort the upload, and\n report the reason for the aborted push back to the user.\n The following \"limit-*\" capabilites are recognized:\n \n+ - limit-object-count= (Maximum number of objects in a pack)\n+\n More \"limit-*\" capabilities may be added in the future. The client\n is free to ignore any \"limit-*\" capabilities it does not understand.\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex c55989d..d919e17 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -28,6 +28,7 @@ static int receive_fsck_objects;\n static int receive_unpack_limit = -1;\n static int transfer_unpack_limit = -1;\n static int unpack_limit = 100;\n+static unsigned long limit_object_count;\n static int report_status;\n static int use_sideband;\n static int prefer_ofs_delta = 1;\n@@ -73,6 +74,11 @@ static int receive_pack_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n+\tif (strcmp(var, \"receive.objectcountlimit\") == 0) {\n+\t\tlimit_object_count = git_config_ulong(var, value);\n+\t\treturn 0;\n+\t}\n+\n \tif (strcmp(var, \"receive.fsckobjects\") == 0) {\n \t\treceive_fsck_objects = git_config_bool(var, value);\n \t\treturn 0;\n@@ -112,6 +118,9 @@ static const char *capabilities()\n \tint ret = snprintf(buf, sizeof(buf),\n \t\t\t \" report-status delete-refs side-band-64k%s\",\n \t\t\t prefer_ofs_delta ? \" ofs-delta\" : \"\");\n+\tif (limit_object_count > 0)\n+\t\tret += snprintf(buf + ret, sizeof(buf) - ret,\n+\t\t\t\t\" limit-object-count=%lu\", limit_object_count);\n \tassert(ret < sizeof(buf));\n \treturn buf;\n }\n@@ -656,7 +665,9 @@ static const char *unpack(void)\n \t\t\t\"--pack_header=%\"PRIu32\",%\"PRIu32,\n \t\t\tntohl(hdr.hdr_version), ntohl(hdr.hdr_entries));\n \n-\tif (ntohl(hdr.hdr_entries) < unpack_limit) {\n+\tif (limit_object_count > 0 && ntohl(hdr.hdr_entries) > limit_object_count)\n+\t\treturn \"received pack exceeds configured receive.objectCountLimit\";\n+\telse if (ntohl(hdr.hdr_entries) < unpack_limit) {\n \t\tint code, i = 0;\n \t\tconst char *unpacker[4];\n \t\tunpacker[i++] = \"unpack-objects\";\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex f571917..4103116 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -49,9 +49,11 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext\n \t\tNULL,\n \t\tNULL,\n \t\tNULL,\n+\t\tNULL,\n \t};\n \tstruct child_process po;\n \tint i;\n+\tchar buf[40];\n \n \ti = 4;\n \tif (args->use_thin_pack)\n@@ -62,6 +64,11 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext\n \t\targv[i++] = \"-q\";\n \tif (args->progress)\n \t\targv[i++] = \"--progress\";\n+\tif (args->max_object_count > 0) {\n+\t\tsnprintf(buf, sizeof(buf), \"--max-object-count=%lu\",\n+\t\t\t args->max_object_count);\n+\t\targv[i++] = buf;\n+\t}\n \tmemset(&po, 0, sizeof(po));\n \tpo.argv = argv;\n \tpo.in = -1;\n@@ -253,6 +260,7 @@ int send_pack(struct send_pack_args *args,\n \tunsigned cmds_sent = 0;\n \tint ret = 0;\n \tstruct async demux;\n+\tconst char *p;\n \n \t/* Does the other end support the reporting? */\n \tif (server_supports(\"report-status\"))\n@@ -263,6 +271,8 @@ int send_pack(struct send_pack_args *args,\n \t\targs->use_ofs_delta = 1;\n \tif (server_supports(\"side-band-64k\"))\n \t\tuse_sideband = 1;\n+\tif ((p = server_supports(\"limit-object-count=\")))\n+\t\targs->max_object_count = strtoul(p + 19, NULL, 10);\n \n \tif (!remote_refs) {\n \t\tfprintf(stderr, \"No refs in common and none specified; doing nothing.\\n\"\ndiff --git a/send-pack.h b/send-pack.h\nindex 05d7ab1..eaacb20 100644\n--- a/send-pack.h\n+++ b/send-pack.h\n@@ -12,6 +12,7 @@ struct send_pack_args {\n \t\tuse_ofs_delta:1,\n \t\tdry_run:1,\n \t\tstateless_rpc:1;\n+\tunsigned long max_object_count;\n };\n \n int send_pack(struct send_pack_args *args,\ndiff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh\nindex 0eace37..f1f8f80 100755\n--- a/t/t5400-send-pack.sh\n+++ b/t/t5400-send-pack.sh\n@@ -222,4 +222,70 @@ test_expect_success 'deny pushing to delete current branch' '\n \t)\n '\n \n+echo \"0000\" > pkt-flush\n+\n+test_expect_success 'verify that limit-object-count capability is not advertised by default' '\n+\trewound_push_setup &&\n+\t(\n+\t cd parent &&\n+\t test_might_fail git receive-pack . <../pkt-flush >output &&\n+\t test_must_fail grep -q \"limit-object-count\" output\n+\t)\n+'\n+\n+test_expect_success 'verify that receive.objectCountLimit triggers limit-object-count capability' '\n+\t(\n+\t cd parent &&\n+\t git config receive.objectCountLimit 1 &&\n+\t test_might_fail git receive-pack . <../pkt-flush >output &&\n+\t grep -q \"limit-object-count=1\" output\n+\t)\n+'\n+\n+test_expect_success 'deny pushing when receive.objectCountLimit is exceeded' '\n+\t(\n+\t cd child &&\n+\t git reset --hard origin/master &&\n+\t echo three > file && git commit -a -m three &&\n+\t test_must_fail git send-pack ../parent master 2>errs &&\n+\t grep -q \"object count limit\" errs\n+\t) &&\n+\tparent_head=$(cd parent && git rev-parse --verify master) &&\n+\tchild_head=$(cd child && git rev-parse --verify master) &&\n+\ttest \"$parent_head\" != \"$child_head\"\n+'\n+\n+test_expect_success 'repeated push failure proves that objects were not stored remotely' '\n+\t(\n+\t cd child &&\n+\t test_must_fail git send-pack ../parent master 2>errs &&\n+\t grep -q \"object count limit\" errs\n+\t) &&\n+\tparent_head=$(cd parent && git rev-parse --verify master) &&\n+\tchild_head=$(cd child && git rev-parse --verify master) &&\n+\ttest \"$parent_head\" != \"$child_head\"\n+'\n+\n+test_expect_success 'increase receive.objectCountLimit' '\n+\t(\n+\t cd parent &&\n+\t git config receive.objectCountLimit 10 &&\n+\t test_might_fail git receive-pack . <../pkt-flush >output &&\n+\t grep -q \"limit-object-count=10\" output\n+\t)\n+'\n+\n+test_expect_success 'push is allowed when object limit is not exceeded' '\n+\t(\n+\t cd child &&\n+\t git send-pack ../parent master 2>errs &&\n+\t test_must_fail grep -q \"object count limit\" errs &&\n+\t # Also no error message from remote receive-pack\n+\t test_must_fail grep -q \"receive\\\\.objectCountLimit\" errs\n+\t) &&\n+\tparent_head=$(cd parent && git rev-parse --verify master) &&\n+\tchild_head=$(cd child && git rev-parse --verify master) &&\n+\ttest \"$parent_head\" = \"$child_head\"\n+'\n+\n test_done\n-- \n1.7.5.rc1.3.g4d7b\n"},{"id":"167934","messageId":"1305495440-30836-9-git-send-email-johan@herland.net","threadId":"27346","inReplyTo":"1305495440-30836-1-git-send-email-johan@herland.net","subject":"[PATCHv3 8/9] send-pack/receive-pack: Allow server to refuse pushing too large packs","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-05-15T21:37:19Z","receivedAt":"2011-05-15T21:37:19Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"Add a new receive.packSizeLimit config variable which defines an upper\nlimit on the pack size to accept in a single push.\n\nThis limit is advertised to clients, using the new \"limit-pack-size=\"\ncapability. The client side - aka. send-pack - parses this capability and\nforwards it to pack-objects, using the --max-pack-size option.\npack-objects then checks the generated pack against the limit and aborts\nthe pack transmission if the pack becomes too large.\n\nHowever, older clients that do not understand the capability will not check\ntheir pack against the limit, and will end up pushing the pack to the server.\nCurrently there is no extra check on the server to detect a push that exceeds\nreceive.packSizeLimit. However, such a check could be done in a pre-receive\nor update hook.\n\nDocumentation and tests are included.\n\nSigned-off-by: Johan Herland \n---\n Documentation/config.txt | 9 +++\n Documentation/technical/protocol-capabilities.txt | 1 +\n builtin/receive-pack.c | 10 +++-\n builtin/send-pack.c | 14 ++++-\n send-pack.h | 1 +\n t/t5400-send-pack.sh | 62 +++++++++++++++++++++\n 6 files changed, 93 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex fd6c130..acc1ef2 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1599,6 +1599,15 @@ receive.unpackLimit::\n \tespecially on slow filesystems. If not set, the value of\n \t`transfer.unpackLimit` is used instead.\n \n+receive.packSizeLimit::\n+\tIf the pack file transferred in a push exceeds this limit,\n+\tthen the entire push will be refused. This is meant to prevent\n+\tan unintended large push (typically a result of the user not\n+\tbeing aware of exactly what is being pushed, e.g. pushing a\n+\tlarge rewritten history) from entering the repo. If not set,\n+\tthere is no upper limit on the size of the pack transferred\n+\tin a single push.\n+\n receive.objectCountLimit::\n \tIf the number of objects received in a push exceeds this limit,\n \tthen the entire push will be refused. This is meant to prevent\ndiff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt\nindex aac66af..d3921a9 100644\n--- a/Documentation/technical/protocol-capabilities.txt\n+++ b/Documentation/technical/protocol-capabilities.txt\n@@ -205,6 +205,7 @@ the server. If the check fails, the client must abort the upload, and\n report the reason for the aborted push back to the user.\n The following \"limit-*\" capabilites are recognized:\n \n+ - limit-pack-size= (Maximum size (in bytes) of uploaded pack)\n - limit-object-count= (Maximum number of objects in a pack)\n \n More \"limit-*\" capabilities may be added in the future. The client\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex d919e17..97c154b 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -28,7 +28,7 @@ static int receive_fsck_objects;\n static int receive_unpack_limit = -1;\n static int transfer_unpack_limit = -1;\n static int unpack_limit = 100;\n-static unsigned long limit_object_count;\n+static unsigned long limit_pack_size, limit_object_count;\n static int report_status;\n static int use_sideband;\n static int prefer_ofs_delta = 1;\n@@ -74,6 +74,11 @@ static int receive_pack_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n+\tif (strcmp(var, \"receive.packsizelimit\") == 0) {\n+\t\tlimit_pack_size = git_config_ulong(var, value);\n+\t\treturn 0;\n+\t}\n+\n \tif (strcmp(var, \"receive.objectcountlimit\") == 0) {\n \t\tlimit_object_count = git_config_ulong(var, value);\n \t\treturn 0;\n@@ -118,6 +123,9 @@ static const char *capabilities()\n \tint ret = snprintf(buf, sizeof(buf),\n \t\t\t \" report-status delete-refs side-band-64k%s\",\n \t\t\t prefer_ofs_delta ? \" ofs-delta\" : \"\");\n+\tif (limit_pack_size > 0)\n+\t\tret += snprintf(buf + ret, sizeof(buf) - ret,\n+\t\t\t\t\" limit-pack-size=%lu\", limit_pack_size);\n \tif (limit_object_count > 0)\n \t\tret += snprintf(buf + ret, sizeof(buf) - ret,\n \t\t\t\t\" limit-object-count=%lu\", limit_object_count);\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex 4103116..e7c82ce 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -50,10 +50,11 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext\n \t\tNULL,\n \t\tNULL,\n \t\tNULL,\n+\t\tNULL,\n \t};\n \tstruct child_process po;\n \tint i;\n-\tchar buf[40];\n+\tchar buf1[40], buf2[40];\n \n \ti = 4;\n \tif (args->use_thin_pack)\n@@ -64,10 +65,15 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext\n \t\targv[i++] = \"-q\";\n \tif (args->progress)\n \t\targv[i++] = \"--progress\";\n+\tif (args->max_pack_size > 0) {\n+\t\tsnprintf(buf1, sizeof(buf1), \"--max-pack-size=%lu\",\n+\t\t\t args->max_pack_size);\n+\t\targv[i++] = buf1;\n+\t}\n \tif (args->max_object_count > 0) {\n-\t\tsnprintf(buf, sizeof(buf), \"--max-object-count=%lu\",\n+\t\tsnprintf(buf2, sizeof(buf2), \"--max-object-count=%lu\",\n \t\t\t args->max_object_count);\n-\t\targv[i++] = buf;\n+\t\targv[i++] = buf2;\n \t}\n \tmemset(&po, 0, sizeof(po));\n \tpo.argv = argv;\n@@ -271,6 +277,8 @@ int send_pack(struct send_pack_args *args,\n \t\targs->use_ofs_delta = 1;\n \tif (server_supports(\"side-band-64k\"))\n \t\tuse_sideband = 1;\n+\tif ((p = server_supports(\"limit-pack-size=\")))\n+\t\targs->max_pack_size = strtoul(p + 16, NULL, 10);\n \tif ((p = server_supports(\"limit-object-count=\")))\n \t\targs->max_object_count = strtoul(p + 19, NULL, 10);\n \ndiff --git a/send-pack.h b/send-pack.h\nindex eaacb20..1c6c202 100644\n--- a/send-pack.h\n+++ b/send-pack.h\n@@ -12,6 +12,7 @@ struct send_pack_args {\n \t\tuse_ofs_delta:1,\n \t\tdry_run:1,\n \t\tstateless_rpc:1;\n+\tunsigned long max_pack_size;\n \tunsigned long max_object_count;\n };\n \ndiff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh\nindex f1f8f80..26fd1b4 100755\n--- a/t/t5400-send-pack.sh\n+++ b/t/t5400-send-pack.sh\n@@ -288,4 +288,66 @@ test_expect_success 'push is allowed when object limit is not exceeded' '\n \ttest \"$parent_head\" = \"$child_head\"\n '\n \n+test_expect_success 'verify that limit-pack-size capability is not advertised by default' '\n+\trewound_push_setup &&\n+\t(\n+\t cd parent &&\n+\t test_might_fail git receive-pack . <../pkt-flush >output &&\n+\t test_must_fail grep -q \"limit-pack-size\" output\n+\t)\n+'\n+\n+test_expect_success 'verify that receive.packSizeLimit triggers limit-pack-size capability' '\n+\t(\n+\t cd parent &&\n+\t git config receive.packSizeLimit 10 &&\n+\t test_might_fail git receive-pack . <../pkt-flush >output &&\n+\t grep -q \"limit-pack-size=10\" output\n+\t)\n+'\n+\n+test_expect_success 'deny pushing when receive.packSizeLimit is exceeded' '\n+\t(\n+\t cd child &&\n+\t git reset --hard origin/master &&\n+\t echo three > file && git commit -a -m three &&\n+\t test_must_fail git send-pack ../parent master 2>errs &&\n+\t grep -q \"pack size limit\" errs\n+\t) &&\n+\tparent_head=$(cd parent && git rev-parse --verify master) &&\n+\tchild_head=$(cd child && git rev-parse --verify master) &&\n+\ttest \"$parent_head\" != \"$child_head\"\n+'\n+\n+test_expect_success 'repeated push failure proves that objects were not stored remotely' '\n+\t(\n+\t cd child &&\n+\t test_must_fail git send-pack ../parent master 2>errs &&\n+\t grep -q \"pack size limit\" errs\n+\t) &&\n+\tparent_head=$(cd parent && git rev-parse --verify master) &&\n+\tchild_head=$(cd child && git rev-parse --verify master) &&\n+\ttest \"$parent_head\" != \"$child_head\"\n+'\n+\n+test_expect_success 'increase receive.packSizeLimit' '\n+\t(\n+\t cd parent &&\n+\t git config receive.packSizeLimit 1000000 &&\n+\t test_might_fail git receive-pack . <../pkt-flush >output &&\n+\t grep -q \"limit-pack-size=1000000\" output\n+\t)\n+'\n+\n+test_expect_success 'push is allowed when pack size is not exceeded' '\n+\t(\n+\t cd child &&\n+\t git send-pack ../parent master 2>errs &&\n+\t test_must_fail grep -q \"pack size limit\" errs\n+\t) &&\n+\tparent_head=$(cd parent && git rev-parse --verify master) &&\n+\tchild_head=$(cd child && git rev-parse --verify master) &&\n+\ttest \"$parent_head\" = \"$child_head\"\n+'\n+\n test_done\n-- \n1.7.5.rc1.3.g4d7b\n"},{"id":"167931","messageId":"1305495440-30836-10-git-send-email-johan@herland.net","threadId":"27346","inReplyTo":"1305495440-30836-1-git-send-email-johan@herland.net","subject":"[PATCHv3 9/9] send-pack/receive-pack: Allow server to refuse pushes with too many commits","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-05-15T21:37:20Z","receivedAt":"2011-05-15T21:37:20Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"Add a new receive.commitCountLimit config variable which defines an upper\nlimit on the number of commits to accept in a single push.\n\nThis limit is advertised to clients, using the new \"limit-commit-count=\"\ncapability. The client side - aka. send-pack - parses this capability and\nforwards it to pack-objects, using the recently added --max-commit-count\noption. pack-objects then checks the generated pack against the limit and\naborts the pack generation if the pack would have too many objects.\n\nHowever, older clients that do not understand the capability will not check\ntheir pack against the limit, and will end up pushing the pack to the server.\nCurrently there is no extra check on the server to detect a push that exceeds\nreceive.commitCountLimit. However, such a check could be done in a pre-receive\nor update hook.\n\nDocumentation and tests are included.\n\nSigned-off-by: Johan Herland \n---\n Documentation/config.txt | 9 +++\n Documentation/technical/protocol-capabilities.txt | 1 +\n builtin/receive-pack.c | 10 +++-\n builtin/send-pack.c | 10 +++-\n send-pack.h | 1 +\n t/t5400-send-pack.sh | 63 +++++++++++++++++++++\n 6 files changed, 92 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex acc1ef2..c63a224 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1617,6 +1617,15 @@ receive.objectCountLimit::\n \tthere is no upper limit on the number of objects transferred\n \tin a single push.\n \n+receive.commitCountLimit::\n+\tIf the number of commits received in a push exceeds this limit,\n+\tthen the entire push will be refused. This is meant to prevent\n+\tan unintended large push (typically a result of the user not\n+\tbeing aware of exactly what is being pushed, e.g. pushing a\n+\tlarge rewritten history) from entering the repo. If not set,\n+\tthere is no upper limit on the number of commits transferred\n+\tin a single push.\n+\n receive.denyDeletes::\n \tIf set to true, git-receive-pack will deny a ref update that deletes\n \tthe ref. Use this to prevent such a ref deletion via a push.\ndiff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt\nindex d3921a9..c16240b 100644\n--- a/Documentation/technical/protocol-capabilities.txt\n+++ b/Documentation/technical/protocol-capabilities.txt\n@@ -207,6 +207,7 @@ The following \"limit-*\" capabilites are recognized:\n \n - limit-pack-size= (Maximum size (in bytes) of uploaded pack)\n - limit-object-count= (Maximum number of objects in a pack)\n+ - limit-commit-count= (Maximum number of commits in a pack)\n \n More \"limit-*\" capabilities may be added in the future. The client\n is free to ignore any \"limit-*\" capabilities it does not understand.\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 97c154b..1d1170b 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -28,7 +28,7 @@ static int receive_fsck_objects;\n static int receive_unpack_limit = -1;\n static int transfer_unpack_limit = -1;\n static int unpack_limit = 100;\n-static unsigned long limit_pack_size, limit_object_count;\n+static unsigned long limit_pack_size, limit_object_count, limit_commit_count;\n static int report_status;\n static int use_sideband;\n static int prefer_ofs_delta = 1;\n@@ -84,6 +84,11 @@ static int receive_pack_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n+\tif (strcmp(var, \"receive.commitcountlimit\") == 0) {\n+\t\tlimit_commit_count = git_config_ulong(var, value);\n+\t\treturn 0;\n+\t}\n+\n \tif (strcmp(var, \"receive.fsckobjects\") == 0) {\n \t\treceive_fsck_objects = git_config_bool(var, value);\n \t\treturn 0;\n@@ -129,6 +134,9 @@ static const char *capabilities()\n \tif (limit_object_count > 0)\n \t\tret += snprintf(buf + ret, sizeof(buf) - ret,\n \t\t\t\t\" limit-object-count=%lu\", limit_object_count);\n+\tif (limit_commit_count > 0)\n+\t\tret += snprintf(buf + ret, sizeof(buf) - ret,\n+\t\t\t\t\" limit-commit-count=%lu\", limit_commit_count);\n \tassert(ret < sizeof(buf));\n \treturn buf;\n }\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex e7c82ce..0ef68ea 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -51,10 +51,11 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext\n \t\tNULL,\n \t\tNULL,\n \t\tNULL,\n+\t\tNULL,\n \t};\n \tstruct child_process po;\n \tint i;\n-\tchar buf1[40], buf2[40];\n+\tchar buf1[40], buf2[40], buf3[40];\n \n \ti = 4;\n \tif (args->use_thin_pack)\n@@ -75,6 +76,11 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext\n \t\t\t args->max_object_count);\n \t\targv[i++] = buf2;\n \t}\n+\tif (args->max_commit_count > 0) {\n+\t\tsnprintf(buf3, sizeof(buf3), \"--max-commit-count=%lu\",\n+\t\t\t args->max_commit_count);\n+\t\targv[i++] = buf3;\n+\t}\n \tmemset(&po, 0, sizeof(po));\n \tpo.argv = argv;\n \tpo.in = -1;\n@@ -281,6 +287,8 @@ int send_pack(struct send_pack_args *args,\n \t\targs->max_pack_size = strtoul(p + 16, NULL, 10);\n \tif ((p = server_supports(\"limit-object-count=\")))\n \t\targs->max_object_count = strtoul(p + 19, NULL, 10);\n+\tif ((p = server_supports(\"limit-commit-count=\")))\n+\t\targs->max_commit_count = strtoul(p + 19, NULL, 10);\n \n \tif (!remote_refs) {\n \t\tfprintf(stderr, \"No refs in common and none specified; doing nothing.\\n\"\ndiff --git a/send-pack.h b/send-pack.h\nindex 1c6c202..9d28cc5 100644\n--- a/send-pack.h\n+++ b/send-pack.h\n@@ -14,6 +14,7 @@ struct send_pack_args {\n \t\tstateless_rpc:1;\n \tunsigned long max_pack_size;\n \tunsigned long max_object_count;\n+\tunsigned long max_commit_count;\n };\n \n int send_pack(struct send_pack_args *args,\ndiff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh\nindex 26fd1b4..a71bc47 100755\n--- a/t/t5400-send-pack.sh\n+++ b/t/t5400-send-pack.sh\n@@ -350,4 +350,67 @@ test_expect_success 'push is allowed when pack size is not exceeded' '\n \ttest \"$parent_head\" = \"$child_head\"\n '\n \n+test_expect_success 'verify that limit-commit-count capability is not advertised by default' '\n+\trewound_push_setup &&\n+\t(\n+\t cd parent &&\n+\t test_might_fail git receive-pack . <../pkt-flush >output &&\n+\t test_must_fail grep -q \"limit-commit-count\" output\n+\t)\n+'\n+\n+test_expect_success 'verify that receive.commitCountLimit triggers limit-commit-count capability' '\n+\t(\n+\t cd parent &&\n+\t git config receive.commitCountLimit 1 &&\n+\t test_might_fail git receive-pack . <../pkt-flush >output &&\n+\t grep -q \"limit-commit-count=1\" output\n+\t)\n+'\n+\n+test_expect_success 'deny pushing when receive.commitCountLimit is exceeded' '\n+\t(\n+\t cd child &&\n+\t git reset --hard origin/master &&\n+\t echo three > file && git commit -a -m three &&\n+\t echo four > file && git commit -a -m four &&\n+\t test_must_fail git send-pack ../parent master 2>errs &&\n+\t grep -q \"commit count limit\" errs\n+\t) &&\n+\tparent_head=$(cd parent && git rev-parse --verify master) &&\n+\tchild_head=$(cd child && git rev-parse --verify master) &&\n+\ttest \"$parent_head\" != \"$child_head\"\n+'\n+\n+test_expect_success 'repeated push failure proves that objects were not stored remotely' '\n+\t(\n+\t cd child &&\n+\t test_must_fail git send-pack ../parent master 2>errs &&\n+\t grep -q \"commit count limit\" errs\n+\t) &&\n+\tparent_head=$(cd parent && git rev-parse --verify master) &&\n+\tchild_head=$(cd child && git rev-parse --verify master) &&\n+\ttest \"$parent_head\" != \"$child_head\"\n+'\n+\n+test_expect_success 'increase receive.commitCountLimit' '\n+\t(\n+\t cd parent &&\n+\t git config receive.commitCountLimit 2 &&\n+\t test_might_fail git receive-pack . <../pkt-flush >output &&\n+\t grep -q \"limit-commit-count=2\" output\n+\t)\n+'\n+\n+test_expect_success 'push is allowed when commit limit is not exceeded' '\n+\t(\n+\t cd child &&\n+\t git send-pack ../parent master 2>errs &&\n+\t test_must_fail grep -q \"commit count limit\" errs\n+\t) &&\n+\tparent_head=$(cd parent && git rev-parse --verify master) &&\n+\tchild_head=$(cd child && git rev-parse --verify master) &&\n+\ttest \"$parent_head\" = \"$child_head\"\n+'\n+\n test_done\n-- \n1.7.5.rc1.3.g4d7b\n"},{"id":"167935","messageId":"BANLkTi=yKYw+kyPBPHSB6QfpjDqOi+WOmg@mail.gmail.com","threadId":"27346","inReplyTo":"1305495440-30836-1-git-send-email-johan@herland.net","subject":"Re: [PATCHv3 0/9] Push limits","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2011-05-15T21:52:10Z","receivedAt":"2011-05-15T21:52:10Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sun, May 15, 2011 at 23:37, Johan Herland wrote:\n> Here's the next iteration of what started out as a simple patch to let\n> the server refuse pushes that exceeded a configurable limit on the\n> #objects in a pack.\n>\n> The current patch series allows limiting pushes by\n>  - size of pack\n>  - #objects in pack\n>  - #commits in pack\n\nFWIW I'd find this very useful. I recently spent a fair amount of time\ncleaning up the mess created by a user pushing all the tags from\nrepository A to repository B (don't ask), the options you've\nimplemented here would have stopped that.\n\nAnd as you point out even if you refuse these sort of things with a\nhook (which I later implemented) that doesn't stop the server from\naccepting the objects and keeping them around.\n"},{"id":"167936","messageId":"BANLkTingc375Kzm-TBS4aQ_912pg7G03cQ@mail.gmail.com","threadId":"27346","inReplyTo":"1305495440-30836-4-git-send-email-johan@herland.net","subject":"Re: [PATCHv3 3/9] pack-objects: Allow --max-pack-size to be used together with --stdout","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2011-05-15T22:06:00Z","receivedAt":"2011-05-15T22:06:00Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Sun, May 15, 2011 at 14:37, Johan Herland wrote:\n> Currently we refuse combining --max-pack-size with --stdout since there's\n> no way to make multiple packs when the pack is written to stdout. However,\n> we want to be able to limit the maximum size of the pack created by\n> --stdout (and abort pack-objects if we are unable to meet that limit).\n>\n> Therefore, when used together with --stdout, we reinterpret --max-pack-size\n> to indicate the maximum pack size which - if exceeded - will cause\n> pack-objects to abort with an error message.\n...\n>                if (pack_to_stdout) {\n> +                       if (nr_written != nr_remaining)\n> +                               die(\"unable to make pack within the pack size\"\n> +                                   \" limit (%lu bytes)\", pack_size_limit);\n\nI think this is too late. We have already output a bunch of data, up\nto the size limit at this point. If the size limit is non-trivial\n(e.g. 5 MB) we have already sent most of that to the remote side, and\nits already written some of that out to disk.\n\nI'd like this to be a soft limit derived from the reused object sizes.\nWhen planning the pack by looking at where we will reuse an object\nfrom, sum those sizes. If the sum of these sizes would break this\nlimit, then we abort before even writing the pack header out.\n\n-- \nShawn.\n"},{"id":"167937","messageId":"BANLkTi=qtA=34=pdXuJ_cQu0pv6BnE2uSA@mail.gmail.com","threadId":"27346","inReplyTo":"1305495440-30836-5-git-send-email-johan@herland.net","subject":"Re: [PATCHv3 4/9] pack-objects: Teach new option --max-object-count, similar to --max-pack-size","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2011-05-15T22:07:17Z","receivedAt":"2011-05-15T22:07:17Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Sun, May 15, 2011 at 14:37, Johan Herland wrote:\n> The new --max-object-count option behaves similarly to --max-pack-size,\n> except that the decision to split packs is determined by the number of\n> objects in the pack, and not by the size of the pack.\n\nLike my note about pack size for this case... I think doing this\nduring writing is too late. We should be aborting the counting phase\nif the output pack is to stdout and we are going to exceed this limit.\n\n-- \nShawn.\n"},{"id":"167938","messageId":"201105160031.15339.johan@herland.net","threadId":"27346","inReplyTo":"BANLkTi=qtA=34=pdXuJ_cQu0pv6BnE2uSA@mail.gmail.com","subject":"Re: [PATCHv3 4/9] pack-objects: Teach new option --max-object-count, similar to --max-pack-size","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-05-15T22:31:15Z","receivedAt":"2011-05-15T22:31:15Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Monday 16 May 2011, Shawn Pearce wrote:\n> On Sun, May 15, 2011 at 14:37, Johan Herland wrote:\n> > The new --max-object-count option behaves similarly to --max-pack-size,\n> > except that the decision to split packs is determined by the number of\n> > objects in the pack, and not by the size of the pack.\n> \n> Like my note about pack size for this case... I think doing this\n> during writing is too late. We should be aborting the counting phase\n> if the output pack is to stdout and we are going to exceed this limit.\n\nThe patch actually does this in the --stdout case. Look at the last\nhunk in builtin/pack-objects.c:\n\n@@ -2349,6 +2366,9 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \n if (non_empty && !nr_result)\n return 0;\n+ if (pack_to_stdout && object_count_limit && object_count_limit < nr_result)\n+ die(\"unable to make pack within the object count limit\"\n+ \" (%lu objects)\", object_count_limit);\n if (nr_result)\n prepare_pack(window, depth);\n write_pack_file();\n\nSo in the --stdout case, we have already aborted before we start\nwriting the pack (i.e. after the counting phase).\n\nThe commit message you quote above, are for the case where someone uses\n--max-object-count _without_ --stdout, in which case we compare\nnr_written to object_count_limit to determine when to split the pack.\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"167940","messageId":"BANLkTi=zB=6eVHdG8N5LLDVx=1hK0tSNGQ@mail.gmail.com","threadId":"27346","inReplyTo":"201105160031.15339.johan@herland.net","subject":"Re: [PATCHv3 4/9] pack-objects: Teach new option --max-object-count, similar to --max-pack-size","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2011-05-15T23:48:11Z","receivedAt":"2011-05-15T23:48:11Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Sun, May 15, 2011 at 15:31, Johan Herland wrote:\n> On Monday 16 May 2011, Shawn Pearce wrote:\n>> On Sun, May 15, 2011 at 14:37, Johan Herland wrote:\n>> > The new --max-object-count option behaves similarly to --max-pack-size,\n>> > except that the decision to split packs is determined by the number of\n>> > objects in the pack, and not by the size of the pack.\n>>\n>> Like my note about pack size for this case... I think doing this\n>> during writing is too late. We should be aborting the counting phase\n>> if the output pack is to stdout and we are going to exceed this limit.\n>\n> The patch actually does this in the --stdout case. Look at the last\n> hunk in builtin/pack-objects.c:\n>\n> @@ -2349,6 +2366,9 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n>\n>        if (non_empty && !nr_result)\n>                return 0;\n> +       if (pack_to_stdout && object_count_limit && object_count_limit < nr_result)\n> +               die(\"unable to make pack within the object count limit\"\n> +                       \" (%lu objects)\", object_count_limit);\n>        if (nr_result)\n>                prepare_pack(window, depth);\n>        write_pack_file();\n>\n> So in the --stdout case, we have already aborted before we start\n> writing the pack (i.e. after the counting phase).\n>\n> The commit message you quote above, are for the case where someone uses\n> --max-object-count _without_ --stdout, in which case we compare\n> nr_written to object_count_limit to determine when to split the pack.\n\nThanks for the clarification. Its Sunday, I am clearly not scanning\npatches with the level of detail I should be. :-)\n\nGiven that this block is in here, most of the series looks pretty good\nto me. Thanks for following up with this round, I know its a lot more\nthan you originally wanted to do for this \"simple\" limit, but I think\nits a worthwhile improvement.\n\n-- \nShawn.\n"},{"id":"167957","messageId":"201105160339.25737.johan@herland.net","threadId":"27346","inReplyTo":"BANLkTingc375Kzm-TBS4aQ_912pg7G03cQ@mail.gmail.com","subject":"Re: [PATCHv3 3/9] pack-objects: Allow --max-pack-size to be used together with --stdout","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-05-16T01:39:25Z","receivedAt":"2011-05-16T01:39:25Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Monday 16 May 2011, Shawn Pearce wrote:\n> On Sun, May 15, 2011 at 14:37, Johan Herland wrote:\n> > Currently we refuse combining --max-pack-size with --stdout since\n> > there's no way to make multiple packs when the pack is written to\n> > stdout. However, we want to be able to limit the maximum size of the\n> > pack created by --stdout (and abort pack-objects if we are unable to\n> > meet that limit).\n> > \n> > Therefore, when used together with --stdout, we reinterpret\n> > --max-pack-size to indicate the maximum pack size which - if exceeded\n> > - will cause pack-objects to abort with an error message.\n> \n> ...\n> \n> > if (pack_to_stdout) {\n> > + if (nr_written != nr_remaining)\n> > + die(\"unable to make pack within the pack size\"\n> > + \" limit (%lu bytes)\", pack_size_limit);\n> \n> I think this is too late. We have already output a bunch of data, up\n> to the size limit at this point. If the size limit is non-trivial\n> (e.g. 5 MB) we have already sent most of that to the remote side, and\n> its already written some of that out to disk.\n> \n> I'd like this to be a soft limit derived from the reused object sizes.\n> When planning the pack by looking at where we will reuse an object\n> from, sum those sizes. If the sum of these sizes would break this\n> limit, then we abort before even writing the pack header out.\n\nI agree, but it's currently late Sunday (early Monday), and after\nlooking at this for a while, I'm no longer thinking straight.\nIf someone that groks the pack-objects internal could help out, I'd be\nreally grateful. AFAICS, we need to drill into prepare_pack() to find\nthe details needed to estimate the total pack size, but I don't know\nexactly which data structure(s) holds the data needed. We probably need\nto accumulate a pack size estimate in find_deltas(), and then sum those\nacross the threads, before we finally compare the total estimate to\npack_size_limit prior to calling write_pack_file().\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"167958","messageId":"20110516040745.GA23583@sigill.intra.peff.net","threadId":"27346","inReplyTo":"1305495440-30836-3-git-send-email-johan@herland.net","subject":"Re: [PATCHv3 2/9] send-pack: Attempt to retrieve remote status even if pack-objects fails","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-16T04:07:45Z","receivedAt":"2011-05-16T04:07:45Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 15, 2011 at 11:37:13PM +0200, Johan Herland wrote:\n\n> I first wrote this patch on a base where e07fd15 (Peff's \"send-pack:\n> unbreak push over stateless rpc\") was not present, and then resolved\n> a conflict when rebasing this patch onto current master. I hope Peff\n> or Johannes (Sixt) can verify that my patch does not reintroduce the\n> deadlock they fixed.\n\nI don't think it reintroduces the deadlock we fixed, but I am worried\nthat it produces a new, similar one. That is, imagine pack-objects fails\nhorribly, maybe or maybe not producing any output. We close its pipe\noutgoing pipe at the end of run_command, and per Johannes' 09c9957, we\nare sure that the sideband demuxer does not hold a pipe end open,\neither.\n\nSo the remote side sees us close our end of the pipe, knows there is no\nmore pack data, and then closes their end. So our receive_status should\neither get some error message, or EOF, either of which is fine. So no\ndeadlock there. Essentially, we have done a half-duplex shutdown of the\nconnection to the remote, and that is enough for everybody to keep\ngoing.\n\nBut what if we are not using pipes, but have an actual TCP socket? In\nthat case, I'm not sure what happens. We don't seem to do a half-duplex\nshutdown() anywhere. So I'm concerned that we are still open for sending\nfrom the remote's perspective, and we may deadlock.\n\nHowever, that would not necessarily be something introduced by your\npatch; you would deadlock in receive_status, but prior to that it would\ndeadlock in the sideband demuxer.\n\nAFAICT, the only way to have an actual TCP connection instead of pipes\nis for the push to go over git://, which is enabled almost nowhere. But\nwe should perhaps check for deadlock on failed pack-objects in that\ncase, both with and without your patch.\n\n-Peff\n"},{"id":"167963","messageId":"7vsjsfmawt.fsf@alter.siamese.dyndns.org","threadId":"27346","inReplyTo":"1305495440-30836-4-git-send-email-johan@herland.net","subject":"Re: [PATCHv3 3/9] pack-objects: Allow --max-pack-size to be used together with --stdout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-16T06:12:18Z","receivedAt":"2011-05-16T06:12:18Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> Currently we refuse combining --max-pack-size with --stdout since there's\n> no way to make multiple packs when the pack is written to stdout. However,\n> we want to be able to limit the maximum size of the pack created by\n> --stdout (and abort pack-objects if we are unable to meet that limit).\n>\n> Therefore, when used together with --stdout, we reinterpret --max-pack-size\n> to indicate the maximum pack size which - if exceeded - will cause\n> pack-objects to abort with an error message.\n\nI only gave the code a cursory look, but I think your patch does more than\nthe above paragraphs say. I am not sure those extra change are justified.\n\nFor example,\n\n> @@ -229,7 +229,7 @@ static unsigned long write_object(struct sha1file *f,\n> \n> \tif (!entry->delta)\n> \t\tusable_delta = 0;\t/* no delta */\n> -\telse if (!pack_size_limit)\n> +\telse if (!pack_size_limit || pack_to_stdout)\n> \t usable_delta = 1;\t/* unlimited packfile */\n\nWhy does this conditional have to change its behaviour when writing to the\nstandard output? I thought that the only thing you are doing \"earlier we\ndidn't allow setting size limit when writing to standard output, now we\ndo\", and I do not see the linkage between that objective and this change.\n\n> @@ -2315,9 +2318,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n> \n> \tif (!pack_to_stdout && !pack_size_limit)\n> \t\tpack_size_limit = pack_size_limit_cfg;\n> -\tif (pack_to_stdout && pack_size_limit)\n> -\t\tdie(\"--max-pack-size cannot be used to build a pack for transfer.\");\n> -\tif (pack_size_limit && pack_size_limit < 1024*1024) {\n> +\tif (!pack_to_stdout && pack_size_limit && pack_size_limit < 1024*1024) {\n> \t\twarning(\"minimum pack size limit is 1 MiB\");\n> \t\tpack_size_limit = 1024*1024;\n> \t}\n\nWhy is the new combination \"writing to the standard output, but the\nmaximum size is limited\" does not have the same lower bound to pack size\nlimit while on-disk packs do?\n\nIf you have a reason to believe 1 MiB is too large for a pack size limit,\nshouldn't that logic apply equally to the on-disk case? What does this\nchange have to do with the interaction with --stdout option?\n"},{"id":"167964","messageId":"20110516061354.GA25731@sigill.intra.peff.net","threadId":"27346","inReplyTo":"20110516040745.GA23583@sigill.intra.peff.net","subject":"Re: [PATCHv3 2/9] send-pack: Attempt to retrieve remote status even if pack-objects fails","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-16T06:13:54Z","receivedAt":"2011-05-16T06:13:54Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 16, 2011 at 12:07:45AM -0400, Jeff King wrote:\n\n> But what if we are not using pipes, but have an actual TCP socket? In\n> that case, I'm not sure what happens. We don't seem to do a half-duplex\n> shutdown() anywhere. So I'm concerned that we are still open for sending\n> from the remote's perspective, and we may deadlock.\n> \n> However, that would not necessarily be something introduced by your\n> patch; you would deadlock in receive_status, but prior to that it would\n> deadlock in the sideband demuxer.\n> \n> AFAICT, the only way to have an actual TCP connection instead of pipes\n> is for the push to go over git://, which is enabled almost nowhere. But\n> we should perhaps check for deadlock on failed pack-objects in that\n> case, both with and without your patch.\n\nUgh, yeah, yet another deadlock. I can reproduce reliably with this:\n\n [in one terminal]\n mkdir daemon &&\n git init --bare daemon/repo.git &&\n git --git-dir=daemon/repo.git config daemon.receivepack true &&\n git daemon --base-path=$PWD/daemon --export-all --verbose\n\n [in another]\n git init repo &&\n cd repo &&\n git remote add origin git://localhost/repo.git &&\n echo content >file && git add file && git commit -a -m one &&\n git push -f origin HEAD &&\n echo content >>file && git commit -a -m two &&\n sha1=`git rev-parse HEAD:file` &&\n file=`echo $sha1 | sed 's,..,&/,'` &&\n rm -fv .git/objects/$file &&\n git push\n\nand this patch fixes it:\n\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex e2f4e21..b9da044 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -345,6 +345,13 @@ int send_pack(struct send_pack_args *args,\n \t\t\t\tref->status = REF_STATUS_NONE;\n \t\t\tif (args->stateless_rpc)\n \t\t\t\tclose(out);\n+\t\t\t/* in case we actually have a full-duplex socket\n+\t\t\t * and not two pipes; we can't use \"out\" because\n+\t\t\t * it has been closed already, but in the full-duplex\n+\t\t\t * case, \"in\" and \"out\" are merely dups of each other.\n+\t\t\t * We can't directly use \"in\" because it may be\n+\t\t\t * pointing to the sideband demuxer now */\n+\t\t\tshutdown(fd[0], SHUT_WR);\n \t\t\tif (use_sideband)\n \t\t\t\tfinish_async(&demux);\n \t\t\treturn -1;\n\nIt does call shutdown() on a non-socket in the pipe case. That should be\na harmless noop, AFAIK.\n\n-Peff\n"},{"id":"167965","messageId":"7vmxinmabl.fsf@alter.siamese.dyndns.org","threadId":"27346","inReplyTo":"BANLkTi=qtA=34=pdXuJ_cQu0pv6BnE2uSA@mail.gmail.com","subject":"Re: [PATCHv3 4/9] pack-objects: Teach new option --max-object-count, similar to --max-pack-size","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-16T06:25:02Z","receivedAt":"2011-05-16T06:25:02Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shawn Pearce writes:\n\n> On Sun, May 15, 2011 at 14:37, Johan Herland wrote:\n>> The new --max-object-count option behaves similarly to --max-pack-size,\n>> except that the decision to split packs is determined by the number of\n>> objects in the pack, and not by the size of the pack.\n>\n> Like my note about pack size for this case... I think doing this\n> during writing is too late. We should be aborting the counting phase\n> if the output pack is to stdout and we are going to exceed this limit.\n\nWell, even more important is if this is even useful. What is the user\ntrying to prevent from happening, and is it a useful thing?\n\nI am not interested in a literal answer \"The user is trying to prevent a\npush that pushes too many objects in a single push into a repository\". I\nam questioning why does anybody even care about the object count per-se.\n\nI think \"do not hog too much disk\" (i.e. size) is an understandable wish,\nand max-pack-size imposed on --stdout would be a good approximation for\nthat.\n\nI would understand \"this project has only these files, and pushing a tree\nthat has 100x leaves than that may be a mistake\" (i.e. recursive sum of\nnumber of entries of an individual tree). I would also sort-of understand\n\"do not push too deep a history at once\" (i.e. we do not welcome pushing a\nwildly diverged fork that has been allowed to grow for too long).\n\nBut I do not think max-object-count is a good approximation for either\nto be useful.\n\nWithout a good answer to the above question, this looks like a \"because we\ncould\", not \"because it is useful\", feature.\n"},{"id":"167966","messageId":"20110516063944.GB25731@sigill.intra.peff.net","threadId":"27346","inReplyTo":"20110516061354.GA25731@sigill.intra.peff.net","subject":"Re: [PATCHv3 2/9] send-pack: Attempt to retrieve remote status even if pack-objects fails","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-16T06:39:44Z","receivedAt":"2011-05-16T06:39:44Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 16, 2011 at 02:13:54AM -0400, Jeff King wrote:\n\n> and this patch fixes it:\n> \n> diff --git a/builtin/send-pack.c b/builtin/send-pack.c\n> index e2f4e21..b9da044 100644\n> --- a/builtin/send-pack.c\n> +++ b/builtin/send-pack.c\n> @@ -345,6 +345,13 @@ int send_pack(struct send_pack_args *args,\n> \t\t\t\tref->status = REF_STATUS_NONE;\n> \t\t\tif (args->stateless_rpc)\n> \t\t\t\tclose(out);\n> +\t\t\t/* in case we actually have a full-duplex socket\n> +\t\t\t * and not two pipes; we can't use \"out\" because\n> +\t\t\t * it has been closed already, but in the full-duplex\n> +\t\t\t * case, \"in\" and \"out\" are merely dups of each other.\n> +\t\t\t * We can't directly use \"in\" because it may be\n> +\t\t\t * pointing to the sideband demuxer now */\n> +\t\t\tshutdown(fd[0], SHUT_WR);\n> \t\t\tif (use_sideband)\n> \t\t\t\tfinish_async(&demux);\n> \t\t\treturn -1;\n> \n> It does call shutdown() on a non-socket in the pipe case. That should be\n> a harmless noop, AFAIK.\n\nIf we do care (or if we just want to be cleaner), this patch series also\nworks (and goes on top of the same deadlock topic, i.e., e07fd15):\n\n [1/3]: connect: treat generic proxy processes like ssh processes\n [2/3]: connect: let callers know if connection is a socket\n [3/3]: send-pack: avoid deadlock on git:// push with failed pack-objects\n\nAnother approach would be to actually spawn a pipe-based helper for tcp\nconnections (sort of a \"git netcat\"). That would mean all git-protocol\nconnections would get the same descriptor semantics, in case any other\nbugs are lurking. I'm not sure if the ugliness (extra process to manage)\nand decreased efficiency (pointlessly proxying data through an extra set\nof pipes) are worth it. The only thing which makes me not reject it out\nof hand is that it is already how git-over-ssh works (and not unlike\ngit-over-http), so the extra process and inefficiency are probably not\n_that_ big a deal. It just feels ugly. I wish there were a portable way\nto split a full-duplex socket into two half-duplex halves, but AFAIK,\nthat is not possible.\n\n-Peff\n"},{"id":"167967","messageId":"20110516064607.GA19078@sigill.intra.peff.net","threadId":"27346","inReplyTo":"20110516063944.GB25731@sigill.intra.peff.net","subject":"[PATCH 1/3] connect: treat generic proxy processes like ssh processes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-16T06:46:07Z","receivedAt":"2011-05-16T06:46:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The git_connect function returns two ends of a pipe for\ntalking with a remote, plus a struct child_process\nrepresenting the other end of the pipe. If we have a direct\nsocket connection, then this points to a special \"no_fork\"\nchild process.\n\nThe code path for doing git-over-pipes or git-over-ssh sets\nup this child process to point to the child git command or\nthe ssh process. When we call finish_connect eventually, we\ncheck wait() on the command and report its return value.\n\nThe code path for git://, on the other hand, always sets it\nto no_fork. In the case of a direct TCP connection, this\nmakes sense; we have no child process. But in the case of a\nproxy command (configured by core.gitproxy), we do have a\nchild process, but we throw away its pid, and therefore\nignore its return code.\n\nInstead, let's keep that information in the proxy case, and\nrespect its return code, which can help catch some errors\n(though depending on your proxy command, it will be errors\nreported by the proxy command itself, and not propagated\nfrom git commands. Still, it is probably better to propagate\nsuch errors than to ignore them).\n\nIt also means that the child_process field can reliably be\nused to determine whether the returned descriptors are\nactually a full-duplex socket, which means we should be\nusing shutdown() instead of a simple close.\n\nSigned-off-by: Jeff King \n---\nObviously I am interested mainly in the last bit for this series. But I\nconsider the rest of it a minor bugfix on its own.\n\n connect.c | 27 +++++++++++++++------------\n 1 files changed, 15 insertions(+), 12 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex db965c9..86ad150 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -403,12 +403,12 @@ static int git_use_proxy(const char *host)\n \treturn (git_proxy_command && *git_proxy_command);\n }\n \n-static void git_proxy_connect(int fd[2], char *host)\n+static struct child_process *git_proxy_connect(int fd[2], char *host)\n {\n \tconst char *port = STR(DEFAULT_GIT_PORT);\n \tchar *colon, *end;\n \tconst char *argv[4];\n-\tstruct child_process proxy;\n+\tstruct child_process *proxy;\n \n \tif (host[0] == '[') {\n \t\tend = strchr(host + 1, ']');\n@@ -431,14 +431,15 @@ static void git_proxy_connect(int fd[2], char *host)\n \targv[1] = host;\n \targv[2] = port;\n \targv[3] = NULL;\n-\tmemset(&proxy, 0, sizeof(proxy));\n-\tproxy.argv = argv;\n-\tproxy.in = -1;\n-\tproxy.out = -1;\n-\tif (start_command(&proxy))\n+\tproxy = xcalloc(1, sizeof(*proxy));\n+\tproxy->argv = argv;\n+\tproxy->in = -1;\n+\tproxy->out = -1;\n+\tif (start_command(proxy))\n \t\tdie(\"cannot start proxy %s\", argv[0]);\n-\tfd[0] = proxy.out; /* read from proxy stdout */\n-\tfd[1] = proxy.in; /* write to proxy stdin */\n+\tfd[0] = proxy->out; /* read from proxy stdout */\n+\tfd[1] = proxy->in; /* write to proxy stdin */\n+\treturn proxy;\n }\n \n #define MAX_CMD_LEN 1024\n@@ -553,9 +554,11 @@ struct child_process *git_connect(int fd[2], const char *url_orig,\n \t\t */\n \t\tchar *target_host = xstrdup(host);\n \t\tif (git_use_proxy(host))\n-\t\t\tgit_proxy_connect(fd, host);\n-\t\telse\n+\t\t\tconn = git_proxy_connect(fd, host);\n+\t\telse {\n \t\t\tgit_tcp_connect(fd, host, flags);\n+\t\t\tconn = &no_fork;\n+\t\t}\n \t\t/*\n \t\t * Separate original protocol components prog and path\n \t\t * from extended host header with a NUL byte.\n@@ -571,7 +574,7 @@ struct child_process *git_connect(int fd[2], const char *url_orig,\n \t\tfree(url);\n \t\tif (free_path)\n \t\t\tfree(path);\n-\t\treturn &no_fork;\n+\t\treturn conn;\n \t}\n \n \tconn = xcalloc(1, sizeof(*conn));\n-- \n1.7.5.1.13.g0ca09.dirty\n"},{"id":"167968","messageId":"7vaaenm957.fsf@alter.siamese.dyndns.org","threadId":"27346","inReplyTo":"1305495440-30836-7-git-send-email-johan@herland.net","subject":"Re: [PATCHv3 6/9] receive-pack: Prepare for addition of the new 'limit-*' family of capabilities","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-16T06:50:28Z","receivedAt":"2011-05-16T06:50:28Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> +const char *server_supports(const char *feature)\n> {\n> -\treturn server_capabilities &&\n> -\t\tstrstr(server_capabilities, feature) != NULL;\n> +\tif (server_capabilities)\n> +\t\treturn strstr(server_capabilities, feature);\n> +\treturn NULL;\n> }\n\nI've been meaning to fix this part, but currently the feature set is given\nas space separated list \" featurea featureb featurec\" and we check with a\ntoken without any space around, e.g. \"if (server_supports(\"no-done\"))\",\nwhich is quite broken.\n\nWe should tighten this strstr() to make sure we are not matching in the\nmiddle of a string, and the need to do so is even greater now that you are\ngoing to introduce \"foo=\" and the value could even be strings in\nthe future.\n\nHow about implementing rules like these:\n\n - feature must appear at the beginning of server_capabilities, or the\n byte immediately before the matched location in server_capabilities\n must be a SP; and\n\n - if \"feature\" does not end with an equal sign, it does not expect a\n value. The byte after the matched location in server_capabilities must\n be either the end of string or a SP. A feature that expects a value is\n checked with 'server_supports(\"feature=\")' and the matched location in\n server_capabilities can be followed by anything (i.e. if at the end of\n string or a SP, it gets an empty string as the value, and otherwise it\n will get the stretch of bytes after the '=' up to the next SP).\n\nGiven the server_capabilities string \"foo=ab bar=froboz boz=nitfol\",\nI would like to see these happen:\n\n server_supports(\"foo=\") matches \"foo=ab\";\n\n server_supports(\"ab\") does not match anything;\n\n server_supports(\"bar\") does not match anything;\n\n server_supports(\"boz\") matches boz=nitfol, without failing at\n the end of bar=froboz that comes earlier.\n"},{"id":"167969","messageId":"20110516065211.GB19078@sigill.intra.peff.net","threadId":"27346","inReplyTo":"20110516063944.GB25731@sigill.intra.peff.net","subject":"[PATCH 2/3] connect: let callers know if connection is a socket","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-16T06:52:11Z","receivedAt":"2011-05-16T06:52:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"They might care because they want to do a half-duplex close.\nWith pipes, that means simply closing the output descriptor;\nwith a socket, you must actually call shutdown.\n\nInstead of exposing the magic no_fork child_process struct,\nlet's encapsulate the test in a function.\n\nSigned-off-by: Jeff King \n---\nAn more object-oriented refactoring would be something like:\n\n struct git_connection {\n struct child_process child;\n int in;\n int out;\n };\n\n void git_connection_read_fd(struct git_connection *c)\n {\n return c->in;\n }\n\n void git_connect_write_fd(struct git_connect *c)\n {\n return c->child.pid ? c->out : c->in;\n }\n\n void git_connection_half_duplex_close(struct git_connection *c)\n {\n if (!c->child.pid)\n shutdown(c->in, SHUT_WR);\n else\n close(c->out);\n }\n\nbut the idea that a git connection is defined by two file descriptors\nruns throughout the code (in fact, we don't even explicitly do the\nhalf-duplex close in the pipe case; we hand the descriptor off to the\npack-objects run-command, which takes ownership). So trying to be fancy\nand abstracted is not worth it in this case.\n\n cache.h | 1 +\n connect.c | 7 ++++++-\n 2 files changed, 7 insertions(+), 1 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex bf468e5..724aad4 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -865,6 +865,7 @@ extern struct ref *find_ref_by_name(const struct ref *list, const char *name);\n #define CONNECT_VERBOSE (1u << 0)\n extern struct child_process *git_connect(int fd[2], const char *url, const char *prog, int flags);\n extern int finish_connect(struct child_process *conn);\n+extern int git_connection_is_socket(struct child_process *conn);\n extern int path_match(const char *path, int nr, char **match);\n struct extra_have_objects {\n \tint nr, alloc;\ndiff --git a/connect.c b/connect.c\nindex 86ad150..20a008d 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -634,10 +634,15 @@ struct child_process *git_connect(int fd[2], const char *url_orig,\n \treturn conn;\n }\n \n+int git_connection_is_socket(struct child_process *conn)\n+{\n+\treturn conn == &no_fork;\n+}\n+\n int finish_connect(struct child_process *conn)\n {\n \tint code;\n-\tif (!conn || conn == &no_fork)\n+\tif (!conn || git_connection_is_socket(conn))\n \t\treturn 0;\n \n \tcode = finish_command(conn);\n-- \n1.7.5.1.13.g0ca09.dirty\n"},{"id":"167970","messageId":"20110516065257.GC19078@sigill.intra.peff.net","threadId":"27346","inReplyTo":"20110516063944.GB25731@sigill.intra.peff.net","subject":"[PATCH 3/3] send-pack: avoid deadlock on git:// push with failed pack-objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-16T06:52:57Z","receivedAt":"2011-05-16T06:52:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit 09c9957c fixes a deadlock in which pack-objects\nfails, the remote end is still waiting for pack data, and we\nare still waiting for the remote end to say something (see\nthat commit for a much more in-depth explanation).\n\nWe solved the problem there by making sure the output pipe\nis closed on error; thus the remote sees EOF, and proceeds\nto complain and close its end of the connection.\n\nHowever, in the special case of push over git://, we don't\nhave a pipe, but rather a full-duplex socket, with another\ndup()-ed descriptor in place of the second half of the pipe.\nIn this case, closing the second descriptor signals nothing\nto the remote end, and we still deadlock.\n\nThis patch calls shutdown() explicitly to signal EOF to the\nother side.\n\nSigned-off-by: Jeff King \n---\nAnd if you wanted to drop the first two patches, this probably works OK\nwithout the conditional, as the shutdown is just a no-op on a pipe\ndescriptor then.\n\n builtin-send-pack.c | 2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex 3e70795..682a3f9 100644\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -520,6 +520,8 @@ int send_pack(struct send_pack_args *args,\n \t\t\t\tref->status = REF_STATUS_NONE;\n \t\t\tif (args->stateless_rpc)\n \t\t\t\tclose(out);\n+\t\t\tif (git_connection_is_socket(conn))\n+\t\t\t\tshutdown(fd[0], SHUT_WR);\n \t\t\tif (use_sideband)\n \t\t\t\tfinish_async(&demux);\n \t\t\treturn -1;\n-- \n1.7.5.1.13.g0ca09.dirty\n"},{"id":"167975","messageId":"201105161127.30228.johan@herland.net","threadId":"27346","inReplyTo":"7vsjsfmawt.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv3 3/9] pack-objects: Allow --max-pack-size to be used together with --stdout","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-05-16T09:27:29Z","receivedAt":"2011-05-16T09:27:29Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Monday 16 May 2011, Junio C Hamano wrote:\n> Johan Herland writes:\n> > Currently we refuse combining --max-pack-size with --stdout since\n> > there's no way to make multiple packs when the pack is written to\n> > stdout. However, we want to be able to limit the maximum size of the\n> > pack created by --stdout (and abort pack-objects if we are unable to\n> > meet that limit).\n> > \n> > Therefore, when used together with --stdout, we reinterpret\n> > --max-pack-size to indicate the maximum pack size which - if exceeded\n> > - will cause pack-objects to abort with an error message.\n> \n> I only gave the code a cursory look, but I think your patch does more\n> than the above paragraphs say. I am not sure those extra change are\n> justified.\n> \n> For example,\n> \n> > @@ -229,7 +229,7 @@ static unsigned long write_object(struct sha1file *f,\n> > \n> > \tif (!entry->delta)\n> > \t\tusable_delta = 0;\t/* no delta */\n> > -\telse if (!pack_size_limit)\n> > +\telse if (!pack_size_limit || pack_to_stdout)\n> > \t usable_delta = 1;\t/* unlimited packfile */\n> \n> Why does this conditional have to change its behaviour when writing to\n> the standard output? I thought that the only thing you are doing\n> \"earlier we didn't allow setting size limit when writing to standard\n> output, now we do\", and I do not see the linkage between that objective\n> and this change.\n\nAFAICS, the intention of the above \"else if\" block, is to enable\nusable_delta when there is no chance of a pack split happening.\nTo establish that a pack split cannot happen, the code checks that\npack_size_limit is disabled. Previously, pack_size_limit and\npack_to_stdout was mutually exclusive (look at the last hunk in\npack-objects.c). However, with this patch, it is possible to have\nboth pack_size_limit and pack_to_stdout enabled.\n\nCrucially, though, when both are enabled, a pack split is _still_\nimpossible, since a pack split while pack_to_stdout is enabled, forces\npack-objects to abort altogether.\n\nIn other words, when pack_to_stdout is enabled, pack_size_limit is no\nlonger a good indicator of whether a pack split might happen. Instead,\npack_to_stdout being enabled is a good enough indicator in itself to\nguarantee that no pack split can happen (and hence that usable_delta\nshould be enabled).\n\n> > @@ -2315,9 +2318,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n> > \n> > \tif (!pack_to_stdout && !pack_size_limit)\n> > \t\tpack_size_limit = pack_size_limit_cfg;\n> > -\tif (pack_to_stdout && pack_size_limit)\n> > -\t\tdie(\"--max-pack-size cannot be used to build a pack for transfer.\");\n> > -\tif (pack_size_limit && pack_size_limit < 1024*1024) {\n> > +\tif (!pack_to_stdout && pack_size_limit && pack_size_limit < 1024*1024) {\n> > \t\twarning(\"minimum pack size limit is 1 MiB\");\n> > \t\tpack_size_limit = 1024*1024;\n> > \t}\n> \n> Why is the new combination \"writing to the standard output, but the\n> maximum size is limited\" does not have the same lower bound to pack size\n> limit while on-disk packs do?\n\nThe reason for this change is purely to leave the server (receive-pack)\nin control of the pack size limit. If the server for any reason does\nspecify a pack size limit less than 1 MiB, we do not want the client\nblindly ignoring that limit, and then submitting a pack that the server\nwill reject.\n\nIf we _do_ want a hard lower bound on the pack size limit, we should\napply that server-side (by setting 1 MiB as the minimum allowed value\nfor receive.packSizeLimit). However, we will now have a problem if we\nin the future decide to change this lower bound for any reason\nwhatsoever: We will need to change it in both receive-pack and\npack-objects, and for as long as there are new clients talking to old\nservers (or vice versa if we decrease the lower bound), there will be\nclients submitting packs that are then rejected by the server.\n\nI'd rather put the server in total control of the pack size limit.\n\n> If you have a reason to believe 1 MiB is too large for a pack size limit,\n> shouldn't that logic apply equally to the on-disk case? What does this\n> change have to do with the interaction with --stdout option?\n\nI have no opinion on the lower bound of the pack size limit in the\non-disk case.\n\nIn the above discussion, the usage of --stdout is synonymous with the\nsend-pack scenario (pack-objects communicating with receive-pack).\nAlthough not true in general, it is true for this patch, since up until\nnow pack-objects would abort if both --stdout and --max-pack-size were\nin use. Therefore, for the two changes discussed above:\n\n - else if (!pack_size_limit)\n + else if (!pack_size_limit || pack_to_stdout)\n\nand\n\n - if (pack_size_limit && pack_size_limit < 1024*1024) {\n + if (!pack_to_stdout && pack_size_limit && pack_size_limit < 1024*1024) {\n\nI can deduce that they only affect the current use case (the send-pack\nscenario), since these changes make no (functional) difference in any\nother use case (where --stdout and --max-pack-size are mutually\nexclusive).\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"167976","messageId":"201105161149.45512.johan@herland.net","threadId":"27346","inReplyTo":"7vmxinmabl.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv3 4/9] pack-objects: Teach new option --max-object-count, similar to --max-pack-size","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-05-16T09:49:45Z","receivedAt":"2011-05-16T09:49:45Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Monday 16 May 2011, Junio C Hamano wrote:\n> Shawn Pearce writes:\n> > On Sun, May 15, 2011 at 14:37, Johan Herland wrote:\n> >> The new --max-object-count option behaves similarly to\n> >> --max-pack-size, except that the decision to split packs is\n> >> determined by the number of objects in the pack, and not by the size\n> >> of the pack.\n> > \n> > Like my note about pack size for this case... I think doing this\n> > during writing is too late. We should be aborting the counting phase\n> > if the output pack is to stdout and we are going to exceed this limit.\n> \n> Well, even more important is if this is even useful. What is the user\n> trying to prevent from happening, and is it a useful thing?\n\nI initially met this problem at $dayjob, where a user would push a large \npack from an unrelated repo, and the push would be refused by our update \nhook, but not without storing the pack on the server first. Obviously, the \nmain objective of the patch is to prevent the pack from being stored on the \nserver in the first place. [1]\n\nBased on that, I do not really care exactly what kind of limit I have to set \nin order to prevent the push. I can easily enough find usable thresholds in \nany of #objects, #commits or pack size.\n\n> I think \"do not hog too much disk\" (i.e. size) is an understandable wish,\n> and max-pack-size imposed on --stdout would be a good approximation for\n> that.\n\nAgreed.\n\n> I would understand \"this project has only these files, and pushing a tree\n> that has 100x leaves than that may be a mistake\" (i.e. recursive sum of\n> number of entries of an individual tree). I would also sort-of understand\n> \"do not push too deep a history at once\" (i.e. we do not welcome pushing\n> a wildly diverged fork that has been allowed to grow for too long).\n> \n> But I do not think max-object-count is a good approximation for either\n> to be useful.\n\nI agree in principle, but currently, it's the only limit that we can enforce \non the server side.\n\n> Without a good answer to the above question, this looks like a \"because\n> we could\", not \"because it is useful\", feature.\n\nOk, I can drop the client side part of this patch (including the limit-\nobject-count capability), but I would like to retain the server side part \n(abort if pack header reveals too many objects), since otherwise I have to \nwait for all users to upgrade to a \"limit-*\"-aware client before I can \nconsider this problem truly solved.\n\n\n...Johan\n\n\n[1]: There is a followup to this problem, where a subsequent 'git gc' will \nfind all the objects in the pushed (but rejected by update-hook) pack to be \nunreferenced, and then explode them into loose objects. In one case, a user \npushed a 300 MB pack to the server that was then exploded into 5 GB of loose \nobjects by a subsequent 'git gc'. Needless to say, we now run 'git gc --\nprune=now' as a workaround until this can be fixed.\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"167977","messageId":"201105161153.18025.johan@herland.net","threadId":"27346","inReplyTo":"7vaaenm957.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv3 6/9] receive-pack: Prepare for addition of the new 'limit-*' family of capabilities","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-05-16T09:53:17Z","receivedAt":"2011-05-16T09:53:17Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Monday 16 May 2011, Junio C Hamano wrote:\n> Johan Herland writes:\n> > +const char *server_supports(const char *feature)\n> > \n> > {\n> > \n> > -\treturn server_capabilities &&\n> > -\t\tstrstr(server_capabilities, feature) != NULL;\n> > +\tif (server_capabilities)\n> > +\t\treturn strstr(server_capabilities, feature);\n> > +\treturn NULL;\n> > \n> > }\n> \n> I've been meaning to fix this part, but currently the feature set is\n> given as space separated list \" featurea featureb featurec\" and we check\n> with a token without any space around, e.g. \"if\n> (server_supports(\"no-done\"))\", which is quite broken.\n> \n> We should tighten this strstr() to make sure we are not matching in the\n> middle of a string, and the need to do so is even greater now that you\n> are going to introduce \"foo=\" and the value could even be strings\n> in the future.\n> \n> How about implementing rules like these:\n> \n> [...]\n\nAgreed. I'll take a stab at this in the re-roll.\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"168009","messageId":"4DD181C6.4020104@kdbg.org","threadId":"27346","inReplyTo":"20110516064607.GA19078@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] connect: treat generic proxy processes like ssh processes","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2011-05-16T19:57:58Z","receivedAt":"2011-05-16T19:57:58Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 16.05.2011 08:46, schrieb Jeff King:\n> The git_connect function returns two ends of a pipe for\n> talking with a remote, plus a struct child_process\n> representing the other end of the pipe. If we have a direct\n> socket connection, then this points to a special \"no_fork\"\n> child process.\n> \n> The code path for doing git-over-pipes or git-over-ssh sets\n> up this child process to point to the child git command or\n> the ssh process. When we call finish_connect eventually, we\n> check wait() on the command and report its return value.\n> \n> The code path for git://, on the other hand, always sets it\n> to no_fork. In the case of a direct TCP connection, this\n> makes sense; we have no child process. But in the case of a\n> proxy command (configured by core.gitproxy), we do have a\n> child process, but we throw away its pid, and therefore\n> ignore its return code.\n> \n> Instead, let's keep that information in the proxy case, and\n> respect its return code, which can help catch some errors\n\nThis patch looks strikingly familiar. I had written an almost identical\nchange more than 3 years ago and forgot about it, though the\njustification I noted in the commit was more to properly shutdown the\nproxy process rather than to abandon it and let it be collected by\ninit(8). Your justification is much better.\n\nThere's one problem with your implementation, though:\n\n> -static void git_proxy_connect(int fd[2], char *host)\n> +static struct child_process *git_proxy_connect(int fd[2], char *host)\n> {\n> \tconst char *port = STR(DEFAULT_GIT_PORT);\n> \tchar *colon, *end;\n> \tconst char *argv[4];\n> -\tstruct child_process proxy;\n> +\tstruct child_process *proxy;\n> \n> \tif (host[0] == '[') {\n> \t\tend = strchr(host + 1, ']');\n> @@ -431,14 +431,15 @@ static void git_proxy_connect(int fd[2], char *host)\n> \targv[1] = host;\n> \targv[2] = port;\n> \targv[3] = NULL;\n> -\tmemset(&proxy, 0, sizeof(proxy));\n> -\tproxy.argv = argv;\n> -\tproxy.in = -1;\n> -\tproxy.out = -1;\n> -\tif (start_command(&proxy))\n> +\tproxy = xcalloc(1, sizeof(*proxy));\n> +\tproxy->argv = argv;\n> +\tproxy->in = -1;\n> +\tproxy->out = -1;\n> +\tif (start_command(proxy))\n\nAt this point, proxy->argv would point to automatic storage; but we\nneed argv[0] in finish_command() for error reporting. In my\nimplementation, I xmalloced the pointer array and leaked it. (And\nthat's probably the reason that I never submitted the patch.) I\nwouldn't dare to make argv just static because this limits us to have\njust one open connection at a time established via git_proxy_connect().\nDunno...\n\nBelow is the interdiff that turns your patch into mine, mostly for\nexposition: The two hunks in git_connect() are just cosmetic\ndifferences. But the first hunk should be squashed into your patch to\nfix a potential crash in an error situation (e.g., when the proxy\ndies from a signal) at the cost of a small memory leak.\n\ndiff --git a/connect.c b/connect.c\nindex a28e084..8668913 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -399,9 +399,10 @@ static struct child_process *git_proxy_connect(int fd[2], char *host)\n {\n \tconst char *port = STR(DEFAULT_GIT_PORT);\n-\tconst char *argv[4];\n+\tconst char **argv;\n \tstruct child_process *proxy;\n \n \tget_host_and_port(&host, &port);\n \n+\targv = xmalloc(4*sizeof(*argv));\n \targv[0] = git_proxy_command;\n \targv[1] = host;\n@@ -457,5 +458,5 @@ struct child_process *git_connect(int fd[2], const char *url_orig,\n \tchar *end;\n \tint c;\n-\tstruct child_process *conn;\n+\tstruct child_process *conn = &no_fork;\n \tenum protocol protocol = PROTO_LOCAL;\n \tint free_path = 0;\n@@ -543,8 +544,6 @@ struct child_process *git_connect(int fd[2], const char *url_orig,\n \t\tif (git_use_proxy(host))\n \t\t\tconn = git_proxy_connect(fd, host);\n-\t\telse {\n+\t\telse\n \t\t\tgit_tcp_connect(fd, host, flags);\n-\t\t\tconn = &no_fork;\n-\t\t}\n \t\t/*\n \t\t * Separate original protocol components prog and path\n"},{"id":"168010","messageId":"4DD182C8.50308@kdbg.org","threadId":"27346","inReplyTo":"20110516065257.GC19078@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] send-pack: avoid deadlock on git:// push with failed pack-objects","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2011-05-16T20:02:16Z","receivedAt":"2011-05-16T20:02:16Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 16.05.2011 08:52, schrieb Jeff King:\n> And if you wanted to drop the first two patches, this probably works OK\n> without the conditional, as the shutdown is just a no-op on a pipe\n> descriptor then.\n> \n> builtin-send-pack.c | 2 ++\n> 1 files changed, 2 insertions(+), 0 deletions(-)\n> \n> diff --git a/builtin-send-pack.c b/builtin-send-pack.c\n> index 3e70795..682a3f9 100644\n> --- a/builtin-send-pack.c\n> +++ b/builtin-send-pack.c\n> @@ -520,6 +520,8 @@ int send_pack(struct send_pack_args *args,\n> \t\t\t\tref->status = REF_STATUS_NONE;\n> \t\t\tif (args->stateless_rpc)\n> \t\t\t\tclose(out);\n> +\t\t\tif (git_connection_is_socket(conn))\n> +\t\t\t\tshutdown(fd[0], SHUT_WR);\n> \t\t\tif (use_sideband)\n> \t\t\t\tfinish_async(&demux);\n> \t\t\treturn -1;\n\nWe probably need a wrapper for shutdown() on Windows. I'll look into\nthis tomorrow.\n\n-- Hannes\n"},{"id":"168024","messageId":"BANLkTimUWHiUdxbBfvvcomqmzQxLPhUsZA@mail.gmail.com","threadId":"27346","inReplyTo":"7vaaenm957.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv3 6/9] receive-pack: Prepare for addition of the new 'limit-*' family of capabilities","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-05-16T22:02:59Z","receivedAt":"2011-05-16T22:02:59Z","isPatch":false,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Sun, May 15, 2011 at 23:50, Junio C Hamano wrote:\n> We should tighten this strstr() to make sure we are not matching in the\n> middle of a string, and the need to do so is even greater now that you are\n> going to introduce \"foo=\" and the value could even be strings in\n> the future.\n\nIf we are writing this down somewhere, should we also dictate how\nspaces should be escaped to be \"forward-compatible\"?\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"168026","messageId":"7vhb8ujo4e.fsf@alter.siamese.dyndns.org","threadId":"27346","inReplyTo":"BANLkTimUWHiUdxbBfvvcomqmzQxLPhUsZA@mail.gmail.com","subject":"Re: [PATCHv3 6/9] receive-pack: Prepare for addition of the new 'limit-*' family of capabilities","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-16T22:07:29Z","receivedAt":"2011-05-16T22:07:29Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sverre Rabbelier writes:\n\n> On Sun, May 15, 2011 at 23:50, Junio C Hamano wrote:\n>> We should tighten this strstr() to make sure we are not matching in the\n>> middle of a string, and the need to do so is even greater now that you are\n>> going to introduce \"foo=\" and the value could even be strings in\n>> the future.\n>\n> If we are writing this down somewhere, should we also dictate how\n> spaces should be escaped to be \"forward-compatible\"?\n\nThe old clients treat it as SP separted list, e.g. \"featurea featureb featureb\", \nfrom the beginning of how capabilities[] code was writte, so I do not see\na point. I would expect an arbitrary string values would be encased in\nsomething like base64, base85 or hex.\n"},{"id":"168027","messageId":"BANLkTimge=rPe6K3rYRqYDGQTN_wbi3a2A@mail.gmail.com","threadId":"27346","inReplyTo":"7vhb8ujo4e.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv3 6/9] receive-pack: Prepare for addition of the new 'limit-*' family of capabilities","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-05-16T22:09:34Z","receivedAt":"2011-05-16T22:09:34Z","isPatch":false,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Mon, May 16, 2011 at 15:07, Junio C Hamano wrote:\n> The old clients treat it as SP separted list, e.g. \"featurea featureb featureb\",\n> from the beginning of how capabilities[] code was writte, so I do not see\n> a point. I would expect an arbitrary string values would be encased in\n> something like base64, base85 or hex.\n\nRight, that's my point, do we want to leave it up to each individual\noption to decide what encoding to use?\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"168028","messageId":"7vd3jijnwe.fsf@alter.siamese.dyndns.org","threadId":"27346","inReplyTo":"BANLkTimge=rPe6K3rYRqYDGQTN_wbi3a2A@mail.gmail.com","subject":"Re: [PATCHv3 6/9] receive-pack: Prepare for addition of the new 'limit-*' family of capabilities","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-16T22:12:17Z","receivedAt":"2011-05-16T22:12:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sverre Rabbelier writes:\n\n> On Mon, May 16, 2011 at 15:07, Junio C Hamano wrote:\n>> The old clients treat it as SP separted list, e.g. \"featurea featureb featureb\",\n>> from the beginning of how capabilities[] code was writte, so I do not see\n>> a point. I would expect an arbitrary string values would be encased in\n>> something like base64, base85 or hex.\n>\n> Right, that's my point, do we want to leave it up to each individual\n> option to decide what encoding to use?\n\nI do not see any point in deciding that right now without even knowing\nwhat kind of strings (short? mostly ascii? etc.) we would typically want\nas payload and tie our hands with a poor decision we would end up making\nout of thin air.\n\nThat is what I meant by \"I do not see a point\".\n"},{"id":"168029","messageId":"BANLkTinxTWK3wgp-WcC9rBpCe2-uXeSE=w@mail.gmail.com","threadId":"27346","inReplyTo":"7vd3jijnwe.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv3 6/9] receive-pack: Prepare for addition of the new 'limit-*' family of capabilities","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-05-16T22:16:37Z","receivedAt":"2011-05-16T22:16:37Z","isPatch":false,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Mon, May 16, 2011 at 15:12, Junio C Hamano wrote:\n> I do not see any point in deciding that right now without even knowing\n> what kind of strings (short? mostly ascii? etc.) we would typically want\n> as payload and tie our hands with a poor decision we would end up making\n> out of thin air.\n\nOk, fair enough. :)\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"168030","messageId":"7vy626i6j1.fsf@alter.siamese.dyndns.org","threadId":"27346","inReplyTo":"4DD181C6.4020104@kdbg.org","subject":"Re: [PATCH 1/3] connect: treat generic proxy processes like ssh processes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-16T23:12:50Z","receivedAt":"2011-05-16T23:12:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> At this point, proxy->argv would point to automatic storage; but we\n> need argv[0] in finish_command() for error reporting. In my\n> implementation, I xmalloced the pointer array and leaked it. (And\n> that's probably the reason that I never submitted the patch.) I\n> wouldn't dare to make argv just static because this limits us to have\n> just one open connection at a time established via git_proxy_connect().\n\nGood point. If we really care, I would imagine that struct child_process\ncan gain a boolean flag to tell finish_command() that the argv[] needs, so\nI do not think leakage here is such a big deal.\n\nWill squash all three hunks in. Assuming no_fork first and updating conn\nas we discover what kind of URL we have feels natural.\n\nThanks, both.\n"},{"id":"168046","messageId":"20110517055422.GD10048@sigill.intra.peff.net","threadId":"27346","inReplyTo":"4DD181C6.4020104@kdbg.org","subject":"Re: [PATCH 1/3] connect: treat generic proxy processes like ssh processes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-17T05:54:22Z","receivedAt":"2011-05-17T05:54:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 16, 2011 at 09:57:58PM +0200, Johannes Sixt wrote:\n\n> > The code path for git://, on the other hand, always sets it\n> > to no_fork. In the case of a direct TCP connection, this\n> > makes sense; we have no child process. But in the case of a\n> > proxy command (configured by core.gitproxy), we do have a\n> > child process, but we throw away its pid, and therefore\n> > ignore its return code.\n> > \n> > Instead, let's keep that information in the proxy case, and\n> > respect its return code, which can help catch some errors\n> \n> This patch looks strikingly familiar. I had written an almost identical\n> change more than 3 years ago and forgot about it, though the\n> justification I noted in the commit was more to properly shutdown the\n> proxy process rather than to abandon it and let it be collected by\n> init(8). Your justification is much better.\n\nThanks, I had no idea your patch existed. I hate to duplicate work, but\nat least it's a sanity check that it's not a totally stupid idea. ;)\n\n> > \tconst char *argv[4];\n> > -\tstruct child_process proxy;\n> > +\tstruct child_process *proxy;\n> [...]\n> At this point, proxy->argv would point to automatic storage; but we\n> need argv[0] in finish_command() for error reporting.\n\nIck. Good catch.\n\n> In my implementation, I xmalloced the pointer array and leaked it.\n> (And that's probably the reason that I never submitted the patch.) I\n> wouldn't dare to make argv just static because this limits us to have\n> just one open connection at a time established via\n> git_proxy_connect(). Dunno...\n\nWe also need to worry about the contents of each argv[] element, no? So\nwe should be xstrdup()ing the host and port, which point into some\nstring which gets passed to us. I didn't trace its provenance but I\nthink it is better to be defensive.\n\nThe leak is probably OK in a practical sense (you generally make no more\nthan one such connection per command), but it does seem ugly. I would\nnot be surprised if many other run-command invocations leak similarly.\n\nThe interdiff with the strdups is:\n\ndiff --git a/connect.c b/connect.c\nindex c678ceb..c24866e 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -398,14 +398,15 @@ static int git_use_proxy(const char *host)\n static struct child_process *git_proxy_connect(int fd[2], char *host)\n {\n \tconst char *port = STR(DEFAULT_GIT_PORT);\n-\tconst char *argv[4];\n+\tconst char **argv;\n \tstruct child_process *proxy;\n \n \tget_host_and_port(&host, &port);\n \n+\targv = xmalloc(4 * sizeof(*argv));\n \targv[0] = git_proxy_command;\n-\targv[1] = host;\n-\targv[2] = port;\n+\targv[1] = xstrdup(host);\n+\targv[2] = xstrdup(port);\n \targv[3] = NULL;\n \tproxy = xcalloc(1, sizeof(*proxy));\n \tproxy->argv = argv;\n"},{"id":"168047","messageId":"20110517055605.GE10048@sigill.intra.peff.net","threadId":"27346","inReplyTo":"4DD182C8.50308@kdbg.org","subject":"Re: [PATCH 3/3] send-pack: avoid deadlock on git:// push with failed pack-objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-17T05:56:05Z","receivedAt":"2011-05-17T05:56:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 16, 2011 at 10:02:16PM +0200, Johannes Sixt wrote:\n\n> > +\t\t\tif (git_connection_is_socket(conn))\n> > +\t\t\t\tshutdown(fd[0], SHUT_WR);\n> \n> We probably need a wrapper for shutdown() on Windows. I'll look into\n> this tomorrow.\n\nFWIW, we already make an identical call in transport-helper.c (I was\ntempted even to use \"1\" instead of SHUT_WR for portability, but it seems\nnobody has complained so far about the use in transport-helper).\n\n-Peff\n"},{"id":"168080","messageId":"4DD2D733.3070004@kdbg.org","threadId":"27346","inReplyTo":"20110517055422.GD10048@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] connect: treat generic proxy processes like ssh processes","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2011-05-17T20:14:43Z","receivedAt":"2011-05-17T20:14:43Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 17.05.2011 07:54, schrieb Jeff King:\n> On Mon, May 16, 2011 at 09:57:58PM +0200, Johannes Sixt wrote:\n>> In my implementation, I xmalloced the pointer array and leaked it.\n\nI noticed that it actually isn't leaked because finish_connect() frees\nit. For this reason, I actually have to wonder why your version that\nstored a pointer to automatic storage in ->argv worked.\n\n> We also need to worry about the contents of each argv[] element, no? So\n> we should be xstrdup()ing the host and port, which point into some\n> string which gets passed to us. I didn't trace its provenance but I\n> think it is better to be defensive.\n\nI would not worry too much today. Of course, functions other than\nstart_command() might begin to access ->argv[i] with i > 0 later, but\nthen we have to audit all users of struct child_process anyway.\nCurrently, only start_command() uses these values, which is always\ncalled at a time when they are still valid.\n\n-- Hannes\n"},{"id":"168121","messageId":"20110518085743.GH27482@sigill.intra.peff.net","threadId":"27346","inReplyTo":"4DD2D733.3070004@kdbg.org","subject":"Re: [PATCH 1/3] connect: treat generic proxy processes like ssh processes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-18T08:57:44Z","receivedAt":"2011-05-18T08:57:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 17, 2011 at 10:14:43PM +0200, Johannes Sixt wrote:\n\n> Am 17.05.2011 07:54, schrieb Jeff King:\n> > On Mon, May 16, 2011 at 09:57:58PM +0200, Johannes Sixt wrote:\n> >> In my implementation, I xmalloced the pointer array and leaked it.\n> \n> I noticed that it actually isn't leaked because finish_connect() frees\n> it. For this reason, I actually have to wonder why your version that\n> stored a pointer to automatic storage in ->argv worked.\n\nIt probably didn't. As a simple refactoring, I didn't test the proxy\ncodepath, and apparently nothing in our test suite does, either. :(\n\nWe can put the patch below on top (it fails with my original series, but\npasses with the bugfix you noticed).\n\n> I would not worry too much today. Of course, functions other than\n> start_command() might begin to access ->argv[i] with i > 0 later, but\n> then we have to audit all users of struct child_process anyway.\n> Currently, only start_command() uses these values, which is always\n> called at a time when they are still valid.\n\nThat makes sense.\n\n-- >8 --\nSubject: [PATCH] test core.gitproxy configuration\n\nThis is just a basic sanity test to see whether\ncore.gitproxy works at all. Until now, we were not testing\nanywhere.\n\nSigned-off-by: Jeff King \n---\nThis is really basic. Apparently you can do horrible things like\n\n git config core.gitproxy \"./proxy for kernel.org\"\n git config core.gitproxy \"./other-proxy for example.com\"\n\nI can make it more elaborate if we really want to care.\n\n t/t5532-fetch-proxy.sh | 42 ++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 42 insertions(+), 0 deletions(-)\n create mode 100755 t/t5532-fetch-proxy.sh\n\ndiff --git a/t/t5532-fetch-proxy.sh b/t/t5532-fetch-proxy.sh\nnew file mode 100755\nindex 0000000..07119d9\n--- /dev/null\n+++ b/t/t5532-fetch-proxy.sh\n@@ -0,0 +1,42 @@\n+#!/bin/sh\n+\n+test_description='fetching via git:// using core.gitproxy'\n+. ./test-lib.sh\n+\n+test_expect_success 'setup remote repo' '\n+\tgit init remote &&\n+\t(cd remote &&\n+\t echo content >file &&\n+\t git add file &&\n+\t git commit -m one\n+\t)\n+'\n+\n+cat >proxy <<'EOF'\n+#!/bin/sh\n+echo >&2 \"proxying for $*\"\n+cmd=`perl -e '\n+\tread(STDIN, $buf, 4);\n+\tmy $n = hex($buf) - 4;\n+\tread(STDIN, $buf, $n);\n+\tmy ($cmd, $other) = split /\\0/, $buf;\n+\t# drop absolute-path on repo name\n+\t$cmd =~ s{ /}{ };\n+\tprint $cmd;\n+'`\n+exec $cmd\n+EOF\n+chmod +x proxy\n+test_expect_success 'setup local repo' '\n+\tgit remote add fake git://example.com/remote &&\n+\tgit config core.gitproxy ./proxy\n+'\n+\n+test_expect_success 'fetch through proxy works' '\n+\tgit fetch fake &&\n+\techo one >expect &&\n+\tgit log -1 --format=%s FETCH_HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n1.7.5.8.ga95ab\n"},{"id":"168159","messageId":"4DD42B05.9050000@kdbg.org","threadId":"27346","inReplyTo":"20110517055605.GE10048@sigill.intra.peff.net","subject":"[PATCH] Windows: add a wrapper for the shutdown() system call","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2011-05-18T20:24:37Z","receivedAt":"2011-05-18T20:24:37Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Even though Windows's socket functions look like their POSIX counter parts,\nthey do not operate on file descriptors, but on \"socket objects\". To bring\nthe functions in line with POSIX, we have proxy functions that wrap and\nunwrap the socket objects in file descriptors using open_osfhandle and\nget_osfhandle. But shutdown() was not proxied, yet. Fix this.\n\nSigned-off-by: Johannes Sixt \n---\nAm 17.05.2011 07:56, schrieb Jeff King:\n> On Mon, May 16, 2011 at 10:02:16PM +0200, Johannes Sixt wrote:\n> \n>>> +\t\t\tif (git_connection_is_socket(conn))\n>>> +\t\t\t\tshutdown(fd[0], SHUT_WR);\n>>\n>> We probably need a wrapper for shutdown() on Windows. I'll look into\n>> this tomorrow.\n\nWe have a shutdown() on Windows, but it does not work out of the box.\nMaking it work is not a big deal, but it is an academic excercise if it\nwere only for this topic: On Windows, send-pack hangs when network\nconnections are involved for unknown reasons as long as side-band-64k\nis enabled. If it is not enabled, the deadlock that you fixed does not\nhappen in the first place and it is irrelevant whether shutdown works.\n\n> FWIW, we already make an identical call in transport-helper.c (I was\n> tempted even to use \"1\" instead of SHUT_WR for portability, but it seems\n> nobody has complained so far about the use in transport-helper).\n\nYes, and for the reasons mentioned above, this patch is intended for\nthe 1.7.4 series, where transport-helper.c went public. (But it should\napply cleanly to current master as well.) We do not have tests that\nexercise the code in transport-helper.c, but I did test that this\nshutdown implementation does something reasonable when it is merged\nwith your send-pack deadlock topic branch.\n\n compat/mingw.c | 7 +++++++\n compat/mingw.h | 3 +++\n 2 files changed, 10 insertions(+), 0 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex bee6054..1cbc9e8 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -1219,6 +1219,13 @@ int mingw_setsockopt(int sockfd, int lvl, int optname, void *optval, int optlen)\n \treturn setsockopt(s, lvl, optname, (const char*)optval, optlen);\n }\n \n+#undef shutdown\n+int mingw_shutdown(int sockfd, int how)\n+{\n+\tSOCKET s = (SOCKET)_get_osfhandle(sockfd);\n+\treturn shutdown(s, how);\n+}\n+\n #undef listen\n int mingw_listen(int sockfd, int backlog)\n {\ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex 14211c6..3b20fa9 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -219,6 +219,9 @@ int mingw_bind(int sockfd, struct sockaddr *sa, size_t sz);\n int mingw_setsockopt(int sockfd, int lvl, int optname, void *optval, int optlen);\n #define setsockopt mingw_setsockopt\n \n+int mingw_shutdown(int sockfd, int how);\n+#define shutdown mingw_shutdown\n+\n int mingw_listen(int sockfd, int backlog);\n #define listen mingw_listen\n \n-- \n1.7.5.rc1.97.ge0653\n"}]}