Re: [PATCH] progress.c tests: fix breakage with COLUMNS != 80

From: Jeff King <peff@peff.net>
Date: Jun 23, 2021, 23:48 UTC
Message-ID: <YNPISWEBxISC30DW@coredump.intra.peff.net>
In-Reply-To: <patch-1.1-cba5d88ca35-20210621T070114Z-avarab@gmail.com>

On Mon, Jun 21, 2021 at 09:01:23AM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 8 quoted lines

> The tests added in 2bb74b53a49 (Test the progress display, 2019-09-16)
> broke under anything except COLUMNS=80, i.e. when running them under
> the "-v" mode under a differently sized terminal.
> 
> Let's set the expected number of COLUMNS at the start of the test to
> fix that bug. It's handy not do do this in test-progress.c itself, in
> case we'd like to test for a different number of COLUMNS, either
> manually or in a future test.

Hmm. So I can easily reproduce the problem here, and your patch fixes it. But my first thought was: shouldn't test-lib.sh be handling this to give all of the scripts a uniform environment?

And indeed, we _do_ unset COLUMNS there. So I think the problem isn't a bad setting of $COLUMNS, but rather that in "-v" mode, the sub-command's stderr is hooked to our tty, and term_columns() is smart enough to use TIOCGWINSZ to get the value (at least on some platforms).

Setting $COLUMNS again in the environment fixes it, because we prefer that value to trying the ioctl.

So I don't think what you have here is wrong (though the commit message is a little misleading). But it seems like the original intent of our "unset COLUMNS" in test-lib.sh would best be fulfilled by setting it to a known value there (like 80), rather than unsetting it.

I admit this a _bit_ of a nitpick (since as far as we know none of the other scripts care about the terminal width), so I'm OK with this as-is if you feel strongly the other way.

-Peff