Git 2.54.0-rc1, subtests of t5310, t5326, t5327

Git 2.54.0-rc1, subtests of t5310, t5326, t5327

[rsbecker]

I can getting numerous issues in t5310, t5326, t2527 relating to the
following use of --git-dir:

In t5310:
fatal: not a git repository: 'clone.git'
not ok 55 - fetch (full bitmap)
#
#                       git --git-dir=clone.git fetch origin second:second
&&
#                       git rev-parse HEAD >expect &&
#                       git --git-dir=clone.git rev-parse HEAD >actual &&
#                       test_cmp expect actual
#

In t5326 and t5327:
fatal: writev error: Invalid function argument
fetch-pack: unexpected disconnect while reading sideband packet
fatal: early EOF
fatal: fetch-pack: invalid index-pack output
not ok 24 - clone from bitmapped repository
#
#                       rm -fr clone.git &&
#                       git clone --no-local --bare . clone.git &&
#                       git rev-parse HEAD >expect &&
#                       git --git-dir=clone.git rev-parse HEAD >actual &&
#                       test_cmp expect actual
#


It appears that the --git-dir argument is not working properly in this
release. Any opinions?

I would have expected HOME or .gitconfig to have been set or redirected
elsewhere in the test.

--Randall


--
Brief whoami: NonStop&UNIX developer since approximately
UNIX(421664400)
NonStop(211288444200000000)
-- In real life, I talk too much.

Re: Git 2.54.0-rc1, subtests of t5310, t5326, t5327

[Jeff King]

On Tue, Apr 07, 2026 at 07:29:50PM -0400, rsbecker@nexbridge.com wrote:

> I can getting numerous issues in t5310, t5326, t2527 relating to the
> following use of --git-dir:
> 
> In t5310:
> fatal: not a git repository: 'clone.git'
> not ok 55 - fetch (full bitmap)
> #
> #                       git --git-dir=clone.git fetch origin second:second
> &&
> #                       git rev-parse HEAD >expect &&
> #                       git --git-dir=clone.git rev-parse HEAD >actual &&
> #                       test_cmp expect actual
> #

This test hasn't changed recently. The clone.git directory should have
been created by an earlier test. Can you try running with "-i" and make
sure that this is the first failing test, and we didn't fail earlier?

Especially because...

> In t5326 and t5327:
> fatal: writev error: Invalid function argument
> fetch-pack: unexpected disconnect while reading sideband packet
> fatal: early EOF
> fatal: fetch-pack: invalid index-pack output
> not ok 24 - clone from bitmapped repository
> #
> #                       rm -fr clone.git &&
> #                       git clone --no-local --bare . clone.git &&
> #                       git rev-parse HEAD >expect &&
> #                       git --git-dir=clone.git rev-parse HEAD >actual &&
> #                       test_cmp expect actual
> #

...it looks less like --git-dir is a problem here, and more like the
introduction of writev() is. It is now used in sideband_send(), so it
seems plausible that a similar failure might have broken the git-clone
operation that the other test was using to create clone.git.

As for why writev() is failing, I don't know. If it were totally broken
on your system I'd expect almost everything to be failing. But maybe try
building with "make NO_WRITEV=Nope" and see if that makes the problems
go away? The compat implementation just does a series of write() calls,
which is what send_sideband() was doing before.

-Peff

RE: Git 2.54.0-rc1, subtests of t5310, t5326, t5327

[rsbecker]

On April 8, 2026 12:17 AM, Jeff King wrote:
>On Tue, Apr 07, 2026 at 07:29:50PM -0400, rsbecker@nexbridge.com wrote:
>
>> I can getting numerous issues in t5310, t5326, t2527 relating to the
>> following use of --git-dir:
>>
>> In t5310:
>> fatal: not a git repository: 'clone.git'
>> not ok 55 - fetch (full bitmap)
>> #
>> #                       git --git-dir=clone.git fetch origin second:second
>> &&
>> #                       git rev-parse HEAD >expect &&
>> #                       git --git-dir=clone.git rev-parse HEAD >actual &&
>> #                       test_cmp expect actual
>> #
>
>This test hasn't changed recently. The clone.git directory should have been created
>by an earlier test. Can you try running with "-i" and make sure that this is the first
>failing test, and we didn't fail earlier?
>
>Especially because...
>
>> In t5326 and t5327:
>> fatal: writev error: Invalid function argument
>> fetch-pack: unexpected disconnect while reading sideband packet
>> fatal: early EOF
>> fatal: fetch-pack: invalid index-pack output not ok 24 - clone from
>> bitmapped repository #
>> #                       rm -fr clone.git &&
>> #                       git clone --no-local --bare . clone.git &&
>> #                       git rev-parse HEAD >expect &&
>> #                       git --git-dir=clone.git rev-parse HEAD >actual &&
>> #                       test_cmp expect actual
>> #
>
>...it looks less like --git-dir is a problem here, and more like the introduction of
>writev() is. It is now used in sideband_send(), so it seems plausible that a similar
>failure might have broken the git-clone operation that the other test was using to
>create clone.git.
>
>As for why writev() is failing, I don't know. If it were totally broken on your system
>I'd expect almost everything to be failing. But maybe try building with "make
>NO_WRITEV=Nope" and see if that makes the problems go away? The compat
>implementation just does a series of write() calls, which is what send_sideband()
>was doing before.

First fail is as follows in subtest 25:

expecting success of 5310.25 'clone from bitmapped repository':
                rm -fr clone.git &&
                git clone --no-local --bare . clone.git &&
                git rev-parse HEAD >expect &&
                git --git-dir=clone.git rev-parse HEAD >actual &&
                test_cmp expect actual

Cloning into bare repository 'clone.git'...
remote: Enumerating objects: 629, done.
fatal: writev error: Invalid function argument
fetch-pack: unexpected disconnect while reading sideband packet
fatal: early EOF
fatal: fetch-pack: invalid index-pack output
not ok 25 - clone from bitmapped repository

I think the invalid function argument maybe an ioctl or socketioctl not supported for the file type.

RE: Git 2.54.0-rc1, subtests of t5310, t5326, t5327

[rsbecker]

On April 8, 2026 10:54 AM, I wrote:
>To: 'Jeff King' <peff@peff.net>
>Cc: git@vger.kernel.org
>Subject: RE: Git 2.54.0-rc1, subtests of t5310, t5326, t5327
>
>On April 8, 2026 12:17 AM, Jeff King wrote:
>>On Tue, Apr 07, 2026 at 07:29:50PM -0400, rsbecker@nexbridge.com wrote:
>>
>>> I can getting numerous issues in t5310, t5326, t2527 relating to the
>>> following use of --git-dir:
>>>
>>> In t5310:
>>> fatal: not a git repository: 'clone.git'
>>> not ok 55 - fetch (full bitmap)
>>> #
>>> #                       git --git-dir=clone.git fetch origin second:second
>>> &&
>>> #                       git rev-parse HEAD >expect &&
>>> #                       git --git-dir=clone.git rev-parse HEAD >actual &&
>>> #                       test_cmp expect actual
>>> #
>>
>>This test hasn't changed recently. The clone.git directory should have been created
>>by an earlier test. Can you try running with "-i" and make sure that this is the first
>>failing test, and we didn't fail earlier?
>>
>>Especially because...
>>
>>> In t5326 and t5327:
>>> fatal: writev error: Invalid function argument
>>> fetch-pack: unexpected disconnect while reading sideband packet
>>> fatal: early EOF
>>> fatal: fetch-pack: invalid index-pack output not ok 24 - clone from
>>> bitmapped repository #
>>> #                       rm -fr clone.git &&
>>> #                       git clone --no-local --bare . clone.git &&
>>> #                       git rev-parse HEAD >expect &&
>>> #                       git --git-dir=clone.git rev-parse HEAD >actual &&
>>> #                       test_cmp expect actual
>>> #
>>
>>...it looks less like --git-dir is a problem here, and more like the introduction of
>>writev() is. It is now used in sideband_send(), so it seems plausible that a similar
>>failure might have broken the git-clone operation that the other test was using to
>>create clone.git.
>>
>>As for why writev() is failing, I don't know. If it were totally broken on your system
>>I'd expect almost everything to be failing. But maybe try building with "make
>>NO_WRITEV=Nope" and see if that makes the problems go away? The compat
>>implementation just does a series of write() calls, which is what send_sideband()
>>was doing before.
>
>First fail is as follows in subtest 25:
>
>expecting success of 5310.25 'clone from bitmapped repository':
>                rm -fr clone.git &&
>                git clone --no-local --bare . clone.git &&
>                git rev-parse HEAD >expect &&
>                git --git-dir=clone.git rev-parse HEAD >actual &&
>                test_cmp expect actual
>
>Cloning into bare repository 'clone.git'...
>remote: Enumerating objects: 629, done.
>fatal: writev error: Invalid function argument
>fetch-pack: unexpected disconnect while reading sideband packet
>fatal: early EOF
>fatal: fetch-pack: invalid index-pack output
>not ok 25 - clone from bitmapped repository
>
>I think the invalid function argument maybe an ioctl or socketioctl not supported for
>the file type.

