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

On Tuesday 24 May 2011, Junio C Hamano wrote:

Johan Herland [off-list ref] writes:

quoted

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.

quoted

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.

quoted

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).

quoted

@@ -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.

quoted

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.

quoted

@@ -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, [off-list ref]
www.herland.net