Re: [PATCH v2 1/2] tests: fix and add lint for non-portable head -c N

From: Jeff King <peff@peff.net>
Date: Aug 23, 2018, 16:11 UTC
Message-ID: <20180823161150.GA29579@sigill.intra.peff.net>
In-Reply-To: <20180823152502.3886-1-avarab@gmail.com>

On Thu, Aug 23, 2018 at 03:25:01PM +0000, Ævar Arnfjörð Bjarmason wrote:

Show 9 quoted lines

> The "head -c BYTES" option is non-portable (not in POSIX[1]). Change
> such invocations to use the test_copy_bytes wrapper added in
> 48860819e8 ("t9300: factor out portable "head -c" replacement",
> 2016-06-30).
> 
> This fixes a test added in 9d2e330b17 ("ewah_read_mmap: bounds-check
> mmap reads", 2018-06-14), which has been breaking
> t5310-pack-bitmaps.sh on OpenBSD since 2.18.0. The OpenBSD ports
> already have a similar workaround after their upgrade to 2.18.0[2].

Heh, I even considered using this when writing that test. But the reason I introduced test_copy_bytes is not because the target platform did not have "head -c" at all, but because some tests need very specific buffering guarantees when reading from a shared pipe.

That said, if OpenBSD's "head" doesn't have "-c" at all, I'm fine with this as a fix (and it sounds like we know that IRIX lacks it, too).

Show 5 quoted lines

> Also, change a valgrind-specific codepath in test-lib.sh to use this
> wrapper. Given where valgrind runs I don't think this would ever
> become a portability issue in practice, but it's easier to just use
> the wrapper than introduce some exception for the "make test-lint"
> check being added here.

When working on 9d2e330b17, I recall finding these other "head -c" invocations in the test suite when I did 9d2e330b17 and took them as evidence that it was OK to use in vanilla cases. So even if these sites don't affect any platforms in practice, I think it's worth it to ban "head -c" completely.

Show 6 quoted lines

> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  t/check-non-portable-shell.pl | 1 +
>  t/t5310-pack-bitmaps.sh       | 2 +-
>  t/test-lib.sh                 | 4 ++--
>  3 files changed, 4 insertions(+), 3 deletions(-)

Patch itself looks good to me. Thanks.

-Peff