{"thread":{"id":"54605","subject":"[PATCH] archive: support compression levels beyond 9","startedAt":"2020-11-09T16:05:43Z","lastAt":"2020-11-10T11:37:33Z","messageCount":4,"participants":["René Scharfe","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"409397","messageId":"96e6e2ce-fc7b-1e73-0112-93589b28506d@web.de","threadId":"54605","inReplyTo":null,"subject":"[PATCH] archive: support compression levels beyond 9","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-11-09T16:05:31Z","receivedAt":"2020-11-09T16:05:43Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Compression programs like zip, gzip, bzip2 and xz allow to adjust the\ntrade-off between CPU cost and size gain with numerical options from -1\nfor fast compression and -9 for high compression ratio.  zip also\naccepts -0 for storing files verbatim.  git archive directly support\nthese single-digit compression levels for ZIP output and passes them to\nfilters like gzip.\n\nZstandard additionally supports compression level options -10 to -19, or\nup to -22 with --ultra.  This *seems* to work with git archive in most\ncases, e.g. it will produce an archive with -19 without complaining, but\nsince it only supports single-digit compression level options this is\nthe same as -1 -9 and thus -9.\n\nAllow git archive to accept multi-digit compression levels to support\nthe full range supported by zstd.  Explicitly reject them for the ZIP\nformat, as otherwise deflateInit2() would just fail with a somewhat\ncryptic \"stream consistency error\".\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n archive-tar.c |  3 ++-\n archive.c     | 26 +++++++++++---------------\n archive.h     |  1 +\n 3 files changed, 14 insertions(+), 16 deletions(-)\n\ndiff --git a/archive-tar.c b/archive-tar.c\nindex f1a1447ebd..a971fdc0f6 100644\n--- a/archive-tar.c\n+++ b/archive-tar.c\n@@ -374,7 +374,8 @@ static int tar_filter_config(const char *var, const char *value, void *data)\n \t\tar = xcalloc(1, sizeof(*ar));\n \t\tar->name = xmemdupz(name, namelen);\n \t\tar->write_archive = write_tar_filter_archive;\n-\t\tar->flags = ARCHIVER_WANT_COMPRESSION_LEVELS;\n+\t\tar->flags = ARCHIVER_WANT_COMPRESSION_LEVELS |\n+\t\t\t    ARCHIVER_HIGH_COMPRESSION_LEVELS;\n \t\tALLOC_GROW(tar_filters, nr_tar_filters + 1, alloc_tar_filters);\n \t\ttar_filters[nr_tar_filters++] = ar;\n \t}\ndiff --git a/archive.c b/archive.c\nindex 3c1541af9e..7a888c5338 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -529,10 +529,12 @@ static int add_file_cb(const struct option *opt, const char *arg, int unset)\n \treturn 0;\n }\n\n-#define OPT__COMPR(s, v, h, p) \\\n-\tOPT_SET_INT_F(s, NULL, v, h, p, PARSE_OPT_NONEG)\n-#define OPT__COMPR_HIDDEN(s, v, p) \\\n-\tOPT_SET_INT_F(s, NULL, v, \"\", p, PARSE_OPT_NONEG | PARSE_OPT_HIDDEN)\n+static int number_callback(const struct option *opt, const char *arg, int unset)\n+{\n+\tBUG_ON_OPT_NEG(unset);\n+\t*(int *)opt->value = strtol(arg, NULL, 10);\n+\treturn 0;\n+}\n\n static int parse_archive_args(int argc, const char **argv,\n \t\tconst struct archiver **ar, struct archiver_args *args,\n@@ -561,16 +563,8 @@ static int parse_archive_args(int argc, const char **argv,\n \t\tOPT_BOOL(0, \"worktree-attributes\", &worktree_attributes,\n \t\t\tN_(\"read .gitattributes in working directory\")),\n \t\tOPT__VERBOSE(&verbose, N_(\"report archived files on stderr\")),\n-\t\tOPT__COMPR('0', &compression_level, N_(\"store only\"), 0),\n-\t\tOPT__COMPR('1', &compression_level, N_(\"compress faster\"), 1),\n-\t\tOPT__COMPR_HIDDEN('2', &compression_level, 2),\n-\t\tOPT__COMPR_HIDDEN('3', &compression_level, 3),\n-\t\tOPT__COMPR_HIDDEN('4', &compression_level, 4),\n-\t\tOPT__COMPR_HIDDEN('5', &compression_level, 5),\n-\t\tOPT__COMPR_HIDDEN('6', &compression_level, 6),\n-\t\tOPT__COMPR_HIDDEN('7', &compression_level, 7),\n-\t\tOPT__COMPR_HIDDEN('8', &compression_level, 8),\n-\t\tOPT__COMPR('9', &compression_level, N_(\"compress better\"), 9),\n+\t\tOPT_NUMBER_CALLBACK(&compression_level,\n+\t\t\tN_(\"set compression level\"), number_callback),\n \t\tOPT_GROUP(\"\"),\n \t\tOPT_BOOL('l', \"list\", &list,\n \t\t\tN_(\"list supported archive formats\")),\n@@ -617,7 +611,9 @@ static int parse_archive_args(int argc, const char **argv,\n\n \targs->compression_level = Z_DEFAULT_COMPRESSION;\n \tif (compression_level != -1) {\n-\t\tif ((*ar)->flags & ARCHIVER_WANT_COMPRESSION_LEVELS)\n+\t\tint levels_ok = (*ar)->flags & ARCHIVER_WANT_COMPRESSION_LEVELS;\n+\t\tint high_ok = (*ar)->flags & ARCHIVER_HIGH_COMPRESSION_LEVELS;\n+\t\tif (levels_ok && (compression_level <= 9 || high_ok))\n \t\t\targs->compression_level = compression_level;\n \t\telse {\n \t\t\tdie(_(\"Argument not supported for format '%s': -%d\"),\ndiff --git a/archive.h b/archive.h\nindex 82b226011a..e3d04e8ab3 100644\n--- a/archive.h\n+++ b/archive.h\n@@ -36,6 +36,7 @@ const char *archive_format_from_filename(const char *filename);\n\n #define ARCHIVER_WANT_COMPRESSION_LEVELS 1\n #define ARCHIVER_REMOTE 2\n+#define ARCHIVER_HIGH_COMPRESSION_LEVELS 4\n struct archiver {\n \tconst char *name;\n \tint (*write_archive)(const struct archiver *, struct archiver_args *);\n--\n2.29.2\n"},{"id":"409403","messageId":"xmqq7dqucr9t.fsf@gitster.c.googlers.com","threadId":"54605","inReplyTo":"96e6e2ce-fc7b-1e73-0112-93589b28506d@web.de","subject":"Re: [PATCH] archive: support compression levels beyond 9","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-09T18:35:26Z","receivedAt":"2020-11-09T18:35:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Compression programs like zip, gzip, bzip2 and xz allow to adjust the\n> trade-off between CPU cost and size gain with numerical options from -1\n> for fast compression and -9 for high compression ratio.  zip also\n> accepts -0 for storing files verbatim.  git archive directly support\n> these single-digit compression levels for ZIP output and passes them to\n> filters like gzip.\n>\n> Zstandard additionally supports compression level options -10 to -19, or\n> up to -22 with --ultra.  This *seems* to work with git archive in most\n> cases, e.g. it will produce an archive with -19 without complaining, but\n> since it only supports single-digit compression level options this is\n> the same as -1 -9 and thus -9.\n>\n> Allow git archive to accept multi-digit compression levels to support\n> the full range supported by zstd.  Explicitly reject them for the ZIP\n> format, as otherwise deflateInit2() would just fail with a somewhat\n> cryptic \"stream consistency error\".\n\nThe implementation looks more like \"not enable them for the ZIP\nformat\", but the symptom observable to end-users is exactly\n\"explicitly reject\", so that's OK ;-)\n\nAs with the usual compression levels, this is only about how\ndeflator finds a better results, and the stream is understandable by\nany existing inflator, right?\n\n> diff --git a/archive.h b/archive.h\n> index 82b226011a..e3d04e8ab3 100644\n> --- a/archive.h\n> +++ b/archive.h\n> @@ -36,6 +36,7 @@ const char *archive_format_from_filename(const char *filename);\n>\n>  #define ARCHIVER_WANT_COMPRESSION_LEVELS 1\n>  #define ARCHIVER_REMOTE 2\n> +#define ARCHIVER_HIGH_COMPRESSION_LEVELS 4\n>  struct archiver {\n>  \tconst char *name;\n>  \tint (*write_archive)(const struct archiver *, struct archiver_args *);\n> diff --git a/archive-tar.c b/archive-tar.c\n> index f1a1447ebd..a971fdc0f6 100644\n> --- a/archive-tar.c\n> +++ b/archive-tar.c\n> @@ -374,7 +374,8 @@ static int tar_filter_config(const char *var, const char *value, void *data)\n>  \t\tar = xcalloc(1, sizeof(*ar));\n>  \t\tar->name = xmemdupz(name, namelen);\n>  \t\tar->write_archive = write_tar_filter_archive;\n> -\t\tar->flags = ARCHIVER_WANT_COMPRESSION_LEVELS;\n> +\t\tar->flags = ARCHIVER_WANT_COMPRESSION_LEVELS |\n> +\t\t\t    ARCHIVER_HIGH_COMPRESSION_LEVELS;\n\nNice.  \n\nHindsight tells me that WANT should have been ACCEPT, though---and\nan addition of ARCHIVER_ACCEPT_HIGH_COMPRESSION_LEVELS would be in\nline with that.  But that probably is too minor---it just stood out\na bit funny to me.\n\n> diff --git a/archive.c b/archive.c\n> index 3c1541af9e..7a888c5338 100644\n> --- a/archive.c\n> +++ b/archive.c\n> @@ -529,10 +529,12 @@ static int add_file_cb(const struct option *opt, const char *arg, int unset)\n>  \treturn 0;\n>  }\n>\n> -#define OPT__COMPR(s, v, h, p) \\\n> -\tOPT_SET_INT_F(s, NULL, v, h, p, PARSE_OPT_NONEG)\n> -#define OPT__COMPR_HIDDEN(s, v, p) \\\n> -\tOPT_SET_INT_F(s, NULL, v, \"\", p, PARSE_OPT_NONEG | PARSE_OPT_HIDDEN)\n> +static int number_callback(const struct option *opt, const char *arg, int unset)\n> +{\n> +\tBUG_ON_OPT_NEG(unset);\n> +\t*(int *)opt->value = strtol(arg, NULL, 10);\n> +\treturn 0;\n> +}\n>\n>  static int parse_archive_args(int argc, const char **argv,\n>  \t\tconst struct archiver **ar, struct archiver_args *args,\n> @@ -561,16 +563,8 @@ static int parse_archive_args(int argc, const char **argv,\n>  \t\tOPT_BOOL(0, \"worktree-attributes\", &worktree_attributes,\n>  \t\t\tN_(\"read .gitattributes in working directory\")),\n>  \t\tOPT__VERBOSE(&verbose, N_(\"report archived files on stderr\")),\n> -\t\tOPT__COMPR('0', &compression_level, N_(\"store only\"), 0),\n> -\t\tOPT__COMPR('1', &compression_level, N_(\"compress faster\"), 1),\n> -\t\tOPT__COMPR_HIDDEN('2', &compression_level, 2),\n> -\t\tOPT__COMPR_HIDDEN('3', &compression_level, 3),\n> -\t\tOPT__COMPR_HIDDEN('4', &compression_level, 4),\n> -\t\tOPT__COMPR_HIDDEN('5', &compression_level, 5),\n> -\t\tOPT__COMPR_HIDDEN('6', &compression_level, 6),\n> -\t\tOPT__COMPR_HIDDEN('7', &compression_level, 7),\n> -\t\tOPT__COMPR_HIDDEN('8', &compression_level, 8),\n> -\t\tOPT__COMPR('9', &compression_level, N_(\"compress better\"), 9),\n> +\t\tOPT_NUMBER_CALLBACK(&compression_level,\n> +\t\t\tN_(\"set compression level\"), number_callback),\n\nDoubly nice.  Adds a feature while removing lines.  \n\nDo we miss the description given in \"git archive -h\" though?\n\n    usage: git archive [<options>] <tree-ish> [<path>...]\n       or: git archive --list\n       ...\n        -v, --verbose         report archived files on stderr\n        -0                    store only\n        -1                    compress faster\n        -9                    compress better\n\n"},{"id":"409438","messageId":"9c35f3e7-e0ac-b68a-2807-ef1ef9ade54f@web.de","threadId":"54605","inReplyTo":"xmqq7dqucr9t.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] archive: support compression levels beyond 9","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-11-09T23:48:32Z","receivedAt":"2020-11-09T23:49:09Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 09.11.20 um 19:35 schrieb Junio C Hamano:\n> René Scharfe <l.s.r@web.de> writes:\n>\n>> Compression programs like zip, gzip, bzip2 and xz allow to adjust the\n>> trade-off between CPU cost and size gain with numerical options from -1\n>> for fast compression and -9 for high compression ratio.  zip also\n>> accepts -0 for storing files verbatim.  git archive directly support\n>> these single-digit compression levels for ZIP output and passes them to\n>> filters like gzip.\n>>\n>> Zstandard additionally supports compression level options -10 to -19, or\n>> up to -22 with --ultra.  This *seems* to work with git archive in most\n>> cases, e.g. it will produce an archive with -19 without complaining, but\n>> since it only supports single-digit compression level options this is\n>> the same as -1 -9 and thus -9.\n>>\n>> Allow git archive to accept multi-digit compression levels to support\n>> the full range supported by zstd.  Explicitly reject them for the ZIP\n>> format, as otherwise deflateInit2() would just fail with a somewhat\n>> cryptic \"stream consistency error\".\n>\n> The implementation looks more like \"not enable them for the ZIP\n> format\", but the symptom observable to end-users is exactly\n> \"explicitly reject\", so that's OK ;-)\n>\n> As with the usual compression levels, this is only about how\n> deflator finds a better results, and the stream is understandable by\n> any existing inflator, right?\n\nSupport for higher levels might have been added in later versions of\nZstandard -- https://github.com/facebook/zstd/blob/dev/CHANGELOG\nmentions \"Command line utility compatible with high compression levels\"\nfor v.0.4.0.  I'm not aware of other implementations  of the algorithm\nthan the original one from Facebook, so I don't know how compatible\nthey are.  It's not a problem we can solve in Git, though.\n\nSide note: Using Zstandard with git archive requires the config setting\ntar.tar.zst.command=zstd.\n\n>> diff --git a/archive.c b/archive.c\n>> index 3c1541af9e..7a888c5338 100644\n>> --- a/archive.c\n>> +++ b/archive.c\n>> @@ -529,10 +529,12 @@ static int add_file_cb(const struct option *opt, const char *arg, int unset)\n>>  \treturn 0;\n>>  }\n>>\n>> -#define OPT__COMPR(s, v, h, p) \\\n>> -\tOPT_SET_INT_F(s, NULL, v, h, p, PARSE_OPT_NONEG)\n>> -#define OPT__COMPR_HIDDEN(s, v, p) \\\n>> -\tOPT_SET_INT_F(s, NULL, v, \"\", p, PARSE_OPT_NONEG | PARSE_OPT_HIDDEN)\n>> +static int number_callback(const struct option *opt, const char *arg, int unset)\n>> +{\n>> +\tBUG_ON_OPT_NEG(unset);\n>> +\t*(int *)opt->value = strtol(arg, NULL, 10);\n>> +\treturn 0;\n>> +}\n>>\n>>  static int parse_archive_args(int argc, const char **argv,\n>>  \t\tconst struct archiver **ar, struct archiver_args *args,\n>> @@ -561,16 +563,8 @@ static int parse_archive_args(int argc, const char **argv,\n>>  \t\tOPT_BOOL(0, \"worktree-attributes\", &worktree_attributes,\n>>  \t\t\tN_(\"read .gitattributes in working directory\")),\n>>  \t\tOPT__VERBOSE(&verbose, N_(\"report archived files on stderr\")),\n>> -\t\tOPT__COMPR('0', &compression_level, N_(\"store only\"), 0),\n>> -\t\tOPT__COMPR('1', &compression_level, N_(\"compress faster\"), 1),\n>> -\t\tOPT__COMPR_HIDDEN('2', &compression_level, 2),\n>> -\t\tOPT__COMPR_HIDDEN('3', &compression_level, 3),\n>> -\t\tOPT__COMPR_HIDDEN('4', &compression_level, 4),\n>> -\t\tOPT__COMPR_HIDDEN('5', &compression_level, 5),\n>> -\t\tOPT__COMPR_HIDDEN('6', &compression_level, 6),\n>> -\t\tOPT__COMPR_HIDDEN('7', &compression_level, 7),\n>> -\t\tOPT__COMPR_HIDDEN('8', &compression_level, 8),\n>> -\t\tOPT__COMPR('9', &compression_level, N_(\"compress better\"), 9),\n>> +\t\tOPT_NUMBER_CALLBACK(&compression_level,\n>> +\t\t\tN_(\"set compression level\"), number_callback),\n>\n> Doubly nice.  Adds a feature while removing lines.\n>\n> Do we miss the description given in \"git archive -h\" though?\n>\n>     usage: git archive [<options>] <tree-ish> [<path>...]\n>        or: git archive --list\n>        ...\n>         -v, --verbose         report archived files on stderr\n>         -0                    store only\n>         -1                    compress faster\n>         -9                    compress better\n>\n\nPerhaps; I just couldn't cram it all into a single line.  Showing an\nacceptable range would be nice and terse, but that depends on the\ncompressor.\n\nHmm, adding an option for passing arbitrary options to the filter and\nremoving the feature flag ARCHIVER_WANT_COMPRESSION_LEVELS from\narchive-tar.c would be cleaner overall.  The latter would be a\nregression, though.\n\nRené\n"},{"id":"409472","messageId":"1104ab8a-2353-c074-3d81-b31ff75287bb@web.de","threadId":"54605","inReplyTo":"9c35f3e7-e0ac-b68a-2807-ef1ef9ade54f@web.de","subject":"Re: [PATCH] archive: support compression levels beyond 9","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-11-10T11:37:22Z","receivedAt":"2020-11-10T11:37:33Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 10.11.20 um 00:48 schrieb René Scharfe:\n> Hmm, adding an option for passing arbitrary options to the filter and\n> removing the feature flag ARCHIVER_WANT_COMPRESSION_LEVELS from\n> archive-tar.c would be cleaner overall.  The latter would be a\n> regression, though.\n\nAnd the former can allow remote code execution with --remote if we're\nnot careful, since filters are called via the shell.\n\nRené\n"}]}