This is also impacting t5608 and t7700. Anywhere where writev() is used, seemingly. We went through
MAX_IO_SIZE issues years ago, instead of using ssize_t as a basis of how big communication is. I think
writev() is not valid. It worked on Lunix, but had issues elsewhere. This broke the compat layer.

Re: Git 2.54.0-rc1, subtests of t5310, t5326, t5327

[Jeff King]

On Wed, Apr 08, 2026 at 10:54:00AM -0400, rsbecker@nexbridge.com wrote:

> First fail is as follows in subtest 25:
> 
> expecting success of 5310.25 'clone from bitmapped repository':
>                 rm -fr clone.git &&
>                 git clone --no-local --bare . clone.git &&
>                 git rev-parse HEAD >expect &&
>                 git --git-dir=clone.git rev-parse HEAD >actual &&
>                 test_cmp expect actual
> 
> Cloning into bare repository 'clone.git'...
> remote: Enumerating objects: 629, done.
> fatal: writev error: Invalid function argument
> fetch-pack: unexpected disconnect while reading sideband packet
> fatal: early EOF
> fatal: fetch-pack: invalid index-pack output
> not ok 25 - clone from bitmapped repository

OK, good. Well, not good, but at least absolves --git-dir, and we know
the problem is just writev() everywhere.

> I think the invalid function argument maybe an ioctl or socketioctl
> not supported for the file type.

Yeah, this is weird. We are just calling writev() here, and not trying
to do anything exotic. I could believe that writev() isn't supported on
certain descriptor types or something, but this should just be a pipe.
But why does it consistently fail here, but not in every clone? Weird.

It might be interesting to use strace or a debugger (or just printf from
writev_or_die()) to see if there's something interesting about the
arguments here. But finding the root cause may not actually be that
helpful (or at least not worth the effort).

Does building with NO_WRITEV make the problem go away?

-Peff

Re: Git 2.54.0-rc1, subtests of t5310, t5326, t5327

[Jeff King]

On Wed, Apr 08, 2026 at 12:25:47PM -0400, rsbecker@nexbridge.com wrote:

> This is also impacting t5608 and t7700. Anywhere where writev() is
> used, seemingly. We went through MAX_IO_SIZE issues years ago, instead
> of using ssize_t as a basis of how big communication is. I think
> writev() is not valid. It worked on Lunix, but had issues elsewhere.
> This broke the compat layer.

I wondered briefly if the problem could be that we're violating
MAX_IO_SIZE here, as our use of writev() does not respect it at all. But
the only spot that uses it is feeding pkt-line packets, which max out at
64k. So unless your MAX_IO_SIZE is smaller than that, I doubt that is
the problem.

-Peff

Re: Git 2.54.0-rc1, subtests of t5310, t5326, t5327

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Apr 08, 2026 at 12:25:47PM -0400, rsbecker@nexbridge.com wrote:
>
>> This is also impacting t5608 and t7700. Anywhere where writev() is
>> used, seemingly. We went through MAX_IO_SIZE issues years ago, instead
>> of using ssize_t as a basis of how big communication is. I think
>> writev() is not valid. It worked on Lunix, but had issues elsewhere.
>> This broke the compat layer.
>
> I wondered briefly if the problem could be that we're violating
> MAX_IO_SIZE here, as our use of writev() does not respect it at all. But
> the only spot that uses it is feeding pkt-line packets, which max out at
> 64k. So unless your MAX_IO_SIZE is smaller than that, I doubt that is
> the problem.

Good point.  The original did not use write(2) directly but used
write_or_die(), that is write_in_full(), that loops over xwrite(),
so it would have worked even with a lot lower MAX_IO_SIZE limit.

According to man7.org, writev() is allowed to transfer fewer bytes
than requested, so our use of writev() may have to be a bit more
careful, though.

RE: Git 2.54.0-rc1, subtests of t5310, t5326, t5327

[rsbecker]

On April 8, 2026 1:40 PM, Jeff King wrote:
>On Wed, Apr 08, 2026 at 12:25:47PM -0400, rsbecker@nexbridge.com wrote:
>
>> This is also impacting t5608 and t7700. Anywhere where writev() is
>> used, seemingly. We went through MAX_IO_SIZE issues years ago, instead
>> of using ssize_t as a basis of how big communication is. I think
>> writev() is not valid. It worked on Lunix, but had issues elsewhere.
>> This broke the compat layer.
>
>I wondered briefly if the problem could be that we're violating MAX_IO_SIZE here,
>as our use of writev() does not respect it at all. But the only spot that uses it is
>feeding pkt-line packets, which max out at 64k. So unless your MAX_IO_SIZE is
>smaller than that, I doubt that is the problem.

SSIZE_MAX on platform is 53248, so yes. We expected git-compat-util.h at line
696 to be honoured.

RE: Git 2.54.0-rc1, subtests of t5310, t5326, t5327

[rsbecker]

On April 8, 2026 2:13 PM, Junio C Hamano wrote:
>Jeff King <peff@peff.net> writes:
>
>> On Wed, Apr 08, 2026 at 12:25:47PM -0400, rsbecker@nexbridge.com wrote:
>>
>>> This is also impacting t5608 and t7700. Anywhere where writev() is
>>> used, seemingly. We went through MAX_IO_SIZE issues years ago,
>>> instead of using ssize_t as a basis of how big communication is. I
>>> think
>>> writev() is not valid. It worked on Lunix, but had issues elsewhere.
>>> This broke the compat layer.
>>
>> I wondered briefly if the problem could be that we're violating
>> MAX_IO_SIZE here, as our use of writev() does not respect it at all.
>> But the only spot that uses it is feeding pkt-line packets, which max
>> out at 64k. So unless your MAX_IO_SIZE is smaller than that, I doubt
>> that is the problem.
>
>Good point.  The original did not use write(2) directly but used
write_or_die(), that
>is write_in_full(), that loops over xwrite(), so it would have worked even
with a lot
>lower MAX_IO_SIZE limit.
>
>According to man7.org, writev() is allowed to transfer fewer bytes than
requested,
>so our use of writev() may have to be a bit more careful, though.

On my box, I have the following note:

Specifying  the sum of the iov_len values in the iov array greater than
the OSS I/O size limit for that open causes the  writev()  function  to
return  -1  and  set errno to [EINVAL].

This can be either 52K (SSIZE_MAX) or 868K depending on the build settings.
We are currently stuck with 32-bit builds, with large file support, so
really no
limit on the file sizes, but due to dependencies that are stuck at 32-bit
until I
convince the powers that be to fix those. Please, can we be compliant with
MAX_IO_SIZE?

Re: Git 2.54.0-rc1, subtests of t5310, t5326, t5327

[Junio C Hamano]

<rsbecker@nexbridge.com> writes:

> On my box, I have the following note:
>
> Specifying  the sum of the iov_len values in the iov array greater than
> the OSS I/O size limit for that open causes the  writev()  function  to
> return  -1  and  set errno to [EINVAL].

That is unexpected.

writev() may fail if the sum of iov_len would not fit within ssize_t
with EINVAL, but unless your "the OSS I/O size limit" is the same as
SSIZE_MAX, what you have above is not quite the same.

Does your build work with NO_WRITEV=Nope?  I think I saw it asked a
few times but I do not recall seeing it answered.  At least we know
xwrite() seems to work well enough on your system, which is what the
writev() emulation is written in terms of, so I suspect it would.

RE: Git 2.54.0-rc1, subtests of t5310, t5326, t5327

[rsbecker]

On April 8, 2026 4:22 PM, Junio C Hamano wrote:
><rsbecker@nexbridge.com> writes:
>
>> On my box, I have the following note:
>>
>> Specifying  the sum of the iov_len values in the iov array greater
>> than the OSS I/O size limit for that open causes the  writev()
>> function  to return  -1  and  set errno to [EINVAL].
>
>That is unexpected.
>
>writev() may fail if the sum of iov_len would not fit within ssize_t with
EINVAL, but
>unless your "the OSS I/O size limit" is the same as SSIZE_MAX, what you
have above
>is not quite the same.
>
>Does your build work with NO_WRITEV=Nope?  I think I saw it asked a few
times
>but I do not recall seeing it answered.  At least we know
>xwrite() seems to work well enough on your system, which is what the
>writev() emulation is written in terms of, so I suspect it would.

Yes, NO_WRITEV=Nope does compile and execute. I am including it
in our CI/CD job for now. Can we plan on a fix for this?

Re: Git 2.54.0-rc1, subtests of t5310, t5326, t5327

