On Tuesday 24 May 2011, Junio C Hamano wrote:
> Johan Herland <johan@xxxxxxxxxxx> 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@xxxxxxxxxxx>
www.herland.net
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html