[PATCHv4 00/10] Push limits

[PATCHv4 00/10] Push limits

[Johan Herland]

Finally, I found some time to re-roll this series. Here's a quick
overview of the changes since the previous iteration:

 - Rebased onto 'next' to include the deadlock fix from Peff and J6t.

 - Reshuffle the patch series to leave the more contentious patches
   towards the end of the series.

 - (patch #3) Implement tighter matching rules in server_supports(),
   as suggested by Junio in the previous thread.

 - Remove --max-object-count from pack-objects and limit-object-count
   capability, since object count is not considered a useful metric
   for limiting pushes. However, keep the server-side object count
   limit, since it is the only metric we can cheaply check on the
   server-side (unless we change the pack format to include pack size
   and/or commit count). This is now found in patch #10.

 - (patch #9) In pack-objects, attempt to estimate the pack size before
   we start writing out pack data. Abort as early as possible if the
   estimated pack size exceeds the pack size limit. The estimate is
   based on the in-pack size of already packed objects (assumed to be
   reused as-is). The patch does not attempt to estimate the packed
   size of currently loose objects. Therefore, whenever we're pushing
   unpacked objects, we end up underestimating the pack size. This is
   suboptimal, but ok, since we will still abort the transfer if we
   exceed the pack size limit while writing out the pack data.


Have fun! :)

...Johan


Johan Herland (10):
  Update technical docs to reflect side-band-64k capability in receive-pack
  send-pack: Attempt to retrieve remote status even if pack-objects fails
  Tighten rules for matching server capabilities in server_supports()
  receive-pack: Prepare for addition of the new 'limit-*' family of capabilities
  pack-objects: Teach new option --max-commit-count, limiting #commits in pack
  send-pack/receive-pack: Allow server to refuse pushes with too many commits
  pack-objects: Allow --max-pack-size to be used together with --stdout
  send-pack/receive-pack: Allow server to refuse pushing too large packs
  pack-objects: Estimate pack size; abort early if pack size limit is exceeded
  receive-pack: Allow server to refuse pushes with too many objects

 Documentation/config.txt                          |   27 ++++
 Documentation/git-pack-objects.txt                |   11 ++
 Documentation/technical/pack-protocol.txt         |    5 +-
 Documentation/technical/protocol-capabilities.txt |   29 ++++-
 builtin/pack-objects.c                            |   56 ++++++-
 builtin/receive-pack.c                            |   42 +++++-
 builtin/send-pack.c                               |   31 +++--
 cache.h                                           |    2 +-
 connect.c                                         |   30 ++++-
 send-pack.h                                       |    2 +
 t/t5300-pack-object.sh                            |   77 +++++++++
 t/t5400-send-pack.sh                              |  171 +++++++++++++++++++++
 12 files changed, 453 insertions(+), 30 deletions(-)

-- 
1.7.5.rc1.3.g4d7b

[PATCHv4 01/10] Update technical docs to reflect side-band-64k capability in receive-pack

[Johan Herland]

Signed-off-by: Johan Herland <johan@herland.net>
---
 Documentation/technical/pack-protocol.txt         |    3 ++-
 Documentation/technical/protocol-capabilities.txt |    4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt
index 369f91d..4a68f0f 100644
--- a/Documentation/technical/pack-protocol.txt
+++ b/Documentation/technical/pack-protocol.txt
@@ -391,7 +391,8 @@ The reference discovery phase is done nearly the same way as it is in the
 fetching protocol. Each reference obj-id and name on the server is sent
 in packet-line format to the client, followed by a flush-pkt.  The only
 real difference is that the capability listing is different - the only
-possible values are 'report-status', 'delete-refs' and 'ofs-delta'.
+possible values are 'report-status', 'delete-refs', 'side-band-64k' and
+'ofs-delta'.
 
 Reference Update Request and Packfile Transfer
 ----------------------------------------------
diff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt
index b15517f..b732e80 100644
--- a/Documentation/technical/protocol-capabilities.txt
+++ b/Documentation/technical/protocol-capabilities.txt
@@ -21,8 +21,8 @@ NOT advertise capabilities it does not understand.
 The 'report-status' and 'delete-refs' capabilities are sent and
 recognized by the receive-pack (push to server) process.
 
-The 'ofs-delta' capability is sent and recognized by both upload-pack
-and receive-pack protocols.
+The 'side-band-64k' and 'ofs-delta' capabilities are sent and
+recognized by both upload-pack and receive-pack protocols.
 
 All other capabilities are only recognized by the upload-pack (fetch
 from server) process.
-- 
1.7.5.rc1.3.g4d7b

[PATCHv4 02/10] send-pack: Attempt to retrieve remote status even if pack-objects fails

[Johan Herland]

When pushing, send-pack uses pack-objects to write the pack data to the
receive-pack process running on the remote end. The scenarios where
pack-objects dies unexpectedly, can be roughly divided based on whether
the reason for the failure is _local_ (i.e. something in pack-objects
caused it to fail of its own accord), or _remote_ (i.e. something in
the remote receive-pack process caused it to fail, leaving the local
pack-objects process with a broken pipe)

If the reason for the failure is local, we expect pack-objects to report
an appropriate error message to the user.

However, if the reason for the failure is remote, pack-objects will merely
abort because of the broken pipe, and the user is left with no clue as to
the reason why the remote receive-pack process died.

In certain cases, though, the receive-pack process on the other end may have
produced an error message immediately before exiting. This error message may
be currently waiting to be read by the local send-pack process.

Therefore, we should try to read from the remote end, even when pack-objects
dies unexepectedly. We accomplish this by _always_ calling receive_status()
after pack_objects(). If the remote end managed to produce a well-formed
status report before exiting, then receive_status() simply presents that to
the user. Even if the data from the remote end cannot be understood by
receive_status(), it will print that data as part of its error message. In
any case, we give the user as much information about the failure as possible.

Signed-off-by: Johan Herland <johan@herland.net>
---
 builtin/send-pack.c |   13 +++----------
 1 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index c1f6ddd..5ba5262 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -251,7 +251,7 @@ int send_pack(struct send_pack_args *args,
 	int status_report = 0;
 	int use_sideband = 0;
 	unsigned cmds_sent = 0;
-	int ret;
+	int ret = 0;
 	struct async demux;
 
 	/* Does the other end support the reporting? */
@@ -339,25 +339,18 @@ int send_pack(struct send_pack_args *args,
 	}
 
 	if (new_refs && cmds_sent) {
-		if (pack_objects(out, remote_refs, extra_have, args) < 0) {
-			for (ref = remote_refs; ref; ref = ref->next)
-				ref->status = REF_STATUS_NONE;
+		if ((ret = pack_objects(out, remote_refs, extra_have, args))) {
 			if (args->stateless_rpc)
 				close(out);
 			if (git_connection_is_socket(conn))
 				shutdown(fd[0], SHUT_WR);
-			if (use_sideband)
-				finish_async(&demux);
-			return -1;
 		}
 	}
 	if (args->stateless_rpc && cmds_sent)
 		packet_flush(out);
 
 	if (status_report && cmds_sent)
-		ret = receive_status(in, remote_refs);
-	else
-		ret = 0;
+		ret |= receive_status(in, remote_refs);
 	if (args->stateless_rpc)
 		packet_flush(out);
 
-- 
1.7.5.rc1.3.g4d7b

Re: [PATCHv4 02/10] send-pack: Attempt to retrieve remote status even if pack-objects fails

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> Therefore, we should try to read from the remote end, even when pack-objects
> dies unexepectedly. We accomplish this by _always_ calling receive_status()
> after pack_objects(). If the remote end managed to produce a well-formed
> status report before exiting, then receive_status() simply presents that to
> the user. Even if the data from the remote end cannot be understood by
> receive_status(), it will print that data as part of its error message. In
> any case, we give the user as much information about the failure as possible.

> diff --git a/builtin/send-pack.c b/builtin/send-pack.c
> index c1f6ddd..5ba5262 100644
> --- a/builtin/send-pack.c
> +++ b/builtin/send-pack.c
> @@ -251,7 +251,7 @@ int send_pack(struct send_pack_args *args,
>  	int status_report = 0;
>  	int use_sideband = 0;
>  	unsigned cmds_sent = 0;
> -	int ret;
> +	int ret = 0;
>  	struct async demux;
>  
>  	/* Does the other end support the reporting? */
> @@ -339,25 +339,18 @@ int send_pack(struct send_pack_args *args,
>  	}
>  
>  	if (new_refs && cmds_sent) {
> -		if (pack_objects(out, remote_refs, extra_have, args) < 0) {
> -			for (ref = remote_refs; ref; ref = ref->next)
> -				ref->status = REF_STATUS_NONE;
> +		if ((ret = pack_objects(out, remote_refs, extra_have, args))) {

I am not very familiar with this codepath, but you no longer set ref->status
to REF_STATUS_NONE ...

> ...
>  	if (status_report && cmds_sent)
> -		ret = receive_status(in, remote_refs);
> -	else
> -		ret = 0;
> +		ret |= receive_status(in, remote_refs);

... before calling receive_status() here, and that function can return
early without setting anything.

Would that have any negative effect on the code that comes after this part
in the codepath?  or if receive_status() returns a failure, we do not even
look at ref->status?

Re: [PATCHv4 02/10] send-pack: Attempt to retrieve remote status even if pack-objects fails

[Johan Herland]

On Monday 23 May 2011, Junio C Hamano wrote:
> Johan Herland <johan@herland.net> writes:
> > @@ -339,25 +339,18 @@ int send_pack(struct send_pack_args *args,
> > 
> >  	}
> >  	
> >  	if (new_refs && cmds_sent) {
> > 
> > -		if (pack_objects(out, remote_refs, extra_have, args) < 0) {
> > -			for (ref = remote_refs; ref; ref = ref->next)
> > -				ref->status = REF_STATUS_NONE;
> > +		if ((ret = pack_objects(out, remote_refs, extra_have, args))) {
> 
> I am not very familiar with this codepath, but you no longer set
> ref->status to REF_STATUS_NONE ...
> 
> > ...
> > 
> >  	if (status_report && cmds_sent)
> > 
> > -		ret = receive_status(in, remote_refs);
> > -	else
> > -		ret = 0;
> > +		ret |= receive_status(in, remote_refs);
> 
> ... before calling receive_status() here, and that function can return
> early without setting anything.
> 
> Would that have any negative effect on the code that comes after this
> part in the codepath?  or if receive_status() returns a failure, we do
> not even look at ref->status?

Hmm... I believe I proved the correctness of this to myself when I first 
wrote the patch, but looking at it a second time, I see that I only did so 
for send_pack() itself. The remote_refs (that no longer has each ref->status 
set to REF_STATUS_NONE on pack_objects() failure) are given as an argument 
to send_pack(), and are still used by the caller after send_pack() has 
returned (even when it returns with errors).

Therefore, I was wrong to remove this "ref->status = REF_STATUS_NONE" loop. 
Will be fixed in the next iteration.


Thanks for noticing,

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

[PATCHv4 03/10] Tighten rules for matching server capabilities in server_supports()

[Johan Herland]

When using server_supports() to match a given "feature" against the server
capabilities, follow these rules:

 - "feature" must appear at the beginning of server_capabilities, or the
   byte immediately before the matched location in server_capabilities
   must be a SP; and

 - if "feature" does not end with an equal sign, it does not expect a
   value. The byte after the matched location in server_capabilities must
   be either the end of string or a SP. A feature that expects a value is
   checked with 'server_supports("feature=")' and the matched location in
   server_capabilities can be followed by anything (i.e. if at the end of
   string or a SP, it gets an empty string as the value, and otherwise it
   will get the stretch of bytes after the '=' up to the next SP).

Given the server_capabilities string "foo=ab bar=froboz boz",
this patch should make it behave as follows:

  server_supports("foo=") matches "foo=ab", returns "ab";

  server_supports("ab") does not match anything;

  server_supports("bar") does not match anything;

  server_supports("boz") matches (and returns "boz"), without failing
                         at the end of bar=froboz that comes earlier.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Johan Herland <johan@herland.net>
---
 cache.h   |    2 +-
 connect.c |   30 +++++++++++++++++++++++++++---
 2 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/cache.h b/cache.h
index 009b365..1a9338f 100644
--- a/cache.h
+++ b/cache.h
@@ -995,7 +995,7 @@ struct extra_have_objects {
 	unsigned char (*array)[20];
 };
 extern struct ref **get_remote_heads(int in, struct ref **list, int nr_match, char **match, unsigned int flags, struct extra_have_objects *);
-extern int server_supports(const char *feature);
+extern const char *server_supports(const char *feature);
 
 extern struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);
 
diff --git a/connect.c b/connect.c
index 2119c3f..3c0a706 100644
--- a/connect.c
+++ b/connect.c
@@ -8,6 +8,7 @@
 #include "url.h"
 
 static char *server_capabilities;
+static size_t server_capabilities_len;
 
 static int check_ref(const char *name, int len, unsigned int flags)
 {
@@ -80,8 +81,16 @@ struct ref **get_remote_heads(int in, struct ref **list,
 
 		name_len = strlen(name);
 		if (len != name_len + 41) {
+			char *p;
 			free(server_capabilities);
 			server_capabilities = xstrdup(name + name_len + 1);
+			server_capabilities_len = strlen(server_capabilities);
+			/* split capabilities on SP */
+			for (p = server_capabilities;
+			     p < server_capabilities + server_capabilities_len;
+			     p++)
+				if (*p == ' ')
+					*p = '\0';
 		}
 
 		if (extra_have &&
@@ -102,10 +111,25 @@ struct ref **get_remote_heads(int in, struct ref **list,
 	return list;
 }
 
-int server_supports(const char *feature)
+const char *server_supports(const char *feature)
 {
-	return server_capabilities &&
-		strstr(server_capabilities, feature) != NULL;
+	const char *p = server_capabilities;
+	size_t feature_len = strlen(feature);
+	int need_value = feature[feature_len - 1] == '=';
+
+	if (!server_capabilities)
+		return NULL;
+
+	for (p = server_capabilities;
+	     p < server_capabilities + server_capabilities_len;
+	     p += strlen(p) + 1) {
+		if (need_value) {
+			if (!strncmp(p, feature, feature_len))
+				return p + feature_len;
+		} else if (!strcmp(p, feature))
+			return p;
+	}
+	return NULL;
 }
 
 int path_match(const char *path, int nr, char **match)
-- 
1.7.5.rc1.3.g4d7b

[PATCHv4 04/10] receive-pack: Prepare for addition of the new 'limit-*' family of capabilities

[Johan Herland]

This adds some technical documentation on the 'limit-*' family of
capabilities that will be added in the following commits.

Also refactor the generation of the capabilities declaration in receive-pack.
This will also be further expanded in the following commits.

Signed-off-by: Johan Herland <johan@herland.net>
---
 Documentation/technical/pack-protocol.txt         |    6 ++--
 Documentation/technical/protocol-capabilities.txt |   22 +++++++++++++++++++++
 builtin/receive-pack.c                            |   16 +++++++++++---
 3 files changed, 37 insertions(+), 7 deletions(-)

diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt
index 4a68f0f..ddc0d0e 100644
--- a/Documentation/technical/pack-protocol.txt
+++ b/Documentation/technical/pack-protocol.txt
@@ -167,7 +167,7 @@ MUST peel the ref if it's an annotated tag.
   other-peeled     =  obj-id SP refname "^{}" LF
 
   capability-list  =  capability *(SP capability)
-  capability       =  1*(LC_ALPHA / DIGIT / "-" / "_")
+  capability       =  1*(LC_ALPHA / DIGIT / "-" / "_" / "=")
   LC_ALPHA         =  %x61-7A
 ----
 
@@ -391,8 +391,8 @@ The reference discovery phase is done nearly the same way as it is in the
 fetching protocol. Each reference obj-id and name on the server is sent
 in packet-line format to the client, followed by a flush-pkt.  The only
 real difference is that the capability listing is different - the only
-possible values are 'report-status', 'delete-refs', 'side-band-64k' and
-'ofs-delta'.
+possible values are 'report-status', 'delete-refs', 'side-band-64k',
+'ofs-delta' and 'limit-*'.
 
 Reference Update Request and Packfile Transfer
 ----------------------------------------------
diff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt
index b732e80..11849a3 100644
--- a/Documentation/technical/protocol-capabilities.txt
+++ b/Documentation/technical/protocol-capabilities.txt
@@ -21,6 +21,9 @@ NOT advertise capabilities it does not understand.
 The 'report-status' and 'delete-refs' capabilities are sent and
 recognized by the receive-pack (push to server) process.
 
+Any 'limit-*' capabilities may only be sent by the receive-pack
+process. It is never requested by client.
+
 The 'side-band-64k' and 'ofs-delta' capabilities are sent and
 recognized by both upload-pack and receive-pack protocols.
 
@@ -185,3 +188,22 @@ it is capable of accepting a zero-id value as the target
 value of a reference update.  It is not sent back by the client, it
 simply informs the client that it can be sent zero-id values
 to delete references.
+
+limit-*
+-------
+
+If the server sends one or more capabilities that start with "limit-",
+it means that there are certain limits to what kind of pack the server
+will receive. More specifically, these capabilities must be of the form
+"limit-<what>=<num>" where "<what>" (a sequence of lower-case letters,
+digits and "-") describes which property of the pack is limited, and
+"<num>" (a sequence of decimal digits) specifies the limit value.
+Capabilities of this type are not sent back by the client; instead the
+client must verify that the created packfile does not exceed the given
+limits. This check should happen prior to transferring the packfile to
+the server. If the check fails, the client must abort the upload, and
+report the reason for the aborted push back to the user.
+The following "limit-*" capabilites are recognized:
+
+More "limit-*" capabilities may be added in the future. The client
+is free to ignore any "limit-*" capabilities it does not understand.
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index e1ba4dc..c55989d 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -106,15 +106,23 @@ static int receive_pack_config(const char *var, const char *value, void *cb)
 	return git_default_config(var, value, cb);
 }
 
+static const char *capabilities()
+{
+	static char buf[1024];
+	int ret = snprintf(buf, sizeof(buf),
+			   " report-status delete-refs side-band-64k%s",
+			   prefer_ofs_delta ? " ofs-delta" : "");
+	assert(ret < sizeof(buf));
+	return buf;
+}
+
 static int show_ref(const char *path, const unsigned char *sha1, int flag, void *cb_data)
 {
 	if (sent_capabilities)
 		packet_write(1, "%s %s\n", sha1_to_hex(sha1), path);
 	else
-		packet_write(1, "%s %s%c%s%s\n",
-			     sha1_to_hex(sha1), path, 0,
-			     " report-status delete-refs side-band-64k",
-			     prefer_ofs_delta ? " ofs-delta" : "");
+		packet_write(1, "%s %s%c%s\n",
+			     sha1_to_hex(sha1), path, 0, capabilities());
 	sent_capabilities = 1;
 	return 0;
 }
-- 
1.7.5.rc1.3.g4d7b

Re: [PATCHv4 04/10] receive-pack: Prepare for addition of the new 'limit-*' family of capabilities

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> This adds some technical documentation on the 'limit-*' family of
> capabilities that will be added in the following commits.
>
> Also refactor the generation of the capabilities declaration in receive-pack.
> This will also be further expanded in the following commits.
>
> Signed-off-by: Johan Herland <johan@herland.net>
> ---
> ...
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index e1ba4dc..c55989d 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -106,15 +106,23 @@ static int receive_pack_config(const char *var, const char *value, void *cb)
>  	return git_default_config(var, value, cb);
>  }
>  
> +static const char *capabilities()

static const char *capabilities(void)

Re: [PATCHv4 04/10] receive-pack: Prepare for addition of the new 'limit-*' family of capabilities

[Johan Herland]

On Monday 23 May 2011, Junio C Hamano wrote:
> Johan Herland <johan@herland.net> writes:
> > This adds some technical documentation on the 'limit-*' family of
> > capabilities that will be added in the following commits.
> > 
> > Also refactor the generation of the capabilities declaration in
> > receive-pack. This will also be further expanded in the following
> > commits.
> > 
> > Signed-off-by: Johan Herland <johan@herland.net>
> > ---
> > ...
> > diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> > index e1ba4dc..c55989d 100644
> > --- a/builtin/receive-pack.c
> > +++ b/builtin/receive-pack.c
> > @@ -106,15 +106,23 @@ static int receive_pack_config(const char *var,
> > const char *value, void *cb)
> > 
> >  	return git_default_config(var, value, cb);
> >  
> >  }
> > 
> > +static const char *capabilities()
> 
> static const char *capabilities(void)

Thanks, will be in the next iteration


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

[PATCHv4 05/10] pack-objects: Teach new option --max-commit-count, limiting #commits in pack

[Johan Herland]

The new --max-commit-count option behaves similarly to --max-object-count,
when used together with --stdout: It limits the number of commits in the
pack written to stdout. If the pack would exceed this limit, pack-objects
will abort with an error message.

Unlike --max-pack-size and --max-object-count, --max-commit-count must
always be used together with --stdout. This is because using the commit
count to split packs is not at all a good heuristic, since Git does not
necessarily distribute commit objects uniformly across packs.

Documentation and tests are included.

Signed-off-by: Johan Herland <johan@herland.net>
---
 Documentation/git-pack-objects.txt |    8 ++++++++
 builtin/pack-objects.c             |   24 +++++++++++++++++++++---
 t/t5300-pack-object.sh             |   34 ++++++++++++++++++++++++++++++++++
 3 files changed, 63 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt
index 20c8551..e43904e 100644
--- a/Documentation/git-pack-objects.txt
+++ b/Documentation/git-pack-objects.txt
@@ -113,6 +113,14 @@ base-name::
 	The default is unlimited, unless the config variable
 	`pack.packSizeLimit` is set.
 
+--max-commit-count=<n>::
+	This option is only useful together with --stdout.
+	Specifies the maximum number of commits allowed in the created
+	pack. If the number of commits would exceed the given limit,
+	pack-objects will fail with an error message.
+	The number can be suffixed with "k", "m", or "g".
+	The default is unlimited.
+
 --honor-pack-keep::
 	This flag causes an object already in a local pack that
 	has a .keep file to be ignored, even if it would have
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index f402a84..f0fc187 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -61,7 +61,7 @@ struct object_entry {
  */
 static struct object_entry *objects;
 static struct pack_idx_entry **written_list;
-static uint32_t nr_objects, nr_alloc, nr_result, nr_written;
+static uint32_t nr_objects, nr_alloc, nr_result, nr_commits, nr_written;
 
 static int non_empty;
 static int reuse_delta = 1, reuse_object = 1;
@@ -653,8 +653,11 @@ static int add_object_entry(const unsigned char *sha1, enum object_type type,
 	if (ix >= 0) {
 		if (exclude) {
 			entry = objects + object_ix[ix] - 1;
-			if (!entry->preferred_base)
+			if (!entry->preferred_base) {
 				nr_result--;
+				if (entry->type == OBJ_COMMIT)
+					nr_commits--;
+			}
 			entry->preferred_base = 1;
 		}
 		return 0;
@@ -694,8 +697,11 @@ static int add_object_entry(const unsigned char *sha1, enum object_type type,
 		entry->type = type;
 	if (exclude)
 		entry->preferred_base = 1;
-	else
+	else {
 		nr_result++;
+		if (type == OBJ_COMMIT)
+			nr_commits++;
+	}
 	if (found_pack) {
 		entry->in_pack = found_pack;
 		entry->in_pack_offset = found_offset;
@@ -2125,6 +2131,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 	const char **rp_av;
 	int rp_ac_alloc = 64;
 	int rp_ac;
+	unsigned long commit_count_limit = 0;
 
 	read_replace_refs = 0;
 
@@ -2179,6 +2186,11 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 				usage(pack_usage);
 			continue;
 		}
+		if (!prefixcmp(arg, "--max-commit-count=")) {
+			if (!git_parse_ulong(arg+19, &commit_count_limit))
+				usage(pack_usage);
+			continue;
+		}
 		if (!prefixcmp(arg, "--window=")) {
 			char *end;
 			window = strtoul(arg+9, &end, 0);
@@ -2322,6 +2334,9 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 		pack_size_limit = 1024*1024;
 	}
 
+	if (!pack_to_stdout && commit_count_limit)
+		die("--max-commit-count is only useful together with --stdout.");
+
 	if (!pack_to_stdout && thin)
 		die("--thin cannot be used to build an indexable pack.");
 
@@ -2348,6 +2363,9 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 
 	if (non_empty && !nr_result)
 		return 0;
+	if (pack_to_stdout && commit_count_limit && commit_count_limit < nr_commits)
+		die("unable to make pack within the commit count limit"
+			" (%lu commits)", commit_count_limit);
 	if (nr_result)
 		prepare_pack(window, depth);
 	write_pack_file();
diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
index 602806d..80df631 100755
--- a/t/t5300-pack-object.sh
+++ b/t/t5300-pack-object.sh
@@ -396,6 +396,40 @@ test_expect_success 'verify resulting packs' '
 	git verify-pack test-11-*.pack
 '
 
+test_expect_success 'make a few more commits' '
+	git reset --hard $commit &&
+	echo "change" > file &&
+	git add file &&
+	git commit -m second &&
+	commit2=`git rev-parse --verify HEAD` &&
+	echo "more change" >> file &&
+	git commit -a -m third &&
+	commit3=`git rev-parse --verify HEAD` &&
+	echo "even more change" >> file &&
+	git commit -a -m fourth &&
+	commit4=`git rev-parse --verify HEAD` && {
+		echo $commit &&
+		echo $commit2 &&
+		echo $commit3 &&
+		echo $commit4
+	} >>commit-list
+'
+
+test_expect_success '--stdout works with large enough --max-commit-count' '
+	git pack-objects --revs --stdout --max-commit-count=4 <commit-list >test-17.pack &&
+	git index-pack --strict test-17.pack
+'
+
+test_expect_success 'verify resulting pack' '
+	git verify-pack test-17.pack
+'
+
+test_expect_success '--stdout fails when pack exceeds --max-commit-count' '
+	test_must_fail git pack-objects --revs --stdout --max-commit-count=3 <commit-list >test-18.pack 2>errs &&
+	test_must_fail git index-pack --strict test-18.pack &&
+	grep -q "commit count limit" errs
+'
+
 #
 # WARNING!
 #
-- 
1.7.5.rc1.3.g4d7b

Re: [PATCHv4 05/10] pack-objects: Teach new option --max-commit-count, limiting #commits in pack

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> The new --max-commit-count option behaves similarly to --max-object-count,

Hmm?

Do we have --max-object-count at this point in the series, or am I missing
a patch?

> diff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt
> index 20c8551..e43904e 100644
> --- a/Documentation/git-pack-objects.txt
> +++ b/Documentation/git-pack-objects.txt
> @@ -113,6 +113,14 @@ base-name::
>  	The default is unlimited, unless the config variable
>  	`pack.packSizeLimit` is set.
>  
> +--max-commit-count=<n>::
> +	This option is only useful together with --stdout.
> +	Specifies the maximum number of commits allowed in the created
> +	pack. If the number of commits would exceed the given limit,
> +	pack-objects will fail with an error message.
> +	The number can be suffixed with "k", "m", or "g".
> +	The default is unlimited.

It feels a tad funny to count number of commits in increments of 2^10, but
that's Ok ;-).

The patch itself looks fine.  Thanks.

Re: [PATCHv4 05/10] pack-objects: Teach new option --max-commit-count, limiting #commits in pack

[Johan Herland]

On Tuesday 24 May 2011, Junio C Hamano wrote:
> Johan Herland <johan@herland.net> writes:
> > The new --max-commit-count option behaves similarly to
> > --max-object-count,
> 
> Hmm?
> 
> Do we have --max-object-count at this point in the series, or am I
> missing a patch?

No, I dropped --max-object-count. Will update this commit message in the 
next iteration.


...Johan


-- 
Johan Herland, <johan@herland.net>
www.herland.net

[PATCHv4 06/10] send-pack/receive-pack: Allow server to refuse pushes with too many commits

[Johan Herland]

Add a new receive.commitCountLimit config variable which defines an upper
limit on the number of commits to accept in a single push.

This limit is advertised to clients, using the new "limit-commit-count=<num>"
capability. The client side - aka. send-pack - parses this capability and
forwards it to pack-objects, using the recently added --max-commit-count
option. pack-objects then checks the generated pack against the limit and
aborts the pack generation if the pack would have too many commits.

However, older clients that do not understand the capability will not check
their pack against the limit, and will end up pushing the pack to the server.
Currently there is no extra check on the server to detect a push that exceeds
receive.commitCountLimit. However, such a check could be done in a pre-receive
or update hook.

Documentation and tests are included.

Signed-off-by: Johan Herland <johan@herland.net>
---
 Documentation/config.txt                          |    9 +++
 Documentation/technical/protocol-capabilities.txt |    2 +
 builtin/receive-pack.c                            |    9 +++
 builtin/send-pack.c                               |   10 +++
 send-pack.h                                       |    1 +
 t/t5400-send-pack.sh                              |   65 +++++++++++++++++++++
 6 files changed, 96 insertions(+), 0 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1a060ec..c18faac 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1592,6 +1592,15 @@ receive.unpackLimit::
 	especially on slow filesystems.  If not set, the value of
 	`transfer.unpackLimit` is used instead.
 
+receive.commitCountLimit::
+	If the number of commits received in a push exceeds this limit,
+	then the entire push will be refused. This is meant to prevent
+	an unintended large push (typically a result of the user not
+	being aware of exactly what is being pushed, e.g. pushing a
+	large rewritten history) from entering the repo. If not set,
+	there is no upper limit on the number of commits transferred
+	in a single push.
+
 receive.denyDeletes::
 	If set to true, git-receive-pack will deny a ref update that deletes
 	the ref. Use this to prevent such a ref deletion via a push.
diff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt
index 11849a3..0240967 100644
--- a/Documentation/technical/protocol-capabilities.txt
+++ b/Documentation/technical/protocol-capabilities.txt
@@ -205,5 +205,7 @@ the server. If the check fails, the client must abort the upload, and
 report the reason for the aborted push back to the user.
 The following "limit-*" capabilites are recognized:
 
+ - limit-commit-count=<num> (Maximum number of commits in a pack)
+
 More "limit-*" capabilities may be added in the future. The client
 is free to ignore any "limit-*" capabilities it does not understand.
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index c55989d..32d95fb 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -28,6 +28,7 @@ static int receive_fsck_objects;
 static int receive_unpack_limit = -1;
 static int transfer_unpack_limit = -1;
 static int unpack_limit = 100;
+static unsigned long limit_commit_count;
 static int report_status;
 static int use_sideband;
 static int prefer_ofs_delta = 1;
@@ -73,6 +74,11 @@ static int receive_pack_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (strcmp(var, "receive.commitcountlimit") == 0) {
+		limit_commit_count = git_config_ulong(var, value);
+		return 0;
+	}
+
 	if (strcmp(var, "receive.fsckobjects") == 0) {
 		receive_fsck_objects = git_config_bool(var, value);
 		return 0;
@@ -112,6 +118,9 @@ static const char *capabilities()
 	int ret = snprintf(buf, sizeof(buf),
 			   " report-status delete-refs side-band-64k%s",
 			   prefer_ofs_delta ? " ofs-delta" : "");
+	if (limit_commit_count > 0)
+		ret += snprintf(buf + ret, sizeof(buf) - ret,
+				" limit-commit-count=%lu", limit_commit_count);
 	assert(ret < sizeof(buf));
 	return buf;
 }
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 5ba5262..f91924f 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -49,9 +49,11 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
 		NULL,
 		NULL,
 		NULL,
+		NULL,
 	};
 	struct child_process po;
 	int i;
+	char buf[40];
 
 	i = 4;
 	if (args->use_thin_pack)
@@ -62,6 +64,11 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
 		argv[i++] = "-q";
 	if (args->progress)
 		argv[i++] = "--progress";
+	if (args->max_commit_count > 0) {
+		snprintf(buf, sizeof(buf), "--max-commit-count=%lu",
+			 args->max_commit_count);
+		argv[i++] = buf;
+	}
 	memset(&po, 0, sizeof(po));
 	po.argv = argv;
 	po.in = -1;
@@ -253,6 +260,7 @@ int send_pack(struct send_pack_args *args,
 	unsigned cmds_sent = 0;
 	int ret = 0;
 	struct async demux;
+	const char *p;
 
 	/* Does the other end support the reporting? */
 	if (server_supports("report-status"))
@@ -263,6 +271,8 @@ int send_pack(struct send_pack_args *args,
 		args->use_ofs_delta = 1;
 	if (server_supports("side-band-64k"))
 		use_sideband = 1;
+	if ((p = server_supports("limit-commit-count=")))
+		args->max_commit_count = strtoul(p, NULL, 10);
 
 	if (!remote_refs) {
 		fprintf(stderr, "No refs in common and none specified; doing nothing.\n"
diff --git a/send-pack.h b/send-pack.h
index 05d7ab1..489aabd 100644
--- a/send-pack.h
+++ b/send-pack.h
@@ -12,6 +12,7 @@ struct send_pack_args {
 		use_ofs_delta:1,
 		dry_run:1,
 		stateless_rpc:1;
+	unsigned long max_commit_count;
 };
 
 int send_pack(struct send_pack_args *args,
diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index 0eace37..285601d 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh
@@ -222,4 +222,69 @@ test_expect_success 'deny pushing to delete current branch' '
 	)
 '
 
+echo "0000" > pkt-flush
+
+test_expect_success 'verify that limit-commit-count capability is not advertised by default' '
+	rewound_push_setup &&
+	(
+	    cd parent &&
+	    test_might_fail git receive-pack . <../pkt-flush >output &&
+	    test_must_fail grep -q "limit-commit-count" output
+	)
+'
+
+test_expect_success 'verify that receive.commitCountLimit triggers limit-commit-count capability' '
+	(
+	    cd parent &&
+	    git config receive.commitCountLimit 1 &&
+	    test_might_fail git receive-pack . <../pkt-flush >output &&
+	    grep -q "limit-commit-count=1" output
+	)
+'
+
+test_expect_success 'deny pushing when receive.commitCountLimit is exceeded' '
+	(
+	    cd child &&
+	    git reset --hard origin/master &&
+	    echo three > file && git commit -a -m three &&
+	    echo four > file && git commit -a -m four &&
+	    test_must_fail git send-pack ../parent master 2>errs &&
+	    grep -q "commit count limit" errs
+	) &&
+	parent_head=$(cd parent && git rev-parse --verify master) &&
+	child_head=$(cd child && git rev-parse --verify master) &&
+	test "$parent_head" != "$child_head"
+'
+
+test_expect_success 'repeated push failure proves that objects were not stored remotely' '
+	(
+	    cd child &&
+	    test_must_fail git send-pack ../parent master 2>errs &&
+	    grep -q "commit count limit" errs
+	) &&
+	parent_head=$(cd parent && git rev-parse --verify master) &&
+	child_head=$(cd child && git rev-parse --verify master) &&
+	test "$parent_head" != "$child_head"
+'
+
+test_expect_success 'increase receive.commitCountLimit' '
+	(
+	    cd parent &&
+	    git config receive.commitCountLimit 2 &&
+	    test_might_fail git receive-pack . <../pkt-flush >output &&
+	    grep -q "limit-commit-count=2" output
+	)
+'
+
+test_expect_success 'push is allowed when commit limit is not exceeded' '
+	(
+	    cd child &&
+	    git send-pack ../parent master 2>errs &&
+	    test_must_fail grep -q "commit count limit" errs
+	) &&
+	parent_head=$(cd parent && git rev-parse --verify master) &&
+	child_head=$(cd child && git rev-parse --verify master) &&
+	test "$parent_head" = "$child_head"
+'
+
 test_done
-- 
1.7.5.rc1.3.g4d7b

Re: [PATCHv4 06/10] send-pack/receive-pack: Allow server to refuse pushes with too many commits

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> However, older clients that do not understand the capability will not check
> their pack against the limit, and will end up pushing the pack to the server.
> Currently there is no extra check on the server to detect a push that exceeds
> receive.commitCountLimit. However, such a check could be done in a pre-receive
> or update hook.

I found the above a reasonable thing to do. In other words, this is an
advisory configuration at this point (and from a cursory scanning of the
rest of the series, throughout the series), and that is OK.

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 1a060ec..c18faac 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1592,6 +1592,15 @@ receive.unpackLimit::
>  	especially on slow filesystems.  If not set, the value of
>  	`transfer.unpackLimit` is used instead.
>  
> +receive.commitCountLimit::
> +	If the number of commits received in a push exceeds this limit,
> +	then the entire push will be refused. This is meant to prevent
> +	an unintended large push (typically a result of the user not
> +	being aware of exactly what is being pushed, e.g. pushing a
> +	large rewritten history) from entering the repo. If not set,
> +	there is no upper limit on the number of commits transferred
> +	in a single push.

But then it may probably be a good idea to reword this a bit, to clarify
the refusal happens voluntarily by the pusher.  E.g.

	Tell "git push" not to push more than this many commits at once
        into this repository. This is meant to prevent ... in a single
        push. Note that older versions of "git push" may ignore this
        advisory, so if you really want to refuse such a push, you would
        need to arrange to do so in either the pre-receive hook or the
        update hook.

> diff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt
> index 11849a3..0240967 100644
> --- a/Documentation/technical/protocol-capabilities.txt
> +++ b/Documentation/technical/protocol-capabilities.txt
> @@ -205,5 +205,7 @@ the server. If the check fails, the client must abort the upload, and
>  report the reason for the aborted push back to the user.
>  The following "limit-*" capabilites are recognized:
>  
> + - limit-commit-count=<num> (Maximum number of commits in a pack)
> +

I think s/in a pack/to transfer/ is more appropriate.

It is a non-essential detail that the current implementation carries only
one pack in a single session between send-pack and receive-pack.  When we
update the protocol (with another capability) so that we can send more
than one packs in a single session, we would want the maximum number of
commits to be honored.

Come to think of it, I do not necessarily agree with the earlier "max
commit count can only be used with max pack size"; I can accept it if the
statement is qualified with "for now", though.

It is entirely reasonable to say that I want to split packs in 2GB chunks,
and I want to keep the number of commits in the resulting packs (notice
the plural) under this fixed ceiling to avoid mistakes, no?

> @@ -112,6 +118,9 @@ static const char *capabilities()
>  	int ret = snprintf(buf, sizeof(buf),
>  			   " report-status delete-refs side-band-64k%s",
>  			   prefer_ofs_delta ? " ofs-delta" : "");
> +	if (limit_commit_count > 0)
> +		ret += snprintf(buf + ret, sizeof(buf) - ret,
> +				" limit-commit-count=%lu", limit_commit_count);
>  	assert(ret < sizeof(buf));

Hmm, at this point wouldn't it become attractive to stop using the static
fixed sized buffer and instead start using a strbuf or something?

> diff --git a/builtin/send-pack.c b/builtin/send-pack.c
> index 5ba5262..f91924f 100644
> --- a/builtin/send-pack.c
> +++ b/builtin/send-pack.c
> @@ -49,9 +49,11 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
>  		NULL,
>  		NULL,
>  		NULL,
> +		NULL,
>  	};
>  	struct child_process po;
>  	int i;
> +	char buf[40];

40 is 19 plus terminating NUL plus 20-decimal digits to hold the count?

>  	i = 4;
>  	if (args->use_thin_pack)
> @@ -62,6 +64,11 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
>  		argv[i++] = "-q";
>  	if (args->progress)
>  		argv[i++] = "--progress";
> +	if (args->max_commit_count > 0) {
> +		snprintf(buf, sizeof(buf), "--max-commit-count=%lu",
> +			 args->max_commit_count);
> +		argv[i++] = buf;
> +	}
>  	memset(&po, 0, sizeof(po));
>  	po.argv = argv;
>  	po.in = -1;
> @@ -253,6 +260,7 @@ int send_pack(struct send_pack_args *args,
>  	unsigned cmds_sent = 0;
>  	int ret = 0;
>  	struct async demux;
> +	const char *p;
>  
>  	/* Does the other end support the reporting? */
>  	if (server_supports("report-status"))
> @@ -263,6 +271,8 @@ int send_pack(struct send_pack_args *args,
>  		args->use_ofs_delta = 1;
>  	if (server_supports("side-band-64k"))
>  		use_sideband = 1;
> +	if ((p = server_supports("limit-commit-count=")))
> +		args->max_commit_count = strtoul(p, NULL, 10);

If we find garbage in *p, we would just run with a random limit, which may
cause the pack-objects to abort, but that still is a controlled failure
and is acceptable.

Re: [PATCHv4 06/10] send-pack/receive-pack: Allow server to refuse pushes with too many commits

[Johan Herland]

On Tuesday 24 May 2011, Junio C Hamano wrote:
> Johan Herland <johan@herland.net> writes:
> > However, older clients that do not understand the capability will not
> > check their pack against the limit, and will end up pushing the pack
> > to the server. Currently there is no extra check on the server to
> > detect a push that exceeds receive.commitCountLimit. However, such a
> > check could be done in a pre-receive or update hook.
> 
> I found the above a reasonable thing to do. In other words, this is an
> advisory configuration at this point (and from a cursory scanning of the
> rest of the series, throughout the series), and that is OK.
> 
> > diff --git a/Documentation/config.txt b/Documentation/config.txt
> > index 1a060ec..c18faac 100644
> > --- a/Documentation/config.txt
> > +++ b/Documentation/config.txt
> > 
> > @@ -1592,6 +1592,15 @@ receive.unpackLimit::
> >  	especially on slow filesystems.  If not set, the value of
> >  	`transfer.unpackLimit` is used instead.
> > 
> > +receive.commitCountLimit::
> > +	If the number of commits received in a push exceeds this limit,
> > +	then the entire push will be refused. This is meant to prevent
> > +	an unintended large push (typically a result of the user not
> > +	being aware of exactly what is being pushed, e.g. pushing a
> > +	large rewritten history) from entering the repo. If not set,
> > +	there is no upper limit on the number of commits transferred
> > +	in a single push.
> 
> But then it may probably be a good idea to reword this a bit, to clarify
> the refusal happens voluntarily by the pusher.  E.g.
> 
> 	Tell "git push" not to push more than this many commits at once
>         into this repository. This is meant to prevent ... in a single
>         push. Note that older versions of "git push" may ignore this
>         advisory, so if you really want to refuse such a push, you would
>         need to arrange to do so in either the pre-receive hook or the
>         update hook.

Agreed. Thanks for the rewording.

> > diff --git a/Documentation/technical/protocol-capabilities.txt
> > b/Documentation/technical/protocol-capabilities.txt index
> > 11849a3..0240967 100644
> > --- a/Documentation/technical/protocol-capabilities.txt
> > +++ b/Documentation/technical/protocol-capabilities.txt
> > @@ -205,5 +205,7 @@ the server. If the check fails, the client must
> > abort the upload, and
> > 
> >  report the reason for the aborted push back to the user.
> > 
> >  The following "limit-*" capabilites are recognized:
> > + - limit-commit-count=<num> (Maximum number of commits in a pack)
> > +
> 
> I think s/in a pack/to transfer/ is more appropriate.
> 
> It is a non-essential detail that the current implementation carries only
> one pack in a single session between send-pack and receive-pack.  When we
> update the protocol (with another capability) so that we can send more
> than one packs in a single session, we would want the maximum number of
> commits to be honored.

Agreed.

> Come to think of it, I do not necessarily agree with the earlier "max
> commit count can only be used with max pack size"; I can accept it if the
> statement is qualified with "for now", though.

I'll add the qualification.

> It is entirely reasonable to say that I want to split packs in 2GB
> chunks, and I want to keep the number of commits in the resulting packs
> (notice the plural) under this fixed ceiling to avoid mistakes, no?

I guess it depends on whether you interpret the commit count limit as a per-
pack threshold that triggers pack splitting (similar to how we interpret the 
pack size limit), or as an upper bound which aborts pack-objects if 
exceeded.

I initially found it more intuitive to interpret all of these as a fixed 
upper bound when paired with --stdout (since that implicitly limits us to a 
single pack), and as a pack splitting threshold when used without --stdout 
(except that triggering pack splits based on commit count is not useful).

> > @@ -112,6 +118,9 @@ static const char *capabilities()
> > 
> >  	int ret = snprintf(buf, sizeof(buf),
> >  	
> >  			   " report-status delete-refs side-band-64k%s",
> >  			   prefer_ofs_delta ? " ofs-delta" : "");
> > 
> > +	if (limit_commit_count > 0)
> > +		ret += snprintf(buf + ret, sizeof(buf) - ret,
> > +				" limit-commit-count=%lu", limit_commit_count);
> > 
> >  	assert(ret < sizeof(buf));
> 
> Hmm, at this point wouldn't it become attractive to stop using the static
> fixed sized buffer and instead start using a strbuf or something?

Yeah. Will fix in next iteration.

> > diff --git a/builtin/send-pack.c b/builtin/send-pack.c
> > index 5ba5262..f91924f 100644
> > --- a/builtin/send-pack.c
> > +++ b/builtin/send-pack.c
> > @@ -49,9 +49,11 @@ static int pack_objects(int fd, struct ref *refs,
> > struct extra_have_objects *ext
> > 
> >  		NULL,
> >  		NULL,
> >  		NULL,
> > 
> > +		NULL,
> > 
> >  	};
> >  	struct child_process po;
> >  	int i;
> > 
> > +	char buf[40];
> 
> 40 is 19 plus terminating NUL plus 20-decimal digits to hold the count?

Indeed. I will document this more clearly.

> > @@ -263,6 +271,8 @@ int send_pack(struct send_pack_args *args,
> > 
> >  		args->use_ofs_delta = 1;
> >  	
> >  	if (server_supports("side-band-64k"))
> >  	
> >  		use_sideband = 1;
> > 
> > +	if ((p = server_supports("limit-commit-count=")))
> > +		args->max_commit_count = strtoul(p, NULL, 10);
> 
> If we find garbage in *p, we would just run with a random limit, which
> may cause the pack-objects to abort, but that still is a controlled
> failure and is acceptable.

Agreed.


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

[PATCHv4 07/10] pack-objects: Allow --max-pack-size to be used together with --stdout

[Johan Herland]

Currently we refuse combining --max-pack-size with --stdout since there's
no way to make multiple packs when the pack is written to stdout. However,
we want to be able to limit the maximum size of the pack created by
--stdout (and abort pack-objects if we are unable to meet that limit).

Therefore, when used together with --stdout, we reinterpret --max-pack-size
to indicate the maximum pack size which - if exceeded - will cause
pack-objects to abort with an error message.

Signed-off-by: Johan Herland <johan@herland.net>
---
 Documentation/git-pack-objects.txt |    3 ++
 builtin/pack-objects.c             |    9 ++++---
 t/t5300-pack-object.sh             |   43 ++++++++++++++++++++++++++++++++++++
 3 files changed, 51 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt
index e43904e..24cf975 100644
--- a/Documentation/git-pack-objects.txt
+++ b/Documentation/git-pack-objects.txt
@@ -112,6 +112,9 @@ base-name::
 	If specified,  multiple packfiles may be created.
 	The default is unlimited, unless the config variable
 	`pack.packSizeLimit` is set.
++
+When used together with --stdout, the command will fail with an error
+message if the pack output exceeds the given limit.
 
 --max-commit-count=<n>::
 	This option is only useful together with --stdout.
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index f0fc187..e226053 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -229,7 +229,7 @@ static unsigned long write_object(struct sha1file *f,
 
 	if (!entry->delta)
 		usable_delta = 0;	/* no delta */
-	else if (!pack_size_limit)
+	else if (!pack_size_limit || pack_to_stdout)
 	       usable_delta = 1;	/* unlimited packfile */
 	else if (entry->delta->idx.offset == (off_t)-1)
 		usable_delta = 0;	/* base was written to another pack */
@@ -478,6 +478,9 @@ static void write_pack_file(void)
 		 * If so, rewrite it like in fast-import
 		 */
 		if (pack_to_stdout) {
+			if (nr_written != nr_remaining)
+				die("unable to make pack within the pack size"
+				    " limit (%lu bytes)", pack_size_limit);
 			sha1close(f, sha1, CSUM_CLOSE);
 		} else if (nr_written == nr_remaining) {
 			sha1close(f, sha1, CSUM_FSYNC);
@@ -2327,9 +2330,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 
 	if (!pack_to_stdout && !pack_size_limit)
 		pack_size_limit = pack_size_limit_cfg;
-	if (pack_to_stdout && pack_size_limit)
-		die("--max-pack-size cannot be used to build a pack for transfer.");
-	if (pack_size_limit && pack_size_limit < 1024*1024) {
+	if (!pack_to_stdout && pack_size_limit && pack_size_limit < 1024*1024) {
 		warning("minimum pack size limit is 1 MiB");
 		pack_size_limit = 1024*1024;
 	}
diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
index 80df631..46c1214 100755
--- a/t/t5300-pack-object.sh
+++ b/t/t5300-pack-object.sh
@@ -396,6 +396,49 @@ test_expect_success 'verify resulting packs' '
 	git verify-pack test-11-*.pack
 '
 
+test_expect_success '--stdout ignores pack.packSizeLimit' '
+	git pack-objects --stdout <obj-list >test-12.pack &&
+	git index-pack --strict test-12.pack
+'
+
+test_expect_success 'verify resulting pack' '
+	git verify-pack test-12.pack
+'
+
+test_expect_success 'honor --max-pack-size' '
+	git config --unset pack.packSizeLimit &&
+	packname_13=$(git pack-objects --max-pack-size=3m test-13 <obj-list) &&
+	test 2 = $(ls test-13-*.pack | wc -l)
+'
+
+test_expect_success 'verify resulting packs' '
+	git verify-pack test-13-*.pack
+'
+
+test_expect_success 'tolerate --max-pack-size smaller than biggest object' '
+	packname_14=$(git pack-objects --max-pack-size=1 test-14 <obj-list) &&
+	test 5 = $(ls test-14-*.pack | wc -l)
+'
+
+test_expect_success 'verify resulting packs' '
+	git verify-pack test-14-*.pack
+'
+
+test_expect_success '--stdout works with large enough --max-pack-size' '
+	git pack-objects --stdout --max-pack-size=10m <obj-list >test-15.pack &&
+	git index-pack --strict test-15.pack
+'
+
+test_expect_success 'verify resulting pack' '
+	git verify-pack test-15.pack
+'
+
+test_expect_success '--stdout fails when pack exceeds --max-pack-size' '
+	test_must_fail git pack-objects --stdout --max-pack-size=1 <obj-list >test-16.pack 2>errs &&
+	test_must_fail git index-pack --strict test-16.pack &&
+	grep -q "pack size limit" errs
+'
+
 test_expect_success 'make a few more commits' '
 	git reset --hard $commit &&
 	echo "change" > file &&
-- 
1.7.5.rc1.3.g4d7b

Re: [PATCHv4 07/10] pack-objects: Allow --max-pack-size to be used together with --stdout

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> Currently we refuse combining --max-pack-size with --stdout since there's
> no way to make multiple packs when the pack is written to stdout. However,
> we want to be able to limit the maximum size of the pack created by
> --stdout (and abort pack-objects if we are unable to meet that limit).
>
> Therefore, when used together with --stdout, we reinterpret --max-pack-size
> to indicate the maximum pack size which - if exceeded - will cause
> pack-objects to abort with an error message.

I have to say that I am not very fond of this approach.

Imagine that in the future we may want to allow 32-bit receivers to say "I
want you to transfer data but I cannot handle very large packs locally, so
please send the packs in less-than-1GB chunks to make it easier for me to
store them in separate packfiles" (obviously this involves a new protocol
extension). The underlying machinery the sending side would use would
naturally want to say

	git pack-objects --max-pack-size=1GB --stdout

and you would see the data for the first pack, followed by the data for
the second pack, etc. on the standard output.  Such a receiver might also
want to say "You are not allowed to send more than 3GB at once to me" to
the sending side. What should the "pack-objects" command line look like?

I think you should keep the two concepts separate. max-pack-size should
stay the limit of each packfile "git pack-objects" is allowed to produce,
and there should be another option to specify the total pack data to be
produced, perhaps named --max-total-pack-size or something.

That would make your earlier "count" thing --max-total-commit-count; it is
perfectly fine that we do not plan to have the --max-commit-count option
that is per packfile.

Some people may kvetch about "inconsistency", but I think this is a
justifiable inconsistency, as "You should refrain from putting more than
this number of commits in a single pack" is a limit that does not have any
practical use, while "You should refrain from sending more than this
number of commits in total" does.

Re: [PATCHv4 07/10] pack-objects: Allow --max-pack-size to be used together with --stdout

[Johan Herland]

On Tuesday 24 May 2011, Junio C Hamano wrote:
> Johan Herland <johan@herland.net> writes:
> > Currently we refuse combining --max-pack-size with --stdout since
> > there's no way to make multiple packs when the pack is written to
> > stdout. However, we want to be able to limit the maximum size of the
> > pack created by --stdout (and abort pack-objects if we are unable to
> > meet that limit).
> > 
> > Therefore, when used together with --stdout, we reinterpret
> > --max-pack-size to indicate the maximum pack size which - if exceeded
> > - will cause pack-objects to abort with an error message.
> 
> I have to say that I am not very fond of this approach.
> 
> Imagine that in the future we may want to allow 32-bit receivers to say
> "I want you to transfer data but I cannot handle very large packs
> locally, so please send the packs in less-than-1GB chunks to make it
> easier for me to store them in separate packfiles" (obviously this
> involves a new protocol extension). The underlying machinery the sending
> side would use would naturally want to say
> 
> 	git pack-objects --max-pack-size=1GB --stdout
> 
> and you would see the data for the first pack, followed by the data for
> the second pack, etc. on the standard output.  Such a receiver might also
> want to say "You are not allowed to send more than 3GB at once to me" to
> the sending side. What should the "pack-objects" command line look like?
> 
> I think you should keep the two concepts separate. max-pack-size should
> stay the limit of each packfile "git pack-objects" is allowed to produce,
> and there should be another option to specify the total pack data to be
> produced, perhaps named --max-total-pack-size or something.

Ok. I will separate the concepts in the next iteration.

> That would make your earlier "count" thing --max-total-commit-count; it
> is perfectly fine that we do not plan to have the --max-commit-count
> option that is per packfile.

Agreed.


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

[PATCHv4 08/10] send-pack/receive-pack: Allow server to refuse pushing too large packs

[Johan Herland]

Add a new receive.packSizeLimit config variable which defines an upper
limit on the pack size to accept in a single push.

This limit is advertised to clients, using the new "limit-pack-size=<num>"
capability. The client side - aka. send-pack - parses this capability and
forwards it to pack-objects, using the --max-pack-size option.
pack-objects then checks the generated pack against the limit and aborts
the pack transmission if the pack becomes too large.

However, older clients that do not understand the capability will not check
their pack against the limit, and will end up pushing the pack to the server.
Currently there is no extra check on the server to detect a push that exceeds
receive.packSizeLimit. However, such a check could be done in a pre-receive
or update hook.

Documentation and tests are included.

Signed-off-by: Johan Herland <johan@herland.net>
---
 Documentation/config.txt                          |    9 +++
 Documentation/technical/protocol-capabilities.txt |    1 +
 builtin/receive-pack.c                            |   10 +++-
 builtin/send-pack.c                               |   14 ++++-
 send-pack.h                                       |    1 +
 t/t5400-send-pack.sh                              |   62 +++++++++++++++++++++
 6 files changed, 93 insertions(+), 4 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index c18faac..79d553a 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1592,6 +1592,15 @@ receive.unpackLimit::
 	especially on slow filesystems.  If not set, the value of
 	`transfer.unpackLimit` is used instead.
 
+receive.packSizeLimit::
+	If the pack file transferred in a push exceeds this limit,
+	then the entire push will be refused. This is meant to prevent
+	an unintended large push (typically a result of the user not
+	being aware of exactly what is being pushed, e.g. pushing a
+	large rewritten history) from entering the repo. If not set,
+	there is no upper limit on the size of the pack transferred
+	in a single push.
+
 receive.commitCountLimit::
 	If the number of commits received in a push exceeds this limit,
 	then the entire push will be refused. This is meant to prevent
diff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt
index 0240967..71566cf 100644
--- a/Documentation/technical/protocol-capabilities.txt
+++ b/Documentation/technical/protocol-capabilities.txt
@@ -205,6 +205,7 @@ the server. If the check fails, the client must abort the upload, and
 report the reason for the aborted push back to the user.
 The following "limit-*" capabilites are recognized:
 
+ - limit-pack-size=<num>    (Maximum size (in bytes) of uploaded pack)
  - limit-commit-count=<num> (Maximum number of commits in a pack)
 
 More "limit-*" capabilities may be added in the future. The client
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 32d95fb..49d29ad 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -28,7 +28,7 @@ static int receive_fsck_objects;
 static int receive_unpack_limit = -1;
 static int transfer_unpack_limit = -1;
 static int unpack_limit = 100;
-static unsigned long limit_commit_count;
+static unsigned long limit_pack_size, limit_commit_count;
 static int report_status;
 static int use_sideband;
 static int prefer_ofs_delta = 1;
@@ -74,6 +74,11 @@ static int receive_pack_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (strcmp(var, "receive.packsizelimit") == 0) {
+		limit_pack_size = git_config_ulong(var, value);
+		return 0;
+	}
+
 	if (strcmp(var, "receive.commitcountlimit") == 0) {
 		limit_commit_count = git_config_ulong(var, value);
 		return 0;
@@ -118,6 +123,9 @@ static const char *capabilities()
 	int ret = snprintf(buf, sizeof(buf),
 			   " report-status delete-refs side-band-64k%s",
 			   prefer_ofs_delta ? " ofs-delta" : "");
+	if (limit_pack_size > 0)
+		ret += snprintf(buf + ret, sizeof(buf) - ret,
+				" limit-pack-size=%lu", limit_pack_size);
 	if (limit_commit_count > 0)
 		ret += snprintf(buf + ret, sizeof(buf) - ret,
 				" limit-commit-count=%lu", limit_commit_count);
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index f91924f..bd7b3c0 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -50,10 +50,11 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
 		NULL,
 		NULL,
 		NULL,
+		NULL,
 	};
 	struct child_process po;
 	int i;
-	char buf[40];
+	char buf1[40], buf2[40];
 
 	i = 4;
 	if (args->use_thin_pack)
@@ -64,10 +65,15 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
 		argv[i++] = "-q";
 	if (args->progress)
 		argv[i++] = "--progress";
+	if (args->max_pack_size > 0) {
+		snprintf(buf1, sizeof(buf1), "--max-pack-size=%lu",
+			 args->max_pack_size);
+		argv[i++] = buf1;
+	}
 	if (args->max_commit_count > 0) {
-		snprintf(buf, sizeof(buf), "--max-commit-count=%lu",
+		snprintf(buf2, sizeof(buf2), "--max-commit-count=%lu",
 			 args->max_commit_count);
-		argv[i++] = buf;
+		argv[i++] = buf2;
 	}
 	memset(&po, 0, sizeof(po));
 	po.argv = argv;
@@ -271,6 +277,8 @@ int send_pack(struct send_pack_args *args,
 		args->use_ofs_delta = 1;
 	if (server_supports("side-band-64k"))
 		use_sideband = 1;
+	if ((p = server_supports("limit-pack-size=")))
+		args->max_pack_size = strtoul(p, NULL, 10);
 	if ((p = server_supports("limit-commit-count=")))
 		args->max_commit_count = strtoul(p, NULL, 10);
 
diff --git a/send-pack.h b/send-pack.h
index 489aabd..4f9b2f1 100644
--- a/send-pack.h
+++ b/send-pack.h
@@ -12,6 +12,7 @@ struct send_pack_args {
 		use_ofs_delta:1,
 		dry_run:1,
 		stateless_rpc:1;
+	unsigned long max_pack_size;
 	unsigned long max_commit_count;
 };
 
diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index 285601d..4ed5ba1 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh
@@ -287,4 +287,66 @@ test_expect_success 'push is allowed when commit limit is not exceeded' '
 	test "$parent_head" = "$child_head"
 '
 
+test_expect_success 'verify that limit-pack-size capability is not advertised by default' '
+	rewound_push_setup &&
+	(
+	    cd parent &&
+	    test_might_fail git receive-pack . <../pkt-flush >output &&
+	    test_must_fail grep -q "limit-pack-size" output
+	)
+'
+
+test_expect_success 'verify that receive.packSizeLimit triggers limit-pack-size capability' '
+	(
+	    cd parent &&
+	    git config receive.packSizeLimit 10 &&
+	    test_might_fail git receive-pack . <../pkt-flush >output &&
+	    grep -q "limit-pack-size=10" output
+	)
+'
+
+test_expect_success 'deny pushing when receive.packSizeLimit is exceeded' '
+	(
+	    cd child &&
+	    git reset --hard origin/master &&
+	    echo three > file && git commit -a -m three &&
+	    test_must_fail git send-pack ../parent master 2>errs &&
+	    grep -q "pack size limit" errs
+	) &&
+	parent_head=$(cd parent && git rev-parse --verify master) &&
+	child_head=$(cd child && git rev-parse --verify master) &&
+	test "$parent_head" != "$child_head"
+'
+
+test_expect_success 'repeated push failure proves that objects were not stored remotely' '
+	(
+	    cd child &&
+	    test_must_fail git send-pack ../parent master 2>errs &&
+	    grep -q "pack size limit" errs
+	) &&
+	parent_head=$(cd parent && git rev-parse --verify master) &&
+	child_head=$(cd child && git rev-parse --verify master) &&
+	test "$parent_head" != "$child_head"
+'
+
+test_expect_success 'increase receive.packSizeLimit' '
+	(
+	    cd parent &&
+	    git config receive.packSizeLimit 1000000 &&
+	    test_might_fail git receive-pack . <../pkt-flush >output &&
+	    grep -q "limit-pack-size=1000000" output
+	)
+'
+
+test_expect_success 'push is allowed when pack size is not exceeded' '
+	(
+	    cd child &&
+	    git send-pack ../parent master 2>errs &&
+	    test_must_fail grep -q "pack size limit" errs
+	) &&
+	parent_head=$(cd parent && git rev-parse --verify master) &&
+	child_head=$(cd child && git rev-parse --verify master) &&
+	test "$parent_head" = "$child_head"
+'
+
 test_done
-- 
1.7.5.rc1.3.g4d7b

Re: [PATCHv4 08/10] send-pack/receive-pack: Allow server to refuse pushing too large packs

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> Add a new receive.packSizeLimit config variable which defines an upper
> limit on the pack size to accept in a single push.
>
> This limit is advertised to clients, using the new "limit-pack-size=<num>"
> capability.

Continuing my comments to 7/10, this shouldn't be closely tied to "pack",
but a more abstract concept of "total transferred data in bytes".

Aside from the name, the implementation looks fine, modulo that it should
not be passing --max-pack-size but --max-total-pack-size to the underlying
pack-objects command.

Thanks.

[PATCHv4 09/10] pack-objects: Estimate pack size; abort early if pack size limit is exceeded

[Johan Herland]

Currently, when pushing a pack to the server that has specified a pack size
limit, we don't detect that we exceed that limit until we have already
generated (and started transmitting) that much pack data.

Ideally, we should be able to predict the approximate pack size _before_ we
start generating and transmitting the pack data, and abort early if the
estimated pack size exceeds the pack size limit.

This patch tries to provide such an estimate: It looks at the objects that
are to be included in the pack, and for already-packed objects, it assumes
that their compressed in-pack size is a good estimate of how much they will
contribute to the pack currently being generated. This assumption should be
valid as long as the objects are reused as-is.

For loose objects that are to be included in the pack, we currently have no
good estimate as to how much they will contribute to the pack size. Since
it's better to underestimate (because an overestimation will prevent us
from sending a pack that might actually be within the pack size limit),
we don't include loose objects at all in the pack size estimate. This makes
the estimate somewhat useless in common workflows (where the push happens
before (most of) the pushed objects are packed).

The estimate is generated before the "Compressing" and "Writing" phases of
the push, so if the estimate exceeds the pack size limit, we abort before
sending any pack data to the server.

If the estimate turns out to be too low (e.g. because we're pushing many
loose objects), there is still code in place to abort the push when we
reach the pack size limit during transmission.

Signed-off-by: Johan Herland <johan@herland.net>
---

I'm not really happy with excluding loose objects in the pack size
estimate. However, the size contributed by loose objects varies wildly
depending on whether a (good) delta is found. Therefore, any estimate
done at an early stage is bound to be wildly inaccurate. We could maybe
use some sort of absolute minimum size per object instead, but I
thought I should publish this version before spending more time futzing
with it...

A drawback of not including loose objects in the pack size estimate,
is that pushing loose objects is a very common use case (most people
push more often than they 'git gc'). However, for the pack sizes that
servers are most likely to refuse (hundreds of megabytes), most of
those objects will probably already be packed anyway (e.g. by
'git gc --auto'), so I still hope the pack size estimate will be useful
when it really matters.


...Johan


 builtin/pack-objects.c |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index e226053..c0c6a0a 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1141,23 +1141,46 @@ static int pack_offset_sort(const void *_a, const void *_b)
 			(a->in_pack_offset > b->in_pack_offset);
 }
 
+static unsigned long estimate_packed_size(const struct object_entry *entry)
+{
+	unsigned long ret;
+	if (entry->in_pack) {
+		/* Assume that all packed objects are reused as-is */
+		struct revindex_entry *revidx = find_pack_revindex(
+			entry->in_pack,
+			entry->in_pack_offset);
+		return revidx[1].offset - entry->in_pack_offset;
+	}
+	return 0;
+}
+
 static void get_object_details(void)
 {
 	uint32_t i;
 	struct object_entry **sorted_by_offset;
+	unsigned long sum_size;
 
 	sorted_by_offset = xcalloc(nr_objects, sizeof(struct object_entry *));
 	for (i = 0; i < nr_objects; i++)
 		sorted_by_offset[i] = objects + i;
 	qsort(sorted_by_offset, nr_objects, sizeof(*sorted_by_offset), pack_offset_sort);
 
+	if (pack_to_stdout && pack_size_limit)
+		sum_size = sizeof(struct pack_header) + 20; /* pack overhead */
+
 	for (i = 0; i < nr_objects; i++) {
 		struct object_entry *entry = sorted_by_offset[i];
 		check_object(entry);
 		if (big_file_threshold <= entry->size)
 			entry->no_try_delta = 1;
+		if (pack_to_stdout && pack_size_limit && !entry->preferred_base)
+			sum_size += estimate_packed_size(entry);
 	}
 
+	if (pack_to_stdout && pack_size_limit && sum_size > pack_size_limit)
+		die("estimated pack size exceeds the pack size limit (%lu bytes)",
+		    pack_size_limit);
+
 	free(sorted_by_offset);
 }
 
-- 
1.7.5.rc1.3.g4d7b

Re: [PATCHv4 09/10] pack-objects: Estimate pack size; abort early if pack size limit is exceeded

[Shawn Pearce]

On Sun, May 22, 2011 at 17:52, Johan Herland <johan@herland.net> wrote:
> Currently, when pushing a pack to the server that has specified a pack size
> limit, we don't detect that we exceed that limit until we have already
> generated (and started transmitting) that much pack data.
>
> Ideally, we should be able to predict the approximate pack size _before_ we
> start generating and transmitting the pack data, and abort early if the
> estimated pack size exceeds the pack size limit.
>
> This patch tries to provide such an estimate: It looks at the objects that
> are to be included in the pack, and for already-packed objects, it assumes
> that their compressed in-pack size is a good estimate of how much they will
> contribute to the pack currently being generated. This assumption should be
> valid as long as the objects are reused as-is.

This looks good to me.

> I'm not really happy with excluding loose objects in the pack size
> estimate. However, the size contributed by loose objects varies wildly
> depending on whether a (good) delta is found. Therefore, any estimate
> done at an early stage is bound to be wildly inaccurate. We could maybe
> use some sort of absolute minimum size per object instead, but I
> thought I should publish this version before spending more time futzing
> with it...
>
> A drawback of not including loose objects in the pack size estimate,
> is that pushing loose objects is a very common use case (most people
> push more often than they 'git gc'). However, for the pack sizes that
> servers are most likely to refuse (hundreds of megabytes), most of
> those objects will probably already be packed anyway (e.g. by
> 'git gc --auto'), so I still hope the pack size estimate will be useful
> when it really matters.

That is my impression too. Most servers using this feature will
probably put a limit of at least 10MB. Once you get into the 25-100M
range, the client probably has already packed the bulk of that
content. Especially if we also have Junio's new stream large blobs to
packs during git add patch. So as you point out, cases where this is
mostly useful (really huge push) this is likely to still trigger
correctly.

We can still get a tighter estimate if we wanted to. I wouldn't mix it
into this patch, but make a new one on top of it. During delta
compression we hold onto deltas, or at least compute and retain the
size of the chosen delta. We could re-check the pack size after the
Compressing phase by including the delta sizes in the estimate, and if
we are over, abort before writing.

For non-delta, non-reuse we may be able to guess by just using the
loose object size. The loose object is most likely compressed at the
same compression ratio as the outgoing pack stream will use, so a
deflate(inflate(loose)) cycle is going to be very close in total bytes
used. If we over shoot the limit by more than some fudge factor (say
8K in 1M limit or 0.7%), abort before writing.

-- 
Shawn.

Re: [PATCHv4 09/10] pack-objects: Estimate pack size; abort early if pack size limit is exceeded

[Johan Herland]

On Monday 23. May 2011, Shawn Pearce wrote:
> We can still get a tighter estimate if we wanted to. I wouldn't mix
> it into this patch, but make a new one on top of it. During delta
> compression we hold onto deltas, or at least compute and retain the
> size of the chosen delta. We could re-check the pack size after the
> Compressing phase by including the delta sizes in the estimate, and
> if we are over, abort before writing.

Ok. Not sure when I'll have the time/courage to dive into this, but I'll 
at least give it a try.

> For non-delta, non-reuse we may be able to guess by just using the
> loose object size. The loose object is most likely compressed at the
> same compression ratio as the outgoing pack stream will use, so a
> deflate(inflate(loose)) cycle is going to be very close in total
> bytes used. If we over shoot the limit by more than some fudge
> factor (say 8K in 1M limit or 0.7%), abort before writing.

I already have an unsubmitted patch on top of the series that includes 
the on-disk/compressed size of loose objects in the estimate. However, 
it's quite intrusive (need to extend sha1_object_info() to return 
compressed size of loose objects). Also, since I don't yet take the 
delta compression into account, these numbers are obviously unreliable.

That said, in the cases where loose objects are not deltified it seems 
the compressed/loose versions are about 3 to 7 bytes larger than the 
corresponding compressed/packed versions. I guess this is due to the 
loose files using a "<type> SP <size> NUL" text header (deflated), 
whereas the pack uses a more compact binary format (not deflated).

We could test a large corpus (e.g. linux-kernel) to find the average 
difference between compressed/loose size and compressed/packed size, and 
then multiply this with the number of non-delta, non-reuse object to 
determine the fudge factor you describe above.


Have fun! :)

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCHv4 09/10] pack-objects: Estimate pack size; abort early if pack size limit is exceeded

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> I'm not really happy with excluding loose objects in the pack size
> estimate. However, the size contributed by loose objects varies wildly
> depending on whether a (good) delta is found. Therefore, any estimate
> done at an early stage is bound to be wildly inaccurate. We could maybe
> use some sort of absolute minimum size per object instead, but I
> thought I should publish this version before spending more time futzing
> with it...

As the initial approximation, I think this implementation is fine.
Continuing my comments on 7/10, "pack_to_stdout && pack_size_limit" part
may need to be replaced with "total_data_limit" or something, though.

After all it isn't fundamentally wrong to say "I don't want to pack more
than 2GB in total" even when you are producing packs on your local disk
without the --stdout option, no?

Re: [PATCHv4 09/10] pack-objects: Estimate pack size; abort early if pack size limit is exceeded

[Johan Herland]

On Tuesday 24 May 2011, Junio C Hamano wrote:
> Johan Herland <johan@herland.net> writes:
> > I'm not really happy with excluding loose objects in the pack size
> > estimate. However, the size contributed by loose objects varies wildly
> > depending on whether a (good) delta is found. Therefore, any estimate
> > done at an early stage is bound to be wildly inaccurate. We could maybe
> > use some sort of absolute minimum size per object instead, but I
> > thought I should publish this version before spending more time futzing
> > with it...
> 
> As the initial approximation, I think this implementation is fine.
> Continuing my comments on 7/10, "pack_to_stdout && pack_size_limit" part
> may need to be replaced with "total_data_limit" or something, though.
> 
> After all it isn't fundamentally wrong to say "I don't want to pack more
> than 2GB in total" even when you are producing packs on your local disk
> without the --stdout option, no?

Agreed. Will fix in the next iteration.


Thanks for reviewing the series! It is very much appreciated.

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

[PATCHv4 10/10] receive-pack: Allow server to refuse pushes with too many objects

[Johan Herland]

Add a new receive.objectCountLimit config variable which defines an upper
limit on the number of objects to accept in a single push. The server
aborts the transfer if the pack header received from the client indicates
a number of objects that exceeds this upper limit.

This limit is not advertised to clients, but is only enforced server-side.
When the limit is exceeded, the server sends a helpful error message to the
client, and then aborts the transfer, leaving the client with a broken pipe.

Server administrators might want to use this config variable to prevent
unintended large pushes from entering the repo (typically a result of the
user not being aware of exactly what is being pushed, e.g. pushing a large
rewritten history). Note that this config variable is not intended to protect
against DoS attacks, since there are countless other ways to attempt to DoS a
server without violating this limit.

Traditionally, this kind of limit would be imposed by a pre-receive or update
hook, but both of those run _after_ the pack has been received and stored by
receive-pack, so they cannot prevent the pack from being stored on the server.

Documentation and tests are included.

Signed-off-by: Johan Herland <johan@herland.net>
---
 Documentation/config.txt |    9 +++++++++
 builtin/receive-pack.c   |   11 +++++++++--
 t/t5400-send-pack.sh     |   44 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 62 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 79d553a..8618979 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1610,6 +1610,15 @@ receive.commitCountLimit::
 	there is no upper limit on the number of commits transferred
 	in a single push.
 
+receive.objectCountLimit::
+	If the number of objects received in a push exceeds this limit,
+	then the entire push will be refused. This is meant to prevent
+	an unintended large push (typically a result of the user not
+	being aware of exactly what is being pushed, e.g. pushing a
+	large rewritten history) from entering the repo. If not set,
+	there is no upper limit on the number of objects transferred
+	in a single push.
+
 receive.denyDeletes::
 	If set to true, git-receive-pack will deny a ref update that deletes
 	the ref. Use this to prevent such a ref deletion via a push.
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 49d29ad..e9e5521 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -28,7 +28,7 @@ static int receive_fsck_objects;
 static int receive_unpack_limit = -1;
 static int transfer_unpack_limit = -1;
 static int unpack_limit = 100;
-static unsigned long limit_pack_size, limit_commit_count;
+static unsigned long limit_pack_size, limit_commit_count, limit_object_count;
 static int report_status;
 static int use_sideband;
 static int prefer_ofs_delta = 1;
@@ -84,6 +84,11 @@ static int receive_pack_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (strcmp(var, "receive.objectcountlimit") == 0) {
+		limit_object_count = git_config_ulong(var, value);
+		return 0;
+	}
+
 	if (strcmp(var, "receive.fsckobjects") == 0) {
 		receive_fsck_objects = git_config_bool(var, value);
 		return 0;
@@ -673,7 +678,9 @@ static const char *unpack(void)
 			"--pack_header=%"PRIu32",%"PRIu32,
 			ntohl(hdr.hdr_version), ntohl(hdr.hdr_entries));
 
-	if (ntohl(hdr.hdr_entries) < unpack_limit) {
+	if (limit_object_count > 0 && ntohl(hdr.hdr_entries) > limit_object_count)
+		return "received pack exceeds configured receive.objectCountLimit";
+	else if (ntohl(hdr.hdr_entries) < unpack_limit) {
 		int code, i = 0;
 		const char *unpacker[4];
 		unpacker[i++] = "unpack-objects";
diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index 4ed5ba1..b65d69d 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh
@@ -349,4 +349,48 @@ test_expect_success 'push is allowed when pack size is not exceeded' '
 	test "$parent_head" = "$child_head"
 '
 
+test_expect_success 'deny pushing when receive.objectCountLimit is exceeded' '
+	rewound_push_setup &&
+	(
+	    cd parent &&
+	    git config receive.objectCountLimit 1
+	) &&
+	(
+	    cd child &&
+	    git reset --hard origin/master &&
+	    echo three > file && git commit -a -m three &&
+	    test_must_fail git send-pack ../parent master 2>errs &&
+	    grep -q "receive\\.objectCountLimit" errs
+	) &&
+	parent_head=$(cd parent && git rev-parse --verify master) &&
+	child_head=$(cd child && git rev-parse --verify master) &&
+	test "$parent_head" != "$child_head"
+'
+
+test_expect_success 'repeated push failure proves that objects were not stored remotely' '
+	(
+	    cd child &&
+	    test_must_fail git send-pack ../parent master 2>errs &&
+	    grep -q "receive\\.objectCountLimit" errs
+	) &&
+	parent_head=$(cd parent && git rev-parse --verify master) &&
+	child_head=$(cd child && git rev-parse --verify master) &&
+	test "$parent_head" != "$child_head"
+'
+
+test_expect_success 'push is allowed when object limit is increased' '
+	(
+	    cd parent &&
+	    git config receive.objectCountLimit 10
+	) &&
+	(
+	    cd child &&
+	    git send-pack ../parent master 2>errs &&
+	    test_must_fail grep -q "receive\\.objectCountLimit" errs
+	) &&
+	parent_head=$(cd parent && git rev-parse --verify master) &&
+	child_head=$(cd child && git rev-parse --verify master) &&
+	test "$parent_head" = "$child_head"
+'
+
 test_done
-- 
1.7.5.rc1.3.g4d7b