Re: [PATCH v2] Custom compression levels for objects and packs

From: Junio C Hamano <junkio@cox.net>
Date: May 8, 2007, 23:56 UTC
Message-ID: <7vk5vi27ko.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <4640FBDE.1000609@gmail.com>

Dana How <danahow@gmail.com> writes:

Show 9 quoted lines

> Add config variables pack.compression and core.loosecompression .
> Loose objects will be compressed using level
>   isset(core.loosecompression) ? core.loosecompression :
>   isset(core.compression) ? core.compression : Z_BEST_SPEED
> and objects in packs will be compressed using level
>   isset(pack.compression) ? pack.compression :
>   isset(core.compression) ? core.compression : Z_DEFAULT_COMPRESSION
> pack-objects also accepts --compression=N which
> overrides the latter expression.

Do you think the above is readable?

  Compression level for loose objects is controlled by variable
  core.loosecompression (or core.compression, if the former is
  missing), and defaults to best-speed.

or something like that?

> This applies on top of the git-repack --max-pack-size patchset.

Hmph, that makes the --max-pack-size patchset take this more trivial and straightforward improvements hostage. In general, I'd prefer more elaborate ones based on less questionable series.

Show 8 quoted lines

> @@ -444,6 +446,10 @@ static unsigned long write_object(struct sha1file *f,
>  				 * and we do not need to deltify it.
>  				 */
>  
> +	/* differing core & pack compression when loose object -> must recompress */
> +	if (!entry->in_pack && pack_compression_level != zlib_compression_level)
> +		to_reuse = 0;
> +	else

I am not sure if that is worth it, as you do not know if the loose object you are looking at were compressed with the current settings.

Show 10 quoted lines

> diff --git a/cache.h b/cache.h
> index 8e76152..2b3f359 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -283,6 +283,8 @@ extern int warn_ambiguous_refs;
>  extern int shared_repository;
>  extern const char *apply_default_whitespace;
>  extern int zlib_compression_level;
> +extern int core_compression_level;
> +extern int core_compression_seen;

Could we somehow remove _seen? Perhaps by initializing the _level to -1?

> +int core_compression_level;
> +int core_compression_seen;

Same here.