{"thread":{"id":"42622","subject":"[PATCH 0/2] friendlier handling of overflows in archive-tar","startedAt":"2016-06-16T06:55:53Z","lastAt":"2016-06-24T23:22:17Z","messageCount":61,"participants":["Jeff King","Junio C Hamano","René Scharfe","Robin H. Johnson","Remi Galan Alfonso","Johannes Sixt","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"289330","messageId":"20160616043523.GA13615@sigill.intra.peff.net","threadId":"42622","inReplyTo":null,"subject":"[PATCH 0/2] friendlier handling of overflows in archive-tar","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-16T04:35:23Z","receivedAt":"2016-06-16T06:55:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The ustar format has some fixed-length numeric fields, and it's possible\nto generate a git tree that can't be represented (namely file size and\nmtime). Since f2f0267 (archive-tar: use xsnprintf for trivial\nformatting, 2015-09-24), we detect and die() in these cases. But we can\nactually do the friendly (and POSIX-approved) thing, and add extended\npax headers to represent the correct values.\n\n [1/2]: archive-tar: write extended headers for file sizes >= 8GB\n [2/2]: archive-tar: write extended headers for far-future mtime\n\n-Peff\n"},{"id":"289331","messageId":"20160616043733.GA18323@sigill.intra.peff.net","threadId":"42622","inReplyTo":"20160616043523.GA13615@sigill.intra.peff.net","subject":"[PATCH 1/2] archive-tar: write extended headers for file sizes >= 8GB","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-16T04:37:33Z","receivedAt":"2016-06-16T06:55:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The ustar format has a fixed-length field for the size of\neach file entry which is supposed to contain up to 11 bytes\nof octal-formatted data plus a NUL or space terminator.\n\nThese means that the largest size we can represent is\n077777777777, or 1 byte short of 8GB. The correct solution\nfor a larger file, according to POSIX.1-2001, is to add an\nextended pax header, similar to how we handle long\nfilenames. This patch does that, and writes zero for the\nsize field in the ustar header (the last bit is not\nmentioned by POSIX, but it matches how GNU tar behaves with\n--format=pax).\n\nThis should be a strict improvement over the current\nbehavior, which is to die in xsnprintf with a \"BUG\".\nHowever, there's some interesting history here.\n\nPrior to f2f0267 (archive-tar: use xsnprintf for trivial\nformatting, 2015-09-24), we silently overflowed the \"size\"\nfield. The extra bytes ended up in the \"mtime\" field of the\nheader, which was then immediately written itself,\noverwriting our extra bytes. What that means depends on how\nmany bytes we wrote.\n\nIf the size was 64GB or greater, then we actually overflowed\ndigits into the mtime field, meaning our value was was\neffectively right-shifted by those lost octal digits. And\nthis patch is again a strict improvement over that.\n\nBut if the size was between 8GB and 64GB, then our 12-byte\nfield held all of the actual digits, and only our NUL\nterminator overflowed. According to POSIX, there should be a\nNUL or space at the end of the field. However, GNU tar seems\nto be lenient here, and will correctly parse a size up 64GB\n(minus one) from the field. So sizes in this range might\nhave just worked, depending on the implementation reading\nthe tarfile.\n\nThis patch is mostly still an improvement there, as the 8GB\nlimit is specifically mentioned in POSIX as the correct\nlimit. But it's possible that it could be a regression\n(versus the pre-f2f0267 state) if all of the following are\ntrue:\n\n 1. You have a file between 8GB and 64GB.\n\n 2. Your tar implementation _doesn't_ know about pax\n extended headers.\n\n 3. Your tar implementation _does_ parse 12-byte sizes from\n the ustar header without a delimiter.\n\nIt's probably not worth worrying about such an obscure set\nof conditions, but I'm documenting it here just in case.\n\nThere's no test included here. I did confirm that this works\n(using GNU tar) with:\n\n $ dd if=/dev/zero seek=64G bs=1 count=1 of=huge\n $ git add huge\n $ git commit -q -m foo\n $ git archive HEAD | head -c 10000 | tar tvf - 2>/dev/null\n -rw-rw-r-- root/root 68719476737 2016-06-15 21:07 huge\n\nPre-f2f0267, this would yield a bogus size of 8GB, and\npost-f2f0267, git-archive simply dies.\n\nUnfortunately, it's quite an expensive test to run. For one\nthing, unless your filesystem supports files with holes, it\ntakes 64GB of disk space (you might think piping straight to\n`hash-object --stdin` would be better, but it's not; that\ntries to buffer all 64GB in RAM!). Furthermore, hashing and\ncompressing the object takes several minutes of CPU time.\n\nWe could ship just the resulting compressed object data as a\nloose object, but even that takes 64MB. So sadly, this code\npath remains untested in the test suite.\n\nSigned-off-by: Jeff King \n---\n archive-tar.c | 28 +++++++++++++++++++++++++++-\n 1 file changed, 27 insertions(+), 1 deletion(-)\n\ndiff --git a/archive-tar.c b/archive-tar.c\nindex cb99df2..7340b64 100644\n--- a/archive-tar.c\n+++ b/archive-tar.c\n@@ -137,6 +137,20 @@ static void strbuf_append_ext_header(struct strbuf *sb, const char *keyword,\n \tstrbuf_addch(sb, '\\n');\n }\n \n+/*\n+ * Like strbuf_append_ext_header, but for numeric values.\n+ */\n+static void strbuf_append_ext_header_uint(struct strbuf *sb,\n+\t\t\t\t\t const char *keyword,\n+\t\t\t\t\t uintmax_t value)\n+{\n+\tchar buf[40]; /* big enough for 2^128 in decimal, plus NUL */\n+\tint len;\n+\n+\tlen = xsnprintf(buf, sizeof(buf), \"%\"PRIuMAX, value);\n+\tstrbuf_append_ext_header(sb, keyword, buf, len);\n+}\n+\n static unsigned int ustar_header_chksum(const struct ustar_header *header)\n {\n \tconst unsigned char *p = (const unsigned char *)header;\n@@ -163,12 +177,21 @@ static size_t get_path_prefix(const char *path, size_t pathlen, size_t maxlen)\n \treturn i;\n }\n \n+static inline unsigned long ustar_size(uintmax_t size)\n+{\n+\tif (size < 077777777777UL)\n+\t\treturn size;\n+\telse\n+\t\treturn 0;\n+}\n+\n static void prepare_header(struct archiver_args *args,\n \t\t\t struct ustar_header *header,\n \t\t\t unsigned int mode, unsigned long size)\n {\n \txsnprintf(header->mode, sizeof(header->mode), \"%07o\", mode & 07777);\n-\txsnprintf(header->size, sizeof(header->size), \"%011lo\", S_ISREG(mode) ? size : 0);\n+\txsnprintf(header->size, sizeof(header->size), \"%011lo\",\n+\t\t S_ISREG(mode) ? ustar_size(size) : 0);\n \txsnprintf(header->mtime, sizeof(header->mtime), \"%011lo\", (unsigned long) args->time);\n \n \txsnprintf(header->uid, sizeof(header->uid), \"%07o\", 0);\n@@ -267,6 +290,9 @@ static int write_tar_entry(struct archiver_args *args,\n \t\t\tmemcpy(header.linkname, buffer, size);\n \t}\n \n+\tif (ustar_size(size) != size)\n+\t\tstrbuf_append_ext_header_uint(&ext_header, \"size\", size);\n+\n \tprepare_header(args, &header, mode, size);\n \n \tif (ext_header.len > 0) {\n-- \n2.9.0.150.g8bd4cf6\n"},{"id":"289332","messageId":"20160616043758.GB18323@sigill.intra.peff.net","threadId":"42622","inReplyTo":"20160616043523.GA13615@sigill.intra.peff.net","subject":"[PATCH 2/2] archive-tar: write extended headers for far-future mtime","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-16T04:37:58Z","receivedAt":"2016-06-16T06:55:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The ustar format represents timestamps as seconds since the\nepoch, but only has room to store 11 octal digits. To\nexpress anything larger, we need to use an extended header.\nThis is exactly the same case we fixed for the size field in\nthe previous commit, and the solution here follows the same\npattern.\n\nThis is even mentioned as an issue in f2f0267 (archive-tar:\nuse xsnprintf for trivial formatting, 2015-09-24), but since\nit only affected things far in the future, it wasn't worth\ndealing with. But note that my calculations claiming\nthousands of years were off there; because our xsnprintf\nproduces a NUL byte, we only have until the year 2242 to\nfix this.\n\nGiven that this is just around the corner (geologically\nspeaking), and because the fix for \"size\" makes doing this\non top trivial, let's just make it work.\n\nNote that we don't (and never did) handle negative\ntimestamps (i.e., before 1970). This would probably not be\ntoo hard to support in the same way, but since git does not\nsupport negative timestamps at all, I didn't bother here.\n\nUnlike the last patch for \"size\", this one is easy to test\nefficiently (the magic date is octal 01000000000001):\n\n $ echo content >file\n $ git add file\n $ GIT_COMMITTER_DATE='@68719476737 +0000' \\\n git commit -q -m 'tempori parendum'\n $ git archive HEAD | tar tvf -\n -rw-rw-r-- root/root 8 4147-08-20 03:32 file\n\nWith the current tip of master, this dies in xsnprintf (and\nprior to f2f0267, it overflowed into the checksum field, and\nthe resulting tarfile claimed to be from the year 2242).\n\nHowever, I decided not to include this in the test suite,\nbecause I suspect it will create portability headaches,\nincluding:\n\n 1. The exact format of the system tar's \"t\" output.\n\n 2. System tars that cannot handle pax headers.\n\n 3. System tars tha cannot handle dates that far in the\n future.\n\n 4. If we replace \"tar t\" with \"tar x\", then filesystems\n that cannot handle dates that far in the future.\n\nIn other words, we do not really have a reliable tar reader\nfor these corner cases, so the best we could do is a\nbyte-for-byte comparison of the output.\n\nSigned-off-by: Jeff King \n---\n archive-tar.c | 13 ++++++++++++-\n 1 file changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/archive-tar.c b/archive-tar.c\nindex 7340b64..749722f 100644\n--- a/archive-tar.c\n+++ b/archive-tar.c\n@@ -185,6 +185,14 @@ static inline unsigned long ustar_size(uintmax_t size)\n \t\treturn 0;\n }\n \n+static inline unsigned long ustar_mtime(time_t mtime)\n+{\n+\tif (mtime < 077777777777)\n+\t\treturn mtime;\n+\telse\n+\t\treturn 0;\n+}\n+\n static void prepare_header(struct archiver_args *args,\n \t\t\t struct ustar_header *header,\n \t\t\t unsigned int mode, unsigned long size)\n@@ -192,7 +200,8 @@ static void prepare_header(struct archiver_args *args,\n \txsnprintf(header->mode, sizeof(header->mode), \"%07o\", mode & 07777);\n \txsnprintf(header->size, sizeof(header->size), \"%011lo\",\n \t\t S_ISREG(mode) ? ustar_size(size) : 0);\n-\txsnprintf(header->mtime, sizeof(header->mtime), \"%011lo\", (unsigned long) args->time);\n+\txsnprintf(header->mtime, sizeof(header->mtime), \"%011lo\",\n+\t\t ustar_mtime(args->time));\n \n \txsnprintf(header->uid, sizeof(header->uid), \"%07o\", 0);\n \txsnprintf(header->gid, sizeof(header->gid), \"%07o\", 0);\n@@ -292,6 +301,8 @@ static int write_tar_entry(struct archiver_args *args,\n \n \tif (ustar_size(size) != size)\n \t\tstrbuf_append_ext_header_uint(&ext_header, \"size\", size);\n+\tif (ustar_mtime(args->time) != args->time)\n+\t\tstrbuf_append_ext_header_uint(&ext_header, \"mtime\", args->time);\n \n \tprepare_header(args, &header, mode, size);\n \n-- \n2.9.0.150.g8bd4cf6\n"},{"id":"289375","messageId":"xmqqfusd588n.fsf@gitster.mtv.corp.google.com","threadId":"42622","inReplyTo":"20160616043523.GA13615@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] friendlier handling of overflows in archive-tar","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-16T17:55:20Z","receivedAt":"2016-06-16T17:55:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Both patches looked good to me. Thanks.\n"},{"id":"289716","messageId":"57687417.4020009@web.de","threadId":"42622","inReplyTo":"20160616043758.GB18323@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] archive-tar: write extended headers for far-future mtime","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-06-20T22:54:15Z","receivedAt":"2016-06-20T22:54:53Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 16.06.2016 um 06:37 schrieb Jeff King:\n> The ustar format represents timestamps as seconds since the\n> epoch, but only has room to store 11 octal digits. To\n> express anything larger, we need to use an extended header.\n> This is exactly the same case we fixed for the size field in\n> the previous commit, and the solution here follows the same\n> pattern.\n>\n> This is even mentioned as an issue in f2f0267 (archive-tar:\n> use xsnprintf for trivial formatting, 2015-09-24), but since\n> it only affected things far in the future, it wasn't worth\n> dealing with. But note that my calculations claiming\n> thousands of years were off there; because our xsnprintf\n> produces a NUL byte, we only have until the year 2242 to\n> fix this.\n>\n> Given that this is just around the corner (geologically\n> speaking), and because the fix for \"size\" makes doing this\n> on top trivial, let's just make it work.\n>\n> Note that we don't (and never did) handle negative\n> timestamps (i.e., before 1970). This would probably not be\n> too hard to support in the same way, but since git does not\n> support negative timestamps at all, I didn't bother here.\n>\n> Unlike the last patch for \"size\", this one is easy to test\n> efficiently (the magic date is octal 01000000000001):\n>\n> $ echo content >file\n> $ git add file\n> $ GIT_COMMITTER_DATE='@68719476737 +0000' \\\n> git commit -q -m 'tempori parendum'\n> $ git archive HEAD | tar tvf -\n> -rw-rw-r-- root/root 8 4147-08-20 03:32 file\n>\n> With the current tip of master, this dies in xsnprintf (and\n> prior to f2f0267, it overflowed into the checksum field, and\n> the resulting tarfile claimed to be from the year 2242).\n>\n> However, I decided not to include this in the test suite,\n> because I suspect it will create portability headaches,\n> including:\n>\n> 1. The exact format of the system tar's \"t\" output.\n\nProbably not worth it.\n\n> 2. System tars that cannot handle pax headers.\n\nIn t5000 there is a simple interpreter for path headers for systems \nwhose tar doesn't handle them. Adding one for mtime headers may be \nfeasible.\n\nIt's just a bit complicated to link a pax header file to the file it \napplies to when it doesn't also contain a path header. But we know that \nthe mtime of all entries in tar files created by git archive are is the \nsame, so we could simply read one header and then adjust the mtime of \nall files accordingly.\n\n> 3. System tars tha cannot handle dates that far in the\n> future.\n>\n> 4. If we replace \"tar t\" with \"tar x\", then filesystems\n> that cannot handle dates that far in the future.\n\nWe can test that by supplying a test archive and set a prerequisite if \ntar (possibly with our header interpreter) and the filesystem can cope \nwith that.\n\n> In other words, we do not really have a reliable tar reader\n> for these corner cases, so the best we could do is a\n> byte-for-byte comparison of the output.\n>\n> Signed-off-by: Jeff King \n> ---\n> archive-tar.c | 13 ++++++++++++-\n> 1 file changed, 12 insertions(+), 1 deletion(-)\n>\n> diff --git a/archive-tar.c b/archive-tar.c\n> index 7340b64..749722f 100644\n> --- a/archive-tar.c\n> +++ b/archive-tar.c\n> @@ -185,6 +185,14 @@ static inline unsigned long ustar_size(uintmax_t size)\n> \t\treturn 0;\n> }\n>\n> +static inline unsigned long ustar_mtime(time_t mtime)\n> +{\n> +\tif (mtime < 077777777777)\n\nThat should be less-or-equal, right?\n\n> +\t\treturn mtime;\n> +\telse\n> +\t\treturn 0;\n> +}\n> +\n> static void prepare_header(struct archiver_args *args,\n> \t\t\t struct ustar_header *header,\n> \t\t\t unsigned int mode, unsigned long size)\n> @@ -192,7 +200,8 @@ static void prepare_header(struct archiver_args *args,\n> \txsnprintf(header->mode, sizeof(header->mode), \"%07o\", mode & 07777);\n> \txsnprintf(header->size, sizeof(header->size), \"%011lo\",\n> \t\t S_ISREG(mode) ? ustar_size(size) : 0);\n> -\txsnprintf(header->mtime, sizeof(header->mtime), \"%011lo\", (unsigned long) args->time);\n> +\txsnprintf(header->mtime, sizeof(header->mtime), \"%011lo\",\n> +\t\t ustar_mtime(args->time));\n>\n> \txsnprintf(header->uid, sizeof(header->uid), \"%07o\", 0);\n> \txsnprintf(header->gid, sizeof(header->gid), \"%07o\", 0);\n> @@ -292,6 +301,8 @@ static int write_tar_entry(struct archiver_args *args,\n>\n> \tif (ustar_size(size) != size)\n> \t\tstrbuf_append_ext_header_uint(&ext_header, \"size\", size);\n> +\tif (ustar_mtime(args->time) != args->time)\n> +\t\tstrbuf_append_ext_header_uint(&ext_header, \"mtime\", args->time);\n>\n> \tprepare_header(args, &header, mode, size);\n>\n>\n"},{"id":"289717","messageId":"57687413.3030609@web.de","threadId":"42622","inReplyTo":"20160616043733.GA18323@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] archive-tar: write extended headers for file sizes >= 8GB","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-06-20T22:54:11Z","receivedAt":"2016-06-20T22:54:55Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 16.06.2016 um 06:37 schrieb Jeff King:\n> The ustar format has a fixed-length field for the size of\n> each file entry which is supposed to contain up to 11 bytes\n> of octal-formatted data plus a NUL or space terminator.\n>\n> These means that the largest size we can represent is\n> 077777777777, or 1 byte short of 8GB. The correct solution\n> for a larger file, according to POSIX.1-2001, is to add an\n> extended pax header, similar to how we handle long\n> filenames. This patch does that, and writes zero for the\n> size field in the ustar header (the last bit is not\n> mentioned by POSIX, but it matches how GNU tar behaves with\n> --format=pax).\n>\n> This should be a strict improvement over the current\n> behavior, which is to die in xsnprintf with a \"BUG\".\n> However, there's some interesting history here.\n>\n> Prior to f2f0267 (archive-tar: use xsnprintf for trivial\n> formatting, 2015-09-24), we silently overflowed the \"size\"\n> field. The extra bytes ended up in the \"mtime\" field of the\n> header, which was then immediately written itself,\n> overwriting our extra bytes. What that means depends on how\n> many bytes we wrote.\n>\n> If the size was 64GB or greater, then we actually overflowed\n> digits into the mtime field, meaning our value was was\n> effectively right-shifted by those lost octal digits. And\n> this patch is again a strict improvement over that.\n>\n> But if the size was between 8GB and 64GB, then our 12-byte\n> field held all of the actual digits, and only our NUL\n> terminator overflowed. According to POSIX, there should be a\n> NUL or space at the end of the field. However, GNU tar seems\n> to be lenient here, and will correctly parse a size up 64GB\n> (minus one) from the field. So sizes in this range might\n> have just worked, depending on the implementation reading\n> the tarfile.\n>\n> This patch is mostly still an improvement there, as the 8GB\n> limit is specifically mentioned in POSIX as the correct\n> limit. But it's possible that it could be a regression\n> (versus the pre-f2f0267 state) if all of the following are\n> true:\n>\n> 1. You have a file between 8GB and 64GB.\n>\n> 2. Your tar implementation _doesn't_ know about pax\n> extended headers.\n>\n> 3. Your tar implementation _does_ parse 12-byte sizes from\n> the ustar header without a delimiter.\n>\n> It's probably not worth worrying about such an obscure set\n> of conditions, but I'm documenting it here just in case.\n>\n> There's no test included here. I did confirm that this works\n> (using GNU tar) with:\n>\n> $ dd if=/dev/zero seek=64G bs=1 count=1 of=huge\n> $ git add huge\n> $ git commit -q -m foo\n> $ git archive HEAD | head -c 10000 | tar tvf - 2>/dev/null\n> -rw-rw-r-- root/root 68719476737 2016-06-15 21:07 huge\n>\n> Pre-f2f0267, this would yield a bogus size of 8GB, and\n> post-f2f0267, git-archive simply dies.\n>\n> Unfortunately, it's quite an expensive test to run. For one\n> thing, unless your filesystem supports files with holes, it\n> takes 64GB of disk space (you might think piping straight to\n> `hash-object --stdin` would be better, but it's not; that\n> tries to buffer all 64GB in RAM!). Furthermore, hashing and\n> compressing the object takes several minutes of CPU time.\n>\n> We could ship just the resulting compressed object data as a\n> loose object, but even that takes 64MB. So sadly, this code\n> path remains untested in the test suite.\n\nIf we could set the limit to a lower value than 8GB for testing then we \ncould at least check if the extended header is written, e.g. if \nustar_size() could be convinced to return 0 every time using a hidden \ncommand line parameter or an environment variable or something better.\n\n>\n> Signed-off-by: Jeff King \n> ---\n> archive-tar.c | 28 +++++++++++++++++++++++++++-\n> 1 file changed, 27 insertions(+), 1 deletion(-)\n>\n> diff --git a/archive-tar.c b/archive-tar.c\n> index cb99df2..7340b64 100644\n> --- a/archive-tar.c\n> +++ b/archive-tar.c\n> @@ -137,6 +137,20 @@ static void strbuf_append_ext_header(struct strbuf *sb, const char *keyword,\n> \tstrbuf_addch(sb, '\\n');\n> }\n>\n> +/*\n> + * Like strbuf_append_ext_header, but for numeric values.\n> + */\n> +static void strbuf_append_ext_header_uint(struct strbuf *sb,\n> +\t\t\t\t\t const char *keyword,\n> +\t\t\t\t\t uintmax_t value)\n> +{\n> +\tchar buf[40]; /* big enough for 2^128 in decimal, plus NUL */\n> +\tint len;\n> +\n> +\tlen = xsnprintf(buf, sizeof(buf), \"%\"PRIuMAX, value);\n> +\tstrbuf_append_ext_header(sb, keyword, buf, len);\n> +}\n> +\n> static unsigned int ustar_header_chksum(const struct ustar_header *header)\n> {\n> \tconst unsigned char *p = (const unsigned char *)header;\n> @@ -163,12 +177,21 @@ static size_t get_path_prefix(const char *path, size_t pathlen, size_t maxlen)\n> \treturn i;\n> }\n>\n> +static inline unsigned long ustar_size(uintmax_t size)\n> +{\n> +\tif (size < 077777777777UL)\n\nShouldn't that be less-or-equal?\n\n> +\t\treturn size;\n> +\telse\n> +\t\treturn 0;\n> +}\n> +\n> static void prepare_header(struct archiver_args *args,\n> \t\t\t struct ustar_header *header,\n> \t\t\t unsigned int mode, unsigned long size)\n> {\n> \txsnprintf(header->mode, sizeof(header->mode), \"%07o\", mode & 07777);\n> -\txsnprintf(header->size, sizeof(header->size), \"%011lo\", S_ISREG(mode) ? size : 0);\n> +\txsnprintf(header->size, sizeof(header->size), \"%011lo\",\n> +\t\t S_ISREG(mode) ? ustar_size(size) : 0);\n> \txsnprintf(header->mtime, sizeof(header->mtime), \"%011lo\", (unsigned long) args->time);\n>\n> \txsnprintf(header->uid, sizeof(header->uid), \"%07o\", 0);\n> @@ -267,6 +290,9 @@ static int write_tar_entry(struct archiver_args *args,\n> \t\t\tmemcpy(header.linkname, buffer, size);\n> \t}\n>\n> +\tif (ustar_size(size) != size)\n> +\t\tstrbuf_append_ext_header_uint(&ext_header, \"size\", size);\n\nIt needs \"S_ISREG(mode) && \" as well, no? In practice it probably \ndoesn't matter (until someone stores a 8GB long symlink target), but the \nsize field should only be set for regular files.\n\n> +\n> \tprepare_header(args, &header, mode, size);\n>\n> \tif (ext_header.len > 0) {\n>\n"},{"id":"289774","messageId":"20160621160249.GA9645@sigill.intra.peff.net","threadId":"42622","inReplyTo":"20160621155920.GA7549@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] archive-tar: write extended headers for file sizes >= 8GB","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-21T16:02:49Z","receivedAt":"2016-06-21T16:09:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 21, 2016 at 11:59:20AM -0400, Jeff King wrote:\n\n> > > +\tif (ustar_size(size) != size)\n> > > +\t\tstrbuf_append_ext_header_uint(&ext_header, \"size\", size);\n> > \n> > It needs \"S_ISREG(mode) && \" as well, no? In practice it probably doesn't\n> > matter (until someone stores a 8GB long symlink target), but the size field\n> > should only be set for regular files.\n> \n> Thanks for noticing that. I remembered wondering that when I was early\n> in debugging/diagnosing, but forgot to follow up on it. I agree it's\n> unlikely in practice, but we should have consistent checks (I think it\n> would actually make sense to move the ISREG check inside ustar_size, and\n> then we can apply it consistently here and when generating the header;\n> my goal with ustar_size() was to avoid having the same logic in multiple\n> places).\n\nActually, scratch that. It makes things awkward because it would be hard\nto tell when ustar_size() returned zero because it's a file with a big\nsize, and thus needs a pax header, versus that it was not a file, and\ntherefore must _not_ have a pax header.\n\n-Peff\n"},{"id":"289775","messageId":"20160621161604.GA17638@sigill.intra.peff.net","threadId":"42622","inReplyTo":"20160616043523.GA13615@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] friendlier handling of overflows in archive-tar","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-21T16:16:04Z","receivedAt":"2016-06-21T16:16:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 16, 2016 at 12:35:23AM -0400, Jeff King wrote:\n\n> The ustar format has some fixed-length numeric fields, and it's possible\n> to generate a git tree that can't be represented (namely file size and\n> mtime). Since f2f0267 (archive-tar: use xsnprintf for trivial\n> formatting, 2015-09-24), we detect and die() in these cases. But we can\n> actually do the friendly (and POSIX-approved) thing, and add extended\n> pax headers to represent the correct values.\n> \n> [1/2]: archive-tar: write extended headers for file sizes >= 8GB\n> [2/2]: archive-tar: write extended headers for far-future mtime\n\nAnd here's a v2 that addresses the smaller comments from René. I punted\non doing something fancy with tests. I'm not opposed to it, but I'm also\nnot convinced it's all that useful. Either way, I think it can come on\ntop if we want it.\n\nJunio, this is the jk/big-and-old-archive-tar topic.\n\nThe interdiff is:\n\ndiff --git a/archive-tar.c b/archive-tar.c\nindex c7b85fd..ed562d4 100644\n--- a/archive-tar.c\n+++ b/archive-tar.c\n@@ -179,7 +179,7 @@ static size_t get_path_prefix(const char *path, size_t pathlen, size_t maxlen)\n \n static inline unsigned long ustar_size(uintmax_t size)\n {\n-\tif (size < 077777777777UL)\n+\tif (size <= 077777777777UL)\n \t\treturn size;\n \telse\n \t\treturn 0;\n@@ -187,7 +187,7 @@ static inline unsigned long ustar_size(uintmax_t size)\n \n static inline unsigned long ustar_mtime(time_t mtime)\n {\n-\tif (mtime < 077777777777UL)\n+\tif (mtime <= 077777777777UL)\n \t\treturn mtime;\n \telse\n \t\treturn 0;\n@@ -299,7 +299,7 @@ static int write_tar_entry(struct archiver_args *args,\n \t\t\tmemcpy(header.linkname, buffer, size);\n \t}\n \n-\tif (ustar_size(size) != size)\n+\tif (S_ISREG(mode) && ustar_size(size) != size)\n \t\tstrbuf_append_ext_header_uint(&ext_header, \"size\", size);\n \tif (ustar_mtime(args->time) != args->time)\n \t\tstrbuf_append_ext_header_uint(&ext_header, \"mtime\", args->time);\n\n"},{"id":"289776","messageId":"20160621161654.GA17826@sigill.intra.peff.net","threadId":"42622","inReplyTo":"20160621161604.GA17638@sigill.intra.peff.net","subject":"[PATCH v2 1/2] archive-tar: write extended headers for file sizes >= 8GB","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-21T16:16:54Z","receivedAt":"2016-06-21T16:17:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The ustar format has a fixed-length field for the size of\neach file entry which is supposed to contain up to 11 bytes\nof octal-formatted data plus a NUL or space terminator.\n\nThese means that the largest size we can represent is\n077777777777, or 1 byte short of 8GB. The correct solution\nfor a larger file, according to POSIX.1-2001, is to add an\nextended pax header, similar to how we handle long\nfilenames. This patch does that, and writes zero for the\nsize field in the ustar header (the last bit is not\nmentioned by POSIX, but it matches how GNU tar behaves with\n--format=pax).\n\nThis should be a strict improvement over the current\nbehavior, which is to die in xsnprintf with a \"BUG\".\nHowever, there's some interesting history here.\n\nPrior to f2f0267 (archive-tar: use xsnprintf for trivial\nformatting, 2015-09-24), we silently overflowed the \"size\"\nfield. The extra bytes ended up in the \"mtime\" field of the\nheader, which was then immediately written itself,\noverwriting our extra bytes. What that means depends on how\nmany bytes we wrote.\n\nIf the size was 64GB or greater, then we actually overflowed\ndigits into the mtime field, meaning our value was was\neffectively right-shifted by those lost octal digits. And\nthis patch is again a strict improvement over that.\n\nBut if the size was between 8GB and 64GB, then our 12-byte\nfield held all of the actual digits, and only our NUL\nterminator overflowed. According to POSIX, there should be a\nNUL or space at the end of the field. However, GNU tar seems\nto be lenient here, and will correctly parse a size up 64GB\n(minus one) from the field. So sizes in this range might\nhave just worked, depending on the implementation reading\nthe tarfile.\n\nThis patch is mostly still an improvement there, as the 8GB\nlimit is specifically mentioned in POSIX as the correct\nlimit. But it's possible that it could be a regression\n(versus the pre-f2f0267 state) if all of the following are\ntrue:\n\n 1. You have a file between 8GB and 64GB.\n\n 2. Your tar implementation _doesn't_ know about pax\n extended headers.\n\n 3. Your tar implementation _does_ parse 12-byte sizes from\n the ustar header without a delimiter.\n\nIt's probably not worth worrying about such an obscure set\nof conditions, but I'm documenting it here just in case.\n\nThere's no test included here. I did confirm that this works\n(using GNU tar) with:\n\n $ dd if=/dev/zero seek=64G bs=1 count=1 of=huge\n $ git add huge\n $ git commit -q -m foo\n $ git archive HEAD | head -c 10000 | tar tvf - 2>/dev/null\n -rw-rw-r-- root/root 68719476737 2016-06-15 21:07 huge\n\nPre-f2f0267, this would yield a bogus size of 8GB, and\npost-f2f0267, git-archive simply dies.\n\nUnfortunately, it's quite an expensive test to run. For one\nthing, unless your filesystem supports files with holes, it\ntakes 64GB of disk space (you might think piping straight to\n`hash-object --stdin` would be better, but it's not; that\ntries to buffer all 64GB in RAM!). Furthermore, hashing and\ncompressing the object takes several minutes of CPU time.\n\nWe could ship just the resulting compressed object data as a\nloose object, but even that takes 64MB. So sadly, this code\npath remains untested in the test suite.\n\nSigned-off-by: Jeff King \n---\n archive-tar.c | 28 +++++++++++++++++++++++++++-\n 1 file changed, 27 insertions(+), 1 deletion(-)\n\ndiff --git a/archive-tar.c b/archive-tar.c\nindex cb99df2..a9caf13 100644\n--- a/archive-tar.c\n+++ b/archive-tar.c\n@@ -137,6 +137,20 @@ static void strbuf_append_ext_header(struct strbuf *sb, const char *keyword,\n \tstrbuf_addch(sb, '\\n');\n }\n \n+/*\n+ * Like strbuf_append_ext_header, but for numeric values.\n+ */\n+static void strbuf_append_ext_header_uint(struct strbuf *sb,\n+\t\t\t\t\t const char *keyword,\n+\t\t\t\t\t uintmax_t value)\n+{\n+\tchar buf[40]; /* big enough for 2^128 in decimal, plus NUL */\n+\tint len;\n+\n+\tlen = xsnprintf(buf, sizeof(buf), \"%\"PRIuMAX, value);\n+\tstrbuf_append_ext_header(sb, keyword, buf, len);\n+}\n+\n static unsigned int ustar_header_chksum(const struct ustar_header *header)\n {\n \tconst unsigned char *p = (const unsigned char *)header;\n@@ -163,12 +177,21 @@ static size_t get_path_prefix(const char *path, size_t pathlen, size_t maxlen)\n \treturn i;\n }\n \n+static inline unsigned long ustar_size(uintmax_t size)\n+{\n+\tif (size <= 077777777777UL)\n+\t\treturn size;\n+\telse\n+\t\treturn 0;\n+}\n+\n static void prepare_header(struct archiver_args *args,\n \t\t\t struct ustar_header *header,\n \t\t\t unsigned int mode, unsigned long size)\n {\n \txsnprintf(header->mode, sizeof(header->mode), \"%07o\", mode & 07777);\n-\txsnprintf(header->size, sizeof(header->size), \"%011lo\", S_ISREG(mode) ? size : 0);\n+\txsnprintf(header->size, sizeof(header->size), \"%011lo\",\n+\t\t S_ISREG(mode) ? ustar_size(size) : 0);\n \txsnprintf(header->mtime, sizeof(header->mtime), \"%011lo\", (unsigned long) args->time);\n \n \txsnprintf(header->uid, sizeof(header->uid), \"%07o\", 0);\n@@ -267,6 +290,9 @@ static int write_tar_entry(struct archiver_args *args,\n \t\t\tmemcpy(header.linkname, buffer, size);\n \t}\n \n+\tif (S_ISREG(mode) && ustar_size(size) != size)\n+\t\tstrbuf_append_ext_header_uint(&ext_header, \"size\", size);\n+\n \tprepare_header(args, &header, mode, size);\n \n \tif (ext_header.len > 0) {\n-- \n2.9.0.204.g1499a7b\n\n"},{"id":"289777","messageId":"20160621161706.GB17826@sigill.intra.peff.net","threadId":"42622","inReplyTo":"20160621161604.GA17638@sigill.intra.peff.net","subject":"[PATCH v2 2/2] archive-tar: write extended headers for far-future mtime","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-21T16:17:06Z","receivedAt":"2016-06-21T16:17:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The ustar format represents timestamps as seconds since the\nepoch, but only has room to store 11 octal digits. To\nexpress anything larger, we need to use an extended header.\nThis is exactly the same case we fixed for the size field in\nthe previous commit, and the solution here follows the same\npattern.\n\nThis is even mentioned as an issue in f2f0267 (archive-tar:\nuse xsnprintf for trivial formatting, 2015-09-24), but since\nit only affected things far in the future, it wasn't worth\ndealing with. But note that my calculations claiming\nthousands of years were off there; because our xsnprintf\nproduces a NUL byte, we only have until the year 2242 to\nfix this.\n\nGiven that this is just around the corner (geologically\nspeaking), and because the fix for \"size\" makes doing this\non top trivial, let's just make it work.\n\nNote that we don't (and never did) handle negative\ntimestamps (i.e., before 1970). This would probably not be\ntoo hard to support in the same way, but since git does not\nsupport negative timestamps at all, I didn't bother here.\n\nUnlike the last patch for \"size\", this one is easy to test\nefficiently (the magic date is octal 01000000000001):\n\n $ echo content >file\n $ git add file\n $ GIT_COMMITTER_DATE='@68719476737 +0000' \\\n git commit -q -m 'tempori parendum'\n $ git archive HEAD | tar tvf -\n -rw-rw-r-- root/root 8 4147-08-20 03:32 file\n\nWith the current tip of master, this dies in xsnprintf (and\nprior to f2f0267, it overflowed into the checksum field, and\nthe resulting tarfile claimed to be from the year 2242).\n\nHowever, I decided not to include this in the test suite,\nbecause I suspect it will create portability headaches,\nincluding:\n\n 1. The exact format of the system tar's \"t\" output.\n\n 2. System tars that cannot handle pax headers.\n\n 3. System tars tha cannot handle dates that far in the\n future.\n\n 4. If we replace \"tar t\" with \"tar x\", then filesystems\n that cannot handle dates that far in the future.\n\nIn other words, we do not really have a reliable tar reader\nfor these corner cases, so the best we could do is a\nbyte-for-byte comparison of the output.\n\nSigned-off-by: Jeff King \n---\n archive-tar.c | 13 ++++++++++++-\n 1 file changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/archive-tar.c b/archive-tar.c\nindex a9caf13..ed562d4 100644\n--- a/archive-tar.c\n+++ b/archive-tar.c\n@@ -185,6 +185,14 @@ static inline unsigned long ustar_size(uintmax_t size)\n \t\treturn 0;\n }\n \n+static inline unsigned long ustar_mtime(time_t mtime)\n+{\n+\tif (mtime <= 077777777777UL)\n+\t\treturn mtime;\n+\telse\n+\t\treturn 0;\n+}\n+\n static void prepare_header(struct archiver_args *args,\n \t\t\t struct ustar_header *header,\n \t\t\t unsigned int mode, unsigned long size)\n@@ -192,7 +200,8 @@ static void prepare_header(struct archiver_args *args,\n \txsnprintf(header->mode, sizeof(header->mode), \"%07o\", mode & 07777);\n \txsnprintf(header->size, sizeof(header->size), \"%011lo\",\n \t\t S_ISREG(mode) ? ustar_size(size) : 0);\n-\txsnprintf(header->mtime, sizeof(header->mtime), \"%011lo\", (unsigned long) args->time);\n+\txsnprintf(header->mtime, sizeof(header->mtime), \"%011lo\",\n+\t\t ustar_mtime(args->time));\n \n \txsnprintf(header->uid, sizeof(header->uid), \"%07o\", 0);\n \txsnprintf(header->gid, sizeof(header->gid), \"%07o\", 0);\n@@ -292,6 +301,8 @@ static int write_tar_entry(struct archiver_args *args,\n \n \tif (S_ISREG(mode) && ustar_size(size) != size)\n \t\tstrbuf_append_ext_header_uint(&ext_header, \"size\", size);\n+\tif (ustar_mtime(args->time) != args->time)\n+\t\tstrbuf_append_ext_header_uint(&ext_header, \"mtime\", args->time);\n \n \tprepare_header(args, &header, mode, size);\n \n-- \n2.9.0.204.g1499a7b\n"},{"id":"289778","messageId":"20160621155920.GA7549@sigill.intra.peff.net","threadId":"42622","inReplyTo":"57687413.3030609@web.de","subject":"Re: [PATCH 1/2] archive-tar: write extended headers for file sizes >= 8GB","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-21T15:59:21Z","receivedAt":"2016-06-21T16:26:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 21, 2016 at 12:54:11AM +0200, René Scharfe wrote:\n\n> > Unfortunately, it's quite an expensive test to run. For one\n> > thing, unless your filesystem supports files with holes, it\n> > takes 64GB of disk space (you might think piping straight to\n> > `hash-object --stdin` would be better, but it's not; that\n> > tries to buffer all 64GB in RAM!). Furthermore, hashing and\n> > compressing the object takes several minutes of CPU time.\n> > \n> > We could ship just the resulting compressed object data as a\n> > loose object, but even that takes 64MB. So sadly, this code\n> > path remains untested in the test suite.\n> \n> If we could set the limit to a lower value than 8GB for testing then we\n> could at least check if the extended header is written, e.g. if ustar_size()\n> could be convinced to return 0 every time using a hidden command line\n> parameter or an environment variable or something better.\n\nYes, we could do that, though I think it loses most of the value of the\ntest. We can check that if we hit an arbitrary value we generate the pax\nheader, but I think what we _really_ care about is: did we generate an\noutput that somebody else's tar implementation can handle.\n\nAnd for the smaller-than-64GB case, GNU tar happily handles our existing\noutput (though I suspect other tars might fail at \"only\" 8GB).\n\n> > +static inline unsigned long ustar_size(uintmax_t size)\n> > +{\n> > +\tif (size < 077777777777UL)\n> \n> Shouldn't that be less-or-equal?\n\nYeah, you're right (and for the one in the next patch, too).\n\n> > +\tif (ustar_size(size) != size)\n> > +\t\tstrbuf_append_ext_header_uint(&ext_header, \"size\", size);\n> \n> It needs \"S_ISREG(mode) && \" as well, no? In practice it probably doesn't\n> matter (until someone stores a 8GB long symlink target), but the size field\n> should only be set for regular files.\n\nThanks for noticing that. I remembered wondering that when I was early\nin debugging/diagnosing, but forgot to follow up on it. I agree it's\nunlikely in practice, but we should have consistent checks (I think it\nwould actually make sense to move the ISREG check inside ustar_size, and\nthen we can apply it consistently here and when generating the header;\nmy goal with ustar_size() was to avoid having the same logic in multiple\nplaces).\n\n-Peff\n"},{"id":"289794","messageId":"xmqqtwgmpell.fsf@gitster.mtv.corp.google.com","threadId":"42622","inReplyTo":"20160621161604.GA17638@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] friendlier handling of overflows in archive-tar","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-21T18:43:34Z","receivedAt":"2016-06-21T18:44:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Junio, this is the jk/big-and-old-archive-tar topic.\n>\n> The interdiff is:\n> ...\n> diff --git a/archive-tar.c b/archive-tar.c\n> index c7b85fd..ed562d4 100644\n> --- a/archive-tar.c\n> +++ b/archive-tar.c\n> @@ -179,7 +179,7 @@ static size_t get_path_prefix(const char *path, size_t pathlen, size_t maxlen)\n> \n> static inline unsigned long ustar_size(uintmax_t size)\n> {\n> -\tif (size < 077777777777UL)\n> +\tif (size <= 077777777777UL)\n> \t\treturn size;\n> \telse\n> \t\treturn 0;\n> @@ -187,7 +187,7 @@ static inline unsigned long ustar_size(uintmax_t size)\n> \n> static inline unsigned long ustar_mtime(time_t mtime)\n> {\n> -\tif (mtime < 077777777777UL)\n> +\tif (mtime <= 077777777777UL)\n> \t\treturn mtime;\n> \telse\n> \t\treturn 0;\n> @@ -299,7 +299,7 @@ static int write_tar_entry(struct archiver_args *args,\n> \t\t\tmemcpy(header.linkname, buffer, size);\n> \t}\n> \n> -\tif (ustar_size(size) != size)\n> +\tif (S_ISREG(mode) && ustar_size(size) != size)\n> \t\tstrbuf_append_ext_header_uint(&ext_header, \"size\", size);\n> \tif (ustar_mtime(args->time) != args->time)\n> \t\tstrbuf_append_ext_header_uint(&ext_header, \"mtime\", args->time);\n\nThanks.\n\nBy the way, I realized the naming mistake and the topic branch is\nnow named s/old/future/. Size being big is one condition that needs\nthis fix, so is timestamp being from far future, not \"old\".\n"},{"id":"289802","messageId":"robbat2-20160621T185126-722667990Z@orbis-terrarum.net","threadId":"42622","inReplyTo":"20160616043733.GA18323@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] archive-tar: write extended headers for file sizes >= 8GB","fromName":"Robin H. Johnson","fromEmail":"robbat2@gentoo.org","sentAt":"2016-06-21T19:44:55Z","receivedAt":"2016-06-21T19:45:08Z","isPatch":true,"sender":{"key":"robbat2@gentoo.org","avatar":"https://avatars.githubusercontent.com/u/373898?v=4"},"body":"On Thu, Jun 16, 2016 at 12:37:33AM -0400, Jeff King wrote:\n> We could ship just the resulting compressed object data as a\n> loose object, but even that takes 64MB. So sadly, this code\n> path remains untested in the test suite.\nAdditionally compress the object data, and insert it for the purpose of\ntesting? It's still an expensive test time-wise, but repeated\ngzip compression on zeros does still help; to that end, here's the\npieces to add a testcase while only being <9KiB.\n\nt5005-archive-hugefile.sh:\n...\nmkdir t\nzcat t5005-barerepo-64gb-obj.tar.gz.gz | tar xzf - -C t\nGIT_DIR=t git archive HEAD | head -c 10000 | tar tvf - 2>/dev/null\n...\n\nTest repo attached, it's only 8KiB.\n\nTest repo can be recreated with:\ntruncate -s 64g bigfile\ngit add bigfile # takes 10 mins\ngit commit -q -m foo # takes another 10 minutes\ntar cf - -C .git . |gzip -9 --no-name |gzip -9f --no-name >barerepo.tar.gz.gz\n\n\n-- \nRobin Hugh Johnson\nGentoo Linux: Dev, Infra Lead, Foundation Trustee & Treasurer\nE-Mail : robbat2@gentoo.org\nGnuPG FP : 11ACBA4F 4778E3F6 E4EDF38E B27B944E 34884E85\nGnuPG FP : 7D0B3CEB E9B85B1F 825BCECF EE05E6F6 A48F6136\n"},{"id":"289807","messageId":"5769A6CC.9030001@web.de","threadId":"42622","inReplyTo":"20160621155920.GA7549@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] archive-tar: write extended headers for file sizes >= 8GB","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-06-21T20:42:52Z","receivedAt":"2016-06-21T20:51:52Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 21.06.2016 um 17:59 schrieb Jeff King:\n> On Tue, Jun 21, 2016 at 12:54:11AM +0200, René Scharfe wrote:\n> \n>>> Unfortunately, it's quite an expensive test to run. For one\n>>> thing, unless your filesystem supports files with holes, it\n>>> takes 64GB of disk space (you might think piping straight to\n>>> `hash-object --stdin` would be better, but it's not; that\n>>> tries to buffer all 64GB in RAM!). Furthermore, hashing and\n>>> compressing the object takes several minutes of CPU time.\n>>>\n>>> We could ship just the resulting compressed object data as a\n>>> loose object, but even that takes 64MB. So sadly, this code\n>>> path remains untested in the test suite.\n>>\n>> If we could set the limit to a lower value than 8GB for testing then we\n>> could at least check if the extended header is written, e.g. if ustar_size()\n>> could be convinced to return 0 every time using a hidden command line\n>> parameter or an environment variable or something better.\n> \n> Yes, we could do that, though I think it loses most of the value of the\n> test. We can check that if we hit an arbitrary value we generate the pax\n> header, but I think what we _really_ care about is: did we generate an\n> output that somebody else's tar implementation can handle.\n\nI agree with the last point, but don't see how that diminishes the\nvalue of such a test. If we provide file sizes only through extended\nheaders (the normal header field being set to 0) and we can extract\nfiles with correct sizes then tar must have interpreted those header\nas intended, right?\n\n(Or it just guessed the sizes by searching for the next header magic,\nbut such a fallback won't be accurate for files ending with NUL\ncharacters due to NUL-padding, so we just have to add such a file.)\n\nRené\n\n\n-- >8 --\nSubject: archive-tar: test creation of pax extended size headers\n\n---\nThe value 120 is magic; we need it to pass the tests. That's\nbecause prepare_header() is used for building extended header\nrecords as well and we don't create extended headers for extended\nheaders (not sure if that would work anyway), so they simply\nvanish when they're over the limit as their size field is set to\nzero.\n\n archive-tar.c | 7 ++++++-\n t/t5000-tar-tree.sh | 7 +++++++\n 2 files changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/archive-tar.c b/archive-tar.c\nindex f53e61c..fbbc4cc 100644\n--- a/archive-tar.c\n+++ b/archive-tar.c\n@@ -14,6 +14,7 @@ static char block[BLOCKSIZE];\n static unsigned long offset;\n \n static int tar_umask = 002;\n+static unsigned long ustar_size_max = 077777777777UL;\n \n static int write_tar_filter_archive(const struct archiver *ar,\n \t\t\t\t struct archiver_args *args);\n@@ -179,7 +180,7 @@ static size_t get_path_prefix(const char *path, size_t pathlen, size_t maxlen)\n \n static inline unsigned long ustar_size(uintmax_t size)\n {\n-\tif (size <= 077777777777UL)\n+\tif (size <= ustar_size_max)\n \t\treturn size;\n \telse\n \t\treturn 0;\n@@ -412,6 +413,10 @@ static int git_tar_config(const char *var, const char *value, void *cb)\n \t\t}\n \t\treturn 0;\n \t}\n+\tif (!strcmp(var, \"tar.ustarsizemax\")) {\n+\t\tustar_size_max = git_config_ulong(var, value);\n+\t\treturn 0;\n+\t}\n \n \treturn tar_filter_config(var, value, cb);\n }\ndiff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\nindex 4b68bba..03bb4c7 100755\n--- a/t/t5000-tar-tree.sh\n+++ b/t/t5000-tar-tree.sh\n@@ -102,6 +102,7 @@ test_expect_success \\\n echo long filename >a/four$hundred &&\n mkdir a/bin &&\n test-genrandom \"frotz\" 500000 >a/bin/sh &&\n+ printf \"\\0\\0\\0\" >>a/bin/sh &&\n printf \"A\\$Format:%s\\$O\" \"$SUBSTFORMAT\" >a/substfile1 &&\n printf \"A not substituted O\" >a/substfile2 &&\n if test_have_prereq SYMLINKS; then\n@@ -157,6 +158,12 @@ test_expect_success 'git-archive --prefix=olde-' '\n \n check_tar with_olde-prefix olde-\n \n+test_expect_success !TAR_NEEDS_PAX_FALLBACK 'pax extended size headers' '\n+\tgit -c tar.ustarsizemax=120 archive HEAD >extended_size_header.tar\n+'\n+\n+check_tar extended_size_header\n+\n test_expect_success 'git archive on large files' '\n test_config core.bigfilethreshold 1 &&\n git archive HEAD >b3.tar &&\n-- \n2.9.0\n\n"},{"id":"289808","messageId":"20160621205720.GA4747@sigill.intra.peff.net","threadId":"42622","inReplyTo":"robbat2-20160621T185126-722667990Z@orbis-terrarum.net","subject":"Re: [PATCH 1/2] archive-tar: write extended headers for file sizes >= 8GB","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-21T20:57:20Z","receivedAt":"2016-06-21T20:58:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 21, 2016 at 07:44:55PM +0000, Robin H. Johnson wrote:\n\n> On Thu, Jun 16, 2016 at 12:37:33AM -0400, Jeff King wrote:\n> > We could ship just the resulting compressed object data as a\n> > loose object, but even that takes 64MB. So sadly, this code\n> > path remains untested in the test suite.\n> Additionally compress the object data, and insert it for the purpose of\n> testing? It's still an expensive test time-wise, but repeated\n> gzip compression on zeros does still help; to that end, here's the\n> pieces to add a testcase while only being <9KiB.\n\nInteresting idea. With bzip2 it actually drops to about 600 bytes (I\nsuspect we could get it even smaller by writing a custom program to\ngenerate it, but it's diminishing returns).\n\nI think there are some other portability issues, though (like does the\nreceiving tar actually support 64GB sizes).\n\n-Peff\n"},{"id":"289809","messageId":"5769AA47.4020806@web.de","threadId":"42622","inReplyTo":"5769A6CC.9030001@web.de","subject":"Re: [PATCH 1/2] archive-tar: write extended headers for file sizes >= 8GB","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-06-21T20:57:43Z","receivedAt":"2016-06-21T20:58:14Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 21.06.2016 um 22:42 schrieb René Scharfe:\n> The value 120 is magic; we need it to pass the tests. That's\n> because prepare_header() is used for building extended header\n> records as well and we don't create extended headers for extended\n> headers (not sure if that would work anyway), so they simply\n> vanish when they're over the limit as their size field is set to\n> zero.\n\nSo how about something like this to make sure extended headers are\nonly written for regular files and not for other extended headers?\n---\n archive-tar.c | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/archive-tar.c b/archive-tar.c\nindex ed562d4..f53e61c 100644\n--- a/archive-tar.c\n+++ b/archive-tar.c\n@@ -199,7 +199,7 @@ static void prepare_header(struct archiver_args *args,\n {\n \txsnprintf(header->mode, sizeof(header->mode), \"%07o\", mode & 07777);\n \txsnprintf(header->size, sizeof(header->size), \"%011lo\",\n-\t\t S_ISREG(mode) ? ustar_size(size) : 0);\n+\t\t S_ISREG(mode) ? size : 0);\n \txsnprintf(header->mtime, sizeof(header->mtime), \"%011lo\",\n \t\t ustar_mtime(args->time));\n \n@@ -240,7 +240,7 @@ static int write_tar_entry(struct archiver_args *args,\n \tstruct ustar_header header;\n \tstruct strbuf ext_header = STRBUF_INIT;\n \tunsigned int old_mode = mode;\n-\tunsigned long size;\n+\tunsigned long size, size_in_header;\n \tvoid *buffer;\n \tint err = 0;\n \n@@ -299,12 +299,14 @@ static int write_tar_entry(struct archiver_args *args,\n \t\t\tmemcpy(header.linkname, buffer, size);\n \t}\n \n-\tif (S_ISREG(mode) && ustar_size(size) != size)\n+\tsize_in_header = S_ISREG(mode) ? ustar_size(size) : size;\n+\tif (size_in_header != size)\n \t\tstrbuf_append_ext_header_uint(&ext_header, \"size\", size);\n+\n \tif (ustar_mtime(args->time) != args->time)\n \t\tstrbuf_append_ext_header_uint(&ext_header, \"mtime\", args->time);\n \n-\tprepare_header(args, &header, mode, size);\n+\tprepare_header(args, &header, mode, size_in_header);\n \n \tif (ext_header.len > 0) {\n \t\terr = write_extended_header(args, sha1, ext_header.buf,\n-- \n2.9.0\n\n\n"},{"id":"289810","messageId":"5769A99A.9040508@web.de","threadId":"42622","inReplyTo":"20160621155920.GA7549@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] archive-tar: write extended headers for file sizes >= 8GB","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-06-21T20:54:50Z","receivedAt":"2016-06-21T21:01:44Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 21.06.2016 um 17:59 schrieb Jeff King:\n> On Tue, Jun 21, 2016 at 12:54:11AM +0200, René Scharfe wrote:\n>\n>>> Unfortunately, it's quite an expensive test to run. For one\n>>> thing, unless your filesystem supports files with holes, it\n>>> takes 64GB of disk space (you might think piping straight to\n>>> `hash-object --stdin` would be better, but it's not; that\n>>> tries to buffer all 64GB in RAM!). Furthermore, hashing and\n>>> compressing the object takes several minutes of CPU time.\n>>>\n>>> We could ship just the resulting compressed object data as a\n>>> loose object, but even that takes 64MB. So sadly, this code\n>>> path remains untested in the test suite.\n>>\n>> If we could set the limit to a lower value than 8GB for testing then we\n>> could at least check if the extended header is written, e.g. if ustar_size()\n>> could be convinced to return 0 every time using a hidden command line\n>> parameter or an environment variable or something better.\n>\n> Yes, we could do that, though I think it loses most of the value of the\n> test. We can check that if we hit an arbitrary value we generate the pax\n> header, but I think what we _really_ care about is: did we generate an\n> output that somebody else's tar implementation can handle.\n\nI agree with the last point, but don't see how that diminishes the\nvalue of such a test. If we provide file sizes only through extended\nheaders (the normal header field being set to 0) and we can extract\nfiles with correct sizes then tar must have interpreted those header\nas intended, right?\n\n(Or it just guessed the sizes by searching for the next header magic,\nbut such a fallback won't be accurate for files ending with NUL\ncharacters due to NUL-padding, so we just have to add such a file.)\n\nRené\n\n\n-- >8 --\nSubject: archive-tar: test creation of pax extended size headers\n\n---\nThe value 120 is magic; we need it to pass the tests. That's\nbecause prepare_header() is used for building extended header\nrecords as well and we don't create extended headers for extended\nheaders (not sure if that would work anyway), so they simply\nvanish when they're over the limit as their size field is set to\nzero.\n\n archive-tar.c | 7 ++++++-\n t/t5000-tar-tree.sh | 7 +++++++\n 2 files changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/archive-tar.c b/archive-tar.c\nindex f53e61c..fbbc4cc 100644\n--- a/archive-tar.c\n+++ b/archive-tar.c\n@@ -14,6 +14,7 @@ static char block[BLOCKSIZE];\n static unsigned long offset;\n \n static int tar_umask = 002;\n+static unsigned long ustar_size_max = 077777777777UL;\n \n static int write_tar_filter_archive(const struct archiver *ar,\n \t\t\t\t struct archiver_args *args);\n@@ -179,7 +180,7 @@ static size_t get_path_prefix(const char *path, size_t pathlen, size_t maxlen)\n \n static inline unsigned long ustar_size(uintmax_t size)\n {\n-\tif (size <= 077777777777UL)\n+\tif (size <= ustar_size_max)\n \t\treturn size;\n \telse\n \t\treturn 0;\n@@ -412,6 +413,10 @@ static int git_tar_config(const char *var, const char *value, void *cb)\n \t\t}\n \t\treturn 0;\n \t}\n+\tif (!strcmp(var, \"tar.ustarsizemax\")) {\n+\t\tustar_size_max = git_config_ulong(var, value);\n+\t\treturn 0;\n+\t}\n \n \treturn tar_filter_config(var, value, cb);\n }\ndiff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\nindex 4b68bba..03bb4c7 100755\n--- a/t/t5000-tar-tree.sh\n+++ b/t/t5000-tar-tree.sh\n@@ -102,6 +102,7 @@ test_expect_success \\\n echo long filename >a/four$hundred &&\n mkdir a/bin &&\n test-genrandom \"frotz\" 500000 >a/bin/sh &&\n+ printf \"\\0\\0\\0\" >>a/bin/sh &&\n printf \"A\\$Format:%s\\$O\" \"$SUBSTFORMAT\" >a/substfile1 &&\n printf \"A not substituted O\" >a/substfile2 &&\n if test_have_prereq SYMLINKS; then\n@@ -157,6 +158,12 @@ test_expect_success 'git-archive --prefix=olde-' '\n \n check_tar with_olde-prefix olde-\n \n+test_expect_success !TAR_NEEDS_PAX_FALLBACK 'pax extended size headers' '\n+\tgit -c tar.ustarsizemax=120 archive HEAD >extended_size_header.tar\n+'\n+\n+check_tar extended_size_header\n+\n test_expect_success 'git archive on large files' '\n test_config core.bigfilethreshold 1 &&\n git archive HEAD >b3.tar &&\n-- \n2.9.0\n\n"},{"id":"289811","messageId":"20160621210234.GB4747@sigill.intra.peff.net","threadId":"42622","inReplyTo":"5769A6CC.9030001@web.de","subject":"Re: [PATCH 1/2] archive-tar: write extended headers for file sizes >= 8GB","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-21T21:02:34Z","receivedAt":"2016-06-21T21:03:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 21, 2016 at 10:42:52PM +0200, René Scharfe wrote:\n\n> >> If we could set the limit to a lower value than 8GB for testing then we\n> >> could at least check if the extended header is written, e.g. if ustar_size()\n> >> could be convinced to return 0 every time using a hidden command line\n> >> parameter or an environment variable or something better.\n> > \n> > Yes, we could do that, though I think it loses most of the value of the\n> > test. We can check that if we hit an arbitrary value we generate the pax\n> > header, but I think what we _really_ care about is: did we generate an\n> > output that somebody else's tar implementation can handle.\n> \n> I agree with the last point, but don't see how that diminishes the\n> value of such a test. If we provide file sizes only through extended\n> headers (the normal header field being set to 0) and we can extract\n> files with correct sizes then tar must have interpreted those header\n> as intended, right?\n\nThe diminished value is:\n\n 1. This is a situation that doesn't actually happen in real life.\n\n 2. Now we're carrying extra code inside git only for the sake of\n testing (which can have its own bugs, etc).\n\nStill, it may be better than nothing.\n\n> -- >8 --\n> Subject: archive-tar: test creation of pax extended size headers\n> \n> ---\n> The value 120 is magic; we need it to pass the tests. That's\n> because prepare_header() is used for building extended header\n> records as well and we don't create extended headers for extended\n> headers (not sure if that would work anyway), so they simply\n> vanish when they're over the limit as their size field is set to\n> zero.\n\nRight, so this is sort of what I meant in (2). Now we have a\ntar.ustarsizemax setting shipped in git that is totally broken if you\nset it to \"1\".\n\nI can live with it as a tradeoff, but it is definitely a negative IMHO.\n\n-Peff\n"},{"id":"289816","messageId":"20160621210454.GC4747@sigill.intra.peff.net","threadId":"42622","inReplyTo":"5769AA47.4020806@web.de","subject":"Re: [PATCH 1/2] archive-tar: write extended headers for file sizes >= 8GB","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-21T21:04:54Z","receivedAt":"2016-06-21T21:33:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 21, 2016 at 10:57:43PM +0200, René Scharfe wrote:\n\n> Am 21.06.2016 um 22:42 schrieb René Scharfe:\n> > The value 120 is magic; we need it to pass the tests. That's\n> > because prepare_header() is used for building extended header\n> > records as well and we don't create extended headers for extended\n> > headers (not sure if that would work anyway), so they simply\n> > vanish when they're over the limit as their size field is set to\n> > zero.\n> \n> So how about something like this to make sure extended headers are\n> only written for regular files and not for other extended headers?\n\nThis is quite similar to what I wrote originally, but I moved to the\nustar_size() format to better match the mtime code (which needs\nsomething like that, because we pass around args->time).\n\nI think you could drop ustar_size() completely here and just put the\n\"if\" into write_tar_entry().\n\n-Peff\n"},{"id":"289827","messageId":"576A2625.8010706@web.de","threadId":"42622","inReplyTo":"20160621210234.GB4747@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] archive-tar: write extended headers for file sizes >= 8GB","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-06-22T05:46:13Z","receivedAt":"2016-06-22T05:46:38Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 21.06.2016 um 23:02 schrieb Jeff King:\n> On Tue, Jun 21, 2016 at 10:42:52PM +0200, René Scharfe wrote:\n>\n>>>> If we could set the limit to a lower value than 8GB for testing then we\n>>>> could at least check if the extended header is written, e.g. if ustar_size()\n>>>> could be convinced to return 0 every time using a hidden command line\n>>>> parameter or an environment variable or something better.\n>>>\n>>> Yes, we could do that, though I think it loses most of the value of the\n>>> test. We can check that if we hit an arbitrary value we generate the pax\n>>> header, but I think what we _really_ care about is: did we generate an\n>>> output that somebody else's tar implementation can handle.\n>>\n>> I agree with the last point, but don't see how that diminishes the\n>> value of such a test. If we provide file sizes only through extended\n>> headers (the normal header field being set to 0) and we can extract\n>> files with correct sizes then tar must have interpreted those header\n>> as intended, right?\n>\n> The diminished value is:\n>\n> 1. This is a situation that doesn't actually happen in real life.\n>\n> 2. Now we're carrying extra code inside git only for the sake of\n> testing (which can have its own bugs, etc).\n>\n> Still, it may be better than nothing.\n>\n>> -- >8 --\n>> Subject: archive-tar: test creation of pax extended size headers\n>>\n>> ---\n>> The value 120 is magic; we need it to pass the tests. That's\n>> because prepare_header() is used for building extended header\n>> records as well and we don't create extended headers for extended\n>> headers (not sure if that would work anyway), so they simply\n>> vanish when they're over the limit as their size field is set to\n>> zero.\n>\n> Right, so this is sort of what I meant in (2). Now we have a\n> tar.ustarsizemax setting shipped in git that is totally broken if you\n> set it to \"1\".\n>\n> I can live with it as a tradeoff, but it is definitely a negative IMHO.\n\nYes, it's only useful as a debug flag, but the fact that it breaks \nhighlights a (admittedly mostly theoretical) shortcoming: The code \ndoesn't handle extended headers that are over the size limit as nicely \nas it could. So the test was already worth it, even if won't land in \nmaster. :)\n\nRené\n\n"},{"id":"289828","messageId":"576A262E.6090703@web.de","threadId":"42622","inReplyTo":"20160621210454.GC4747@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] archive-tar: write extended headers for file sizes >= 8GB","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-06-22T05:46:22Z","receivedAt":"2016-06-22T05:46:45Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 21.06.2016 um 23:04 schrieb Jeff King:\n> On Tue, Jun 21, 2016 at 10:57:43PM +0200, René Scharfe wrote:\n> \n>> Am 21.06.2016 um 22:42 schrieb René Scharfe:\n>>> The value 120 is magic; we need it to pass the tests. That's\n>>> because prepare_header() is used for building extended header\n>>> records as well and we don't create extended headers for extended\n>>> headers (not sure if that would work anyway), so they simply\n>>> vanish when they're over the limit as their size field is set to\n>>> zero.\n>>\n>> So how about something like this to make sure extended headers are\n>> only written for regular files and not for other extended headers?\n> \n> This is quite similar to what I wrote originally, but I moved to the\n> ustar_size() format to better match the mtime code (which needs\n> something like that, because we pass around args->time).\n> \n> I think you could drop ustar_size() completely here and just put the\n> \"if\" into write_tar_entry().\n\nWhich would look like this:\n\n---\n archive-tar.c | 24 ++++++++++++++++++++++--\n 1 file changed, 22 insertions(+), 2 deletions(-)\n\ndiff --git a/archive-tar.c b/archive-tar.c\nindex cb99df2..274bdfa 100644\n--- a/archive-tar.c\n+++ b/archive-tar.c\n@@ -137,6 +137,20 @@ static void strbuf_append_ext_header(struct strbuf *sb, const char *keyword,\n \tstrbuf_addch(sb, '\\n');\n }\n \n+/*\n+ * Like strbuf_append_ext_header, but for numeric values.\n+ */\n+static void strbuf_append_ext_header_uint(struct strbuf *sb,\n+\t\t\t\t\t const char *keyword,\n+\t\t\t\t\t uintmax_t value)\n+{\n+\tchar buf[40]; /* big enough for 2^128 in decimal, plus NUL */\n+\tint len;\n+\n+\tlen = xsnprintf(buf, sizeof(buf), \"%\"PRIuMAX, value);\n+\tstrbuf_append_ext_header(sb, keyword, buf, len);\n+}\n+\n static unsigned int ustar_header_chksum(const struct ustar_header *header)\n {\n \tconst unsigned char *p = (const unsigned char *)header;\n@@ -208,7 +222,7 @@ static int write_tar_entry(struct archiver_args *args,\n \tstruct ustar_header header;\n \tstruct strbuf ext_header = STRBUF_INIT;\n \tunsigned int old_mode = mode;\n-\tunsigned long size;\n+\tunsigned long size, size_in_header;\n \tvoid *buffer;\n \tint err = 0;\n \n@@ -267,7 +281,13 @@ static int write_tar_entry(struct archiver_args *args,\n \t\t\tmemcpy(header.linkname, buffer, size);\n \t}\n \n-\tprepare_header(args, &header, mode, size);\n+\tsize_in_header = size;\n+\tif (S_ISREG(mode) && size > 077777777777UL) {\n+\t\tsize_in_header = 0;\n+\t\tstrbuf_append_ext_header_uint(&ext_header, \"size\", size);\n+\t}\n+\n+\tprepare_header(args, &header, mode, size_in_header);\n \n \tif (ext_header.len > 0) {\n \t\terr = write_extended_header(args, sha1, ext_header.buf,\n-- \n2.9.0\n"},{"id":"289829","messageId":"576A2631.2000608@web.de","threadId":"42622","inReplyTo":"57687417.4020009@web.de","subject":"Re: [PATCH 2/2] archive-tar: write extended headers for far-future mtime","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-06-22T05:46:25Z","receivedAt":"2016-06-22T05:46:51Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 21.06.2016 um 00:54 schrieb René Scharfe:\n> Am 16.06.2016 um 06:37 schrieb Jeff King:\n>> 2. System tars that cannot handle pax headers.\n>\n> In t5000 there is a simple interpreter for path headers for systems\n> whose tar doesn't handle them. Adding one for mtime headers may be\n> feasible.\n>\n> It's just a bit complicated to link a pax header file to the file it\n> applies to when it doesn't also contain a path header. But we know that\n> the mtime of all entries in tar files created by git archive are is the\n> same, so we could simply read one header and then adjust the mtime of\n> all files accordingly.\n\nThis brings me to the idea of using a single global pax header for mtime \ninstead of one per entry. It reduces the size of the archive and allows \nfor slightly easier testing -- it just fits better since we know that \nall our mtimes are the same.\n\nRené\n\n"},{"id":"289992","messageId":"20160623192124.GA32745@sigill.intra.peff.net","threadId":"42622","inReplyTo":"576A2625.8010706@web.de","subject":"Re: [PATCH 1/2] archive-tar: write extended headers for file sizes >= 8GB","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-23T19:21:24Z","receivedAt":"2016-06-23T19:21:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 22, 2016 at 07:46:13AM +0200, René Scharfe wrote:\n\n> Yes, it's only useful as a debug flag, but the fact that it breaks\n> highlights a (admittedly mostly theoretical) shortcoming: The code doesn't\n> handle extended headers that are over the size limit as nicely as it could.\n> So the test was already worth it, even if won't land in master. :)\n\nKind of. It was impossible to trigger in the original (and we still\ndon't actually handle it in the revised version; we just die in\nxsnprintf).\n\nBut still, I'll go with the simpler thing we've discussed here. The\nsymmetry with ustar_mtime isn't worth it, and doubly so if we just write\na single pax header.\n\n-Peff\n"},{"id":"289993","messageId":"20160623192251.GB32745@sigill.intra.peff.net","threadId":"42622","inReplyTo":"576A2631.2000608@web.de","subject":"Re: [PATCH 2/2] archive-tar: write extended headers for far-future mtime","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-23T19:22:51Z","receivedAt":"2016-06-23T19:22:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 22, 2016 at 07:46:25AM +0200, René Scharfe wrote:\n\n> Am 21.06.2016 um 00:54 schrieb René Scharfe:\n> > Am 16.06.2016 um 06:37 schrieb Jeff King:\n> > > 2. System tars that cannot handle pax headers.\n> > \n> > In t5000 there is a simple interpreter for path headers for systems\n> > whose tar doesn't handle them. Adding one for mtime headers may be\n> > feasible.\n> > \n> > It's just a bit complicated to link a pax header file to the file it\n> > applies to when it doesn't also contain a path header. But we know that\n> > the mtime of all entries in tar files created by git archive are is the\n> > same, so we could simply read one header and then adjust the mtime of\n> > all files accordingly.\n> \n> This brings me to the idea of using a single global pax header for mtime\n> instead of one per entry. It reduces the size of the archive and allows for\n> slightly easier testing -- it just fits better since we know that all our\n> mtimes are the same.\n\nYeah, I had a similar thought while writing it, but wasn't quite sure\nhow that was supposed to be formatted. I modeled my output after what\nGNU tar writes, but of course they are expecting a different mtime for\neach file.\n\nI'll look into how global pax headers should look.\n\n-Peff\n"},{"id":"289999","messageId":"576C56CB.1040701@web.de","threadId":"42622","inReplyTo":"20160623192251.GB32745@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] archive-tar: write extended headers for far-future mtime","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-06-23T21:38:19Z","receivedAt":"2016-06-23T21:38:49Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 23.06.2016 um 21:22 schrieb Jeff King:\n> On Wed, Jun 22, 2016 at 07:46:25AM +0200, René Scharfe wrote:\n>\n>> Am 21.06.2016 um 00:54 schrieb René Scharfe:\n>>> Am 16.06.2016 um 06:37 schrieb Jeff King:\n>>>> 2. System tars that cannot handle pax headers.\n>>>\n>>> In t5000 there is a simple interpreter for path headers for systems\n>>> whose tar doesn't handle them. Adding one for mtime headers may be\n>>> feasible.\n>>>\n>>> It's just a bit complicated to link a pax header file to the file it\n>>> applies to when it doesn't also contain a path header. But we know that\n>>> the mtime of all entries in tar files created by git archive are is the\n>>> same, so we could simply read one header and then adjust the mtime of\n>>> all files accordingly.\n>>\n>> This brings me to the idea of using a single global pax header for mtime\n>> instead of one per entry. It reduces the size of the archive and allows for\n>> slightly easier testing -- it just fits better since we know that all our\n>> mtimes are the same.\n>\n> Yeah, I had a similar thought while writing it, but wasn't quite sure\n> how that was supposed to be formatted. I modeled my output after what\n> GNU tar writes, but of course they are expecting a different mtime for\n> each file.\n>\n> I'll look into how global pax headers should look.\n\nMoving the strbuf_append_ext_header() call into \nwrite_global_extended_header() should be enough. The changes to the \ntest script are a bit more interesting, though. Perhaps I can come up \nwith something over the weekend.\n\nRené\n"},{"id":"290000","messageId":"20160623213924.GA1123@sigill.intra.peff.net","threadId":"42622","inReplyTo":"576C56CB.1040701@web.de","subject":"Re: [PATCH 2/2] archive-tar: write extended headers for far-future mtime","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-23T21:39:24Z","receivedAt":"2016-06-23T21:39:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 23, 2016 at 11:38:19PM +0200, René Scharfe wrote:\n\n> > Yeah, I had a similar thought while writing it, but wasn't quite sure\n> > how that was supposed to be formatted. I modeled my output after what\n> > GNU tar writes, but of course they are expecting a different mtime for\n> > each file.\n> > \n> > I'll look into how global pax headers should look.\n> \n> Moving the strbuf_append_ext_header() call into\n> write_global_extended_header() should be enough. The changes to the test\n> script are a bit more interesting, though. Perhaps I can come up with\n> something over the weekend.\n\nYeah, I have the code itself working now. I'm just slogging through\nwriting tests that are efficient and portable. I hope to have something\nout soon.\n\n-Peff\n"},{"id":"290002","messageId":"20160623231512.GA27683@sigill.intra.peff.net","threadId":"42622","inReplyTo":"20160621161604.GA17638@sigill.intra.peff.net","subject":"[PATCH v3] friendlier handling of overflows in archive-tar","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-23T23:15:13Z","receivedAt":"2016-06-23T23:15:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is a replacement for the jk/big-and-future-archive-tar topic. It's\nsufficiently rewritten that I won't bother with an interdiff, but rather\njust describe the changes below. Thanks to René for talking through some\nof the issues, and to Robin for suggesting the double-compression trick\nfor the tests.\n\nThe important changes here are:\n\n - the \"size\" fix is more localized to write_tar_entry(). This results\n in shorter code, and means that if one were to somehow write an\n extended header larger than 8GB, we would notice and die(), rather\n than just writing a \"0\" (though such a thing should not be\n possible).\n\n - the mtime is now written only in a single global header, rather than\n once per file. I checked POSIX and this is explicitly allowed, and\n of course I also checked that it works against GNU tar.\n\n - there are now tests! I tried hard to make them portable and\n efficient. See the first patch for details.\n\n - the final patch is just a cleanup I noticed in the area; it could be\n done independently, but I floated it to the end in case it's\n controversial.\n\nThe patches are:\n\n [1/4]: t5000: test tar files that overflow ustar headers\n [2/4]: archive-tar: write extended headers for file sizes >= 8GB\n [3/4]: archive-tar: write extended headers for far-future mtime\n [4/4]: archive-tar: drop return value\n\n-Peff\n"},{"id":"290003","messageId":"20160623232041.GA3668@sigill.intra.peff.net","threadId":"42622","inReplyTo":"20160623231512.GA27683@sigill.intra.peff.net","subject":"[PATCH v3 1/4] t5000: test tar files that overflow ustar headers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-23T23:20:45Z","receivedAt":"2016-06-23T23:20:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The ustar format only has room for 11 (or 12, depending on\nsome implementations) octal digits for the size and mtime of\neach file. After this, we have to add pax extended headers\nto specify the real data, and git does not yet know how to\ndo so.\n\nBefore fixing that, let's start off with some test\ninfrastructure, as designing portable and efficient tests\nfor this is non-trivial.\n\nWe want to use the system tar to check our output (because\nwhat we really care about is interoperability), but we can't\nrely on it:\n\n 1. being able to read pax headers\n\n 2. being able to handle huge sizes or mtimes\n\n 3. supporting a \"t\" format we can parse\n\nSo as a prerequisite, we can feed the system tar a reference\ntarball to make sure it can handle these features. The\nreference tar here was created with:\n\n dd if=/dev/zero seek=64G bs=1 count=1 of=huge\n touch -d @68719476737 huge\n tar cf - --format=pax |\n head -c 2048\n\nusing GNU tar. Note that this is not a complete tarfile, but\nit's enough to contain the headers we want to examine.\n\nLikewise, we need to convince git that it has a 64GB blob to\noutput. Running \"git add\" on that 64GB file takes many\nminutes of CPU, and even compressed, the result is 64MB. So\nagain, I pre-generated that loose object, and then used\nbzip2 on the result, which shrinks it to a few hundred\nbytes. Unfortunately, we do still inflate it to 64MB on\ndisk briefly while the test is running.\n\nThe tests are split so that we test as much as we can even\nwith an uncooperative system tar. This actually catches the\ncurrent breakage (which is that we die(\"BUG\") trying to\nwrite the ustar header) on every system, and then on systems\nwhere we can, we go farther and actually verify the result.\n\nHelped-by: Robin H. Johnson \nSigned-off-by: Jeff King \n---\nI'm still not excited about the 64MB write, just because it's awfully\nheavyweight for such a trivial test. It runs pretty fast on my RAM disk,\nbut maybe not on other people's system.\n\nI considered but didn't explore two other options:\n\n 1. I couldn't convince zlib to write a smaller file (this is done with\n core.compression=9). But I'm not sure if that's inherent to the\n on-disk format, or simply the maximum size of a deflate block.\n\n So it's possible that one could hand-roll zlib data that says \"I'm\n 64GB\" but is only a few bytes long.\n\n 2. We don't ever want to see the whole 64GB, of course; we want to\n stream it out and only care about the header (as an aside, this\n makes a wonderful test that we are hitting the streaming code path,\n as it's unlikely to work without it :) ).\n\n So another option would be to include a truncated file that claims\n to be 64GB, and has only the first 256kb or something worth of data\n (which should deflate down to almost nothing).\n\n git-fsck wouldn't work, of course, but we don't need to run it.\n Other bits of git might complain, but our plan is for git to get\n SIGPIPE before hitting that point anyway.\n\n So that seems pretty easy, but it is potentially flaky.\n\n t/t5000-tar-tree.sh | 73 +++++++++++++++++++++\n .../19f9c8273ec45a8938e6999cb59b3ff66739902a.bz2 | Bin 0 -> 578 bytes\n t/t5000/huge-and-future.tar | Bin 0 -> 2048 bytes\n 3 files changed, 73 insertions(+)\n create mode 100644 t/t5000/19f9c8273ec45a8938e6999cb59b3ff66739902a.bz2\n create mode 100644 t/t5000/huge-and-future.tar\n\ndiff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\nindex 4b68bba..e7c9271 100755\n--- a/t/t5000-tar-tree.sh\n+++ b/t/t5000-tar-tree.sh\n@@ -319,4 +319,77 @@ test_expect_success 'catch non-matching pathspec' '\n \ttest_must_fail git archive -v HEAD -- \"*.abc\" >/dev/null\n '\n \n+# See if our system tar can handle a tar file with huge sizes and dates far in\n+# the future, and that we can actually parse its output.\n+#\n+# The reference file was generated by GNU tar, and the magic time and size are\n+# both octal 01000000000001, which overflows normal ustar fields.\n+#\n+# When parsing, we'll pull out only the year from the date; that\n+# avoids any question of timezones impacting the result. The output\n+# of tar_info is expected to be \" \", both in decimal. It ignores\n+# the return value of tar. We have to do this, because our reference file is\n+# only a partial (the whole thing would be 64GB!).\n+tar_info () {\n+\t\"$TAR\" tvf \"$1\" | awk '{print $3 \" \" $4}' | cut -d- -f1\n+}\n+test_lazy_prereq TAR_HUGE '\n+\techo \"68719476737 4147\" >expect &&\n+\ttar_info \"$TEST_DIRECTORY\"/t5000/huge-and-future.tar >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+# Likewise, we need bunzip for the 64GB git object.\n+test_lazy_prereq BUNZIP '\n+\tbunzip2 --version\n+'\n+\n+test_expect_success BUNZIP 'set up repository with huge blob' '\n+\tobj_d=19 &&\n+\tobj_f=f9c8273ec45a8938e6999cb59b3ff66739902a &&\n+\tobj=${obj_d}${obj_f} &&\n+\tmkdir -p .git/objects/$obj_d &&\n+\tbunzip2 -c \\\n+\t\t<\"$TEST_DIRECTORY\"/t5000/$obj.bz2 \\\n+\t\t>.git/objects/$obj_d/$obj_f &&\n+\trm -f .git/index &&\n+\tgit update-index --add --cacheinfo 100644,$obj,huge &&\n+\tgit commit -m huge\n+'\n+\n+# We expect git to die with SIGPIPE here (otherwise we\n+# would generate the whole 64GB).\n+test_expect_failure BUNZIP 'generate tar with huge size' '\n+\t{\n+\t\tgit archive HEAD\n+\t\techo $? >exit-code\n+\t} | head -c 4096 >huge.tar &&\n+\techo 141 >expect &&\n+\ttest_cmp expect exit-code\n+'\n+\n+test_expect_failure BUNZIP,TAR_HUGE 'system tar can read our huge size' '\n+\techo 68719476737 >expect &&\n+\ttar_info huge.tar | cut -d\" \" -f1 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'set up repository with far-future commit' '\n+\trm -f .git/index &&\n+\techo content >file &&\n+\tgit add file &&\n+\tGIT_COMMITTER_DATE=\"@68719476737 +0000\" \\\n+\t\tgit commit -m \"tempori parendum\"\n+'\n+\n+test_expect_failure 'generate tar with future mtime' '\n+\tgit archive HEAD >future.tar\n+'\n+\n+test_expect_failure TAR_HUGE 'tar can encode dates far in future' '\n+\techo 4147 >expect &&\n+\ttar_info future.tar | cut -d\" \" -f2 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\ndiff --git a/t/t5000/19f9c8273ec45a8938e6999cb59b3ff66739902a.bz2 b/t/t5000/19f9c8273ec45a8938e6999cb59b3ff66739902a.bz2\nnew file mode 100644\nindex 0000000000000000000000000000000000000000..9619fd3c5f6f345a40aba1b91807bb4d937fc51c\nGIT binary patch\nliteral 578\nzcmV-I0=@l0T4*^jL0KkKS$mI#Bmk}@egE{{00qbxVL(9uNQgj6FakgbL{P{8umA%P\nz1cU%-zyaKJ3Zno3&;S`U000^T007Vc88iR@8UZy`L7)Mk0iXZ?8a+UzR8!JvwH}iK\nzXvkv}!$8#Dpwk&jhdD}y=}Ly#N{6=5N`@-9@zf#~Y}ml6A5xVKIEqv?6sUZ+ic~-0\nz3bD}&yn;flel6M|SGh`t%>^nMd9)O$Wu&Noa%SgnPira`Ve7_Hm1V`0kgE@zR6?%0tYir&brh(3dz7ebDNy(^\nzL0KkKS?}&rK>-rd{r~jXFbzl$fdD`NUjRG-;qPzyn5xj3Xw4Mw$Zz)lx%14F-mTL6atf)Gz?ip`!@NpwXs)%#?y?q!Xf$\nzP8vZs=>+{8*(#7u93>!3K5|fsT$F-(VMr#rK{}}f`lJ&zAez-6nY%u7(h0VZPg+4e\nzX$0a(CY`i`W~3AAPBemQq!ZGRP7dlpJ?RA1nvhKtf@|4GCaOU@$`MOSl~R?f3Ia)6\nzDFpYV6Llb)sRYUuLMe6QNGF9Ln9>Q3G=gtZ37;(>om7H;BohijH-{QkNGI(epUDKq\nQkWcYgk^{~&l;zIw((@uRaR^\n\nliteral 0\nHcmV?d00001\n\n-- \n2.9.0.217.g096ca68\n\n"},{"id":"290004","messageId":"20160623232104.GB3668@sigill.intra.peff.net","threadId":"42622","inReplyTo":"20160623231512.GA27683@sigill.intra.peff.net","subject":"[PATCH v3 2/4] archive-tar: write extended headers for file sizes >= 8GB","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-23T23:21:05Z","receivedAt":"2016-06-23T23:21:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The ustar format has a fixed-length field for the size of\neach file entry which is supposed to contain up to 11 bytes\nof octal-formatted data plus a NUL or space terminator.\n\nThese means that the largest size we can represent is\n077777777777, or 1 byte short of 8GB. The correct solution\nfor a larger file, according to POSIX.1-2001, is to add an\nextended pax header, similar to how we handle long\nfilenames. This patch does that, and writes zero for the\nsize field in the ustar header (the last bit is not\nmentioned by POSIX, but it matches how GNU tar behaves with\n--format=pax).\n\nThis should be a strict improvement over the current\nbehavior, which is to die in xsnprintf with a \"BUG\".\nHowever, there's some interesting history here.\n\nPrior to f2f0267 (archive-tar: use xsnprintf for trivial\nformatting, 2015-09-24), we silently overflowed the \"size\"\nfield. The extra bytes ended up in the \"mtime\" field of the\nheader, which was then immediately written itself,\noverwriting our extra bytes. What that means depends on how\nmany bytes we wrote.\n\nIf the size was 64GB or greater, then we actually overflowed\ndigits into the mtime field, meaning our value was was\neffectively right-shifted by those lost octal digits. And\nthis patch is again a strict improvement over that.\n\nBut if the size was between 8GB and 64GB, then our 12-byte\nfield held all of the actual digits, and only our NUL\nterminator overflowed. According to POSIX, there should be a\nNUL or space at the end of the field. However, GNU tar seems\nto be lenient here, and will correctly parse a size up 64GB\n(minus one) from the field. So sizes in this range might\nhave just worked, depending on the implementation reading\nthe tarfile.\n\nThis patch is mostly still an improvement there, as the 8GB\nlimit is specifically mentioned in POSIX as the correct\nlimit. But it's possible that it could be a regression\n(versus the pre-f2f0267 state) if all of the following are\ntrue:\n\n 1. You have a file between 8GB and 64GB.\n\n 2. Your tar implementation _doesn't_ know about pax\n extended headers.\n\n 3. Your tar implementation _does_ parse 12-byte sizes from\n the ustar header without a delimiter.\n\nIt's probably not worth worrying about such an obscure set\nof conditions, but I'm documenting it here just in case.\n\nHelped-by: René Scharfe \nSigned-off-by: Jeff King \n---\n archive-tar.c | 24 ++++++++++++++++++++++--\n t/t5000-tar-tree.sh | 4 ++--\n 2 files changed, 24 insertions(+), 4 deletions(-)\n\ndiff --git a/archive-tar.c b/archive-tar.c\nindex cb99df2..274bdfa 100644\n--- a/archive-tar.c\n+++ b/archive-tar.c\n@@ -137,6 +137,20 @@ static void strbuf_append_ext_header(struct strbuf *sb, const char *keyword,\n \tstrbuf_addch(sb, '\\n');\n }\n \n+/*\n+ * Like strbuf_append_ext_header, but for numeric values.\n+ */\n+static void strbuf_append_ext_header_uint(struct strbuf *sb,\n+\t\t\t\t\t const char *keyword,\n+\t\t\t\t\t uintmax_t value)\n+{\n+\tchar buf[40]; /* big enough for 2^128 in decimal, plus NUL */\n+\tint len;\n+\n+\tlen = xsnprintf(buf, sizeof(buf), \"%\"PRIuMAX, value);\n+\tstrbuf_append_ext_header(sb, keyword, buf, len);\n+}\n+\n static unsigned int ustar_header_chksum(const struct ustar_header *header)\n {\n \tconst unsigned char *p = (const unsigned char *)header;\n@@ -208,7 +222,7 @@ static int write_tar_entry(struct archiver_args *args,\n \tstruct ustar_header header;\n \tstruct strbuf ext_header = STRBUF_INIT;\n \tunsigned int old_mode = mode;\n-\tunsigned long size;\n+\tunsigned long size, size_in_header;\n \tvoid *buffer;\n \tint err = 0;\n \n@@ -267,7 +281,13 @@ static int write_tar_entry(struct archiver_args *args,\n \t\t\tmemcpy(header.linkname, buffer, size);\n \t}\n \n-\tprepare_header(args, &header, mode, size);\n+\tsize_in_header = size;\n+\tif (S_ISREG(mode) && size > 077777777777UL) {\n+\t\tsize_in_header = 0;\n+\t\tstrbuf_append_ext_header_uint(&ext_header, \"size\", size);\n+\t}\n+\n+\tprepare_header(args, &header, mode, size_in_header);\n \n \tif (ext_header.len > 0) {\n \t\terr = write_extended_header(args, sha1, ext_header.buf,\ndiff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\nindex e7c9271..79dbc88 100755\n--- a/t/t5000-tar-tree.sh\n+++ b/t/t5000-tar-tree.sh\n@@ -359,7 +359,7 @@ test_expect_success BUNZIP 'set up repository with huge blob' '\n \n # We expect git to die with SIGPIPE here (otherwise we\n # would generate the whole 64GB).\n-test_expect_failure BUNZIP 'generate tar with huge size' '\n+test_expect_success BUNZIP 'generate tar with huge size' '\n \t{\n \t\tgit archive HEAD\n \t\techo $? >exit-code\n@@ -368,7 +368,7 @@ test_expect_failure BUNZIP 'generate tar with huge size' '\n \ttest_cmp expect exit-code\n '\n \n-test_expect_failure BUNZIP,TAR_HUGE 'system tar can read our huge size' '\n+test_expect_success BUNZIP,TAR_HUGE 'system tar can read our huge size' '\n \techo 68719476737 >expect &&\n \ttar_info huge.tar | cut -d\" \" -f1 >actual &&\n \ttest_cmp expect actual\n-- \n2.9.0.217.g096ca68\n\n"},{"id":"290005","messageId":"20160623232112.GC3668@sigill.intra.peff.net","threadId":"42622","inReplyTo":"20160623231512.GA27683@sigill.intra.peff.net","subject":"[PATCH v3 3/4] archive-tar: write extended headers for far-future mtime","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-23T23:21:13Z","receivedAt":"2016-06-23T23:21:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The ustar format represents timestamps as seconds since the\nepoch, but only has room to store 11 octal digits. To\nexpress anything larger, we need to use an extended header.\nThis is exactly the same case we fixed for the size field in\nthe previous commit, and the solution here follows the same\npattern.\n\nThis is even mentioned as an issue in f2f0267 (archive-tar:\nuse xsnprintf for trivial formatting, 2015-09-24), but since\nit only affected things far in the future, it wasn't deemed\nworth dealing with. But note that my calculations claiming\nthousands of years were off there; because our xsnprintf\nproduces a NUL byte, we only have until the year 2242 to fix\nthis.\n\nGiven that this is just around the corner (geologically\nspeaking, anyway), and because it's easy to fix, let's just\nmake it work. Unlike the previous fix for \"size\", where we\nhad to write an individual extended header for each file, we\ncan write one global header (since we have only one mtime\nfor the whole archive).\n\nThere's a slight bit of trickiness there. We may already be\nwriting a global header with a \"comment\" field for the\ncommit sha1. So we need to write our new field into the same\nheader. To do this, we push the decision of whether to write\nsuch a header down into write_global_extended_header(),\nwhich will now assemble the header as it sees fit, and will\nreturn early if we have nothing to write (in practice, we'll\nonly have a large mtime if it comes from a commit, but this\nmakes it also work if you set your system clock ahead such\nthat time() returns a huge value).\n\nNote that we don't (and never did) handle negative\ntimestamps (i.e., before 1970). This would probably not be\ntoo hard to support in the same way, but since git does not\nsupport negative timestamps at all, I didn't bother here.\n\nAfter writing the extended header, we munge the timestamp in\nthe ustar headers to the maximum-allowable size. This is\nwrong, but it's the least-wrong thing we can provide to a\ntar implementation that doesn't understand pax headers (it's\nalso what GNU tar does).\n\nHelped-by: René Scharfe \nSigned-off-by: Jeff King \n---\n archive-tar.c | 16 +++++++++++++---\n t/t5000-tar-tree.sh | 4 ++--\n 2 files changed, 15 insertions(+), 5 deletions(-)\n\ndiff --git a/archive-tar.c b/archive-tar.c\nindex 274bdfa..0bb164c 100644\n--- a/archive-tar.c\n+++ b/archive-tar.c\n@@ -317,7 +317,18 @@ static int write_global_extended_header(struct archiver_args *args)\n \tunsigned int mode;\n \tint err = 0;\n \n-\tstrbuf_append_ext_header(&ext_header, \"comment\", sha1_to_hex(sha1), 40);\n+\tif (sha1)\n+\t\tstrbuf_append_ext_header(&ext_header, \"comment\",\n+\t\t\t\t\t sha1_to_hex(sha1), 40);\n+\tif (args->time > 077777777777UL) {\n+\t\tstrbuf_append_ext_header_uint(&ext_header, \"mtime\",\n+\t\t\t\t\t args->time);\n+\t\targs->time = 077777777777UL;\n+\t}\n+\n+\tif (!ext_header.len)\n+\t\treturn 0;\n+\n \tmemset(&header, 0, sizeof(header));\n \t*header.typeflag = TYPEFLAG_GLOBAL_HEADER;\n \tmode = 0100666;\n@@ -402,8 +413,7 @@ static int write_tar_archive(const struct archiver *ar,\n {\n \tint err = 0;\n \n-\tif (args->commit_sha1)\n-\t\terr = write_global_extended_header(args);\n+\terr = write_global_extended_header(args);\n \tif (!err)\n \t\terr = write_archive_entries(args, write_tar_entry);\n \tif (!err)\ndiff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\nindex 79dbc88..07e0bdc 100755\n--- a/t/t5000-tar-tree.sh\n+++ b/t/t5000-tar-tree.sh\n@@ -382,11 +382,11 @@ test_expect_success 'set up repository with far-future commit' '\n \t\tgit commit -m \"tempori parendum\"\n '\n \n-test_expect_failure 'generate tar with future mtime' '\n+test_expect_success 'generate tar with future mtime' '\n \tgit archive HEAD >future.tar\n '\n \n-test_expect_failure TAR_HUGE 'tar can encode dates far in future' '\n+test_expect_success TAR_HUGE 'tar can encode dates far in future' '\n \techo 4147 >expect &&\n \ttar_info future.tar | cut -d\" \" -f2 >actual &&\n \ttest_cmp expect actual\n-- \n2.9.0.217.g096ca68\n\n"},{"id":"290006","messageId":"20160623232158.GD3668@sigill.intra.peff.net","threadId":"42622","inReplyTo":"20160623231512.GA27683@sigill.intra.peff.net","subject":"[PATCH v3 4/4] archive-tar: drop return value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-23T23:21:59Z","receivedAt":"2016-06-23T23:22:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We never do any error checks, and so never return anything\nbut \"0\". Let's just drop this to simplify the code.\n\nSigned-off-by: Jeff King \n---\nI wasn't sure if this was perhaps kept as an interface decision, in case\nthe function grew errors later on. If so, it can still drop the \"err\"\nvariable internally. :)\n\n archive-tar.c | 8 +++-----\n 1 file changed, 3 insertions(+), 5 deletions(-)\n\ndiff --git a/archive-tar.c b/archive-tar.c\nindex 0bb164c..903b74d 100644\n--- a/archive-tar.c\n+++ b/archive-tar.c\n@@ -309,13 +309,12 @@ static int write_tar_entry(struct archiver_args *args,\n \treturn err;\n }\n \n-static int write_global_extended_header(struct archiver_args *args)\n+static void write_global_extended_header(struct archiver_args *args)\n {\n \tconst unsigned char *sha1 = args->commit_sha1;\n \tstruct strbuf ext_header = STRBUF_INIT;\n \tstruct ustar_header header;\n \tunsigned int mode;\n-\tint err = 0;\n \n \tif (sha1)\n \t\tstrbuf_append_ext_header(&ext_header, \"comment\",\n@@ -327,7 +326,7 @@ static int write_global_extended_header(struct archiver_args *args)\n \t}\n \n \tif (!ext_header.len)\n-\t\treturn 0;\n+\t\treturn;\n \n \tmemset(&header, 0, sizeof(header));\n \t*header.typeflag = TYPEFLAG_GLOBAL_HEADER;\n@@ -337,7 +336,6 @@ static int write_global_extended_header(struct archiver_args *args)\n \twrite_blocked(&header, sizeof(header));\n \twrite_blocked(ext_header.buf, ext_header.len);\n \tstrbuf_release(&ext_header);\n-\treturn err;\n }\n \n static struct archiver **tar_filters;\n@@ -413,7 +411,7 @@ static int write_tar_archive(const struct archiver *ar,\n {\n \tint err = 0;\n \n-\terr = write_global_extended_header(args);\n+\twrite_global_extended_header(args);\n \tif (!err)\n \t\terr = write_archive_entries(args, write_tar_entry);\n \tif (!err)\n-- \n2.9.0.217.g096ca68\n"},{"id":"290007","messageId":"20160623233114.GA5605@sigill.intra.peff.net","threadId":"42622","inReplyTo":"20160623232041.GA3668@sigill.intra.peff.net","subject":"Re: [PATCH v3 1/4] t5000: test tar files that overflow ustar headers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-23T23:31:15Z","receivedAt":"2016-06-23T23:31:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 23, 2016 at 07:20:44PM -0400, Jeff King wrote:\n\n> I'm still not excited about the 64MB write, just because it's awfully\n> heavyweight for such a trivial test. It runs pretty fast on my RAM disk,\n> but maybe not on other people's system.\n> \n> I considered but didn't explore two other options:\n> \n> 1. I couldn't convince zlib to write a smaller file (this is done with\n> core.compression=9). But I'm not sure if that's inherent to the\n> on-disk format, or simply the maximum size of a deflate block.\n> \n> So it's possible that one could hand-roll zlib data that says \"I'm\n> 64GB\" but is only a few bytes long.\n> \n> 2. We don't ever want to see the whole 64GB, of course; we want to\n> stream it out and only care about the header (as an aside, this\n> makes a wonderful test that we are hitting the streaming code path,\n> as it's unlikely to work without it :) ).\n> \n> So another option would be to include a truncated file that claims\n> to be 64GB, and has only the first 256kb or something worth of data\n> (which should deflate down to almost nothing).\n> \n> git-fsck wouldn't work, of course, but we don't need to run it.\n> Other bits of git might complain, but our plan is for git to get\n> SIGPIPE before hitting that point anyway.\n> \n> So that seems pretty easy, but it is potentially flaky.\n\nWriting that convinced me that (2) is actually quite a sane way to go.\nThe patch is below, which seems to work.\n\nI arbitrarily picked the first 2048 bytes of the loose object. That's\n1/32768 of the original. If we assume the compression ratio is stable\nthrough the file (and it should be; the file is all zeroes), that should\ngenerate 2MB of data should we need it (way more than we feed to our\n\"head -c\" invocation).\n\nThis patch is on top of the whole series just to illustrate it. Doing it\nfor real will involve squashing it into the first patch (and adjusting\nthe commit message), and then handling the minor rebase conflicts. I'll\nhold off on a re-roll until I get any comments on v3.\n\n-Peff\n\n---\ndiff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\nindex 07e0bdc..e542938 100755\n--- a/t/t5000-tar-tree.sh\n+++ b/t/t5000-tar-tree.sh\n@@ -339,19 +339,12 @@ test_lazy_prereq TAR_HUGE '\n \ttest_cmp expect actual\n '\n \n-# Likewise, we need bunzip for the 64GB git object.\n-test_lazy_prereq BUNZIP '\n-\tbunzip2 --version\n-'\n-\n-test_expect_success BUNZIP 'set up repository with huge blob' '\n+test_expect_success 'set up repository with huge blob' '\n \tobj_d=19 &&\n \tobj_f=f9c8273ec45a8938e6999cb59b3ff66739902a &&\n \tobj=${obj_d}${obj_f} &&\n \tmkdir -p .git/objects/$obj_d &&\n-\tbunzip2 -c \\\n-\t\t<\"$TEST_DIRECTORY\"/t5000/$obj.bz2 \\\n-\t\t>.git/objects/$obj_d/$obj_f &&\n+\tcp \"$TEST_DIRECTORY\"/t5000/$obj .git/objects/$obj_d/$obj_f &&\n \trm -f .git/index &&\n \tgit update-index --add --cacheinfo 100644,$obj,huge &&\n \tgit commit -m huge\n@@ -359,7 +352,7 @@ test_expect_success BUNZIP 'set up repository with huge blob' '\n \n # We expect git to die with SIGPIPE here (otherwise we\n # would generate the whole 64GB).\n-test_expect_success BUNZIP 'generate tar with huge size' '\n+test_expect_success 'generate tar with huge size' '\n \t{\n \t\tgit archive HEAD\n \t\techo $? >exit-code\n@@ -368,7 +361,7 @@ test_expect_success BUNZIP 'generate tar with huge size' '\n \ttest_cmp expect exit-code\n '\n \n-test_expect_success BUNZIP,TAR_HUGE 'system tar can read our huge size' '\n+test_expect_success TAR_HUGE 'system tar can read our huge size' '\n \techo 68719476737 >expect &&\n \ttar_info huge.tar | cut -d\" \" -f1 >actual &&\n \ttest_cmp expect actual\ndiff --git a/t/t5000/19f9c8273ec45a8938e6999cb59b3ff66739902a b/t/t5000/19f9c8273ec45a8938e6999cb59b3ff66739902a\nnew file mode 100644\nindex 0000000000000000000000000000000000000000..5cbe9ec312bfd7b7e0398ca281e9d42848743704\nGIT binary patch\nliteral 2048\nzcmb=p_2!@^nMd9)O$Wu&Noa%SgnPira`Ve7_Hm1V`0kgE@zR6?%0tYir&brh(3dz7ebDNy(^\nzL0KkKS?}&rK>-rd{r~jXFbzl$fdD`NUjRG-;qPzyn5xj3Xw4Mw$Zz)lx%14F-mTL6atf)Gz?ip`!@NpwXs)%#?y?q!Xf$\nzP8vZs=>+{8*(#7u93>!3K5|fsT$F-(VMr#rK{}}f`lJ&zAez-6nY%u7(h0VZPg+4e\nzX$0a(CY`i`W~3AAPBemQq!ZGRP7dlpJ?RA1nvhKtf@|4GCaOU@$`MOSl~R?f3Ia)6\nzDFpYV6Llb)sRYUuLMe6QNGF9Ln9>Q3G=gtZ37;(>om7H;BohijH-{QkNGI(epUDKq\nQkWcY writes:\n> @@ -413,7 +411,7 @@ static int write_tar_archive(const struct archiver *ar,\n> {\n> int err = 0;\n> \n> - err = write_global_extended_header(args);\n> + write_global_extended_header(args);\n> if (!err)\n> err = write_archive_entries(args, write_tar_entry);\n\nIf we drop the error code from 'write_global_extended_header' then the\nabove 'if (!err)' becomes useless (always evaluates to 'true' since\n'err' is set to '0').\n\n> if (!err)\n\nThanks,\nRémi\n"},{"id":"290018","messageId":"20160624131325.GA28941@sigill.intra.peff.net","threadId":"42622","inReplyTo":"1865881092.716701.1466768964884.JavaMail.zimbra@ensimag.grenoble-inp.fr","subject":"Re: [PATCH v3 4/4] archive-tar: drop return value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-24T13:13:25Z","receivedAt":"2016-06-24T13:13:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 24, 2016 at 01:49:24PM +0200, Remi Galan Alfonso wrote:\n\n> Hi Peff,\n> \n> Jeff King writes:\n> > @@ -413,7 +411,7 @@ static int write_tar_archive(const struct archiver *ar,\n> > {\n> > int err = 0;\n> > \n> > - err = write_global_extended_header(args);\n> > + write_global_extended_header(args);\n> > if (!err)\n> > err = write_archive_entries(args, write_tar_entry);\n> \n> If we drop the error code from 'write_global_extended_header' then the\n> above 'if (!err)' becomes useless (always evaluates to 'true' since\n> 'err' is set to '0').\n\nThanks, you're right.\n\nI wondered if we could drop \"err\" entirely, but write_archive_entries()\ndoes indeed have some error code paths (everybody uses write_or_die, but\nwe return an error for things like unknown file types).\n\n-Peff\n"},{"id":"290027","messageId":"576D621F.1030000@kdbg.org","threadId":"42622","inReplyTo":"20160623232041.GA3668@sigill.intra.peff.net","subject":"Re: [PATCH v3 1/4] t5000: test tar files that overflow ustar headers","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-06-24T16:38:55Z","receivedAt":"2016-06-24T16:39:45Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 24.06.2016 um 01:20 schrieb Jeff King:\n> +# We expect git to die with SIGPIPE here (otherwise we\n> +# would generate the whole 64GB).\n> +test_expect_failure BUNZIP 'generate tar with huge size' '\n> +\t{\n> +\t\tgit archive HEAD\n> +\t\techo $? >exit-code\n> +\t} | head -c 4096 >huge.tar &&\n> +\techo 141 >expect &&\n> +\ttest_cmp expect exit-code\n\nIt's going to be 269 with ksh, and who-knows-what on Windows (due to \nlack of SIGPIPE - I haven't tested this, yet).\n\n-- Hannes\n\n"},{"id":"290029","messageId":"20160624164603.GA13789@sigill.intra.peff.net","threadId":"42622","inReplyTo":"576D621F.1030000@kdbg.org","subject":"Re: [PATCH v3 1/4] t5000: test tar files that overflow ustar headers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-24T16:46:03Z","receivedAt":"2016-06-24T16:46:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 24, 2016 at 06:38:55PM +0200, Johannes Sixt wrote:\n\n> Am 24.06.2016 um 01:20 schrieb Jeff King:\n> > +# We expect git to die with SIGPIPE here (otherwise we\n> > +# would generate the whole 64GB).\n> > +test_expect_failure BUNZIP 'generate tar with huge size' '\n> > +\t{\n> > +\t\tgit archive HEAD\n> > +\t\techo $? >exit-code\n> > +\t} | head -c 4096 >huge.tar &&\n> > +\techo 141 >expect &&\n> > +\ttest_cmp expect exit-code\n> \n> It's going to be 269 with ksh, and who-knows-what on Windows (due to lack of\n> SIGPIPE - I haven't tested this, yet).\n\nThanks, I meant to ask about that. We do a workaround in t0005, but we\n_don't_ do it in the new sigpipe handling for test_must_fail. Is the\nlatter just broken, too?\n\n-Peff\n"},{"id":"290031","messageId":"576D684A.6030406@kdbg.org","threadId":"42622","inReplyTo":"20160624164603.GA13789@sigill.intra.peff.net","subject":"Re: [PATCH v3 1/4] t5000: test tar files that overflow ustar headers","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-06-24T17:05:14Z","receivedAt":"2016-06-24T17:05:22Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 24.06.2016 um 18:46 schrieb Jeff King:\n> On Fri, Jun 24, 2016 at 06:38:55PM +0200, Johannes Sixt wrote:\n>> It's going to be 269 with ksh, and who-knows-what on Windows (due to lack of\n>> SIGPIPE - I haven't tested this, yet).\n>\n> Thanks, I meant to ask about that. We do a workaround in t0005, but we\n> _don't_ do it in the new sigpipe handling for test_must_fail. Is the\n> latter just broken, too?\n\nThat's well possible. It is not prepared to see ksh's exit codes for \nsignals.\n\n-- Hannes\n\n"},{"id":"290040","messageId":"xmqq1t3mh0vg.fsf@gitster.mtv.corp.google.com","threadId":"42622","inReplyTo":"20160623232041.GA3668@sigill.intra.peff.net","subject":"Re: [PATCH v3 1/4] t5000: test tar files that overflow ustar headers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-24T18:56:19Z","receivedAt":"2016-06-24T18:56:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The ustar format only has room for 11 (or 12, depending on\n> some implementations) octal digits for the size and mtime of\n> each file. After this, we have to add pax extended headers\n> to specify the real data, and git does not yet know how to\n> do so.\n\nI am not a native speaker but \"After\" above made me hiccup. I think\nI am correct to understand that it means \"after passing this limit\",\naka \"to represent files bigger or newer than these\", but still it\nfelt somewhat strange.\n\n> So as a prerequisite, we can feed the system tar a reference\n> tarball to make sure it can handle these features. The\n> reference tar here was created with:\n>\n> dd if=/dev/zero seek=64G bs=1 count=1 of=huge\n> touch -d @68719476737 huge\n> tar cf - --format=pax |\n> head -c 2048\n>\n> using GNU tar. Note that this is not a complete tarfile, but\n> it's enough to contain the headers we want to examine.\n\nCute. I didn't remember they had @ format,\neven though I must have seen what they do while working on 2c733fb2\n(parse_date(): '@' prefix forces git-timestamp, 2012-02-02).\n\n> +# See if our system tar can handle a tar file with huge sizes and dates far in\n> +# the future, and that we can actually parse its output.\n> +#\n> +# The reference file was generated by GNU tar, and the magic time and size are\n> +# both octal 01000000000001, which overflows normal ustar fields.\n> +#\n> +# When parsing, we'll pull out only the year from the date; that\n> +# avoids any question of timezones impacting the result. \n\n... as long as the month-day part is not close to the year boundary.\nSo this explanation is insuffucient to convince the reader that\n\"that avoids any question\" is correct, without saying that it is in\nAugust of year 4147.\n\n> +tar_info () {\n> +\t\"$TAR\" tvf \"$1\" | awk '{print $3 \" \" $4}' | cut -d- -f1\n> +}\n\nA blank after the shell function to make it easier to see the\nboundary.\n\nSeeing an awk piped into cut always makes me want to suggest a\nsingle sed/awk/perl invocation.\n\n> +# We expect git to die with SIGPIPE here (otherwise we\n> +# would generate the whole 64GB).\n> +test_expect_failure BUNZIP 'generate tar with huge size' '\n> +\t{\n> +\t\tgit archive HEAD\n> +\t\techo $? >exit-code\n> +\t} | head -c 4096 >huge.tar &&\n> +\techo 141 >expect &&\n> +\ttest_cmp expect exit-code\n> +'\n\n\"head -c\" is GNU-ism, isn't it?\n\n\"dd bs=1 count=4096\" is hopefully more portable.\n\nksh signal death you already know about. I wonder if we want to\nexpose something like list_contains as a friend of test_cmp.\n\n\tlist_contains 141,269 $(cat exit-code)\n\nThanks.\n"},{"id":"290041","messageId":"xmqqwplefm2r.fsf@gitster.mtv.corp.google.com","threadId":"42622","inReplyTo":"20160623232104.GB3668@sigill.intra.peff.net","subject":"Re: [PATCH v3 2/4] archive-tar: write extended headers for file sizes >= 8GB","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-24T19:01:16Z","receivedAt":"2016-06-24T19:01:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> If the size was 64GB or greater, then we actually overflowed\n> digits into the mtime field, meaning our value was was\n\nwas was\n\n> effectively right-shifted by those lost octal digits. And\n> this patch is again a strict improvement over that.\n\n> diff --git a/archive-tar.c b/archive-tar.c\n> index cb99df2..274bdfa 100644\n> --- a/archive-tar.c\n> +++ b/archive-tar.c\n> @@ -137,6 +137,20 @@ static void strbuf_append_ext_header(struct strbuf *sb, const char *keyword,\n> \tstrbuf_addch(sb, '\\n');\n> }\n> \n> +/*\n> + * Like strbuf_append_ext_header, but for numeric values.\n> + */\n> +static void strbuf_append_ext_header_uint(struct strbuf *sb,\n> +\t\t\t\t\t const char *keyword,\n> +\t\t\t\t\t uintmax_t value)\n> +{\n> +\tchar buf[40]; /* big enough for 2^128 in decimal, plus NUL */\n> +\tint len;\n> +\n> +\tlen = xsnprintf(buf, sizeof(buf), \"%\"PRIuMAX, value);\n> +\tstrbuf_append_ext_header(sb, keyword, buf, len);\n> +}\n> +\n> static unsigned int ustar_header_chksum(const struct ustar_header *header)\n> {\n> \tconst unsigned char *p = (const unsigned char *)header;\n> @@ -208,7 +222,7 @@ static int write_tar_entry(struct archiver_args *args,\n> \tstruct ustar_header header;\n> \tstruct strbuf ext_header = STRBUF_INIT;\n> \tunsigned int old_mode = mode;\n> -\tunsigned long size;\n> +\tunsigned long size, size_in_header;\n> \tvoid *buffer;\n> \tint err = 0;\n> \n> @@ -267,7 +281,13 @@ static int write_tar_entry(struct archiver_args *args,\n> \t\t\tmemcpy(header.linkname, buffer, size);\n> \t}\n> \n> -\tprepare_header(args, &header, mode, size);\n> +\tsize_in_header = size;\n> +\tif (S_ISREG(mode) && size > 077777777777UL) {\n\nWant a symbolic constant with a comment that says why you have\neleven 7's?\n\n> +\t\tsize_in_header = 0;\n> +\t\tstrbuf_append_ext_header_uint(&ext_header, \"size\", size);\n> +\t}\n> +\n> +\tprepare_header(args, &header, mode, size_in_header);\n\nThe change itself makes sense.\n\nThanks.\n"},{"id":"290042","messageId":"xmqqshw2flt8.fsf@gitster.mtv.corp.google.com","threadId":"42622","inReplyTo":"20160623232112.GC3668@sigill.intra.peff.net","subject":"Re: [PATCH v3 3/4] archive-tar: write extended headers for far-future mtime","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-24T19:06:59Z","receivedAt":"2016-06-24T19:07:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> There's a slight bit of trickiness there. We may already be\n> ...\n> After writing the extended header, we munge the timestamp in\n> the ustar headers to the maximum-allowable size. This is\n> wrong, but it's the least-wrong thing we can provide to a\n> tar implementation that doesn't understand pax headers (it's\n> also what GNU tar does).\n\nThe above looks very sensible design, and its implementation is\nsurprisingly compact. Very nicely done.\n\n> Helped-by: René Scharfe \n> Signed-off-by: Jeff King \n> ---\n> archive-tar.c | 16 +++++++++++++---\n> t/t5000-tar-tree.sh | 4 ++--\n> 2 files changed, 15 insertions(+), 5 deletions(-)\n>\n> diff --git a/archive-tar.c b/archive-tar.c\n> index 274bdfa..0bb164c 100644\n> --- a/archive-tar.c\n> +++ b/archive-tar.c\n> @@ -317,7 +317,18 @@ static int write_global_extended_header(struct archiver_args *args)\n> \tunsigned int mode;\n> \tint err = 0;\n> \n> -\tstrbuf_append_ext_header(&ext_header, \"comment\", sha1_to_hex(sha1), 40);\n> +\tif (sha1)\n> +\t\tstrbuf_append_ext_header(&ext_header, \"comment\",\n> +\t\t\t\t\t sha1_to_hex(sha1), 40);\n> +\tif (args->time > 077777777777UL) {\n> +\t\tstrbuf_append_ext_header_uint(&ext_header, \"mtime\",\n> +\t\t\t\t\t args->time);\n> +\t\targs->time = 077777777777UL;\n> +\t}\n> +\n> +\tif (!ext_header.len)\n> +\t\treturn 0;\n\nAnother symbolic constant to explain this, e.g. TAR_TIME_LIMIT, may\nwant to exist.\n\nThanks.\n\n\n"},{"id":"290043","messageId":"20160624190744.GA32118@sigill.intra.peff.net","threadId":"42622","inReplyTo":"xmqq1t3mh0vg.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 1/4] t5000: test tar files that overflow ustar headers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-24T19:07:44Z","receivedAt":"2016-06-24T19:07:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 24, 2016 at 11:56:19AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > The ustar format only has room for 11 (or 12, depending on\n> > some implementations) octal digits for the size and mtime of\n> > each file. After this, we have to add pax extended headers\n> > to specify the real data, and git does not yet know how to\n> > do so.\n> \n> I am not a native speaker but \"After\" above made me hiccup. I think\n> I am correct to understand that it means \"after passing this limit\",\n> aka \"to represent files bigger or newer than these\", but still it\n> felt somewhat strange.\n\nYeah, I agree that it reads badly. I'm not sure what I was thinking.\nI'll tweak it in the re-roll.\n\n> > +# See if our system tar can handle a tar file with huge sizes and dates far in\n> > +# the future, and that we can actually parse its output.\n> > +#\n> > +# The reference file was generated by GNU tar, and the magic time and size are\n> > +# both octal 01000000000001, which overflows normal ustar fields.\n> > +#\n> > +# When parsing, we'll pull out only the year from the date; that\n> > +# avoids any question of timezones impacting the result. \n> \n> ... as long as the month-day part is not close to the year boundary.\n> So this explanation is insuffucient to convince the reader that\n> \"that avoids any question\" is correct, without saying that it is in\n> August of year 4147.\n\nI thought that part didn't need to be said, but I can say it\n(technically we can include the month, too, but I don't think that level\nof accuracy is really important for these tests).\n\n> > +tar_info () {\n> > +\t\"$TAR\" tvf \"$1\" | awk '{print $3 \" \" $4}' | cut -d- -f1\n> > +}\n> \n> A blank after the shell function to make it easier to see the\n> boundary.\n\nI was intentionally trying to couple it with prereq below, as the\ncomment describes both of them.\n\n> Seeing an awk piped into cut always makes me want to suggest a\n> single sed/awk/perl invocation.\n\nI want the auto-splitting of awk, but then to auto-split the result\nusing a different delimiter. Is there a not-painful way to do that in\nawk?\n\nI could certainly come up with a regex to do it in sed, but I wanted to\nkeep the parsing as liberal and generic as possible.\n\nCertainly I could do it in perl, but I had the general impression that\nwe prefer to keep the dependency on perl to a minimum. Maybe it doesn't\nmatter.\n\n> > +# We expect git to die with SIGPIPE here (otherwise we\n> > +# would generate the whole 64GB).\n> > +test_expect_failure BUNZIP 'generate tar with huge size' '\n> > +\t{\n> > +\t\tgit archive HEAD\n> > +\t\techo $? >exit-code\n> > +\t} | head -c 4096 >huge.tar &&\n> > +\techo 141 >expect &&\n> > +\ttest_cmp expect exit-code\n> > +'\n> \n> \"head -c\" is GNU-ism, isn't it?\n\nYou're right; for some reason I thought it was in POSIX.\n\nWe do have a couple instances of it, but they are all in the valgrind\nsetup code (which I guess most people don't ever run).\n\n> \"dd bs=1 count=4096\" is hopefully more portable.\n\nHmm. I always wonder whether dd is actually very portable, but we do use\nit already, at least.\n\nPerhaps the perl monstrosity in t9300 could be replaced with that, too.\n\n> ksh signal death you already know about. I wonder if we want to\n> expose something like list_contains as a friend of test_cmp.\n> \n> \tlist_contains 141,269 $(cat exit-code)\n\nI think we would want something more like:\n\n test_signal_match 13 $(cat exit-code)\n\nEach call site should not have to know about every signal convention\n(and in your example, the magic \"3\" of Windows is left out).\n\n-Peff\n"},{"id":"290044","messageId":"20160624191044.GB32118@sigill.intra.peff.net","threadId":"42622","inReplyTo":"xmqqwplefm2r.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 2/4] archive-tar: write extended headers for file sizes >= 8GB","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-24T19:10:44Z","receivedAt":"2016-06-24T19:10:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 24, 2016 at 12:01:16PM -0700, Junio C Hamano wrote:\n\n> > @@ -267,7 +281,13 @@ static int write_tar_entry(struct archiver_args *args,\n> > \t\t\tmemcpy(header.linkname, buffer, size);\n> > \t}\n> > \n> > -\tprepare_header(args, &header, mode, size);\n> > +\tsize_in_header = size;\n> > +\tif (S_ISREG(mode) && size > 077777777777UL) {\n> \n> Want a symbolic constant with a comment that says why you have\n> eleven 7's?\n\nI tried instead to make sure we only mention it once to avoid a symbolic\nconstant (even though the same constant appears in the next patch, too,\nit would be a mistake to give them the same name; they just happen to be\nthe same size).\n\nSo if anything, I would put a comment here, explaining that ustar cannot\nhandle anything larger than this, and POSIX mandates it (but I didn't\nbecause the commit message already goes into much more detail).\n\n-Peff\n"},{"id":"290045","messageId":"xmqqoa6qflmz.fsf@gitster.mtv.corp.google.com","threadId":"42622","inReplyTo":"20160624131325.GA28941@sigill.intra.peff.net","subject":"Re: [PATCH v3 4/4] archive-tar: drop return value","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-24T19:10:44Z","receivedAt":"2016-06-24T19:10:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Jun 24, 2016 at 01:49:24PM +0200, Remi Galan Alfonso wrote:\n>\n>> Hi Peff,\n>> \n>> Jeff King writes:\n>> > @@ -413,7 +411,7 @@ static int write_tar_archive(const struct archiver *ar,\n>> > {\n>> > int err = 0;\n>> > \n>> > - err = write_global_extended_header(args);\n>> > + write_global_extended_header(args);\n>> > if (!err)\n>> > err = write_archive_entries(args, write_tar_entry);\n>> \n>> If we drop the error code from 'write_global_extended_header' then the\n>> above 'if (!err)' becomes useless (always evaluates to 'true' since\n>> 'err' is set to '0').\n>\n> Thanks, you're right.\n>\n> I wondered if we could drop \"err\" entirely, but write_archive_entries()\n> does indeed have some error code paths (everybody uses write_or_die, but\n> we return an error for things like unknown file types).\n\nYou could if you did this ;-)\n\nWhich I do not necessarily think is a good idea.\n\nThis may be easier to read\n\n\twrite_global_extended_header(args)\n err = write_archive_entries(args, write_tar_entry);\n if (!err)\n\t\twrite_trailer();\n\treturn err;\n\nthough.\n\n archive-tar.c | 8 +++-----\n 1 file changed, 3 insertions(+), 5 deletions(-)\n\ndiff --git a/archive-tar.c b/archive-tar.c\nindex 903b74d..eed2c96 100644\n--- a/archive-tar.c\n+++ b/archive-tar.c\n@@ -412,11 +412,9 @@ static int write_tar_archive(const struct archiver *ar,\n \tint err = 0;\n \n \twrite_global_extended_header(args);\n-\tif (!err)\n-\t\terr = write_archive_entries(args, write_tar_entry);\n-\tif (!err)\n-\t\twrite_trailer();\n-\treturn err;\n+\n+\treturn (write_archive_entries(args, write_tar_entry) ||\n+\t\twrite_trailer());\n }\n \n static int write_tar_filter_archive(const struct archiver *ar,\n"},{"id":"290046","messageId":"20160624191648.GC32118@sigill.intra.peff.net","threadId":"42622","inReplyTo":"xmqqshw2flt8.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 3/4] archive-tar: write extended headers for far-future mtime","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-24T19:16:48Z","receivedAt":"2016-06-24T19:16:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 24, 2016 at 12:06:59PM -0700, Junio C Hamano wrote:\n\n> > +\tif (args->time > 077777777777UL) {\n> > +\t\tstrbuf_append_ext_header_uint(&ext_header, \"mtime\",\n> > +\t\t\t\t\t args->time);\n> > +\t\targs->time = 077777777777UL;\n> > +\t}\n> > +\n> > +\tif (!ext_header.len)\n> > +\t\treturn 0;\n> \n> Another symbolic constant to explain this, e.g. TAR_TIME_LIMIT, may\n> want to exist.\n\nThis one at least appears twice. I think one of the reasons I am\nslightly resistant to a symbolic constant is that it tempts people to\nthink that it's OK to change it. It's not. These values are mandated by\nPOSIX, and must match the size of the ustar header field.\n\nSo the least-repetitive thing would be to define it as:\n\n (1UL << (1 + (3 * (sizeof(ustar_header.mtime) - 1)))) - 1\n\nThat's pretty horrible to read, but if wrapped in a symbolic constant,\nat least people would think twice before touching it. ;)\n\n-Peff\n"},{"id":"290048","messageId":"20160624193924.GA6282@sigill.intra.peff.net","threadId":"42622","inReplyTo":"576D684A.6030406@kdbg.org","subject":"[PATCH 0/4] portable signal-checking in tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-24T19:39:24Z","receivedAt":"2016-06-24T19:39:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 24, 2016 at 07:05:14PM +0200, Johannes Sixt wrote:\n\n> Am 24.06.2016 um 18:46 schrieb Jeff King:\n> > On Fri, Jun 24, 2016 at 06:38:55PM +0200, Johannes Sixt wrote:\n> > > It's going to be 269 with ksh, and who-knows-what on Windows (due to lack of\n> > > SIGPIPE - I haven't tested this, yet).\n> > \n> > Thanks, I meant to ask about that. We do a workaround in t0005, but we\n> > _don't_ do it in the new sigpipe handling for test_must_fail. Is the\n> > latter just broken, too?\n> \n> That's well possible. It is not prepared to see ksh's exit codes for\n> signals.\n\nI'm actually not convinced that old versions of ksh are viable for\nrunning the test suite. mksh seems to use POSIX semantics, and I cannot\nget through t0005 with ksh93, as it parses nested parentheses wrong. But\nmaybe there are other ksh variants that use the funny exit codes, but\nare otherwise not too buggy.\n\nI'd be more concerned with Windows. The SIGPIPE tests in t0005 are\nalready marked !MINGW, but other checks elsewhere are not. I know there\nis no SIGPIPE on Windows, so it may be that some cases happen to work\nbecause we end up in write_or_die(), which converts EPIPE into a 141\nexit.\n\nAnyway. Here's a series that I think makes things better, and it is not\ntoo painful to do.\n\n [1/4]: tests: factor portable signal check out of t0005\n [2/4]: t0005: use test_match_signal as appropriate\n [3/4]: test_must_fail: use test_match_signal\n [4/4]: t/lib-git-daemon: use test_match_signal\n\n-Peff\n"},{"id":"290049","messageId":"20160624194357.GA6441@sigill.intra.peff.net","threadId":"42622","inReplyTo":"20160624193924.GA6282@sigill.intra.peff.net","subject":"[PATCH 1/4] tests: factor portable signal check out of t0005","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-24T19:43:58Z","receivedAt":"2016-06-24T19:44:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In POSIX shells, a program which exits due to a signal\ngenerally has an exit code of 128 plus the signal number.\nHowever, some platforms do other things. ksh uses 256 plus\nthe signal number, and on Windows, all signals are just \"3\".\n\nWe've accounted for that in t0005, but not in other tests.\nLet's pull out the logic so we can use it elsewhere.\n\nIt would be nice for debugging if this additionally printed\nerrors to stderr, like our other test_* helpers. But we're\ngoing to need to use it in other places besides the innards\nof a test_expect block. So let's leave it as generic as\npossible.\n\nSigned-off-by: Jeff King \n---\nI didn't get into the weirdness of SIGPIPE on Windows here, but I think\nthis is probably a first step toward handling it better. E.g., it may be\nthat test_match_signal should respect 128 (or even any code) when we are\nchecking for SIGPIPE.\n\nI also didn't bother with symbolic names. We could make:\n\n test_match_signal sigterm $?\n\nwork, but I didn't think it was worth the effort. While numbers for some\nobscure signals do vary on platforms, sigpipe and sigterm are standard\nenough to rely on.\n\n t/t0005-signals.sh | 7 +------\n t/test-lib-functions.sh | 18 ++++++++++++++++++\n 2 files changed, 19 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t0005-signals.sh b/t/t0005-signals.sh\nindex e7f27eb..2d96265 100755\n--- a/t/t0005-signals.sh\n+++ b/t/t0005-signals.sh\n@@ -11,12 +11,7 @@ EOF\n \n test_expect_success 'sigchain works' '\n \t{ test-sigchain >actual; ret=$?; } &&\n-\tcase \"$ret\" in\n-\t143) true ;; # POSIX w/ SIGTERM=15\n-\t271) true ;; # ksh w/ SIGTERM=15\n-\t 3) true ;; # Windows\n-\t *) false ;;\n-\tesac &&\n+\ttest_match_signal 15 \"$ret\" &&\n \ttest_cmp expect actual\n '\n \ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 48884d5..51d3775 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -961,3 +961,21 @@ test_env () {\n \t\tdone\n \t)\n }\n+\n+# Returns true if the numeric exit code in \"$2\" represents the expected signal\n+# in \"$1\". Signals should be given numerically.\n+test_match_signal () {\n+\tif test \"$2\" = \"$((128 + $1))\"\n+\tthen\n+\t\t# POSIX\n+\t\treturn 0\n+\telif test \"$2\" = \"$((256 + $1))\"\n+\tthen\n+\t\t# ksh\n+\t\treturn 0\n+\telif test \"$2\" = \"3\"; then\n+\t\t# Windows\n+\t\treturn 0\n+\tfi\n+\treturn 1\n+}\n-- \n2.9.0.215.gc5c4261\n\n"},{"id":"290050","messageId":"xmqqk2hefk3f.fsf@gitster.mtv.corp.google.com","threadId":"42622","inReplyTo":"20160624190744.GA32118@sigill.intra.peff.net","subject":"Re: [PATCH v3 1/4] t5000: test tar files that overflow ustar headers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-24T19:44:04Z","receivedAt":"2016-06-24T19:44:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> > +# When parsing, we'll pull out only the year from the date; that\n>> > +# avoids any question of timezones impacting the result. \n>> \n>> ... as long as the month-day part is not close to the year boundary.\n>> So this explanation is insuffucient to convince the reader that\n>> \"that avoids any question\" is correct, without saying that it is in\n>> August of year 4147.\n>\n> I thought that part didn't need to be said, but I can say it\n> (technically we can include the month, too, but I don't think that level\n> of accuracy is really important for these tests).\n\nOh, I wasn't suggesting to include the month in the comparison. But\nto understand why it is safe from TZ jitters to test only year, the\nreader needs to know (or do the math herself) that the timestamp is\naway from the year boundary, so mentioning August in the justifying\ncomment is needed.\n\n>> Seeing an awk piped into cut always makes me want to suggest a\n>> single sed/awk/perl invocation.\n>\n> I want the auto-splitting of awk, but then to auto-split the result\n> using a different delimiter. Is there a not-painful way to do that in\n> awk?\n>\n> I could certainly come up with a regex to do it in sed, but I wanted to\n> keep the parsing as liberal and generic as possible.\n>\n> Certainly I could do it in perl, but I had the general impression that\n> we prefer to keep the dependency on perl to a minimum. Maybe it doesn't\n> matter.\n\nHeh. It was merely \"makes me want to suggest\", not \"I suggest\". If\nI were doing this myself, I would have done a single sed but it does\nnot matter.\n\n> I think we would want something more like:\n>\n> test_signal_match 13 $(cat exit-code)\n\nI like that.\n"},{"id":"290051","messageId":"20160624194450.GB6441@sigill.intra.peff.net","threadId":"42622","inReplyTo":"20160624193924.GA6282@sigill.intra.peff.net","subject":"[PATCH 2/4] t0005: use test_match_signal as appropriate","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-24T19:44:50Z","receivedAt":"2016-06-24T19:44:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The first test already uses this more portable construct\n(that was where it was factored from initially), but the\nlater tests do a raw comparison against 141 to look for\nSIGPIPE, which can fail on some shells and platforms.\n\nSigned-off-by: Jeff King \n---\nAgain, I couldn't test these. They're skipped on MINGW, and ksh93 barfs\nbefore even executing the tests.\n\n t/t0005-signals.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t0005-signals.sh b/t/t0005-signals.sh\nindex 2d96265..c80c995 100755\n--- a/t/t0005-signals.sh\n+++ b/t/t0005-signals.sh\n@@ -36,12 +36,12 @@ test_expect_success 'create blob' '\n \n test_expect_success !MINGW 'a constipated git dies with SIGPIPE' '\n \tOUT=$( ((large_git; echo $? 1>&3) | :) 3>&1 ) &&\n-\ttest \"$OUT\" -eq 141\n+\ttest_match_signal 13 \"$OUT\"\n '\n \n test_expect_success !MINGW 'a constipated git dies with SIGPIPE even if parent ignores it' '\n \tOUT=$( ((trap \"\" PIPE; large_git; echo $? 1>&3) | :) 3>&1 ) &&\n-\ttest \"$OUT\" -eq 141\n+\ttest_match_signal 13 \"$OUT\"\n '\n \n test_done\n-- \n2.9.0.215.gc5c4261\n\n"},{"id":"290053","messageId":"xmqqfus2fk1q.fsf@gitster.mtv.corp.google.com","threadId":"42622","inReplyTo":"20160624191044.GB32118@sigill.intra.peff.net","subject":"Re: [PATCH v3 2/4] archive-tar: write extended headers for file sizes >= 8GB","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-24T19:45:05Z","receivedAt":"2016-06-24T20:02:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So if anything, I would put a comment here, explaining that ustar cannot\n> handle anything larger than this, and POSIX mandates it (but I didn't\n> because the commit message already goes into much more detail).\n\nThat sounds like a good thing to do. Not everybody runs \"blame\" to\nfind the commit whose log message she must read before touching a\nline.\n\n\n"},{"id":"290054","messageId":"20160624194504.GC6441@sigill.intra.peff.net","threadId":"42622","inReplyTo":"20160624193924.GA6282@sigill.intra.peff.net","subject":"[PATCH 3/4] test_must_fail: use test_match_signal","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-24T19:45:04Z","receivedAt":"2016-06-24T20:02:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In 8bf4bec (add \"ok=sigpipe\" to test_must_fail and use it to\nfix flaky tests, 2015-11-27), test_must_fail learned to\nrecognize \"141\" as a sigpipe failure. However, testing for\na signal is more complicated than that; we should use\ntest_match_signal to implement more portable checking.\n\nSigned-off-by: Jeff King \n---\n t/test-lib-functions.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 51d3775..91d3b58 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -612,7 +612,7 @@ test_must_fail () {\n \tthen\n \t\techo >&2 \"test_must_fail: command succeeded: $*\"\n \t\treturn 1\n-\telif test $exit_code -eq 141 && list_contains \"$_test_ok\" sigpipe\n+\telif test_match_signal 13 $exit_code && list_contains \"$_test_ok\" sigpipe\n \tthen\n \t\treturn 0\n \telif test $exit_code -gt 129 && test $exit_code -le 192\n-- \n2.9.0.215.gc5c4261\n\n"},{"id":"290055","messageId":"20160624194606.GA6521@sigill.intra.peff.net","threadId":"42622","inReplyTo":"xmqqfus2fk1q.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 2/4] archive-tar: write extended headers for file sizes >= 8GB","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-24T19:46:07Z","receivedAt":"2016-06-24T20:02:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 24, 2016 at 12:45:05PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > So if anything, I would put a comment here, explaining that ustar cannot\n> > handle anything larger than this, and POSIX mandates it (but I didn't\n> > because the commit message already goes into much more detail).\n> \n> That sounds like a good thing to do. Not everybody runs \"blame\" to\n> find the commit whose log message she must read before touching a\n> line.\n\nWell, they should. My commit messages are works of literature. ;)\n\nI'll add a comment.\n\n-Peff\n"},{"id":"290056","messageId":"20160624194512.GD6441@sigill.intra.peff.net","threadId":"42622","inReplyTo":"20160624193924.GA6282@sigill.intra.peff.net","subject":"[PATCH 4/4] t/lib-git-daemon: use test_match_signal","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-24T19:45:12Z","receivedAt":"2016-06-24T20:02:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When git-daemon exits, we expect it to be with the SIGTERM\nwe just sent it. If we see anything else, we'll complain.\nBut our check against exit code \"143\" is not portable. For\nexample:\n\n $ ksh93 t5570-git-daemon.sh\n [...]\n error: git daemon exited with status: 271\n\nWe can fix this by using test_match_signal.\n\nSigned-off-by: Jeff King \n---\n t/lib-git-daemon.sh | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh\nindex 340534c..f9cbd47 100644\n--- a/t/lib-git-daemon.sh\n+++ b/t/lib-git-daemon.sh\n@@ -82,8 +82,7 @@ stop_git_daemon() {\n \tkill \"$GIT_DAEMON_PID\"\n \twait \"$GIT_DAEMON_PID\" >&3 2>&4\n \tret=$?\n-\t# expect exit with status 143 = 128+15 for signal TERM=15\n-\tif test $ret -ne 143\n+\tif test_match_signal 15 $?\n \tthen\n \t\terror \"git daemon exited with status: $ret\"\n \tfi\n-- \n2.9.0.215.gc5c4261\n"},{"id":"290057","messageId":"20160624194819.GB6282@sigill.intra.peff.net","threadId":"42622","inReplyTo":"20160624193924.GA6282@sigill.intra.peff.net","subject":"Re: [PATCH 0/4] portable signal-checking in tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-24T19:48:19Z","receivedAt":"2016-06-24T20:02:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 24, 2016 at 03:39:24PM -0400, Jeff King wrote:\n\n> Anyway. Here's a series that I think makes things better, and it is not\n> too painful to do.\n> \n> [1/4]: tests: factor portable signal check out of t0005\n> [2/4]: t0005: use test_match_signal as appropriate\n> [3/4]: test_must_fail: use test_match_signal\n> [4/4]: t/lib-git-daemon: use test_match_signal\n\nOh, and I meant to mention: this covers everything I found grepping for\n\"141\" and \"143\" in the test suite (though you have to filter the results\na bit for false positives).\n\nIt doesn't fix the case newly added in the tar series that started this\ndiscussion. I don't want to hold either topic hostage to the other, so\nI'll prepare a patch to go on top.\n\n-Peff\n"},{"id":"290062","messageId":"576D9D90.3070605@kdbg.org","threadId":"42622","inReplyTo":"20160624194357.GA6441@sigill.intra.peff.net","subject":"Re: [PATCH 1/4] tests: factor portable signal check out of t0005","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-06-24T20:52:32Z","receivedAt":"2016-06-24T20:52:40Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 24.06.2016 um 21:43 schrieb Jeff King:\n> In POSIX shells, a program which exits due to a signal\n> generally has an exit code of 128 plus the signal number.\n> However, some platforms do other things. ksh uses 256 plus\n> the signal number, and on Windows, all signals are just \"3\".\n\nThat's not true, see below.\n\n> I didn't get into the weirdness of SIGPIPE on Windows here, but I think\n> this is probably a first step toward handling it better. E.g., it may be\n> that test_match_signal should respect 128 (or even any code) when we are\n> checking for SIGPIPE.\n\nThe Windows behavior is most closely described as having signal(SIGPIPE, \nSIG_IGN) at the very beginning of the program.\n\n> +# Returns true if the numeric exit code in \"$2\" represents the expected signal\n> +# in \"$1\". Signals should be given numerically.\n> +test_match_signal () {\n> +\tif test \"$2\" = \"$((128 + $1))\"\n> +\tthen\n> +\t\t# POSIX\n> +\t\treturn 0\n> +\telif test \"$2\" = \"$((256 + $1))\"\n> +\tthen\n> +\t\t# ksh\n> +\t\treturn 0\n> +\telif test \"$2\" = \"3\"; then\n> +\t\t# Windows\n\nYou meant well here, but this is close to pointless as a general check. \nWe check for this exit code in t0005 because there program termination \nhappens via raise(), which on Window just calls exit(3). This exit code \nis not an indication that something related to SIGPIPE (or any signal) \nhappened.\n\nIMO there is too much danger to trigger a false positive if exit code 3 \nis treated special in this generality.\n\n> +\t\treturn 0\n> +\tfi\n> +\treturn 1\n> +}\n\n-- Hannes\n\n"},{"id":"290064","messageId":"CAPig+cRD43DnaiZrT4ODnT7Nm5J8q=MhtSeyK3ZNjwaTW8mdbg@mail.gmail.com","threadId":"42622","inReplyTo":"20160624190744.GA32118@sigill.intra.peff.net","subject":"Re: [PATCH v3 1/4] t5000: test tar files that overflow ustar headers","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-06-24T20:58:58Z","receivedAt":"2016-06-24T20:59:04Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jun 24, 2016 at 3:07 PM, Jeff King wrote:\n> On Fri, Jun 24, 2016 at 11:56:19AM -0700, Junio C Hamano wrote:\n>> Jeff King writes:\n>> > +tar_info () {\n>> > + \"$TAR\" tvf \"$1\" | awk '{print $3 \" \" $4}' | cut -d- -f1\n>> > +}\n>\n>> Seeing an awk piped into cut always makes me want to suggest a\n>> single sed/awk/perl invocation.\n>\n> I want the auto-splitting of awk, but then to auto-split the result\n> using a different delimiter. Is there a not-painful way to do that in\n> awk?\n\nThe awk split() function is POSIX and accepts an optional separator argument.\n"},{"id":"290065","messageId":"20160624205858.GA23315@sigill.intra.peff.net","threadId":"42622","inReplyTo":"20160624190744.GA32118@sigill.intra.peff.net","subject":"Re: [PATCH v3 1/4] t5000: test tar files that overflow ustar headers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-24T20:58:58Z","receivedAt":"2016-06-24T20:59:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 24, 2016 at 03:07:44PM -0400, Jeff King wrote:\n\n> > \"dd bs=1 count=4096\" is hopefully more portable.\n> \n> Hmm. I always wonder whether dd is actually very portable, but we do use\n> it already, at least.\n> \n> Perhaps the perl monstrosity in t9300 could be replaced with that, too.\n\nHrm. So I wrote a patch for t9300 for this. But I wanted to flip the\norder to:\n\n dd bs=4096 count=1\n\nbecause otherwise, dd will call read() 4096 times, for 1 byte each.\n\nBut it's not safe to do that on a pipe. For example:\n\n {\n\techo 1\n\tsleep 1\n\techo 2\n } | dd bs=4 count=1\n\nwill copy only 2 bytes. So it's racily wrong, depending on how the\nwriter feeds the data to write().\n\nThe 1-byte reads do work (assuming blocking descriptors and that dd\nrestarts a read after a signal, which mine seems to). But yuck.\n\nThe difference in time between the two is measurable on my system, but\nit's only a few milliseconds (for 4096 bytes). So maybe it's not worth\nworrying about (though as a general technique, it does make me worry\nthat it's easy to get wrong in a way that will fail racily).\n\n-Peff\n"},{"id":"290066","messageId":"20160624210541.GC6282@sigill.intra.peff.net","threadId":"42622","inReplyTo":"576D9D90.3070605@kdbg.org","subject":"Re: [PATCH 1/4] tests: factor portable signal check out of t0005","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-24T21:05:41Z","receivedAt":"2016-06-24T21:05:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 24, 2016 at 10:52:32PM +0200, Johannes Sixt wrote:\n\n> Am 24.06.2016 um 21:43 schrieb Jeff King:\n> > In POSIX shells, a program which exits due to a signal\n> > generally has an exit code of 128 plus the signal number.\n> > However, some platforms do other things. ksh uses 256 plus\n> > the signal number, and on Windows, all signals are just \"3\".\n> \n> That's not true, see below.\n\nI was worried about that. Git for Windows seems like a labyrinth of\nbizarre special cases.\n\n> > I didn't get into the weirdness of SIGPIPE on Windows here, but I think\n> > this is probably a first step toward handling it better. E.g., it may be\n> > that test_match_signal should respect 128 (or even any code) when we are\n> > checking for SIGPIPE.\n> \n> The Windows behavior is most closely described as having signal(SIGPIPE,\n> SIG_IGN) at the very beginning of the program.\n\nRight, but then we would get EPIPE. So what does git do in such cases?\nI'd expect it generally to either hit the check_pipe() part of\nwrite_or_die(), or to end up complaining via die() that the write didn't\ngo as expected.\n\n> > +# Returns true if the numeric exit code in \"$2\" represents the expected signal\n> > +# in \"$1\". Signals should be given numerically.\n> > +test_match_signal () {\n> > +\tif test \"$2\" = \"$((128 + $1))\"\n> > +\tthen\n> > +\t\t# POSIX\n> > +\t\treturn 0\n> > +\telif test \"$2\" = \"$((256 + $1))\"\n> > +\tthen\n> > +\t\t# ksh\n> > +\t\treturn 0\n> > +\telif test \"$2\" = \"3\"; then\n> > +\t\t# Windows\n> \n> You meant well here, but this is close to pointless as a general check. We\n> check for this exit code in t0005 because there program termination happens\n> via raise(), which on Window just calls exit(3). This exit code is not an\n> indication that something related to SIGPIPE (or any signal) happened.\n> \n> IMO there is too much danger to trigger a false positive if exit code 3 is\n> treated special in this generality.\n\nYeah, I agree. But what _should_ it do? E.g., what happens to git-daemon\nwhen it is killed via TERM?\n\n-Peff\n"},{"id":"290067","messageId":"20160624210906.GD6282@sigill.intra.peff.net","threadId":"42622","inReplyTo":"CAPig+cRD43DnaiZrT4ODnT7Nm5J8q=MhtSeyK3ZNjwaTW8mdbg@mail.gmail.com","subject":"Re: [PATCH v3 1/4] t5000: test tar files that overflow ustar headers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-24T21:09:06Z","receivedAt":"2016-06-24T21:09:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 24, 2016 at 04:58:58PM -0400, Eric Sunshine wrote:\n\n> On Fri, Jun 24, 2016 at 3:07 PM, Jeff King wrote:\n> > On Fri, Jun 24, 2016 at 11:56:19AM -0700, Junio C Hamano wrote:\n> >> Jeff King writes:\n> >> > +tar_info () {\n> >> > + \"$TAR\" tvf \"$1\" | awk '{print $3 \" \" $4}' | cut -d- -f1\n> >> > +}\n> >\n> >> Seeing an awk piped into cut always makes me want to suggest a\n> >> single sed/awk/perl invocation.\n> >\n> > I want the auto-splitting of awk, but then to auto-split the result\n> > using a different delimiter. Is there a not-painful way to do that in\n> > awk?\n> \n> The awk split() function is POSIX and accepts an optional separator argument.\n\nThanks. I'm not that familiar with awk functions, simply because I came\nof age after perl existed, and using perl tends to be more portable and\npowerful (if you can assume it's available). But this is simple enough\nthat it should be OK.\n\nReplacing it with:\n\n \"$TAR\" tvf \"$1\" |\n awk '{\n split($4, date, \"-\")\n print $3 \" \" date[1]\n }'\n\nseems to work.\n\n-Peff\n"},{"id":"290068","messageId":"576DA6FB.1050108@kdbg.org","threadId":"42622","inReplyTo":"20160624210541.GC6282@sigill.intra.peff.net","subject":"Re: [PATCH 1/4] tests: factor portable signal check out of t0005","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-06-24T21:32:43Z","receivedAt":"2016-06-24T21:33:10Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 24.06.2016 um 23:05 schrieb Jeff King:\n> On Fri, Jun 24, 2016 at 10:52:32PM +0200, Johannes Sixt wrote:\n>> The Windows behavior is most closely described as having signal(SIGPIPE,\n>> SIG_IGN) at the very beginning of the program.\n>\n> Right, but then we would get EPIPE. So what does git do in such cases?\n> I'd expect it generally to either hit the check_pipe() part of\n> write_or_die(), or to end up complaining via die() that the write didn't\n> go as expected.\n\nAh, I have forgotten about this code path. Looks like it will trigger \nexactly the same raise() as test_sigchain. Then the check for exit code \n3 makes a bit more sense. But I'm sure we have code paths that do not go \nthrough checK_pipe(). Those would proceed through whatever error \nhandling we have and most likely die().\n\n>> IMO there is too much danger to trigger a false positive if exit code 3 is\n>> treated special in this generality.\n>\n> Yeah, I agree. But what _should_ it do? E.g., what happens to git-daemon\n> when it is killed via TERM?\n\nFrankly, I don't know how bash's 'kill -TERM' and a Windows program \ninteract. I've avoided this topic like the plague so far.\n\n-- Hannes\n\n"},{"id":"290074","messageId":"xmqqk2hedxas.fsf@gitster.mtv.corp.google.com","threadId":"42622","inReplyTo":"20160624205858.GA23315@sigill.intra.peff.net","subject":"Re: [PATCH v3 1/4] t5000: test tar files that overflow ustar headers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-24T22:41:47Z","receivedAt":"2016-06-24T22:41:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The difference in time between the two is measurable on my system, but\n> it's only a few milliseconds (for 4096 bytes). So maybe it's not worth\n> worrying about (though as a general technique, it does make me worry\n> that it's easy to get wrong in a way that will fail racily).\n\nYeah, GNU dd has iflag=fullblock, but if we assume GNU, we can\nsafely assume \"head -c\", so I do not think of a way to do this\nportably enough.\n\n"},{"id":"290088","messageId":"20160624232206.GA28664@sigill.intra.peff.net","threadId":"42622","inReplyTo":"xmqqk2hedxas.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 1/4] t5000: test tar files that overflow ustar headers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-24T23:22:06Z","receivedAt":"2016-06-24T23:22:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 24, 2016 at 03:41:47PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > The difference in time between the two is measurable on my system, but\n> > it's only a few milliseconds (for 4096 bytes). So maybe it's not worth\n> > worrying about (though as a general technique, it does make me worry\n> > that it's easy to get wrong in a way that will fail racily).\n> \n> Yeah, GNU dd has iflag=fullblock, but if we assume GNU, we can\n> safely assume \"head -c\", so I do not think of a way to do this\n> portably enough.\n\nThinking on it more, \"head -c\" is _not_ what one would want in all\ncases. It would work here, but not in t9300, for instance, where the\ncode is trying to read an exact number of bytes from a fifo. I don't\nthink \"head\" makes any promises about buffering and may read extra\nbytes.\n\nSo I dunno. \"dd\" generally does make such promises, or perhaps the perl\nsysread() solution in t9300 is not so bad.\n\n-Peff\n"}]}