[Junio C Hamano]

<rsbecker@nexbridge.com> writes:

> On April 8, 2026 4:22 PM, Junio C Hamano wrote:
>><rsbecker@nexbridge.com> writes:
>>
>>> On my box, I have the following note:
>>>
>>> Specifying  the sum of the iov_len values in the iov array greater
>>> than the OSS I/O size limit for that open causes the  writev()
>>> function  to return  -1  and  set errno to [EINVAL].
>>
>>That is unexpected.
>>
>>writev() may fail if the sum of iov_len would not fit within ssize_t with
> EINVAL, but
>>unless your "the OSS I/O size limit" is the same as SSIZE_MAX, what you
> have above
>>is not quite the same.
>>
>>Does your build work with NO_WRITEV=Nope?  I think I saw it asked a few
> times
>>but I do not recall seeing it answered.  At least we know
>>xwrite() seems to work well enough on your system, which is what the
>>writev() emulation is written in terms of, so I suspect it would.
>
> Yes, NO_WRITEV=Nope does compile and execute. I am including it
> in our CI/CD job for now. Can we plan on a fix for this?

What I have heard so far indicate that the code that uses writev()
would need to loop over to prepare for short writes, but your
writev() that fails for "the OSS I/O size limit" (whatever it is)
does not sound like something we want to change the callers to chomp
the writev() calls into smaller chunks for.  Such a platform is far
better off using the compat/writev for the code path we recently
started using writev() in.

To be quite honest, I am not sure if it is even worth using writev()
if we need a loop that protects against shrot writes, so unless I am
grossly mistaken (e.g., perhaps there is some guarantee that there
won't be any short writes for writev() that sends data smaller than
64k that I missed in the docs), the best course of action might be
to revert the change to use writev() and use the two write(2)s as
before, *if* we actually observe that the current code is broken by
short writes.

RE: Git 2.54.0-rc1, subtests of t5310, t5326, t5327

[rsbecker]

On April 8, 2026 5:43 PM, Junio C Hamano wrote:
><rsbecker@nexbridge.com> writes:
>
>> On April 8, 2026 4:22 PM, Junio C Hamano wrote:
>>><rsbecker@nexbridge.com> writes:
>>>
>>>> On my box, I have the following note:
>>>>
>>>> Specifying  the sum of the iov_len values in the iov array greater
>>>> than the OSS I/O size limit for that open causes the  writev()
>>>> function  to return  -1  and  set errno to [EINVAL].
>>>
>>>That is unexpected.
>>>
>>>writev() may fail if the sum of iov_len would not fit within ssize_t
>>>with
>> EINVAL, but
>>>unless your "the OSS I/O size limit" is the same as SSIZE_MAX, what
>>>you
>> have above
>>>is not quite the same.
>>>
>>>Does your build work with NO_WRITEV=Nope?  I think I saw it asked a
>>>few
>> times
>>>but I do not recall seeing it answered.  At least we know
>>>xwrite() seems to work well enough on your system, which is what the
>>>writev() emulation is written in terms of, so I suspect it would.
>>
>> Yes, NO_WRITEV=Nope does compile and execute. I am including it in our
>> CI/CD job for now. Can we plan on a fix for this?
>
>What I have heard so far indicate that the code that uses writev() would
need to
>loop over to prepare for short writes, but your
>writev() that fails for "the OSS I/O size limit" (whatever it is) does not
sound like
>something we want to change the callers to chomp the writev() calls into
smaller
>chunks for.  Such a platform is far better off using the compat/writev for
the code
>path we recently started using writev() in.
>
>To be quite honest, I am not sure if it is even worth using writev() if we
need a loop
>that protects against shrot writes, so unless I am grossly mistaken (e.g.,
perhaps
>there is some guarantee that there won't be any short writes for writev()
that sends
>data smaller than 64k that I missed in the docs), the best course of action
might be
>to revert the change to use writev() and use the two write(2)s as before,
*if* we
>actually observe that the current code is broken by short writes.

I am 100% sure that EINVAL is returned by writev() on NonStop if the size
exceeds 52K
In 32-bit models. Whether it supports 868K for 64-bit is conjecture.
NO_WRITEV=Nope
works, which I am trying for everything at RC1, then we can live with this.

Re: Git 2.54.0-rc1, subtests of t5310, t5326, t5327

[Jeff King]

On Wed, Apr 08, 2026 at 02:36:27PM -0400, rsbecker@nexbridge.com wrote:

> On April 8, 2026 1:40 PM, Jeff King wrote:
> >On Wed, Apr 08, 2026 at 12:25:47PM -0400, rsbecker@nexbridge.com wrote:
> >
> >> This is also impacting t5608 and t7700. Anywhere where writev() is
> >> used, seemingly. We went through MAX_IO_SIZE issues years ago, instead
> >> of using ssize_t as a basis of how big communication is. I think
> >> writev() is not valid. It worked on Lunix, but had issues elsewhere.
> >> This broke the compat layer.
> >
> >I wondered briefly if the problem could be that we're violating MAX_IO_SIZE here,
> >as our use of writev() does not respect it at all. But the only spot that uses it is
> >feeding pkt-line packets, which max out at 64k. So unless your MAX_IO_SIZE is
> >smaller than that, I doubt that is the problem.
> 
> SSIZE_MAX on platform is 53248, so yes. We expected git-compat-util.h at line
> 696 to be honoured.

Oof, that is small. So yeah, that is almost certainly the problem (and
explains why it only happens for some writes in the test suite).

-Peff

Re: Git 2.54.0-rc1, subtests of t5310, t5326, t5327

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> To be quite honest, I am not sure if it is even worth using writev()
> if we need a loop that protects against shrot writes, so unless I am
> grossly mistaken (e.g., perhaps there is some guarantee that there
> won't be any short writes for writev() that sends data smaller than
> 64k that I missed in the docs), the best course of action might be
> to revert the change to use writev() and use the two write(2)s as
> before, *if* we actually observe that the current code is broken by
> short writes.

Ah, sorry, I should have double checked the actual code.  We already
use a looping writev_in_full() that wraps writev(), so there is
nothing extra that we still need to do to prepare for short writes.

Unfortunately, comparing write_in_full() vs writev_in_full(), there
is nothing that corresponds to xwrite() that can be used to hide the
short writes and chomps an originally larger I/O into smaller
pieces.  Unlike write() that we may receive a single linear large
sequence of bytes, which we can choose to chomp into artificially
smaller pieces and write them out (up to 8MB by default), writev()
API lets the caller to prepare chunks of memory and I do not think
there is a good way for the writev_in_full() at the lower layer to
chomp these into smaller pieces, and even if we could, that would
defeat the whole reason why we rewrote the original code that used
write_in_full() into using writev(), i.e., to avoid extra allocation
(and extra system calls---but if your I/O layer is limited to very
small writes, no matter how we chop it, you will have to issue extra
system calls to flush all of the data out).

So, I dunno.

Re: Git 2.54.0-rc1, subtests of t5310, t5326, t5327

[Jeff King]

On Wed, Apr 08, 2026 at 02:43:20PM -0700, Junio C Hamano wrote:

> > Yes, NO_WRITEV=Nope does compile and execute. I am including it
> > in our CI/CD job for now. Can we plan on a fix for this?
> 
> What I have heard so far indicate that the code that uses writev()
> would need to loop over to prepare for short writes, but your
> writev() that fails for "the OSS I/O size limit" (whatever it is)
> does not sound like something we want to change the callers to chomp
> the writev() calls into smaller chunks for.  Such a platform is far
> better off using the compat/writev for the code path we recently
> started using writev() in.

Yeah, we definitely do not want callers to worry about this. I think it
would be possible to have xwritev() handle this, but it gets ugly. If we
are worried about an individual iovec being larger than MAX_IO_SIZE,
then we may need to rewrite the iovec array to break it apart. And if we
are worried about the sum of the pieces being larger than MAX_IO_SIZE,
then we end up with multiple writev() calls anyway.

At which point the least-painful thing is probably just calling write()
on each segment anyway, which is exactly what the compat wrapper does.

> To be quite honest, I am not sure if it is even worth using writev()
> if we need a loop that protects against shrot writes, so unless I am
> grossly mistaken (e.g., perhaps there is some guarantee that there
> won't be any short writes for writev() that sends data smaller than
> 64k that I missed in the docs), the best course of action might be
> to revert the change to use writev() and use the two write(2)s as
> before, *if* we actually observe that the current code is broken by
> short writes.

I think writev() is buying us something when it works (it is halving the
number of writes for sideband packets). And it works when either:

  1. the platform is OK with writing up to 64k in a single writev()

  2. the platform has a limit that is small (like NonStop here), but
     writes less than MAX_IO_SIZE work and will save a write() call

If we just care about (1), then the right solution is to declare that
writev() isn't fully functional for us on some platforms, and they
should build with NO_WRITEV. And we should probably embed that in
config.mak.uname.

