Re: [PATCHv3 3/9] pack-objects: Allow --max-pack-size to be used together with --stdout

From: Johan Herland <johan@herland.net>
Date: May 16, 2011, 09:27 UTC
Message-ID: <201105161127.30228.johan@herland.net>
In-Reply-To: <7vsjsfmawt.fsf@alter.siamese.dyndns.org>

On Monday 16 May 2011, Junio C Hamano wrote:

Show 30 quoted lines

> 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 only gave the code a cursory look, but I think your patch does more
> than the above paragraphs say. I am not sure those extra change are
> justified.
> 
> For example,
> 
> > @@ -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 */
> 
> Why does this conditional have to change its behaviour when writing to
> the standard output?  I thought that the only thing you are doing
> "earlier we didn't allow setting size limit when writing to standard
> output, now we do", and I do not see the linkage between that objective
> and this change.

AFAICS, the intention of the above "else if" block, is to enable usable_delta when there is no chance of a pack split happening. To establish that a pack split cannot happen, the code checks that pack_size_limit is disabled. Previously, pack_size_limit and pack_to_stdout was mutually exclusive (look at the last hunk in pack-objects.c). However, with this patch, it is possible to have both pack_size_limit and pack_to_stdout enabled.

Crucially, though, when both are enabled, a pack split is _still_ impossible, since a pack split while pack_to_stdout is enabled, forces pack-objects to abort altogether.

In other words, when pack_to_stdout is enabled, pack_size_limit is no longer a good indicator of whether a pack split might happen. Instead, pack_to_stdout being enabled is a good enough indicator in itself to guarantee that no pack split can happen (and hence that usable_delta should be enabled).

Show 15 quoted lines

> > @@ -2315,9 +2318,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;
> >  	}
> 
> Why is the new combination "writing to the standard output, but the
> maximum size is limited" does not have the same lower bound to pack size
> limit while on-disk packs do?

The reason for this change is purely to leave the server (receive-pack) in control of the pack size limit. If the server for any reason does specify a pack size limit less than 1 MiB, we do not want the client blindly ignoring that limit, and then submitting a pack that the server will reject.

If we _do_ want a hard lower bound on the pack size limit, we should apply that server-side (by setting 1 MiB as the minimum allowed value for receive.packSizeLimit). However, we will now have a problem if we in the future decide to change this lower bound for any reason whatsoever: We will need to change it in both receive-pack and pack-objects, and for as long as there are new clients talking to old servers (or vice versa if we decrease the lower bound), there will be clients submitting packs that are then rejected by the server.

I'd rather put the server in total control of the pack size limit.

> If you have a reason to believe 1 MiB is too large for a pack size limit,
> shouldn't that logic apply equally to the on-disk case?  What does this
> change have to do with the interaction with --stdout option?

I have no opinion on the lower bound of the pack size limit in the on-disk case.

In the above discussion, the usage of --stdout is synonymous with the send-pack scenario (pack-objects communicating with receive-pack). Although not true in general, it is true for this patch, since up until now pack-objects would abort if both --stdout and --max-pack-size were in use. Therefore, for the two changes discussed above:

 - else if (!pack_size_limit)
 + else if (!pack_size_limit || pack_to_stdout)

and

 - if (pack_size_limit && pack_size_limit < 1024*1024) {
 + if (!pack_to_stdout && pack_size_limit && pack_size_limit < 1024*1024) {

I can deduce that they only affect the current use case (the send-pack scenario), since these changes make no (functional) difference in any other use case (where --stdout and --max-pack-size are mutually exclusive).

...Johan

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