{"thread":{"id":"27432","subject":"[PATCHv4 00/10] Push limits","startedAt":"2011-05-23T00:51:53Z","lastAt":"2011-05-24T01:17:56Z","messageCount":26,"participants":["Johan Herland","Shawn Pearce","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"168463","messageId":"1306111923-16859-1-git-send-email-johan@herland.net","threadId":"27432","inReplyTo":null,"subject":"[PATCHv4 00/10] Push limits","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-05-23T00:51:53Z","receivedAt":"2011-05-23T00:51:53Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"Finally, I found some time to re-roll this series. Here's a quick\noverview of the changes since the previous iteration:\n\n - Rebased onto 'next' to include the deadlock fix from Peff and J6t.\n\n - Reshuffle the patch series to leave the more contentious patches\n towards the end of the series.\n\n - (patch #3) Implement tighter matching rules in server_supports(),\n as suggested by Junio in the previous thread.\n\n - Remove --max-object-count from pack-objects and limit-object-count\n capability, since object count is not considered a useful metric\n for limiting pushes. However, keep the server-side object count\n limit, since it is the only metric we can cheaply check on the\n server-side (unless we change the pack format to include pack size\n and/or commit count). This is now found in patch #10.\n\n - (patch #9) In pack-objects, attempt to estimate the pack size before\n we start writing out pack data. Abort as early as possible if the\n estimated pack size exceeds the pack size limit. The estimate is\n based on the in-pack size of already packed objects (assumed to be\n reused as-is). The patch does not attempt to estimate the packed\n size of currently loose objects. Therefore, whenever we're pushing\n unpacked objects, we end up underestimating the pack size. This is\n suboptimal, but ok, since we will still abort the transfer if we\n exceed the pack size limit while writing out the pack data.\n\n\nHave fun! :)\n\n...Johan\n\n\nJohan Herland (10):\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 Tighten rules for matching server capabilities in server_supports()\n receive-pack: Prepare for addition of the new 'limit-*' family of capabilities\n pack-objects: Teach new option --max-commit-count, limiting #commits in pack\n send-pack/receive-pack: Allow server to refuse pushes with too many commits\n pack-objects: Allow --max-pack-size to be used together with --stdout\n send-pack/receive-pack: Allow server to refuse pushing too large packs\n pack-objects: Estimate pack size; abort early if pack size limit is exceeded\n receive-pack: Allow server to refuse pushes with too many objects\n\n Documentation/config.txt | 27 ++++\n Documentation/git-pack-objects.txt | 11 ++\n Documentation/technical/pack-protocol.txt | 5 +-\n Documentation/technical/protocol-capabilities.txt | 29 ++++-\n builtin/pack-objects.c | 56 ++++++-\n builtin/receive-pack.c | 42 +++++-\n builtin/send-pack.c | 31 +++--\n cache.h | 2 +-\n connect.c | 30 ++++-\n send-pack.h | 2 +\n t/t5300-pack-object.sh | 77 +++++++++\n t/t5400-send-pack.sh | 171 +++++++++++++++++++++\n 12 files changed, 453 insertions(+), 30 deletions(-)\n\n-- \n1.7.5.rc1.3.g4d7b\n"},{"id":"168472","messageId":"1306111923-16859-2-git-send-email-johan@herland.net","threadId":"27432","inReplyTo":"1306111923-16859-1-git-send-email-johan@herland.net","subject":"[PATCHv4 01/10] Update technical docs to reflect side-band-64k capability in receive-pack","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-05-23T00:51:54Z","receivedAt":"2011-05-23T00:51:54Z","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":"168464","messageId":"1306111923-16859-3-git-send-email-johan@herland.net","threadId":"27432","inReplyTo":"1306111923-16859-1-git-send-email-johan@herland.net","subject":"[PATCHv4 02/10] send-pack: Attempt to retrieve remote status even if pack-objects fails","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-05-23T00:51:55Z","receivedAt":"2011-05-23T00:51:55Z","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 on whether\nthe reason for the failure is _local_ (i.e. something in pack-objects\ncaused it to fail of its own accord), or _remote_ (i.e. something in\nthe remote receive-pack process caused it to fail, leaving the local\npack-objects process 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 builtin/send-pack.c | 13 +++----------\n 1 files changed, 3 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex c1f6ddd..5ba5262 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,25 +339,18 @@ 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\tif ((ret = pack_objects(out, remote_refs, extra_have, args))) {\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 \t\t}\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":"168465","messageId":"1306111923-16859-4-git-send-email-johan@herland.net","threadId":"27432","inReplyTo":"1306111923-16859-1-git-send-email-johan@herland.net","subject":"[PATCHv4 03/10] Tighten rules for matching server capabilities in server_supports()","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-05-23T00:51:56Z","receivedAt":"2011-05-23T00:51:56Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"When using server_supports() to match a given \"feature\" against the server\ncapabilities, follow these rules:\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\",\nthis patch should make it behave as follows:\n\n server_supports(\"foo=\") matches \"foo=ab\", returns \"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 (and returns \"boz\"), without failing\n at the end of bar=froboz that comes earlier.\n\nSuggested-by: Junio C Hamano \nSigned-off-by: Johan Herland \n---\n cache.h | 2 +-\n connect.c | 30 +++++++++++++++++++++++++++---\n 2 files changed, 28 insertions(+), 4 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 009b365..1a9338f 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -995,7 +995,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 2119c3f..3c0a706 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -8,6 +8,7 @@\n #include \"url.h\"\n \n static char *server_capabilities;\n+static size_t server_capabilities_len;\n \n static int check_ref(const char *name, int len, unsigned int flags)\n {\n@@ -80,8 +81,16 @@ struct ref **get_remote_heads(int in, struct ref **list,\n \n \t\tname_len = strlen(name);\n \t\tif (len != name_len + 41) {\n+\t\t\tchar *p;\n \t\t\tfree(server_capabilities);\n \t\t\tserver_capabilities = xstrdup(name + name_len + 1);\n+\t\t\tserver_capabilities_len = strlen(server_capabilities);\n+\t\t\t/* split capabilities on SP */\n+\t\t\tfor (p = server_capabilities;\n+\t\t\t p < server_capabilities + server_capabilities_len;\n+\t\t\t p++)\n+\t\t\t\tif (*p == ' ')\n+\t\t\t\t\t*p = '\\0';\n \t\t}\n \n \t\tif (extra_have &&\n@@ -102,10 +111,25 @@ 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+\tconst char *p = server_capabilities;\n+\tsize_t feature_len = strlen(feature);\n+\tint need_value = feature[feature_len - 1] == '=';\n+\n+\tif (!server_capabilities)\n+\t\treturn NULL;\n+\n+\tfor (p = server_capabilities;\n+\t p < server_capabilities + server_capabilities_len;\n+\t p += strlen(p) + 1) {\n+\t\tif (need_value) {\n+\t\t\tif (!strncmp(p, feature, feature_len))\n+\t\t\t\treturn p + feature_len;\n+\t\t} else if (!strcmp(p, feature))\n+\t\t\treturn p;\n+\t}\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":"168467","messageId":"1306111923-16859-5-git-send-email-johan@herland.net","threadId":"27432","inReplyTo":"1306111923-16859-1-git-send-email-johan@herland.net","subject":"[PATCHv4 04/10] receive-pack: Prepare for addition of the new 'limit-*' family of capabilities","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-05-23T00:51:57Z","receivedAt":"2011-05-23T00:51:57Z","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\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 3 files changed, 37 insertions(+), 7 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 }\n-- \n1.7.5.rc1.3.g4d7b\n"},{"id":"168470","messageId":"1306111923-16859-6-git-send-email-johan@herland.net","threadId":"27432","inReplyTo":"1306111923-16859-1-git-send-email-johan@herland.net","subject":"[PATCHv4 05/10] pack-objects: Teach new option --max-commit-count, limiting #commits in pack","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-05-23T00:51:58Z","receivedAt":"2011-05-23T00:51:58Z","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 20c8551..e43904e 100644\n--- a/Documentation/git-pack-objects.txt\n+++ b/Documentation/git-pack-objects.txt\n@@ -113,6 +113,14 @@ base-name::\n \tThe default is unlimited, unless the config variable\n \t`pack.packSizeLimit` is set.\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 f402a84..f0fc187 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@@ -653,8 +653,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@@ -694,8 +697,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@@ -2125,6 +2131,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@@ -2179,6 +2186,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@@ -2322,6 +2334,9 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\tpack_size_limit = 1024*1024;\n \t}\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@@ -2348,6 +2363,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 && 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 602806d..80df631 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -396,6 +396,40 @@ test_expect_success 'verify resulting packs' '\n \tgit verify-pack test-11-*.pack\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-17.pack &&\n+\tgit index-pack --strict test-17.pack\n+'\n+\n+test_expect_success 'verify resulting pack' '\n+\tgit verify-pack test-17.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-18.pack 2>errs &&\n+\ttest_must_fail git index-pack --strict test-18.pack &&\n+\tgrep -q \"commit count limit\" errs\n+'\n+\n #\n # WARNING!\n #\n-- \n1.7.5.rc1.3.g4d7b\n"},{"id":"168471","messageId":"1306111923-16859-7-git-send-email-johan@herland.net","threadId":"27432","inReplyTo":"1306111923-16859-1-git-send-email-johan@herland.net","subject":"[PATCHv4 06/10] send-pack/receive-pack: Allow server to refuse pushes with too many commits","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-05-23T00:51:59Z","receivedAt":"2011-05-23T00:51:59Z","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 commits.\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 | 2 +\n builtin/receive-pack.c | 9 +++\n builtin/send-pack.c | 10 +++\n send-pack.h | 1 +\n t/t5400-send-pack.sh | 65 +++++++++++++++++++++\n 6 files changed, 96 insertions(+), 0 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 1a060ec..c18faac 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1592,6 +1592,15 @@ receive.unpackLimit::\n \tespecially on slow filesystems. If not set, the value of\n \t`transfer.unpackLimit` is used instead.\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 11849a3..0240967 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-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 c55989d..32d95fb 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_commit_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.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@@ -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_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 5ba5262..f91924f 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_commit_count > 0) {\n+\t\tsnprintf(buf, sizeof(buf), \"--max-commit-count=%lu\",\n+\t\t\t args->max_commit_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-commit-count=\")))\n+\t\targs->max_commit_count = strtoul(p, 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..489aabd 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_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 0eace37..285601d 100755\n--- a/t/t5400-send-pack.sh\n+++ b/t/t5400-send-pack.sh\n@@ -222,4 +222,69 @@ 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-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":"168466","messageId":"1306111923-16859-8-git-send-email-johan@herland.net","threadId":"27432","inReplyTo":"1306111923-16859-1-git-send-email-johan@herland.net","subject":"[PATCHv4 07/10] pack-objects: Allow --max-pack-size to be used together with --stdout","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-05-23T00:52:00Z","receivedAt":"2011-05-23T00:52:00Z","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 e43904e..24cf975 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 --max-commit-count=::\n \tThis option is only useful together with --stdout.\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex f0fc187..e226053 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@@ -2327,9 +2330,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 80df631..46c1214 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 test_expect_success 'make a few more commits' '\n \tgit reset --hard $commit &&\n \techo \"change\" > file &&\n-- \n1.7.5.rc1.3.g4d7b\n"},{"id":"168469","messageId":"1306111923-16859-9-git-send-email-johan@herland.net","threadId":"27432","inReplyTo":"1306111923-16859-1-git-send-email-johan@herland.net","subject":"[PATCHv4 08/10] send-pack/receive-pack: Allow server to refuse pushing too large packs","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-05-23T00:52:01Z","receivedAt":"2011-05-23T00:52:01Z","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 c18faac..79d553a 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1592,6 +1592,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.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\ndiff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt\nindex 0240967..71566cf 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-commit-count= (Maximum number of commits 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 32d95fb..49d29ad 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_commit_count;\n+static unsigned long limit_pack_size, limit_commit_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.commitcountlimit\") == 0) {\n \t\tlimit_commit_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_commit_count > 0)\n \t\tret += snprintf(buf + ret, sizeof(buf) - ret,\n \t\t\t\t\" limit-commit-count=%lu\", limit_commit_count);\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex f91924f..bd7b3c0 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_commit_count > 0) {\n-\t\tsnprintf(buf, sizeof(buf), \"--max-commit-count=%lu\",\n+\t\tsnprintf(buf2, sizeof(buf2), \"--max-commit-count=%lu\",\n \t\t\t args->max_commit_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, NULL, 10);\n \tif ((p = server_supports(\"limit-commit-count=\")))\n \t\targs->max_commit_count = strtoul(p, NULL, 10);\n \ndiff --git a/send-pack.h b/send-pack.h\nindex 489aabd..4f9b2f1 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_commit_count;\n };\n \ndiff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh\nindex 285601d..4ed5ba1 100755\n--- a/t/t5400-send-pack.sh\n+++ b/t/t5400-send-pack.sh\n@@ -287,4 +287,66 @@ test_expect_success 'push is allowed when commit 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":"168468","messageId":"1306111923-16859-10-git-send-email-johan@herland.net","threadId":"27432","inReplyTo":"1306111923-16859-1-git-send-email-johan@herland.net","subject":"[PATCHv4 09/10] pack-objects: Estimate pack size; abort early if pack size limit is exceeded","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-05-23T00:52:02Z","receivedAt":"2011-05-23T00:52:02Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"Currently, when pushing a pack to the server that has specified a pack size\nlimit, we don't detect that we exceed that limit until we have already\ngenerated (and started transmitting) that much pack data.\n\nIdeally, we should be able to predict the approximate pack size _before_ we\nstart generating and transmitting the pack data, and abort early if the\nestimated pack size exceeds the pack size limit.\n\nThis patch tries to provide such an estimate: It looks at the objects that\nare to be included in the pack, and for already-packed objects, it assumes\nthat their compressed in-pack size is a good estimate of how much they will\ncontribute to the pack currently being generated. This assumption should be\nvalid as long as the objects are reused as-is.\n\nFor loose objects that are to be included in the pack, we currently have no\ngood estimate as to how much they will contribute to the pack size. Since\nit's better to underestimate (because an overestimation will prevent us\nfrom sending a pack that might actually be within the pack size limit),\nwe don't include loose objects at all in the pack size estimate. This makes\nthe estimate somewhat useless in common workflows (where the push happens\nbefore (most of) the pushed objects are packed).\n\nThe estimate is generated before the \"Compressing\" and \"Writing\" phases of\nthe push, so if the estimate exceeds the pack size limit, we abort before\nsending any pack data to the server.\n\nIf the estimate turns out to be too low (e.g. because we're pushing many\nloose objects), there is still code in place to abort the push when we\nreach the pack size limit during transmission.\n\nSigned-off-by: Johan Herland \n---\n\nI'm not really happy with excluding loose objects in the pack size\nestimate. However, the size contributed by loose objects varies wildly\ndepending on whether a (good) delta is found. Therefore, any estimate\ndone at an early stage is bound to be wildly inaccurate. We could maybe\nuse some sort of absolute minimum size per object instead, but I\nthought I should publish this version before spending more time futzing\nwith it...\n\nA drawback of not including loose objects in the pack size estimate,\nis that pushing loose objects is a very common use case (most people\npush more often than they 'git gc'). However, for the pack sizes that\nservers are most likely to refuse (hundreds of megabytes), most of\nthose objects will probably already be packed anyway (e.g. by\n'git gc --auto'), so I still hope the pack size estimate will be useful\nwhen it really matters.\n\n\n...Johan\n\n\n builtin/pack-objects.c | 23 +++++++++++++++++++++++\n 1 files changed, 23 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex e226053..c0c6a0a 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1141,23 +1141,46 @@ static int pack_offset_sort(const void *_a, const void *_b)\n \t\t\t(a->in_pack_offset > b->in_pack_offset);\n }\n \n+static unsigned long estimate_packed_size(const struct object_entry *entry)\n+{\n+\tunsigned long ret;\n+\tif (entry->in_pack) {\n+\t\t/* Assume that all packed objects are reused as-is */\n+\t\tstruct revindex_entry *revidx = find_pack_revindex(\n+\t\t\tentry->in_pack,\n+\t\t\tentry->in_pack_offset);\n+\t\treturn revidx[1].offset - entry->in_pack_offset;\n+\t}\n+\treturn 0;\n+}\n+\n static void get_object_details(void)\n {\n \tuint32_t i;\n \tstruct object_entry **sorted_by_offset;\n+\tunsigned long sum_size;\n \n \tsorted_by_offset = xcalloc(nr_objects, sizeof(struct object_entry *));\n \tfor (i = 0; i < nr_objects; i++)\n \t\tsorted_by_offset[i] = objects + i;\n \tqsort(sorted_by_offset, nr_objects, sizeof(*sorted_by_offset), pack_offset_sort);\n \n+\tif (pack_to_stdout && pack_size_limit)\n+\t\tsum_size = sizeof(struct pack_header) + 20; /* pack overhead */\n+\n \tfor (i = 0; i < nr_objects; i++) {\n \t\tstruct object_entry *entry = sorted_by_offset[i];\n \t\tcheck_object(entry);\n \t\tif (big_file_threshold <= entry->size)\n \t\t\tentry->no_try_delta = 1;\n+\t\tif (pack_to_stdout && pack_size_limit && !entry->preferred_base)\n+\t\t\tsum_size += estimate_packed_size(entry);\n \t}\n \n+\tif (pack_to_stdout && pack_size_limit && sum_size > pack_size_limit)\n+\t\tdie(\"estimated pack size exceeds the pack size limit (%lu bytes)\",\n+\t\t pack_size_limit);\n+\n \tfree(sorted_by_offset);\n }\n \n-- \n1.7.5.rc1.3.g4d7b\n"},{"id":"168473","messageId":"1306111923-16859-11-git-send-email-johan@herland.net","threadId":"27432","inReplyTo":"1306111923-16859-1-git-send-email-johan@herland.net","subject":"[PATCHv4 10/10] receive-pack: Allow server to refuse pushes with too many objects","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-05-23T00:52:03Z","receivedAt":"2011-05-23T00:52:03Z","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. The server\naborts the transfer if the pack header received from the client indicates\na number of objects that exceeds this upper limit.\n\nThis limit is not advertised to clients, but is only enforced server-side.\nWhen the limit is exceeded, the server sends a helpful error message to the\nclient, and then aborts the transfer, leaving the client with a broken pipe.\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 builtin/receive-pack.c | 11 +++++++++--\n t/t5400-send-pack.sh | 44 ++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 62 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 79d553a..8618979 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1610,6 +1610,15 @@ receive.commitCountLimit::\n \tthere is no upper limit on the number of commits 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\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 49d29ad..e9e5521 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_commit_count;\n+static unsigned long limit_pack_size, limit_commit_count, limit_object_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.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@@ -673,7 +678,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/t/t5400-send-pack.sh b/t/t5400-send-pack.sh\nindex 4ed5ba1..b65d69d 100755\n--- a/t/t5400-send-pack.sh\n+++ b/t/t5400-send-pack.sh\n@@ -349,4 +349,48 @@ test_expect_success 'push is allowed when pack size is not exceeded' '\n \ttest \"$parent_head\" = \"$child_head\"\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 'push is allowed when object limit is increased' '\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":"168505","messageId":"BANLkTimQURetDxYeAJr5sVRB-ja9yuqT7Q@mail.gmail.com","threadId":"27432","inReplyTo":"1306111923-16859-10-git-send-email-johan@herland.net","subject":"Re: [PATCHv4 09/10] pack-objects: Estimate pack size; abort early if pack size limit is exceeded","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2011-05-23T16:11:15Z","receivedAt":"2011-05-23T16:11:15Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Sun, May 22, 2011 at 17:52, Johan Herland wrote:\n> Currently, when pushing a pack to the server that has specified a pack size\n> limit, we don't detect that we exceed that limit until we have already\n> generated (and started transmitting) that much pack data.\n>\n> Ideally, we should be able to predict the approximate pack size _before_ we\n> start generating and transmitting the pack data, and abort early if the\n> estimated pack size exceeds the pack size limit.\n>\n> This patch tries to provide such an estimate: It looks at the objects that\n> are to be included in the pack, and for already-packed objects, it assumes\n> that their compressed in-pack size is a good estimate of how much they will\n> contribute to the pack currently being generated. This assumption should be\n> valid as long as the objects are reused as-is.\n\nThis looks good to me.\n\n> I'm not really happy with excluding loose objects in the pack size\n> estimate. However, the size contributed by loose objects varies wildly\n> depending on whether a (good) delta is found. Therefore, any estimate\n> done at an early stage is bound to be wildly inaccurate. We could maybe\n> use some sort of absolute minimum size per object instead, but I\n> thought I should publish this version before spending more time futzing\n> with it...\n>\n> A drawback of not including loose objects in the pack size estimate,\n> is that pushing loose objects is a very common use case (most people\n> push more often than they 'git gc'). However, for the pack sizes that\n> servers are most likely to refuse (hundreds of megabytes), most of\n> those objects will probably already be packed anyway (e.g. by\n> 'git gc --auto'), so I still hope the pack size estimate will be useful\n> when it really matters.\n\nThat is my impression too. Most servers using this feature will\nprobably put a limit of at least 10MB. Once you get into the 25-100M\nrange, the client probably has already packed the bulk of that\ncontent. Especially if we also have Junio's new stream large blobs to\npacks during git add patch. So as you point out, cases where this is\nmostly useful (really huge push) this is likely to still trigger\ncorrectly.\n\nWe can still get a tighter estimate if we wanted to. I wouldn't mix it\ninto this patch, but make a new one on top of it. During delta\ncompression we hold onto deltas, or at least compute and retain the\nsize of the chosen delta. We could re-check the pack size after the\nCompressing phase by including the delta sizes in the estimate, and if\nwe are over, abort before writing.\n\nFor non-delta, non-reuse we may be able to guess by just using the\nloose object size. The loose object is most likely compressed at the\nsame compression ratio as the outgoing pack stream will use, so a\ndeflate(inflate(loose)) cycle is going to be very close in total bytes\nused. If we over shoot the limit by more than some fudge factor (say\n8K in 1M limit or 0.7%), abort before writing.\n\n-- \nShawn.\n"},{"id":"168510","messageId":"201105231907.27639.johan@herland.net","threadId":"27432","inReplyTo":"BANLkTimQURetDxYeAJr5sVRB-ja9yuqT7Q@mail.gmail.com","subject":"Re: [PATCHv4 09/10] pack-objects: Estimate pack size; abort early if pack size limit is exceeded","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-05-23T17:07:27Z","receivedAt":"2011-05-23T17:07:27Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Monday 23. May 2011, Shawn Pearce wrote:\n> We can still get a tighter estimate if we wanted to. I wouldn't mix\n> it into this patch, but make a new one on top of it. During delta\n> compression we hold onto deltas, or at least compute and retain the\n> size of the chosen delta. We could re-check the pack size after the\n> Compressing phase by including the delta sizes in the estimate, and\n> if we are over, abort before writing.\n\nOk. Not sure when I'll have the time/courage to dive into this, but I'll \nat least give it a try.\n\n> For non-delta, non-reuse we may be able to guess by just using the\n> loose object size. The loose object is most likely compressed at the\n> same compression ratio as the outgoing pack stream will use, so a\n> deflate(inflate(loose)) cycle is going to be very close in total\n> bytes used. If we over shoot the limit by more than some fudge\n> factor (say 8K in 1M limit or 0.7%), abort before writing.\n\nI already have an unsubmitted patch on top of the series that includes \nthe on-disk/compressed size of loose objects in the estimate. However, \nit's quite intrusive (need to extend sha1_object_info() to return \ncompressed size of loose objects). Also, since I don't yet take the \ndelta compression into account, these numbers are obviously unreliable.\n\nThat said, in the cases where loose objects are not deltified it seems \nthe compressed/loose versions are about 3 to 7 bytes larger than the \ncorresponding compressed/packed versions. I guess this is due to the \nloose files using a \" SP NUL\" text header (deflated), \nwhereas the pack uses a more compact binary format (not deflated).\n\nWe could test a large corpus (e.g. linux-kernel) to find the average \ndifference between compressed/loose size and compressed/packed size, and \nthen multiply this with the number of non-delta, non-reuse object to \ndetermine the fudge factor you describe above.\n\n\nHave fun! :)\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"168521","messageId":"7vei3puqp2.fsf@alter.siamese.dyndns.org","threadId":"27432","inReplyTo":"1306111923-16859-3-git-send-email-johan@herland.net","subject":"Re: [PATCHv4 02/10] send-pack: Attempt to retrieve remote status even if pack-objects fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-23T20:06:49Z","receivedAt":"2011-05-23T20:06:49Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> Therefore, we should try to read from the remote end, even when pack-objects\n> dies unexepectedly. We accomplish this by _always_ calling receive_status()\n> after pack_objects(). If the remote end managed to produce a well-formed\n> status report before exiting, then receive_status() simply presents that to\n> the user. Even if the data from the remote end cannot be understood by\n> receive_status(), it will print that data as part of its error message. In\n> any case, we give the user as much information about the failure as possible.\n\n> diff --git a/builtin/send-pack.c b/builtin/send-pack.c\n> index c1f6ddd..5ba5262 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,25 +339,18 @@ 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\tif ((ret = pack_objects(out, remote_refs, extra_have, args))) {\n\nI am not very familiar with this codepath, but you no longer set ref->status\nto REF_STATUS_NONE ...\n\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\n... before calling receive_status() here, and that function can return\nearly without setting anything.\n\nWould that have any negative effect on the code that comes after this part\nin the codepath? or if receive_status() returns a failure, we do not even\nlook at ref->status?\n"},{"id":"168545","messageId":"7v8vtxt3yf.fsf@alter.siamese.dyndns.org","threadId":"27432","inReplyTo":"1306111923-16859-5-git-send-email-johan@herland.net","subject":"Re: [PATCHv4 04/10] receive-pack: Prepare for addition of the new 'limit-*' family of capabilities","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-23T20:21:49Z","receivedAt":"2011-05-23T20:21:49Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> This adds some technical documentation on the 'limit-*' family of\n> capabilities that will be added in the following commits.\n>\n> Also refactor the generation of the capabilities declaration in receive-pack.\n> This will also be further expanded in the following commits.\n>\n> Signed-off-by: Johan Herland \n> ---\n> ...\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 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\nstatic const char *capabilities(void)\n"},{"id":"168542","messageId":"201105240058.10974.johan@herland.net","threadId":"27432","inReplyTo":"7vei3puqp2.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv4 02/10] send-pack: Attempt to retrieve remote status even if pack-objects fails","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-05-23T22:58:10Z","receivedAt":"2011-05-23T22:58:10Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Monday 23 May 2011, Junio C Hamano wrote:\n> Johan Herland writes:\n> > @@ -339,25 +339,18 @@ int send_pack(struct send_pack_args *args,\n> > \n> > \t}\n> > \t\n> > \tif (new_refs && cmds_sent) {\n> > \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\tif ((ret = pack_objects(out, remote_refs, extra_have, args))) {\n> \n> I am not very familiar with this codepath, but you no longer set\n> ref->status to REF_STATUS_NONE ...\n> \n> > ...\n> > \n> > \tif (status_report && cmds_sent)\n> > \n> > -\t\tret = receive_status(in, remote_refs);\n> > -\telse\n> > -\t\tret = 0;\n> > +\t\tret |= receive_status(in, remote_refs);\n> \n> ... before calling receive_status() here, and that function can return\n> early without setting anything.\n> \n> Would that have any negative effect on the code that comes after this\n> part in the codepath? or if receive_status() returns a failure, we do\n> not even look at ref->status?\n\nHmm... I believe I proved the correctness of this to myself when I first \nwrote the patch, but looking at it a second time, I see that I only did so \nfor send_pack() itself. The remote_refs (that no longer has each ref->status \nset to REF_STATUS_NONE on pack_objects() failure) are given as an argument \nto send_pack(), and are still used by the caller after send_pack() has \nreturned (even when it returns with errors).\n\nTherefore, I was wrong to remove this \"ref->status = REF_STATUS_NONE\" loop. \nWill be fixed in the next iteration.\n\n\nThanks for noticing,\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"168546","messageId":"7v4o4lt3bh.fsf@alter.siamese.dyndns.org","threadId":"27432","inReplyTo":"1306111923-16859-6-git-send-email-johan@herland.net","subject":"Re: [PATCHv4 05/10] pack-objects: Teach new option --max-commit-count, limiting #commits in pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-23T23:17:06Z","receivedAt":"2011-05-23T23:17:06Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> The new --max-commit-count option behaves similarly to --max-object-count,\n\nHmm?\n\nDo we have --max-object-count at this point in the series, or am I missing\na patch?\n\n> diff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt\n> index 20c8551..e43904e 100644\n> --- a/Documentation/git-pack-objects.txt\n> +++ b/Documentation/git-pack-objects.txt\n> @@ -113,6 +113,14 @@ base-name::\n> \tThe default is unlimited, unless the config variable\n> \t`pack.packSizeLimit` is set.\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\nIt feels a tad funny to count number of commits in increments of 2^10, but\nthat's Ok ;-).\n\nThe patch itself looks fine. Thanks.\n"},{"id":"168549","messageId":"7vpqn9rnpd.fsf@alter.siamese.dyndns.org","threadId":"27432","inReplyTo":"1306111923-16859-7-git-send-email-johan@herland.net","subject":"Re: [PATCHv4 06/10] send-pack/receive-pack: Allow server to refuse pushes with too many commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-23T23:39:42Z","receivedAt":"2011-05-23T23:39:42Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> However, older clients that do not understand the capability will not check\n> their pack against the limit, and will end up pushing the pack to the server.\n> Currently there is no extra check on the server to detect a push that exceeds\n> receive.commitCountLimit. However, such a check could be done in a pre-receive\n> or update hook.\n\nI found the above a reasonable thing to do. In other words, this is an\nadvisory configuration at this point (and from a cursory scanning of the\nrest of the series, throughout the series), and that is OK.\n\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 1a060ec..c18faac 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1592,6 +1592,15 @@ receive.unpackLimit::\n> \tespecially on slow filesystems. If not set, the value of\n> \t`transfer.unpackLimit` is used instead.\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\nBut then it may probably be a good idea to reword this a bit, to clarify\nthe refusal happens voluntarily by the pusher. E.g.\n\n\tTell \"git push\" not to push more than this many commits at once\n into this repository. This is meant to prevent ... in a single\n push. Note that older versions of \"git push\" may ignore this\n advisory, so if you really want to refuse such a push, you would\n need to arrange to do so in either the pre-receive hook or the\n update hook.\n\n> diff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt\n> index 11849a3..0240967 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-commit-count= (Maximum number of commits in a pack)\n> +\n\nI think s/in a pack/to transfer/ is more appropriate.\n\nIt is a non-essential detail that the current implementation carries only\none pack in a single session between send-pack and receive-pack. When we\nupdate the protocol (with another capability) so that we can send more\nthan one packs in a single session, we would want the maximum number of\ncommits to be honored.\n\nCome to think of it, I do not necessarily agree with the earlier \"max\ncommit count can only be used with max pack size\"; I can accept it if the\nstatement is qualified with \"for now\", though.\n\nIt is entirely reasonable to say that I want to split packs in 2GB chunks,\nand I want to keep the number of commits in the resulting packs (notice\nthe plural) under this fixed ceiling to avoid mistakes, no?\n\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_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\nHmm, at this point wouldn't it become attractive to stop using the static\nfixed sized buffer and instead start using a strbuf or something?\n\n> diff --git a/builtin/send-pack.c b/builtin/send-pack.c\n> index 5ba5262..f91924f 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\n40 is 19 plus terminating NUL plus 20-decimal digits to hold the count?\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_commit_count > 0) {\n> +\t\tsnprintf(buf, sizeof(buf), \"--max-commit-count=%lu\",\n> +\t\t\t args->max_commit_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-commit-count=\")))\n> +\t\targs->max_commit_count = strtoul(p, NULL, 10);\n\nIf we find garbage in *p, we would just run with a random limit, which may\ncause the pack-objects to abort, but that still is a controlled failure\nand is acceptable.\n"},{"id":"168553","messageId":"7vlixxrmbr.fsf@alter.siamese.dyndns.org","threadId":"27432","inReplyTo":"1306111923-16859-8-git-send-email-johan@herland.net","subject":"Re: [PATCHv4 07/10] pack-objects: Allow --max-pack-size to be used together with --stdout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-24T00:09:28Z","receivedAt":"2011-05-24T00:09:28Z","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 have to say that I am not very fond of this approach.\n\nImagine that in the future we may want to allow 32-bit receivers to say \"I\nwant you to transfer data but I cannot handle very large packs locally, so\nplease send the packs in less-than-1GB chunks to make it easier for me to\nstore them in separate packfiles\" (obviously this involves a new protocol\nextension). The underlying machinery the sending side would use would\nnaturally want to say\n\n\tgit pack-objects --max-pack-size=1GB --stdout\n\nand you would see the data for the first pack, followed by the data for\nthe second pack, etc. on the standard output. Such a receiver might also\nwant to say \"You are not allowed to send more than 3GB at once to me\" to\nthe sending side. What should the \"pack-objects\" command line look like?\n\nI think you should keep the two concepts separate. max-pack-size should\nstay the limit of each packfile \"git pack-objects\" is allowed to produce,\nand there should be another option to specify the total pack data to be\nproduced, perhaps named --max-total-pack-size or something.\n\nThat would make your earlier \"count\" thing --max-total-commit-count; it is\nperfectly fine that we do not plan to have the --max-commit-count option\nthat is per packfile.\n\nSome people may kvetch about \"inconsistency\", but I think this is a\njustifiable inconsistency, as \"You should refrain from putting more than\nthis number of commits in a single pack\" is a limit that does not have any\npractical use, while \"You should refrain from sending more than this\nnumber of commits in total\" does.\n"},{"id":"168554","messageId":"7vhb8lrm68.fsf@alter.siamese.dyndns.org","threadId":"27432","inReplyTo":"1306111923-16859-9-git-send-email-johan@herland.net","subject":"Re: [PATCHv4 08/10] send-pack/receive-pack: Allow server to refuse pushing too large packs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-24T00:12:47Z","receivedAt":"2011-05-24T00:12:47Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> Add a new receive.packSizeLimit config variable which defines an upper\n> limit on the pack size to accept in a single push.\n>\n> This limit is advertised to clients, using the new \"limit-pack-size=\"\n> capability.\n\nContinuing my comments to 7/10, this shouldn't be closely tied to \"pack\",\nbut a more abstract concept of \"total transferred data in bytes\".\n\nAside from the name, the implementation looks fine, modulo that it should\nnot be passing --max-pack-size but --max-total-pack-size to the underlying\npack-objects command.\n\nThanks.\n"},{"id":"168555","messageId":"201105240216.49225.johan@herland.net","threadId":"27432","inReplyTo":"7v8vtxt3yf.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv4 04/10] receive-pack: Prepare for addition of the new 'limit-*' family of capabilities","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-05-24T00:16:48Z","receivedAt":"2011-05-24T00:16:48Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Monday 23 May 2011, Junio C Hamano wrote:\n> Johan Herland writes:\n> > This adds some technical documentation on the 'limit-*' family of\n> > capabilities that will be added in the following commits.\n> > \n> > Also refactor the generation of the capabilities declaration in\n> > receive-pack. This will also be further expanded in the following\n> > commits.\n> > \n> > Signed-off-by: Johan Herland \n> > ---\n> > ...\n> > diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> > index 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,\n> > const char *value, void *cb)\n> > \n> > \treturn git_default_config(var, value, cb);\n> > \n> > }\n> > \n> > +static const char *capabilities()\n> \n> static const char *capabilities(void)\n\nThanks, will be in the next iteration\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"168556","messageId":"201105240218.22030.johan@herland.net","threadId":"27432","inReplyTo":"7v4o4lt3bh.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv4 05/10] pack-objects: Teach new option --max-commit-count, limiting #commits in pack","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-05-24T00:18:21Z","receivedAt":"2011-05-24T00:18:21Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Tuesday 24 May 2011, Junio C Hamano wrote:\n> Johan Herland writes:\n> > The new --max-commit-count option behaves similarly to\n> > --max-object-count,\n> \n> Hmm?\n> \n> Do we have --max-object-count at this point in the series, or am I\n> missing a patch?\n\nNo, I dropped --max-object-count. Will update this commit message in the \nnext iteration.\n\n\n...Johan\n\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"168557","messageId":"7vd3j9rlw7.fsf@alter.siamese.dyndns.org","threadId":"27432","inReplyTo":"1306111923-16859-10-git-send-email-johan@herland.net","subject":"Re: [PATCHv4 09/10] pack-objects: Estimate pack size; abort early if pack size limit is exceeded","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-24T00:18:48Z","receivedAt":"2011-05-24T00:18:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> I'm not really happy with excluding loose objects in the pack size\n> estimate. However, the size contributed by loose objects varies wildly\n> depending on whether a (good) delta is found. Therefore, any estimate\n> done at an early stage is bound to be wildly inaccurate. We could maybe\n> use some sort of absolute minimum size per object instead, but I\n> thought I should publish this version before spending more time futzing\n> with it...\n\nAs the initial approximation, I think this implementation is fine.\nContinuing my comments on 7/10, \"pack_to_stdout && pack_size_limit\" part\nmay need to be replaced with \"total_data_limit\" or something, though.\n\nAfter all it isn't fundamentally wrong to say \"I don't want to pack more\nthan 2GB in total\" even when you are producing packs on your local disk\nwithout the --stdout option, no?\n"},{"id":"168562","messageId":"201105240311.41440.johan@herland.net","threadId":"27432","inReplyTo":"7vpqn9rnpd.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv4 06/10] send-pack/receive-pack: Allow server to refuse pushes with too many commits","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-05-24T01:11:41Z","receivedAt":"2011-05-24T01:11:41Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Tuesday 24 May 2011, Junio C Hamano wrote:\n> Johan Herland writes:\n> > However, older clients that do not understand the capability will not\n> > check their pack against the limit, and will end up pushing the pack\n> > to the server. Currently there is no extra check on the server to\n> > detect a push that exceeds receive.commitCountLimit. However, such a\n> > check could be done in a pre-receive or update hook.\n> \n> I found the above a reasonable thing to do. In other words, this is an\n> advisory configuration at this point (and from a cursory scanning of the\n> rest of the series, throughout the series), and that is OK.\n> \n> > diff --git a/Documentation/config.txt b/Documentation/config.txt\n> > index 1a060ec..c18faac 100644\n> > --- a/Documentation/config.txt\n> > +++ b/Documentation/config.txt\n> > \n> > @@ -1592,6 +1592,15 @@ receive.unpackLimit::\n> > \tespecially on slow filesystems. If not set, the value of\n> > \t`transfer.unpackLimit` is used instead.\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> But then it may probably be a good idea to reword this a bit, to clarify\n> the refusal happens voluntarily by the pusher. E.g.\n> \n> \tTell \"git push\" not to push more than this many commits at once\n> into this repository. This is meant to prevent ... in a single\n> push. Note that older versions of \"git push\" may ignore this\n> advisory, so if you really want to refuse such a push, you would\n> need to arrange to do so in either the pre-receive hook or the\n> update hook.\n\nAgreed. Thanks for the rewording.\n\n> > diff --git a/Documentation/technical/protocol-capabilities.txt\n> > b/Documentation/technical/protocol-capabilities.txt index\n> > 11849a3..0240967 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\n> > abort the upload, and\n> > \n> > report the reason for the aborted push back to the user.\n> > \n> > The following \"limit-*\" capabilites are recognized:\n> > + - limit-commit-count= (Maximum number of commits in a pack)\n> > +\n> \n> I think s/in a pack/to transfer/ is more appropriate.\n> \n> It is a non-essential detail that the current implementation carries only\n> one pack in a single session between send-pack and receive-pack. When we\n> update the protocol (with another capability) so that we can send more\n> than one packs in a single session, we would want the maximum number of\n> commits to be honored.\n\nAgreed.\n\n> Come to think of it, I do not necessarily agree with the earlier \"max\n> commit count can only be used with max pack size\"; I can accept it if the\n> statement is qualified with \"for now\", though.\n\nI'll add the qualification.\n\n> It is entirely reasonable to say that I want to split packs in 2GB\n> chunks, and I want to keep the number of commits in the resulting packs\n> (notice the plural) under this fixed ceiling to avoid mistakes, no?\n\nI guess it depends on whether you interpret the commit count limit as a per-\npack threshold that triggers pack splitting (similar to how we interpret the \npack size limit), or as an upper bound which aborts pack-objects if \nexceeded.\n\nI initially found it more intuitive to interpret all of these as a fixed \nupper bound when paired with --stdout (since that implicitly limits us to a \nsingle pack), and as a pack splitting threshold when used without --stdout \n(except that triggering pack splits based on commit count is not useful).\n\n> > @@ -112,6 +118,9 @@ static const char *capabilities()\n> > \n> > \tint ret = snprintf(buf, sizeof(buf),\n> > \t\n> > \t\t\t \" report-status delete-refs side-band-64k%s\",\n> > \t\t\t prefer_ofs_delta ? \" ofs-delta\" : \"\");\n> > \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> > \n> > \tassert(ret < sizeof(buf));\n> \n> Hmm, at this point wouldn't it become attractive to stop using the static\n> fixed sized buffer and instead start using a strbuf or something?\n\nYeah. Will fix in next iteration.\n\n> > diff --git a/builtin/send-pack.c b/builtin/send-pack.c\n> > index 5ba5262..f91924f 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,\n> > struct extra_have_objects *ext\n> > \n> > \t\tNULL,\n> > \t\tNULL,\n> > \t\tNULL,\n> > \n> > +\t\tNULL,\n> > \n> > \t};\n> > \tstruct child_process po;\n> > \tint i;\n> > \n> > +\tchar buf[40];\n> \n> 40 is 19 plus terminating NUL plus 20-decimal digits to hold the count?\n\nIndeed. I will document this more clearly.\n\n> > @@ -263,6 +271,8 @@ int send_pack(struct send_pack_args *args,\n> > \n> > \t\targs->use_ofs_delta = 1;\n> > \t\n> > \tif (server_supports(\"side-band-64k\"))\n> > \t\n> > \t\tuse_sideband = 1;\n> > \n> > +\tif ((p = server_supports(\"limit-commit-count=\")))\n> > +\t\targs->max_commit_count = strtoul(p, NULL, 10);\n> \n> If we find garbage in *p, we would just run with a random limit, which\n> may cause the pack-objects to abort, but that still is a controlled\n> failure and is acceptable.\n\nAgreed.\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"168563","messageId":"201105240315.06549.johan@herland.net","threadId":"27432","inReplyTo":"7vlixxrmbr.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv4 07/10] pack-objects: Allow --max-pack-size to be used together with --stdout","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-05-24T01:15:06Z","receivedAt":"2011-05-24T01:15:06Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Tuesday 24 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 have to say that I am not very fond of this approach.\n> \n> Imagine that in the future we may want to allow 32-bit receivers to say\n> \"I want you to transfer data but I cannot handle very large packs\n> locally, so please send the packs in less-than-1GB chunks to make it\n> easier for me to store them in separate packfiles\" (obviously this\n> involves a new protocol extension). The underlying machinery the sending\n> side would use would naturally want to say\n> \n> \tgit pack-objects --max-pack-size=1GB --stdout\n> \n> and you would see the data for the first pack, followed by the data for\n> the second pack, etc. on the standard output. Such a receiver might also\n> want to say \"You are not allowed to send more than 3GB at once to me\" to\n> the sending side. What should the \"pack-objects\" command line look like?\n> \n> I think you should keep the two concepts separate. max-pack-size should\n> stay the limit of each packfile \"git pack-objects\" is allowed to produce,\n> and there should be another option to specify the total pack data to be\n> produced, perhaps named --max-total-pack-size or something.\n\nOk. I will separate the concepts in the next iteration.\n\n> That would make your earlier \"count\" thing --max-total-commit-count; it\n> is perfectly fine that we do not plan to have the --max-commit-count\n> option that is per packfile.\n\nAgreed.\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"168564","messageId":"201105240317.56144.johan@herland.net","threadId":"27432","inReplyTo":"7vd3j9rlw7.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv4 09/10] pack-objects: Estimate pack size; abort early if pack size limit is exceeded","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-05-24T01:17:56Z","receivedAt":"2011-05-24T01:17:56Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Tuesday 24 May 2011, Junio C Hamano wrote:\n> Johan Herland writes:\n> > I'm not really happy with excluding loose objects in the pack size\n> > estimate. However, the size contributed by loose objects varies wildly\n> > depending on whether a (good) delta is found. Therefore, any estimate\n> > done at an early stage is bound to be wildly inaccurate. We could maybe\n> > use some sort of absolute minimum size per object instead, but I\n> > thought I should publish this version before spending more time futzing\n> > with it...\n> \n> As the initial approximation, I think this implementation is fine.\n> Continuing my comments on 7/10, \"pack_to_stdout && pack_size_limit\" part\n> may need to be replaced with \"total_data_limit\" or something, though.\n> \n> After all it isn't fundamentally wrong to say \"I don't want to pack more\n> than 2GB in total\" even when you are producing packs on your local disk\n> without the --stdout option, no?\n\nAgreed. Will fix in the next iteration.\n\n\nThanks for reviewing the series! It is very much appreciated.\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"}]}