If we want to care about (2), then we could make the decision at
runtime, something like:

diff --git a/compat/writev.c b/compat/writev.c
index 3a94870a2f..cf2fbb39c9 100644
--- a/compat/writev.c
+++ b/compat/writev.c
@@ -1,7 +1,7 @@
 #include "../git-compat-util.h"
 #include "../wrapper.h"
 
-ssize_t git_writev(int fd, const struct iovec *iov, int iovcnt)
+ssize_t git_writev_with_write(int fd, const struct iovec *iov, int iovcnt)
 {
 	size_t total_written = 0;
 	size_t sum = 0;
@@ -42,3 +42,17 @@ ssize_t git_writev(int fd, const struct iovec *iov, int iovcnt)
 out:
 	return (ssize_t) total_written;
 }
+
+ssize_t git_writev(int fd, const struct iovec *iov, int iovcnt)
+{
+	size_t total = 0;
+	for (int i = 0; i < iovcnt; i++)
+		total += iov[i].iov_len;
+	if (total > MAX_IO_SIZE) {
+		/* too big; bail to wrapper which will limit individual writes */
+		return git_writev_with_write(fd, iov, iovcnt);
+	}
+
+	/* otherwise, use the real system writev */
+	return writev(fd, iov, iovcnt);
+}

I'm not sure that complexity is worth it, though. If your write-limit is
less than 64k you are already doing lots of extra write() calls, and
trying to squeeze out a tiny bit of performance by omitting a few of
them is not worth the trouble.

Though it does make things Just Work on such platforms without having to
set another build-time knob.

