{"thread":{"id":"61018","subject":"[PATCH v2 0/2] Change xwrite() to write_in_full() in builtins.","startedAt":"2024-02-27T15:09:59Z","lastAt":"2024-03-07T10:00:19Z","messageCount":12,"participants":["Randall S. Becker","Junio C Hamano","rsbecker@nexbridge.com","Jeff King"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"489534","messageId":"20240227150934.7950-1-randall.becker@nexbridge.ca","threadId":"61018","inReplyTo":null,"subject":"[PATCH v2 0/2] Change xwrite() to write_in_full() in builtins.","fromName":"Randall S. Becker","fromEmail":"the.n.e.key@gmail.com","sentAt":"2024-02-27T15:09:31Z","receivedAt":"2024-02-27T15:09:59Z","isPatch":true,"sender":{"key":"the.n.e.key@gmail.com","avatar":null},"body":"From: \"Randall S. Becker\" <rsbecker@nexbridge.com>\n\nThis series replaces xwrite to write_in_full in builtins/. The change is\nrequired to fix critical problems that prevent full writes to be\nprocessed by on platforms where xwrite may be limited to a platform size\nlimit. Further changes outside of builtins/ may be required but do not\nappear to be as urgent as this change, which causes test breakage in\nt7704. A separate series will be contributed for changes outside of\nbuiltins/ at a later date.\n\nThe change in unpack-objects.c is necessary as len is being passed into\nxwrite that exceeds the size supported by the limit in that method\n(56Kb on NonStop ia64). \n\nRandall S. Becker (3):\n  builtin/repack.c: change xwrite to write_in_full and report errors.\n  builtin/receive-pack.c: change xwrite to write_in_full.\n  builtin/unpack-objects.c: change xwrite to write_in_full avoid\n    truncation.\n\n builtin/receive-pack.c   | 2 +-\n builtin/repack.c         | 9 +++++++--\n builtin/unpack-objects.c | 2 +-\n 3 files changed, 9 insertions(+), 4 deletions(-)\n\n-- \n2.42.1\n\n"},{"id":"489535","messageId":"20240227150934.7950-2-randall.becker@nexbridge.ca","threadId":"61018","inReplyTo":"20240227150934.7950-1-randall.becker@nexbridge.ca","subject":"[PATCH v2 1/3] builtin/repack.c: change xwrite to write_in_full and report errors.","fromName":"Randall S. Becker","fromEmail":"the.n.e.key@gmail.com","sentAt":"2024-02-27T15:09:32Z","receivedAt":"2024-02-27T15:09:59Z","isPatch":true,"sender":{"key":"the.n.e.key@gmail.com","avatar":null},"body":"From: \"Randall S. Becker\" <rsbecker@nexbridge.com>\n\nThis change is required because some platforms do not support file writes of\narbitrary sizes (e.g, NonStop). xwrite ends up truncating the output to the\nmaximum single I/O size possible for the destination device. The result of\nwrite_in_full() is also passed to the caller, which was previously ignored.\n\nSigned-off-by: Randall S. Becker <rsbecker@nexbridge.com>\n---\n builtin/repack.c | 9 +++++++--\n 1 file changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex ede36328a3..932d24c60b 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -307,6 +307,7 @@ static int write_oid(const struct object_id *oid,\n \t\t     struct packed_git *pack UNUSED,\n \t\t     uint32_t pos UNUSED, void *data)\n {\n+\tint err;\n \tstruct child_process *cmd = data;\n \n \tif (cmd->in == -1) {\n@@ -314,8 +315,12 @@ static int write_oid(const struct object_id *oid,\n \t\t\tdie(_(\"could not start pack-objects to repack promisor objects\"));\n \t}\n \n-\txwrite(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz);\n-\txwrite(cmd->in, \"\\n\", 1);\n+\terr = write_in_full(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz);\n+\tif (err <= 0)\n+\t\treturn err;\n+\terr = write_in_full(cmd->in, \"\\n\", 1);\n+\tif (err <= 0)\n+\t\treturn err;\n \treturn 0;\n }\n \n-- \n2.42.1\n\n"},{"id":"489536","messageId":"20240227150934.7950-3-randall.becker@nexbridge.ca","threadId":"61018","inReplyTo":"20240227150934.7950-1-randall.becker@nexbridge.ca","subject":"[PATCH v2 2/3] builtin/receive-pack.c: change xwrite to write_in_full.","fromName":"Randall S. Becker","fromEmail":"the.n.e.key@gmail.com","sentAt":"2024-02-27T15:09:33Z","receivedAt":"2024-02-27T15:10:00Z","isPatch":true,"sender":{"key":"the.n.e.key@gmail.com","avatar":null},"body":"From: \"Randall S. Becker\" <rsbecker@nexbridge.com>\n\nThis change is required because some platforms do not support file writes of    arbitrary sizes (e.g, NonStop). xwrite ends up truncating the output to the\nmaximum single I/O size possible for the destination device.\n\nSigned-off-by: Randall S. Becker <rsbecker@nexbridge.com>\n---\n builtin/receive-pack.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex db65607485..4277c63d08 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -456,7 +456,7 @@ static void report_message(const char *prefix, const char *err, va_list params)\n \tif (use_sideband)\n \t\tsend_sideband(1, 2, msg, sz, use_sideband);\n \telse\n-\t\txwrite(2, msg, sz);\n+\t\twrite_in_full(2, msg, sz);\n }\n \n __attribute__((format (printf, 1, 2)))\n-- \n2.42.1\n\n"},{"id":"489537","messageId":"20240227150934.7950-4-randall.becker@nexbridge.ca","threadId":"61018","inReplyTo":"20240227150934.7950-1-randall.becker@nexbridge.ca","subject":"[PATCH v2 3/3] builtin/unpack-objects.c: change xwrite to write_in_full avoid truncation.","fromName":"Randall S. Becker","fromEmail":"the.n.e.key@gmail.com","sentAt":"2024-02-27T15:09:34Z","receivedAt":"2024-02-27T15:10:01Z","isPatch":true,"sender":{"key":"the.n.e.key@gmail.com","avatar":null},"body":"From: \"Randall S. Becker\" <rsbecker@nexbridge.com>\n\nThis change is required because some platforms do not support file writes of\narbitrary sizes (e.g, NonStop). xwrite ends up truncating the output to the\nmaximum single I/O size possible for the destination device if the supplied\nlen value exceeds the supported value. Replacing xwrite with write_in_full\ncorrects this problem. Future optimisations could remove the loop in favour\nof just calling write_in_full.\n\nSigned-off-by: Randall S. Becker <rsbecker@nexbridge.com>\n---\n builtin/unpack-objects.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c\nindex e0a701f2b3..6935c4574e 100644\n--- a/builtin/unpack-objects.c\n+++ b/builtin/unpack-objects.c\n@@ -680,7 +680,7 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix UNUSED)\n \n \t/* Write the last part of the buffer to stdout */\n \twhile (len) {\n-\t\tint ret = xwrite(1, buffer + offset, len);\n+\t\tint ret = write_in_full(1, buffer + offset, len);\n \t\tif (ret <= 0)\n \t\t\tbreak;\n \t\tlen -= ret;\n-- \n2.42.1\n\n"},{"id":"489562","messageId":"xmqq5xy9spxi.fsf@gitster.g","threadId":"61018","inReplyTo":"20240227150934.7950-2-randall.becker@nexbridge.ca","subject":"Re: [PATCH v2 1/3] builtin/repack.c: change xwrite to write_in_full and report errors.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-27T18:49:29Z","receivedAt":"2024-02-27T18:49:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Randall S. Becker\" <the.n.e.key@gmail.com> writes:\n\n> From: \"Randall S. Becker\" <rsbecker@nexbridge.com>\n>\n> This change is required because some platforms do not support file writes of\n> arbitrary sizes (e.g, NonStop). xwrite ends up truncating the output to the\n> maximum single I/O size possible for the destination device. The result of\n> write_in_full() is also passed to the caller, which was previously ignored.\n\nThis misleads readers to think that maximum single I/O size is\nsmaller than a single write of oid_to_hex() string on some\nplatforms.  I somehow do not think that is why we want to make this\nchange.\n\nRather, the use of these xwrites() are simply wrong regardless of\nmaximum I/O size of the platforms, as this caller is not prepared to\nsee xwrite() result in a short write(2), and we do want to write all\nbytes we have even in such a case.\n\nYou're right to also point out that we attempt to propagate the errors\nto the caller (but see below).\n\n> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>\n> ---\n>  builtin/repack.c | 9 +++++++--\n>  1 file changed, 7 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index ede36328a3..932d24c60b 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -307,6 +307,7 @@ static int write_oid(const struct object_id *oid,\n>  \t\t     struct packed_git *pack UNUSED,\n>  \t\t     uint32_t pos UNUSED, void *data)\n>  {\n> +\tint err;\n>  \tstruct child_process *cmd = data;\n>  \n>  \tif (cmd->in == -1) {\n> @@ -314,8 +315,12 @@ static int write_oid(const struct object_id *oid,\n>  \t\t\tdie(_(\"could not start pack-objects to repack promisor objects\"));\n>  \t}\n>  \n> -\txwrite(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz);\n> -\txwrite(cmd->in, \"\\n\", 1);\n> +\terr = write_in_full(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz);\n> +\tif (err <= 0)\n> +\t\treturn err;\n> +\terr = write_in_full(cmd->in, \"\\n\", 1);\n> +\tif (err <= 0)\n> +\t\treturn err;\n>  \treturn 0;\n>  }\n\nI think this has already been brought up, but the caller of this\nhelper does not make such an error stand out enough and instead\nmakes the resulting repack silently produce wrong result, which\nis not an improvement.  Perhaps\n\n\tif (write_in_full(...) ||\n\t    write_in_full(...))\n\t\tdie(_(\"failed to list promisor objects to repack\"));\n\nor something?\n\nThanks.\n"},{"id":"489563","messageId":"xmqq1q8xspht.fsf@gitster.g","threadId":"61018","inReplyTo":"20240227150934.7950-4-randall.becker@nexbridge.ca","subject":"Re: [PATCH v2 3/3] builtin/unpack-objects.c: change xwrite to write_in_full avoid truncation.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-27T18:58:54Z","receivedAt":"2024-02-27T18:58:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Randall S. Becker\" <the.n.e.key@gmail.com> writes:\n\n> From: \"Randall S. Becker\" <rsbecker@nexbridge.com>\n>\n> This change is required because some platforms do not support file writes of\n> arbitrary sizes (e.g, NonStop). xwrite ends up truncating the output to the\n> maximum single I/O size possible for the destination device if the supplied\n> len value exceeds the supported value. Replacing xwrite with write_in_full\n> corrects this problem. Future optimisations could remove the loop in favour\n> of just calling write_in_full.\n>\n> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>\n> ---\n>  builtin/unpack-objects.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c\n> index e0a701f2b3..6935c4574e 100644\n> --- a/builtin/unpack-objects.c\n> +++ b/builtin/unpack-objects.c\n> @@ -680,7 +680,7 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix UNUSED)\n>  \n>  \t/* Write the last part of the buffer to stdout */\n>  \twhile (len) {\n> -\t\tint ret = xwrite(1, buffer + offset, len);\n> +\t\tint ret = write_in_full(1, buffer + offset, len);\n>  \t\tif (ret <= 0)\n>  \t\t\tbreak;\n>  \t\tlen -= ret;\n\nWhy do we need this with a retry loop that is prepared for short\nwrite(2) specifically like this?\n\nIf xwrite() calls underlying write(2) with too large a value, then\nyour MAX_IO_SIZE is misconfigured, and the fix should go there, not\nhere in a loop that expects a working xwrite() that is allowed to\nreturn on short write(2), I would think.\n\n"},{"id":"489564","messageId":"xmqqwmqprav6.fsf@gitster.g","threadId":"61018","inReplyTo":"20240227150934.7950-3-randall.becker@nexbridge.ca","subject":"Re: [PATCH v2 2/3] builtin/receive-pack.c: change xwrite to write_in_full.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-27T19:00:13Z","receivedAt":"2024-02-27T19:00:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Randall S. Becker\" <the.n.e.key@gmail.com> writes:\n\n> From: \"Randall S. Becker\" <rsbecker@nexbridge.com>\n>\n> This change is required because some platforms do not support file writes of    arbitrary sizes (e.g, NonStop). xwrite ends up truncating the output to the\n> maximum single I/O size possible for the destination device.\n>\n> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>\n> ---\n>  builtin/receive-pack.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index db65607485..4277c63d08 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -456,7 +456,7 @@ static void report_message(const char *prefix, const char *err, va_list params)\n>  \tif (use_sideband)\n>  \t\tsend_sideband(1, 2, msg, sz, use_sideband);\n>  \telse\n> -\t\txwrite(2, msg, sz);\n> +\t\twrite_in_full(2, msg, sz);\n>  }\n\nThis change does make sense, as we can see a short write(2) from\nxwrite() and this caller is not repeating the call to flush the\nremainder after a short write.\n\n>  \n>  __attribute__((format (printf, 1, 2)))\n"},{"id":"489565","messageId":"03be01da69af$d8366e10$88a34a30$@nexbridge.com","threadId":"61018","inReplyTo":"xmqq1q8xspht.fsf@gitster.g","subject":"RE: [PATCH v2 3/3] builtin/unpack-objects.c: change xwrite to write_in_full avoid truncation.","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2024-02-27T19:04:46Z","receivedAt":"2024-02-27T19:05:00Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Tuesday, February 27, 2024 1:59 PM, Junio C Hamano wrote:\n>\"Randall S. Becker\" <the.n.e.key@gmail.com> writes:\n>\n>> From: \"Randall S. Becker\" <rsbecker@nexbridge.com>\n>>\n>> This change is required because some platforms do not support file\n>> writes of arbitrary sizes (e.g, NonStop). xwrite ends up truncating\n>> the output to the maximum single I/O size possible for the destination\n>> device if the supplied len value exceeds the supported value.\n>> Replacing xwrite with write_in_full corrects this problem. Future\n>> optimisations could remove the loop in favour of just calling\nwrite_in_full.\n>>\n>> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>\n>> ---\n>>  builtin/unpack-objects.c | 2 +-\n>>  1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c index\n>> e0a701f2b3..6935c4574e 100644\n>> --- a/builtin/unpack-objects.c\n>> +++ b/builtin/unpack-objects.c\n>> @@ -680,7 +680,7 @@ int cmd_unpack_objects(int argc, const char\n>> **argv, const char *prefix UNUSED)\n>>\n>>  \t/* Write the last part of the buffer to stdout */\n>>  \twhile (len) {\n>> -\t\tint ret = xwrite(1, buffer + offset, len);\n>> +\t\tint ret = write_in_full(1, buffer + offset, len);\n>>  \t\tif (ret <= 0)\n>>  \t\t\tbreak;\n>>  \t\tlen -= ret;\n>\n>Why do we need this with a retry loop that is prepared for short\n>write(2) specifically like this?\n>\n>If xwrite() calls underlying write(2) with too large a value, then your\nMAX_IO_SIZE is misconfigured, and the fix should go there, not\n>here in a loop that expects a working xwrite() that is allowed to return on\nshort write(2), I would think.\n\nI experimented with using write_in_full vs. keeping xwrite. With xwrite in\nthis loop, t7704.9 consistently fails as described in the other thread. With\nwrite_in_full, the code works correctly. I assume there are side-effects\nthat are present. This change is critical to having the code work on\nNonStop. Otherwise git seems to be at risk of actually being seriously\nbroken if unpack does not work correctly. I am happy to have my series\nignored as long as the problem is otherwise corrected.\n\n"},{"id":"489569","messageId":"20240227192530.GD3784114@coredump.intra.peff.net","threadId":"61018","inReplyTo":"03be01da69af$d8366e10$88a34a30$@nexbridge.com","subject":"Re: [PATCH v2 3/3] builtin/unpack-objects.c: change xwrite to write_in_full avoid truncation.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-27T19:25:30Z","receivedAt":"2024-02-27T19:25:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 27, 2024 at 02:04:46PM -0500, rsbecker@nexbridge.com wrote:\n\n> >> diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c index\n> >> e0a701f2b3..6935c4574e 100644\n> >> --- a/builtin/unpack-objects.c\n> >> +++ b/builtin/unpack-objects.c\n> >> @@ -680,7 +680,7 @@ int cmd_unpack_objects(int argc, const char\n> >> **argv, const char *prefix UNUSED)\n> >>\n> >>  \t/* Write the last part of the buffer to stdout */\n> >>  \twhile (len) {\n> >> -\t\tint ret = xwrite(1, buffer + offset, len);\n> >> +\t\tint ret = write_in_full(1, buffer + offset, len);\n> >>  \t\tif (ret <= 0)\n> >>  \t\t\tbreak;\n> >>  \t\tlen -= ret;\n> [...]\n> I experimented with using write_in_full vs. keeping xwrite. With xwrite in\n> this loop, t7704.9 consistently fails as described in the other thread. With\n> write_in_full, the code works correctly. I assume there are side-effects\n> that are present. This change is critical to having the code work on\n> NonStop. Otherwise git seems to be at risk of actually being seriously\n> broken if unpack does not work correctly. I am happy to have my series\n> ignored as long as the problem is otherwise corrected.\n\nI'm somewhat skeptical that this code is to blame, as it should be run\nvery rarely at all; it is just dumping any content in the pack stream\nafter the end of the checksum to stdout. But in normal use by Git, there\nis no such content in the first place.\n\nIf I do this:\n\ndiff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c\nindex e0a701f2b3..affe55035d 100644\n--- a/builtin/unpack-objects.c\n+++ b/builtin/unpack-objects.c\n@@ -680,11 +680,7 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix UNUSED)\n \n \t/* Write the last part of the buffer to stdout */\n \twhile (len) {\n-\t\tint ret = xwrite(1, buffer + offset, len);\n-\t\tif (ret <= 0)\n-\t\t\tbreak;\n-\t\tlen -= ret;\n-\t\toffset += ret;\n+\t\tBUG(\"cruft at the end of the pack!\");\n \t}\n \n \t/* All done */\n\nthen t7704 still passes, as it does not run this code at all. In fact,\nnothing in the test suite fails. Which is not to say we should get rid\nof those code. If we were writing today we might flag it as an error,\nbut we should keep it for historical compatibility.\n\nBut I do not see any bug in the code, and nor do I think it could\ncontribute to a test failure.\n\n-Peff\n"},{"id":"489575","messageId":"03d701da69c0$c3430e80$49c92b80$@nexbridge.com","threadId":"61018","inReplyTo":"20240227192530.GD3784114@coredump.intra.peff.net","subject":"RE: [PATCH v2 3/3] builtin/unpack-objects.c: change xwrite to write_in_full avoid truncation.","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2024-02-27T21:05:53Z","receivedAt":"2024-02-27T21:06:10Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Tuesday, February 27, 2024 2:26 PM, Peff wrote:\n>To: rsbecker@nexbridge.com\n>Cc: 'Junio C Hamano' <gitster@pobox.com>; 'Randall S. Becker' <the.n.e.key@gmail.com>; git@vger.kernel.org\n>Subject: Re: [PATCH v2 3/3] builtin/unpack-objects.c: change xwrite to write_in_full avoid truncation.\n>\n>On Tue, Feb 27, 2024 at 02:04:46PM -0500, rsbecker@nexbridge.com wrote:\n>\n>> >> diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c\n>> >> index e0a701f2b3..6935c4574e 100644\n>> >> --- a/builtin/unpack-objects.c\n>> >> +++ b/builtin/unpack-objects.c\n>> >> @@ -680,7 +680,7 @@ int cmd_unpack_objects(int argc, const char\n>> >> **argv, const char *prefix UNUSED)\n>> >>\n>> >>  \t/* Write the last part of the buffer to stdout */\n>> >>  \twhile (len) {\n>> >> -\t\tint ret = xwrite(1, buffer + offset, len);\n>> >> +\t\tint ret = write_in_full(1, buffer + offset, len);\n>> >>  \t\tif (ret <= 0)\n>> >>  \t\t\tbreak;\n>> >>  \t\tlen -= ret;\n>> [...]\n>> I experimented with using write_in_full vs. keeping xwrite. With\n>> xwrite in this loop, t7704.9 consistently fails as described in the\n>> other thread. With write_in_full, the code works correctly. I assume\n>> there are side-effects that are present. This change is critical to\n>> having the code work on NonStop. Otherwise git seems to be at risk of\n>> actually being seriously broken if unpack does not work correctly. I\n>> am happy to have my series ignored as long as the problem is otherwise corrected.\n>\n>I'm somewhat skeptical that this code is to blame, as it should be run very rarely at all; it is just dumping any content in the pack stream\n>after the end of the checksum to stdout. But in normal use by Git, there is no such content in the first place.\n>\n>If I do this:\n>\n>diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c index e0a701f2b3..affe55035d 100644\n>--- a/builtin/unpack-objects.c\n>+++ b/builtin/unpack-objects.c\n>@@ -680,11 +680,7 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix UNUSED)\n>\n> \t/* Write the last part of the buffer to stdout */\n> \twhile (len) {\n>-\t\tint ret = xwrite(1, buffer + offset, len);\n>-\t\tif (ret <= 0)\n>-\t\t\tbreak;\n>-\t\tlen -= ret;\n>-\t\toffset += ret;\n>+\t\tBUG(\"cruft at the end of the pack!\");\n> \t}\n>\n> \t/* All done */\n>\n>then t7704 still passes, as it does not run this code at all. In fact, nothing in the test suite fails. Which is not to say we should get rid of\n>those code. If we were writing today we might flag it as an error, but we should keep it for historical compatibility.\n>\n>But I do not see any bug in the code, and nor do I think it could contribute to a test failure.\n\nI have obviously gone down the wrong path trying to resolve this situation. Please consider this entire series dropped with my apologies for the time-waste.\n\nUnfortunately, I do not have sufficient knowledge of the code to resolve the originally reported problem without further assistance to determine the root case (assuming it still is a problem). Changes in master post-2.44.0 appear to have contributed to resolving the situation, so I am now getting random pass/fail on the test. I'm going to hold 2.44.0 on ia64 and wait for a subsequent release at retest at that time.\n\nSadly,\n--Randall\n\n"},{"id":"489577","messageId":"03d801da69c1$8b7339c0$a259ad40$@nexbridge.com","threadId":"61018","inReplyTo":"20240227150934.7950-1-randall.becker@nexbridge.ca","subject":"RE: [PATCH v2 0/2] Change xwrite() to write_in_full() in builtins.","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2024-02-27T21:11:29Z","receivedAt":"2024-02-27T21:11:36Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"Please withdraw this series.\n\n>-----Original Message-----\n>From: Randall S. Becker <the.n.e.key@gmail.com>\n>Sent: Tuesday, February 27, 2024 10:10 AM\n>To: git@vger.kernel.org\n>Cc: Randall S. Becker <rsbecker@nexbridge.com>\n>Subject: [PATCH v2 0/2] Change xwrite() to write_in_full() in builtins.\n>\n>From: \"Randall S. Becker\" <rsbecker@nexbridge.com>\n>\n>This series replaces xwrite to write_in_full in builtins/. The change is\nrequired to fix critical problems that prevent full writes to be\n>processed by on platforms where xwrite may be limited to a platform size\nlimit. Further changes outside of builtins/ may be required\n>but do not appear to be as urgent as this change, which causes test\nbreakage in t7704. A separate series will be contributed for\n>changes outside of builtins/ at a later date.\n>\n>The change in unpack-objects.c is necessary as len is being passed into\nxwrite that exceeds the size supported by the limit in that\n>method (56Kb on NonStop ia64).\n>\n>Randall S. Becker (3):\n>  builtin/repack.c: change xwrite to write_in_full and report errors.\n>  builtin/receive-pack.c: change xwrite to write_in_full.\n>  builtin/unpack-objects.c: change xwrite to write_in_full avoid\n>    truncation.\n>\n> builtin/receive-pack.c   | 2 +-\n> builtin/repack.c         | 9 +++++++--\n> builtin/unpack-objects.c | 2 +-\n> 3 files changed, 9 insertions(+), 4 deletions(-)\n>\n>--\n>2.42.1\n\n"},{"id":"490169","messageId":"20240307100018.GE2650063@coredump.intra.peff.net","threadId":"61018","inReplyTo":"03d701da69c0$c3430e80$49c92b80$@nexbridge.com","subject":"Re: [PATCH v2 3/3] builtin/unpack-objects.c: change xwrite to write_in_full avoid truncation.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-07T10:00:18Z","receivedAt":"2024-03-07T10:00:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 27, 2024 at 04:05:53PM -0500, rsbecker@nexbridge.com wrote:\n\n> Unfortunately, I do not have sufficient knowledge of the code to\n> resolve the originally reported problem without further assistance to\n> determine the root case (assuming it still is a problem). Changes in\n> master post-2.44.0 appear to have contributed to resolving the\n> situation, so I am now getting random pass/fail on the test. I'm going\n> to hold 2.44.0 on ia64 and wait for a subsequent release at retest at\n> that time.\n\nIf you're getting random pass/fail (which does seem like the kind of\nthing that could be related to pipe write() sizes), you might try using\nthe \"--stress\" argument. That can give you more consistent results while\nbisecting (e.g., if \"--stress\" runs successfully for a few minutes).\n\nThat said, given the failing test you mentioned, I kind of assume that\nit was not a code change that caused the problem, but rather a new test\nexercising new code that happens to tickle your race.\n\n-Peff\n"}]}