[PATCH v1 0/4] Change xwrite() to write_in_full() in builtins.

[PATCH v1 0/4] Change xwrite() to write_in_full() in builtins.

[Randall S. Becker]

From: "Randall S. Becker" <rsbecker@nexbridge.com>

This series replaces xwrite to write_in_full in builtins/. The change is
required to fix critical problems that prevent full writes to be
processed by on platforms where xwrite may be limited to a platform size
limit. Further changes outside of builtins/ may be required but do not
appear to be as urgent as this change, which causes test breakage in
t7704. A separate series will be contributed for changes outside of
builtins/ at a later date.

Randall S. Becker (4):
  builtin/index-pack.c: change xwrite to write_in_full to allow large
    sizes.
  builtin/receive-pack.c: change xwrite to write_in_full to allow large
    sizes.
  builtin/repack.c: change xwrite to write_in_full to allow large sizes.
  builtin/unpack-objects.c: change xwrite to write_in_full to allow
    large sizes.

 builtin/index-pack.c     | 2 +-
 builtin/receive-pack.c   | 5 +++--
 builtin/repack.c         | 9 +++++++--
 builtin/unpack-objects.c | 2 +-
 4 files changed, 12 insertions(+), 6 deletions(-)

-- 
2.42.1

[PATCH v1 1/4] builtin/index-pack.c: change xwrite to write_in_full to allow large sizes.

[Randall S. Becker]

From: "Randall S. Becker" <rsbecker@nexbridge.com>

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
maximum single I/O size possible for the destination device.

Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
---
 builtin/index-pack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index a3a37bd215..f80b8d101a 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1571,7 +1571,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
 		 * the last part of the input buffer to stdout.
 		 */
 		while (input_len) {
-			err = xwrite(1, input_buffer + input_offset, input_len);
+			err = write_in_full(1, input_buffer + input_offset, input_len);
 			if (err <= 0)
 				break;
 			input_len -= err;
-- 
2.42.1

[PATCH v1 2/4] builtin/receive-pack.c: change xwrite to write_in_full to allow large sizes.

[Randall S. Becker]

From: "Randall S. Becker" <rsbecker@nexbridge.com>

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
maximum single I/O size possible for the destination device.

Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
---
 builtin/receive-pack.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index db65607485..5064f3d300 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -455,8 +455,9 @@ static void report_message(const char *prefix, const char *err, va_list params)
 
 	if (use_sideband)
 		send_sideband(1, 2, msg, sz, use_sideband);
-	else
-		xwrite(2, msg, sz);
+	else {
+		write_in_full(2, msg, sz);
+	}
 }
 
 __attribute__((format (printf, 1, 2)))
-- 
2.42.1

[PATCH v1 3/4] builtin/repack.c: change xwrite to write_in_full to allow large sizes.

[Randall S. Becker]

