From: "René Scharfe" <l.s.r@web•de>
To: "Torsten Bögershausen" <tboegi@web•de>
Cc: Git List <git@vger•kernel.org>
Subject: Re: t3900 failure on macOS, iconv(3) broken?
Date: Tue, 9 Dec 2025 23:25:32 +0100 [thread overview]
Message-ID: <22b1f482-2012-4ee9-bc12-1b2123ee0101@web.de> (raw)
In-Reply-To: <20251209212420.GA10149@tb-raspi4>
On 12/9/25 10:24 PM, Torsten Bögershausen wrote:
> On Tue, Dec 09, 2025 at 08:35:23PM +0100, René Scharfe wrote:
>> On 12/9/25 5:33 PM, Torsten Bögershausen wrote:
>>> On Mon, Dec 08, 2025 at 11:59:11PM +0100, René Scharfe wrote:
>>>>
> [snip]
>>
>> This forgets to reset outsz and the converter state. With this patch
>> t0028-working-tree-encoding.sh seems to get stuck in an endless loop.
>
> Thanks for testing.
> I did another test here
> (increase the outbuffer with only one byte per round, old MacOs)
> and yes, we need to reset iconv.
> Back to your patch. I think it is good to go further,
> with one or 2 remarks, see TB
>
> out = xrealloc(out, outalloc);
> // TB: move into else outpos = out + sofar;
> // TB: move into else outsz = outalloc - sofar - 1;
> // TB: We have seen different breakages of apple iconv. Should we run the same code
> // on all versions of MacOs to be more future proof ?
> // and do we need a Makefile knob, if one, and only one platform is affected ?
> // I don't know
> #ifdef __APPLE__
> or
> #ifdef ICONV_BREAKS
macOS 14.8.2 reportedly doesn't have this particular issue, and I can
only hope that Apple will eventually fix that bug, so __APPLE__ seems a
bit too broad.
I'm also not thrilled about adding yet another build flag. The patch I
just posted sidesteps the issue by using the existing ICONVDIR setting
to use libiconv from Homebrew. We do that for gettext already, so it
should be fine..
> /*
> * If iconv(3) messes up piecemeal conversions
> * then restore the original pointers, sizes,
> * and converter state, then retry converting
> * the full string using the reallocated buffer.
> */
> insz += (char *)cp - in; /* TB stumbled here: "in" is "const char *"
We can add the const qualifier, but it won't affect the pointer
arithmetic. Perhaps casting to iconv_ibp would be more consistent?
> And I didn't like the fact that insz is destroyed
> and needs to be restored. That is why I had a originsz
> (or szinorig ?)
Sure, storing the original value would work, but is slightly more effort
than subtracting the progress made so far. originsz would only be used
if ICONV_BREAKS is defined, you'd need to declare it conditionally,
adding yet more overhead.
> cp = (iconv_ibp)in;
> outpos = out + bom_len;
> outsz = outalloc - bom_len - 1;
> iconv(conv, NULL, NULL, NULL, NULL);
> #else
> outpos = out + sofar;
> outsz = outalloc - sofar - 1;
I'd like to keep buffer increase and rollback separate. Perhaps
splitting out the output buffer adjustment is worth it, though? Not
sure. *shrug*
diff --git a/utf8.c b/utf8.c
index 35a0251939..c99243a63b 100644
--- a/utf8.c
+++ b/utf8.c
@@ -513,6 +513,18 @@ char *reencode_string_iconv(const char *in, size_t insz, iconv_t conv,
sofar = outpos - out;
outalloc = st_add3(sofar, st_mult(insz, 2), 32);
out = xrealloc(out, outalloc);
+#ifdef ICONV_BREAKS
+ /*
+ * If iconv(3) messes up piecemeal conversions
+ * then restore the original pointers, sizes,
+ * and converter state, then retry converting
+ * the full string using the reallocated buffer.
+ */
+ insz += cp - (iconv_ibp)in;
+ cp = (iconv_ibp)in;
+ sofar = bom_len;
+ iconv(conv, NULL, NULL, NULL, NULL);
+#endif
outpos = out + sofar;
outsz = outalloc - sofar - 1;
}
> #endif
next prev parent reply other threads:[~2025-12-09 22:25 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-12-08 22:59 t3900 failure on macOS, iconv(3) broken? René Scharfe
2025-12-09 3:18 ` Koji Nakamaru
2025-12-09 3:50 ` Yee Cheng Chin
2025-12-09 4:03 ` Collin Funk
2025-12-09 16:33 ` Torsten Bögershausen
2025-12-09 19:35 ` René Scharfe
2025-12-09 21:24 ` Torsten Bögershausen
2025-12-09 22:25 ` René Scharfe [this message]
2025-12-09 19:35 ` [PATCH] config.mak.uname: use iconv from Homebrew on macOS René Scharfe
2025-12-09 20:39 ` Yee Cheng Chin
2025-12-09 21:27 ` René Scharfe
2025-12-10 11:17 ` Carlo Marcelo Arenas Belón
2025-12-10 17:56 ` René Scharfe
2025-12-11 2:53 ` Junio C Hamano
2025-12-11 11:17 ` Carlo Marcelo Arenas Belón
2025-12-12 2:20 ` Junio C Hamano
2025-12-12 9:16 ` René Scharfe
2025-12-12 10:02 ` Carlo Marcelo Arenas Belón
2025-12-12 13:04 ` Re* " Junio C Hamano
2025-12-12 13:48 ` René Scharfe
2025-12-12 23:39 ` Junio C Hamano
2025-12-10 16:42 ` Torsten Bögershausen
2025-12-10 17:56 ` René Scharfe
2025-12-10 23:10 ` brian m. carlson
2025-12-11 2:36 ` Junio C Hamano
2025-12-11 9:59 ` Junio C Hamano
2025-12-11 14:34 ` René Scharfe
2025-12-12 3:35 ` Junio C Hamano
2025-12-12 10:40 ` t3900 failure on macOS, iconv(3) broken? René Scharfe
2025-12-13 18:42 ` [PATCH v2 1/2] Makefile: add NO_HOMEBREW René Scharfe
2025-12-14 6:45 ` Torsten Bögershausen
2025-12-14 7:13 ` Junio C Hamano
2025-12-14 9:02 ` Torsten Bögershausen
2025-12-14 11:07 ` Junio C Hamano
2025-12-14 11:13 ` René Scharfe
2025-12-14 23:19 ` Junio C Hamano
2025-12-16 18:53 ` René Scharfe
2025-12-13 18:42 ` [PATCH v2 2/2] config.mak.uname: use iconv from Homebrew on macOS René Scharfe
2025-12-16 18:53 ` [PATCH v3 1/2] macOS: make Homebrew use configurable René Scharfe
2025-12-16 19:11 ` René Scharfe
2025-12-16 21:49 ` Torsten Bögershausen
2025-12-16 18:53 ` [PATCH v3 2/2] macOS: use iconv from Homebrew if present René Scharfe
2025-12-24 7:52 ` [PATCH v4 0/2] macOS: use iconv from Homebrew if needed and present René Scharfe
2025-12-24 8:02 ` [PATCH v4 1/2] macOS: make Homebrew use configurable René Scharfe
2025-12-24 8:03 ` [PATCH v4 2/2] macOS: use iconv from Homebrew if needed and present René Scharfe
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=22b1f482-2012-4ee9-bc12-1b2123ee0101@web.de \
--to=l.s.r@web$(echo .)de \
--cc=git@vger$(echo .)kernel.org \
--cc=tboegi@web$(echo .)de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox