Re: [PATCH v2 3/3] shorten_unambiguous_ref(): avoid sscanf()

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 16, 2023, 23:36 UTC
Message-ID: <xmqq1qmpnpxs.fsf@gitster.g>
In-Reply-To: <Y+5nuTbsbuqll5YP@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 6 quoted lines

> 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.

That was my thought as well. I do not see this particular test to be about protecting against regression. With the greediness of its patterns, it is not likely that our future update will reintroduce use of sscanf(3) and suffer from the same platform bug. It is about documenting the historical bug, i.e. the value of the test part of the patch primarily is that macOS folks can apply it (and revert the code fix) and demonstrate the presence of a problem.

The refs/remotes/%s/HEAD test does have value---it is to keep us away from the temptation to go back to the sscanf(3) based solution. So do the other two to lessor degree, as they cover corner cases that a similar/rejected implementation would have failed to handle.

Thanks.