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

From: Junio C Hamano <gitster@pobox.com>
Date: May 16, 2011, 06:12 UTC
Message-ID: <7vsjsfmawt.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1305495440-30836-4-git-send-email-johan@herland.net>

Johan Herland <johan@herland.net> writes:

Show 8 quoted lines

> 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,

Show 7 quoted lines

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

Show 11 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?

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?