I do wonder what it all means for systems with MAX_IO_SIZE that is more
reasonable. Right now we are using writev() only for sideband packets.
But one of the stated reasons for using it there (instead of tweaking
the packet buffer so that there's room for the header in it) is that
people wanted to be able to start using writev() elsewhere. If we start
feeding unbounded data to it (say, a blob buffer), then we're going to
be violating MAX_IO_SIZE for those calls. So there may be more of this
headache down the road.

-Peff

Re: Git 2.54.0-rc1, subtests of t5310, t5326, t5327

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> To be quite honest, I am not sure if it is even worth using writev()
>> if we need a loop that protects against shrot writes, so unless I am
>> grossly mistaken (e.g., perhaps there is some guarantee that there
>> won't be any short writes for writev() that sends data smaller than
>> 64k that I missed in the docs), the best course of action might be
>> to revert the change to use writev() and use the two write(2)s as
>> before, *if* we actually observe that the current code is broken by
>> short writes.
>
> Ah, sorry, I should have double checked the actual code.  We already
> use a looping writev_in_full() that wraps writev(), so there is
> nothing extra that we still need to do to prepare for short writes.
>
> Unfortunately, comparing write_in_full() vs writev_in_full(), there
> is nothing that corresponds to xwrite() that can be used to hide the
> short writes and chomps an originally larger I/O into smaller
> pieces.

Oops, the beauty of having xwrite() is *not* that it hides short
writes (it doesn't), but it can be used to pretend that short writes
happened on platforms with unreasonably small I/O limit by setting
MAX_IO_SIZE to unusually low.  But the point that ...

> Unlike write() that we may receive a single linear large
> sequence of bytes, which we can choose to chomp into artificially
> smaller pieces and write them out (up to 8MB by default), writev()
> API lets the caller to prepare chunks of memory and I do not think
> there is a good way for the writev_in_full() at the lower layer to
> chomp these into smaller pieces, and even if we could, that would
> defeat the whole reason why we rewrote the original code that used
> write_in_full() into using writev(), i.e., to avoid extra allocation
> (and extra system calls---but if your I/O layer is limited to very
> small writes, no matter how we chop it, you will have to issue extra
> system calls to flush all of the data out).
>
> So, I dunno.

... I doubt that there exists a good way to have xwritev() that
wraps around writev() and pretend that a short write happened,
instead of issuing a large I/O, still stands.

RE: Git 2.54.0-rc1, subtests of t5310, t5326, t5327

[rsbecker]

On April 8, 2026 6:35 PM, Junio C Hamano wrote
>Junio C Hamano <gitster@pobox.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> To be quite honest, I am not sure if it is even worth using writev()
>>> if we need a loop that protects against shrot writes, so unless I am
>>> grossly mistaken (e.g., perhaps there is some guarantee that there
>>> won't be any short writes for writev() that sends data smaller than
>>> 64k that I missed in the docs), the best course of action might be to
>>> revert the change to use writev() and use the two write(2)s as
>>> before, *if* we actually observe that the current code is broken by
>>> short writes.
>>
>> Ah, sorry, I should have double checked the actual code.  We already
>> use a looping writev_in_full() that wraps writev(), so there is
>> nothing extra that we still need to do to prepare for short writes.
>>
>> Unfortunately, comparing write_in_full() vs writev_in_full(), there is
>> nothing that corresponds to xwrite() that can be used to hide the
>> short writes and chomps an originally larger I/O into smaller pieces.
>
>Oops, the beauty of having xwrite() is *not* that it hides short writes (it
doesn't),
>but it can be used to pretend that short writes happened on platforms with
>unreasonably small I/O limit by setting MAX_IO_SIZE to unusually low.  But
the
>point that ...
>
>> Unlike write() that we may receive a single linear large sequence of
>> bytes, which we can choose to chomp into artificially smaller pieces
>> and write them out (up to 8MB by default), writev() API lets the
>> caller to prepare chunks of memory and I do not think there is a good
>> way for the writev_in_full() at the lower layer to chomp these into
>> smaller pieces, and even if we could, that would defeat the whole
>> reason why we rewrote the original code that used
>> write_in_full() into using writev(), i.e., to avoid extra allocation
>> (and extra system calls---but if your I/O layer is limited to very
>> small writes, no matter how we chop it, you will have to issue extra
>> system calls to flush all of the data out).
>>
>> So, I dunno.
>
>... I doubt that there exists a good way to have xwritev() that wraps
around writev()
>and pretend that a short write happened, instead of issuing a large I/O,
still stands.

I am partial to Peff's runtime approach, personally.

If it helps (probably does not), the limit is due to the DMA/VMM limits on
the
hardware. The NonStop Message system is blindingly fast when sending
messages around between processes on other CPUs - far faster than I have
seen
on most other platforms. But there are physical limits in the chipset. It is
interesting
to use it in an abstracted way, like hacking the TCP/IP stack to pass
handles around.

Re: Git 2.54.0-rc1, subtests of t5310, t5326, t5327

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1889 bytes --]

On 2026-04-08 at 22:32:33, Jeff King wrote:
> I think writev() is buying us something when it works (it is hlving the
> number of writes for sideband packets). And it works when either:
> 
>   1. the platform is OK with writing up to 64k in a single writev()
> 
>   2. the platform has a limit that is small (like NonStop here), but
>      writes less than MAX_IO_SIZE work and will save a write() call
> 
> If we just care about (1), then the right solution is to declare that
> writev() isn't fully functional for us on some platforms, and they
> should build with NO_WRITEV. And we should probably embed that in
> config.mak.uname.

Looking at POSIX, there doesn't seem to be any constraints on the size
of individual vectors other than that they must total to less than
SSIZE_MAX.  iovcnt can be limited to 16, but I don't think we're hitting
that here.  POSIX does say that SSIZE_MAX does not need to exceed 32767,
which may be what's going on here, although that does seem like an
unreasonable value for a real system.  Linux, FreeBSD, and NetBSD all
set SSIZE_MAX to either INT_MAX or LONG_MAX.

I also think that 64 KiB is more than reasonable in terms of the size
that people should be able to send.  I'd personally expect to be able to
send values much larger, at least 512 KiB, and I have code that expects
even larger (16 MiB).

So I'd simply say that for systems that have a constraint on the size
that is "too small", they should just use NO_WRITEV.

However, I don't have a strong opinion on this and if people want to do
the proposal for option 2, that's fine with me.

I will say that we may find ourselves in a pickle with Rust code in the
future if we use `write_vectored` since that will probably just use the
OS implementation, but we can worry about that when we get there.
-- 
brian m. carlson (they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 325 bytes --]

Re: Git 2.54.0-rc1, subtests of t5310, t5326, t5327

[Patrick Steinhardt]

On Thu, Apr 09, 2026 at 12:20:26AM +0000, brian m. carlson wrote:
> On 2026-04-08 at 22:32:33, Jeff King wrote:
> > I think writev() is buying us something when it works (it is hlving the
> > number of writes for sideband packets). And it works when either:
> > 
> >   1. the platform is OK with writing up to 64k in a single writev()
> > 
> >   2. the platform has a limit that is small (like NonStop here), but
> >      writes less than MAX_IO_SIZE work and will save a write() call
> > 
> > If we just care about (1), then the right solution is to declare that
> > writev() isn't fully functional for us on some platforms, and they
> > should build with NO_WRITEV. And we should probably embed that in
> > config.mak.uname.

Yeah, agreed. I think we shouldn't make ourselves a hostage to platforms
that don't have reasonable support for writev(3p), as it does buy us
something on the majority of platforms that actually support it well.

That of course doesn't mean that we shouldn't support such platforms.

> Looking at POSIX, there doesn't seem to be any constraints on the size
> of individual vectors other than that they must total to less than
> SSIZE_MAX.  iovcnt can be limited to 16, but I don't think we're hitting
> that here.  POSIX does say that SSIZE_MAX does not need to exceed 32767,
> which may be what's going on here, although that does seem like an
> unreasonable value for a real system.  Linux, FreeBSD, and NetBSD all
> set SSIZE_MAX to either INT_MAX or LONG_MAX.
> 
> I also think that 64 KiB is more than reasonable in terms of the size
> that people should be able to send.  I'd personally expect to be able to
> send values much larger, at least 512 KiB, and I have code that expects
> even larger (16 MiB).
> 
> So I'd simply say that for systems that have a constraint on the size
> that is "too small", they should just use NO_WRITEV.

I would be happy with this as an intermediate step, as Randall has
confirmed it would fix the issue. It is the least intrusive step and has
the lowest risk.

> However, I don't have a strong opinion on this and if people want to do
> the proposal for option 2, that's fine with me.

I think in the long term this is the most sensible approach though so
that we don't have to special-case platforms. I've crafted the below
alternative to Peff's patch, and I think it's ultimately not too bad.

One question to Randall though: does MAX_IO_SIZE apply to the overall
size of the iovec or to the individual iovec entries? I think it should
be the latter, but I cannot easily verify and couldn't find any docs
around this. So could you please try the patch at the end of this mail
to verify that it works on your system?

In any case, I've tested that my patch also works when defining
MAX_IO_SIZE to 128 bytes on my system, which hopefully demonstrates that
it works as expected:

diff --git a/git-compat-util.h b/git-compat-util.h
index 4b4ea2498f..8e02b5f673 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -690,14 +690,8 @@ static inline uint64_t u64_add(uint64_t a, uint64_t b)
  * to override this, if the definition of SSIZE_MAX given by the platform
  * is broken.
  */
-#ifndef MAX_IO_SIZE
-# define MAX_IO_SIZE_DEFAULT (8*1024*1024)
-# if defined(SSIZE_MAX) && (SSIZE_MAX < MAX_IO_SIZE_DEFAULT)
-#  define MAX_IO_SIZE SSIZE_MAX
-# else
-#  define MAX_IO_SIZE MAX_IO_SIZE_DEFAULT
-# endif
-#endif
+#undef MAX_IO_SIZE
+#define MAX_IO_SIZE 128
 
 #ifdef HAVE_ALLOCA_H
 # include <alloca.h>

I'm happy to go either way, but think that we should definitely aim for
the below patch eventually. Just let me know which way you prefer and
I'm happy to polish up the patch.

Patrick

diff --git a/wrapper.c b/wrapper.c
index be8fa575e6..645dbc5f20 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -323,21 +323,50 @@ ssize_t write_in_full(int fd, const void *buf, size_t count)
 	return total;
 }
 
+ssize_t xwritev(int fd, struct iovec *iov, int iovcnt)
+{
+	ssize_t bytes_written;
+	int i;
+
+	/*
+	 * We need to make sure that no individual iovec entry exceeds
+	 * `MAX_IO_SIZE`. If there's any entry that does exceed this limit
+	 * we'll pass all entries up to it to `writev()`, and then process the
+	 * exceeding entry via a call to `xwrite()`.
+	 */
+	for (i = 0; i < iovcnt; i++)
+		if (iov[i].iov_len > MAX_IO_SIZE)
+			break;
+	if (i < iovcnt) {
+		/*
+		 * The first entry exceeds MAX_IO_SIZE, so we pass it to
+		 * xwrite, which knows to handle his case.
+		 */
+		if (!i)
+			return xwrite(fd, iov->iov_base, iov->iov_len);
+		iovcnt = i;
+	}
+
+	bytes_written = writev(fd, iov, iovcnt);
+	if (!bytes_written) {
+		errno = ENOSPC;
+		return -1;
+	}
+
+	return bytes_written;
+}
+
 ssize_t writev_in_full(int fd, struct iovec *iov, int iovcnt)
 {
 	ssize_t total_written = 0;
 
 	while (iovcnt) {
-		ssize_t bytes_written = writev(fd, iov, iovcnt);
-		if (bytes_written < 0) {
+		ssize_t bytes_written = xwritev(fd, iov, iovcnt);
+		if (bytes_written <= 0) {
 			if (errno == EINTR || errno == EAGAIN)
 				continue;
 			return -1;
 		}
-		if (!bytes_written) {
-			errno = ENOSPC;
-			return -1;
-		}
 
 		total_written += bytes_written;
 
diff --git a/wrapper.h b/wrapper.h
index 27519b32d1..a6287d7f4d 100644
--- a/wrapper.h
+++ b/wrapper.h
@@ -16,6 +16,7 @@ void *xmmap_gently(void *start, size_t length, int prot, int flags, int fd, off_
 int xopen(const char *path, int flags, ...);
 ssize_t xread(int fd, void *buf, size_t len);
 ssize_t xwrite(int fd, const void *buf, size_t len);
+ssize_t xwritev(int fd, struct iovec *iov, int iovcnt);
 ssize_t xpread(int fd, void *buf, size_t len, off_t offset);
 int xdup(int fd);
 FILE *xfopen(const char *path, const char *mode);

Re: Git 2.54.0-rc1, subtests of t5310, t5326, t5327

[Phillip Wood]

On 09/04/2026 09:17, Patrick Steinhardt wrote:
> 
> One question to Randall though: does MAX_IO_SIZE apply to the overall
> size of the iovec or to the individual iovec entries? 

In <014e01dcc793$8a9bab90$9fd302b0$@nexbridge.com> Randall says

     Specifying  the sum of the iov_len values in the iov array greater
     than the OSS I/O size limit for that open causes the  writev()
     function  to return  -1  and  set errno to [EINVAL].

So it is the overall size which fits with POSIX limiting to overall size 
to SSIZE_MAX.

Thanks

Phillip

> I think it should
> be the latter, but I cannot easily verify and couldn't find any docs
> around this. So could you please try the patch at the end of this mail
> to verify that it works on your system?
> 
> In any case, I've tested that my patch also works when defining
> MAX_IO_SIZE to 128 bytes on my system, which hopefully demonstrates that
> it works as expected:
> 
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 4b4ea2498f..8e02b5f673 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -690,14 +690,8 @@ static inline uint64_t u64_add(uint64_t a, uint64_t b)
>    * to override this, if the definition of SSIZE_MAX given by the platform
>    * is broken.
>    */
> -#ifndef MAX_IO_SIZE
> -# define MAX_IO_SIZE_DEFAULT (8*1024*1024)
> -# if defined(SSIZE_MAX) && (SSIZE_MAX < MAX_IO_SIZE_DEFAULT)
> -#  define MAX_IO_SIZE SSIZE_MAX
> -# else
> -#  define MAX_IO_SIZE MAX_IO_SIZE_DEFAULT
> -# endif
> -#endif
> +#undef MAX_IO_SIZE
> +#define MAX_IO_SIZE 128
>   
>   #ifdef HAVE_ALLOCA_H
>   # include <alloca.h>
> 
> I'm happy to go either way, but think that we should definitely aim for
> the below patch eventually. Just let me know which way you prefer and
> I'm happy to polish up the patch.
> 
> Patrick
> 
> diff --git a/wrapper.c b/wrapper.c
> index be8fa575e6..645dbc5f20 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -323,21 +323,50 @@ ssize_t write_in_full(int fd, const void *buf, size_t count)
>   	return total;
>   }
>   
> +ssize_t xwritev(int fd, struct iovec *iov, int iovcnt)
> +{
> +	ssize_t bytes_written;
> +	int i;
> +
> +	/*
> +	 * We need to make sure that no individual iovec entry exceeds
> +	 * `MAX_IO_SIZE`. If there's any entry that does exceed this limit
> +	 * we'll pass all entries up to it to `writev()`, and then process the
> +	 * exceeding entry via a call to `xwrite()`.
> +	 */
> +	for (i = 0; i < iovcnt; i++)
> +		if (iov[i].iov_len > MAX_IO_SIZE)
> +			break;
> +	if (i < iovcnt) {
> +		/*
> +		 * The first entry exceeds MAX_IO_SIZE, so we pass it to
> +		 * xwrite, which knows to handle his case.
> +		 */
> +		if (!i)
> +			return xwrite(fd, iov->iov_base, iov->iov_len);
> +		iovcnt = i;
> +	}
> +
> +	bytes_written = writev(fd, iov, iovcnt);
> +	if (!bytes_written) {
> +		errno = ENOSPC;
> +		return -1;
> +	}
> +
> +	return bytes_written;
> +}
> +
>   ssize_t writev_in_full(int fd, struct iovec *iov, int iovcnt)
>   {
>   	ssize_t total_written = 0;
>   
>   	while (iovcnt) {
> -		ssize_t bytes_written = writev(fd, iov, iovcnt);
> -		if (bytes_written < 0) {
> +		ssize_t bytes_written = xwritev(fd, iov, iovcnt);
> +		if (bytes_written <= 0) {
>   			if (errno == EINTR || errno == EAGAIN)
>   				continue;
>   			return -1;
>   		}
> -		if (!bytes_written) {
> -			errno = ENOSPC;
> -			return -1;
> -		}
>   
>   		total_written += bytes_written;
>   
> diff --git a/wrapper.h b/wrapper.h
> index 27519b32d1..a6287d7f4d 100644
> --- a/wrapper.h
> +++ b/wrapper.h
> @@ -16,6 +16,7 @@ void *xmmap_gently(void *start, size_t length, int prot, int flags, int fd, off_
>   int xopen(const char *path, int flags, ...);
>   ssize_t xread(int fd, void *buf, size_t len);
>   ssize_t xwrite(int fd, const void *buf, size_t len);
> +ssize_t xwritev(int fd, struct iovec *iov, int iovcnt);
>   ssize_t xpread(int fd, void *buf, size_t len, off_t offset);
>   int xdup(int fd);
>   FILE *xfopen(const char *path, const char *mode);
> 

Re: Git 2.54.0-rc1, subtests of t5310, t5326, t5327

[Patrick Steinhardt]

On Thu, Apr 09, 2026 at 10:48:02AM +0100, Phillip Wood wrote:
> On 09/04/2026 09:17, Patrick Steinhardt wrote:
> > 
> > One question to Randall though: does MAX_IO_SIZE apply to the overall
> > size of the iovec or to the individual iovec entries?
> 
> In <014e01dcc793$8a9bab90$9fd302b0$@nexbridge.com> Randall says
> 
>     Specifying  the sum of the iov_len values in the iov array greater
>     than the OSS I/O size limit for that open causes the  writev()
>     function  to return  -1  and  set errno to [EINVAL].
> 
> So it is the overall size which fits with POSIX limiting to overall size to
> SSIZE_MAX.

Ah, thanks for the pointer. I've adapted the patch a bit to the below
one. Again, I've tested it with `#define MAX_IO_SIZE 100` to verify that
it works as expected.

I guess I'll send a polished version to the mailing list in a bit.

Patrick

diff --git a/wrapper.c b/wrapper.c
index be8fa575e6..d989c78b4b 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -323,21 +323,60 @@ ssize_t write_in_full(int fd, const void *buf, size_t count)
 	return total;
 }
 
+ssize_t xwritev(int fd, struct iovec *iov, int iovcnt)
+{
+	ssize_t bytes_written;
+	size_t total_length;
+	int i;
+
+	/*
+	 * We need to make sure that writev(3p) call does not write more than
+	 * `MAX_IO_SIZE` many bytes. If we do exceed that limit, we only pass
+	 * those iovecs to writev(3p) that sum up to less than the limit.
+	 *
+	 * If on the other hand the first iovec entry already exceeds this
+	 * limit we'll instead use xwrite() to write it, which knows to handle
+	 * `MAX_IO_SIZE` for us.
+	 */
+	for (i = 0, total_length = 0; i < iovcnt; i++) {
+		if (unsigned_add_overflows(total_length, iov[i].iov_len))
+			break;
+
+		total_length += iov[i].iov_len;
+		if (total_length > MAX_IO_SIZE)
+			break;
+	}
+
+	if (i < iovcnt) {
+		/*
+		 * The first entry exceeds MAX_IO_SIZE, so we pass it to
+		 * xwrite, which knows to handle this case.
+		 */
+		if (!i)
+			return xwrite(fd, iov->iov_base, iov->iov_len);
+		iovcnt = i;
+	}
+
+	bytes_written = writev(fd, iov, iovcnt);
+	if (!bytes_written) {
+		errno = ENOSPC;
+		return -1;
+	}
+
+	return bytes_written;
+}
+
 ssize_t writev_in_full(int fd, struct iovec *iov, int iovcnt)
 {
 	ssize_t total_written = 0;
 
 	while (iovcnt) {
-		ssize_t bytes_written = writev(fd, iov, iovcnt);
-		if (bytes_written < 0) {
+		ssize_t bytes_written = xwritev(fd, iov, iovcnt);
+		if (bytes_written <= 0) {
 			if (errno == EINTR || errno == EAGAIN)
 				continue;
 			return -1;
 		}
-		if (!bytes_written) {
-			errno = ENOSPC;
-			return -1;
-		}
 
 		total_written += bytes_written;
 
diff --git a/wrapper.h b/wrapper.h
index 27519b32d1..a6287d7f4d 100644
--- a/wrapper.h
+++ b/wrapper.h
@@ -16,6 +16,7 @@ void *xmmap_gently(void *start, size_t length, int prot, int flags, int fd, off_
 int xopen(const char *path, int flags, ...);
 ssize_t xread(int fd, void *buf, size_t len);
 ssize_t xwrite(int fd, const void *buf, size_t len);
+ssize_t xwritev(int fd, struct iovec *iov, int iovcnt);
 ssize_t xpread(int fd, void *buf, size_t len, off_t offset);
 int xdup(int fd);
 FILE *xfopen(const char *path, const char *mode);

RE: Git 2.54.0-rc1, subtests of t5310, t5326, t5327

[rsbecker]

On April 9, 2026 4:17 AM, Patrick Steinhardt wrote
>On Thu, Apr 09, 2026 at 12:20:26AM +0000, brian m. carlson wrote:
>> On 2026-04-08 at 22:32:33, Jeff King wrote:
>> > I think writev() is buying us something when it works (it is hlving
>> > the number of writes for sideband packets). And it works when either:
>> >
>> >   1. the platform is OK with writing up to 64k in a single writev()
>> >
>> >   2. the platform has a limit that is small (like NonStop here), but
>> >      writes less than MAX_IO_SIZE work and will save a write() call
>> >
>> > If we just care about (1), then the right solution is to declare
>> > that
>> > writev() isn't fully functional for us on some platforms, and they
>> > should build with NO_WRITEV. And we should probably embed that in
>> > config.mak.uname.
>
>Yeah, agreed. I think we shouldn't make ourselves a hostage to platforms
that don't
>have reasonable support for writev(3p), as it does buy us something on the
>majority of platforms that actually support it well.
>
>That of course doesn't mean that we shouldn't support such platforms.
>
>> Looking at POSIX, there doesn't seem to be any constraints on the size
>> of individual vectors other than that they must total to less than
>> SSIZE_MAX.  iovcnt can be limited to 16, but I don't think we're
>> hitting that here.  POSIX does say that SSIZE_MAX does not need to
>> exceed 32767, which may be what's going on here, although that does
>> seem like an unreasonable value for a real system.  Linux, FreeBSD,
>> and NetBSD all set SSIZE_MAX to either INT_MAX or LONG_MAX.
>>
>> I also think that 64 KiB is more than reasonable in terms of the size
>> that people should be able to send.  I'd personally expect to be able
>> to send values much larger, at least 512 KiB, and I have code that
>> expects even larger (16 MiB).
>>
>> So I'd simply say that for systems that have a constraint on the size
>> that is "too small", they should just use NO_WRITEV.
>
>I would be happy with this as an intermediate step, as Randall has
confirmed it
>would fix the issue. It is the least intrusive step and has the lowest
risk.
>
>> However, I don't have a strong opinion on this and if people want to
>> do the proposal for option 2, that's fine with me.
>
>I think in the long term this is the most sensible approach though so that
we don't
>have to special-case platforms. I've crafted the below alternative to
Peff's patch, and
>I think it's ultimately not too bad.
>
>One question to Randall though: does MAX_IO_SIZE apply to the overall size
of the
>iovec or to the individual iovec entries? I think it should be the latter,
but I cannot
>easily verify and couldn't find any docs around this. So could you please
try the
>patch at the end of this mail to verify that it works on your system?
>
>In any case, I've tested that my patch also works when defining MAX_IO_SIZE
to
>128 bytes on my system, which hopefully demonstrates that it works as
expected:
>
>diff --git a/git-compat-util.h b/git-compat-util.h index
4b4ea2498f..8e02b5f673
>100644
>--- a/git-compat-util.h
>+++ b/git-compat-util.h
>@@ -690,14 +690,8 @@ static inline uint64_t u64_add(uint64_t a, uint64_t b)
>  * to override this, if the definition of SSIZE_MAX given by the platform
>  * is broken.
>  */
>-#ifndef MAX_IO_SIZE
>-# define MAX_IO_SIZE_DEFAULT (8*1024*1024) -# if defined(SSIZE_MAX) &&
>(SSIZE_MAX < MAX_IO_SIZE_DEFAULT) -#  define MAX_IO_SIZE SSIZE_MAX -# else
-
>#  define MAX_IO_SIZE MAX_IO_SIZE_DEFAULT -# endif -#endif
>+#undef MAX_IO_SIZE
>+#define MAX_IO_SIZE 128
>
> #ifdef HAVE_ALLOCA_H
> # include <alloca.h>
>
>I'm happy to go either way, but think that we should definitely aim for the
below
>patch eventually. Just let me know which way you prefer and I'm happy to
polish up
>the patch.
>
>Patrick
>
>diff --git a/wrapper.c b/wrapper.c
>index be8fa575e6..645dbc5f20 100644
>--- a/wrapper.c
>+++ b/wrapper.c
>@@ -323,21 +323,50 @@ ssize_t write_in_full(int fd, const void *buf, size_t
count)
> 	return total;
> }
>
>+ssize_t xwritev(int fd, struct iovec *iov, int iovcnt) {
>+	ssize_t bytes_written;
>+	int i;
>+
>+	/*
>+	 * We need to make sure that no individual iovec entry exceeds
>+	 * `MAX_IO_SIZE`. If there's any entry that does exceed this limit
>+	 * we'll pass all entries up to it to `writev()`, and then process
the
>+	 * exceeding entry via a call to `xwrite()`.
>+	 */
>+	for (i = 0; i < iovcnt; i++)
>+		if (iov[i].iov_len > MAX_IO_SIZE)
>+			break;
>+	if (i < iovcnt) {
>+		/*
>+		 * The first entry exceeds MAX_IO_SIZE, so we pass it to
>+		 * xwrite, which knows to handle his case.
>+		 */
>+		if (!i)
>+			return xwrite(fd, iov->iov_base, iov->iov_len);
>+		iovcnt = i;
>+	}
>+
>+	bytes_written = writev(fd, iov, iovcnt);
>+	if (!bytes_written) {
>+		errno = ENOSPC;
>+		return -1;
>+	}
>+
>+	return bytes_written;
>+}
>+
> ssize_t writev_in_full(int fd, struct iovec *iov, int iovcnt)  {
> 	ssize_t total_written = 0;
>
> 	while (iovcnt) {
>-		ssize_t bytes_written = writev(fd, iov, iovcnt);
>-		if (bytes_written < 0) {
>+		ssize_t bytes_written = xwritev(fd, iov, iovcnt);
>+		if (bytes_written <= 0) {
> 			if (errno == EINTR || errno == EAGAIN)
> 				continue;
> 			return -1;
> 		}
>-		if (!bytes_written) {
>-			errno = ENOSPC;
>-			return -1;
>-		}
>
> 		total_written += bytes_written;
>
>diff --git a/wrapper.h b/wrapper.h
>index 27519b32d1..a6287d7f4d 100644
>--- a/wrapper.h
>+++ b/wrapper.h
>@@ -16,6 +16,7 @@ void *xmmap_gently(void *start, size_t length, int prot,
int
>flags, int fd, off_  int xopen(const char *path, int flags, ...);  ssize_t
xread(int fd, void
>*buf, size_t len);  ssize_t xwrite(int fd, const void *buf, size_t len);
>+ssize_t xwritev(int fd, struct iovec *iov, int iovcnt);
> ssize_t xpread(int fd, void *buf, size_t len, off_t offset);  int xdup(int
fd);  FILE
>*xfopen(const char *path, const char *mode);

Please do not make the change in git-compat-util. This will break xwrite().
We already have MAX_IO_SIZE working and verified from years ago. Changing
that will remove our platform from being supportable.

Re: Git 2.54.0-rc1, subtests of t5310, t5326, t5327

[Jeff King]

On Thu, Apr 09, 2026 at 09:46:39AM -0400, rsbecker@nexbridge.com wrote:

> >--- a/git-compat-util.h
> >+++ b/git-compat-util.h
> >@@ -690,14 +690,8 @@ static inline uint64_t u64_add(uint64_t a, uint64_t b)
> >  * to override this, if the definition of SSIZE_MAX given by the platform
> >  * is broken.
> >  */
> >-#ifndef MAX_IO_SIZE
> >-# define MAX_IO_SIZE_DEFAULT (8*1024*1024) -# if defined(SSIZE_MAX) &&
> >(SSIZE_MAX < MAX_IO_SIZE_DEFAULT) -#  define MAX_IO_SIZE SSIZE_MAX -# else
> -
> >#  define MAX_IO_SIZE MAX_IO_SIZE_DEFAULT -# endif -#endif
> >+#undef MAX_IO_SIZE
> >+#define MAX_IO_SIZE 128
> [...]
> Please do not make the change in git-compat-util. This will break xwrite().
> We already have MAX_IO_SIZE working and verified from years ago. Changing
> that will remove our platform from being supportable.

I think that was just there to demonstrate that the patch works
regardless of the size, and would not be included in the final.
Building with:

  make CFLAGS=-DMAX_IO_SIZE=128

is probably a nicer way of doing that, though. ;)

-Peff

Re: Git 2.54.0-rc1, subtests of t5310, t5326, t5327

[Jeff King]

On Thu, Apr 09, 2026 at 10:17:22AM +0200, Patrick Steinhardt wrote:

> > that people should be able to send.  I'd personally expect to be able to
> > send values much larger, at least 512 KiB, and I have code that expects
> > even larger (16 MiB).
> > 
> > So I'd simply say that for systems that have a constraint on the size
> > that is "too small", they should just use NO_WRITEV.
> 
> I would be happy with this as an intermediate step, as Randall has
> confirmed it would fix the issue. It is the least intrusive step and has
> the lowest risk.

The only downside is that if there are other systems with a small
SSIZE_MAX, they'll need similar treatment.

I took a look at auto-detecting this at the preprocessor stage. The
conditional itself is not too bad, but we have to compile writev.o
unconditionally, as well as re-order some definitions:

diff --git a/Makefile b/Makefile
index 5d22394c2e..f9b4dd8ae3 100644
--- a/Makefile
+++ b/Makefile
@@ -1125,6 +1125,7 @@ LIB_OBJS += compat/nonblock.o
 LIB_OBJS += compat/obstack.o
 LIB_OBJS += compat/open.o
 LIB_OBJS += compat/terminal.o
+LIB_OBJS += compat/writev.o
 LIB_OBJS += compiler-tricks/not-constant.o
 LIB_OBJS += config.o
 LIB_OBJS += connect.o
@@ -2031,7 +2032,6 @@ ifdef NO_PREAD
 endif
 ifdef NO_WRITEV
 	COMPAT_CFLAGS += -DNO_WRITEV
-	COMPAT_OBJS += compat/writev.o
 endif
 ifdef NO_FAST_WORKING_DIRECTORY
 	BASIC_CFLAGS += -DNO_FAST_WORKING_DIRECTORY
diff --git a/compat/posix.h b/compat/posix.h
index 94699a03fa..57c11938c7 100644
--- a/compat/posix.h
+++ b/compat/posix.h
@@ -326,6 +326,39 @@ int git_lstat(const char *, struct stat *);
 ssize_t git_pread(int fd, void *buf, size_t count, off_t offset);
 #endif
 
+/*
+ * Limit size of IO chunks, because huge chunks only cause pain.  OS X
+ * 64-bit is buggy, returning EINVAL if len >= INT_MAX; and even in
+ * the absence of bugs, large chunks can result in bad latencies when
+ * you decide to kill the process.
+ *
+ * We pick 8 MiB as our default, but if the platform defines SSIZE_MAX
+ * that is smaller than that, clip it to SSIZE_MAX, as a call to
+ * read(2) or write(2) larger than that is allowed to fail.  As the last
+ * resort, we allow a port to pass via CFLAGS e.g. "-DMAX_IO_SIZE=value"
+ * to override this, if the definition of SSIZE_MAX given by the platform
+ * is broken.
+ */
+#ifndef MAX_IO_SIZE
+# define MAX_IO_SIZE_DEFAULT (8*1024*1024)
+# if defined(SSIZE_MAX) && (SSIZE_MAX < MAX_IO_SIZE_DEFAULT)
+#  define MAX_IO_SIZE SSIZE_MAX
+# else
+#  define MAX_IO_SIZE MAX_IO_SIZE_DEFAULT
+# endif
+#endif
+
+/*
+ * Temporary hack: our use of writev() does not respect MAX_IO_SIZE, but we
+ * never feed more than a single pkt-line packet to it. Disable writev()
+ * and use our fallback wrapper if the system can't support that size.
+ *
+ * This should be removed once writev() supports MAX_IO_SIZE.
+ */
+#if !defined(NO_WRITEV) && (MAX_IO_SIZE < 65536)
+#define NO_WRITEV
+#endif
+
 #ifdef NO_WRITEV
 #define writev git_writev
 #define iovec git_iovec
diff --git a/git-compat-util.h b/git-compat-util.h
index 4b4ea2498f..60108459f3 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -677,28 +677,6 @@ static inline uint64_t u64_add(uint64_t a, uint64_t b)
 	return a + b;
 }
 
-/*
- * Limit size of IO chunks, because huge chunks only cause pain.  OS X
- * 64-bit is buggy, returning EINVAL if len >= INT_MAX; and even in
- * the absence of bugs, large chunks can result in bad latencies when
- * you decide to kill the process.
- *
- * We pick 8 MiB as our default, but if the platform defines SSIZE_MAX
- * that is smaller than that, clip it to SSIZE_MAX, as a call to
- * read(2) or write(2) larger than that is allowed to fail.  As the last
- * resort, we allow a port to pass via CFLAGS e.g. "-DMAX_IO_SIZE=value"
- * to override this, if the definition of SSIZE_MAX given by the platform
- * is broken.
- */
-#ifndef MAX_IO_SIZE
-# define MAX_IO_SIZE_DEFAULT (8*1024*1024)
-# if defined(SSIZE_MAX) && (SSIZE_MAX < MAX_IO_SIZE_DEFAULT)
-#  define MAX_IO_SIZE SSIZE_MAX
-# else
-#  define MAX_IO_SIZE MAX_IO_SIZE_DEFAULT
-# endif
-#endif
-
 #ifdef HAVE_ALLOCA_H
 # include <alloca.h>
 # define xalloca(size)      (alloca(size))


It's not _too_ complicated, but it's a little more than I'd like for an
-rc2. Yet another option is to swap out NO_WRITEV for USE_WRITEV,
effectively reverting the feature temporarily until it supports
MAX_IO_SIZE. But that's also slightly non-trivial.

So I dunno. Do we want just:

diff --git a/config.mak.uname b/config.mak.uname
index ccb3f71881..b672b1d077 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -654,6 +654,7 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
 	CSPRNG_METHOD = openssl
 	SANE_TOOL_PATH = /usr/coreutils/bin:/usr/local/bin
 	SHELL_PATH = /usr/coreutils/bin/bash
+	NO_WRITEV = NotUntilItSupportsMaxIOSize
 endif
 ifeq ($(uname_S),OS/390)
 	NO_SYS_POLL_H = YesPlease

and hope it's the only one?

-Peff

RE: Git 2.54.0-rc1, subtests of t5310, t5326, t5327

[rsbecker]

On April 9, 2026 4:34 PM, Jeff King wrote:
>On Thu, Apr 09, 2026 at 09:46:39AM -0400, rsbecker@nexbridge.com wrote:
>
>> >--- a/git-compat-util.h
>> >+++ b/git-compat-util.h
>> >@@ -690,14 +690,8 @@ static inline uint64_t u64_add(uint64_t a,
>> >uint64_t b)
>> >  * to override this, if the definition of SSIZE_MAX given by the
>> >platform
>> >  * is broken.
>> >  */
>> >-#ifndef MAX_IO_SIZE
>> >-# define MAX_IO_SIZE_DEFAULT (8*1024*1024) -# if defined(SSIZE_MAX)
>> >&& (SSIZE_MAX < MAX_IO_SIZE_DEFAULT) -#  define MAX_IO_SIZE SSIZE_MAX
>> >-# else
>> -
>> >#  define MAX_IO_SIZE MAX_IO_SIZE_DEFAULT -# endif -#endif
>> >+#undef MAX_IO_SIZE
>> >+#define MAX_IO_SIZE 128
>> [...]
>> Please do not make the change in git-compat-util. This will break xwrite().
>> We already have MAX_IO_SIZE working and verified from years ago.
>> Changing that will remove our platform from being supportable.
>
>I think that was just there to demonstrate that the patch works regardless of the
>size, and would not be included in the final.
>Building with:
>
>  make CFLAGS=-DMAX_IO_SIZE=128
>
>is probably a nicer way of doing that, though. ;)

We had that set properly in git-compat-util.h for years. MAX_IO_SIZE should be set to SSIZE_MAX if SSIZE_MAX is defined.
#ifndef MAX_IO_SIZE
# define MAX_IO_SIZE_DEFAULT (8*1024*1024)
# if defined(SSIZE_MAX) && (SSIZE_MAX < MAX_IO_SIZE_DEFAULT)
#  define MAX_IO_SIZE SSIZE_MAX
# else
#  define MAX_IO_SIZE MAX_IO_SIZE_DEFAULT

Re: Git 2.54.0-rc1, subtests of t5310, t5326, t5327

[Jeff King]

On Thu, Apr 09, 2026 at 06:40:00PM -0400, rsbecker@nexbridge.com wrote:

> >> Please do not make the change in git-compat-util. This will break xwrite().
> >> We already have MAX_IO_SIZE working and verified from years ago.
> >> Changing that will remove our platform from being supportable.
> >
> >I think that was just there to demonstrate that the patch works regardless of the
> >size, and would not be included in the final.
> >Building with:
> >
> >  make CFLAGS=-DMAX_IO_SIZE=128
> >
> >is probably a nicer way of doing that, though. ;)
> 
> We had that set properly in git-compat-util.h for years. MAX_IO_SIZE should be set to SSIZE_MAX if SSIZE_MAX is defined.
> #ifndef MAX_IO_SIZE
> # define MAX_IO_SIZE_DEFAULT (8*1024*1024)
> # if defined(SSIZE_MAX) && (SSIZE_MAX < MAX_IO_SIZE_DEFAULT)
> #  define MAX_IO_SIZE SSIZE_MAX
> # else
> #  define MAX_IO_SIZE MAX_IO_SIZE_DEFAULT

Right. We would retain that. I think the point was that Patrick was
dropping MAX_IO_SIZE artificially on his Linux system to exercise the
new code.

-Peff

Re: Git 2.54.0-rc1, subtests of t5310, t5326, t5327

[Patrick Steinhardt]

On Thu, Apr 09, 2026 at 06:58:06PM -0400, Jeff King wrote:
> On Thu, Apr 09, 2026 at 06:40:00PM -0400, rsbecker@nexbridge.com wrote:
> 
> > >> Please do not make the change in git-compat-util. This will break xwrite().
> > >> We already have MAX_IO_SIZE working and verified from years ago.
> > >> Changing that will remove our platform from being supportable.
> > >
> > >I think that was just there to demonstrate that the patch works regardless of the
> > >size, and would not be included in the final.
> > >Building with:
> > >
> > >  make CFLAGS=-DMAX_IO_SIZE=128
> > >
> > >is probably a nicer way of doing that, though. ;)
> > 
> > We had that set properly in git-compat-util.h for years. MAX_IO_SIZE should be set to SSIZE_MAX if SSIZE_MAX is defined.
> > #ifndef MAX_IO_SIZE
> > # define MAX_IO_SIZE_DEFAULT (8*1024*1024)
> > # if defined(SSIZE_MAX) && (SSIZE_MAX < MAX_IO_SIZE_DEFAULT)
> > #  define MAX_IO_SIZE SSIZE_MAX
> > # else
> > #  define MAX_IO_SIZE MAX_IO_SIZE_DEFAULT
> 
> Right. We would retain that. I think the point was that Patrick was
> dropping MAX_IO_SIZE artificially on his Linux system to exercise the
> new code.

Yup, exactly. I don't have any intent to change the above snippet in our
code base.

Patrick

Re: Git 2.54.0-rc1, subtests of t5310, t5326, t5327

[Johannes Sixt]

Am 09.04.26 um 10:17 schrieb Patrick Steinhardt:
> Yeah, agreed. I think we shouldn't make ourselves a hostage to platforms
> that don't have reasonable support for writev(3p), as it does buy us
> something on the majority of platforms that actually support it well.
> 
> That of course doesn't mean that we shouldn't support such platforms.

My take on this matter is that the use writev in Git's code gives POSIX
centered contributors a false sense of security. POSIX's writev offers a
number of guarantees that a compatibility implementation cannot provide.
If uses of writev proliferate, I forsee the time when someone reports a
problem on a compatibility platform, and one of us will point at POSIX
and say "but that cannot happen because we have this and that guarantee".

What did we gain with writev? There are half that many system calls. So
what? Have there been any hard numbers about better performance, for
example? Not even the call sites became simpler, see the commit that
introduced the only caller: 26986f4cbaf3 ("sideband: use writev(3p) to
send pktlines", 2026-03-13).

-- Hannes