{"thread":{"id":"60352","subject":"[PATCH v2 1/2] t: add a test helper to truncate files","startedAt":"2023-10-12T16:10:00Z","lastAt":"2023-10-18T00:42:30Z","messageCount":16,"participants":["brian m. carlson","Eric Sunshine","Junio C Hamano","Jeff King","Jason Hatton"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"483150","messageId":"20231012160930.330618-2-sandals@crustytoothpaste.net","threadId":"60352","inReplyTo":"20231012160930.330618-1-sandals@crustytoothpaste.net","subject":"[PATCH v2 1/2] t: add a test helper to truncate files","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2023-10-12T16:09:29Z","receivedAt":"2023-10-12T16:10:00Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"From: \"brian m. carlson\" <bk2204@github.com>\n\nIn a future commit, we're going to work with some large files which will\nbe at least 4 GiB in size.  To take advantage of the sparseness\nfunctionality on most Unix systems and avoid running the system out of\ndisk, it would be convenient to use truncate(2) to simply create a\nsparse file of sufficient size.\n\nHowever, the GNU truncate(1) utility isn't portable, so let's write a\ntiny test helper that does the work for us.\n\nSigned-off-by: brian m. carlson <bk2204@github.com>\n---\n Makefile                 |  1 +\n t/helper/test-tool.c     |  1 +\n t/helper/test-tool.h     |  1 +\n t/helper/test-truncate.c | 27 +++++++++++++++++++++++++++\n 4 files changed, 30 insertions(+)\n create mode 100644 t/helper/test-truncate.c\n\ndiff --git a/Makefile b/Makefile\nindex 9c6a2f125f..03adcb5a48 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -852,6 +852,7 @@ TEST_BUILTINS_OBJS += test-submodule-nested-repo-config.o\n TEST_BUILTINS_OBJS += test-submodule.o\n TEST_BUILTINS_OBJS += test-subprocess.o\n TEST_BUILTINS_OBJS += test-trace2.o\n+TEST_BUILTINS_OBJS += test-truncate.o\n TEST_BUILTINS_OBJS += test-urlmatch-normalization.o\n TEST_BUILTINS_OBJS += test-userdiff.o\n TEST_BUILTINS_OBJS += test-wildmatch.o\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex 9010ac6de7..876cd2dc31 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -86,6 +86,7 @@ static struct test_cmd cmds[] = {\n \t{ \"submodule-nested-repo-config\", cmd__submodule_nested_repo_config },\n \t{ \"subprocess\", cmd__subprocess },\n \t{ \"trace2\", cmd__trace2 },\n+\t{ \"truncate\", cmd__truncate },\n \t{ \"userdiff\", cmd__userdiff },\n \t{ \"urlmatch-normalization\", cmd__urlmatch_normalization },\n \t{ \"xml-encode\", cmd__xml_encode },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex f134f96b97..70dd4eba11 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -79,6 +79,7 @@ int cmd__submodule_config(int argc, const char **argv);\n int cmd__submodule_nested_repo_config(int argc, const char **argv);\n int cmd__subprocess(int argc, const char **argv);\n int cmd__trace2(int argc, const char **argv);\n+int cmd__truncate(int argc, const char **argv);\n int cmd__userdiff(int argc, const char **argv);\n int cmd__urlmatch_normalization(int argc, const char **argv);\n int cmd__xml_encode(int argc, const char **argv);\ndiff --git a/t/helper/test-truncate.c b/t/helper/test-truncate.c\nnew file mode 100644\nindex 0000000000..bd3fde364b\n--- /dev/null\n+++ b/t/helper/test-truncate.c\n@@ -0,0 +1,27 @@\n+#include \"test-tool.h\"\n+#include \"git-compat-util.h\"\n+\n+/*\n+ * Truncate a file to the given size.\n+ */\n+int cmd__truncate(int argc, const char **argv)\n+{\n+\tchar *p = NULL;\n+\tuintmax_t sz = 0;\n+\tint fd = -1;\n+\n+\tif (argc != 3)\n+\t\tdie(\"expected filename and size\");\n+\n+\tsz = strtoumax(argv[2], &p, 0);\n+\tif (*p)\n+\t\tdie(\"invalid size\");\n+\n+\tfd = open(argv[1], O_WRONLY | O_CREAT, 0600);\n+\tif (fd < 0)\n+\t\tdie_errno(\"failed to open file %s\", argv[1]);\n+\n+\tif (ftruncate(fd, (off_t) sz) < 0)\n+\t\tdie_errno(\"failed to truncate file\");\n+\treturn 0;\n+}\n"},{"id":"483151","messageId":"20231012160930.330618-3-sandals@crustytoothpaste.net","threadId":"60352","inReplyTo":"20231012160930.330618-1-sandals@crustytoothpaste.net","subject":"[PATCH v2 2/2] Prevent git from rehashing 4GiB files","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2023-10-12T16:09:30Z","receivedAt":"2023-10-12T16:10:00Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"From: Jason Hatton <jhatton@globalfinishing.com>\n\nThe index stores file sizes using a uint32_t. This causes any file\nthat is a multiple of 2^32 to have a cached file size of zero.\nZero is a special value used by racily clean. This causes git to\nrehash every file that is a multiple of 2^32 every time git status\nor git commit is run.\n\nThis patch mitigates the problem by making all files that are a\nmultiple of 2^32 appear to have a size of 1<<31 instead of zero.\n\nThe value of 1<<31 is chosen to keep it as far away from zero\nas possible to help prevent things getting mixed up with unpatched\nversions of git.\n\nAn example would be to have a 2^32 sized file in the index of\npatched git. Patched git would save the file as 2^31 in the cache.\nAn unpatched git would very much see the file has changed in size\nand force it to rehash the file, which is safe. The file would\nhave to grow or shrink by exactly 2^31 and retain all of its\nctime, mtime, and other attributes for old git to not notice\nthe change.\n\nThis patch does not change the behavior of any file that is not\nan exact multiple of 2^32.\n\nSigned-off-by: Jason D. Hatton <jhatton@globalfinishing.com>\nSigned-off-by: brian m. carlson <bk2204@github.com>\n---\n statinfo.c        | 20 ++++++++++++++++++--\n t/t7508-status.sh | 16 ++++++++++++++++\n 2 files changed, 34 insertions(+), 2 deletions(-)\n\ndiff --git a/statinfo.c b/statinfo.c\nindex 17bb8966c3..9367ca099c 100644\n--- a/statinfo.c\n+++ b/statinfo.c\n@@ -2,6 +2,22 @@\n #include \"environment.h\"\n #include \"statinfo.h\"\n \n+/*\n+ * Munge st_size into an unsigned int.\n+ */\n+static unsigned int munge_st_size(off_t st_size) {\n+\tunsigned int sd_size = st_size;\n+\n+\t/*\n+\t * If the file is an exact multiple of 4 GiB, modify the value so it\n+\t * doesn't get marked as racily clean (zero).\n+\t */\n+\tif (!sd_size && st_size)\n+\t\treturn 0x80000000;\n+\telse\n+\t\treturn sd_size;\n+}\n+\n void fill_stat_data(struct stat_data *sd, struct stat *st)\n {\n \tsd->sd_ctime.sec = (unsigned int)st->st_ctime;\n@@ -12,7 +28,7 @@ void fill_stat_data(struct stat_data *sd, struct stat *st)\n \tsd->sd_ino = st->st_ino;\n \tsd->sd_uid = st->st_uid;\n \tsd->sd_gid = st->st_gid;\n-\tsd->sd_size = st->st_size;\n+\tsd->sd_size = munge_st_size(st->st_size);\n }\n \n int match_stat_data(const struct stat_data *sd, struct stat *st)\n@@ -51,7 +67,7 @@ int match_stat_data(const struct stat_data *sd, struct stat *st)\n \t\t\tchanged |= INODE_CHANGED;\n #endif\n \n-\tif (sd->sd_size != (unsigned int) st->st_size)\n+\tif (sd->sd_size != munge_st_size(st->st_size))\n \t\tchanged |= DATA_CHANGED;\n \n \treturn changed;\ndiff --git a/t/t7508-status.sh b/t/t7508-status.sh\nindex 6928fd89f5..6c46648e11 100755\n--- a/t/t7508-status.sh\n+++ b/t/t7508-status.sh\n@@ -1745,4 +1745,20 @@ test_expect_success 'slow status advice when core.untrackedCache true, and fsmon\n \t)\n '\n \n+test_expect_success EXPENSIVE 'status does not re-read unchanged 4 or 8 GiB file' '\n+\t(\n+\t\tmkdir large-file &&\n+\t\tcd large-file &&\n+\t\t# Files are 2 GiB, 4 GiB, and 8 GiB sparse files.\n+\t\ttest-tool truncate file-a 0x080000000 &&\n+\t\ttest-tool truncate file-b 0x100000000 &&\n+\t\ttest-tool truncate file-c 0x200000000 &&\n+\t\t# This will be slow.\n+\t\tgit add file-a file-b file-c &&\n+\t\tgit commit -m \"add large files\" &&\n+\t\tgit diff-index HEAD file-a file-b file-c >actual &&\n+\t\ttest_must_be_empty actual\n+\t)\n+'\n+\n test_done\n"},{"id":"483152","messageId":"20231012160930.330618-1-sandals@crustytoothpaste.net","threadId":"60352","inReplyTo":null,"subject":"[PATCH v2 0/2] Prevent re-reading 4 GiB files on every status","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2023-10-12T16:09:28Z","receivedAt":"2023-10-12T16:10:03Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"Several people have noticed that Git continually re-reads and re-hashes\nfiles that are an exact multiple of 4 GiB every time the index is\nrefreshed (for example, when \"git status\" is called).  This is slow and\nexpensive, especially with SHA-1 DC, and it also causes performance\nproblems when these files are used with Git LFS (because the same issue\noccurs, just with Git LFS hashing the data instead of Git).\n\nJason Hatton sent a patch previously to fix this, but it lacked tests\nand didn't get picked up.  I've adopted their patch, making some minor\nchanges to the commit message and including some tests, and also\nincluding a suitable test helper to make the tests possible.  All credit\nshould be directed to Jason, and I'll accept all the responsibility for\nany problems.\n\nI don't anticipate this being in any way controversial, so I'm not\nexpecting a huge number of rerolls, but of course one or two might be\nnecessary.\n\nJason Hatton (1):\n  Prevent git from rehashing 4GiB files\n\nbrian m. carlson (1):\n  t: add a test helper to truncate files\n\n Makefile                 |  1 +\n statinfo.c               | 20 ++++++++++++++++++--\n t/helper/test-tool.c     |  1 +\n t/helper/test-tool.h     |  1 +\n t/helper/test-truncate.c | 27 +++++++++++++++++++++++++++\n t/t7508-status.sh        | 16 ++++++++++++++++\n 6 files changed, 64 insertions(+), 2 deletions(-)\n create mode 100644 t/helper/test-truncate.c\n\n"},{"id":"483164","messageId":"CAPig+cSUvyCS-NOYOoJAmg7LGyU5Dqky5HyS-QTNW1QoHj-0bA@mail.gmail.com","threadId":"60352","inReplyTo":"20231012160930.330618-2-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2 1/2] t: add a test helper to truncate files","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-10-12T17:49:13Z","receivedAt":"2023-10-12T17:49:30Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Oct 12, 2023 at 12:10 PM brian m. carlson\n<sandals@crustytoothpaste.net> wrote:\n> In a future commit, we're going to work with some large files which will\n> be at least 4 GiB in size.  To take advantage of the sparseness\n> functionality on most Unix systems and avoid running the system out of\n> disk, it would be convenient to use truncate(2) to simply create a\n> sparse file of sufficient size.\n>\n> However, the GNU truncate(1) utility isn't portable, so let's write a\n> tiny test helper that does the work for us.\n>\n> Signed-off-by: brian m. carlson <bk2204@github.com>\n> ---\n> diff --git a/t/helper/test-truncate.c b/t/helper/test-truncate.c\n> @@ -0,0 +1,27 @@\n> +int cmd__truncate(int argc, const char **argv)\n> +{\n> +       char *p = NULL;\n> +       uintmax_t sz = 0;\n> +       int fd = -1;\n> +\n> +       if (argc != 3)\n> +               die(\"expected filename and size\");\n> +\n> +       sz = strtoumax(argv[2], &p, 0);\n> +       if (*p)\n> +               die(\"invalid size\");\n\nDo you want to check 'errno' here, as well (probably before the '*p' check)?\n\nOr is that being too defensive for a 'test-tool' command?\n\n> +       fd = open(argv[1], O_WRONLY | O_CREAT, 0600);\n> +       if (fd < 0)\n> +               die_errno(\"failed to open file %s\", argv[1]);\n> +\n> +       if (ftruncate(fd, (off_t) sz) < 0)\n> +               die_errno(\"failed to truncate file\");\n> +       return 0;\n> +}\n"},{"id":"483165","messageId":"xmqqpm1jn2nh.fsf@gitster.g","threadId":"60352","inReplyTo":"20231012160930.330618-3-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2 2/2] Prevent git from rehashing 4GiB files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-12T17:58:42Z","receivedAt":"2023-10-12T17:59:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n> An example would be to have a 2^32 sized file in the index of\n> patched git. Patched git would save the file as 2^31 in the cache.\n> An unpatched git would very much see the file has changed in size\n> and force it to rehash the file, which is safe.\n\nThe reason why this is \"safe\" is because an older Git will would\nkeep rehashing whether 2^31 or 0 is stored as its sd_size, so the\nchange is not making things worse?  With older git, \"git diff-files\"\nwill report that such a file is not up to date, and then the user\nwill refresh the index, which will store 0 as its sd_file, so\ntentatively \"git status\" may give a wrong information, but we\nprobalby do not care?  Is that how the reasoning goes?\n\n> +/*\n> + * Munge st_size into an unsigned int.\n> + */\n> +static unsigned int munge_st_size(off_t st_size) {\n> +\tunsigned int sd_size = st_size;\n> +\n> +\t/*\n> +\t * If the file is an exact multiple of 4 GiB, modify the value so it\n> +\t * doesn't get marked as racily clean (zero).\n> +\t */\n> +\tif (!sd_size && st_size)\n> +\t\treturn 0x80000000;\n> +\telse\n> +\t\treturn sd_size;\n> +}\n\nThis assumes typeof(sd_size) aka \"unsigned int\" is always 32-bit,\nwhich does not sound reasonable.  Reference to 4GiB, 2^32 and 2^31\nin the code and the proposed commit log message need to be qualified\nwith \"on a platform whose uint is 32-bit\" or something, or better\nyet, phrased in a way that is agnostic to the integer size.  At\nthe very least, the hardcoded 0x80000000 needs to be rethought, I\nam afraid.\n\nOther than that, the workaround for the racy-git avoidance code does\nsound good.  I actually wonder if we should always use 1 regardless\nof integer size.\n\n"},{"id":"483183","messageId":"ZShsG1SKfrefsCtu@tapette.crustytoothpaste.net","threadId":"60352","inReplyTo":"xmqqpm1jn2nh.fsf@gitster.g","subject":"Re: [PATCH v2 2/2] Prevent git from rehashing 4GiB files","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2023-10-12T21:58:51Z","receivedAt":"2023-10-12T21:58:55Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2023-10-12 at 17:58:42, Junio C Hamano wrote:\n> \"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n> \n> > An example would be to have a 2^32 sized file in the index of\n> > patched git. Patched git would save the file as 2^31 in the cache.\n> > An unpatched git would very much see the file has changed in size\n> > and force it to rehash the file, which is safe.\n> \n> The reason why this is \"safe\" is because an older Git will would\n> keep rehashing whether 2^31 or 0 is stored as its sd_size, so the\n> change is not making things worse?  With older git, \"git diff-files\"\n> will report that such a file is not up to date, and then the user\n> will refresh the index, which will store 0 as its sd_file, so\n> tentatively \"git status\" may give a wrong information, but we\n> probalby do not care?  Is that how the reasoning goes?\n\nCorrect.  An older Git will rehash it either way, on every git status.\nIf you run git diff-files on an older Git, it will always show it as\nmodified (hence why I used that in the tests).  The git status will\nrehash it and then realize that it isn't modified, not printing any\nchanges, so the behaviour is not wrong, it's just extremely slow (SHA-1\nDC on 4 GiB of data).\n\nIf you use a new Git with an old index, git status will still rehash it\nthe first time and correctly determine that it isn't changed, then write\nthe 0x80000000 to the index.  It just won't rehash it on subsequent\ncalls to git status.\n\nThe only incorrect behaviour (assuming that slow is not incorrect) that\nI'm aware of on older Git is that git diff-files marks it as modified\nwhen it's not.  There is no incorrect behaviour on a fixed Git.\n\n> > +/*\n> > + * Munge st_size into an unsigned int.\n> > + */\n> > +static unsigned int munge_st_size(off_t st_size) {\n> > +\tunsigned int sd_size = st_size;\n> > +\n> > +\t/*\n> > +\t * If the file is an exact multiple of 4 GiB, modify the value so it\n> > +\t * doesn't get marked as racily clean (zero).\n> > +\t */\n> > +\tif (!sd_size && st_size)\n> > +\t\treturn 0x80000000;\n> > +\telse\n> > +\t\treturn sd_size;\n> > +}\n> \n> This assumes typeof(sd_size) aka \"unsigned int\" is always 32-bit,\n> which does not sound reasonable.  Reference to 4GiB, 2^32 and 2^31\n> in the code and the proposed commit log message need to be qualified\n> with \"on a platform whose uint is 32-bit\" or something, or better\n> yet, phrased in a way that is agnostic to the integer size.  At\n> the very least, the hardcoded 0x80000000 needs to be rethought, I\n> am afraid.\n\nunsigned int is 32-bit on every modern 32- or 64-bit platform known to\nexist today.  I don't believe we support MS-DOS or systems of its era,\nnor should we support 8- or 16-bit systems.  If you'd prefer \"uint32_t\",\nI can do that.\n\nNote that the problem is reproducible on a stock Ubuntu amd64 system, so\nit is definitely a problem on all modern systems.\n\n> Other than that, the workaround for the racy-git avoidance code does\n> sound good.  I actually wonder if we should always use 1 regardless\n> of integer size.\n\nI think using 2^31 is better because it's far away from very small\nvalues and very large values, which means that it's a hard to modify a\nfile which used to have its value as a multiple of 4 GiB and\naccidentally make Git think it was unchanged.  Using 1 would make a\nsimple \"echo > foo\" possibly think things were unchanged in some cases,\nwhich we should avoid.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"483184","messageId":"xmqqzg0n32zo.fsf@gitster.g","threadId":"60352","inReplyTo":"ZShsG1SKfrefsCtu@tapette.crustytoothpaste.net","subject":"Re: [PATCH v2 2/2] Prevent git from rehashing 4GiB files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-12T22:11:39Z","receivedAt":"2023-10-12T22:11:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n> nor should we support 8- or 16-bit systems.  If you'd prefer \"uint32_t\",\n> I can do that.\n\nGoing into statinfo.h and updating sd_size to uint32_t is totally\noutside the scope of this fix.\n\n> I think using 2^31 is better because it's far away from very small\n> values and very large values, which means that it's a hard to modify a\n> file which used to have its value as a multiple of 4 GiB and\n> accidentally make Git think it was unchanged.  Using 1 would make a\n> simple \"echo > foo\" possibly think things were unchanged in some cases,\n> which we should avoid.\n\nThe reason I gave the extreme \"1-byte\" was to gauge how much actual\n\"size\" we are relying on the correctness of this change.  As mtime\nis giving the primary protection from false matching of cached stat\ninformation, I do not think \"echo >foo\" would be a huge issue.  IOW\nmy stance is 1U<<31 is as good as 1U<<0, so I do not oppose to the\nformer, either.  But in a few years, 64-bit integers may cease to be\ntoo odd, who knows ;-)\n\n\n"},{"id":"483186","messageId":"xmqqsf6f312s.fsf@gitster.g","threadId":"60352","inReplyTo":"20231012160930.330618-2-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2 1/2] t: add a test helper to truncate files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-12T22:52:59Z","receivedAt":"2023-10-12T22:53:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n> +\tfd = open(argv[1], O_WRONLY | O_CREAT, 0600);\n> +\tif (fd < 0)\n> +\t\tdie_errno(\"failed to open file %s\", argv[1]);\n\ncontrib/coccinelle/xopen.cocci tells us to write this simply as\n\n\tfd = xopen(argv[1], O_WRONLY | O_CREAT, 0600);\n\nhttps://github.com/git/git/actions/runs/6500777388/job/17656837393#step:4:657\n\n"},{"id":"483228","messageId":"ZSmmD5sz7TPWCGRd@tapette.crustytoothpaste.net","threadId":"60352","inReplyTo":"xmqqsf6f312s.fsf@gitster.g","subject":"Re: [PATCH v2 1/2] t: add a test helper to truncate files","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2023-10-13T20:18:23Z","receivedAt":"2023-10-13T20:18:29Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2023-10-12 at 22:52:59, Junio C Hamano wrote:\n> \"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n> \n> > +\tfd = open(argv[1], O_WRONLY | O_CREAT, 0600);\n> > +\tif (fd < 0)\n> > +\t\tdie_errno(\"failed to open file %s\", argv[1]);\n> \n> contrib/coccinelle/xopen.cocci tells us to write this simply as\n> \n> \tfd = xopen(argv[1], O_WRONLY | O_CREAT, 0600);\n\nSure, I can do that.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"483229","messageId":"ZSmnXpQ3hpLM6Xpf@tapette.crustytoothpaste.net","threadId":"60352","inReplyTo":"CAPig+cSUvyCS-NOYOoJAmg7LGyU5Dqky5HyS-QTNW1QoHj-0bA@mail.gmail.com","subject":"Re: [PATCH v2 1/2] t: add a test helper to truncate files","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2023-10-13T20:23:58Z","receivedAt":"2023-10-13T20:24:01Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2023-10-12 at 17:49:13, Eric Sunshine wrote:\n> > +       sz = strtoumax(argv[2], &p, 0);\n> > +       if (*p)\n> > +               die(\"invalid size\");\n> \n> Do you want to check 'errno' here, as well (probably before the '*p' check)?\n> \n> Or is that being too defensive for a 'test-tool' command?\n\nI don't believe that's necessary.  The Linux manual page leads me to\nbelieve that if *argv[2] is not 0 but *p is 0, then the entire string is\nvalid, which would imply that errno is not set.  I'm happy to ignore for\nthe moment the case where the user specifies \"\" as the argument, because\nit is a test helper and \"don't do weird, unexpected things with the test\nhelper without looking at the source code first\" is legitimate advice\nfor Git developers.\n\nIf this were user-facing, improved robustness would be warranted, I\nagree.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"483232","messageId":"xmqq5y3az2ju.fsf@gitster.g","threadId":"60352","inReplyTo":"ZSmmD5sz7TPWCGRd@tapette.crustytoothpaste.net","subject":"Re: [PATCH v2 1/2] t: add a test helper to truncate files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-13T20:32:21Z","receivedAt":"2023-10-13T20:32:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n> On 2023-10-12 at 22:52:59, Junio C Hamano wrote:\n>> \"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n>> \n>> > +\tfd = open(argv[1], O_WRONLY | O_CREAT, 0600);\n>> > +\tif (fd < 0)\n>> > +\t\tdie_errno(\"failed to open file %s\", argv[1]);\n>> \n>> contrib/coccinelle/xopen.cocci tells us to write this simply as\n>> \n>> \tfd = xopen(argv[1], O_WRONLY | O_CREAT, 0600);\n>\n> Sure, I can do that.\n\nUnless there are other changes needed, I'll handle it on this end.\n\nThanks.\n"},{"id":"483324","messageId":"20231016235333.GA551672@coredump.intra.peff.net","threadId":"60352","inReplyTo":"20231012160930.330618-2-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2 1/2] t: add a test helper to truncate files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-16T23:53:33Z","receivedAt":"2023-10-16T23:53:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 12, 2023 at 04:09:29PM +0000, brian m. carlson wrote:\n\n> +int cmd__truncate(int argc, const char **argv)\n> +{\n> +\tchar *p = NULL;\n> +\tuintmax_t sz = 0;\n> +\tint fd = -1;\n> +\n> +\tif (argc != 3)\n> +\t\tdie(\"expected filename and size\");\n> +\n> +\tsz = strtoumax(argv[2], &p, 0);\n> +\tif (*p)\n> +\t\tdie(\"invalid size\");\n> +\n> +\tfd = open(argv[1], O_WRONLY | O_CREAT, 0600);\n> +\tif (fd < 0)\n> +\t\tdie_errno(\"failed to open file %s\", argv[1]);\n> +\n> +\tif (ftruncate(fd, (off_t) sz) < 0)\n> +\t\tdie_errno(\"failed to truncate file\");\n> +\treturn 0;\n> +}\n\nCoverity flagged this as leaking the descriptor \"fd\" (which is obviously\ntrue). I guess it doesn't matter much in practice, since we're exiting\nthe process directly afterwards. If it were a memory leak we'd free() it\nanyway to shut up the leak detector, but I don't know if we want to be\nas careful here (in theory it creates noise that distracts from real\nproblems, but Coverity is noisy enough that I don't even bother looking\nat existing problems).\n\n-Peff\n"},{"id":"483325","messageId":"20231017000019.GB551672@coredump.intra.peff.net","threadId":"60352","inReplyTo":"20231012160930.330618-3-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2 2/2] Prevent git from rehashing 4GiB files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-17T00:00:19Z","receivedAt":"2023-10-17T00:00:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 12, 2023 at 04:09:30PM +0000, brian m. carlson wrote:\n\n> +static unsigned int munge_st_size(off_t st_size) {\n> +\tunsigned int sd_size = st_size;\n> +\n> +\t/*\n> +\t * If the file is an exact multiple of 4 GiB, modify the value so it\n> +\t * doesn't get marked as racily clean (zero).\n> +\t */\n> +\tif (!sd_size && st_size)\n> +\t\treturn 0x80000000;\n> +\telse\n> +\t\treturn sd_size;\n> +}\n\nCoverity complained that the \"true\" side of this conditional is\nunreachable, since sd_size is assigned from st_size, so the two values\ncannot be both true and false. But obviously we are depending here on\nthe truncation of off_t to \"unsigned int\". I'm not sure if Coverity is\njust dumb, or if it somehow has a different size for off_t.\n\nI don't _think_ this would ever cause confusion in a real compiler, as\nassignment from a larger type to a smaller has well-defined truncation,\nas far as I know.\n\nBut I do wonder if an explicit \"& 0xFFFFFFFF\" would make it more obvious\nwhat is happening (which would also do the right thing if in some\nhypothetical platform \"unsigned int\" ended up larger than 32 bits).\n\n-Peff\n"},{"id":"483342","messageId":"MW5PR16MB50791EDD3D006EFBE8DBBCB3AFD6A@MW5PR16MB5079.namprd16.prod.outlook.com","threadId":"60352","inReplyTo":"20231017000019.GB551672@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/2] Prevent git from rehashing 4GiB files","fromName":"Jason Hatton","fromEmail":"jhatton@globalfinishing.com","sentAt":"2023-10-17T14:49:09Z","receivedAt":"2023-10-17T14:50:49Z","isPatch":true,"sender":{"key":"jhatton@globalfinishing.com","avatar":"https://avatars.githubusercontent.com/u/36747462?v=4"},"body":"From: Jeff King <peff@peff.net>\n\n> On Thu, Oct 12, 2023 at 04:09:30PM +0000, brian m. carlson wrote:\n> \n> > +static unsigned int munge_st_size(off_t st_size) {\n> > +\tunsigned int sd_size = st_size;\n> > +\n> > +\t/*\n> > +\t * If the file is an exact multiple of 4 GiB, modify the value so it\n> > +\t * doesn't get marked as racily clean (zero).\n> > +\t */\n> > +\tif (!sd_size && st_size)\n> > +\t\treturn 0x80000000;\n> > +\telse\n> > +\t\treturn sd_size;\n> > +}\n> \n> Coverity complained that the \"true\" side of this conditional is\n> unreachable, since sd_size is assigned from st_size, so the two values\n> cannot be both true and false. But obviously we are depending here on\n> the truncation of off_t to \"unsigned int\". I'm not sure if Coverity is\n> just dumb, or if it somehow has a different size for off_t.\n> \n> I don't _think_ this would ever cause confusion in a real compiler, as\n> assignment from a larger type to a smaller has well-defined truncation,\n> as far as I know.\n> \n> But I do wonder if an explicit \"& 0xFFFFFFFF\" would make it more obvious\n> what is happening (which would also do the right thing if in some\n> hypothetical platform \"unsigned int\" ended up larger than 32 bits).\n> \n> -Peff\n\nI originally wrote the code this way to work exactly like the original\ncode with one exception: Never truncate a nonzero st_size to a zero\nsd_size. The original code is here in fill_stat_data:\n\nI was attempting to use exactly the same implicit type conversion and\ntypes as the original.\n\nWe could probably write the below to do the same thing.\n\nvoid fill_stat_data(struct stat_data *sd, struct stat *st)\n{\n      sd->sd_ctime.sec = (unsigned int)st->st_ctime;\n      sd->sd_mtime.sec = (unsigned int)st->st_mtime;\n      sd->sd_ctime.nsec = ST_CTIME_NSEC(*st);\n      sd->sd_mtime.nsec = ST_MTIME_NSEC(*st);\n      sd->sd_dev = st->st_dev;\n      sd->sd_ino = st->st_ino;\n      sd->sd_uid = st->st_uid;\n      sd->sd_gid = st->st_gid;\n      sd->sd_size = st->st_size;\n+      if (sd->sd_size == 0 && st->st_size!= 0) {\n+            sd->sd_size = 1;\n+      }\n}\n\n- Jason D. Hatton\n\n"},{"id":"483354","messageId":"xmqqedhtji66.fsf@gitster.g","threadId":"60352","inReplyTo":"MW5PR16MB50791EDD3D006EFBE8DBBCB3AFD6A@MW5PR16MB5079.namprd16.prod.outlook.com","subject":"Re: [PATCH v2 2/2] Prevent git from rehashing 4GiB files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-17T17:02:57Z","receivedAt":"2023-10-17T17:03:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jason Hatton <jhatton@globalfinishing.com> writes:\n\n> We could probably write the below to do the same thing.\n>\n> void fill_stat_data(struct stat_data *sd, struct stat *st)\n> {\n>       sd->sd_ctime.sec = (unsigned int)st->st_ctime;\n>       sd->sd_mtime.sec = (unsigned int)st->st_mtime;\n>       sd->sd_ctime.nsec = ST_CTIME_NSEC(*st);\n>       sd->sd_mtime.nsec = ST_MTIME_NSEC(*st);\n>       sd->sd_dev = st->st_dev;\n>       sd->sd_ino = st->st_ino;\n>       sd->sd_uid = st->st_uid;\n>       sd->sd_gid = st->st_gid;\n>       sd->sd_size = st->st_size;\n> +      if (sd->sd_size == 0 && st->st_size!= 0) {\n> +            sd->sd_size = 1;\n> +      }\n> }\n\nThe above is a fairly straight-forward inlining (except that it does\nexplicit comparisons with zero) of the helper function the version\nof patch under discussion added, and uses 1 instead of (1<<31) as an\narbigrary nonzero number that can be used to work around the issue.\n\nSo I agree with you that it would do the same thing.  I am not\nsurprised if it also gets scolded by Coverity the same way, though.\n"},{"id":"483377","messageId":"ZS8p8pdUiM6NmiVo@tapette.crustytoothpaste.net","threadId":"60352","inReplyTo":"20231017000019.GB551672@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/2] Prevent git from rehashing 4GiB files","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2023-10-18T00:42:26Z","receivedAt":"2023-10-18T00:42:30Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2023-10-17 at 00:00:19, Jeff King wrote:\n> On Thu, Oct 12, 2023 at 04:09:30PM +0000, brian m. carlson wrote:\n> \n> > +static unsigned int munge_st_size(off_t st_size) {\n> > +\tunsigned int sd_size = st_size;\n> > +\n> > +\t/*\n> > +\t * If the file is an exact multiple of 4 GiB, modify the value so it\n> > +\t * doesn't get marked as racily clean (zero).\n> > +\t */\n> > +\tif (!sd_size && st_size)\n> > +\t\treturn 0x80000000;\n> > +\telse\n> > +\t\treturn sd_size;\n> > +}\n> \n> Coverity complained that the \"true\" side of this conditional is\n> unreachable, since sd_size is assigned from st_size, so the two values\n> cannot be both true and false. But obviously we are depending here on\n> the truncation of off_t to \"unsigned int\". I'm not sure if Coverity is\n> just dumb, or if it somehow has a different size for off_t.\n\nTechnically, on 32-bit machines, you can have a 32-bit off_t if you\ndon't compile with -D_FILE_OFFSET_BITS=64.  However, such a program is\nnot very useful on modern systems, since it wouldn't be able to handle\nfiles that are 2 GiB or larger, and so everyone considers that a silly\nand buggy way to compile programs.\n\n> I don't _think_ this would ever cause confusion in a real compiler, as\n> assignment from a larger type to a smaller has well-defined truncation,\n> as far as I know.\n> \n> But I do wonder if an explicit \"& 0xFFFFFFFF\" would make it more obvious\n> what is happening (which would also do the right thing if in some\n> hypothetical platform \"unsigned int\" ended up larger than 32 bits).\n\nSuch a hypothetical platform is of course allowed by the C standard, but\nit's also going to run near zero real Unix programs or kernels.  I am at\nthis point reflecting on the prudent decision Rust made to make\nalmost everything an explicit bit size (e.g., u32, i64).\n\nNonetheless, I'll reroll this with the other things you mentioned, and\nI'll switch that \"unsigned int\" above to \"uint32_t\", which I think makes\nthis more obvious.\n\nI don't, however, intend to change the constant from 0x8000000 to 1,\nbecause I think that's a poorer choice, but I'll try to explain it\nbetter in the commit message.  (I had originally aimed to avoid editing\nit as much as possible since it's not my name on the commit to avoid\nJason getting blamed unnecessarily for any mistakes I might make.)\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"}]}