{"thread":{"id":"56792","subject":"[PATCH 0/5] Allow clean/smudge filters to handle huge files in the LLP64 data model","startedAt":"2021-10-27T07:49:17Z","lastAt":"2021-11-04T17:26:59Z","messageCount":78,"participants":["Johannes Schindelin via GitGitGadget","Matt Cooper via GitGitGadget","Carlo Arenas","Carlo Marcelo Arenas Belón","Johannes Schindelin","Carlo Marcelo Arenas Belón via GitGitGadget","brian m. carlson","Junio C Hamano","Philip Oakley","Torsten Bögershausen","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"439712","messageId":"pull.1068.git.1635320952.gitgitgadget@gmail.com","threadId":"56792","inReplyTo":null,"subject":"[PATCH 0/5] Allow clean/smudge filters to handle huge files in the LLP64 data model","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-27T07:49:07Z","receivedAt":"2021-10-27T07:49:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This patch series came in via the Git for Windows fork\n[https://github.com/git-for-windows/git/pull/3487], and I intend to merge it\nbefore v2.34.0-rc0, therefore I appreciate every careful review you gentle\npeople can spare.\n\nThe x86_64 variant of Windows uses the LLP64 data model, where the long data\ntype is 32-bit. This is very different from the LP64 data model used e.g. by\nx86_64 Linux, where unsigned long is 64-bit.\n\nMost notably, this means that sizeof(unsigned long) != sizeof(size_t) in\ngeneral.\n\nHowever, since Git was born in the Linux ecosystem, where that inequality\ndoes not hold true, it is understandable that unsigned long is used in many\ncode locations where size_t should have been used. As a consequence, quite a\nfew things are broken e.g. on Windows, when it comes to 4GB file contents or\nlarger.\n\nUsing Git LFS [https://git-lfs.github.io/] trying to work around such issues\nis one such a broken scenario. You cannot git checkout, say, 5GB files. Huge\nfiles will be truncated to whatever the file size is modulo 4GB (in the case\nof a 5GB file, it would be truncated to 1GB).\n\nThis patch series primarily fixes the Git LFS scenario, by allowing clean\nfilters to accept 5GB files, and by allowing smudge filters to produce 5GB\nfiles.\n\nThe much larger project to teach Git to use size_t instead of unsigned long\nin all the appropriate places is hardly scratched by this patch series.\n\nSide note: The fix for the clean filter included in this series does not\nactually affect Git LFS! The reason is that Git LFS marks its filter as\nrequired, and therefore Git streams the file contents to Git LFS via a file\ndescriptor (which is unaffected by LLP64). A \"clean\" filter that is not\nmarked as required, however, lets Git take the code path that is fixed by\nthis patch series.\n\nJohannes Schindelin (1):\n git-compat-util: introduce more size_t helpers\n\nMatt Cooper (4):\n t1051: introduce a smudge filter test for extremely large files\n odb: teach read_blob_entry to use size_t\n odb: guard against data loss checking out a huge file\n clean/smudge: allow clean filters to process extremely large files\n\n convert.c | 2 +-\n delta.h | 6 +++---\n entry.c | 8 +++++---\n entry.h | 2 +-\n git-compat-util.h | 25 +++++++++++++++++++++++++\n object-file.c | 6 +++---\n packfile.c | 6 +++---\n parallel-checkout.c | 2 +-\n t/t1051-large-conversion.sh | 22 ++++++++++++++++++++++\n 9 files changed, 64 insertions(+), 15 deletions(-)\n\n\nbase-commit: ebf3c04b262aa27fbb97f8a0156c2347fecafafb\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1068%2Fdscho%2Fhuge-file-smudge-clean-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1068/dscho/huge-file-smudge-clean-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1068\n-- \ngitgitgadget\n"},{"id":"439713","messageId":"449eb5c205e139e21b619c4eb975afc3d47427f3.1635320952.git.gitgitgadget@gmail.com","threadId":"56792","inReplyTo":"pull.1068.git.1635320952.gitgitgadget@gmail.com","subject":"[PATCH 1/5] t1051: introduce a smudge filter test for extremely large files","fromName":"Matt Cooper via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-27T07:49:08Z","receivedAt":"2021-10-27T07:49:18Z","isPatch":true,"sender":{"key":"name:Matt Cooper","avatar":null},"body":"From: Matt Cooper \n\nThe filter system allows for alterations to file contents when they're\nadded to the database or workdir. (\"Smudge\" when moving to the workdir;\n\"clean\" when moving to the database.) This is used natively to handle CRLF\nto LF conversions. It's also employed by Git-LFS to replace large files\nfrom the workdir with small tracking files in the repo and vice versa.\n\nGit pulls the entire smudged file into memory. While this is inefficient,\nthere's a more insidious problem on some platforms due to inconsistency\nbetween using unsigned long and size_t for the same type of data (size of\na file in bytes). On most 64-bit platforms, unsigned long is 64 bits, and\nsize_t is typedef'd to unsigned long. On Windows, however, unsigned long is\nonly 32 bits (and therefore on 64-bit Windows, size_t is typedef'd to\nunsigned long long in order to be 64 bits).\n\nPractically speaking, this means 64-bit Windows users of Git-LFS can't\nhandle files larger than 2^32 bytes. Other 64-bit platforms don't suffer\nthis limitation.\n\nThis commit introduces a test exposing the issue; future commits make it\npass. The test simulates the way Git-LFS works by having a tiny file\nchecked into the repository and expanding it to a huge file on checkout.\n\nHelped-by: Johannes Schindelin \nSigned-off-by: Matt Cooper \nSigned-off-by: Johannes Schindelin \n---\n t/t1051-large-conversion.sh | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git a/t/t1051-large-conversion.sh b/t/t1051-large-conversion.sh\nindex 8b7640b3ba8..684ba5bd0a5 100755\n--- a/t/t1051-large-conversion.sh\n+++ b/t/t1051-large-conversion.sh\n@@ -83,4 +83,16 @@ test_expect_success 'ident converts on output' '\n \ttest_cmp small.clean large.clean\n '\n \n+# This smudge filter prepends 5GB of zeros to the file it checks out. This\n+# ensures that smudging doesn't mangle large files on 64-bit Windows.\n+test_expect_failure EXPENSIVE,!LONG_IS_64BIT 'files over 4GB convert on output' '\n+\ttest_commit test small \"a small file\" &&\n+\ttest_config filter.makelarge.smudge \"dd if=/dev/zero bs=$((1024*1024)) count=$((5*1024)) && cat\" &&\n+\techo \"small filter=makelarge\" >.gitattributes &&\n+\trm small &&\n+\tgit checkout -- small &&\n+\tsize=$(test_file_size small) &&\n+\ttest \"$size\" -ge $((5 * 1024 * 1024 * 1024))\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"439714","messageId":"5b9d149ba23be06c70262128616e80f45a053a84.1635320952.git.gitgitgadget@gmail.com","threadId":"56792","inReplyTo":"pull.1068.git.1635320952.gitgitgadget@gmail.com","subject":"[PATCH 2/5] odb: teach read_blob_entry to use size_t","fromName":"Matt Cooper via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-27T07:49:09Z","receivedAt":"2021-10-27T07:49:24Z","isPatch":true,"sender":{"key":"name:Matt Cooper","avatar":null},"body":"From: Matt Cooper \n\nThere is mixed use of size_t and unsigned long to deal with sizes in the\ncodebase. Recall that Windows defines unsigned long as 32 bits even on\n64-bit platforms, meaning that converting size_t to unsigned long narrows\nthe range. This mostly doesn't cause a problem since Git rarely deals\nwith files larger than 2^32 bytes.\n\nBut adjunct systems such as Git LFS, which use smudge/clean filters to\nkeep huge files out of the repository, may have huge file contents passed\nthrough some of the functions in entry.c and convert.c. On Windows, this\nresults in a truncated file being written to the workdir. I traced this to\none specific use of unsigned long in write_entry (and a similar instance\nin write_pc_item_to_fd for parallel checkout). That appeared to be for\nthe call to read_blob_entry, which expects a pointer to unsigned long.\n\nBy altering the signature of read_blob_entry to expect a size_t,\nwrite_entry can be switched to use size_t internally (which all of its\ncallers and most of its callees already used). To avoid touching dozens of\nadditional files, read_blob_entry uses a local unsigned long to call a\nchain of functions which aren't prepared to accept size_t.\n\nHelped-by: Johannes Schindelin \nSigned-off-by: Matt Cooper \nSigned-off-by: Johannes Schindelin \n---\n entry.c | 8 +++++---\n entry.h | 2 +-\n parallel-checkout.c | 2 +-\n t/t1051-large-conversion.sh | 2 +-\n 4 files changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex 711ee0693c7..4cb3942dbdc 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -82,11 +82,13 @@ static int create_file(const char *path, unsigned int mode)\n \treturn open(path, O_WRONLY | O_CREAT | O_EXCL, mode);\n }\n \n-void *read_blob_entry(const struct cache_entry *ce, unsigned long *size)\n+void *read_blob_entry(const struct cache_entry *ce, size_t *size)\n {\n \tenum object_type type;\n-\tvoid *blob_data = read_object_file(&ce->oid, &type, size);\n+\tunsigned long ul;\n+\tvoid *blob_data = read_object_file(&ce->oid, &type, &ul);\n \n+\t*size = ul;\n \tif (blob_data) {\n \t\tif (type == OBJ_BLOB)\n \t\t\treturn blob_data;\n@@ -270,7 +272,7 @@ static int write_entry(struct cache_entry *ce, char *path, struct conv_attrs *ca\n \tint fd, ret, fstat_done = 0;\n \tchar *new_blob;\n \tstruct strbuf buf = STRBUF_INIT;\n-\tunsigned long size;\n+\tsize_t size;\n \tssize_t wrote;\n \tsize_t newsize = 0;\n \tstruct stat st;\ndiff --git a/entry.h b/entry.h\nindex b8c0e170dc7..61ee8c17604 100644\n--- a/entry.h\n+++ b/entry.h\n@@ -51,7 +51,7 @@ int finish_delayed_checkout(struct checkout *state, int *nr_checkouts);\n */\n void unlink_entry(const struct cache_entry *ce);\n \n-void *read_blob_entry(const struct cache_entry *ce, unsigned long *size);\n+void *read_blob_entry(const struct cache_entry *ce, size_t *size);\n int fstat_checkout_output(int fd, const struct checkout *state, struct stat *st);\n void update_ce_after_write(const struct checkout *state, struct cache_entry *ce,\n \t\t\t struct stat *st);\ndiff --git a/parallel-checkout.c b/parallel-checkout.c\nindex 6b1af32bb3d..b6f4a25642e 100644\n--- a/parallel-checkout.c\n+++ b/parallel-checkout.c\n@@ -261,7 +261,7 @@ static int write_pc_item_to_fd(struct parallel_checkout_item *pc_item, int fd,\n \tstruct stream_filter *filter;\n \tstruct strbuf buf = STRBUF_INIT;\n \tchar *blob;\n-\tunsigned long size;\n+\tsize_t size;\n \tssize_t wrote;\n \n \t/* Sanity check */\ndiff --git a/t/t1051-large-conversion.sh b/t/t1051-large-conversion.sh\nindex 684ba5bd0a5..38aa0d8a075 100755\n--- a/t/t1051-large-conversion.sh\n+++ b/t/t1051-large-conversion.sh\n@@ -85,7 +85,7 @@ test_expect_success 'ident converts on output' '\n \n # This smudge filter prepends 5GB of zeros to the file it checks out. This\n # ensures that smudging doesn't mangle large files on 64-bit Windows.\n-test_expect_failure EXPENSIVE,!LONG_IS_64BIT 'files over 4GB convert on output' '\n+test_expect_success EXPENSIVE,!LONG_IS_64BIT 'files over 4GB convert on output' '\n \ttest_commit test small \"a small file\" &&\n \ttest_config filter.makelarge.smudge \"dd if=/dev/zero bs=$((1024*1024)) count=$((5*1024)) && cat\" &&\n \techo \"small filter=makelarge\" >.gitattributes &&\n-- \ngitgitgadget\n\n"},{"id":"439715","messageId":"c81ee778b26310d1aee923671c07cbd53496297b.1635320952.git.gitgitgadget@gmail.com","threadId":"56792","inReplyTo":"pull.1068.git.1635320952.gitgitgadget@gmail.com","subject":"[PATCH 3/5] git-compat-util: introduce more size_t helpers","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-27T07:49:10Z","receivedAt":"2021-10-27T07:49:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWe will use them in the next commit.\n\nSigned-off-by: Johannes Schindelin \nSigned-off-by: Matt Cooper \nSigned-off-by: Johannes Schindelin \n---\n git-compat-util.h | 25 +++++++++++++++++++++++++\n 1 file changed, 25 insertions(+)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex a508dbe5a35..7977720655c 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -113,6 +113,14 @@\n #define unsigned_mult_overflows(a, b) \\\n ((a) && (b) > maximum_unsigned_value_of_type(a) / (a))\n \n+/*\n+ * Returns true if the left shift of \"a\" by \"shift\" bits will\n+ * overflow. The types of \"a\" and \"b\" must be unsigned.\n+ * Note that this macro evaluates \"a\" twice!\n+ */\n+#define unsigned_left_shift_overflows(a, shift) \\\n+ ((a) > maximum_unsigned_value_of_type(a) >> shift)\n+\n #ifdef __GNUC__\n #define TYPEOF(x) (__typeof__(x))\n #else\n@@ -859,6 +867,23 @@ static inline size_t st_sub(size_t a, size_t b)\n \treturn a - b;\n }\n \n+static inline size_t st_left_shift(size_t a, unsigned shift)\n+{\n+\tif (unsigned_left_shift_overflows(a, shift))\n+\t\tdie(\"size_t overflow: %\"PRIuMAX\" << %u\",\n+\t\t (uintmax_t)a, shift);\n+\treturn a << shift;\n+}\n+\n+static inline unsigned long cast_size_t_to_ulong(size_t a)\n+{\n+\tif (a != (unsigned long)a)\n+\t\tdie(\"object too large to read on this platform: %\"\n+\t\t PRIuMAX\" is cut off to %lu\",\n+\t\t (uintmax_t)a, (unsigned long)a);\n+\treturn (unsigned long)a;\n+}\n+\n #ifdef HAVE_ALLOCA_H\n # include \n # define xalloca(size) (alloca(size))\n-- \ngitgitgadget\n\n"},{"id":"439716","messageId":"20387ce355759629f9456d8b02226ba2600e2d36.1635320952.git.gitgitgadget@gmail.com","threadId":"56792","inReplyTo":"pull.1068.git.1635320952.gitgitgadget@gmail.com","subject":"[PATCH 5/5] clean/smudge: allow clean filters to process extremely large files","fromName":"Matt Cooper via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-27T07:49:12Z","receivedAt":"2021-10-27T07:49:28Z","isPatch":true,"sender":{"key":"name:Matt Cooper","avatar":null},"body":"From: Matt Cooper \n\nThe filter system allows for alterations to file contents when they're\nmoved between the database and the worktree. We already made sure that\nit is possible for smudge filters to produce contents that are larger\nthan `unsigned long` can represent (which matters on systems where\n`unsigned long` is narrower than `size_t`, most notably 64-bit Windows).\nNow we make sure that clean filters can _consume_ contents that are\nlarger than that.\n\nNote that this commit only allows clean filters' _input_ to be larger\nthan can be represented by `unsigned long`.\n\nThis change makes only a very minute dent into the much larger project\nto teach Git to use `size_t` instead of `unsigned long` wherever\nappropriate.\n\nHelped-by: Johannes Schindelin \nSigned-off-by: Matt Cooper \nSigned-off-by: Johannes Schindelin \n---\n convert.c | 2 +-\n t/t1051-large-conversion.sh | 10 ++++++++++\n 2 files changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/convert.c b/convert.c\nindex fd9c84b0257..5ad6dfc08a0 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -613,7 +613,7 @@ static int crlf_to_worktree(const char *src, size_t len, struct strbuf *buf,\n \n struct filter_params {\n \tconst char *src;\n-\tunsigned long size;\n+\tsize_t size;\n \tint fd;\n \tconst char *cmd;\n \tconst char *path;\ndiff --git a/t/t1051-large-conversion.sh b/t/t1051-large-conversion.sh\nindex 38aa0d8a075..c488850bee4 100755\n--- a/t/t1051-large-conversion.sh\n+++ b/t/t1051-large-conversion.sh\n@@ -95,4 +95,14 @@ test_expect_success EXPENSIVE,!LONG_IS_64BIT 'files over 4GB convert on output'\n \ttest \"$size\" -ge $((5 * 1024 * 1024 * 1024))\n '\n \n+# This clean filter writes down the size of input it receives. By checking against\n+# the actual size, we ensure that cleaning doesn't mangle large files on 64-bit Windows.\n+test_expect_success EXPENSIVE,!LONG_IS_64BIT 'files over 4GB convert on input' '\n+\tdd if=/dev/zero bs=$((1024*1024)) count=$((5*1024)) >big &&\n+\ttest_config filter.checklarge.clean \"wc -c >big.size\" &&\n+\techo \"big filter=checklarge\" >.gitattributes &&\n+\tgit add big &&\n+\ttest $(test_file_size big) -eq $(cat big.size)\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"439717","messageId":"bca013a051117473bdfbaddeeb05d519b7c76ed2.1635320952.git.gitgitgadget@gmail.com","threadId":"56792","inReplyTo":"pull.1068.git.1635320952.gitgitgadget@gmail.com","subject":"[PATCH 4/5] odb: guard against data loss checking out a huge file","fromName":"Matt Cooper via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-27T07:49:11Z","receivedAt":"2021-10-27T07:49:28Z","isPatch":true,"sender":{"key":"name:Matt Cooper","avatar":null},"body":"From: Matt Cooper \n\nThis introduces an additional guard for platforms where `unsigned long`\nand `size_t` are not of the same size. If the size of an object in the\ndatabase would overflow `unsigned long`, instead we now exit with an\nerror.\n\nA complete fix will have to update _many_ other functions throughout the\ncodebase to use `size_t` instead of `unsigned long`. It will have to be\nimplemented at some stage.\n\nThis commit puts in a stop-gap for the time being.\n\nHelped-by: Johannes Schindelin \nSigned-off-by: Matt Cooper \nSigned-off-by: Johannes Schindelin \n---\n delta.h | 6 +++---\n object-file.c | 6 +++---\n packfile.c | 6 +++---\n 3 files changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/delta.h b/delta.h\nindex 2df5fe13d95..8a56ec07992 100644\n--- a/delta.h\n+++ b/delta.h\n@@ -90,15 +90,15 @@ static inline unsigned long get_delta_hdr_size(const unsigned char **datap,\n \t\t\t\t\t const unsigned char *top)\n {\n \tconst unsigned char *data = *datap;\n-\tunsigned long cmd, size = 0;\n+\tsize_t cmd, size = 0;\n \tint i = 0;\n \tdo {\n \t\tcmd = *data++;\n-\t\tsize |= (cmd & 0x7f) << i;\n+\t\tsize |= st_left_shift(cmd & 0x7f, i);\n \t\ti += 7;\n \t} while (cmd & 0x80 && data < top);\n \t*datap = data;\n-\treturn size;\n+\treturn cast_size_t_to_ulong(size);\n }\n \n #endif\ndiff --git a/object-file.c b/object-file.c\nindex f233b440b22..70e456fc2a3 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1344,7 +1344,7 @@ static int parse_loose_header_extended(const char *hdr, struct object_info *oi,\n \t\t\t\t unsigned int flags)\n {\n \tconst char *type_buf = hdr;\n-\tunsigned long size;\n+\tsize_t size;\n \tint type, type_len = 0;\n \n \t/*\n@@ -1388,12 +1388,12 @@ static int parse_loose_header_extended(const char *hdr, struct object_info *oi,\n \t\t\tif (c > 9)\n \t\t\t\tbreak;\n \t\t\thdr++;\n-\t\t\tsize = size * 10 + c;\n+\t\t\tsize = st_add(st_mult(size, 10), c);\n \t\t}\n \t}\n \n \tif (oi->sizep)\n-\t\t*oi->sizep = size;\n+\t\t*oi->sizep = cast_size_t_to_ulong(size);\n \n \t/*\n \t * The length must be followed by a zero byte\ndiff --git a/packfile.c b/packfile.c\nindex 755aa7aec5e..3ccea004396 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1059,7 +1059,7 @@ unsigned long unpack_object_header_buffer(const unsigned char *buf,\n \t\tunsigned long len, enum object_type *type, unsigned long *sizep)\n {\n \tunsigned shift;\n-\tunsigned long size, c;\n+\tsize_t size, c;\n \tunsigned long used = 0;\n \n \tc = buf[used++];\n@@ -1073,10 +1073,10 @@ unsigned long unpack_object_header_buffer(const unsigned char *buf,\n \t\t\tbreak;\n \t\t}\n \t\tc = buf[used++];\n-\t\tsize += (c & 0x7f) << shift;\n+\t\tsize = st_add(size, st_left_shift(c & 0x7f, shift));\n \t\tshift += 7;\n \t}\n-\t*sizep = size;\n+\t*sizep = cast_size_t_to_ulong(size);\n \treturn used;\n }\n \n-- \ngitgitgadget\n\n"},{"id":"439859","messageId":"CAPUEspi_nxpYzJOjJ0osRRLVQ42uPRA9=9eWp3NBtbpeVtJZng@mail.gmail.com","threadId":"56792","inReplyTo":"449eb5c205e139e21b619c4eb975afc3d47427f3.1635320952.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/5] t1051: introduce a smudge filter test for extremely large files","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-10-28T07:15:04Z","receivedAt":"2021-10-28T07:15:17Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Wed, Oct 27, 2021 at 12:03 PM Matt Cooper via GitGitGadget\n wrote:\n> diff --git a/t/t1051-large-conversion.sh b/t/t1051-large-conversion.sh\n> index 8b7640b3ba8..684ba5bd0a5 100755\n> --- a/t/t1051-large-conversion.sh\n> +++ b/t/t1051-large-conversion.sh\n> @@ -83,4 +83,16 @@ test_expect_success 'ident converts on output' '\n> test_cmp small.clean large.clean\n> '\n>\n> +# This smudge filter prepends 5GB of zeros to the file it checks out. This\n> +# ensures that smudging doesn't mangle large files on 64-bit Windows.\n> +test_expect_failure EXPENSIVE,!LONG_IS_64BIT 'files over 4GB convert on output' '\n> + test_commit test small \"a small file\" &&\n> + test_config filter.makelarge.smudge \"dd if=/dev/zero bs=$((1024*1024)) count=$((5*1024)) && cat\" &&\n\n/dev/zero doesn't exist in HP NonStop, a portable solution would be to\nuse `test-tool genzeros` that is available since d5cfd142ec (tests:\nteach the test-tool to generate NUL bytes and use it, 2019-02-14)\n\nCarlo\n"},{"id":"439865","messageId":"20211028085446.78536-1-carenas@gmail.com","threadId":"56792","inReplyTo":"CAPUEspi_nxpYzJOjJ0osRRLVQ42uPRA9=9eWp3NBtbpeVtJZng@mail.gmail.com","subject":"[PATCH] helper/test-genzeros: allow more than 2G zeros in Windows","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-10-28T08:54:46Z","receivedAt":"2021-10-28T08:55:01Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"d5cfd142ec (tests: teach the test-tool to generate NUL bytes and\nuse it, 2019-02-14), add a way to generate zeroes in a portable\nway without using /dev/zero (needed by HP NonStop), but uses a\nlong variable that is limited to 2^31 in Windows.\n\nUse instead a (POSIX/C99) intmax_t that is at least 64bit wide\nin 64-bit Windows to use in a future test.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n t/helper/test-genzeros.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/helper/test-genzeros.c b/t/helper/test-genzeros.c\nindex 9532f5bac9..b1197e91a8 100644\n--- a/t/helper/test-genzeros.c\n+++ b/t/helper/test-genzeros.c\n@@ -3,14 +3,14 @@\n \n int cmd__genzeros(int argc, const char **argv)\n {\n-\tlong count;\n+\tintmax_t count;\n \n \tif (argc > 2) {\n \t\tfprintf(stderr, \"usage: %s []\\n\", argv[0]);\n \t\treturn 1;\n \t}\n \n-\tcount = argc > 1 ? strtol(argv[1], NULL, 0) : -1L;\n+\tcount = argc > 1 ? strtoimax(argv[1], NULL, 0) : -1;\n \n \twhile (count < 0 || count--) {\n \t\tif (putchar(0) == EOF)\n-- \n2.33.0.1155.gbdb71ac078\n\n"},{"id":"439935","messageId":"nycvar.QRO.7.76.6.2110282229280.56@tvgsbejvaqbjf.bet","threadId":"56792","inReplyTo":"20211028085446.78536-1-carenas@gmail.com","subject":"Re: [PATCH] helper/test-genzeros: allow more than 2G zeros in Windows","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-10-28T20:32:52Z","receivedAt":"2021-10-28T20:32:57Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Carlo,\n\nOn Thu, 28 Oct 2021, Carlo Marcelo Arenas Belón wrote:\n\n> d5cfd142ec (tests: teach the test-tool to generate NUL bytes and\n> use it, 2019-02-14), add a way to generate zeroes in a portable\n> way without using /dev/zero (needed by HP NonStop), but uses a\n> long variable that is limited to 2^31 in Windows.\n>\n> Use instead a (POSIX/C99) intmax_t that is at least 64bit wide\n> in 64-bit Windows to use in a future test.\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón \n\nThank you for this patch. I integrated it into the patch series.\n\nUnfortunately, it is incomplete, not because it does not work, but because\nit comes at a hefty performance cost. In my tests, generating a gigabyte\nof NULs took around 27 seconds with `genzeros`. Compare that to ~0.75\nseconds with `dd`, and it is not funny, stop laughing.\n\nHappily, I was able to rewrite the core part of `genzeros` to write chunks\nof a 256kB array instead, which pushed it back down to ~0.6 seconds.\n\nWill send out a new iteration as soon as the CI build passes.\n\nCiao,\nDscho\n\n> ---\n> t/helper/test-genzeros.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/helper/test-genzeros.c b/t/helper/test-genzeros.c\n> index 9532f5bac9..b1197e91a8 100644\n> --- a/t/helper/test-genzeros.c\n> +++ b/t/helper/test-genzeros.c\n> @@ -3,14 +3,14 @@\n>\n> int cmd__genzeros(int argc, const char **argv)\n> {\n> -\tlong count;\n> +\tintmax_t count;\n>\n> \tif (argc > 2) {\n> \t\tfprintf(stderr, \"usage: %s []\\n\", argv[0]);\n> \t\treturn 1;\n> \t}\n>\n> -\tcount = argc > 1 ? strtol(argv[1], NULL, 0) : -1L;\n> +\tcount = argc > 1 ? strtoimax(argv[1], NULL, 0) : -1;\n>\n> \twhile (count < 0 || count--) {\n> \t\tif (putchar(0) == EOF)\n> --\n> 2.33.0.1155.gbdb71ac078\n>\n>\n>\n"},{"id":"439937","messageId":"pull.1068.v2.git.1635454237.gitgitgadget@gmail.com","threadId":"56792","inReplyTo":"pull.1068.git.1635320952.gitgitgadget@gmail.com","subject":"[PATCH v2 0/7] Allow clean/smudge filters to handle huge files in the LLP64 data model","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-28T20:50:30Z","receivedAt":"2021-10-28T20:50:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This patch series came in via the Git for Windows fork\n[https://github.com/git-for-windows/git/pull/3487], and I intend to merge it\nbefore v2.34.0-rc0, therefore I appreciate every careful review you gentle\npeople can spare.\n\nThe x86_64 variant of Windows uses the LLP64 data model, where the long data\ntype is 32-bit. This is very different from the LP64 data model used e.g. by\nx86_64 Linux, where unsigned long is 64-bit.\n\nMost notably, this means that sizeof(unsigned long) != sizeof(size_t) in\ngeneral.\n\nHowever, since Git was born in the Linux ecosystem, where that inequality\ndoes not hold true, it is understandable that unsigned long is used in many\ncode locations where size_t should have been used. As a consequence, quite a\nfew things are broken e.g. on Windows, when it comes to 4GB file contents or\nlarger.\n\nUsing Git LFS [https://git-lfs.github.io/] trying to work around such issues\nis one such a broken scenario. You cannot git checkout, say, 5GB files. Huge\nfiles will be truncated to whatever the file size is modulo 4GB (in the case\nof a 5GB file, it would be truncated to 1GB).\n\nThis patch series primarily fixes the Git LFS scenario, by allowing clean\nfilters to accept 5GB files, and by allowing smudge filters to produce 5GB\nfiles.\n\nThe much larger project to teach Git to use size_t instead of unsigned long\nin all the appropriate places is hardly scratched by this patch series.\n\nSide note: The fix for the clean filter included in this series does not\nactually affect Git LFS! The reason is that Git LFS marks its filter as\nrequired, and therefore Git streams the file contents to Git LFS via a file\ndescriptor (which is unaffected by LLP64). A \"clean\" filter that is not\nmarked as required, however, lets Git take the code path that is fixed by\nthis patch series.\n\nChanges since v1:\n\n * Removed extraneous \"Signed-off-by:\" lines from \"git-compat-util:\n introduce more size_t helpers\".\n * Integrated Carlo's patch to allow genzeros to generate large amounts of\n NULs, even in LLP64 data models.\n * Using test-tool genzeros instead of dd if=/dev/zero, to help HP NonStop\n (which appears to use the LP64 data model and therefore should pass the\n new test cases even without the fixes provided in this patch series).\n * Accelerating genzeros to have performance characteristics similar to dd\n if=/dev/zero instead of being ~50x slower.\n\nCarlo Marcelo Arenas Belón (1):\n test-genzeros: allow more than 2G zeros in Windows\n\nJohannes Schindelin (2):\n test-tool genzeros: generate large amounts of data more efficiently\n git-compat-util: introduce more size_t helpers\n\nMatt Cooper (4):\n t1051: introduce a smudge filter test for extremely large files\n odb: teach read_blob_entry to use size_t\n odb: guard against data loss checking out a huge file\n clean/smudge: allow clean filters to process extremely large files\n\n convert.c | 2 +-\n delta.h | 6 +++---\n entry.c | 8 +++++---\n entry.h | 2 +-\n git-compat-util.h | 25 +++++++++++++++++++++++++\n object-file.c | 6 +++---\n packfile.c | 6 +++---\n parallel-checkout.c | 2 +-\n t/helper/test-genzeros.c | 21 +++++++++++++++++----\n t/t1051-large-conversion.sh | 23 +++++++++++++++++++++++\n 10 files changed, 82 insertions(+), 19 deletions(-)\n\n\nbase-commit: ebf3c04b262aa27fbb97f8a0156c2347fecafafb\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1068%2Fdscho%2Fhuge-file-smudge-clean-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1068/dscho/huge-file-smudge-clean-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1068\n\nRange-diff vs v1:\n\n -: ----------- > 1: 068f897b973 test-genzeros: allow more than 2G zeros in Windows\n -: ----------- > 2: 6edcbae372e test-tool genzeros: generate large amounts of data more efficiently\n 1: 449eb5c205e ! 3: 1bdded86f5d t1051: introduce a smudge filter test for extremely large files\n @@ t/t1051-large-conversion.sh: test_expect_success 'ident converts on output' '\n +# ensures that smudging doesn't mangle large files on 64-bit Windows.\n +test_expect_failure EXPENSIVE,!LONG_IS_64BIT 'files over 4GB convert on output' '\n +\ttest_commit test small \"a small file\" &&\n -+\ttest_config filter.makelarge.smudge \"dd if=/dev/zero bs=$((1024*1024)) count=$((5*1024)) && cat\" &&\n ++\ttest_config filter.makelarge.smudge \\\n ++\t\t\"test-tool genzeros $((5*1024*1024*1024)) && cat\" &&\n +\techo \"small filter=makelarge\" >.gitattributes &&\n +\trm small &&\n +\tgit checkout -- small &&\n 2: 5b9d149ba23 ! 4: 3ffd3a001f7 odb: teach read_blob_entry to use size_t\n @@ t/t1051-large-conversion.sh: test_expect_success 'ident converts on output' '\n -test_expect_failure EXPENSIVE,!LONG_IS_64BIT 'files over 4GB convert on output' '\n +test_expect_success EXPENSIVE,!LONG_IS_64BIT 'files over 4GB convert on output' '\n \ttest_commit test small \"a small file\" &&\n - \ttest_config filter.makelarge.smudge \"dd if=/dev/zero bs=$((1024*1024)) count=$((5*1024)) && cat\" &&\n - \techo \"small filter=makelarge\" >.gitattributes &&\n + \ttest_config filter.makelarge.smudge \\\n + \t\t\"test-tool genzeros $((5*1024*1024*1024)) && cat\" &&\n 3: c81ee778b26 ! 5: 32472ae3f98 git-compat-util: introduce more size_t helpers\n @@ Commit message\n We will use them in the next commit.\n \n Signed-off-by: Johannes Schindelin \n - Signed-off-by: Matt Cooper \n - Signed-off-by: Johannes Schindelin \n \n ## git-compat-util.h ##\n @@\n 4: bca013a0511 = 6: c6910584108 odb: guard against data loss checking out a huge file\n 5: 20387ce3557 ! 7: d87d4229bb4 clean/smudge: allow clean filters to process extremely large files\n @@ t/t1051-large-conversion.sh: test_expect_success EXPENSIVE,!LONG_IS_64BIT 'files\n +# This clean filter writes down the size of input it receives. By checking against\n +# the actual size, we ensure that cleaning doesn't mangle large files on 64-bit Windows.\n +test_expect_success EXPENSIVE,!LONG_IS_64BIT 'files over 4GB convert on input' '\n -+\tdd if=/dev/zero bs=$((1024*1024)) count=$((5*1024)) >big &&\n ++\ttest-tool genzeros $((5*1024*1024*1024)) >big &&\n +\ttest_config filter.checklarge.clean \"wc -c >big.size\" &&\n +\techo \"big filter=checklarge\" >.gitattributes &&\n +\tgit add big &&\n\n-- \ngitgitgadget\n"},{"id":"439938","messageId":"068f897b973b1f8889145f97c42fe6233c272dd5.1635454237.git.gitgitgadget@gmail.com","threadId":"56792","inReplyTo":"pull.1068.v2.git.1635454237.gitgitgadget@gmail.com","subject":"[PATCH v2 1/7] test-genzeros: allow more than 2G zeros in Windows","fromName":"Carlo Marcelo Arenas Belón via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-28T20:50:31Z","receivedAt":"2021-10-28T20:50:44Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= \n\nd5cfd142ec (tests: teach the test-tool to generate NUL bytes and\nuse it, 2019-02-14), add a way to generate zeroes in a portable\nway without using /dev/zero (needed by HP NonStop), but uses a\nlong variable that is limited to 2^31 in Windows.\n\nUse instead a (POSIX/C99) intmax_t that is at least 64bit wide\nin 64-bit Windows to use in a future test.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \nSigned-off-by: Johannes Schindelin \n---\n t/helper/test-genzeros.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/helper/test-genzeros.c b/t/helper/test-genzeros.c\nindex 9532f5bac97..b1197e91a89 100644\n--- a/t/helper/test-genzeros.c\n+++ b/t/helper/test-genzeros.c\n@@ -3,14 +3,14 @@\n \n int cmd__genzeros(int argc, const char **argv)\n {\n-\tlong count;\n+\tintmax_t count;\n \n \tif (argc > 2) {\n \t\tfprintf(stderr, \"usage: %s []\\n\", argv[0]);\n \t\treturn 1;\n \t}\n \n-\tcount = argc > 1 ? strtol(argv[1], NULL, 0) : -1L;\n+\tcount = argc > 1 ? strtoimax(argv[1], NULL, 0) : -1;\n \n \twhile (count < 0 || count--) {\n \t\tif (putchar(0) == EOF)\n-- \ngitgitgadget\n\n"},{"id":"439939","messageId":"6edcbae372ef63bd75ca6cc2d181f7506f35880f.1635454237.git.gitgitgadget@gmail.com","threadId":"56792","inReplyTo":"pull.1068.v2.git.1635454237.gitgitgadget@gmail.com","subject":"[PATCH v2 2/7] test-tool genzeros: generate large amounts of data more efficiently","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-28T20:50:32Z","receivedAt":"2021-10-28T20:50:46Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn this developer's tests, producing one gigabyte worth of NULs in a\nbusy loop that writes out individual bytes, unbuffered, took ~27sec.\nWriting chunked 256kB buffers instead only took ~0.6sec\n\nThis matters because we are about to introduce a pair of test cases that\nwant to be able to produce 5GB of NULs, and we cannot use `/dev/zero`\nbecause of the HP NonStop platform's lack of support for that device.\n\nSigned-off-by: Johannes Schindelin \n---\n t/helper/test-genzeros.c | 17 +++++++++++++++--\n 1 file changed, 15 insertions(+), 2 deletions(-)\n\ndiff --git a/t/helper/test-genzeros.c b/t/helper/test-genzeros.c\nindex b1197e91a89..c061c429da9 100644\n--- a/t/helper/test-genzeros.c\n+++ b/t/helper/test-genzeros.c\n@@ -3,7 +3,10 @@\n \n int cmd__genzeros(int argc, const char **argv)\n {\n+\t/* static, so that it is NUL-initialized */\n+\tstatic char zeros[256 * 1024];\n \tintmax_t count;\n+\tssize_t n;\n \n \tif (argc > 2) {\n \t\tfprintf(stderr, \"usage: %s []\\n\", argv[0]);\n@@ -12,9 +15,19 @@ int cmd__genzeros(int argc, const char **argv)\n \n \tcount = argc > 1 ? strtoimax(argv[1], NULL, 0) : -1;\n \n-\twhile (count < 0 || count--) {\n-\t\tif (putchar(0) == EOF)\n+\t/* Writing out individual NUL bytes is slow... */\n+\twhile (count < 0)\n+\t\tif (write(1, zeros, ARRAY_SIZE(zeros) < 0))\n \t\t\treturn -1;\n+\n+\twhile (count > 0) {\n+\t\tn = write(1, zeros, count < ARRAY_SIZE(zeros) ?\n+\t\t\t count : ARRAY_SIZE(zeros));\n+\n+\t\tif (n < 0)\n+\t\t\treturn -1;\n+\n+\t\tcount -= n;\n \t}\n \n \treturn 0;\n-- \ngitgitgadget\n\n"},{"id":"439940","messageId":"1bdded86f5db61e3983a0f817817f0fbfbae112c.1635454237.git.gitgitgadget@gmail.com","threadId":"56792","inReplyTo":"pull.1068.v2.git.1635454237.gitgitgadget@gmail.com","subject":"[PATCH v2 3/7] t1051: introduce a smudge filter test for extremely large files","fromName":"Matt Cooper via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-28T20:50:33Z","receivedAt":"2021-10-28T20:50:47Z","isPatch":true,"sender":{"key":"name:Matt Cooper","avatar":null},"body":"From: Matt Cooper \n\nThe filter system allows for alterations to file contents when they're\nadded to the database or workdir. (\"Smudge\" when moving to the workdir;\n\"clean\" when moving to the database.) This is used natively to handle CRLF\nto LF conversions. It's also employed by Git-LFS to replace large files\nfrom the workdir with small tracking files in the repo and vice versa.\n\nGit pulls the entire smudged file into memory. While this is inefficient,\nthere's a more insidious problem on some platforms due to inconsistency\nbetween using unsigned long and size_t for the same type of data (size of\na file in bytes). On most 64-bit platforms, unsigned long is 64 bits, and\nsize_t is typedef'd to unsigned long. On Windows, however, unsigned long is\nonly 32 bits (and therefore on 64-bit Windows, size_t is typedef'd to\nunsigned long long in order to be 64 bits).\n\nPractically speaking, this means 64-bit Windows users of Git-LFS can't\nhandle files larger than 2^32 bytes. Other 64-bit platforms don't suffer\nthis limitation.\n\nThis commit introduces a test exposing the issue; future commits make it\npass. The test simulates the way Git-LFS works by having a tiny file\nchecked into the repository and expanding it to a huge file on checkout.\n\nHelped-by: Johannes Schindelin \nSigned-off-by: Matt Cooper \nSigned-off-by: Johannes Schindelin \n---\n t/t1051-large-conversion.sh | 13 +++++++++++++\n 1 file changed, 13 insertions(+)\n\ndiff --git a/t/t1051-large-conversion.sh b/t/t1051-large-conversion.sh\nindex 8b7640b3ba8..7c1a2845005 100755\n--- a/t/t1051-large-conversion.sh\n+++ b/t/t1051-large-conversion.sh\n@@ -83,4 +83,17 @@ test_expect_success 'ident converts on output' '\n \ttest_cmp small.clean large.clean\n '\n \n+# This smudge filter prepends 5GB of zeros to the file it checks out. This\n+# ensures that smudging doesn't mangle large files on 64-bit Windows.\n+test_expect_failure EXPENSIVE,!LONG_IS_64BIT 'files over 4GB convert on output' '\n+\ttest_commit test small \"a small file\" &&\n+\ttest_config filter.makelarge.smudge \\\n+\t\t\"test-tool genzeros $((5*1024*1024*1024)) && cat\" &&\n+\techo \"small filter=makelarge\" >.gitattributes &&\n+\trm small &&\n+\tgit checkout -- small &&\n+\tsize=$(test_file_size small) &&\n+\ttest \"$size\" -ge $((5 * 1024 * 1024 * 1024))\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"439941","messageId":"32472ae3f98bbe0162b39a16109522ec18026404.1635454237.git.gitgitgadget@gmail.com","threadId":"56792","inReplyTo":"pull.1068.v2.git.1635454237.gitgitgadget@gmail.com","subject":"[PATCH v2 5/7] git-compat-util: introduce more size_t helpers","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-28T20:50:35Z","receivedAt":"2021-10-28T20:50:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWe will use them in the next commit.\n\nSigned-off-by: Johannes Schindelin \n---\n git-compat-util.h | 25 +++++++++++++++++++++++++\n 1 file changed, 25 insertions(+)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex a508dbe5a35..7977720655c 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -113,6 +113,14 @@\n #define unsigned_mult_overflows(a, b) \\\n ((a) && (b) > maximum_unsigned_value_of_type(a) / (a))\n \n+/*\n+ * Returns true if the left shift of \"a\" by \"shift\" bits will\n+ * overflow. The types of \"a\" and \"b\" must be unsigned.\n+ * Note that this macro evaluates \"a\" twice!\n+ */\n+#define unsigned_left_shift_overflows(a, shift) \\\n+ ((a) > maximum_unsigned_value_of_type(a) >> shift)\n+\n #ifdef __GNUC__\n #define TYPEOF(x) (__typeof__(x))\n #else\n@@ -859,6 +867,23 @@ static inline size_t st_sub(size_t a, size_t b)\n \treturn a - b;\n }\n \n+static inline size_t st_left_shift(size_t a, unsigned shift)\n+{\n+\tif (unsigned_left_shift_overflows(a, shift))\n+\t\tdie(\"size_t overflow: %\"PRIuMAX\" << %u\",\n+\t\t (uintmax_t)a, shift);\n+\treturn a << shift;\n+}\n+\n+static inline unsigned long cast_size_t_to_ulong(size_t a)\n+{\n+\tif (a != (unsigned long)a)\n+\t\tdie(\"object too large to read on this platform: %\"\n+\t\t PRIuMAX\" is cut off to %lu\",\n+\t\t (uintmax_t)a, (unsigned long)a);\n+\treturn (unsigned long)a;\n+}\n+\n #ifdef HAVE_ALLOCA_H\n # include \n # define xalloca(size) (alloca(size))\n-- \ngitgitgadget\n\n"},{"id":"439942","messageId":"3ffd3a001f742713eb0fde8508f876ff95103d82.1635454237.git.gitgitgadget@gmail.com","threadId":"56792","inReplyTo":"pull.1068.v2.git.1635454237.gitgitgadget@gmail.com","subject":"[PATCH v2 4/7] odb: teach read_blob_entry to use size_t","fromName":"Matt Cooper via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-28T20:50:34Z","receivedAt":"2021-10-28T20:50:49Z","isPatch":true,"sender":{"key":"name:Matt Cooper","avatar":null},"body":"From: Matt Cooper \n\nThere is mixed use of size_t and unsigned long to deal with sizes in the\ncodebase. Recall that Windows defines unsigned long as 32 bits even on\n64-bit platforms, meaning that converting size_t to unsigned long narrows\nthe range. This mostly doesn't cause a problem since Git rarely deals\nwith files larger than 2^32 bytes.\n\nBut adjunct systems such as Git LFS, which use smudge/clean filters to\nkeep huge files out of the repository, may have huge file contents passed\nthrough some of the functions in entry.c and convert.c. On Windows, this\nresults in a truncated file being written to the workdir. I traced this to\none specific use of unsigned long in write_entry (and a similar instance\nin write_pc_item_to_fd for parallel checkout). That appeared to be for\nthe call to read_blob_entry, which expects a pointer to unsigned long.\n\nBy altering the signature of read_blob_entry to expect a size_t,\nwrite_entry can be switched to use size_t internally (which all of its\ncallers and most of its callees already used). To avoid touching dozens of\nadditional files, read_blob_entry uses a local unsigned long to call a\nchain of functions which aren't prepared to accept size_t.\n\nHelped-by: Johannes Schindelin \nSigned-off-by: Matt Cooper \nSigned-off-by: Johannes Schindelin \n---\n entry.c | 8 +++++---\n entry.h | 2 +-\n parallel-checkout.c | 2 +-\n t/t1051-large-conversion.sh | 2 +-\n 4 files changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex 711ee0693c7..4cb3942dbdc 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -82,11 +82,13 @@ static int create_file(const char *path, unsigned int mode)\n \treturn open(path, O_WRONLY | O_CREAT | O_EXCL, mode);\n }\n \n-void *read_blob_entry(const struct cache_entry *ce, unsigned long *size)\n+void *read_blob_entry(const struct cache_entry *ce, size_t *size)\n {\n \tenum object_type type;\n-\tvoid *blob_data = read_object_file(&ce->oid, &type, size);\n+\tunsigned long ul;\n+\tvoid *blob_data = read_object_file(&ce->oid, &type, &ul);\n \n+\t*size = ul;\n \tif (blob_data) {\n \t\tif (type == OBJ_BLOB)\n \t\t\treturn blob_data;\n@@ -270,7 +272,7 @@ static int write_entry(struct cache_entry *ce, char *path, struct conv_attrs *ca\n \tint fd, ret, fstat_done = 0;\n \tchar *new_blob;\n \tstruct strbuf buf = STRBUF_INIT;\n-\tunsigned long size;\n+\tsize_t size;\n \tssize_t wrote;\n \tsize_t newsize = 0;\n \tstruct stat st;\ndiff --git a/entry.h b/entry.h\nindex b8c0e170dc7..61ee8c17604 100644\n--- a/entry.h\n+++ b/entry.h\n@@ -51,7 +51,7 @@ int finish_delayed_checkout(struct checkout *state, int *nr_checkouts);\n */\n void unlink_entry(const struct cache_entry *ce);\n \n-void *read_blob_entry(const struct cache_entry *ce, unsigned long *size);\n+void *read_blob_entry(const struct cache_entry *ce, size_t *size);\n int fstat_checkout_output(int fd, const struct checkout *state, struct stat *st);\n void update_ce_after_write(const struct checkout *state, struct cache_entry *ce,\n \t\t\t struct stat *st);\ndiff --git a/parallel-checkout.c b/parallel-checkout.c\nindex 6b1af32bb3d..b6f4a25642e 100644\n--- a/parallel-checkout.c\n+++ b/parallel-checkout.c\n@@ -261,7 +261,7 @@ static int write_pc_item_to_fd(struct parallel_checkout_item *pc_item, int fd,\n \tstruct stream_filter *filter;\n \tstruct strbuf buf = STRBUF_INIT;\n \tchar *blob;\n-\tunsigned long size;\n+\tsize_t size;\n \tssize_t wrote;\n \n \t/* Sanity check */\ndiff --git a/t/t1051-large-conversion.sh b/t/t1051-large-conversion.sh\nindex 7c1a2845005..5ba03d02682 100755\n--- a/t/t1051-large-conversion.sh\n+++ b/t/t1051-large-conversion.sh\n@@ -85,7 +85,7 @@ test_expect_success 'ident converts on output' '\n \n # This smudge filter prepends 5GB of zeros to the file it checks out. This\n # ensures that smudging doesn't mangle large files on 64-bit Windows.\n-test_expect_failure EXPENSIVE,!LONG_IS_64BIT 'files over 4GB convert on output' '\n+test_expect_success EXPENSIVE,!LONG_IS_64BIT 'files over 4GB convert on output' '\n \ttest_commit test small \"a small file\" &&\n \ttest_config filter.makelarge.smudge \\\n \t\t\"test-tool genzeros $((5*1024*1024*1024)) && cat\" &&\n-- \ngitgitgadget\n\n"},{"id":"439943","messageId":"c6910584108250be18713ba248dd7eff8571667b.1635454237.git.gitgitgadget@gmail.com","threadId":"56792","inReplyTo":"pull.1068.v2.git.1635454237.gitgitgadget@gmail.com","subject":"[PATCH v2 6/7] odb: guard against data loss checking out a huge file","fromName":"Matt Cooper via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-28T20:50:36Z","receivedAt":"2021-10-28T20:50:56Z","isPatch":true,"sender":{"key":"name:Matt Cooper","avatar":null},"body":"From: Matt Cooper \n\nThis introduces an additional guard for platforms where `unsigned long`\nand `size_t` are not of the same size. If the size of an object in the\ndatabase would overflow `unsigned long`, instead we now exit with an\nerror.\n\nA complete fix will have to update _many_ other functions throughout the\ncodebase to use `size_t` instead of `unsigned long`. It will have to be\nimplemented at some stage.\n\nThis commit puts in a stop-gap for the time being.\n\nHelped-by: Johannes Schindelin \nSigned-off-by: Matt Cooper \nSigned-off-by: Johannes Schindelin \n---\n delta.h | 6 +++---\n object-file.c | 6 +++---\n packfile.c | 6 +++---\n 3 files changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/delta.h b/delta.h\nindex 2df5fe13d95..8a56ec07992 100644\n--- a/delta.h\n+++ b/delta.h\n@@ -90,15 +90,15 @@ static inline unsigned long get_delta_hdr_size(const unsigned char **datap,\n \t\t\t\t\t const unsigned char *top)\n {\n \tconst unsigned char *data = *datap;\n-\tunsigned long cmd, size = 0;\n+\tsize_t cmd, size = 0;\n \tint i = 0;\n \tdo {\n \t\tcmd = *data++;\n-\t\tsize |= (cmd & 0x7f) << i;\n+\t\tsize |= st_left_shift(cmd & 0x7f, i);\n \t\ti += 7;\n \t} while (cmd & 0x80 && data < top);\n \t*datap = data;\n-\treturn size;\n+\treturn cast_size_t_to_ulong(size);\n }\n \n #endif\ndiff --git a/object-file.c b/object-file.c\nindex f233b440b22..70e456fc2a3 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1344,7 +1344,7 @@ static int parse_loose_header_extended(const char *hdr, struct object_info *oi,\n \t\t\t\t unsigned int flags)\n {\n \tconst char *type_buf = hdr;\n-\tunsigned long size;\n+\tsize_t size;\n \tint type, type_len = 0;\n \n \t/*\n@@ -1388,12 +1388,12 @@ static int parse_loose_header_extended(const char *hdr, struct object_info *oi,\n \t\t\tif (c > 9)\n \t\t\t\tbreak;\n \t\t\thdr++;\n-\t\t\tsize = size * 10 + c;\n+\t\t\tsize = st_add(st_mult(size, 10), c);\n \t\t}\n \t}\n \n \tif (oi->sizep)\n-\t\t*oi->sizep = size;\n+\t\t*oi->sizep = cast_size_t_to_ulong(size);\n \n \t/*\n \t * The length must be followed by a zero byte\ndiff --git a/packfile.c b/packfile.c\nindex 755aa7aec5e..3ccea004396 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1059,7 +1059,7 @@ unsigned long unpack_object_header_buffer(const unsigned char *buf,\n \t\tunsigned long len, enum object_type *type, unsigned long *sizep)\n {\n \tunsigned shift;\n-\tunsigned long size, c;\n+\tsize_t size, c;\n \tunsigned long used = 0;\n \n \tc = buf[used++];\n@@ -1073,10 +1073,10 @@ unsigned long unpack_object_header_buffer(const unsigned char *buf,\n \t\t\tbreak;\n \t\t}\n \t\tc = buf[used++];\n-\t\tsize += (c & 0x7f) << shift;\n+\t\tsize = st_add(size, st_left_shift(c & 0x7f, shift));\n \t\tshift += 7;\n \t}\n-\t*sizep = size;\n+\t*sizep = cast_size_t_to_ulong(size);\n \treturn used;\n }\n \n-- \ngitgitgadget\n\n"},{"id":"439944","messageId":"d87d4229bb4a5ed6903d277d38899b4f8f592c00.1635454237.git.gitgitgadget@gmail.com","threadId":"56792","inReplyTo":"pull.1068.v2.git.1635454237.gitgitgadget@gmail.com","subject":"[PATCH v2 7/7] clean/smudge: allow clean filters to process extremely large files","fromName":"Matt Cooper via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-28T20:50:37Z","receivedAt":"2021-10-28T20:50:57Z","isPatch":true,"sender":{"key":"name:Matt Cooper","avatar":null},"body":"From: Matt Cooper \n\nThe filter system allows for alterations to file contents when they're\nmoved between the database and the worktree. We already made sure that\nit is possible for smudge filters to produce contents that are larger\nthan `unsigned long` can represent (which matters on systems where\n`unsigned long` is narrower than `size_t`, most notably 64-bit Windows).\nNow we make sure that clean filters can _consume_ contents that are\nlarger than that.\n\nNote that this commit only allows clean filters' _input_ to be larger\nthan can be represented by `unsigned long`.\n\nThis change makes only a very minute dent into the much larger project\nto teach Git to use `size_t` instead of `unsigned long` wherever\nappropriate.\n\nHelped-by: Johannes Schindelin \nSigned-off-by: Matt Cooper \nSigned-off-by: Johannes Schindelin \n---\n convert.c | 2 +-\n t/t1051-large-conversion.sh | 10 ++++++++++\n 2 files changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/convert.c b/convert.c\nindex fd9c84b0257..5ad6dfc08a0 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -613,7 +613,7 @@ static int crlf_to_worktree(const char *src, size_t len, struct strbuf *buf,\n \n struct filter_params {\n \tconst char *src;\n-\tunsigned long size;\n+\tsize_t size;\n \tint fd;\n \tconst char *cmd;\n \tconst char *path;\ndiff --git a/t/t1051-large-conversion.sh b/t/t1051-large-conversion.sh\nindex 5ba03d02682..83d9cf485a3 100755\n--- a/t/t1051-large-conversion.sh\n+++ b/t/t1051-large-conversion.sh\n@@ -96,4 +96,14 @@ test_expect_success EXPENSIVE,!LONG_IS_64BIT 'files over 4GB convert on output'\n \ttest \"$size\" -ge $((5 * 1024 * 1024 * 1024))\n '\n \n+# This clean filter writes down the size of input it receives. By checking against\n+# the actual size, we ensure that cleaning doesn't mangle large files on 64-bit Windows.\n+test_expect_success EXPENSIVE,!LONG_IS_64BIT 'files over 4GB convert on input' '\n+\ttest-tool genzeros $((5*1024*1024*1024)) >big &&\n+\ttest_config filter.checklarge.clean \"wc -c >big.size\" &&\n+\techo \"big filter=checklarge\" >.gitattributes &&\n+\tgit add big &&\n+\ttest $(test_file_size big) -eq $(cat big.size)\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"439945","messageId":"20211028205649.84036-1-carenas@gmail.com","threadId":"56792","inReplyTo":"pull.1068.git.1635320952.gitgitgadget@gmail.com","subject":"[PATCH 0/3] Allow clean/smudge filters to handle huge files in the LLP64 data model","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-10-28T20:56:46Z","receivedAt":"2021-10-28T20:57:02Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Following series is a fixup to avoid breaking in 32-bit systems; first\npatch should be added as a prerequisite with the 2 following able to\napply to the corresponding patch in the original series.\n\nCarlo Marcelo Arenas Belón (3):\n test-lib: add prerequisite for 64-bit platforms\n fixup! t1051: introduce a smudge filter test for extremely large files\n fixup! clean/smudge: allow clean filters to process extremely large\n files\n\n t/t1051-large-conversion.sh | 4 ++--\n t/test-lib.sh | 4 ++++\n 2 files changed, 6 insertions(+), 2 deletions(-)\n\n-- \n2.33.0.1155.gbdb71ac078\n\n"},{"id":"439946","messageId":"20211028205649.84036-2-carenas@gmail.com","threadId":"56792","inReplyTo":"20211028205649.84036-1-carenas@gmail.com","subject":"[PATCH 1/3] test-lib: add prerequisite for 64-bit platforms","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-10-28T20:56:47Z","receivedAt":"2021-10-28T20:57:03Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Allow tests that assume a 64-bit size_t to be skipped in 32-bit platforms\nand regardless of the size of long.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n t/test-lib.sh | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex fc1e521519..5fa7fb5719 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1687,6 +1687,10 @@ build_option () {\n \tsed -ne \"s/^$1: //p\"\n }\n \n+test_lazy_prereq IS_64BIT '\n+\ttest 8 -eq \"$(build_option sizeof-size_t)\"\n+'\n+\n test_lazy_prereq LONG_IS_64BIT '\n \ttest 8 -le \"$(build_option sizeof-long)\"\n '\n-- \n2.33.0.1155.gbdb71ac078\n\n"},{"id":"439947","messageId":"20211028205649.84036-3-carenas@gmail.com","threadId":"56792","inReplyTo":"20211028205649.84036-1-carenas@gmail.com","subject":"[PATCH 2/3] fixup! t1051: introduce a smudge filter test for extremely large files","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-10-28T20:56:48Z","receivedAt":"2021-10-28T20:57:04Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Signed-off-by: Carlo Marcelo Arenas Belón \n---\n t/t1051-large-conversion.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t1051-large-conversion.sh b/t/t1051-large-conversion.sh\nindex c488850bee..dd5f270b20 100755\n--- a/t/t1051-large-conversion.sh\n+++ b/t/t1051-large-conversion.sh\n@@ -85,7 +85,7 @@ test_expect_success 'ident converts on output' '\n \n # This smudge filter prepends 5GB of zeros to the file it checks out. This\n # ensures that smudging doesn't mangle large files on 64-bit Windows.\n-test_expect_success EXPENSIVE,!LONG_IS_64BIT 'files over 4GB convert on output' '\n+test_expect_success EXPENSIVE,IS_64BIT,!LONG_IS_64BIT 'files over 4GB convert on output' '\n \ttest_commit test small \"a small file\" &&\n \ttest_config filter.makelarge.smudge \"dd if=/dev/zero bs=$((1024*1024)) count=$((5*1024)) && cat\" &&\n \techo \"small filter=makelarge\" >.gitattributes &&\n-- \n2.33.0.1155.gbdb71ac078\n\n"},{"id":"439948","messageId":"20211028205649.84036-4-carenas@gmail.com","threadId":"56792","inReplyTo":"20211028205649.84036-1-carenas@gmail.com","subject":"[PATCH 3/3] fixup! clean/smudge: allow clean filters to process extremely large files","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-10-28T20:56:49Z","receivedAt":"2021-10-28T20:57:06Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Signed-off-by: Carlo Marcelo Arenas Belón \n---\n t/t1051-large-conversion.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t1051-large-conversion.sh b/t/t1051-large-conversion.sh\nindex dd5f270b20..e903570f30 100755\n--- a/t/t1051-large-conversion.sh\n+++ b/t/t1051-large-conversion.sh\n@@ -97,7 +97,7 @@ test_expect_success EXPENSIVE,IS_64BIT,!LONG_IS_64BIT 'files over 4GB convert on\n \n # This clean filter writes down the size of input it receives. By checking against\n # the actual size, we ensure that cleaning doesn't mangle large files on 64-bit Windows.\n-test_expect_success EXPENSIVE,!LONG_IS_64BIT 'files over 4GB convert on input' '\n+test_expect_success EXPENSIVE,IS_64BIT,!LONG_IS_64BIT 'files over 4GB convert on input' '\n \tdd if=/dev/zero bs=$((1024*1024)) count=$((5*1024)) >big &&\n \ttest_config filter.checklarge.clean \"wc -c >big.size\" &&\n \techo \"big filter=checklarge\" >.gitattributes &&\n-- \n2.33.0.1155.gbdb71ac078\n\n"},{"id":"439952","messageId":"nycvar.QRO.7.76.6.2110282344330.56@tvgsbejvaqbjf.bet","threadId":"56792","inReplyTo":"20211028205649.84036-2-carenas@gmail.com","subject":"Re: [PATCH 1/3] test-lib: add prerequisite for 64-bit platforms","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-10-28T21:45:59Z","receivedAt":"2021-10-28T21:46:04Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Carlo,\n\nOn Thu, 28 Oct 2021, Carlo Marcelo Arenas Belón wrote:\n\n> Allow tests that assume a 64-bit size_t to be skipped in 32-bit platforms\n> and regardless of the size of long.\n\nMakes sense, but...\n\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> t/test-lib.sh | 4 ++++\n> 1 file changed, 4 insertions(+)\n>\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index fc1e521519..5fa7fb5719 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -1687,6 +1687,10 @@ build_option () {\n> \tsed -ne \"s/^$1: //p\"\n> }\n>\n> +test_lazy_prereq IS_64BIT '\n\nThis should be `SIZE_T_IS_64BIT`.\n\n> +\ttest 8 -eq \"$(build_option sizeof-size_t)\"\n\nSince this is clearly copied from `LONG_IS_64BIT`, why the change from\n`-le` to `-eq`? It is at least inconsistent to use anything different\nhere.\n\nCiao,\nDscho\n\n> +'\n> +\n> test_lazy_prereq LONG_IS_64BIT '\n> \ttest 8 -le \"$(build_option sizeof-long)\"\n> '\n> --\n> 2.33.0.1155.gbdb71ac078\n>\n>\n>\n"},{"id":"439954","messageId":"CAPUEspj2B7M_-cbA2O3LickF7MeVYNtXjfJMBMeYkLag5K=Z3g@mail.gmail.com","threadId":"56792","inReplyTo":"nycvar.QRO.7.76.6.2110282344330.56@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/3] test-lib: add prerequisite for 64-bit platforms","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-10-28T22:09:13Z","receivedAt":"2021-10-28T22:09:27Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Thu, Oct 28, 2021 at 2:46 PM Johannes Schindelin\n wrote:\n>\n> On Thu, 28 Oct 2021, Carlo Marcelo Arenas Belón wrote:\n>\n> > Allow tests that assume a 64-bit size_t to be skipped in 32-bit platforms\n> > and regardless of the size of long.\n>\n> Makes sense, but...\n>\n> >\n> > Signed-off-by: Carlo Marcelo Arenas Belón \n> > ---\n> > t/test-lib.sh | 4 ++++\n> > 1 file changed, 4 insertions(+)\n> >\n> > diff --git a/t/test-lib.sh b/t/test-lib.sh\n> > index fc1e521519..5fa7fb5719 100644\n> > --- a/t/test-lib.sh\n> > +++ b/t/test-lib.sh\n> > @@ -1687,6 +1687,10 @@ build_option () {\n> > sed -ne \"s/^$1: //p\"\n> > }\n> >\n> > +test_lazy_prereq IS_64BIT '\n>\n> This should be `SIZE_T_IS_64BIT`.\n\nFair point, but...\n\n> > + test 8 -eq \"$(build_option sizeof-size_t)\"\n>\n> Since this is clearly copied from `LONG_IS_64BIT`, why the change from\n> `-le` to `-eq`? It is at least inconsistent to use anything different\n> here.\n\nMy assumption is that the check for sizeof(size_t) we have is really\nabout finding the bit width of the platform, and we currently support\n2 of them (32-bit and 64-bit), which is why the name I chose was\n\"IS_64BIT\" and also why I was strict on it being exactly 8 bytes\n(considering all platforms git supports have bytes with 8 bits).\n\nIt can go eitherway IMHO, and your point about being inconsistent\n(with my lack of explanation in the commit) suggests we should instead\nuse your proposal, do you want me to resend or could adjust them in\nyour tree?\n\nCarlo\n\nPS. I think we should also add a \"TODO\" comment in the code, but other\nthan that could also take my \"Reviewed-by\" for the series\n"},{"id":"439956","messageId":"CAPUEsphWBuQybeBe8bjaUFZ=k+5maYCxiY4wche4XLVFPph+fw@mail.gmail.com","threadId":"56792","inReplyTo":"3ffd3a001f742713eb0fde8508f876ff95103d82.1635454237.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 4/7] odb: teach read_blob_entry to use size_t","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-10-28T22:14:51Z","receivedAt":"2021-10-28T22:15:08Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Thu, Oct 28, 2021 at 1:50 PM Matt Cooper via GitGitGadget\n wrote:\n> diff --git a/entry.c b/entry.c\n> index 711ee0693c7..4cb3942dbdc 100644\n> --- a/entry.c\n> +++ b/entry.c\n> @@ -82,11 +82,13 @@ static int create_file(const char *path, unsigned int mode)\n> return open(path, O_WRONLY | O_CREAT | O_EXCL, mode);\n> }\n>\n> -void *read_blob_entry(const struct cache_entry *ce, unsigned long *size)\n> +void *read_blob_entry(const struct cache_entry *ce, size_t *size)\n> {\n> enum object_type type;\n> - void *blob_data = read_object_file(&ce->oid, &type, size);\n> + unsigned long ul;\n> + void *blob_data = read_object_file(&ce->oid, &type, &ul);\n>\n> + *size = ul;\n\nConsidering this unsigned long variable is obviously holding an\nincorrect value, might be worth adding a \"TODO\" comment here,\nmentioning it should be changed to a real size_t (probably after\nrelease, since it is obvious that change is too intrusive to need this\nas a kludge for now).\n\nCarlo\n"},{"id":"439958","messageId":"nycvar.QRO.7.76.6.2110290019540.56@tvgsbejvaqbjf.bet","threadId":"56792","inReplyTo":"CAPUEsphWBuQybeBe8bjaUFZ=k+5maYCxiY4wche4XLVFPph+fw@mail.gmail.com","subject":"Re: [PATCH v2 4/7] odb: teach read_blob_entry to use size_t","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-10-28T22:21:59Z","receivedAt":"2021-10-28T22:22:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Carlo,\n\nOn Thu, 28 Oct 2021, Carlo Arenas wrote:\n\n> On Thu, Oct 28, 2021 at 1:50 PM Matt Cooper via GitGitGadget\n> wrote:\n> > diff --git a/entry.c b/entry.c\n> > index 711ee0693c7..4cb3942dbdc 100644\n> > --- a/entry.c\n> > +++ b/entry.c\n> > @@ -82,11 +82,13 @@ static int create_file(const char *path, unsigned int mode)\n> > return open(path, O_WRONLY | O_CREAT | O_EXCL, mode);\n> > }\n> >\n> > -void *read_blob_entry(const struct cache_entry *ce, unsigned long *size)\n> > +void *read_blob_entry(const struct cache_entry *ce, size_t *size)\n> > {\n> > enum object_type type;\n> > - void *blob_data = read_object_file(&ce->oid, &type, size);\n> > + unsigned long ul;\n> > + void *blob_data = read_object_file(&ce->oid, &type, &ul);\n> >\n> > + *size = ul;\n>\n> Considering this unsigned long variable is obviously holding an\n> incorrect value, might be worth adding a \"TODO\" comment here,\n> mentioning it should be changed to a real size_t (probably after\n> release, since it is obvious that change is too intrusive to need this\n> as a kludge for now).\n\nI do not think it would be a good idea to introduce a `TODO` comment here.\nWhy _here_, of all places? Just because we use `size_t` correctly for a\nbit of the function?\n\nNo, that `TODO` would easily be forgotten when somebody tackles the big\n`unsigned long` -> `size_t` project. I therefore do not even want to add\nit. We know about this problem. No need for a code comment that is prone\nto become stale.\n\nCiao,\nDscho\n"},{"id":"439960","messageId":"YXsk8XBwmErf2I3I@camp.crustytoothpaste.net","threadId":"56792","inReplyTo":"pull.1068.v2.git.1635454237.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/7] Allow clean/smudge filters to handle huge files in the LLP64 data model","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2021-10-28T22:32:18Z","receivedAt":"2021-10-28T22:33:03Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2021-10-28 at 20:50:30, Johannes Schindelin via GitGitGadget wrote:\n> This patch series came in via the Git for Windows fork\n> [https://github.com/git-for-windows/git/pull/3487], and I intend to merge it\n> before v2.34.0-rc0, therefore I appreciate every careful review you gentle\n> people can spare.\n> \n> The x86_64 variant of Windows uses the LLP64 data model, where the long data\n> type is 32-bit. This is very different from the LP64 data model used e.g. by\n> x86_64 Linux, where unsigned long is 64-bit.\n> \n> Most notably, this means that sizeof(unsigned long) != sizeof(size_t) in\n> general.\n> \n> However, since Git was born in the Linux ecosystem, where that inequality\n> does not hold true, it is understandable that unsigned long is used in many\n> code locations where size_t should have been used. As a consequence, quite a\n> few things are broken e.g. on Windows, when it comes to 4GB file contents or\n> larger.\n> \n> Using Git LFS [https://git-lfs.github.io/] trying to work around such issues\n> is one such a broken scenario. You cannot git checkout, say, 5GB files. Huge\n> files will be truncated to whatever the file size is modulo 4GB (in the case\n> of a 5GB file, it would be truncated to 1GB).\n> \n> This patch series primarily fixes the Git LFS scenario, by allowing clean\n> filters to accept 5GB files, and by allowing smudge filters to produce 5GB\n> files.\n> \n> The much larger project to teach Git to use size_t instead of unsigned long\n> in all the appropriate places is hardly scratched by this patch series.\n> \n> Side note: The fix for the clean filter included in this series does not\n> actually affect Git LFS! The reason is that Git LFS marks its filter as\n> required, and therefore Git streams the file contents to Git LFS via a file\n> descriptor (which is unaffected by LLP64). A \"clean\" filter that is not\n> marked as required, however, lets Git take the code path that is fixed by\n> this patch series.\n\nThis series seems sane to me. I'm delighted we can fix this issue with\nso little code, since it's a been a very inconvenient problem for a lot\nof Windows users.\n\nI might suggest when we make the giant transition project in the future\nthat we use size_t for things that are going to be in memory and off_t\nor, if necessary, intmax_t for general object sizes so it's clear which\none we want. However, that has no effect on this series since this\nintentionally has a limited scope.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"439961","messageId":"xmqqee84pyfb.fsf@gitster.g","threadId":"56792","inReplyTo":"CAPUEspj2B7M_-cbA2O3LickF7MeVYNtXjfJMBMeYkLag5K=Z3g@mail.gmail.com","subject":"Re: [PATCH 1/3] test-lib: add prerequisite for 64-bit platforms","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-28T22:38:48Z","receivedAt":"2021-10-28T22:38:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Arenas writes:\n\n>> Since this is clearly copied from `LONG_IS_64BIT`, why the change from\n>> `-le` to `-eq`? It is at least inconsistent to use anything different\n>> here.\n>\n> My assumption is that the check for sizeof(size_t) we have is really\n> about finding the bit width of the platform, and we currently support\n> 2 of them (32-bit and 64-bit), which is why the name I chose was\n> \"IS_64BIT\" and also why I was strict on it being exactly 8 bytes\n> (considering all platforms git supports have bytes with 8 bits).\n>\n> It can go eitherway IMHO, and your point about being inconsistent\n> (with my lack of explanation in the commit) suggests we should instead\n> use your proposal, do you want me to resend or could adjust them in\n> your tree?\n\nIs LONG_IS_64BIT used to ensure that long is _at least_ 64 bit? If\nso, perhaps its name may need to be rethought. On the other hand,\nif it is meant to ensure that long is exactly 64 bit, then it should\nuse -eq to compare.\n\nAnd from that point of view, SIZE_T_IS_64BIT and use of -eq look\nconsistent with each other, I would think.\n\nThanks.\n\n"},{"id":"439969","messageId":"xmqq4k90pxng.fsf@gitster.g","threadId":"56792","inReplyTo":"6edcbae372ef63bd75ca6cc2d181f7506f35880f.1635454237.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/7] test-tool genzeros: generate large amounts of data more efficiently","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-28T22:55:31Z","receivedAt":"2021-10-28T22:55:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> int cmd__genzeros(int argc, const char **argv)\n> {\n> +\t/* static, so that it is NUL-initialized */\n> +\tstatic char zeros[256 * 1024];\n\nOf course, another benefit is that you do not waste 256kB of\nstackspace for this array. We could probably even mark it as\n\"const\" to emphasize that it is initialized with and will stay\nto be bunch of NULs.\n\n> \tintmax_t count;\n> +\tssize_t n;\n> \n> \tif (argc > 2) {\n> \t\tfprintf(stderr, \"usage: %s []\\n\", argv[0]);\n> @@ -12,9 +15,19 @@ int cmd__genzeros(int argc, const char **argv)\n> \n> \tcount = argc > 1 ? strtoimax(argv[1], NULL, 0) : -1;\n> \n> -\twhile (count < 0 || count--) {\n> -\t\tif (putchar(0) == EOF)\n\n> +\t/* Writing out individual NUL bytes is slow... */\n> +\twhile (count < 0)\n> +\t\tif (write(1, zeros, ARRAY_SIZE(zeros) < 0))\n> \t\t\treturn -1;\n\nBe careful where you close your parentheses. \n\nI wonder if your compiler didn't warn that ARRAY_SIZE(x) will never\nbe negative?\n\n> +\twhile (count > 0) {\n> +\t\tn = write(1, zeros, count < ARRAY_SIZE(zeros) ?\n> +\t\t\t count : ARRAY_SIZE(zeros));\n\nThis side looks OK.\n\n> +\t\tif (n < 0)\n> +\t\t\treturn -1;\n> +\n> +\t\tcount -= n;\n> \t}\n> \n> \treturn 0;\n\n"},{"id":"439970","messageId":"xmqqr1c4oim9.fsf@gitster.g","threadId":"56792","inReplyTo":"32472ae3f98bbe0162b39a16109522ec18026404.1635454237.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 5/7] git-compat-util: introduce more size_t helpers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-28T23:05:34Z","receivedAt":"2021-10-28T23:05:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> We will use them in the next commit.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> git-compat-util.h | 25 +++++++++++++++++++++++++\n> 1 file changed, 25 insertions(+)\n>\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index a508dbe5a35..7977720655c 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -113,6 +113,14 @@\n> #define unsigned_mult_overflows(a, b) \\\n> ((a) && (b) > maximum_unsigned_value_of_type(a) / (a))\n> \n> +/*\n> + * Returns true if the left shift of \"a\" by \"shift\" bits will\n> + * overflow. The types of \"a\" and \"b\" must be unsigned.\n\nThe type of \"a\" must be unsigned, and there is no \"b\".\n\n\"shift\" can be of an integral type, and it probably is a good idea\nto feed a positive value that is smaller than bitsizeof(type(a)),\nbut we probably do not have to say anything about it.\n\n> + * Note that this macro evaluates \"a\" twice!\n\nmaximum_unsigned_value_of_type() does take bitsizeof() of the thing,\nbut it only needs the type of it, not the value, so I doubt that it\nwould evaluate 'a' even once. This macro does need the value of 'a'\nso it would evaluate it once.\n\n> + */\n> +#define unsigned_left_shift_overflows(a, shift) \\\n> + ((a) > maximum_unsigned_value_of_type(a) >> shift)\n\ntry:\n\n\tunsigned a = 0;\n\tint ov = unsigned_left_shift_overflows(++a, 4);\n\t\n\tprintf(\"a = %d, ov = %d\\n\", a, ov);\n\treturn 0;\n\nI think you'd get \"a = 1\".\n\n"},{"id":"439971","messageId":"xmqqmtmsoija.fsf@gitster.g","threadId":"56792","inReplyTo":"YXsk8XBwmErf2I3I@camp.crustytoothpaste.net","subject":"Re: [PATCH v2 0/7] Allow clean/smudge filters to handle huge files in the LLP64 data model","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-28T23:07:21Z","receivedAt":"2021-10-28T23:07:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> This series seems sane to me. I'm delighted we can fix this issue with\n> so little code, since it's a been a very inconvenient problem for a lot\n> of Windows users.\n\nYup. I do find the goal this series aims is quite worthy, and the\nsize of the series is \"reviewable in a single sitting\" bite-size,\nwhich was pleasant.\n\nThanks.\n"},{"id":"440009","messageId":"068f897b973b1f8889145f97c42fe6233c272dd5.1635515959.git.gitgitgadget@gmail.com","threadId":"56792","inReplyTo":"pull.1068.v3.git.1635515959.gitgitgadget@gmail.com","subject":"[PATCH v3 1/8] test-genzeros: allow more than 2G zeros in Windows","fromName":"Carlo Marcelo Arenas Belón via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-29T13:59:12Z","receivedAt":"2021-10-29T13:59:25Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= \n\nd5cfd142ec (tests: teach the test-tool to generate NUL bytes and\nuse it, 2019-02-14), add a way to generate zeroes in a portable\nway without using /dev/zero (needed by HP NonStop), but uses a\nlong variable that is limited to 2^31 in Windows.\n\nUse instead a (POSIX/C99) intmax_t that is at least 64bit wide\nin 64-bit Windows to use in a future test.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \nSigned-off-by: Johannes Schindelin \n---\n t/helper/test-genzeros.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/helper/test-genzeros.c b/t/helper/test-genzeros.c\nindex 9532f5bac97..b1197e91a89 100644\n--- a/t/helper/test-genzeros.c\n+++ b/t/helper/test-genzeros.c\n@@ -3,14 +3,14 @@\n \n int cmd__genzeros(int argc, const char **argv)\n {\n-\tlong count;\n+\tintmax_t count;\n \n \tif (argc > 2) {\n \t\tfprintf(stderr, \"usage: %s []\\n\", argv[0]);\n \t\treturn 1;\n \t}\n \n-\tcount = argc > 1 ? strtol(argv[1], NULL, 0) : -1L;\n+\tcount = argc > 1 ? strtoimax(argv[1], NULL, 0) : -1;\n \n \twhile (count < 0 || count--) {\n \t\tif (putchar(0) == EOF)\n-- \ngitgitgadget\n\n"},{"id":"440010","messageId":"pull.1068.v3.git.1635515959.gitgitgadget@gmail.com","threadId":"56792","inReplyTo":"pull.1068.v2.git.1635454237.gitgitgadget@gmail.com","subject":"[PATCH v3 0/8] Allow clean/smudge filters to handle huge files in the LLP64 data model","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-29T13:59:11Z","receivedAt":"2021-10-29T13:59:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This patch series came in via the Git for Windows fork\n[https://github.com/git-for-windows/git/pull/3487], and I intend to merge it\nbefore v2.34.0-rc0, therefore I appreciate every careful review you gentle\npeople can spare.\n\nThe x86_64 variant of Windows uses the LLP64 data model, where the long data\ntype is 32-bit. This is very different from the LP64 data model used e.g. by\nx86_64 Linux, where unsigned long is 64-bit.\n\nMost notably, this means that sizeof(unsigned long) != sizeof(size_t) in\ngeneral.\n\nHowever, since Git was born in the Linux ecosystem, where that inequality\ndoes not hold true, it is understandable that unsigned long is used in many\ncode locations where size_t should have been used. As a consequence, quite a\nfew things are broken e.g. on Windows, when it comes to 4GB file contents or\nlarger.\n\nUsing Git LFS [https://git-lfs.github.io/] trying to work around such issues\nis one such a broken scenario. You cannot git checkout, say, 5GB files. Huge\nfiles will be truncated to whatever the file size is modulo 4GB (in the case\nof a 5GB file, it would be truncated to 1GB).\n\nThis patch series primarily fixes the Git LFS scenario, by allowing clean\nfilters to accept 5GB files, and by allowing smudge filters to produce 5GB\nfiles.\n\nThe much larger project to teach Git to use size_t instead of unsigned long\nin all the appropriate places is hardly scratched by this patch series.\n\nSide note: The fix for the clean filter included in this series does not\nactually affect Git LFS! The reason is that Git LFS marks its filter as\nrequired, and therefore Git streams the file contents to Git LFS via a file\ndescriptor (which is unaffected by LLP64). A \"clean\" filter that is not\nmarked as required, however, lets Git take the code path that is fixed by\nthis patch series.\n\nChanges since v2:\n\n * The test cases now use a prereq to avoid running in 32-bit setups (where\n they would be guaranteed to fail).\n * The SIZE_T_IS64BIT prereq specifically does not follow LONG_IS_64BIT, by\n testing for exactly 64 bits.\n * The code comment above unsigned_left_shift_overflows() was fixed.\n * unsigned_left_shift_overflows() now verifies that shift is smaller than\n the bit size of the operand.\n * genzeros now marks its buffer of NULs as const.\n * The error check in the infinite loop genzeros was fixed.\n\nChanges since v1:\n\n * Removed extraneous \"Signed-off-by:\" lines from \"git-compat-util:\n introduce more size_t helpers\".\n * Integrated Carlo's patch to allow genzeros to generate large amounts of\n NULs, even in LLP64 data models.\n * Using test-tool genzeros instead of dd if=/dev/zero, to help HP NonStop\n (which appears to use the LP64 data model and therefore should pass the\n new test cases even without the fixes provided in this patch series).\n * Accelerating genzeros to have performance characteristics similar to dd\n if=/dev/zero instead of being ~50x slower.\n\nCarlo Marcelo Arenas Belón (2):\n test-genzeros: allow more than 2G zeros in Windows\n test-lib: add prerequisite for 64-bit platforms\n\nJohannes Schindelin (2):\n test-tool genzeros: generate large amounts of data more efficiently\n git-compat-util: introduce more size_t helpers\n\nMatt Cooper (4):\n t1051: introduce a smudge filter test for extremely large files\n odb: teach read_blob_entry to use size_t\n odb: guard against data loss checking out a huge file\n clean/smudge: allow clean filters to process extremely large files\n\n convert.c | 2 +-\n delta.h | 6 +++---\n entry.c | 8 +++++---\n entry.h | 2 +-\n git-compat-util.h | 25 +++++++++++++++++++++++++\n object-file.c | 6 +++---\n packfile.c | 6 +++---\n parallel-checkout.c | 2 +-\n t/helper/test-genzeros.c | 21 +++++++++++++++++----\n t/t1051-large-conversion.sh | 25 +++++++++++++++++++++++++\n t/test-lib.sh | 4 ++++\n 11 files changed, 88 insertions(+), 19 deletions(-)\n\n\nbase-commit: ebf3c04b262aa27fbb97f8a0156c2347fecafafb\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1068%2Fdscho%2Fhuge-file-smudge-clean-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1068/dscho/huge-file-smudge-clean-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1068\n\nRange-diff vs v2:\n\n 1: 068f897b973 = 1: 068f897b973 test-genzeros: allow more than 2G zeros in Windows\n 2: 6edcbae372e ! 2: 05219720014 test-tool genzeros: generate large amounts of data more efficiently\n @@ t/helper/test-genzeros.c\n int cmd__genzeros(int argc, const char **argv)\n {\n +\t/* static, so that it is NUL-initialized */\n -+\tstatic char zeros[256 * 1024];\n ++\tstatic const char zeros[256 * 1024];\n \tintmax_t count;\n +\tssize_t n;\n \n @@ t/helper/test-genzeros.c: int cmd__genzeros(int argc, const char **argv)\n -\t\tif (putchar(0) == EOF)\n +\t/* Writing out individual NUL bytes is slow... */\n +\twhile (count < 0)\n -+\t\tif (write(1, zeros, ARRAY_SIZE(zeros) < 0))\n ++\t\tif (write(1, zeros, ARRAY_SIZE(zeros)) < 0)\n \t\t\treturn -1;\n +\n +\twhile (count > 0) {\n -: ----------- > 3: 489500bb1dc test-lib: add prerequisite for 64-bit platforms\n 3: 1bdded86f5d ! 4: ce9dfaac9f8 t1051: introduce a smudge filter test for extremely large files\n @@ t/t1051-large-conversion.sh: test_expect_success 'ident converts on output' '\n \n +# This smudge filter prepends 5GB of zeros to the file it checks out. This\n +# ensures that smudging doesn't mangle large files on 64-bit Windows.\n -+test_expect_failure EXPENSIVE,!LONG_IS_64BIT 'files over 4GB convert on output' '\n ++test_expect_failure EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \\\n ++\t\t'files over 4GB convert on output' '\n +\ttest_commit test small \"a small file\" &&\n +\ttest_config filter.makelarge.smudge \\\n +\t\t\"test-tool genzeros $((5*1024*1024*1024)) && cat\" &&\n 4: 3ffd3a001f7 ! 5: dbef8168bc7 odb: teach read_blob_entry to use size_t\n @@ t/t1051-large-conversion.sh: test_expect_success 'ident converts on output' '\n \n # This smudge filter prepends 5GB of zeros to the file it checks out. This\n # ensures that smudging doesn't mangle large files on 64-bit Windows.\n --test_expect_failure EXPENSIVE,!LONG_IS_64BIT 'files over 4GB convert on output' '\n -+test_expect_success EXPENSIVE,!LONG_IS_64BIT 'files over 4GB convert on output' '\n +-test_expect_failure EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \\\n ++test_expect_success EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \\\n + \t\t'files over 4GB convert on output' '\n \ttest_commit test small \"a small file\" &&\n \ttest_config filter.makelarge.smudge \\\n - \t\t\"test-tool genzeros $((5*1024*1024*1024)) && cat\" &&\n 5: 32472ae3f98 ! 6: 18419070c29 git-compat-util: introduce more size_t helpers\n @@ git-compat-util.h\n \n +/*\n + * Returns true if the left shift of \"a\" by \"shift\" bits will\n -+ * overflow. The types of \"a\" and \"b\" must be unsigned.\n -+ * Note that this macro evaluates \"a\" twice!\n ++ * overflow. The type of \"a\" must be unsigned.\n + */\n +#define unsigned_left_shift_overflows(a, shift) \\\n -+ ((a) > maximum_unsigned_value_of_type(a) >> shift)\n ++ ((shift) < bitsizeof(a) && \\\n ++ (a) > maximum_unsigned_value_of_type(a) >> (shift))\n +\n #ifdef __GNUC__\n #define TYPEOF(x) (__typeof__(x))\n 6: c6910584108 = 7: f59c523bcc4 odb: guard against data loss checking out a huge file\n 7: d87d4229bb4 ! 8: acc5591517f clean/smudge: allow clean filters to process extremely large files\n @@ convert.c: static int crlf_to_worktree(const char *src, size_t len, struct strbu\n \tconst char *path;\n \n ## t/t1051-large-conversion.sh ##\n -@@ t/t1051-large-conversion.sh: test_expect_success EXPENSIVE,!LONG_IS_64BIT 'files over 4GB convert on output'\n +@@ t/t1051-large-conversion.sh: test_expect_success EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \\\n \ttest \"$size\" -ge $((5 * 1024 * 1024 * 1024))\n '\n \n +# This clean filter writes down the size of input it receives. By checking against\n +# the actual size, we ensure that cleaning doesn't mangle large files on 64-bit Windows.\n -+test_expect_success EXPENSIVE,!LONG_IS_64BIT 'files over 4GB convert on input' '\n ++test_expect_success EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \\\n ++\t\t'files over 4GB convert on input' '\n +\ttest-tool genzeros $((5*1024*1024*1024)) >big &&\n +\ttest_config filter.checklarge.clean \"wc -c >big.size\" &&\n +\techo \"big filter=checklarge\" >.gitattributes &&\n\n-- \ngitgitgadget\n"},{"id":"440011","messageId":"052197200141c321118b7766f5615a61f951e59f.1635515959.git.gitgitgadget@gmail.com","threadId":"56792","inReplyTo":"pull.1068.v3.git.1635515959.gitgitgadget@gmail.com","subject":"[PATCH v3 2/8] test-tool genzeros: generate large amounts of data more efficiently","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-29T13:59:13Z","receivedAt":"2021-10-29T13:59:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn this developer's tests, producing one gigabyte worth of NULs in a\nbusy loop that writes out individual bytes, unbuffered, took ~27sec.\nWriting chunked 256kB buffers instead only took ~0.6sec\n\nThis matters because we are about to introduce a pair of test cases that\nwant to be able to produce 5GB of NULs, and we cannot use `/dev/zero`\nbecause of the HP NonStop platform's lack of support for that device.\n\nSigned-off-by: Johannes Schindelin \n---\n t/helper/test-genzeros.c | 17 +++++++++++++++--\n 1 file changed, 15 insertions(+), 2 deletions(-)\n\ndiff --git a/t/helper/test-genzeros.c b/t/helper/test-genzeros.c\nindex b1197e91a89..8ca988d6216 100644\n--- a/t/helper/test-genzeros.c\n+++ b/t/helper/test-genzeros.c\n@@ -3,7 +3,10 @@\n \n int cmd__genzeros(int argc, const char **argv)\n {\n+\t/* static, so that it is NUL-initialized */\n+\tstatic const char zeros[256 * 1024];\n \tintmax_t count;\n+\tssize_t n;\n \n \tif (argc > 2) {\n \t\tfprintf(stderr, \"usage: %s []\\n\", argv[0]);\n@@ -12,9 +15,19 @@ int cmd__genzeros(int argc, const char **argv)\n \n \tcount = argc > 1 ? strtoimax(argv[1], NULL, 0) : -1;\n \n-\twhile (count < 0 || count--) {\n-\t\tif (putchar(0) == EOF)\n+\t/* Writing out individual NUL bytes is slow... */\n+\twhile (count < 0)\n+\t\tif (write(1, zeros, ARRAY_SIZE(zeros)) < 0)\n \t\t\treturn -1;\n+\n+\twhile (count > 0) {\n+\t\tn = write(1, zeros, count < ARRAY_SIZE(zeros) ?\n+\t\t\t count : ARRAY_SIZE(zeros));\n+\n+\t\tif (n < 0)\n+\t\t\treturn -1;\n+\n+\t\tcount -= n;\n \t}\n \n \treturn 0;\n-- \ngitgitgadget\n\n"},{"id":"440012","messageId":"489500bb1dcaffecab42672658990cfc26d52d7c.1635515959.git.gitgitgadget@gmail.com","threadId":"56792","inReplyTo":"pull.1068.v3.git.1635515959.gitgitgadget@gmail.com","subject":"[PATCH v3 3/8] test-lib: add prerequisite for 64-bit platforms","fromName":"Carlo Marcelo Arenas Belón via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-29T13:59:14Z","receivedAt":"2021-10-29T13:59:28Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= \n\nAllow tests that assume a 64-bit `size_t` to be skipped in 32-bit\nplatforms and regardless of the size of `long`.\n\nThis imitates the `LONG_IS_64BIT` prerequisite.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \nSigned-off-by: Johannes Schindelin \n---\n t/test-lib.sh | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex adaf03543e8..af1a94c2c20 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1642,6 +1642,10 @@ build_option () {\n \tsed -ne \"s/^$1: //p\"\n }\n \n+test_lazy_prereq SIZE_T_IS_64BIT '\n+\ttest 8 -eq \"$(build_option sizeof-size_t)\"\n+'\n+\n test_lazy_prereq LONG_IS_64BIT '\n \ttest 8 -le \"$(build_option sizeof-long)\"\n '\n-- \ngitgitgadget\n\n"},{"id":"440013","messageId":"ce9dfaac9f8693890aa402161b38292b31d3690c.1635515959.git.gitgitgadget@gmail.com","threadId":"56792","inReplyTo":"pull.1068.v3.git.1635515959.gitgitgadget@gmail.com","subject":"[PATCH v3 4/8] t1051: introduce a smudge filter test for extremely large files","fromName":"Matt Cooper via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-29T13:59:15Z","receivedAt":"2021-10-29T13:59:32Z","isPatch":true,"sender":{"key":"name:Matt Cooper","avatar":null},"body":"From: Matt Cooper \n\nThe filter system allows for alterations to file contents when they're\nadded to the database or workdir. (\"Smudge\" when moving to the workdir;\n\"clean\" when moving to the database.) This is used natively to handle CRLF\nto LF conversions. It's also employed by Git-LFS to replace large files\nfrom the workdir with small tracking files in the repo and vice versa.\n\nGit pulls the entire smudged file into memory. While this is inefficient,\nthere's a more insidious problem on some platforms due to inconsistency\nbetween using unsigned long and size_t for the same type of data (size of\na file in bytes). On most 64-bit platforms, unsigned long is 64 bits, and\nsize_t is typedef'd to unsigned long. On Windows, however, unsigned long is\nonly 32 bits (and therefore on 64-bit Windows, size_t is typedef'd to\nunsigned long long in order to be 64 bits).\n\nPractically speaking, this means 64-bit Windows users of Git-LFS can't\nhandle files larger than 2^32 bytes. Other 64-bit platforms don't suffer\nthis limitation.\n\nThis commit introduces a test exposing the issue; future commits make it\npass. The test simulates the way Git-LFS works by having a tiny file\nchecked into the repository and expanding it to a huge file on checkout.\n\nHelped-by: Johannes Schindelin \nSigned-off-by: Matt Cooper \nSigned-off-by: Johannes Schindelin \n---\n t/t1051-large-conversion.sh | 14 ++++++++++++++\n 1 file changed, 14 insertions(+)\n\ndiff --git a/t/t1051-large-conversion.sh b/t/t1051-large-conversion.sh\nindex 8b7640b3ba8..bff86c13208 100755\n--- a/t/t1051-large-conversion.sh\n+++ b/t/t1051-large-conversion.sh\n@@ -83,4 +83,18 @@ test_expect_success 'ident converts on output' '\n \ttest_cmp small.clean large.clean\n '\n \n+# This smudge filter prepends 5GB of zeros to the file it checks out. This\n+# ensures that smudging doesn't mangle large files on 64-bit Windows.\n+test_expect_failure EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \\\n+\t\t'files over 4GB convert on output' '\n+\ttest_commit test small \"a small file\" &&\n+\ttest_config filter.makelarge.smudge \\\n+\t\t\"test-tool genzeros $((5*1024*1024*1024)) && cat\" &&\n+\techo \"small filter=makelarge\" >.gitattributes &&\n+\trm small &&\n+\tgit checkout -- small &&\n+\tsize=$(test_file_size small) &&\n+\ttest \"$size\" -ge $((5 * 1024 * 1024 * 1024))\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"440014","messageId":"dbef8168bc7044dbc8471c5ecc7146cf3e979263.1635515959.git.gitgitgadget@gmail.com","threadId":"56792","inReplyTo":"pull.1068.v3.git.1635515959.gitgitgadget@gmail.com","subject":"[PATCH v3 5/8] odb: teach read_blob_entry to use size_t","fromName":"Matt Cooper via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-29T13:59:16Z","receivedAt":"2021-10-29T13:59:34Z","isPatch":true,"sender":{"key":"name:Matt Cooper","avatar":null},"body":"From: Matt Cooper \n\nThere is mixed use of size_t and unsigned long to deal with sizes in the\ncodebase. Recall that Windows defines unsigned long as 32 bits even on\n64-bit platforms, meaning that converting size_t to unsigned long narrows\nthe range. This mostly doesn't cause a problem since Git rarely deals\nwith files larger than 2^32 bytes.\n\nBut adjunct systems such as Git LFS, which use smudge/clean filters to\nkeep huge files out of the repository, may have huge file contents passed\nthrough some of the functions in entry.c and convert.c. On Windows, this\nresults in a truncated file being written to the workdir. I traced this to\none specific use of unsigned long in write_entry (and a similar instance\nin write_pc_item_to_fd for parallel checkout). That appeared to be for\nthe call to read_blob_entry, which expects a pointer to unsigned long.\n\nBy altering the signature of read_blob_entry to expect a size_t,\nwrite_entry can be switched to use size_t internally (which all of its\ncallers and most of its callees already used). To avoid touching dozens of\nadditional files, read_blob_entry uses a local unsigned long to call a\nchain of functions which aren't prepared to accept size_t.\n\nHelped-by: Johannes Schindelin \nSigned-off-by: Matt Cooper \nSigned-off-by: Johannes Schindelin \n---\n entry.c | 8 +++++---\n entry.h | 2 +-\n parallel-checkout.c | 2 +-\n t/t1051-large-conversion.sh | 2 +-\n 4 files changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex 711ee0693c7..4cb3942dbdc 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -82,11 +82,13 @@ static int create_file(const char *path, unsigned int mode)\n \treturn open(path, O_WRONLY | O_CREAT | O_EXCL, mode);\n }\n \n-void *read_blob_entry(const struct cache_entry *ce, unsigned long *size)\n+void *read_blob_entry(const struct cache_entry *ce, size_t *size)\n {\n \tenum object_type type;\n-\tvoid *blob_data = read_object_file(&ce->oid, &type, size);\n+\tunsigned long ul;\n+\tvoid *blob_data = read_object_file(&ce->oid, &type, &ul);\n \n+\t*size = ul;\n \tif (blob_data) {\n \t\tif (type == OBJ_BLOB)\n \t\t\treturn blob_data;\n@@ -270,7 +272,7 @@ static int write_entry(struct cache_entry *ce, char *path, struct conv_attrs *ca\n \tint fd, ret, fstat_done = 0;\n \tchar *new_blob;\n \tstruct strbuf buf = STRBUF_INIT;\n-\tunsigned long size;\n+\tsize_t size;\n \tssize_t wrote;\n \tsize_t newsize = 0;\n \tstruct stat st;\ndiff --git a/entry.h b/entry.h\nindex b8c0e170dc7..61ee8c17604 100644\n--- a/entry.h\n+++ b/entry.h\n@@ -51,7 +51,7 @@ int finish_delayed_checkout(struct checkout *state, int *nr_checkouts);\n */\n void unlink_entry(const struct cache_entry *ce);\n \n-void *read_blob_entry(const struct cache_entry *ce, unsigned long *size);\n+void *read_blob_entry(const struct cache_entry *ce, size_t *size);\n int fstat_checkout_output(int fd, const struct checkout *state, struct stat *st);\n void update_ce_after_write(const struct checkout *state, struct cache_entry *ce,\n \t\t\t struct stat *st);\ndiff --git a/parallel-checkout.c b/parallel-checkout.c\nindex 6b1af32bb3d..b6f4a25642e 100644\n--- a/parallel-checkout.c\n+++ b/parallel-checkout.c\n@@ -261,7 +261,7 @@ static int write_pc_item_to_fd(struct parallel_checkout_item *pc_item, int fd,\n \tstruct stream_filter *filter;\n \tstruct strbuf buf = STRBUF_INIT;\n \tchar *blob;\n-\tunsigned long size;\n+\tsize_t size;\n \tssize_t wrote;\n \n \t/* Sanity check */\ndiff --git a/t/t1051-large-conversion.sh b/t/t1051-large-conversion.sh\nindex bff86c13208..8b23d862600 100755\n--- a/t/t1051-large-conversion.sh\n+++ b/t/t1051-large-conversion.sh\n@@ -85,7 +85,7 @@ test_expect_success 'ident converts on output' '\n \n # This smudge filter prepends 5GB of zeros to the file it checks out. This\n # ensures that smudging doesn't mangle large files on 64-bit Windows.\n-test_expect_failure EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \\\n+test_expect_success EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \\\n \t\t'files over 4GB convert on output' '\n \ttest_commit test small \"a small file\" &&\n \ttest_config filter.makelarge.smudge \\\n-- \ngitgitgadget\n\n"},{"id":"440015","messageId":"18419070c29aef85c266f01174f436566bf792fd.1635515959.git.gitgitgadget@gmail.com","threadId":"56792","inReplyTo":"pull.1068.v3.git.1635515959.gitgitgadget@gmail.com","subject":"[PATCH v3 6/8] git-compat-util: introduce more size_t helpers","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-29T13:59:17Z","receivedAt":"2021-10-29T13:59:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWe will use them in the next commit.\n\nSigned-off-by: Johannes Schindelin \n---\n git-compat-util.h | 25 +++++++++++++++++++++++++\n 1 file changed, 25 insertions(+)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex a508dbe5a35..1f41e5611a1 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -113,6 +113,14 @@\n #define unsigned_mult_overflows(a, b) \\\n ((a) && (b) > maximum_unsigned_value_of_type(a) / (a))\n \n+/*\n+ * Returns true if the left shift of \"a\" by \"shift\" bits will\n+ * overflow. The type of \"a\" must be unsigned.\n+ */\n+#define unsigned_left_shift_overflows(a, shift) \\\n+ ((shift) < bitsizeof(a) && \\\n+ (a) > maximum_unsigned_value_of_type(a) >> (shift))\n+\n #ifdef __GNUC__\n #define TYPEOF(x) (__typeof__(x))\n #else\n@@ -859,6 +867,23 @@ static inline size_t st_sub(size_t a, size_t b)\n \treturn a - b;\n }\n \n+static inline size_t st_left_shift(size_t a, unsigned shift)\n+{\n+\tif (unsigned_left_shift_overflows(a, shift))\n+\t\tdie(\"size_t overflow: %\"PRIuMAX\" << %u\",\n+\t\t (uintmax_t)a, shift);\n+\treturn a << shift;\n+}\n+\n+static inline unsigned long cast_size_t_to_ulong(size_t a)\n+{\n+\tif (a != (unsigned long)a)\n+\t\tdie(\"object too large to read on this platform: %\"\n+\t\t PRIuMAX\" is cut off to %lu\",\n+\t\t (uintmax_t)a, (unsigned long)a);\n+\treturn (unsigned long)a;\n+}\n+\n #ifdef HAVE_ALLOCA_H\n # include \n # define xalloca(size) (alloca(size))\n-- \ngitgitgadget\n\n"},{"id":"440016","messageId":"f59c523bcc48b187680be9149e9311f15bfe06dc.1635515959.git.gitgitgadget@gmail.com","threadId":"56792","inReplyTo":"pull.1068.v3.git.1635515959.gitgitgadget@gmail.com","subject":"[PATCH v3 7/8] odb: guard against data loss checking out a huge file","fromName":"Matt Cooper via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-29T13:59:18Z","receivedAt":"2021-10-29T13:59:38Z","isPatch":true,"sender":{"key":"name:Matt Cooper","avatar":null},"body":"From: Matt Cooper \n\nThis introduces an additional guard for platforms where `unsigned long`\nand `size_t` are not of the same size. If the size of an object in the\ndatabase would overflow `unsigned long`, instead we now exit with an\nerror.\n\nA complete fix will have to update _many_ other functions throughout the\ncodebase to use `size_t` instead of `unsigned long`. It will have to be\nimplemented at some stage.\n\nThis commit puts in a stop-gap for the time being.\n\nHelped-by: Johannes Schindelin \nSigned-off-by: Matt Cooper \nSigned-off-by: Johannes Schindelin \n---\n delta.h | 6 +++---\n object-file.c | 6 +++---\n packfile.c | 6 +++---\n 3 files changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/delta.h b/delta.h\nindex 2df5fe13d95..8a56ec07992 100644\n--- a/delta.h\n+++ b/delta.h\n@@ -90,15 +90,15 @@ static inline unsigned long get_delta_hdr_size(const unsigned char **datap,\n \t\t\t\t\t const unsigned char *top)\n {\n \tconst unsigned char *data = *datap;\n-\tunsigned long cmd, size = 0;\n+\tsize_t cmd, size = 0;\n \tint i = 0;\n \tdo {\n \t\tcmd = *data++;\n-\t\tsize |= (cmd & 0x7f) << i;\n+\t\tsize |= st_left_shift(cmd & 0x7f, i);\n \t\ti += 7;\n \t} while (cmd & 0x80 && data < top);\n \t*datap = data;\n-\treturn size;\n+\treturn cast_size_t_to_ulong(size);\n }\n \n #endif\ndiff --git a/object-file.c b/object-file.c\nindex f233b440b22..70e456fc2a3 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1344,7 +1344,7 @@ static int parse_loose_header_extended(const char *hdr, struct object_info *oi,\n \t\t\t\t unsigned int flags)\n {\n \tconst char *type_buf = hdr;\n-\tunsigned long size;\n+\tsize_t size;\n \tint type, type_len = 0;\n \n \t/*\n@@ -1388,12 +1388,12 @@ static int parse_loose_header_extended(const char *hdr, struct object_info *oi,\n \t\t\tif (c > 9)\n \t\t\t\tbreak;\n \t\t\thdr++;\n-\t\t\tsize = size * 10 + c;\n+\t\t\tsize = st_add(st_mult(size, 10), c);\n \t\t}\n \t}\n \n \tif (oi->sizep)\n-\t\t*oi->sizep = size;\n+\t\t*oi->sizep = cast_size_t_to_ulong(size);\n \n \t/*\n \t * The length must be followed by a zero byte\ndiff --git a/packfile.c b/packfile.c\nindex 755aa7aec5e..3ccea004396 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1059,7 +1059,7 @@ unsigned long unpack_object_header_buffer(const unsigned char *buf,\n \t\tunsigned long len, enum object_type *type, unsigned long *sizep)\n {\n \tunsigned shift;\n-\tunsigned long size, c;\n+\tsize_t size, c;\n \tunsigned long used = 0;\n \n \tc = buf[used++];\n@@ -1073,10 +1073,10 @@ unsigned long unpack_object_header_buffer(const unsigned char *buf,\n \t\t\tbreak;\n \t\t}\n \t\tc = buf[used++];\n-\t\tsize += (c & 0x7f) << shift;\n+\t\tsize = st_add(size, st_left_shift(c & 0x7f, shift));\n \t\tshift += 7;\n \t}\n-\t*sizep = size;\n+\t*sizep = cast_size_t_to_ulong(size);\n \treturn used;\n }\n \n-- \ngitgitgadget\n\n"},{"id":"440017","messageId":"acc5591517fa519fc45c07c27defb309c891dea0.1635515959.git.gitgitgadget@gmail.com","threadId":"56792","inReplyTo":"pull.1068.v3.git.1635515959.gitgitgadget@gmail.com","subject":"[PATCH v3 8/8] clean/smudge: allow clean filters to process extremely large files","fromName":"Matt Cooper via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-29T13:59:19Z","receivedAt":"2021-10-29T13:59:40Z","isPatch":true,"sender":{"key":"name:Matt Cooper","avatar":null},"body":"From: Matt Cooper \n\nThe filter system allows for alterations to file contents when they're\nmoved between the database and the worktree. We already made sure that\nit is possible for smudge filters to produce contents that are larger\nthan `unsigned long` can represent (which matters on systems where\n`unsigned long` is narrower than `size_t`, most notably 64-bit Windows).\nNow we make sure that clean filters can _consume_ contents that are\nlarger than that.\n\nNote that this commit only allows clean filters' _input_ to be larger\nthan can be represented by `unsigned long`.\n\nThis change makes only a very minute dent into the much larger project\nto teach Git to use `size_t` instead of `unsigned long` wherever\nappropriate.\n\nHelped-by: Johannes Schindelin \nSigned-off-by: Matt Cooper \nSigned-off-by: Johannes Schindelin \n---\n convert.c | 2 +-\n t/t1051-large-conversion.sh | 11 +++++++++++\n 2 files changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/convert.c b/convert.c\nindex fd9c84b0257..5ad6dfc08a0 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -613,7 +613,7 @@ static int crlf_to_worktree(const char *src, size_t len, struct strbuf *buf,\n \n struct filter_params {\n \tconst char *src;\n-\tunsigned long size;\n+\tsize_t size;\n \tint fd;\n \tconst char *cmd;\n \tconst char *path;\ndiff --git a/t/t1051-large-conversion.sh b/t/t1051-large-conversion.sh\nindex 8b23d862600..d4cfe8bf5de 100755\n--- a/t/t1051-large-conversion.sh\n+++ b/t/t1051-large-conversion.sh\n@@ -97,4 +97,15 @@ test_expect_success EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \\\n \ttest \"$size\" -ge $((5 * 1024 * 1024 * 1024))\n '\n \n+# This clean filter writes down the size of input it receives. By checking against\n+# the actual size, we ensure that cleaning doesn't mangle large files on 64-bit Windows.\n+test_expect_success EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \\\n+\t\t'files over 4GB convert on input' '\n+\ttest-tool genzeros $((5*1024*1024*1024)) >big &&\n+\ttest_config filter.checklarge.clean \"wc -c >big.size\" &&\n+\techo \"big filter=checklarge\" >.gitattributes &&\n+\tgit add big &&\n+\ttest $(test_file_size big) -eq $(cat big.size)\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"440037","messageId":"xmqqk0hvllxp.fsf@gitster.g","threadId":"56792","inReplyTo":"pull.1068.v3.git.1635515959.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 0/8] Allow clean/smudge filters to handle huge files in the LLP64 data model","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-29T18:34:26Z","receivedAt":"2021-10-29T18:34:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> This patch series came in via the Git for Windows fork\n> [https://github.com/git-for-windows/git/pull/3487], and I intend to merge it\n> before v2.34.0-rc0, therefore I appreciate every careful review you gentle\n> people can spare.\n\nIt is way too late for my tree to go in before -rc0, but the patches\nin the last round, with the \"Changes since v2\" description below,\nall sound sensible, including the decision to stop here, instead of\ndoing \"everything should be either size_t or intmax_t\" conversion.\n\n7/8 did not apply for me to the tip of 'master', but \"am -3\" wiggled\nit in. You may want to double check the results.\n\nAs the primary author of the series, given the cover title matches\nthe title of one step in the series, seems to be Matt, let me queue\nthem under mc/clean-smudge-with-llp64 topic.\n\nThanks.\n"},{"id":"440072","messageId":"nycvar.QRO.7.76.6.2110292311200.56@tvgsbejvaqbjf.bet","threadId":"56792","inReplyTo":"nycvar.QRO.7.76.6.2110292239170.56@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v3 0/8] Allow clean/smudge filters to handle huge files in the LLP64 data model","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmail.com","sentAt":"2021-10-29T21:12:16Z","receivedAt":"2021-10-29T21:12:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmail.com","avatar":null},"body":"[Re-sending, as it seems that the Git mailing list is causing trouble\nagain, at least I do not see this on lore.kernel.org/git]\n\n\nOn Fri, 29 Oct 2021, Johannes Schindelin wrote:\n\n> Hi Junio,\n> \n> On Fri, 29 Oct 2021, Junio C Hamano wrote:\n> \n> > \"Johannes Schindelin via GitGitGadget\" \n> > writes:\n> > \n> > > This patch series came in via the Git for Windows fork\n> > > [https://github.com/git-for-windows/git/pull/3487], and I intend to merge it\n> > > before v2.34.0-rc0, therefore I appreciate every careful review you gentle\n> > > people can spare.\n> > \n> > It is way too late for my tree to go in before -rc0, but the patches\n> > in the last round, with the \"Changes since v2\" description below,\n> > all sound sensible, including the decision to stop here, instead of\n> > doing \"everything should be either size_t or intmax_t\" conversion.\n> \n> Thank you for your careful review, it definitely helped with polishing the\n> patches.\n> \n> > 7/8 did not apply for me to the tip of 'master', but \"am -3\" wiggled\n> > it in. You may want to double check the results.\n> \n> Right, I purposefully based the patches on v2.32.0 so they would merge\n> cleanly into Git for Windows' `main` branch.\n> \n> I should have clarified that I was talking about merging that PR into Git\n> for Windows before the -rc0 ;-)\n> \n> Speaking of which, -rc0 is still coming, right? ;-)\n> https://tinyurl.com/gitcal still claims that it was scheduled for\n> yesterday.\n> \n> > As the primary author of the series, given the cover title matches the\n> > title of one step in the series, seems to be Matt, let me queue them\n> > under mc/clean-smudge-with-llp64 topic.\n> \n> Absolutely. Matt was in the driving seat, I was just reviewing and helping\n> here and there, and then shepherding the patch series upstream. It was my\n> decision to start upstreaming before merging it into Git for Windows, but\n> the plan was all along to get this into Git for Windows v2.34.0 because\n> there are Git LFS users using Windows who are eagerly awaiting this fix.\n> \n> I am not aware of any other popular platform using the LLP64 data model,\n> therefore I do not even think that these patches have to be fast-tracked\n> into Git v2.34.0, next cycle would be good enough. Unless you are aware of\n> other such platforms that do not rely on the Git for Windows fork, but on\n> Git built from your repository?\n> \n> Ciao,\n> Dscho\n> \n"},{"id":"440102","messageId":"xmqqy26bh2cy.fsf@gitster.g","threadId":"56792","inReplyTo":"052197200141c321118b7766f5615a61f951e59f.1635515959.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 2/8] test-tool genzeros: generate large amounts of data more efficiently","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-29T22:50:53Z","receivedAt":"2021-10-29T22:51:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> @@ -12,9 +15,19 @@ int cmd__genzeros(int argc, const char **argv)\n> \n> \tcount = argc > 1 ? strtoimax(argv[1], NULL, 0) : -1;\n> \n> -\twhile (count < 0 || count--) {\n> -\t\tif (putchar(0) == EOF)\n> +\t/* Writing out individual NUL bytes is slow... */\n> +\twhile (count < 0)\n> +\t\tif (write(1, zeros, ARRAY_SIZE(zeros)) < 0)\n> \t\t\treturn -1;\n> +\n> +\twhile (count > 0) {\n> +\t\tn = write(1, zeros, count < ARRAY_SIZE(zeros) ?\n> +\t\t\t count : ARRAY_SIZE(zeros));\n> +\n> +\t\tif (n < 0)\n> +\t\t\treturn -1;\n> +\n> +\t\tcount -= n;\n> \t}\n> \n> \treturn 0;\n\nThis round looks OK to me.\n"},{"id":"440103","messageId":"xmqqtugzh29w.fsf@gitster.g","threadId":"56792","inReplyTo":"489500bb1dcaffecab42672658990cfc26d52d7c.1635515959.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 3/8] test-lib: add prerequisite for 64-bit platforms","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-29T22:52:43Z","receivedAt":"2021-10-29T22:52:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Carlo Marcelo Arenas Belón via GitGitGadget\"\n writes:\n\n> +test_lazy_prereq SIZE_T_IS_64BIT '\n> +\ttest 8 -eq \"$(build_option sizeof-size_t)\"\n> +'\n> +\n> test_lazy_prereq LONG_IS_64BIT '\n> \ttest 8 -le \"$(build_option sizeof-long)\"\n> '\n\nIn the longer run, LONG_IS_64BIT wants to be renamed to indicate\nthat it is at least 64-bit long. LONG_HAS_64BIT, perhaps?\n\nObviously it can be left outside the scope of this series.\n"},{"id":"440104","messageId":"xmqqo877h1x5.fsf@gitster.g","threadId":"56792","inReplyTo":"ce9dfaac9f8693890aa402161b38292b31d3690c.1635515959.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 4/8] t1051: introduce a smudge filter test for extremely large files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-29T23:00:22Z","receivedAt":"2021-10-29T23:00:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Matt Cooper via GitGitGadget\" writes:\n\n> From: Matt Cooper \n>\n> The filter system allows for alterations to file contents when they're\n> added to the database or workdir. (\"Smudge\" when moving to the workdir;\n> \"clean\" when moving to the database.) This is used natively to handle CRLF\n> to LF conversions. It's also employed by Git-LFS to replace large files\n> from the workdir with small tracking files in the repo and vice versa.\n\nNot a huge deal, but make it a habit to spell \"working tree\" not \"workdir\",\nas someday you'd write end-user facing documentation in our tree ;-).\n \n> Git pulls the entire smudged file into memory.\n\nGiving \"for what\" would be helpful to readers.\n\n Git reads the entire smudged file into memory to convert it into\n a \"clean\" form to be used in-core.\n\n> While this is inefficient,\n> there's a more insidious problem on some platforms due to inconsistency\n> between using unsigned long and size_t for the same type of data (size of\n> a file in bytes). On most 64-bit platforms, unsigned long is 64 bits, and\n> size_t is typedef'd to unsigned long. On Windows, however, unsigned long is\n> only 32 bits (and therefore on 64-bit Windows, size_t is typedef'd to\n> unsigned long long in order to be 64 bits).\n>\n> Practically speaking, this means 64-bit Windows users of Git-LFS can't\n> handle files larger than 2^32 bytes. Other 64-bit platforms don't suffer\n> this limitation.\n>\n> This commit introduces a test exposing the issue; future commits make it\n> pass. The test simulates the way Git-LFS works by having a tiny file\n> checked into the repository and expanding it to a huge file on checkout.\n>\n> Helped-by: Johannes Schindelin \n> Signed-off-by: Matt Cooper \n> Signed-off-by: Johannes Schindelin \n> ---\n> t/t1051-large-conversion.sh | 14 ++++++++++++++\n> 1 file changed, 14 insertions(+)\n>\n> diff --git a/t/t1051-large-conversion.sh b/t/t1051-large-conversion.sh\n> index 8b7640b3ba8..bff86c13208 100755\n> --- a/t/t1051-large-conversion.sh\n> +++ b/t/t1051-large-conversion.sh\n> @@ -83,4 +83,18 @@ test_expect_success 'ident converts on output' '\n> \ttest_cmp small.clean large.clean\n> '\n> \n> +# This smudge filter prepends 5GB of zeros to the file it checks out. This\n> +# ensures that smudging doesn't mangle large files on 64-bit Windows.\n> +test_expect_failure EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \\\n> +\t\t'files over 4GB convert on output' '\n> +\ttest_commit test small \"a small file\" &&\n> +\ttest_config filter.makelarge.smudge \\\n> +\t\t\"test-tool genzeros $((5*1024*1024*1024)) && cat\" &&\n> +\techo \"small filter=makelarge\" >.gitattributes &&\n> +\trm small &&\n> +\tgit checkout -- small &&\n> +\tsize=$(test_file_size small) &&\n> +\ttest \"$size\" -ge $((5 * 1024 * 1024 * 1024))\n> +'\n\nWhy not exactly 5G, but anything that is at least 5G is OK?\n\nThanks.\n\n> test_done\n"},{"id":"440105","messageId":"xmqqcznnh1gc.fsf@gitster.g","threadId":"56792","inReplyTo":"18419070c29aef85c266f01174f436566bf792fd.1635515959.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 6/8] git-compat-util: introduce more size_t helpers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-29T23:10:27Z","receivedAt":"2021-10-29T23:10:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> +/*\n> + * Returns true if the left shift of \"a\" by \"shift\" bits will\n> + * overflow. The type of \"a\" must be unsigned.\n> + */\n> +#define unsigned_left_shift_overflows(a, shift) \\\n> + ((shift) < bitsizeof(a) && \\\n> + (a) > maximum_unsigned_value_of_type(a) >> (shift))\n\nCute. So somebody asks\n\n if (unsigned_left_shift_overflows(a, 100)\n\nand they get \"you are unsafe, regardless of the value of a, you get\nan overflow\". Makes perfect sensen.\n\n> #ifdef __GNUC__\n> #define TYPEOF(x) (__typeof__(x))\n> #else\n> @@ -859,6 +867,23 @@ static inline size_t st_sub(size_t a, size_t b)\n> \treturn a - b;\n> }\n> \n> +static inline size_t st_left_shift(size_t a, unsigned shift)\n> +{\n> +\tif (unsigned_left_shift_overflows(a, shift))\n> +\t\tdie(\"size_t overflow: %\"PRIuMAX\" << %u\",\n> +\t\t (uintmax_t)a, shift);\n> +\treturn a << shift;\n> +}\n\nMakes sense.\n\n> +static inline unsigned long cast_size_t_to_ulong(size_t a)\n> +{\n> +\tif (a != (unsigned long)a)\n> +\t\tdie(\"object too large to read on this platform: %\"\n> +\t\t PRIuMAX\" is cut off to %lu\",\n> +\t\t (uintmax_t)a, (unsigned long)a);\n> +\treturn (unsigned long)a;\n> +}\n> +\n> #ifdef HAVE_ALLOCA_H\n> # include \n> # define xalloca(size) (alloca(size))\n"},{"id":"440107","messageId":"xmqq8rybh1b2.fsf@gitster.g","threadId":"56792","inReplyTo":"f59c523bcc48b187680be9149e9311f15bfe06dc.1635515959.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 7/8] odb: guard against data loss checking out a huge file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-29T23:13:37Z","receivedAt":"2021-10-29T23:13:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Matt Cooper via GitGitGadget\" writes:\n\n> diff --git a/delta.h b/delta.h\n> index 2df5fe13d95..8a56ec07992 100644\n> --- a/delta.h\n> +++ b/delta.h\n> @@ -90,15 +90,15 @@ static inline unsigned long get_delta_hdr_size(const unsigned char **datap,\n> \t\t\t\t\t const unsigned char *top)\n> {\n> \tconst unsigned char *data = *datap;\n> -\tunsigned long cmd, size = 0;\n> +\tsize_t cmd, size = 0;\n> \tint i = 0;\n> \tdo {\n> \t\tcmd = *data++;\n> -\t\tsize |= (cmd & 0x7f) << i;\n> +\t\tsize |= st_left_shift(cmd & 0x7f, i);\n> \t\ti += 7;\n> \t} while (cmd & 0x80 && data < top);\n> \t*datap = data;\n> -\treturn size;\n> +\treturn cast_size_t_to_ulong(size);\n> }\n\nOK. The patch-delta code is reasonably self contained, so the next\nstep to measure the in-core object size in size_t shouldn't be too\nbad, but before everything got size_t aware, we have to have the\n\"safe casting\" somewhere, and I agree this \"immediately before\nreturn\" is a good place to draw the line.\n\nNicely done.\n\n> \t\t\tif (c > 9)\n> \t\t\t\tbreak;\n> \t\t\thdr++;\n> -\t\t\tsize = size * 10 + c;\n> +\t\t\tsize = st_add(st_mult(size, 10), c);\n\nNice.\n\n> \tif (oi->sizep)\n> -\t\t*oi->sizep = size;\n> +\t\t*oi->sizep = cast_size_t_to_ulong(size);\n\nOK.\n\n> @@ -1073,10 +1073,10 @@ unsigned long unpack_object_header_buffer(const unsigned char *buf,\n> \t\t\tbreak;\n> \t\t}\n> \t\tc = buf[used++];\n> -\t\tsize += (c & 0x7f) << shift;\n> +\t\tsize = st_add(size, st_left_shift(c & 0x7f, shift));\n> \t\tshift += 7;\n> \t}\n> -\t*sizep = size;\n> +\t*sizep = cast_size_t_to_ulong(size);\n\nOK.\n\nLooking good.\n"},{"id":"440108","messageId":"xmqq4k8zh14r.fsf@gitster.g","threadId":"56792","inReplyTo":"acc5591517fa519fc45c07c27defb309c891dea0.1635515959.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 8/8] clean/smudge: allow clean filters to process extremely large files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-29T23:17:24Z","receivedAt":"2021-10-29T23:17:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Matt Cooper via GitGitGadget\" writes:\n\n> +# This clean filter writes down the size of input it receives. By checking against\n> +# the actual size, we ensure that cleaning doesn't mangle large files on 64-bit Windows.\n> +test_expect_success EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \\\n> +\t\t'files over 4GB convert on input' '\n> +\ttest-tool genzeros $((5*1024*1024*1024)) >big &&\n> +\ttest_config filter.checklarge.clean \"wc -c >big.size\" &&\n> +\techo \"big filter=checklarge\" >.gitattributes &&\n> +\tgit add big &&\n> +\ttest $(test_file_size big) -eq $(cat big.size)\n> +'\n\nI would have expected that the clean filter to be sending the count\nto its standard output (to be hashed and made into a blob object),\nand the test wuld be doing \"git cat-file blob :big\" to read the\ncontents of the raw blob, bypassing the filter system. But we are\ntesting with only a single path anyway, use of this single extra\nfile is OK.\n\nLooking good.\n\n"},{"id":"440109","messageId":"xmqqwnlvfmjr.fsf@gitster.g","threadId":"56792","inReplyTo":"dbef8168bc7044dbc8471c5ecc7146cf3e979263.1635515959.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 5/8] odb: teach read_blob_entry to use size_t","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-29T23:17:44Z","receivedAt":"2021-10-29T23:17:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Matt Cooper via GitGitGadget\" writes:\n\n> -void *read_blob_entry(const struct cache_entry *ce, unsigned long *size)\n> +void *read_blob_entry(const struct cache_entry *ce, size_t *size)\n> {\n> \tenum object_type type;\n> -\tvoid *blob_data = read_object_file(&ce->oid, &type, size);\n> +\tunsigned long ul;\n> +\tvoid *blob_data = read_object_file(&ce->oid, &type, &ul);\n> \n> +\t*size = ul;\n\nIt is a bit curious place to draw the line; we want to make sure\nthat blob_entry can hold huge data, but in this step we do not mind\nread_object_file() is not capable of going full 64-bit?\n\nI guess I'll see soon enough why by reading later steps. I can see\nthat for the purpose of making write_entry() aware of the size_t,\nthis is necessary at the minimum.\n\nLooking good.\n\n> \tif (blob_data) {\n> \t\tif (type == OBJ_BLOB)\n> \t\t\treturn blob_data;\n> @@ -270,7 +272,7 @@ static int write_entry(struct cache_entry *ce, char *path, struct conv_attrs *ca\n> \tint fd, ret, fstat_done = 0;\n> \tchar *new_blob;\n> \tstruct strbuf buf = STRBUF_INIT;\n> -\tunsigned long size;\n> +\tsize_t size;\n> \tssize_t wrote;\n> \tsize_t newsize = 0;\n> \tstruct stat st;\n> diff --git a/entry.h b/entry.h\n> index b8c0e170dc7..61ee8c17604 100644\n> --- a/entry.h\n> +++ b/entry.h\n> @@ -51,7 +51,7 @@ int finish_delayed_checkout(struct checkout *state, int *nr_checkouts);\n> */\n> void unlink_entry(const struct cache_entry *ce);\n> \n> -void *read_blob_entry(const struct cache_entry *ce, unsigned long *size);\n> +void *read_blob_entry(const struct cache_entry *ce, size_t *size);\n> int fstat_checkout_output(int fd, const struct checkout *state, struct stat *st);\n> void update_ce_after_write(const struct checkout *state, struct cache_entry *ce,\n> \t\t\t struct stat *st);\n> diff --git a/parallel-checkout.c b/parallel-checkout.c\n> index 6b1af32bb3d..b6f4a25642e 100644\n> --- a/parallel-checkout.c\n> +++ b/parallel-checkout.c\n> @@ -261,7 +261,7 @@ static int write_pc_item_to_fd(struct parallel_checkout_item *pc_item, int fd,\n> \tstruct stream_filter *filter;\n> \tstruct strbuf buf = STRBUF_INIT;\n> \tchar *blob;\n> -\tunsigned long size;\n> +\tsize_t size;\n> \tssize_t wrote;\n> \n> \t/* Sanity check */\n> diff --git a/t/t1051-large-conversion.sh b/t/t1051-large-conversion.sh\n> index bff86c13208..8b23d862600 100755\n> --- a/t/t1051-large-conversion.sh\n> +++ b/t/t1051-large-conversion.sh\n> @@ -85,7 +85,7 @@ test_expect_success 'ident converts on output' '\n> \n> # This smudge filter prepends 5GB of zeros to the file it checks out. This\n> # ensures that smudging doesn't mangle large files on 64-bit Windows.\n> -test_expect_failure EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \\\n> +test_expect_success EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \\\n> \t\t'files over 4GB convert on output' '\n> \ttest_commit test small \"a small file\" &&\n> \ttest_config filter.makelarge.smudge \\\n"},{"id":"440111","messageId":"xmqqlf2bfmdb.fsf@gitster.g","threadId":"56792","inReplyTo":"xmqqo877h1x5.fsf@gitster.g","subject":"Re: [PATCH v3 4/8] t1051: introduce a smudge filter test for extremely large files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-29T23:21:36Z","receivedAt":"2021-10-29T23:21:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> +# This smudge filter prepends 5GB of zeros to the file it checks out. This\n>> +# ensures that smudging doesn't mangle large files on 64-bit Windows.\n>> +test_expect_failure EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \\\n>> +\t\t'files over 4GB convert on output' '\n>> +\ttest_commit test small \"a small file\" &&\n>> +\ttest_config filter.makelarge.smudge \\\n>> +\t\t\"test-tool genzeros $((5*1024*1024*1024)) && cat\" &&\n>> +\techo \"small filter=makelarge\" >.gitattributes &&\n>> +\trm small &&\n>> +\tgit checkout -- small &&\n>> +\tsize=$(test_file_size small) &&\n>> +\ttest \"$size\" -ge $((5 * 1024 * 1024 * 1024))\n>> +'\n>\n> Why not exactly 5G, but anything that is at least 5G is OK?\n\nI know it is more than 5G, thanks to the \"&& cat\". THe question was\nwhy aren't we measuring the size of \"a small file\" so that we can\ncheck against an exact size to be expected.\n\nThanks.\n"},{"id":"440112","messageId":"xmqqh7czfm6f.fsf@gitster.g","threadId":"56792","inReplyTo":"nycvar.QRO.7.76.6.2110292311200.56@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v3 0/8] Allow clean/smudge filters to handle huge files in the LLP64 data model","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-29T23:25:44Z","receivedAt":"2021-10-29T23:25:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> Speaking of which, -rc0 is still coming, right? ;-)\n>> https://tinyurl.com/gitcal still claims that it was scheduled for\n>> yesterday.\n\nWas swamped too deeply to even make noises about the delay. Sorry\nabout that.\n\n"},{"id":"440125","messageId":"ae900419-39ca-10f6-8ff2-d278f1b8e2e0@iee.email","threadId":"56792","inReplyTo":"nycvar.QRO.7.76.6.2110292311200.56@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v3 0/8] Allow clean/smudge filters to handle huge files in the LLP64 data model","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.email","sentAt":"2021-10-30T15:16:31Z","receivedAt":"2021-10-30T15:16:35Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"\nOn 29/10/2021 22:12, Johannes Schindelin wrote:\n> I am not aware of any other popular platform using the LLP64 data model,\n> therefore I do not even think that these patches have to be fast-tracked\n> into Git v2.34.0, next cycle would be good enough. Unless you are aware of\n> other such platforms that do not rely on the Git for Windows fork, but on\n> Git built from your repository?\n\nI was under the impression that the original Raspberry Pi also used the\nLLP64 model, or similar, and that had started of Torsten (tboegi) on the\nextensive early work on this. I was just looking at the zlib parts\nfollowing the Git Merge.\n\nTorsten was compiling for Rasbian (gcc (Raspbian 6.3.0-18+rpi1+deb9u1)\n6.3.0 20170516)\n\nPhilip\n"},{"id":"440131","messageId":"20211030173547.fse5iuq7omvfalke@tb-raspi4","threadId":"56792","inReplyTo":"ae900419-39ca-10f6-8ff2-d278f1b8e2e0@iee.email","subject":"Re: [PATCH v3 0/8] Allow clean/smudge filters to handle huge files in the LLP64 data model","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2021-10-30T17:35:47Z","receivedAt":"2021-10-30T17:36:05Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Sat, Oct 30, 2021 at 04:16:31PM +0100, Philip Oakley wrote:\n>\n> On 29/10/2021 22:12, Johannes Schindelin wrote:\n> > I am not aware of any other popular platform using the LLP64 data model,\n> > therefore I do not even think that these patches have to be fast-tracked\n> > into Git v2.34.0, next cycle would be good enough. Unless you are aware of\n> > other such platforms that do not rely on the Git for Windows fork, but on\n> > Git built from your repository?\n>\n> I was under the impression that the original Raspberry Pi also used the\n> LLP64 model, or similar, and that had started of Torsten (tboegi) on the\n> extensive early work on this. I was just looking at the zlib parts\n> following the Git Merge.\n>\n> Torsten was compiling for Rasbian (gcc (Raspbian 6.3.0-18+rpi1+deb9u1)\n> 6.3.0 20170516)\n>\n> Philip\n\nThe raspi does not use LLP64.\nHowever, the gcc from above did warn about a long - size_t mixup.\nEven if both are 32 bit.\n"},{"id":"440133","messageId":"d42213c8-c07c-eedf-6011-bc5d9996a419@iee.email","threadId":"56792","inReplyTo":"20211030173547.fse5iuq7omvfalke@tb-raspi4","subject":"Re: [PATCH v3 0/8] Allow clean/smudge filters to handle huge files in the LLP64 data model","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.email","sentAt":"2021-10-30T19:29:16Z","receivedAt":"2021-10-30T19:29:20Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"Hi Torsten,\nOn 30/10/2021 18:35, Torsten Bögershausen wrote:\n> On Sat, Oct 30, 2021 at 04:16:31PM +0100, Philip Oakley wrote:\n>> On 29/10/2021 22:12, Johannes Schindelin wrote:\n>>> I am not aware of any other popular platform using the LLP64 data model,\n>>> therefore I do not even think that these patches have to be fast-tracked\n>>> into Git v2.34.0, next cycle would be good enough. Unless you are aware of\n>>> other such platforms that do not rely on the Git for Windows fork, but on\n>>> Git built from your repository?\n>> I was under the impression that the original Raspberry Pi also used the\n>> LLP64 model, or similar, and that had started of Torsten (tboegi) on the\n>> extensive early work on this. I was just looking at the zlib parts\n>> following the Git Merge.\n>>\n>> Torsten was compiling for Rasbian (gcc (Raspbian 6.3.0-18+rpi1+deb9u1)\n>> 6.3.0 20170516)\n>>\n>> Philip\n> The raspi does not use LLP64.\n> However, the gcc from above did warn about a long - size_t mixup.\n> Even if both are 32 bit.\nThanks for the clarification. Looks like I'd misinterpreted what you'd\nsaid at the time.\n--\nPhilip\n"},{"id":"440314","messageId":"nycvar.QRO.7.76.6.2111021531070.56@tvgsbejvaqbjf.bet","threadId":"56792","inReplyTo":"xmqqtugzh29w.fsf@gitster.g","subject":"Re: [PATCH v3 3/8] test-lib: add prerequisite for 64-bit platforms","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-11-02T14:35:02Z","receivedAt":"2021-11-02T14:35:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 29 Oct 2021, Junio C Hamano wrote:\n\n> \"Carlo Marcelo Arenas Belón via GitGitGadget\"\n> writes:\n>\n> > +test_lazy_prereq SIZE_T_IS_64BIT '\n> > +\ttest 8 -eq \"$(build_option sizeof-size_t)\"\n> > +'\n> > +\n> > test_lazy_prereq LONG_IS_64BIT '\n> > \ttest 8 -le \"$(build_option sizeof-long)\"\n> > '\n>\n> In the longer run, LONG_IS_64BIT wants to be renamed to indicate\n> that it is at least 64-bit long. LONG_HAS_64BIT, perhaps?\n\nOr `LONG_AT_LEAST_64BIT`. It does look as if the current users are asking\nfor that, not for precisely 64-bit.\n\n> Obviously it can be left outside the scope of this series.\n\nDefinitely. The patch series already grew from 5 to 8 patches. We really\nneed to be more conscious of scope here.\n\nCiao,\nDscho\n"},{"id":"440315","messageId":"nycvar.QRO.7.76.6.2111021535240.56@tvgsbejvaqbjf.bet","threadId":"56792","inReplyTo":"xmqqk0hvllxp.fsf@gitster.g","subject":"Re: [PATCH v3 0/8] Allow clean/smudge filters to handle huge files in the LLP64 data model","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-11-02T14:41:13Z","receivedAt":"2021-11-02T14:41:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 29 Oct 2021, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > This patch series came in via the Git for Windows fork\n> > [https://github.com/git-for-windows/git/pull/3487], and I intend to merge it\n> > before v2.34.0-rc0, therefore I appreciate every careful review you gentle\n> > people can spare.\n>\n> It is way too late for my tree to go in before -rc0,\n\nAs I said, the intention was to merge this into Git _for Windows_ before\n-rc0. And I did. The non-Windows platforms are much less likely to benefit\nfrom this patch series.\n\n> but the patches in the last round, with the \"Changes since v2\"\n> description below, all sound sensible, including the decision to stop\n> here, instead of doing \"everything should be either size_t or intmax_t\"\n> conversion.\n\nYep, if you want to weep, you can have a look at\nhttps://github.com/git-for-windows/git/pull/2179/files, which tries to do\nthat `unsigned long` -> `size_t` thing (although admittedly in an as yet\nunreviewable shape).\n\n> 7/8 did not apply for me to the tip of 'master', but \"am -3\" wiggled\n> it in. You may want to double check the results.\n\nThe range-diff looks good.\n\n> As the primary author of the series, given the cover title matches\n> the title of one step in the series, seems to be Matt, let me queue\n> them under mc/clean-smudge-with-llp64 topic.\n\nYep, this came in via Git for Windows, traditionally I take it upon me\nto shepherd those contributions into upstream Git.\n\nCiao,\nDscho\n"},{"id":"440317","messageId":"nycvar.QRO.7.76.6.2111021542110.56@tvgsbejvaqbjf.bet","threadId":"56792","inReplyTo":"xmqqlf2bfmdb.fsf@gitster.g","subject":"Re: [PATCH v3 4/8] t1051: introduce a smudge filter test for extremely large files","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-11-02T14:56:00Z","receivedAt":"2021-11-02T14:57:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 29 Oct 2021, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n>\n> >> +# This smudge filter prepends 5GB of zeros to the file it checks out. This\n> >> +# ensures that smudging doesn't mangle large files on 64-bit Windows.\n> >> +test_expect_failure EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \\\n> >> +\t\t'files over 4GB convert on output' '\n> >> +\ttest_commit test small \"a small file\" &&\n> >> +\ttest_config filter.makelarge.smudge \\\n> >> +\t\t\"test-tool genzeros $((5*1024*1024*1024)) && cat\" &&\n> >> +\techo \"small filter=makelarge\" >.gitattributes &&\n> >> +\trm small &&\n> >> +\tgit checkout -- small &&\n> >> +\tsize=$(test_file_size small) &&\n> >> +\ttest \"$size\" -ge $((5 * 1024 * 1024 * 1024))\n> >> +'\n> >\n> > Why not exactly 5G, but anything that is at least 5G is OK?\n>\n> I know it is more than 5G, thanks to the \"&& cat\". THe question was\n> why aren't we measuring the size of \"a small file\" so that we can\n> check against an exact size to be expected.\n\nThe main problem solved by this patch series is the fact that by virtue of\nusing `unsigned long`, Git truncated the contents to less than 4GB. So,\nreally, what we care about here is that that does not happen anymore.\n\nI will change it to take the extra time to determine `small`'s file size\nand add that to 5GB, to avoid confusing readers, though.\n\nCiao,\nDscho\n"},{"id":"440318","messageId":"nycvar.QRO.7.76.6.2111021556480.56@tvgsbejvaqbjf.bet","threadId":"56792","inReplyTo":"xmqqo877h1x5.fsf@gitster.g","subject":"Re: [PATCH v3 4/8] t1051: introduce a smudge filter test for extremely large files","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-11-02T14:57:01Z","receivedAt":"2021-11-02T14:57:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 29 Oct 2021, Junio C Hamano wrote:\n\n> \"Matt Cooper via GitGitGadget\" writes:\n>\n> > From: Matt Cooper \n> >\n> > The filter system allows for alterations to file contents when they're\n> > added to the database or workdir. (\"Smudge\" when moving to the workdir;\n> > \"clean\" when moving to the database.) This is used natively to handle CRLF\n> > to LF conversions. It's also employed by Git-LFS to replace large files\n> > from the workdir with small tracking files in the repo and vice versa.\n>\n> Not a huge deal, but make it a habit to spell \"working tree\" not \"workdir\",\n> as someday you'd write end-user facing documentation in our tree ;-).\n>\n> > Git pulls the entire smudged file into memory.\n>\n> Giving \"for what\" would be helpful to readers.\n>\n> Git reads the entire smudged file into memory to convert it into\n> a \"clean\" form to be used in-core.\n\nI changed both.\n\nCiao,\nDscho\n"},{"id":"440319","messageId":"nycvar.QRO.7.76.6.2111021557500.56@tvgsbejvaqbjf.bet","threadId":"56792","inReplyTo":"xmqq4k8zh14r.fsf@gitster.g","subject":"Re: [PATCH v3 8/8] clean/smudge: allow clean filters to process extremely large files","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-11-02T14:59:21Z","receivedAt":"2021-11-02T14:59:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 29 Oct 2021, Junio C Hamano wrote:\n\n> \"Matt Cooper via GitGitGadget\" writes:\n>\n> > +# This clean filter writes down the size of input it receives. By checking against\n> > +# the actual size, we ensure that cleaning doesn't mangle large files on 64-bit Windows.\n> > +test_expect_success EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \\\n> > +\t\t'files over 4GB convert on input' '\n> > +\ttest-tool genzeros $((5*1024*1024*1024)) >big &&\n> > +\ttest_config filter.checklarge.clean \"wc -c >big.size\" &&\n> > +\techo \"big filter=checklarge\" >.gitattributes &&\n> > +\tgit add big &&\n> > +\ttest $(test_file_size big) -eq $(cat big.size)\n> > +'\n>\n> I would have expected that the clean filter to be sending the count\n> to its standard output (to be hashed and made into a blob object),\n> and the test wuld be doing \"git cat-file blob :big\" to read the\n> contents of the raw blob, bypassing the filter system.\n\nThat was exactly what Matt had in his first iteration. But I dislike\nunnecessarily spawned processes, they are not \"free\" on Windows, so I\nshortened the design to take this shortcut.\n\n> But we are testing with only a single path anyway, use of this single\n> extra file is OK.\n\nPrecisely.\n\nCiao,\nDscho\n"},{"id":"440320","messageId":"nycvar.QRO.7.76.6.2111021601360.56@tvgsbejvaqbjf.bet","threadId":"56792","inReplyTo":"xmqqwnlvfmjr.fsf@gitster.g","subject":"Re: [PATCH v3 5/8] odb: teach read_blob_entry to use size_t","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-11-02T15:10:57Z","receivedAt":"2021-11-02T15:12:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 29 Oct 2021, Junio C Hamano wrote:\n\n> \"Matt Cooper via GitGitGadget\" writes:\n>\n> > -void *read_blob_entry(const struct cache_entry *ce, unsigned long *size)\n> > +void *read_blob_entry(const struct cache_entry *ce, size_t *size)\n> > {\n> > \tenum object_type type;\n> > -\tvoid *blob_data = read_object_file(&ce->oid, &type, size);\n> > +\tunsigned long ul;\n> > +\tvoid *blob_data = read_object_file(&ce->oid, &type, &ul);\n> >\n> > +\t*size = ul;\n>\n> It is a bit curious place to draw the line; we want to make sure\n> that blob_entry can hold huge data, but in this step we do not mind\n> read_object_file() is not capable of going full 64-bit?\n\nIndeed that is the case. The consideration here is: how large of a patch\nseries do I want to take this late in the cycle?\n\nHere is the call tree (for full details, look no further than\nhttps://github.com/git-for-windows/git/pull/3487#issuecomment-950727616):\n\n read_object_file()\n repo_read_object_file()\n read_object_file_extended()\n read_object()\n oid_object_info_extended()\n do_oid_object_info_extended()\n loose_object_info()\n parse_loose_header_extended()\n packed_object_info()\n cache_or_unpack_entry()\n unpack_entry()\n unpack_object_header()\n unpack_object_header_buffer()\n get_size_from_delta()\n get_delta_hdr_size()\n\nAll three leaves have code that needs to be adjusted to use `size_t`\ninstead of `unsigned long`, and all of the other functions in that call\ntree need to be adjusted for that. Some of the callers do not even pass an\n`unsigned long` pointer around, but instead a pointer to `struct\nobject_info` (which, you guessed it, also has an `unsigned long` that\nshould have been a `size_t` from the beginning).\n\nThis is too big a change I am willing to work on, let alone accept, this\nlate in the cycle.\n\nSure, it would fix the scenario where clean/smudge filters are not even\ninvolved (read: `git add`ing a 5GB file _directly_). But the potential for\nbugs to hide!\n\n> I guess I'll see soon enough why by reading later steps. I can see\n> that for the purpose of making write_entry() aware of the size_t,\n> this is necessary at the minimum.\n\nI fear that you won't see more about this in the later steps ;-)\n\nCiao,\nDscho\n"},{"id":"440321","messageId":"nycvar.QRO.7.76.6.2111021619020.56@tvgsbejvaqbjf.bet","threadId":"56792","inReplyTo":"xmqqee84pyfb.fsf@gitster.g","subject":"Re: [PATCH 1/3] test-lib: add prerequisite for 64-bit platforms","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-11-02T15:20:09Z","receivedAt":"2021-11-02T15:20:20Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 28 Oct 2021, Junio C Hamano wrote:\n\n> Carlo Arenas writes:\n>\n> >> Since this is clearly copied from `LONG_IS_64BIT`, why the change from\n> >> `-le` to `-eq`? It is at least inconsistent to use anything different\n> >> here.\n> >\n> > My assumption is that the check for sizeof(size_t) we have is really\n> > about finding the bit width of the platform, and we currently support\n> > 2 of them (32-bit and 64-bit), which is why the name I chose was\n> > \"IS_64BIT\" and also why I was strict on it being exactly 8 bytes\n> > (considering all platforms git supports have bytes with 8 bits).\n> >\n> > It can go eitherway IMHO, and your point about being inconsistent\n> > (with my lack of explanation in the commit) suggests we should instead\n> > use your proposal, do you want me to resend or could adjust them in\n> > your tree?\n>\n> Is LONG_IS_64BIT used to ensure that long is _at least_ 64 bit? If\n> so, perhaps its name may need to be rethought. On the other hand,\n> if it is meant to ensure that long is exactly 64 bit, then it should\n> use -eq to compare.\n\n`LONG_IS_64BIT` is used by the `tar` tests to ensure that `long` can\nrepresent file sizes larger than 4GB. So yeah, it is an `_AT_LEAST_`\ninstead of an `_IS_`.\n\nNot -rc material, though.\n\nCiao,\nDscho\n"},{"id":"440324","messageId":"pull.1068.v4.git.1635867971.gitgitgadget@gmail.com","threadId":"56792","inReplyTo":"pull.1068.v3.git.1635515959.gitgitgadget@gmail.com","subject":"[PATCH v4 0/8] Allow clean/smudge filters to handle huge files in the LLP64 data model","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-11-02T15:46:03Z","receivedAt":"2021-11-02T15:46:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This patch series came in via the Git for Windows fork\n[https://github.com/git-for-windows/git/pull/3487], and I intend to merge it\nbefore v2.34.0-rc0, therefore I appreciate every careful review you gentle\npeople can spare.\n\nThe x86_64 variant of Windows uses the LLP64 data model, where the long data\ntype is 32-bit. This is very different from the LP64 data model used e.g. by\nx86_64 Linux, where unsigned long is 64-bit.\n\nMost notably, this means that sizeof(unsigned long) != sizeof(size_t) in\ngeneral.\n\nHowever, since Git was born in the Linux ecosystem, where that inequality\ndoes not hold true, it is understandable that unsigned long is used in many\ncode locations where size_t should have been used. As a consequence, quite a\nfew things are broken e.g. on Windows, when it comes to 4GB file contents or\nlarger.\n\nUsing Git LFS [https://git-lfs.github.io/] trying to work around such issues\nis one such a broken scenario. You cannot git checkout, say, 5GB files. Huge\nfiles will be truncated to whatever the file size is modulo 4GB (in the case\nof a 5GB file, it would be truncated to 1GB).\n\nThis patch series primarily fixes the Git LFS scenario, by allowing clean\nfilters to accept 5GB files, and by allowing smudge filters to produce 5GB\nfiles.\n\nThe much larger project to teach Git to use size_t instead of unsigned long\nin all the appropriate places is hardly scratched by this patch series.\n\nSide note: The fix for the clean filter included in this series does not\nactually affect Git LFS! The reason is that Git LFS marks its filter as\nrequired, and therefore Git streams the file contents to Git LFS via a file\ndescriptor (which is unaffected by LLP64). A \"clean\" filter that is not\nmarked as required, however, lets Git take the code path that is fixed by\nthis patch series.\n\nChanges since v3:\n\n * The commit message of the fourth patch no longer talks about a \"workdir\"\n but about the \"working tree\".\n * The smudge filter test case is now more precise when verifying the\n result's file size.\n\nChanges since v2:\n\n * The test cases now use a prereq to avoid running in 32-bit setups (where\n they would be guaranteed to fail).\n * The SIZE_T_IS64BIT prereq specifically does not follow LONG_IS_64BIT, by\n testing for exactly 64 bits.\n * The code comment above unsigned_left_shift_overflows() was fixed.\n * unsigned_left_shift_overflows() now verifies that shift is smaller than\n the bit size of the operand.\n * genzeros now marks its buffer of NULs as const.\n * The error check in the infinite loop genzeros was fixed.\n\nChanges since v1:\n\n * Removed extraneous \"Signed-off-by:\" lines from \"git-compat-util:\n introduce more size_t helpers\".\n * Integrated Carlo's patch to allow genzeros to generate large amounts of\n NULs, even in LLP64 data models.\n * Using test-tool genzeros instead of dd if=/dev/zero, to help HP NonStop\n (which appears to use the LP64 data model and therefore should pass the\n new test cases even without the fixes provided in this patch series).\n * Accelerating genzeros to have performance characteristics similar to dd\n if=/dev/zero instead of being ~50x slower.\n\nCarlo Marcelo Arenas Belón (2):\n test-genzeros: allow more than 2G zeros in Windows\n test-lib: add prerequisite for 64-bit platforms\n\nJohannes Schindelin (2):\n test-tool genzeros: generate large amounts of data more efficiently\n git-compat-util: introduce more size_t helpers\n\nMatt Cooper (4):\n t1051: introduce a smudge filter test for extremely large files\n odb: teach read_blob_entry to use size_t\n odb: guard against data loss checking out a huge file\n clean/smudge: allow clean filters to process extremely large files\n\n convert.c | 2 +-\n delta.h | 6 +++---\n entry.c | 8 +++++---\n entry.h | 2 +-\n git-compat-util.h | 25 +++++++++++++++++++++++++\n object-file.c | 6 +++---\n packfile.c | 6 +++---\n parallel-checkout.c | 2 +-\n t/helper/test-genzeros.c | 21 +++++++++++++++++----\n t/t1051-large-conversion.sh | 26 ++++++++++++++++++++++++++\n t/test-lib.sh | 4 ++++\n 11 files changed, 89 insertions(+), 19 deletions(-)\n\n\nbase-commit: ebf3c04b262aa27fbb97f8a0156c2347fecafafb\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1068%2Fdscho%2Fhuge-file-smudge-clean-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1068/dscho/huge-file-smudge-clean-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/1068\n\nRange-diff vs v3:\n\n 1: 068f897b973 = 1: 068f897b973 test-genzeros: allow more than 2G zeros in Windows\n 2: 05219720014 = 2: 05219720014 test-tool genzeros: generate large amounts of data more efficiently\n 3: 489500bb1dc = 3: 489500bb1dc test-lib: add prerequisite for 64-bit platforms\n 4: ce9dfaac9f8 ! 4: f099b4eaead t1051: introduce a smudge filter test for extremely large files\n @@ Commit message\n t1051: introduce a smudge filter test for extremely large files\n \n The filter system allows for alterations to file contents when they're\n - added to the database or workdir. (\"Smudge\" when moving to the workdir;\n - \"clean\" when moving to the database.) This is used natively to handle CRLF\n - to LF conversions. It's also employed by Git-LFS to replace large files\n - from the workdir with small tracking files in the repo and vice versa.\n + added to the database or working tree. (\"Smudge\" when moving to the\n + working tree; \"clean\" when moving to the database.) This is used\n + natively to handle CRLF to LF conversions. It's also employed by Git-LFS\n + to replace large files from the working tree with small tracking files\n + in the repo and vice versa.\n \n - Git pulls the entire smudged file into memory. While this is inefficient,\n - there's a more insidious problem on some platforms due to inconsistency\n - between using unsigned long and size_t for the same type of data (size of\n - a file in bytes). On most 64-bit platforms, unsigned long is 64 bits, and\n - size_t is typedef'd to unsigned long. On Windows, however, unsigned long is\n - only 32 bits (and therefore on 64-bit Windows, size_t is typedef'd to\n + Git reads the entire smudged file into memory to convert it into a\n + \"clean\" form to be used in-core. While this is inefficient, there's a\n + more insidious problem on some platforms due to inconsistency between\n + using unsigned long and size_t for the same type of data (size of a file\n + in bytes). On most 64-bit platforms, unsigned long is 64 bits, and\n + size_t is typedef'd to unsigned long. On Windows, however, unsigned long\n + is only 32 bits (and therefore on 64-bit Windows, size_t is typedef'd to\n unsigned long long in order to be 64 bits).\n \n Practically speaking, this means 64-bit Windows users of Git-LFS can't\n @@ t/t1051-large-conversion.sh: test_expect_success 'ident converts on output' '\n +test_expect_failure EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \\\n +\t\t'files over 4GB convert on output' '\n +\ttest_commit test small \"a small file\" &&\n ++\tsmall_size=$(test_file_size small) &&\n +\ttest_config filter.makelarge.smudge \\\n +\t\t\"test-tool genzeros $((5*1024*1024*1024)) && cat\" &&\n +\techo \"small filter=makelarge\" >.gitattributes &&\n +\trm small &&\n +\tgit checkout -- small &&\n +\tsize=$(test_file_size small) &&\n -+\ttest \"$size\" -ge $((5 * 1024 * 1024 * 1024))\n ++\ttest \"$size\" -eq $((5 * 1024 * 1024 * 1024 + $small_size))\n +'\n +\n test_done\n 5: dbef8168bc7 ! 5: 308a8f2a3ad odb: teach read_blob_entry to use size_t\n @@ t/t1051-large-conversion.sh: test_expect_success 'ident converts on output' '\n +test_expect_success EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \\\n \t\t'files over 4GB convert on output' '\n \ttest_commit test small \"a small file\" &&\n - \ttest_config filter.makelarge.smudge \\\n + \tsmall_size=$(test_file_size small) &&\n 6: 18419070c29 = 6: 65bc291b680 git-compat-util: introduce more size_t helpers\n 7: f59c523bcc4 = 7: 7b6655f03f5 odb: guard against data loss checking out a huge file\n 8: acc5591517f ! 8: 41fda423982 clean/smudge: allow clean filters to process extremely large files\n @@ convert.c: static int crlf_to_worktree(const char *src, size_t len, struct strbu\n \n ## t/t1051-large-conversion.sh ##\n @@ t/t1051-large-conversion.sh: test_expect_success EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \\\n - \ttest \"$size\" -ge $((5 * 1024 * 1024 * 1024))\n + \ttest \"$size\" -eq $((5 * 1024 * 1024 * 1024 + $small_size))\n '\n \n +# This clean filter writes down the size of input it receives. By checking against\n\n-- \ngitgitgadget\n"},{"id":"440325","messageId":"068f897b973b1f8889145f97c42fe6233c272dd5.1635867971.git.gitgitgadget@gmail.com","threadId":"56792","inReplyTo":"pull.1068.v4.git.1635867971.gitgitgadget@gmail.com","subject":"[PATCH v4 1/8] test-genzeros: allow more than 2G zeros in Windows","fromName":"Carlo Marcelo Arenas Belón via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-11-02T15:46:04Z","receivedAt":"2021-11-02T15:46:17Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= \n\nd5cfd142ec (tests: teach the test-tool to generate NUL bytes and\nuse it, 2019-02-14), add a way to generate zeroes in a portable\nway without using /dev/zero (needed by HP NonStop), but uses a\nlong variable that is limited to 2^31 in Windows.\n\nUse instead a (POSIX/C99) intmax_t that is at least 64bit wide\nin 64-bit Windows to use in a future test.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \nSigned-off-by: Johannes Schindelin \n---\n t/helper/test-genzeros.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/helper/test-genzeros.c b/t/helper/test-genzeros.c\nindex 9532f5bac97..b1197e91a89 100644\n--- a/t/helper/test-genzeros.c\n+++ b/t/helper/test-genzeros.c\n@@ -3,14 +3,14 @@\n \n int cmd__genzeros(int argc, const char **argv)\n {\n-\tlong count;\n+\tintmax_t count;\n \n \tif (argc > 2) {\n \t\tfprintf(stderr, \"usage: %s []\\n\", argv[0]);\n \t\treturn 1;\n \t}\n \n-\tcount = argc > 1 ? strtol(argv[1], NULL, 0) : -1L;\n+\tcount = argc > 1 ? strtoimax(argv[1], NULL, 0) : -1;\n \n \twhile (count < 0 || count--) {\n \t\tif (putchar(0) == EOF)\n-- \ngitgitgadget\n\n"},{"id":"440326","messageId":"052197200141c321118b7766f5615a61f951e59f.1635867971.git.gitgitgadget@gmail.com","threadId":"56792","inReplyTo":"pull.1068.v4.git.1635867971.gitgitgadget@gmail.com","subject":"[PATCH v4 2/8] test-tool genzeros: generate large amounts of data more efficiently","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-11-02T15:46:05Z","receivedAt":"2021-11-02T15:46:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn this developer's tests, producing one gigabyte worth of NULs in a\nbusy loop that writes out individual bytes, unbuffered, took ~27sec.\nWriting chunked 256kB buffers instead only took ~0.6sec\n\nThis matters because we are about to introduce a pair of test cases that\nwant to be able to produce 5GB of NULs, and we cannot use `/dev/zero`\nbecause of the HP NonStop platform's lack of support for that device.\n\nSigned-off-by: Johannes Schindelin \n---\n t/helper/test-genzeros.c | 17 +++++++++++++++--\n 1 file changed, 15 insertions(+), 2 deletions(-)\n\ndiff --git a/t/helper/test-genzeros.c b/t/helper/test-genzeros.c\nindex b1197e91a89..8ca988d6216 100644\n--- a/t/helper/test-genzeros.c\n+++ b/t/helper/test-genzeros.c\n@@ -3,7 +3,10 @@\n \n int cmd__genzeros(int argc, const char **argv)\n {\n+\t/* static, so that it is NUL-initialized */\n+\tstatic const char zeros[256 * 1024];\n \tintmax_t count;\n+\tssize_t n;\n \n \tif (argc > 2) {\n \t\tfprintf(stderr, \"usage: %s []\\n\", argv[0]);\n@@ -12,9 +15,19 @@ int cmd__genzeros(int argc, const char **argv)\n \n \tcount = argc > 1 ? strtoimax(argv[1], NULL, 0) : -1;\n \n-\twhile (count < 0 || count--) {\n-\t\tif (putchar(0) == EOF)\n+\t/* Writing out individual NUL bytes is slow... */\n+\twhile (count < 0)\n+\t\tif (write(1, zeros, ARRAY_SIZE(zeros)) < 0)\n \t\t\treturn -1;\n+\n+\twhile (count > 0) {\n+\t\tn = write(1, zeros, count < ARRAY_SIZE(zeros) ?\n+\t\t\t count : ARRAY_SIZE(zeros));\n+\n+\t\tif (n < 0)\n+\t\t\treturn -1;\n+\n+\t\tcount -= n;\n \t}\n \n \treturn 0;\n-- \ngitgitgadget\n\n"},{"id":"440327","messageId":"489500bb1dcaffecab42672658990cfc26d52d7c.1635867971.git.gitgitgadget@gmail.com","threadId":"56792","inReplyTo":"pull.1068.v4.git.1635867971.gitgitgadget@gmail.com","subject":"[PATCH v4 3/8] test-lib: add prerequisite for 64-bit platforms","fromName":"Carlo Marcelo Arenas Belón via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-11-02T15:46:06Z","receivedAt":"2021-11-02T15:46:20Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= \n\nAllow tests that assume a 64-bit `size_t` to be skipped in 32-bit\nplatforms and regardless of the size of `long`.\n\nThis imitates the `LONG_IS_64BIT` prerequisite.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \nSigned-off-by: Johannes Schindelin \n---\n t/test-lib.sh | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex adaf03543e8..af1a94c2c20 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1642,6 +1642,10 @@ build_option () {\n \tsed -ne \"s/^$1: //p\"\n }\n \n+test_lazy_prereq SIZE_T_IS_64BIT '\n+\ttest 8 -eq \"$(build_option sizeof-size_t)\"\n+'\n+\n test_lazy_prereq LONG_IS_64BIT '\n \ttest 8 -le \"$(build_option sizeof-long)\"\n '\n-- \ngitgitgadget\n\n"},{"id":"440328","messageId":"f099b4eaead1799908eee000c99af3a381394e6d.1635867971.git.gitgitgadget@gmail.com","threadId":"56792","inReplyTo":"pull.1068.v4.git.1635867971.gitgitgadget@gmail.com","subject":"[PATCH v4 4/8] t1051: introduce a smudge filter test for extremely large files","fromName":"Matt Cooper via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-11-02T15:46:07Z","receivedAt":"2021-11-02T15:46:22Z","isPatch":true,"sender":{"key":"name:Matt Cooper","avatar":null},"body":"From: Matt Cooper \n\nThe filter system allows for alterations to file contents when they're\nadded to the database or working tree. (\"Smudge\" when moving to the\nworking tree; \"clean\" when moving to the database.) This is used\nnatively to handle CRLF to LF conversions. It's also employed by Git-LFS\nto replace large files from the working tree with small tracking files\nin the repo and vice versa.\n\nGit reads the entire smudged file into memory to convert it into a\n\"clean\" form to be used in-core. While this is inefficient, there's a\nmore insidious problem on some platforms due to inconsistency between\nusing unsigned long and size_t for the same type of data (size of a file\nin bytes). On most 64-bit platforms, unsigned long is 64 bits, and\nsize_t is typedef'd to unsigned long. On Windows, however, unsigned long\nis only 32 bits (and therefore on 64-bit Windows, size_t is typedef'd to\nunsigned long long in order to be 64 bits).\n\nPractically speaking, this means 64-bit Windows users of Git-LFS can't\nhandle files larger than 2^32 bytes. Other 64-bit platforms don't suffer\nthis limitation.\n\nThis commit introduces a test exposing the issue; future commits make it\npass. The test simulates the way Git-LFS works by having a tiny file\nchecked into the repository and expanding it to a huge file on checkout.\n\nHelped-by: Johannes Schindelin \nSigned-off-by: Matt Cooper \nSigned-off-by: Johannes Schindelin \n---\n t/t1051-large-conversion.sh | 15 +++++++++++++++\n 1 file changed, 15 insertions(+)\n\ndiff --git a/t/t1051-large-conversion.sh b/t/t1051-large-conversion.sh\nindex 8b7640b3ba8..e7f9f0bdc56 100755\n--- a/t/t1051-large-conversion.sh\n+++ b/t/t1051-large-conversion.sh\n@@ -83,4 +83,19 @@ test_expect_success 'ident converts on output' '\n \ttest_cmp small.clean large.clean\n '\n \n+# This smudge filter prepends 5GB of zeros to the file it checks out. This\n+# ensures that smudging doesn't mangle large files on 64-bit Windows.\n+test_expect_failure EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \\\n+\t\t'files over 4GB convert on output' '\n+\ttest_commit test small \"a small file\" &&\n+\tsmall_size=$(test_file_size small) &&\n+\ttest_config filter.makelarge.smudge \\\n+\t\t\"test-tool genzeros $((5*1024*1024*1024)) && cat\" &&\n+\techo \"small filter=makelarge\" >.gitattributes &&\n+\trm small &&\n+\tgit checkout -- small &&\n+\tsize=$(test_file_size small) &&\n+\ttest \"$size\" -eq $((5 * 1024 * 1024 * 1024 + $small_size))\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"440329","messageId":"308a8f2a3ade63ef21feb945e45866f2a83ae101.1635867971.git.gitgitgadget@gmail.com","threadId":"56792","inReplyTo":"pull.1068.v4.git.1635867971.gitgitgadget@gmail.com","subject":"[PATCH v4 5/8] odb: teach read_blob_entry to use size_t","fromName":"Matt Cooper via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-11-02T15:46:08Z","receivedAt":"2021-11-02T15:46:23Z","isPatch":true,"sender":{"key":"name:Matt Cooper","avatar":null},"body":"From: Matt Cooper \n\nThere is mixed use of size_t and unsigned long to deal with sizes in the\ncodebase. Recall that Windows defines unsigned long as 32 bits even on\n64-bit platforms, meaning that converting size_t to unsigned long narrows\nthe range. This mostly doesn't cause a problem since Git rarely deals\nwith files larger than 2^32 bytes.\n\nBut adjunct systems such as Git LFS, which use smudge/clean filters to\nkeep huge files out of the repository, may have huge file contents passed\nthrough some of the functions in entry.c and convert.c. On Windows, this\nresults in a truncated file being written to the workdir. I traced this to\none specific use of unsigned long in write_entry (and a similar instance\nin write_pc_item_to_fd for parallel checkout). That appeared to be for\nthe call to read_blob_entry, which expects a pointer to unsigned long.\n\nBy altering the signature of read_blob_entry to expect a size_t,\nwrite_entry can be switched to use size_t internally (which all of its\ncallers and most of its callees already used). To avoid touching dozens of\nadditional files, read_blob_entry uses a local unsigned long to call a\nchain of functions which aren't prepared to accept size_t.\n\nHelped-by: Johannes Schindelin \nSigned-off-by: Matt Cooper \nSigned-off-by: Johannes Schindelin \n---\n entry.c | 8 +++++---\n entry.h | 2 +-\n parallel-checkout.c | 2 +-\n t/t1051-large-conversion.sh | 2 +-\n 4 files changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex 711ee0693c7..4cb3942dbdc 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -82,11 +82,13 @@ static int create_file(const char *path, unsigned int mode)\n \treturn open(path, O_WRONLY | O_CREAT | O_EXCL, mode);\n }\n \n-void *read_blob_entry(const struct cache_entry *ce, unsigned long *size)\n+void *read_blob_entry(const struct cache_entry *ce, size_t *size)\n {\n \tenum object_type type;\n-\tvoid *blob_data = read_object_file(&ce->oid, &type, size);\n+\tunsigned long ul;\n+\tvoid *blob_data = read_object_file(&ce->oid, &type, &ul);\n \n+\t*size = ul;\n \tif (blob_data) {\n \t\tif (type == OBJ_BLOB)\n \t\t\treturn blob_data;\n@@ -270,7 +272,7 @@ static int write_entry(struct cache_entry *ce, char *path, struct conv_attrs *ca\n \tint fd, ret, fstat_done = 0;\n \tchar *new_blob;\n \tstruct strbuf buf = STRBUF_INIT;\n-\tunsigned long size;\n+\tsize_t size;\n \tssize_t wrote;\n \tsize_t newsize = 0;\n \tstruct stat st;\ndiff --git a/entry.h b/entry.h\nindex b8c0e170dc7..61ee8c17604 100644\n--- a/entry.h\n+++ b/entry.h\n@@ -51,7 +51,7 @@ int finish_delayed_checkout(struct checkout *state, int *nr_checkouts);\n */\n void unlink_entry(const struct cache_entry *ce);\n \n-void *read_blob_entry(const struct cache_entry *ce, unsigned long *size);\n+void *read_blob_entry(const struct cache_entry *ce, size_t *size);\n int fstat_checkout_output(int fd, const struct checkout *state, struct stat *st);\n void update_ce_after_write(const struct checkout *state, struct cache_entry *ce,\n \t\t\t struct stat *st);\ndiff --git a/parallel-checkout.c b/parallel-checkout.c\nindex 6b1af32bb3d..b6f4a25642e 100644\n--- a/parallel-checkout.c\n+++ b/parallel-checkout.c\n@@ -261,7 +261,7 @@ static int write_pc_item_to_fd(struct parallel_checkout_item *pc_item, int fd,\n \tstruct stream_filter *filter;\n \tstruct strbuf buf = STRBUF_INIT;\n \tchar *blob;\n-\tunsigned long size;\n+\tsize_t size;\n \tssize_t wrote;\n \n \t/* Sanity check */\ndiff --git a/t/t1051-large-conversion.sh b/t/t1051-large-conversion.sh\nindex e7f9f0bdc56..e6d52f98b15 100755\n--- a/t/t1051-large-conversion.sh\n+++ b/t/t1051-large-conversion.sh\n@@ -85,7 +85,7 @@ test_expect_success 'ident converts on output' '\n \n # This smudge filter prepends 5GB of zeros to the file it checks out. This\n # ensures that smudging doesn't mangle large files on 64-bit Windows.\n-test_expect_failure EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \\\n+test_expect_success EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \\\n \t\t'files over 4GB convert on output' '\n \ttest_commit test small \"a small file\" &&\n \tsmall_size=$(test_file_size small) &&\n-- \ngitgitgadget\n\n"},{"id":"440330","messageId":"65bc291b680a30a63cf62a1dc5411ab47e67ae09.1635867971.git.gitgitgadget@gmail.com","threadId":"56792","inReplyTo":"pull.1068.v4.git.1635867971.gitgitgadget@gmail.com","subject":"[PATCH v4 6/8] git-compat-util: introduce more size_t helpers","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-11-02T15:46:09Z","receivedAt":"2021-11-02T15:46:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWe will use them in the next commit.\n\nSigned-off-by: Johannes Schindelin \n---\n git-compat-util.h | 25 +++++++++++++++++++++++++\n 1 file changed, 25 insertions(+)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex a508dbe5a35..1f41e5611a1 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -113,6 +113,14 @@\n #define unsigned_mult_overflows(a, b) \\\n ((a) && (b) > maximum_unsigned_value_of_type(a) / (a))\n \n+/*\n+ * Returns true if the left shift of \"a\" by \"shift\" bits will\n+ * overflow. The type of \"a\" must be unsigned.\n+ */\n+#define unsigned_left_shift_overflows(a, shift) \\\n+ ((shift) < bitsizeof(a) && \\\n+ (a) > maximum_unsigned_value_of_type(a) >> (shift))\n+\n #ifdef __GNUC__\n #define TYPEOF(x) (__typeof__(x))\n #else\n@@ -859,6 +867,23 @@ static inline size_t st_sub(size_t a, size_t b)\n \treturn a - b;\n }\n \n+static inline size_t st_left_shift(size_t a, unsigned shift)\n+{\n+\tif (unsigned_left_shift_overflows(a, shift))\n+\t\tdie(\"size_t overflow: %\"PRIuMAX\" << %u\",\n+\t\t (uintmax_t)a, shift);\n+\treturn a << shift;\n+}\n+\n+static inline unsigned long cast_size_t_to_ulong(size_t a)\n+{\n+\tif (a != (unsigned long)a)\n+\t\tdie(\"object too large to read on this platform: %\"\n+\t\t PRIuMAX\" is cut off to %lu\",\n+\t\t (uintmax_t)a, (unsigned long)a);\n+\treturn (unsigned long)a;\n+}\n+\n #ifdef HAVE_ALLOCA_H\n # include \n # define xalloca(size) (alloca(size))\n-- \ngitgitgadget\n\n"},{"id":"440331","messageId":"7b6655f03f50925747fa9870333bab39a690d3f8.1635867971.git.gitgitgadget@gmail.com","threadId":"56792","inReplyTo":"pull.1068.v4.git.1635867971.gitgitgadget@gmail.com","subject":"[PATCH v4 7/8] odb: guard against data loss checking out a huge file","fromName":"Matt Cooper via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-11-02T15:46:10Z","receivedAt":"2021-11-02T15:46:39Z","isPatch":true,"sender":{"key":"name:Matt Cooper","avatar":null},"body":"From: Matt Cooper \n\nThis introduces an additional guard for platforms where `unsigned long`\nand `size_t` are not of the same size. If the size of an object in the\ndatabase would overflow `unsigned long`, instead we now exit with an\nerror.\n\nA complete fix will have to update _many_ other functions throughout the\ncodebase to use `size_t` instead of `unsigned long`. It will have to be\nimplemented at some stage.\n\nThis commit puts in a stop-gap for the time being.\n\nHelped-by: Johannes Schindelin \nSigned-off-by: Matt Cooper \nSigned-off-by: Johannes Schindelin \n---\n delta.h | 6 +++---\n object-file.c | 6 +++---\n packfile.c | 6 +++---\n 3 files changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/delta.h b/delta.h\nindex 2df5fe13d95..8a56ec07992 100644\n--- a/delta.h\n+++ b/delta.h\n@@ -90,15 +90,15 @@ static inline unsigned long get_delta_hdr_size(const unsigned char **datap,\n \t\t\t\t\t const unsigned char *top)\n {\n \tconst unsigned char *data = *datap;\n-\tunsigned long cmd, size = 0;\n+\tsize_t cmd, size = 0;\n \tint i = 0;\n \tdo {\n \t\tcmd = *data++;\n-\t\tsize |= (cmd & 0x7f) << i;\n+\t\tsize |= st_left_shift(cmd & 0x7f, i);\n \t\ti += 7;\n \t} while (cmd & 0x80 && data < top);\n \t*datap = data;\n-\treturn size;\n+\treturn cast_size_t_to_ulong(size);\n }\n \n #endif\ndiff --git a/object-file.c b/object-file.c\nindex f233b440b22..70e456fc2a3 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1344,7 +1344,7 @@ static int parse_loose_header_extended(const char *hdr, struct object_info *oi,\n \t\t\t\t unsigned int flags)\n {\n \tconst char *type_buf = hdr;\n-\tunsigned long size;\n+\tsize_t size;\n \tint type, type_len = 0;\n \n \t/*\n@@ -1388,12 +1388,12 @@ static int parse_loose_header_extended(const char *hdr, struct object_info *oi,\n \t\t\tif (c > 9)\n \t\t\t\tbreak;\n \t\t\thdr++;\n-\t\t\tsize = size * 10 + c;\n+\t\t\tsize = st_add(st_mult(size, 10), c);\n \t\t}\n \t}\n \n \tif (oi->sizep)\n-\t\t*oi->sizep = size;\n+\t\t*oi->sizep = cast_size_t_to_ulong(size);\n \n \t/*\n \t * The length must be followed by a zero byte\ndiff --git a/packfile.c b/packfile.c\nindex 755aa7aec5e..3ccea004396 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1059,7 +1059,7 @@ unsigned long unpack_object_header_buffer(const unsigned char *buf,\n \t\tunsigned long len, enum object_type *type, unsigned long *sizep)\n {\n \tunsigned shift;\n-\tunsigned long size, c;\n+\tsize_t size, c;\n \tunsigned long used = 0;\n \n \tc = buf[used++];\n@@ -1073,10 +1073,10 @@ unsigned long unpack_object_header_buffer(const unsigned char *buf,\n \t\t\tbreak;\n \t\t}\n \t\tc = buf[used++];\n-\t\tsize += (c & 0x7f) << shift;\n+\t\tsize = st_add(size, st_left_shift(c & 0x7f, shift));\n \t\tshift += 7;\n \t}\n-\t*sizep = size;\n+\t*sizep = cast_size_t_to_ulong(size);\n \treturn used;\n }\n \n-- \ngitgitgadget\n\n"},{"id":"440332","messageId":"41fda423982d99847d3879f5ea1eb3570ae9eab6.1635867971.git.gitgitgadget@gmail.com","threadId":"56792","inReplyTo":"pull.1068.v4.git.1635867971.gitgitgadget@gmail.com","subject":"[PATCH v4 8/8] clean/smudge: allow clean filters to process extremely large files","fromName":"Matt Cooper via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-11-02T15:46:11Z","receivedAt":"2021-11-02T15:46:41Z","isPatch":true,"sender":{"key":"name:Matt Cooper","avatar":null},"body":"From: Matt Cooper \n\nThe filter system allows for alterations to file contents when they're\nmoved between the database and the worktree. We already made sure that\nit is possible for smudge filters to produce contents that are larger\nthan `unsigned long` can represent (which matters on systems where\n`unsigned long` is narrower than `size_t`, most notably 64-bit Windows).\nNow we make sure that clean filters can _consume_ contents that are\nlarger than that.\n\nNote that this commit only allows clean filters' _input_ to be larger\nthan can be represented by `unsigned long`.\n\nThis change makes only a very minute dent into the much larger project\nto teach Git to use `size_t` instead of `unsigned long` wherever\nappropriate.\n\nHelped-by: Johannes Schindelin \nSigned-off-by: Matt Cooper \nSigned-off-by: Johannes Schindelin \n---\n convert.c | 2 +-\n t/t1051-large-conversion.sh | 11 +++++++++++\n 2 files changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/convert.c b/convert.c\nindex fd9c84b0257..5ad6dfc08a0 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -613,7 +613,7 @@ static int crlf_to_worktree(const char *src, size_t len, struct strbuf *buf,\n \n struct filter_params {\n \tconst char *src;\n-\tunsigned long size;\n+\tsize_t size;\n \tint fd;\n \tconst char *cmd;\n \tconst char *path;\ndiff --git a/t/t1051-large-conversion.sh b/t/t1051-large-conversion.sh\nindex e6d52f98b15..042b0e44292 100755\n--- a/t/t1051-large-conversion.sh\n+++ b/t/t1051-large-conversion.sh\n@@ -98,4 +98,15 @@ test_expect_success EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \\\n \ttest \"$size\" -eq $((5 * 1024 * 1024 * 1024 + $small_size))\n '\n \n+# This clean filter writes down the size of input it receives. By checking against\n+# the actual size, we ensure that cleaning doesn't mangle large files on 64-bit Windows.\n+test_expect_success EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \\\n+\t\t'files over 4GB convert on input' '\n+\ttest-tool genzeros $((5*1024*1024*1024)) >big &&\n+\ttest_config filter.checklarge.clean \"wc -c >big.size\" &&\n+\techo \"big filter=checklarge\" >.gitattributes &&\n+\tgit add big &&\n+\ttest $(test_file_size big) -eq $(cat big.size)\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"440349","messageId":"20211102204040.nnndsqrzu7ozpva7@tb-raspi4","threadId":"56792","inReplyTo":"308a8f2a3ade63ef21feb945e45866f2a83ae101.1635867971.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 5/8] odb: teach read_blob_entry to use size_t","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2021-11-02T20:40:41Z","receivedAt":"2021-11-02T20:46:04Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"Nicely explained, some comments inline\n\nOn Tue, Nov 02, 2021 at 03:46:08PM +0000, Matt Cooper via GitGitGadget wrote:\n> From: Matt Cooper \n>\n> There is mixed use of size_t and unsigned long to deal with sizes in the\n> codebase. Recall that Windows defines unsigned long as 32 bits even on\n> 64-bit platforms, meaning that converting size_t to unsigned long narrows\n> the range. This mostly doesn't cause a problem since Git rarely deals\n> with files larger than 2^32 bytes.\n\nWhat does this mean ?\n> ... This mostly doesn't cause a problem since Git rarely deals\n> with files larger than 2^32 bytes.\n\nIs \"mostly\" is a good wording here ?\nMay be\nThis doesn't cause a problem when files smaller than 2^32 bytes are handled by Git.\n\n>\n> But adjunct systems such as Git LFS, which use smudge/clean filters to\n> keep huge files out of the repository, may have huge file contents passed\n> through some of the functions in entry.c and convert.c. On Windows, this\n> results in a truncated file being written to the workdir. I traced this to\n> one specific use of unsigned long in write_entry (and a similar instance\n> in write_pc_item_to_fd for parallel checkout). That appeared to be for\n> the call to read_blob_entry, which expects a pointer to unsigned long.\n>\n> By altering the signature of read_blob_entry to expect a size_t,\n\n\"expect\" -> \"use\"\n\n(I am not a native English speaker, would \"changing\" be better than \"altering\" ?)\n By changing the signature of read_blob_entry to use size_t,\n\n\n\n> write_entry can be switched to use size_t internally (which all of its\n> callers and most of its callees already used). To avoid touching dozens of\n> additional files, read_blob_entry uses a local unsigned long to call a\n> chain of functions which aren't prepared to accept size_t.\n>\n> Helped-by: Johannes Schindelin \n> Signed-off-by: Matt Cooper \n> Signed-off-by: Johannes Schindelin \n> ---\n> entry.c | 8 +++++---\n> entry.h | 2 +-\n> parallel-checkout.c | 2 +-\n> t/t1051-large-conversion.sh | 2 +-\n> 4 files changed, 8 insertions(+), 6 deletions(-)\n>\n> diff --git a/entry.c b/entry.c\n> index 711ee0693c7..4cb3942dbdc 100644\n> --- a/entry.c\n> +++ b/entry.c\n> @@ -82,11 +82,13 @@ static int create_file(const char *path, unsigned int mode)\n> \treturn open(path, O_WRONLY | O_CREAT | O_EXCL, mode);\n> }\n>\n> -void *read_blob_entry(const struct cache_entry *ce, unsigned long *size)\n> +void *read_blob_entry(const struct cache_entry *ce, size_t *size)\n> {\n> \tenum object_type type;\n> -\tvoid *blob_data = read_object_file(&ce->oid, &type, size);\n> +\tunsigned long ul;\n> +\tvoid *blob_data = read_object_file(&ce->oid, &type, &ul);\n>\n> +\t*size = ul;\n> \tif (blob_data) {\n> \t\tif (type == OBJ_BLOB)\n> \t\t\treturn blob_data;\n> @@ -270,7 +272,7 @@ static int write_entry(struct cache_entry *ce, char *path, struct conv_attrs *ca\n> \tint fd, ret, fstat_done = 0;\n> \tchar *new_blob;\n> \tstruct strbuf buf = STRBUF_INIT;\n> -\tunsigned long size;\n> +\tsize_t size;\n> \tssize_t wrote;\n> \tsize_t newsize = 0;\n> \tstruct stat st;\n> diff --git a/entry.h b/entry.h\n> index b8c0e170dc7..61ee8c17604 100644\n> --- a/entry.h\n> +++ b/entry.h\n> @@ -51,7 +51,7 @@ int finish_delayed_checkout(struct checkout *state, int *nr_checkouts);\n> */\n> void unlink_entry(const struct cache_entry *ce);\n>\n> -void *read_blob_entry(const struct cache_entry *ce, unsigned long *size);\n> +void *read_blob_entry(const struct cache_entry *ce, size_t *size);\n> int fstat_checkout_output(int fd, const struct checkout *state, struct stat *st);\n> void update_ce_after_write(const struct checkout *state, struct cache_entry *ce,\n> \t\t\t struct stat *st);\n> diff --git a/parallel-checkout.c b/parallel-checkout.c\n> index 6b1af32bb3d..b6f4a25642e 100644\n> --- a/parallel-checkout.c\n> +++ b/parallel-checkout.c\n> @@ -261,7 +261,7 @@ static int write_pc_item_to_fd(struct parallel_checkout_item *pc_item, int fd,\n> \tstruct stream_filter *filter;\n> \tstruct strbuf buf = STRBUF_INIT;\n> \tchar *blob;\n> -\tunsigned long size;\n> +\tsize_t size;\n> \tssize_t wrote;\n>\n> \t/* Sanity check */\n> diff --git a/t/t1051-large-conversion.sh b/t/t1051-large-conversion.sh\n> index e7f9f0bdc56..e6d52f98b15 100755\n> --- a/t/t1051-large-conversion.sh\n> +++ b/t/t1051-large-conversion.sh\n> @@ -85,7 +85,7 @@ test_expect_success 'ident converts on output' '\n>\n> # This smudge filter prepends 5GB of zeros to the file it checks out. This\n> # ensures that smudging doesn't mangle large files on 64-bit Windows.\n> -test_expect_failure EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \\\n> +test_expect_success EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \\\n> \t\t'files over 4GB convert on output' '\n> \ttest_commit test small \"a small file\" &&\n> \tsmall_size=$(test_file_size small) &&\n> --\n> gitgitgadget\n>\n"},{"id":"440350","messageId":"20211102204737.3wzphfcd6ne3sj2n@tb-raspi4","threadId":"56792","inReplyTo":"41fda423982d99847d3879f5ea1eb3570ae9eab6.1635867971.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 8/8] clean/smudge: allow clean filters to process extremely large files","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2021-11-02T20:47:37Z","receivedAt":"2021-11-02T20:47:50Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Tue, Nov 02, 2021 at 03:46:11PM +0000, Matt Cooper via GitGitGadget wrote:\n> From: Matt Cooper \n>\n> The filter system allows for alterations to file contents when they're\n\nSome nit-picking:\nlooking at\nhttps://git-scm.com/book/en/v2/Customizing-Git-Git-Attributes\nwe can read\n\"...substitutions in files on commit/checkout.\"\n\nShould we use this wording here as well ?\n\n\n> moved between the database and the worktree. We already made sure that\n> it is possible for smudge filters to produce contents that are larger\n> than `unsigned long` can represent (which matters on systems where\n> `unsigned long` is narrower than `size_t`, most notably 64-bit Windows).\n> Now we make sure that clean filters can _consume_ contents that are\n> larger than that.\n>\n> Note that this commit only allows clean filters' _input_ to be larger\n> than can be represented by `unsigned long`.\n>\n> This change makes only a very minute dent into the much larger project\n> to teach Git to use `size_t` instead of `unsigned long` wherever\n> appropriate.\n>\n> Helped-by: Johannes Schindelin \n> Signed-off-by: Matt Cooper \n> Signed-off-by: Johannes Schindelin \n> ---\n> convert.c | 2 +-\n> t/t1051-large-conversion.sh | 11 +++++++++++\n> 2 files changed, 12 insertions(+), 1 deletion(-)\n>\n> diff --git a/convert.c b/convert.c\n> index fd9c84b0257..5ad6dfc08a0 100644\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -613,7 +613,7 @@ static int crlf_to_worktree(const char *src, size_t len, struct strbuf *buf,\n>\n> struct filter_params {\n> \tconst char *src;\n> -\tunsigned long size;\n> +\tsize_t size;\n> \tint fd;\n> \tconst char *cmd;\n> \tconst char *path;\n> diff --git a/t/t1051-large-conversion.sh b/t/t1051-large-conversion.sh\n> index e6d52f98b15..042b0e44292 100755\n> --- a/t/t1051-large-conversion.sh\n> +++ b/t/t1051-large-conversion.sh\n> @@ -98,4 +98,15 @@ test_expect_success EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \\\n> \ttest \"$size\" -eq $((5 * 1024 * 1024 * 1024 + $small_size))\n> '\n>\n> +# This clean filter writes down the size of input it receives. By checking against\n> +# the actual size, we ensure that cleaning doesn't mangle large files on 64-bit Windows.\n> +test_expect_success EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \\\n> +\t\t'files over 4GB convert on input' '\n> +\ttest-tool genzeros $((5*1024*1024*1024)) >big &&\n> +\ttest_config filter.checklarge.clean \"wc -c >big.size\" &&\n> +\techo \"big filter=checklarge\" >.gitattributes &&\n> +\tgit add big &&\n> +\ttest $(test_file_size big) -eq $(cat big.size)\n> +'\n> +\n> test_done\n> --\n> gitgitgadget\n"},{"id":"440354","messageId":"20211102214615.sflg5xgztdzrb27l@tb-raspi4","threadId":"56792","inReplyTo":"pull.1068.v4.git.1635867971.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 0/8] Allow clean/smudge filters to handle huge files in the LLP64 data model","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2021-11-02T21:46:15Z","receivedAt":"2021-11-02T21:46:26Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Tue, Nov 02, 2021 at 03:46:03PM +0000, Johannes Schindelin via GitGitGadget wrote:\n\nI could not convince my raspi to apply patch 7/8:\n\n git am On Tue, Nov 02, 2021 at 03:46:03PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> \n> I could not convince my raspi to apply patch 7/8:\n> \n> git am Applying: odb: guard against data loss checking out a huge file\n> error: patch failed: object-file.c:1344\n> error: object-file.c: patch does not apply\n> Patch failed at 0001 odb: guard against data loss checking out a huge file\n> hint: Use 'git am --show-current-patch=diff' to see the failed patch\n> When you have resolved this problem, run \"git am --continue\".\n> If you prefer to skip this patch, run \"git am --skip\" instead.\n> To restore the original branch and stop patching, run \"git am --abort\".\n> \n> I am not sure, what went wrong. I can retry the next days - or is this\n> branch/series somewhere available ?\n\nThis was submitted via Gitgitgadget. Therefore, the cover letter\nthat you are responding to has these footers:\n\n> base-commit: ebf3c04b262aa27fbb97f8a0156c2347fecafafb\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1068%2Fdscho%2Fhuge-file-smudge-clean-v4\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1068/dscho/huge-file-smudge-clean-v4\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1068\n\n-- Hannes\n"},{"id":"440413","messageId":"nycvar.QRO.7.76.6.2111040102560.56@tvgsbejvaqbjf.bet","threadId":"56792","inReplyTo":"20211102204040.nnndsqrzu7ozpva7@tb-raspi4","subject":"Re: [PATCH v4 5/8] odb: teach read_blob_entry to use size_t","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-11-04T00:09:27Z","receivedAt":"2021-11-04T00:10:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Torsten,\n\nOn Tue, 2 Nov 2021, Torsten Bögershausen wrote:\n\n> On Tue, Nov 02, 2021 at 03:46:08PM +0000, Matt Cooper via GitGitGadget wrote:\n> > From: Matt Cooper \n> >\n> > There is mixed use of size_t and unsigned long to deal with sizes in the\n> > codebase. Recall that Windows defines unsigned long as 32 bits even on\n> > 64-bit platforms, meaning that converting size_t to unsigned long narrows\n> > the range. This mostly doesn't cause a problem since Git rarely deals\n> > with files larger than 2^32 bytes.\n>\n> What does this mean ?\n\nI found the explanation to be quite clear (otherwise I would have changed\nit before submitting, obviously).\n\nGit is rarely used with large files, as it was meant to track source code\nfiles, and it is pretty rare that a source code file is larger than 4GB.\nTherefore, the described issues are edge cases rather than common ones.\n\n> > ... This mostly doesn't cause a problem since Git rarely deals\n> > with files larger than 2^32 bytes.\n>\n> Is \"mostly\" is a good wording here ?\n\nI do think so.\n\n> May be\n\ns/May be/Maybe/ since you're nitpicking wording ;-)\n\n> This doesn't cause a problem when files smaller than 2^32 bytes are handled by Git.\n\nThat would lose the rather important fact that it is common to encounter\nonly tracked files that are much smaller than 4GB.\n\n> > But adjunct systems such as Git LFS, which use smudge/clean filters to\n> > keep huge files out of the repository, may have huge file contents passed\n> > through some of the functions in entry.c and convert.c. On Windows, this\n> > results in a truncated file being written to the workdir. I traced this to\n> > one specific use of unsigned long in write_entry (and a similar instance\n> > in write_pc_item_to_fd for parallel checkout). That appeared to be for\n> > the call to read_blob_entry, which expects a pointer to unsigned long.\n> >\n> > By altering the signature of read_blob_entry to expect a size_t,\n>\n> \"expect\" -> \"use\"\n\nI would say that in the context of talking about a signature, \"expect\" is\na better verb than \"use\".\n\nBut then, just like you I am not a native speaker, so I think we should\nmaybe stop telling a native speaker like Matt how to use his native\ntongue...\n\n> (I am not a native English speaker, would \"changing\" be better than \"altering\" ?)\n> By changing the signature of read_blob_entry to use size_t,\n\nAs I said, I am not a native English speaker, either. So I believe that\nMatt knows better than the two of us together how to phrase things in\nEnglish.\n\nSince you had nothing to say about the patch itself, may I assume that\nyou're fine with it?\n\nCiao,\nDscho\n"},{"id":"440414","messageId":"nycvar.QRO.7.76.6.2111040109440.56@tvgsbejvaqbjf.bet","threadId":"56792","inReplyTo":"20211102204737.3wzphfcd6ne3sj2n@tb-raspi4","subject":"Re: [PATCH v4 8/8] clean/smudge: allow clean filters to process extremely large files","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-11-04T00:11:44Z","receivedAt":"2021-11-04T00:11:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Torsten,\n\nOn Tue, 2 Nov 2021, Torsten Bögershausen wrote:\n\n> On Tue, Nov 02, 2021 at 03:46:11PM +0000, Matt Cooper via GitGitGadget wrote:\n> > From: Matt Cooper \n> >\n> > The filter system allows for alterations to file contents when they're\n>\n> Some nit-picking:\n> looking at\n> https://git-scm.com/book/en/v2/Customizing-Git-Git-Attributes\n> we can read\n> \"...substitutions in files on commit/checkout.\"\n>\n> Should we use this wording here as well ?\n\nAgain, I believe that Matt's command of the English language is pretty\ngood (but then, I have the advantage of knowing him and I very much enjoy\nlearning new English words while chatting with him). I would therefore\nchalk it up to artistic license when he uses the word \"alterations\".\n\nSince you did not comment on the patch, may I assume that you find it\nflawless?\n\nCiao,\nDscho\n"},{"id":"440432","messageId":"20211104083355.wybcczuwktlu2fmu@tb-raspi4","threadId":"56792","inReplyTo":"nycvar.QRO.7.76.6.2111040109440.56@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v4 8/8] clean/smudge: allow clean filters to process extremely large files","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2021-11-04T08:33:55Z","receivedAt":"2021-11-04T08:34:10Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Thu, Nov 04, 2021 at 01:11:44AM +0100, Johannes Schindelin wrote:\n> Hi Torsten,\n>\n> On Tue, 2 Nov 2021, Torsten Bögershausen wrote:\n>\n> > On Tue, Nov 02, 2021 at 03:46:11PM +0000, Matt Cooper via GitGitGadget wrote:\n> > > From: Matt Cooper \n> > >\n> > > The filter system allows for alterations to file contents when they're\n> >\n> > Some nit-picking:\n> > looking at\n> > https://git-scm.com/book/en/v2/Customizing-Git-Git-Attributes\n> > we can read\n> > \"...substitutions in files on commit/checkout.\"\n> >\n> > Should we use this wording here as well ?\n>\n> Again, I believe that Matt's command of the English language is pretty\n> good (but then, I have the advantage of knowing him and I very much enjoy\n> learning new English words while chatting with him). I would therefore\n> chalk it up to artistic license when he uses the word \"alterations\".\n\nThat was not really what my comment was about.\nWe have exising documentations about Git at other places, and my question\nwas if we can/should/could use the same terminolgy here in the commit message\nas well.\nThis could make it easier for readers, if the same words are used for the same\nthing.\n\n>\n> Since you did not comment on the patch, may I assume that you find it\n> flawless?\n\nI could not find any flaws.\n\n>\n> Ciao,\n> Dscho\n\n"},{"id":"440459","messageId":"36140d50-be1f-669f-999b-4ee2314200bf@iee.email","threadId":"56792","inReplyTo":"nycvar.QRO.7.76.6.2111040102560.56@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v4 5/8] odb: teach read_blob_entry to use size_t","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.email","sentAt":"2021-11-04T12:24:31Z","receivedAt":"2021-11-04T16:08:58Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"On 04/11/2021 00:09, Johannes Schindelin wrote:\n> I would say that in the context of talking about a signature, \"expect\" is\n> a better verb than \"use\".\n>\n> But then, just like you I am not a native speaker, so I think we should\n> maybe stop telling a native speaker like Matt how to use his native\n> tongue...\n>\n>> (I am not a native English speaker, would \"changing\" be better than \"altering\" ?)\n>> By changing the signature of read_blob_entry to use size_t,\n> As I said, I am not a native English speaker, either. So I believe that\n> Matt knows better than the two of us together how to phrase things in\n> English.\nBeing a native speaker can be a bit of a Catch 22, especially in\nEnglish, as we can use unusual idioms and find them normal, and often\nweren't taught 'grammar'.\n\nIt is important that the text is understandable to those for whom\nEnglish isn't their first language. At least we don't have to use\nsimplified English [e.g. 1,2]\n\nPhilip\n\n[1] https://en.wikipedia.org/wiki/Simplified_Technical_English\n[2]\nhttps://www.boeing.com/company/key-orgs/licensing/simplified-english-checker.page\n(not actually used;-)\n\n\n"},{"id":"440472","messageId":"xmqqlf233k80.fsf@gitster.g","threadId":"56792","inReplyTo":"41fda423982d99847d3879f5ea1eb3570ae9eab6.1635867971.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 8/8] clean/smudge: allow clean filters to process extremely large files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-04T17:26:55Z","receivedAt":"2021-11-04T17:26:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Matt Cooper via GitGitGadget\" writes:\n\n> struct filter_params {\n> \tconst char *src;\n> -\tunsigned long size;\n> +\tsize_t size;\n> \tint fd;\n> \tconst char *cmd;\n> \tconst char *path;\n\nOK, this member is used in only two places in the file. One is used\nas a parameter to write_in_full() that is prepared to take size_t.\nThe other is to assign to this member from a size_t parameter the\napply_single_file_filter() function got from the caller, and the\ncallchain leading down to the function are size_t aware.\n\nSo, this may only make \"a small dent\" as described in the proposed\nlog message, but it does move it in the right direction.\n\nThanks.\n"}]}