| Message ID | patch-1.1-cba5d88ca35-20210621T070114Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | progress.c tests: fix breakage with COLUMNS != 80 | expand |
On Mon, Jun 21, 2021 at 09:01:23AM +0200, Ævar Arnfjörð Bjarmason wrote: > 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
On Wed, Jun 23, 2021 at 07:48:25PM -0400, Jeff King wrote: > On Mon, Jun 21, 2021 at 09:01:23AM +0200, Ævar Arnfjörð Bjarmason wrote: > > > 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). It is misleading indeed and needs to be updated. I did my own analysis and arrived to the same conclusions wrt COLUMNS being unset vs. the ioctl() and stderr being a tty. > 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. I remember one commit-graph test that does check the number of lines in the progress output, assuming one progress line per commit-graph layer, which breaks when we break the progress line in a too narrow terminal. Running './t5324-split-commit-graph.sh -v -i' in a 46 column wide terminal fails for me, but succeeds with 47 columns. I do suggest setting COLUMNS=80 in 'test-lib.sh'.
On Thu, Jun 24, 2021 at 07:12:53AM +0200, SZEDER Gábor wrote: > > 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. > > I remember one commit-graph test that does check the number of lines > in the progress output, assuming one progress line per commit-graph > layer, which breaks when we break the progress line in a too narrow > terminal. Running './t5324-split-commit-graph.sh -v -i' in a 46 > column wide terminal fails for me, but succeeds with 47 columns. > > I do suggest setting COLUMNS=80 in 'test-lib.sh'. Thanks for providing a concrete example. I agree we should just set it for all scripts via test-lib.sh, then. -Peff
On 24/06/2021 00:48, Jeff King wrote: > On Mon, Jun 21, 2021 at 09:01:23AM +0200, Ævar Arnfjörð Bjarmason wrote: > >> 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 There has been a similar recent issue [1] on Git for Windows regarding these settings where different terminals had different views about the size of the display window which resulted in incorrect out puts. Philip [1] https://github.com/git-for-windows/git/issues/3235
diff --git a/t/t0500-progress-display.sh b/t/t0500-progress-display.sh index 22058b503ac..66c092a0fe3 100755 --- a/t/t0500-progress-display.sh +++ b/t/t0500-progress-display.sh @@ -8,6 +8,11 @@ show_cr () { tr '\015' Q | sed -e "s/Q/<CR>\\$LF/g" } +test_expect_success 'setup COLUMNS' ' + COLUMNS=80 && + export COLUMNS +' + test_expect_success 'simple progress display' ' cat >expect <<-\EOF && Working hard: 1<CR>
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. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/t0500-progress-display.sh | 5 +++++ 1 file changed, 5 insertions(+)