Re: [PATCH v2 3/3] shorten_unambiguous_ref(): avoid sscanf()
- From
Jeff King <peff@peff.net>
- Date
- Feb 16, 2023, 17:28 UTC
- Message-ID
- <Y+5nuTbsbuqll5YP@coredump.intra.peff.net>
- In-Reply-To
- <xmqqlekxr0fo.fsf@gitster.g>
On Thu, Feb 16, 2023 at 09:21:15AM -0800, Junio C Hamano wrote:
Show 28 quoted lines
> How about a bit more detail on sscanf(), like this? > > t/t1401-symbolic-ref.sh | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git c/t/t1401-symbolic-ref.sh w/t/t1401-symbolic-ref.sh > index be23be30c7..dafcb4d61b 100755 > --- c/t/t1401-symbolic-ref.sh > +++ w/t/t1401-symbolic-ref.sh > @@ -192,11 +192,13 @@ test_expect_success 'symbolic-ref pointing at another' ' > test_expect_success 'symbolic-ref --short handles complex utf8 case' ' > name="测试-加-增加-加-增加" && > git symbolic-ref TEST_SYMREF "refs/heads/$name" && > - # In the real world, we saw problems with this case only > - # when the locale includes UTF-8. Set it here to try to make things as > - # hard as possible for us to pass, but in practice we should do the > - # right thing regardless (and of course some platforms may not even > - # have this locale). > + # In the real world, we saw this case misbehaved on macOS only > + # when the locale includes UTF-8, back when "symbolic-ref --short" > + # used sscanf(3) as part of its implementation. Set it here to > + # try to make things as hard as possible for us to pass, but in > + # practice we should do the right thing regardless (and of course > + # some platforms may not even have this locale), as we no longer > + # use platform sscanf(3) there. > LC_ALL=en_US.UTF-8 git symbolic-ref --short TEST_SYMREF >actual && > echo "$name" >expect && > test_cmp expect actual
I am OK with that squashed in. I hadn't bothered to mention macOS, etc, because that was covered in the commit message. My point in this comment was mostly to say "don't just remove this LC_ALL! It is doing something".
But your text makes the situation even more clear.
I do kind of wonder if this test is even doing much. It is nice to verify the fix (and hopefully somebody with macOS did indeed verify that it fails before the fix!). But it does not seem all that likely that we are going to regress in this area. I think it's reasonable to include it, but if the LC_ALL bit starts creating any portability problems, my first instinct would be to drop the test.
-Peff