* [PATCH] progress.c tests: fix breakage with COLUMNS != 80 @ 2021-06-21 7:01 Ævar Arnfjörð Bjarmason 2021-06-23 23:48 ` Jeff King 2021-06-24 10:19 ` [PATCH] test-lib.sh: set COLUMNS=80 for --verbose repeatability Ævar Arnfjörð Bjarmason 0 siblings, 2 replies; 10+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-06-21 7:01 UTC (permalink / raw) To: git Cc: Junio C Hamano, SZEDER Gábor, Ævar Arnfjörð Bjarmason 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(+) 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> -- 2.32.0.595.g55105f1e212 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] progress.c tests: fix breakage with COLUMNS != 80 2021-06-21 7:01 [PATCH] progress.c tests: fix breakage with COLUMNS != 80 Ævar Arnfjörð Bjarmason @ 2021-06-23 23:48 ` Jeff King 2021-06-24 5:12 ` SZEDER Gábor 2021-06-24 15:05 ` Philip Oakley 2021-06-24 10:19 ` [PATCH] test-lib.sh: set COLUMNS=80 for --verbose repeatability Ævar Arnfjörð Bjarmason 1 sibling, 2 replies; 10+ messages in thread From: Jeff King @ 2021-06-23 23:48 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Junio C Hamano, SZEDER Gábor 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] progress.c tests: fix breakage with COLUMNS != 80 2021-06-23 23:48 ` Jeff King @ 2021-06-24 5:12 ` SZEDER Gábor 2021-06-24 5:40 ` Jeff King 2021-06-24 15:05 ` Philip Oakley 1 sibling, 1 reply; 10+ messages in thread From: SZEDER Gábor @ 2021-06-24 5:12 UTC (permalink / raw) To: Jeff King; +Cc: Ævar Arnfjörð Bjarmason, git, Junio C Hamano 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'. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] progress.c tests: fix breakage with COLUMNS != 80 2021-06-24 5:12 ` SZEDER Gábor @ 2021-06-24 5:40 ` Jeff King 0 siblings, 0 replies; 10+ messages in thread From: Jeff King @ 2021-06-24 5:40 UTC (permalink / raw) To: SZEDER Gábor Cc: Ævar Arnfjörð Bjarmason, git, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] progress.c tests: fix breakage with COLUMNS != 80 2021-06-23 23:48 ` Jeff King 2021-06-24 5:12 ` SZEDER Gábor @ 2021-06-24 15:05 ` Philip Oakley 1 sibling, 0 replies; 10+ messages in thread From: Philip Oakley @ 2021-06-24 15:05 UTC (permalink / raw) To: Jeff King, Ævar Arnfjörð Bjarmason Cc: git, Junio C Hamano, SZEDER Gábor 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] test-lib.sh: set COLUMNS=80 for --verbose repeatability 2021-06-21 7:01 [PATCH] progress.c tests: fix breakage with COLUMNS != 80 Ævar Arnfjörð Bjarmason 2021-06-23 23:48 ` Jeff King @ 2021-06-24 10:19 ` Ævar Arnfjörð Bjarmason 2021-06-24 14:50 ` Jeff King ` (2 more replies) 1 sibling, 3 replies; 10+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-06-24 10:19 UTC (permalink / raw) To: git Cc: Junio C Hamano, SZEDER Gábor, Jeff King, Ævar Arnfjörð Bjarmason Some tests will fail under --verbose because while we've unset COLUMNS since b1d645b58ac (tests: unset COLUMNS inherited from environment, 2012-03-27), we also look for the columns with an ioctl(.., TIOCGWINSZ, ...) on some platforms. By setting COLUMNS again we preempt the TIOCGWINSZ lookup in pager.c's term_columns(), it'll take COLUMNS over TIOCGWINSZ, This fixes the t0500-progress-display.sh test when run as: ./t0500-progress-display.sh --verbose It broke because of a combination of the this issue and the progress output reacting to the column width since 545dc345ebd (progress: break too long progress bar lines, 2019-04-12). The t5324-split-commit-graph.sh fails in a similar manner due to progress output, see [1] for details. A more narrow fix here would be to only do this in the --verbose mode, but there's no harm in setting this for everything. If we're not connected to a TTY the COLUMNS setting won't matter. See ea77e675e56 (Make "git help" react to window size correctly, 2005-12-18) and ad6c3739a33 (pager: find out the terminal width before spawning the pager, 2012-02-12) for how the TIOCGWINSZ code ended up in pager.c 1. http://lore.kernel.org/git/20210624051253.GG6312@szeder.dev Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- This replaces a more narrow fix in https://lore.kernel.org/git/patch-1.1-cba5d88ca35-20210621T070114Z-avarab@gmail.com/, which as SZEDER points out was also needed by another test using the progress.c code. t/test-lib.sh | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/t/test-lib.sh b/t/test-lib.sh index 54938c64279..1a6ca772d6c 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -406,14 +406,15 @@ LANG=C LC_ALL=C PAGER=cat TZ=UTC -export LANG LC_ALL PAGER TZ +COLUMNS=80 +export LANG LC_ALL PAGER TZ COLUMNS EDITOR=: # A call to "unset" with no arguments causes at least Solaris 10 # /usr/xpg4/bin/sh and /bin/ksh to bail out. So keep the unsets # deriving from the command substitution clustered with the other # ones. -unset VISUAL EMAIL LANGUAGE COLUMNS $("$PERL_PATH" -e ' +unset VISUAL EMAIL LANGUAGE $("$PERL_PATH" -e ' my @env = keys %ENV; my $ok = join("|", qw( TRACE -- 2.32.0.605.g06ef811e153 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] test-lib.sh: set COLUMNS=80 for --verbose repeatability 2021-06-24 10:19 ` [PATCH] test-lib.sh: set COLUMNS=80 for --verbose repeatability Ævar Arnfjörð Bjarmason @ 2021-06-24 14:50 ` Jeff King 2021-06-27 7:44 ` SZEDER Gábor 2021-06-29 11:29 ` [PATCH v2] " Ævar Arnfjörð Bjarmason 2 siblings, 0 replies; 10+ messages in thread From: Jeff King @ 2021-06-24 14:50 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Junio C Hamano, SZEDER Gábor On Thu, Jun 24, 2021 at 12:19:31PM +0200, Ævar Arnfjörð Bjarmason wrote: > Some tests will fail under --verbose because while we've unset COLUMNS > since b1d645b58ac (tests: unset COLUMNS inherited from environment, > 2012-03-27), we also look for the columns with an ioctl(.., > TIOCGWINSZ, ...) on some platforms. By setting COLUMNS again we > preempt the TIOCGWINSZ lookup in pager.c's term_columns(), it'll take > COLUMNS over TIOCGWINSZ, > > This fixes the t0500-progress-display.sh test when run as: > > ./t0500-progress-display.sh --verbose > > It broke because of a combination of the this issue and the progress > output reacting to the column width since 545dc345ebd (progress: break > too long progress bar lines, 2019-04-12). The > t5324-split-commit-graph.sh fails in a similar manner due to progress > output, see [1] for details. > > A more narrow fix here would be to only do this in the --verbose mode, > but there's no harm in setting this for everything. If we're not > connected to a TTY the COLUMNS setting won't matter. Thanks, this explanation and location make sense to me. (minor s/the this/this/ in the third paragraph above). -Peff ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] test-lib.sh: set COLUMNS=80 for --verbose repeatability 2021-06-24 10:19 ` [PATCH] test-lib.sh: set COLUMNS=80 for --verbose repeatability Ævar Arnfjörð Bjarmason 2021-06-24 14:50 ` Jeff King @ 2021-06-27 7:44 ` SZEDER Gábor 2021-06-29 1:42 ` Junio C Hamano 2021-06-29 11:29 ` [PATCH v2] " Ævar Arnfjörð Bjarmason 2 siblings, 1 reply; 10+ messages in thread From: SZEDER Gábor @ 2021-06-27 7:44 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, Junio C Hamano, Jeff King On Thu, Jun 24, 2021 at 12:19:31PM +0200, Ævar Arnfjörð Bjarmason wrote: > Some tests will fail under --verbose because while we've unset COLUMNS > since b1d645b58ac (tests: unset COLUMNS inherited from environment, > 2012-03-27), we also look for the columns with an ioctl(.., > TIOCGWINSZ, ...) on some platforms. By setting COLUMNS again we > preempt the TIOCGWINSZ lookup in pager.c's term_columns(), it'll take > COLUMNS over TIOCGWINSZ, > > This fixes the t0500-progress-display.sh test when run as: > > ./t0500-progress-display.sh --verbose > > It broke because of a combination of the this issue and the progress > output reacting to the column width since 545dc345ebd (progress: break > too long progress bar lines, 2019-04-12). The > t5324-split-commit-graph.sh fails in a similar manner due to progress > output, see [1] for details. This issue is not progress-specific. I run the test suite with 'export COLUMNS=10' in 'test-lib.sh' and got: t0500-progress-display.sh (Wstat: 256 Tests: 12 Failed: 10) Failed tests: 1-5, 7-11 t4012-diff-binary.sh (Wstat: 256 Tests: 13 Failed: 1) Failed test: 13 t4052-stat-output.sh (Wstat: 256 Tests: 78 Failed: 14) Failed tests: 3-5, 13, 17, 21, 38-41, 49, 52, 55, 57 t5510-fetch.sh (Wstat: 256 Tests: 181 Failed: 1) Failed test: 175 t5324-split-commit-graph.sh (Wstat: 256 Tests: 36 Failed: 1) Failed test: 22 t5150-request-pull.sh (Wstat: 256 Tests: 10 Failed: 1) Failed test: 6 t4016-diff-quote.sh (Wstat: 256 Tests: 5 Failed: 1) Failed test: 5 Then with COLUMNS=238: t0500-progress-display.sh (Wstat: 256 Tests: 12 Failed: 4) Failed tests: 3-6 t4012-diff-binary.sh (Wstat: 256 Tests: 13 Failed: 1) Failed test: 13 t4052-stat-output.sh (Wstat: 256 Tests: 78 Failed: 3) Failed tests: 3-5 From these only the failures in t0500 and t5324 are caused by the progress display, the rest fail with things like: --- expect 2021-06-26 17:07:58.489958439 +0000 +++ actual 2021-06-26 17:07:58.957964694 +0000 @@ -1,2 +1,2 @@ binfile | Bin 0 -> 1026 bytes - textfile | 10000 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ + textfile | 10000 ++++++++ --- expect80 2021-06-26 17:07:58.321956193 +0000 +++ actual 2021-06-26 17:07:58.333956354 +0000 @@ -1 +1 @@ - ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 + + ...aaaaaaaaaaaa | 1 + --- expect 2021-06-26 17:09:05.198849586 +0000 +++ actual 2021-06-26 17:09:05.194849532 +0000 @@ -1,2 +1,2 @@ -main -> origin/main +main -> origin/main looooooooooooong-tag -> looooooooooooong-tag --- expect 2021-06-26 17:14:31.819199273 +0000 +++ request.fuzzy 2021-06-26 17:14:31.951201026 +0000 @@ -16,4 +16,5 @@ ---------------------------------------------------------------- SHORTLOG -DIFFSTAT + ...nic.txt | 5 +++++ + 1 file changed, 5 insertions(+) --- expect 2021-06-26 16:55:41.632510754 +0000 +++ actual 2021-06-26 16:55:42.052516149 +0000 @@ -1,2 +1,2 @@ binfile | Bin 0 -> 1026 bytes - textfile | 10000 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ + textfile | 10000 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ --- expect80 2021-06-26 16:55:41.416507979 +0000 +++ actual 2021-06-26 16:55:41.436508236 +0000 @@ -1 +1 @@ - ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 + + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 + So there are more diffstat-related failures than progress-related. > A more narrow fix here would be to only do this in the --verbose mode, > but there's no harm in setting this for everything. If we're not > connected to a TTY the COLUMNS setting won't matter. This is wrong, we check the terminal width even when not connected to a TTY, therefore COLUMNS definitely matters; all the failures reported above were with '--verbose-log'. > See ea77e675e56 (Make "git help" react to window size correctly, > 2005-12-18) and ad6c3739a33 (pager: find out the terminal width before > spawning the pager, 2012-02-12) for how the TIOCGWINSZ code ended up > in pager.c > > 1. http://lore.kernel.org/git/20210624051253.GG6312@szeder.dev > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > > This replaces a more narrow fix in > https://lore.kernel.org/git/patch-1.1-cba5d88ca35-20210621T070114Z-avarab@gmail.com/, > which as SZEDER points out was also needed by another test using the > progress.c code. > > t/test-lib.sh | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/t/test-lib.sh b/t/test-lib.sh > index 54938c64279..1a6ca772d6c 100644 > --- a/t/test-lib.sh > +++ b/t/test-lib.sh > @@ -406,14 +406,15 @@ LANG=C > LC_ALL=C > PAGER=cat > TZ=UTC > -export LANG LC_ALL PAGER TZ > +COLUMNS=80 > +export LANG LC_ALL PAGER TZ COLUMNS > EDITOR=: > > # A call to "unset" with no arguments causes at least Solaris 10 > # /usr/xpg4/bin/sh and /bin/ksh to bail out. So keep the unsets > # deriving from the command substitution clustered with the other > # ones. > -unset VISUAL EMAIL LANGUAGE COLUMNS $("$PERL_PATH" -e ' > +unset VISUAL EMAIL LANGUAGE $("$PERL_PATH" -e ' > my @env = keys %ENV; > my $ok = join("|", qw( > TRACE > -- > 2.32.0.605.g06ef811e153 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] test-lib.sh: set COLUMNS=80 for --verbose repeatability 2021-06-27 7:44 ` SZEDER Gábor @ 2021-06-29 1:42 ` Junio C Hamano 0 siblings, 0 replies; 10+ messages in thread From: Junio C Hamano @ 2021-06-29 1:42 UTC (permalink / raw) To: SZEDER Gábor; +Cc: Ævar Arnfjörð Bjarmason, git, Jeff King SZEDER Gábor <szeder.dev@gmail.com> writes: > This issue is not progress-specific. As discussed elsewhere with Dscho on a Windows specific workaround, progress is not the only consumer of term_columns() and the code being "fixed" here would affect other things like "diff --stat" and hunk header title (aka funcname). Together with a typo Peff pointed out, the patch deserves an updated log message before it becomes ready. The patch text looks good. Thanks. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2] test-lib.sh: set COLUMNS=80 for --verbose repeatability 2021-06-24 10:19 ` [PATCH] test-lib.sh: set COLUMNS=80 for --verbose repeatability Ævar Arnfjörð Bjarmason 2021-06-24 14:50 ` Jeff King 2021-06-27 7:44 ` SZEDER Gábor @ 2021-06-29 11:29 ` Ævar Arnfjörð Bjarmason 2 siblings, 0 replies; 10+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-06-29 11:29 UTC (permalink / raw) To: git Cc: Junio C Hamano, SZEDER Gábor, Jeff King, Ævar Arnfjörð Bjarmason Some tests will fail under --verbose because while we've unset COLUMNS since b1d645b58ac (tests: unset COLUMNS inherited from environment, 2012-03-27), we also look for the columns with an ioctl(.., TIOCGWINSZ, ...) on some platforms. By setting COLUMNS again we preempt the TIOCGWINSZ lookup in pager.c's term_columns(), it'll take COLUMNS over TIOCGWINSZ, This fixes t0500-progress-display.sh., which broke because of a combination of the this issue and the progress output reacting to the column width since 545dc345ebd (progress: break too long progress bar lines, 2019-04-12). The t5324-split-commit-graph.sh fails in a similar manner due to progress output, see [1] for details. The issue is not specific to progress.c, the diff code also checks COLUMNS and some of its tests can be made to fail in a similar manner[2], anything that invokes a pager is potentially affected. See ea77e675e56 (Make "git help" react to window size correctly, 2005-12-18) and ad6c3739a33 (pager: find out the terminal width before spawning the pager, 2012-02-12) for how the TIOCGWINSZ code ended up in pager.c 1. http://lore.kernel.org/git/20210624051253.GG6312@szeder.dev 2. https://lore.kernel.org/git/20210627074419.GH6312@szeder.dev/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- No code changes since v1, just the suggested commit message change. Range-diff against v1: 1: 765c279312 ! 1: bd2bd89a2b test-lib.sh: set COLUMNS=80 for --verbose repeatability @@ Commit message preempt the TIOCGWINSZ lookup in pager.c's term_columns(), it'll take COLUMNS over TIOCGWINSZ, - This fixes the t0500-progress-display.sh test when run as: + This fixes t0500-progress-display.sh., which broke because of a + combination of the this issue and the progress output reacting to the + column width since 545dc345ebd (progress: break too long progress bar + lines, 2019-04-12). The t5324-split-commit-graph.sh fails in a similar + manner due to progress output, see [1] for details. - ./t0500-progress-display.sh --verbose - - It broke because of a combination of the this issue and the progress - output reacting to the column width since 545dc345ebd (progress: break - too long progress bar lines, 2019-04-12). The - t5324-split-commit-graph.sh fails in a similar manner due to progress - output, see [1] for details. - - A more narrow fix here would be to only do this in the --verbose mode, - but there's no harm in setting this for everything. If we're not - connected to a TTY the COLUMNS setting won't matter. + The issue is not specific to progress.c, the diff code also checks + COLUMNS and some of its tests can be made to fail in a similar + manner[2], anything that invokes a pager is potentially affected. See ea77e675e56 (Make "git help" react to window size correctly, 2005-12-18) and ad6c3739a33 (pager: find out the terminal width before @@ Commit message in pager.c 1. http://lore.kernel.org/git/20210624051253.GG6312@szeder.dev + 2. https://lore.kernel.org/git/20210627074419.GH6312@szeder.dev/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> t/test-lib.sh | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/t/test-lib.sh b/t/test-lib.sh index 54938c6427..1a6ca772d6 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -406,14 +406,15 @@ LANG=C LC_ALL=C PAGER=cat TZ=UTC -export LANG LC_ALL PAGER TZ +COLUMNS=80 +export LANG LC_ALL PAGER TZ COLUMNS EDITOR=: # A call to "unset" with no arguments causes at least Solaris 10 # /usr/xpg4/bin/sh and /bin/ksh to bail out. So keep the unsets # deriving from the command substitution clustered with the other # ones. -unset VISUAL EMAIL LANGUAGE COLUMNS $("$PERL_PATH" -e ' +unset VISUAL EMAIL LANGUAGE $("$PERL_PATH" -e ' my @env = keys %ENV; my $ok = join("|", qw( TRACE -- 2.32.0.615.g90fb4d7369 ^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-06-29 11:30 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-21 7:01 [PATCH] progress.c tests: fix breakage with COLUMNS != 80 Ævar Arnfjörð Bjarmason 2021-06-23 23:48 ` Jeff King 2021-06-24 5:12 ` SZEDER Gábor 2021-06-24 5:40 ` Jeff King 2021-06-24 15:05 ` Philip Oakley 2021-06-24 10:19 ` [PATCH] test-lib.sh: set COLUMNS=80 for --verbose repeatability Ævar Arnfjörð Bjarmason 2021-06-24 14:50 ` Jeff King 2021-06-27 7:44 ` SZEDER Gábor 2021-06-29 1:42 ` Junio C Hamano 2021-06-29 11:29 ` [PATCH v2] " Ævar Arnfjörð Bjarmason
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).