From: "Randall S. Becker" <rsbecker@nexbridge.com>

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
maximum single I/O size possible for the destination device. The result of
write_in_full() is also passed to the caller, which was previously ignored.

Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
---
 builtin/repack.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index ede36328a3..932d24c60b 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -307,6 +307,7 @@ static int write_oid(const struct object_id *oid,
 		     struct packed_git *pack UNUSED,
 		     uint32_t pos UNUSED, void *data)
 {
+	int err;
 	struct child_process *cmd = data;
 
 	if (cmd->in == -1) {
@@ -314,8 +315,12 @@ static int write_oid(const struct object_id *oid,
 			die(_("could not start pack-objects to repack promisor objects"));
 	}
 
-	xwrite(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz);
-	xwrite(cmd->in, "\n", 1);
+	err = write_in_full(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz);
+	if (err <= 0)
+		return err;
+	err = write_in_full(cmd->in, "\n", 1);
+	if (err <= 0)
+		return err;
 	return 0;
 }
 
-- 
2.42.1

[PATCH v1 4/4] builtin/unpack-objects.c: change xwrite to write_in_full to allow large sizes.

[Randall S. Becker]

From: "Randall S. Becker" <rsbecker@nexbridge.com>

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
maximum single I/O size possible for the destination device.

Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
---
 builtin/unpack-objects.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index e0a701f2b3..6935c4574e 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -680,7 +680,7 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix UNUSED)
 
 	/* Write the last part of the buffer to stdout */
 	while (len) {
-		int ret = xwrite(1, buffer + offset, len);
+		int ret = write_in_full(1, buffer + offset, len);
 		if (ret <= 0)
 			break;
 		len -= ret;
-- 
2.42.1

Re: [PATCH v1 1/4] builtin/index-pack.c: change xwrite to write_in_full to allow large sizes.

[Taylor Blau]

On Mon, Feb 26, 2024 at 05:05:35PM -0500, Randall S. Becker wrote:
> From: "Randall S. Becker" <rsbecker@nexbridge.com>
>
> 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
> maximum single I/O size possible for the destination device.

Hmm. I'm not sure I understand what NonStop's behavior is here...

> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index a3a37bd215..f80b8d101a 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -1571,7 +1571,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
>  		 * the last part of the input buffer to stdout.
>  		 */
>  		while (input_len) {
> -			err = xwrite(1, input_buffer + input_offset, input_len);
> +			err = write_in_full(1, input_buffer + input_offset, input_len);
>  			if (err <= 0)
>  				break;
>  			input_len -= err;
> --
> 2.42.1

The code above loops while input_len is non-zero, and correctly
decrements it by the number of bytes written by xwrite() after each
iteration.

Assuming that xwrite()/write(2) works how I think it does on NonStop,
I'm not sure I understand why this change is necessary.

Thanks,
Taylor

RE: [PATCH v1 1/4] builtin/index-pack.c: change xwrite to write_in_full to allow large sizes.

[rsbecker]

On Monday, February 26, 2024 5:39 PM, Taylor Blau wrote:
>On Mon, Feb 26, 2024 at 05:05:35PM -0500, Randall S. Becker wrote:
>> From: "Randall S. Becker" <rsbecker@nexbridge.com>
>>
>> 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 maximum single I/O size possible for the destination device.
>
>Hmm. I'm not sure I understand what NonStop's behavior is here...
>
>> diff --git a/builtin/index-pack.c b/builtin/index-pack.c index
>> a3a37bd215..f80b8d101a 100644
>> --- a/builtin/index-pack.c
>> +++ b/builtin/index-pack.c
>> @@ -1571,7 +1571,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
>>  		 * the last part of the input buffer to stdout.
>>  		 */
>>  		while (input_len) {
>> -			err = xwrite(1, input_buffer + input_offset, input_len);
>> +			err = write_in_full(1, input_buffer + input_offset, input_len);
>>  			if (err <= 0)
>>  				break;
>>  			input_len -= err;
>> --
>> 2.42.1
>
>The code above loops while input_len is non-zero, and correctly decrements it by the number of bytes written by xwrite() after each
>iteration.
>
>Assuming that xwrite()/write(2) works how I think it does on NonStop, I'm not sure I understand why this change is necessary.

NonStop has a limited SSIZE_MAX. xwrite only handles that much so anything beyond that gets dropped (not in the above code but in other builtins); hence the critical nature of getting this fix out. This particular change probably could be tightened up on a re-roll to just call write_in_full instead of the while loop. I can fix that for v2. The goal suggested by Phillip W was to change xwrite to write_in_full, so I guess I went a little too far. 

RE: [PATCH v1 2/4] builtin/receive-pack.c: change xwrite to write_in_full to allow large sizes.

[rsbecker]

On Monday, February 26, 2024 5:06 PM, I wrote:
>From: "Randall S. Becker" <rsbecker@nexbridge.com>
>
>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 maximum single I/O size possible for the destination
device.
>
>Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
>---
> builtin/receive-pack.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
>diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index
db65607485..5064f3d300 100644
>--- a/builtin/receive-pack.c
>+++ b/builtin/receive-pack.c
>@@ -455,8 +455,9 @@ static void report_message(const char *prefix, const
char *err, va_list params)
>
> 	if (use_sideband)
> 		send_sideband(1, 2, msg, sz, use_sideband);
>-	else
>-		xwrite(2, msg, sz);
>+	else {
>+		write_in_full(2, msg, sz);
>+	}
> }
>
> __attribute__((format (printf, 1, 2)))
>--
>2.42.1

This needs to be fixed, so the {} after the else is removed. Will be in v2.

RE: [PATCH v1 1/4] builtin/index-pack.c: change xwrite to write_in_full to allow large sizes.

[rsbecker]

On Monday, February 26, 2024 5:39 PM, Taylor Blau wrote:
>On Mon, Feb 26, 2024 at 05:05:35PM -0500, Randall S. Becker wrote:
>> From: "Randall S. Becker" <rsbecker@nexbridge.com>
>>
>> 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 maximum single I/O size possible for the destination device.
>
>Hmm. I'm not sure I understand what NonStop's behavior is here...
>
>> diff --git a/builtin/index-pack.c b/builtin/index-pack.c index
>> a3a37bd215..f80b8d101a 100644
>> --- a/builtin/index-pack.c
>> +++ b/builtin/index-pack.c
>> @@ -1571,7 +1571,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
>>  		 * the last part of the input buffer to stdout.
>>  		 */
>>  		while (input_len) {
>> -			err = xwrite(1, input_buffer + input_offset, input_len);
>> +			err = write_in_full(1, input_buffer + input_offset, input_len);
>>  			if (err <= 0)
>>  				break;
>>  			input_len -= err;
>> --
>> 2.42.1
>
>The code above loops while input_len is non-zero, and correctly decrements it by the number of bytes written by xwrite() after each
>iteration.
>
>Assuming that xwrite()/write(2) works how I think it does on NonStop, I'm not sure I understand why this change is necessary.

After thinking about it, I'm going to revert the change in this file, so it will not be in v2. I'm a bit uncomfortable with having the write sizes in global, so will drop this bit.

Re: [PATCH v1 1/4] builtin/index-pack.c: change xwrite to write_in_full to allow large sizes.

[Junio C Hamano]

<rsbecker@nexbridge.com> writes:

>>The code above loops while input_len is non-zero, and correctly
>>decrements it by the number of bytes written by xwrite() after
>>each iteration.
>>
>>Assuming that xwrite()/write(2) works how I think it does on
>>NonStop, I'm not sure I understand why this change is necessary.
>
> NonStop has a limited SSIZE_MAX. xwrite only handles that much so
> anything beyond that gets dropped (not in the above code but in
> other builtins)

xwrite() caps a single write attempt to MAX_IO_SIZE and can return a
short-write, so anything beyound MAX_IO_SIZE will not even be sent
to the underlying write(2).  There is a heuristic based on the value
of SSIZE_MAX to define MAX_IO_SIZE in <git-compat-util.h>, and if
the value given by that heuristics is too large for your platform,
you can tweak your own MAX_IO_SIZE (see the comments in that header
file).

The caller of xwrite() must be prepared to see a write return with
value less than the length it used to call the function, either
because of this MAX_IO_SIZE cut-off, or because of the underlying
write(2) returning after a short write.  As long as the caller is
prepared, like Taylor pointed out, I am not sure why you'd need to
change it.

Re: [PATCH v1 2/4] builtin/receive-pack.c: change xwrite to write_in_full to allow large sizes.

[Junio C Hamano]

"Randall S. Becker" <the.n.e.key@gmail.com> writes:

> From: "Randall S. Becker" <rsbecker@nexbridge.com>
>
> 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
> maximum single I/O size possible for the destination device.

As msg[] here is 4k on-stack buffer, if the I/O size is small
enough, the above may happen, and I think write-in-full is warranted
here.  If your I/O must be done in 1k chunks, it would be very slow
to run things like writing a pack stream to clone any non-toy
projects, though X-<.

> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> ---
>  builtin/receive-pack.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index db65607485..5064f3d300 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -455,8 +455,9 @@ static void report_message(const char *prefix, const char *err, va_list params)
>  
>  	if (use_sideband)
>  		send_sideband(1, 2, msg, sz, use_sideband);
> -	else
> -		xwrite(2, msg, sz);
> +	else {
> +		write_in_full(2, msg, sz);
> +	}
>  }
>  
>  __attribute__((format (printf, 1, 2)))

Re: [PATCH v1 3/4] builtin/repack.c: change xwrite to write_in_full to allow large sizes.

[Junio C Hamano]

"Randall S. Becker" <the.n.e.key@gmail.com> writes:

> From: "Randall S. Becker" <rsbecker@nexbridge.com>
>
> 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
> maximum single I/O size possible for the destination device. The result of
> write_in_full() is also passed to the caller, which was previously ignored.

This one smells more like a theoretical issue than realistic, in
that these writes are done only with .hexsz (either 40 or 64) bytes
oid string, or a single byte "\n", for either of which it is hard to
imagine that it is even remotely close to platform "maximum single
I/O size".

But we'd need to look for the error return anyway, so switching to
write_in_full() while we are doing so is also good.

> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> ---
>  builtin/repack.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/repack.c b/builtin/repack.c
> index ede36328a3..932d24c60b 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -307,6 +307,7 @@ static int write_oid(const struct object_id *oid,
>  		     struct packed_git *pack UNUSED,
>  		     uint32_t pos UNUSED, void *data)
>  {
> +	int err;
>  	struct child_process *cmd = data;
>  
>  	if (cmd->in == -1) {
> @@ -314,8 +315,12 @@ static int write_oid(const struct object_id *oid,
>  			die(_("could not start pack-objects to repack promisor objects"));
>  	}
>  
> -	xwrite(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz);
> -	xwrite(cmd->in, "\n", 1);
> +	err = write_in_full(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz);
> +	if (err <= 0)
> +		return err;
> +	err = write_in_full(cmd->in, "\n", 1);
> +	if (err <= 0)
> +		return err;
>  	return 0;
>  }

Re: [PATCH v1 4/4] builtin/unpack-objects.c: change xwrite to write_in_full to allow large sizes.

[Junio C Hamano]

"Randall S. Becker" <the.n.e.key@gmail.com> writes:

> From: "Randall S. Becker" <rsbecker@nexbridge.com>
>
> 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
> maximum single I/O size possible for the destination device.
>
> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> ---
>  builtin/unpack-objects.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

The same comment as [1/4].  Perhaps your MAX_IO_SIZE should be tuned
downwards, so that xwrite() works as it was designed to work.

> diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
> index e0a701f2b3..6935c4574e 100644
> --- a/builtin/unpack-objects.c
> +++ b/builtin/unpack-objects.c
> @@ -680,7 +680,7 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix UNUSED)
>  
>  	/* Write the last part of the buffer to stdout */
>  	while (len) {
> -		int ret = xwrite(1, buffer + offset, len);
> +		int ret = write_in_full(1, buffer + offset, len);
>  		if (ret <= 0)
>  			break;
>  		len -= ret;

RE: [PATCH v1 1/4] builtin/index-pack.c: change xwrite to write_in_full to allow large sizes.

[rsbecker]

On Monday, February 26, 2024 6:47 PM, Junio C Hamano wrote:
><rsbecker@nexbridge.com> writes:
>
>>>The code above loops while input_len is non-zero, and correctly
>>>decrements it by the number of bytes written by xwrite() after each
>>>iteration.
>>>
>>>Assuming that xwrite()/write(2) works how I think it does on NonStop,
>>>I'm not sure I understand why this change is necessary.
>>
>> NonStop has a limited SSIZE_MAX. xwrite only handles that much so
>> anything beyond that gets dropped (not in the above code but in other
>> builtins)
>
>xwrite() caps a single write attempt to MAX_IO_SIZE and can return a
short-write, so anything beyound MAX_IO_SIZE will not even be
>sent to the underlying write(2).  There is a heuristic based on the value
of SSIZE_MAX to define MAX_IO_SIZE in <git-compat-util.h>,
>and if the value given by that heuristics is too large for your platform,
you can tweak your own MAX_IO_SIZE (see the comments in
>that header file).
>
>The caller of xwrite() must be prepared to see a write return with value
less than the length it used to call the function, either because
>of this MAX_IO_SIZE cut-off, or because of the underlying
>write(2) returning after a short write.  As long as the caller is prepared,
like Taylor pointed out, I am not sure why you'd need to change
>it.

I understand. I was involved in xwrite() a few years ago. The problem is
that users of xwrite() did not account for that and t7704.9 failed as a
result. These changes did fix the issue. I am not sure how to proceed based
on the above, however. Continue or recode the callers (which is part of what
this does)?

RE: [PATCH v1 2/4] builtin/receive-pack.c: change xwrite to write_in_full to allow large sizes.

[rsbecker]

On Monday, February 26, 2024 6:51 PM, Junio C Hamano wrote:
>"Randall S. Becker" <the.n.e.key@gmail.com> writes:
>
>> From: "Randall S. Becker" <rsbecker@nexbridge.com>
>>
>> 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 maximum single I/O size possible for the destination
device.
>
>As msg[] here is 4k on-stack buffer, if the I/O size is small enough, the
above may happen, and I think write-in-full is warranted here.  If
>your I/O must be done in 1k chunks, it would be very slow to run things
like writing a pack stream to clone any non-toy projects,
>though X-<.

On the x86 platform, we get a size large enough not to trigger the failure
in t7704. However, on ia64, the limit is 56Kb, which apparently does. I'm
hoping no one else has a 1Kb limit - although some TCP stacks might
experience it. Either way, truncating a package is bad. Fortunately the I/O
subsystem on NonStop is very fast (basically DMA) between process memory
space.

>
>> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
>> ---
>>  builtin/receive-pack.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index
>> db65607485..5064f3d300 100644
>> --- a/builtin/receive-pack.c
>> +++ b/builtin/receive-pack.c
>> @@ -455,8 +455,9 @@ static void report_message(const char *prefix,
>> const char *err, va_list params)
>>
>>  	if (use_sideband)
>>  		send_sideband(1, 2, msg, sz, use_sideband);
>> -	else
>> -		xwrite(2, msg, sz);
>> +	else {
>> +		write_in_full(2, msg, sz);
>> +	}
>>  }
>>
>>  __attribute__((format (printf, 1, 2)))

RE: [PATCH v1 4/4] builtin/unpack-objects.c: change xwrite to write_in_full to allow large sizes.

[rsbecker]

On Monday, February 26, 2024 6:56 PM, Junio C Hamano wrote:
>"Randall S. Becker" <the.n.e.key@gmail.com> writes:
>
>> From: "Randall S. Becker" <rsbecker@nexbridge.com>
>>
>> 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 maximum single I/O size possible for the destination
device.
>>
>> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
>> ---
>>  builtin/unpack-objects.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>
>The same comment as [1/4].  Perhaps your MAX_IO_SIZE should be tuned
downwards, so that xwrite() works as it was designed to
>work.

I am considering undoing this one, other than ensuring that the error code
is checked and returned. The MAX_IO_SIZE is sufficient. I think the actual
fail was in the original repack.c not this one.

>
>> diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c index
>> e0a701f2b3..6935c4574e 100644
>> --- a/builtin/unpack-objects.c
>> +++ b/builtin/unpack-objects.c
>> @@ -680,7 +680,7 @@ int cmd_unpack_objects(int argc, const char
>> **argv, const char *prefix UNUSED)
>>
>>  	/* Write the last part of the buffer to stdout */
>>  	while (len) {
>> -		int ret = xwrite(1, buffer + offset, len);
>> +		int ret = write_in_full(1, buffer + offset, len);
>>  		if (ret <= 0)
>>  			break;
>>  		len -= ret;

Re: [PATCH v1 3/4] builtin/repack.c: change xwrite to write_in_full to allow large sizes.

[Jeff King]

On Mon, Feb 26, 2024 at 05:05:37PM -0500, Randall S. Becker wrote:

> From: "Randall S. Becker" <rsbecker@nexbridge.com>
> 
> 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
> maximum single I/O size possible for the destination device. The result of
> write_in_full() is also passed to the caller, which was previously ignored.

These are going to be tiny compared to single-write() I/O limits, I'd
think, but in general we should be on guard for the OS returning short
reads (this is a pipe and so for most systems PIPE_BUF would guarantee
atomicity, I think, but IMHO it is simpler to just make things
obviously-correct by looping with write_in_full). So I'd be surprised if
this spot was the cause of a visible bug, but I think it's worth
changing regardless.

The error detection is a separate question, though. I think it is good
to check the result of the write here, as an error here means that the
child pack-objects misses some objects we wanted it to pack, which could
lead to a corrupt repository. But I don't think what you have here is
quite enough:

> @@ -314,8 +315,12 @@ static int write_oid(const struct object_id *oid,
>  			die(_("could not start pack-objects to repack promisor objects"));
>  	}
>  
> -	xwrite(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz);
> -	xwrite(cmd->in, "\n", 1);
> +	err = write_in_full(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz);
> +	if (err <= 0)
> +		return err;
> +	err = write_in_full(cmd->in, "\n", 1);
> +	if (err <= 0)
> +		return err;
>  	return 0;

OK, so we detect the error and return it to the caller. Who is the
caller? The only use of this function is in repack_promisor_objects(),
which calls:

        for_each_packed_object(write_oid, &cmd,
                               FOR_EACH_OBJECT_PROMISOR_ONLY);

So when we return the error, now for_each_packed_object() will stop
traversing, and propagate that error up to the caller. But as we can see
above, the caller ignores it!

So I think you'd either want to die directly (perhaps using
write_or_die). Or you'd need to additionally check the return from
for_each_packed_object(). That would also catch cases where that
function failed to open a pack (I'm not sure how important that is to
this code).

But as it is, your patch just causes a write error to truncate the list
of oids send to the child process (though that is probably not
materially different from the current behavior, as the subsequent calls
would presumably fail, too).

-Peff

Re: [PATCH v1 3/4] builtin/repack.c: change xwrite to write_in_full to allow large sizes.

[Jeff King]

On Tue, Feb 27, 2024 at 03:20:27AM -0500, Jeff King wrote:

> OK, so we detect the error and return it to the caller. Who is the
> caller? The only use of this function is in repack_promisor_objects(),
> which calls:
> 
>         for_each_packed_object(write_oid, &cmd,
>                                FOR_EACH_OBJECT_PROMISOR_ONLY);
> 
> So when we return the error, now for_each_packed_object() will stop
> traversing, and propagate that error up to the caller. But as we can see
> above, the caller ignores it!

Oh, one other thing I meant to mention: as the test failure you saw was
related to repacking, this seemed like a likely culprit. But the code is
only triggered when repacking promisor objects in a partial clone, and
it didn't look like the test you posted covered that (it was just about
cruft packs). So I would not expect this code to be run at all in the
failing test you saw.

-Peff