Re: What's cooking in git.git (Sep 2018, #01; Tue, 4)

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Sep 5, 2018, 08:59 UTC
Message-ID: <CAPig+cRrwW662Hzp8V1sH51x8qYxPyqG=VHpOkfd-=RwUSSZbw@mail.gmail.com>
In-Reply-To: <87bm9cs5y6.fsf@evledraar.gmail.com>

On Wed, Sep 5, 2018 at 4:29 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 8 quoted lines

> I recently gained access to a Solaris 10 SPARC (5.10) box and discovered
> that the chainlint.sed implementation in 2.19.0 has more sed portability
> issues.
>
> First, whoever implemented the /bin/sed on Solaris apparently read the
> POSIX requirements for a max length label of 8 to mean that 8 characters
> should include the colon, so a bunch of things fail because of that, but
> are fixed with a shorter 7 character label.

I'm pretty sure that Solaris 'sed' predates POSIX by a good bit, but that's neither here nor there.

> Then GIT_TEST_CHAIN_LINT=1 still fails because 878f988350 ("t/test-lib:
> teach --chain-lint to detect broken &&-chains in subshells", 2018-07-11)
> added a "grep -q" invocation. The /bin/grep on that version of Solaris
> doesn't have -q.

I knew that '-q' was potentially problematic on some platforms, so I checked and saw that existing tests were already using it, thus went ahead and used it. Dropping '-q' here and redirecting stderr to /dev/null is a perfectly fine alternative.

Show 7 quoted lines

> We fixed a similar issue way back in 80700fde91
> ("t/t1304: make a second colon optional in the mask ACL check",
> 2010-03-15) by redirecting to /dev/null instead.
>
> A bunch of other tests in the test suite rely on "grep -q", but nothing
> as central as chainlint, so it makes everything break. Do we want to
> away with "grep -q" entirely because of old Solaris /bin/grep?

I count 132 instances in existing tests (though, I may have missed some).

Show 9 quoted lines

> At this point those familiar with Solaris are screaming ("why are you
> using anything in /bin!"). Okey fine, but it mostly worked before, so
> are we OK with breaking it? "Mostly" here is "test suite would fail
> 20-30 tests for various reasons, but at least no failures in test-lib.sh
> and the like".
>
> However, if as config.mak.uname does we run the tests with
> PATH=/usr/xpg6/bin:/usr/xpg4/bin:$PATH, at this point sed is fine with 8
> character labels [...]

So, if you run the tests via 'make test' (or whatever), then you get /usr/xpg?/bin in PATH, but if you run an individual test script (and haven't set your PATH appropriately), then you encounter the problems you report above?

> [...] but starts complaining about this (also in
> chainlint.sed):
>
>     sed: Too many commands, last: s/\n//

Erm. Any additional context to help narrow this down?

Show 13 quoted lines

>     diff --git a/config.mak.uname b/config.mak.uname
>     @@ -163,6 +163,10 @@ ifeq ($(uname_S),SunOS)
>             BASIC_CFLAGS += -D__EXTENSIONS__ -D__sun__
>     +       # t/chainlint.sed is hopelessly broken all known (tested
>     +       # Solaris 10 & 11) versions of Solaris, both /bin/sed and
>     +       # /usr/xpg4/bin/sed
>     +       GIT_TEST_CHAIN_LINT = 0
>      endif
>
> It slightly sucks to not have chainlint on
> Solaris, but it would also suck to revert chainlint.sed back to 2.18.0
> (there were some big improvements). So I think the patch above is the
> best way forward, especially since we're on rc2. What do you think?

Keeping in mind that the main goal of 'chainlint' is to prevent _new_ breakage from entering the test suite, disabling 'chainlint' on Solaris is an entirely reasonable way forward. In present day, it seems quite unlikely that we'll see someone develop new tests on Solaris, so having 'chainlint' disabled there isn't likely to be a big deal. Moreover, if someone does write a new test on Solaris which has a broken &&-chain in a subshell, that breakage will be caught very quickly once the test is run by anyone on Linux or MacOS (or Windows or BSD or AIX), so it's not like the broken test will make it into the project undetected.