progress test failure on fedora34

progress test failure on fedora34

[Fabian Stelzer]

Hi,
The test t0500-progress-display.sh in current master fails on latest 
fedora34.
The break was introduced with:

83ae1edff7ee0b7674bd556955d2cf1706bddb21
ab/fix-columns-to-80-during-tests (2021-06-29) 1 commit

Kind regards,
Fabian


expecting success of 0500.3 'progress display breaks long lines #1':
     sed -e "s/Z$//" >expect <<\EOF &&
Working hard.......2.........3.........4.........5.........6:   0% 
(100/100000)<CR>
Working hard.......2.........3.........4.........5.........6:   1% 
(1000/100000)<CR>
Working hard.......2.........3.........4.........5.........6: Z
    10% (10000/100000)<CR>
   100% (100000/100000)<CR>
   100% (100000/100000), done.
EOF

     cat >in <<-\EOF &&
     progress 100
     progress 1000
     progress 10000
     progress 100000
     EOF
     test-tool progress --total=100000 \
         "Working hard.......2.........3.........4.........5.........6" \
         <in 2>stderr &&

     show_cr <stderr >out &&
     test_cmp expect out

++ sed -e 's/Z$//'
++ cat
++ test-tool progress --total=100000 'Working 
hard.......2.........3.........4.........5.........6'
++ show_cr
++ tr '\015' Q
++ sed -e 's/Q/<CR>\
/g'
++ test_cmp expect out
++ test 2 -ne 2
++ eval 'diff -u' '"$@"'
+++ diff -u expect out
--- expect    2021-07-14 12:29:49.576970165 +0000
+++ out    2021-07-14 12:29:49.580970145 +0000
@@ -1,6 +1,5 @@
  Working hard.......2.........3.........4.........5.........6:   0% 
(100/100000)<CR>
  Working hard.......2.........3.........4.........5.........6:   1% 
(1000/100000)<CR>
-Working hard.......2.........3.........4.........5.........6:
-   10% (10000/100000)<CR>
-  100% (100000/100000)<CR>
-  100% (100000/100000), done.
+Working hard.......2.........3.........4.........5.........6:  10% 
(10000/100000)<CR>
+Working hard.......2.........3.........4.........5.........6: 100% 
(100000/100000)<CR>
+Working hard.......2.........3.........4.........5.........6: 100% 
(100000/100000), done.
error: last command exited with $?=1
not ok 3 - progress display breaks long lines #1
#
#        sed -e "s/Z$//" >expect <<\EOF &&
#    Working hard.......2.........3.........4.........5.........6: 0% 
(100/100000)<CR>
#    Working hard.......2.........3.........4.........5.........6: 1% 
(1000/100000)<CR>
#    Working hard.......2.........3.........4.........5.........6: Z
#       10% (10000/100000)<CR>
#      100% (100000/100000)<CR>
#      100% (100000/100000), done.
#    EOF
#
#        cat >in <<-\EOF &&
#        progress 100
#        progress 1000
#        progress 10000
#        progress 100000
#        EOF
#        test-tool progress --total=100000 \
#            "Working 
hard.......2.........3.........4.........5.........6" \
#            <in 2>stderr &&
#
#        show_cr <stderr >out &&
#        test_cmp expect out
#

Re: progress test failure on fedora34

[Ævar Arnfjörð Bjarmason]

On Wed, Jul 14 2021, Fabian Stelzer wrote:

> Hi,
> The test t0500-progress-display.sh in current master fails on latest
> fedora34.
> The break was introduced with:
>
> 83ae1edff7ee0b7674bd556955d2cf1706bddb21
> ab/fix-columns-to-80-during-tests (2021-06-29) 1 commit
>
> Kind regards,
> Fabian

I have not been able to reproduce this, it seems the below E-Mail was
word-wrapped by your mailer, which is especially bad here since getting
to the bottom of this requires looking at the whitespace.

Is there a way you could tar that up and send it (to me personally is
fine, or some pastebin or whatever).

I am able to reproduce something that looks like this if I
s/COLUMNS=80/COLUMNS=79/g in the test-lib, but given that we set it to
80, and that the progress.c code just ends up with an
atoi(getenv("COLUMNS")), and we do our own wrapping (with no other fancy
logic) in progress.c, I'm not seeing right now how this could happen...

Re: progress test failure on fedora34

[Alex Henrie]

On Wed, Jul 14, 2021 at 9:39 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
>
> On Wed, Jul 14 2021, Fabian Stelzer wrote:
>
> > Hi,
> > The test t0500-progress-display.sh in current master fails on latest
> > fedora34.
> > The break was introduced with:
> >
> > 83ae1edff7ee0b7674bd556955d2cf1706bddb21
> > ab/fix-columns-to-80-during-tests (2021-06-29) 1 commit
> >
> > Kind regards,
> > Fabian
>
> I have not been able to reproduce this, it seems the below E-Mail was
> word-wrapped by your mailer, which is especially bad here since getting
> to the bottom of this requires looking at the whitespace.
>
> Is there a way you could tar that up and send it (to me personally is
> fine, or some pastebin or whatever).
>
> I am able to reproduce something that looks like this if I
> s/COLUMNS=80/COLUMNS=79/g in the test-lib, but given that we set it to
> 80, and that the progress.c code just ends up with an
> atoi(getenv("COLUMNS")), and we do our own wrapping (with no other fancy
> logic) in progress.c, I'm not seeing right now how this could happen...

This test also fails for me when using QTerminal or Konsole, but it
passes on XTerm and LXTerminal.

-Alex

Re: progress test failure on fedora34

[Ævar Arnfjörð Bjarmason]

On Wed, Jul 14 2021, Alex Henrie wrote:

> On Wed, Jul 14, 2021 at 9:39 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>>
>>
>> On Wed, Jul 14 2021, Fabian Stelzer wrote:
>>
>> > Hi,
>> > The test t0500-progress-display.sh in current master fails on latest
>> > fedora34.
>> > The break was introduced with:
>> >
>> > 83ae1edff7ee0b7674bd556955d2cf1706bddb21
>> > ab/fix-columns-to-80-during-tests (2021-06-29) 1 commit
>> >
>> > Kind regards,
>> > Fabian
>>
>> I have not been able to reproduce this, it seems the below E-Mail was
>> word-wrapped by your mailer, which is especially bad here since getting
>> to the bottom of this requires looking at the whitespace.
>>
>> Is there a way you could tar that up and send it (to me personally is
>> fine, or some pastebin or whatever).
>>
>> I am able to reproduce something that looks like this if I
>> s/COLUMNS=80/COLUMNS=79/g in the test-lib, but given that we set it to
>> 80, and that the progress.c code just ends up with an
>> atoi(getenv("COLUMNS")), and we do our own wrapping (with no other fancy
>> logic) in progress.c, I'm not seeing right now how this could happen...
>
> This test also fails for me when using QTerminal or Konsole, but it
> passes on XTerm and LXTerminal.

I tried this on Debian 11 with QTerminal 0.16.1 and can't reproduce it,
resized the window etc., always get COLUMNS=80 if I add some printf
debugging.

Do you mind testing with an ad-hoc patch like this on top? It will fail
right away, but should say COLUMNS = 80 in the output.

The only thing I can think of right now is that some terminals are doing
some evil trickery to LD_PRELOAD or whatever and intercept getenv() for
COLUMNS and the like, but that's an entirely unfounded hunch.

diff --git a/progress.c b/progress.c
index 680c6a8bf9..dca254b515 100644
--- a/progress.c
+++ b/progress.c
@@ -144,6 +144,7 @@ static void display(struct progress *progress, uint64_t n, const char *done)
                        size_t progress_line_len = progress->title_len +
                                                counters_sb->len + 2;
                        int cols = term_columns();
+                       fprintf(stderr, "cols = %d\n", cols);
 
                        if (progress->split) {
                                fprintf(stderr, "  %s%*s", counters_sb->buf,

Re: progress test failure on fedora34

[Jeff King]

On Sun, Jul 18, 2021 at 10:05:44AM +0200, Ævar Arnfjörð Bjarmason wrote:

> > This test also fails for me when using QTerminal or Konsole, but it
> > passes on XTerm and LXTerminal.
> 
> I tried this on Debian 11 with QTerminal 0.16.1 and can't reproduce it,
> resized the window etc., always get COLUMNS=80 if I add some printf
> debugging.
> 
> Do you mind testing with an ad-hoc patch like this on top? It will fail
> right away, but should say COLUMNS = 80 in the output.
> 
> The only thing I can think of right now is that some terminals are doing
> some evil trickery to LD_PRELOAD or whatever and intercept getenv() for
> COLUMNS and the like, but that's an entirely unfounded hunch.

That would be truly evil. :)

Another possible source of weirdness: some shells are picky about
assigning to COLUMNS, and are eager to set it themselves. E.g.:

  $ echo $COLUMNS
  119
  $ COLUMNS=80 bash -c 'echo $COLUMNS'
  80
  $ COLUMNS=80 zsh -c 'echo $COLUMNS'
  119

So zsh, even in a non-interactive shell, will set it. It does at least
accept setting it, and will preserve it in sub-shells and forks. But it
will silently ignore invalid values, instead going back to the ioctl:

  $ zsh -c 'COLUMNS=80; echo $COLUMNS; COLUMNS=foo; echo $COLUMNS'
  80
  119

mksh behaves the same way; I was tipped off to this by b082687cba
(test-lib: skip test with COLUMNS=1 under mksh, 2012-04-27).

I have trouble seeing how this could cause a problem since "80" seems
like a perfectly sensible value. And one would imagine that the same
shell is being used in all cases above (but maybe not, depending on the
configuration of the terminal programs?). But it's another possible
avenue of investigation.

> diff --git a/progress.c b/progress.c
> index 680c6a8bf9..dca254b515 100644
> --- a/progress.c
> +++ b/progress.c
> @@ -144,6 +144,7 @@ static void display(struct progress *progress, uint64_t n, const char *done)
>                         size_t progress_line_len = progress->title_len +
>                                                 counters_sb->len + 2;
>                         int cols = term_columns();
> +                       fprintf(stderr, "cols = %d\n", cols);
>  
>                         if (progress->split) {
>                                 fprintf(stderr, "  %s%*s", counters_sb->buf,

Yeah, this seems like a good start to see if the value we're getting is
bogus. Likewise, it might be interesting for term_columns() to tell us
if it's getting the value from $COLUMNS or from the ioctl (but it's hard
to believe it wouldn't be from $COLUMNS, given the code).

-Peff

Re: progress test failure on fedora34

[Alex Henrie]

On Sun, Jul 18, 2021 at 2:08 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> On Wed, Jul 14 2021, Alex Henrie wrote:
>
> > On Wed, Jul 14, 2021 at 9:39 AM Ævar Arnfjörð Bjarmason
> > <avarab@gmail.com> wrote:
> >>
> >> On Wed, Jul 14 2021, Fabian Stelzer wrote:
> >>
> >> > Hi,
> >> > The test t0500-progress-display.sh in current master fails on latest
> >> > fedora34.
> >> > The break was introduced with:
> >> >
> >> > 83ae1edff7ee0b7674bd556955d2cf1706bddb21
> >> > ab/fix-columns-to-80-during-tests (2021-06-29) 1 commit
> >> >
> >> > Kind regards,
> >> > Fabian
> >>
> >> I have not been able to reproduce this, it seems the below E-Mail was
> >> word-wrapped by your mailer, which is especially bad here since getting
> >> to the bottom of this requires looking at the whitespace.
> >>
> >> Is there a way you could tar that up and send it (to me personally is
> >> fine, or some pastebin or whatever).
> >>
> >> I am able to reproduce something that looks like this if I
> >> s/COLUMNS=80/COLUMNS=79/g in the test-lib, but given that we set it to
> >> 80, and that the progress.c code just ends up with an
> >> atoi(getenv("COLUMNS")), and we do our own wrapping (with no other fancy
> >> logic) in progress.c, I'm not seeing right now how this could happen...
> >
> > This test also fails for me when using QTerminal or Konsole, but it
> > passes on XTerm and LXTerminal.
>
> I tried this on Debian 11 with QTerminal 0.16.1 and can't reproduce it,
> resized the window etc., always get COLUMNS=80 if I add some printf
> debugging.

Actually, it looks like the difference was that I didn't resize the
XTerm or LXTerminal windows. The tests pass on all four if the
terminal emulator window is exactly 80 columns wide, and they fail on
all four if the window is any wider or narrower.

-Alex

Re: progress test failure on fedora34

[Alex Henrie]

On Mon, Jul 19, 2021 at 11:18 AM Alex Henrie <alexhenrie24@gmail.com> wrote:
>
> On Sun, Jul 18, 2021 at 2:08 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
> >
> > On Wed, Jul 14 2021, Alex Henrie wrote:
> >
> > > On Wed, Jul 14, 2021 at 9:39 AM Ævar Arnfjörð Bjarmason
> > > <avarab@gmail.com> wrote:
> > >>
> > >> On Wed, Jul 14 2021, Fabian Stelzer wrote:
> > >>
> > >> > Hi,
> > >> > The test t0500-progress-display.sh in current master fails on latest
> > >> > fedora34.
> > >> > The break was introduced with:
> > >> >
> > >> > 83ae1edff7ee0b7674bd556955d2cf1706bddb21
> > >> > ab/fix-columns-to-80-during-tests (2021-06-29) 1 commit
> > >> >
> > >> > Kind regards,
> > >> > Fabian
> > >>
> > >> I have not been able to reproduce this, it seems the below E-Mail was
> > >> word-wrapped by your mailer, which is especially bad here since getting
> > >> to the bottom of this requires looking at the whitespace.
> > >>
> > >> Is there a way you could tar that up and send it (to me personally is
> > >> fine, or some pastebin or whatever).
> > >>
> > >> I am able to reproduce something that looks like this if I
> > >> s/COLUMNS=80/COLUMNS=79/g in the test-lib, but given that we set it to
> > >> 80, and that the progress.c code just ends up with an
> > >> atoi(getenv("COLUMNS")), and we do our own wrapping (with no other fancy
> > >> logic) in progress.c, I'm not seeing right now how this could happen...
> > >
> > > This test also fails for me when using QTerminal or Konsole, but it
> > > passes on XTerm and LXTerminal.
> >
> > I tried this on Debian 11 with QTerminal 0.16.1 and can't reproduce it,
> > resized the window etc., always get COLUMNS=80 if I add some printf
> > debugging.
>
> Actually, it looks like the difference was that I didn't resize the
> XTerm or LXTerminal windows. The tests pass on all four if the
> terminal emulator window is exactly 80 columns wide, and they fail on
> all four if the window is any wider or narrower.

I have narrowed the problem down to the `tput` command in test-lib.sh:
When `tput` runs, $COLUMNS is reset to the width of the terminal
emulator window.

-Alex

Re: progress test failure on fedora34

[Felipe Contreras]

Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Jul 14 2021, Alex Henrie wrote:
> 
> > On Wed, Jul 14, 2021 at 9:39 AM Ævar Arnfjörð Bjarmason
> > <avarab@gmail.com> wrote:
> >>
> >>
> >> On Wed, Jul 14 2021, Fabian Stelzer wrote:
> >>
> >> > Hi,
> >> > The test t0500-progress-display.sh in current master fails on latest
> >> > fedora34.
> >> > The break was introduced with:
> >> >
> >> > 83ae1edff7ee0b7674bd556955d2cf1706bddb21
> >> > ab/fix-columns-to-80-during-tests (2021-06-29) 1 commit
> >> >
> >> > Kind regards,
> >> > Fabian
> >>
> >> I have not been able to reproduce this, it seems the below E-Mail was
> >> word-wrapped by your mailer, which is especially bad here since getting
> >> to the bottom of this requires looking at the whitespace.
> >>
> >> Is there a way you could tar that up and send it (to me personally is
> >> fine, or some pastebin or whatever).
> >>
> >> I am able to reproduce something that looks like this if I
> >> s/COLUMNS=80/COLUMNS=79/g in the test-lib, but given that we set it to
> >> 80, and that the progress.c code just ends up with an
> >> atoi(getenv("COLUMNS")), and we do our own wrapping (with no other fancy
> >> logic) in progress.c, I'm not seeing right now how this could happen...
> >
> > This test also fails for me when using QTerminal or Konsole, but it
> > passes on XTerm and LXTerminal.
> 
> I tried this on Debian 11 with QTerminal 0.16.1 and can't reproduce it,
> resized the window etc., always get COLUMNS=80 if I add some printf
> debugging.
> 
> Do you mind testing with an ad-hoc patch like this on top? It will fail
> right away, but should say COLUMNS = 80 in the output.
> 
> The only thing I can think of right now is that some terminals are doing
> some evil trickery to LD_PRELOAD or whatever and intercept getenv() for
> COLUMNS and the like, but that's an entirely unfounded hunch.

I'm able to reproduce this. The test fails when running directly with
bash, but not with prove.

And it seems to be a bug in bash:

  export COLUMNS=80

  echo "COLUMNS: $COLUMNS"
  cat > /tmp/expect <<EOF
  foobar
  EOF
  echo "COLUMNS: $COLUMNS"

I get:

  COLUMNS: 80
  COLUMNS: 115

Even directly in the console.

-- 
Felipe Contreras

Re: progress test failure on fedora34

[Felipe Contreras]

Felipe Contreras wrote:
> Ævar Arnfjörð Bjarmason wrote:
> > 
> > On Wed, Jul 14 2021, Alex Henrie wrote:
> > 
> > > On Wed, Jul 14, 2021 at 9:39 AM Ævar Arnfjörð Bjarmason
> > > <avarab@gmail.com> wrote:
> > >>
> > >>
> > >> On Wed, Jul 14 2021, Fabian Stelzer wrote:
> > >>
> > >> > Hi,
> > >> > The test t0500-progress-display.sh in current master fails on latest
> > >> > fedora34.
> > >> > The break was introduced with:
> > >> >
> > >> > 83ae1edff7ee0b7674bd556955d2cf1706bddb21
> > >> > ab/fix-columns-to-80-during-tests (2021-06-29) 1 commit
> > >> >
> > >> > Kind regards,
> > >> > Fabian
> > >>
> > >> I have not been able to reproduce this, it seems the below E-Mail was
> > >> word-wrapped by your mailer, which is especially bad here since getting
> > >> to the bottom of this requires looking at the whitespace.
> > >>
> > >> Is there a way you could tar that up and send it (to me personally is
> > >> fine, or some pastebin or whatever).
> > >>
> > >> I am able to reproduce something that looks like this if I
> > >> s/COLUMNS=80/COLUMNS=79/g in the test-lib, but given that we set it to
> > >> 80, and that the progress.c code just ends up with an
> > >> atoi(getenv("COLUMNS")), and we do our own wrapping (with no other fancy
> > >> logic) in progress.c, I'm not seeing right now how this could happen...
> > >
> > > This test also fails for me when using QTerminal or Konsole, but it
> > > passes on XTerm and LXTerminal.
> > 
> > I tried this on Debian 11 with QTerminal 0.16.1 and can't reproduce it,
> > resized the window etc., always get COLUMNS=80 if I add some printf
> > debugging.
> > 
> > Do you mind testing with an ad-hoc patch like this on top? It will fail
> > right away, but should say COLUMNS = 80 in the output.
> > 
> > The only thing I can think of right now is that some terminals are doing
> > some evil trickery to LD_PRELOAD or whatever and intercept getenv() for
> > COLUMNS and the like, but that's an entirely unfounded hunch.
> 
> I'm able to reproduce this. The test fails when running directly with
> bash, but not with prove.
> 
> And it seems to be a bug in bash:
> 
>   export COLUMNS=80
> 
>   echo "COLUMNS: $COLUMNS"
>   cat > /tmp/expect <<EOF
>   foobar
>   EOF
>   echo "COLUMNS: $COLUMNS"
> 
> I get:
> 
>   COLUMNS: 80
>   COLUMNS: 115
> 
> Even directly in the console.

Hmm, from man bash:

  checkwinsize
    If  set, bash checks the window size after each external (non‐builtin) com‐
    mand and, if necessary, updates the values of LINES and COLUMNS.  This  op‐
    tion is enabled by default.

Seems like since bash 5.0 this is on by default.

-- 
Felipe Contreras

Re: progress test failure on fedora34

[Alex Henrie]

On Mon, Jul 19, 2021 at 1:34 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> Felipe Contreras wrote:
> > Ævar Arnfjörð Bjarmason wrote:
> > >
> > > On Wed, Jul 14 2021, Alex Henrie wrote:
> > >
> > > > On Wed, Jul 14, 2021 at 9:39 AM Ævar Arnfjörð Bjarmason
> > > > <avarab@gmail.com> wrote:
> > > >>
> > > >>
> > > >> On Wed, Jul 14 2021, Fabian Stelzer wrote:
> > > >>
> > > >> > Hi,
> > > >> > The test t0500-progress-display.sh in current master fails on latest
> > > >> > fedora34.
> > > >> > The break was introduced with:
> > > >> >
> > > >> > 83ae1edff7ee0b7674bd556955d2cf1706bddb21
> > > >> > ab/fix-columns-to-80-during-tests (2021-06-29) 1 commit
> > > >> >
> > > >> > Kind regards,
> > > >> > Fabian
> > > >>
> > > >> I have not been able to reproduce this, it seems the below E-Mail was
> > > >> word-wrapped by your mailer, which is especially bad here since getting
> > > >> to the bottom of this requires looking at the whitespace.
> > > >>
> > > >> Is there a way you could tar that up and send it (to me personally is
> > > >> fine, or some pastebin or whatever).
> > > >>
> > > >> I am able to reproduce something that looks like this if I
> > > >> s/COLUMNS=80/COLUMNS=79/g in the test-lib, but given that we set it to
> > > >> 80, and that the progress.c code just ends up with an
> > > >> atoi(getenv("COLUMNS")), and we do our own wrapping (with no other fancy
> > > >> logic) in progress.c, I'm not seeing right now how this could happen...
> > > >
> > > > This test also fails for me when using QTerminal or Konsole, but it
> > > > passes on XTerm and LXTerminal.
> > >
> > > I tried this on Debian 11 with QTerminal 0.16.1 and can't reproduce it,
> > > resized the window etc., always get COLUMNS=80 if I add some printf
> > > debugging.
> > >
> > > Do you mind testing with an ad-hoc patch like this on top? It will fail
> > > right away, but should say COLUMNS = 80 in the output.
> > >
> > > The only thing I can think of right now is that some terminals are doing
> > > some evil trickery to LD_PRELOAD or whatever and intercept getenv() for
> > > COLUMNS and the like, but that's an entirely unfounded hunch.
> >
> > I'm able to reproduce this. The test fails when running directly with
> > bash, but not with prove.
> >
> > And it seems to be a bug in bash:
> >
> >   export COLUMNS=80
> >
> >   echo "COLUMNS: $COLUMNS"
> >   cat > /tmp/expect <<EOF
> >   foobar
> >   EOF
> >   echo "COLUMNS: $COLUMNS"
> >
> > I get:
> >
> >   COLUMNS: 80
> >   COLUMNS: 115
> >
> > Even directly in the console.
>
> Hmm, from man bash:
>
>   checkwinsize
>     If  set, bash checks the window size after each external (non‐builtin) com‐
>     mand and, if necessary, updates the values of LINES and COLUMNS.  This  op‐
>     tion is enabled by default.
>
> Seems like since bash 5.0 this is on by default.

Indeed, running `shopt -u checkwinsize` right after exporting COLUMNS
in test-lib.sh fixes the tests. Great work!

-Alex

Re: progress test failure on fedora34

[Felipe Contreras]

Alex Henrie wrote:
> On Mon, Jul 19, 2021 at 1:34 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> >
> > Felipe Contreras wrote:
> > > Ævar Arnfjörð Bjarmason wrote:
> > > >
> > > > On Wed, Jul 14 2021, Alex Henrie wrote:
> > > >
> > > > > On Wed, Jul 14, 2021 at 9:39 AM Ævar Arnfjörð Bjarmason
> > > > > <avarab@gmail.com> wrote:
> > > > >>
> > > > >>
> > > > >> On Wed, Jul 14 2021, Fabian Stelzer wrote:
> > > > >>
> > > > >> > Hi,
> > > > >> > The test t0500-progress-display.sh in current master fails on latest
> > > > >> > fedora34.
> > > > >> > The break was introduced with:
> > > > >> >
> > > > >> > 83ae1edff7ee0b7674bd556955d2cf1706bddb21
> > > > >> > ab/fix-columns-to-80-during-tests (2021-06-29) 1 commit
> > > > >> >
> > > > >> > Kind regards,
> > > > >> > Fabian
> > > > >>
> > > > >> I have not been able to reproduce this, it seems the below E-Mail was
> > > > >> word-wrapped by your mailer, which is especially bad here since getting
> > > > >> to the bottom of this requires looking at the whitespace.
> > > > >>
> > > > >> Is there a way you could tar that up and send it (to me personally is
> > > > >> fine, or some pastebin or whatever).
> > > > >>
> > > > >> I am able to reproduce something that looks like this if I
> > > > >> s/COLUMNS=80/COLUMNS=79/g in the test-lib, but given that we set it to
> > > > >> 80, and that the progress.c code just ends up with an
> > > > >> atoi(getenv("COLUMNS")), and we do our own wrapping (with no other fancy
> > > > >> logic) in progress.c, I'm not seeing right now how this could happen...
> > > > >
> > > > > This test also fails for me when using QTerminal or Konsole, but it
> > > > > passes on XTerm and LXTerminal.
> > > >
> > > > I tried this on Debian 11 with QTerminal 0.16.1 and can't reproduce it,
> > > > resized the window etc., always get COLUMNS=80 if I add some printf
> > > > debugging.
> > > >
> > > > Do you mind testing with an ad-hoc patch like this on top? It will fail
> > > > right away, but should say COLUMNS = 80 in the output.
> > > >
> > > > The only thing I can think of right now is that some terminals are doing
> > > > some evil trickery to LD_PRELOAD or whatever and intercept getenv() for
> > > > COLUMNS and the like, but that's an entirely unfounded hunch.
> > >
> > > I'm able to reproduce this. The test fails when running directly with
> > > bash, but not with prove.
> > >
> > > And it seems to be a bug in bash:
> > >
> > >   export COLUMNS=80
> > >
> > >   echo "COLUMNS: $COLUMNS"
> > >   cat > /tmp/expect <<EOF
> > >   foobar
> > >   EOF
> > >   echo "COLUMNS: $COLUMNS"
> > >
> > > I get:
> > >
> > >   COLUMNS: 80
> > >   COLUMNS: 115
> > >
> > > Even directly in the console.
> >
> > Hmm, from man bash:
> >
> >   checkwinsize
> >     If  set, bash checks the window size after each external (non‐builtin) com‐
> >     mand and, if necessary, updates the values of LINES and COLUMNS.  This  op‐
> >     tion is enabled by default.
> >
> > Seems like since bash 5.0 this is on by default.
> 
> Indeed, running `shopt -u checkwinsize` right after exporting COLUMNS
> in test-lib.sh fixes the tests. Great work!

Yeah, this fixes it, but it doesn't seem we are setting any
bash-specific options right now, and additionally I don't think bash
should be doing that in the first place. If the shell is
non-interactive, why is checkwinsize being honored?

Moreover, why does it work with prove? I'm investigating that right now,
but so far I haven't found any reason.

-- 
Felipe Contreras

Re: progress test failure on fedora34

[ZheNing Hu]

Hi, I met this bug too on my ArchLinux.

Felipe Contreras <felipe.contreras@gmail.com> 于2021年7月20日周二 上午9:04写道：

>
>
> Yeah, this fixes it, but it doesn't seem we are setting any
> bash-specific options right now, and additionally I don't think bash
> should be doing that in the first place. If the shell is
> non-interactive, why is checkwinsize being honored?
>
> Moreover, why does it work with prove? I'm investigating that right now,
> but so far I haven't found any reason.
>

I ask this question on IRC #git, and ikke said that after bisecting,
he thought that
this bug was introduced in c49a177be.

> --
> Felipe Contreras

--
ZheNing Hu

Re: progress test failure on fedora34

[Felipe Contreras]

ZheNing Hu wrote:
> Hi, I met this bug too on my ArchLinux.
> 
> Felipe Contreras <felipe.contreras@gmail.com> 于2021年7月20日周二 上午9:04写道：
> > Yeah, this fixes it, but it doesn't seem we are setting any
> > bash-specific options right now, and additionally I don't think bash
> > should be doing that in the first place. If the shell is
> > non-interactive, why is checkwinsize being honored?
> >
> > Moreover, why does it work with prove? I'm investigating that right now,
> > but so far I haven't found any reason.
> 
> I ask this question on IRC #git, and ikke said that after bisecting,
> he thought that
> this bug was introduced in c49a177be.

Yes, this was already known. The root of this thread [1] mentions
83ae1edff7, c49a177bec is the second parent of that comit.

But I don't think the bug is in git, it's in bash.

[1] https://lore.kernel.org/git/49498ed0-cfd5-2305-cee7-5c5939a19bcf@campoint.net/

-- 
Felipe Contreras

[PATCH] test-lib.sh: use GIT_TEST_COLUMNS over COLUMNS

[Ævar Arnfjörð Bjarmason]

In my c49a177beca (test-lib.sh: set COLUMNS=80 for --verbose
repeatability, 2021-06-29) the test suite started breaking on recent
versions of bash.

This is because it sets "shopt -s checkwinsize" starting with version
5.0, furthermore it started setting COLUMNS under "shopt -s
checkwinsize" for non-interactive use around version 4.3.

A narrow fix for that issue would be to add this just above our
setting of COLUMNS in test-lib.sh:

	shopt -u checkwinsize >/dev/null 2>&1

But we'd then be at the mercy of the next shell or terminal that wants
to be clever about COLUMNS.

Let's instead solve this more thoroughly. We'll now take
GIT_TEST_COLUMNS over COLUMNS, and furthermore intentionally spoil the
COLUMNS variable to break any tests that rely on it being set to a
sane value.

If something breaks because we have a codepath that's not
term_columns() checking COLUMNS we'd like to know about it, the narrow
"shopt -u checkwinsize" fix won't give us that.

The "shopt" fix won't future-poof us against other 3rd party software
changes either. If that third-party software e.g. takes TIOCGWINSZ
over columns on some platforms, our tests would be flaky and break
there even without this change.

This approach does mean that any tests of ours that expected to test
term_columns() behavior by setting COLUMNS will need to explicitly
unset GIT_TEST_COLUMNS, or set it to the empty string. I'm doing the
latter in all the tests changed here.

Reported-by: Fabian Stelzer <fabian.stelzer@campoint.net>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

I was able to reproduce the reported issue, turned out I just needed
to run it with my /bin/bash on debian (it uses dash for /bin/sh by
default).

I wrote this on the 22nd, had a patch queued up, and thought I'd sent
it, but evidently I did not. So here it is, finally.

 pager.c                       |  7 +++++++
 t/t3200-branch.sh             |  8 ++++----
 t/t4052-stat-output.sh        | 22 +++++++++++-----------
 t/t4205-log-pretty-formats.sh |  6 +++---
 t/t7004-tag.sh                |  6 +++---
 t/t7006-pager.sh              |  2 +-
 t/t7508-status.sh             |  4 ++--
 t/t9002-column.sh             | 23 ++++++++++-------------
 t/test-lib.sh                 | 13 +++++++++++--
 9 files changed, 52 insertions(+), 39 deletions(-)

diff --git a/pager.c b/pager.c
index 52f27a6765..cfcc6dc04b 100644
--- a/pager.c
+++ b/pager.c
@@ -165,6 +165,13 @@ int term_columns(void)
 	term_columns_at_startup = 80;
 	term_columns_guessed = 1;
 
+	col_string = getenv("GIT_TEST_COLUMNS");
+	if (col_string && (n_cols = atoi(col_string)) > 0) {
+		term_columns_at_startup = n_cols;
+		term_columns_guessed = 0;
+		return term_columns_at_startup;
+	}
+
 	col_string = getenv("COLUMNS");
 	if (col_string && (n_cols = atoi(col_string)) > 0) {
 		term_columns_at_startup = n_cols;
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index cc4b10236e..3e0b71a908 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -328,7 +328,7 @@ test_expect_success 'git branch --list -v with --abbrev' '
 '
 
 test_expect_success 'git branch --column' '
-	COLUMNS=81 git branch --column=column >actual &&
+	GIT_TEST_COLUMNS= COLUMNS=81 git branch --column=column >actual &&
 	cat >expect <<\EOF &&
   a/b/c   bam     foo     l     * main    n       o/p     r
   abc     bar     j/k     m/m     mb      o/o     q       topic
@@ -341,7 +341,7 @@ test_expect_success 'git branch --column with an extremely long branch name' '
 	long=z$long/$long/$long/$long &&
 	test_when_finished "git branch -d $long" &&
 	git branch $long &&
-	COLUMNS=80 git branch --column=column >actual &&
+	GIT_TEST_COLUMNS= COLUMNS=80 git branch --column=column >actual &&
 	cat >expect <<EOF &&
   a/b/c
   abc
@@ -367,7 +367,7 @@ EOF
 test_expect_success 'git branch with column.*' '
 	git config column.ui column &&
 	git config column.branch "dense" &&
-	COLUMNS=80 git branch >actual &&
+	GIT_TEST_COLUMNS= COLUMNS=80 git branch >actual &&
 	git config --unset column.branch &&
 	git config --unset column.ui &&
 	cat >expect <<\EOF &&
@@ -383,7 +383,7 @@ test_expect_success 'git branch --column -v should fail' '
 
 test_expect_success 'git branch -v with column.ui ignored' '
 	git config column.ui column &&
-	COLUMNS=80 git branch -v | cut -c -8 | sed "s/ *$//" >actual &&
+	GIT_TEST_COLUMNS= COLUMNS=80 git branch -v | cut -c -8 | sed "s/ *$//" >actual &&
 	git config --unset column.ui &&
 	cat >expect <<\EOF &&
   a/b/c
diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh
index 9eba436211..08f8615809 100755
--- a/t/t4052-stat-output.sh
+++ b/t/t4052-stat-output.sh
@@ -111,7 +111,7 @@ cat >expect72 <<'EOF'
  abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 EOF
 test_expect_success "format-patch --cover-letter ignores COLUMNS (big change)" '
-	COLUMNS=200 git format-patch -1 --stdout --cover-letter >output &&
+	GIT_TEST_COLUMNS= COLUMNS=200 git format-patch -1 --stdout --cover-letter >output &&
 	grep " | " output >actual &&
 	test_cmp expect72 actual
 '
@@ -131,7 +131,7 @@ EOF
 while read verb expect cmd args
 do
 	test_expect_success "$cmd $verb COLUMNS (big change)" '
-		COLUMNS=200 git $cmd $args >output &&
+		GIT_TEST_COLUMNS= COLUMNS=200 git $cmd $args >output &&
 		grep " | " output >actual &&
 		test_cmp "$expect" actual
 	'
@@ -139,7 +139,7 @@ do
 	case "$cmd" in diff|show) continue;; esac
 
 	test_expect_success "$cmd --graph $verb COLUMNS (big change)" '
-		COLUMNS=200 git $cmd $args --graph >output &&
+		GIT_TEST_COLUMNS= COLUMNS=200 git $cmd $args --graph >output &&
 		grep " | " output >actual &&
 		test_cmp "$expect-graph" actual
 	'
@@ -159,7 +159,7 @@ EOF
 while read verb expect cmd args
 do
 	test_expect_success "$cmd $verb not enough COLUMNS (big change)" '
-		COLUMNS=40 git $cmd $args >output &&
+		GIT_TEST_COLUMNS= COLUMNS=40 git $cmd $args >output &&
 		grep " | " output >actual &&
 		test_cmp "$expect" actual
 	'
@@ -167,7 +167,7 @@ do
 	case "$cmd" in diff|show) continue;; esac
 
 	test_expect_success "$cmd --graph $verb not enough COLUMNS (big change)" '
-		COLUMNS=40 git $cmd $args --graph >output &&
+		GIT_TEST_COLUMNS= COLUMNS=40 git $cmd $args --graph >output &&
 		grep " | " output >actual &&
 		test_cmp "$expect-graph" actual
 	'
@@ -302,7 +302,7 @@ EOF
 while read verb expect cmd args
 do
 	test_expect_success "$cmd $verb COLUMNS (long filename)" '
-		COLUMNS=200 git $cmd $args >output &&
+		GIT_TEST_COLUMNS= COLUMNS=200 git $cmd $args >output &&
 		grep " | " output >actual &&
 		test_cmp "$expect" actual
 	'
@@ -310,7 +310,7 @@ do
 	case "$cmd" in diff|show) continue;; esac
 
 	test_expect_success "$cmd --graph $verb COLUMNS (long filename)" '
-		COLUMNS=200 git $cmd $args --graph >output &&
+		GIT_TEST_COLUMNS= COLUMNS=200 git $cmd $args --graph >output &&
 		grep " | " output >actual &&
 		test_cmp "$expect-graph" actual
 	'
@@ -331,7 +331,7 @@ while read verb expect cmd args
 do
 	test_expect_success COLUMNS_CAN_BE_1 \
 		"$cmd $verb prefix greater than COLUMNS (big change)" '
-		COLUMNS=1 git $cmd $args >output &&
+		GIT_TEST_COLUMNS= COLUMNS=1 git $cmd $args >output &&
 		grep " | " output >actual &&
 		test_cmp "$expect" actual
 	'
@@ -340,7 +340,7 @@ do
 
 	test_expect_success COLUMNS_CAN_BE_1 \
 		"$cmd --graph $verb prefix greater than COLUMNS (big change)" '
-		COLUMNS=1 git $cmd $args --graph >output &&
+		GIT_TEST_COLUMNS= COLUMNS=1 git $cmd $args --graph >output &&
 		grep " | " output >actual &&
 		test_cmp "$expect-graph" actual
 	'
@@ -356,7 +356,7 @@ cat >expect <<'EOF'
 EOF
 test_expect_success 'merge --stat respects COLUMNS (big change)' '
 	git checkout -b branch HEAD^^ &&
-	COLUMNS=100 git merge --stat --no-ff main^ >output &&
+	GIT_TEST_COLUMNS= COLUMNS=100 git merge --stat --no-ff main^ >output &&
 	grep " | " output >actual &&
 	test_cmp expect actual
 '
@@ -365,7 +365,7 @@ cat >expect <<'EOF'
  aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 +++++++++++++++++++++++++++++++++++++++
 EOF
 test_expect_success 'merge --stat respects COLUMNS (long filename)' '
-	COLUMNS=100 git merge --stat --no-ff main >output &&
+	GIT_TEST_COLUMNS= COLUMNS=100 git merge --stat --no-ff main >output &&
 	grep " | " output >actual &&
 	test_cmp expect actual
 '
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 5865daa8f8..6c8e1b3f1a 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -207,7 +207,7 @@ test_expect_success 'left alignment formatting at the nth column' '
 '
 
 test_expect_success 'left alignment formatting at the nth column' '
-	COLUMNS=50 git log --pretty="tformat:%h %<|(-10)%s" >actual &&
+	GIT_TEST_COLUMNS= COLUMNS=50 git log --pretty="tformat:%h %<|(-10)%s" >actual &&
 	qz_to_tab_space <<-EOF >expected &&
 	$head1 message two                    Z
 	$head2 message one                    Z
@@ -350,7 +350,7 @@ test_expect_success 'right alignment formatting at the nth column' '
 '
 
 test_expect_success 'right alignment formatting at the nth column' '
-	COLUMNS=50 git log --pretty="tformat:%h %>|(-10)%s" >actual &&
+	GIT_TEST_COLUMNS= COLUMNS=50 git log --pretty="tformat:%h %>|(-10)%s" >actual &&
 	qz_to_tab_space <<-EOF >expected &&
 	$head1                      message two
 	$head2                      message one
@@ -450,7 +450,7 @@ test_expect_success 'center alignment formatting at the nth column' '
 '
 
 test_expect_success 'center alignment formatting at the nth column' '
-	COLUMNS=70 git log --pretty="tformat:%h %><|(-30)%s" >actual &&
+	GIT_TEST_COLUMNS= COLUMNS=70 git log --pretty="tformat:%h %><|(-30)%s" >actual &&
 	qz_to_tab_space <<-EOF >expected &&
 	$head1           message two          Z
 	$head2           message one          Z
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 2f72c5c688..1c3d8cfd16 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -372,7 +372,7 @@ test_expect_success 'tag -l <pattern> -l <pattern> works, as our buggy documenta
 '
 
 test_expect_success 'listing tags in column' '
-	COLUMNS=41 git tag -l --column=row >actual &&
+	GIT_TEST_COLUMNS= COLUMNS=41 git tag -l --column=row >actual &&
 	cat >expected <<\EOF &&
 a1      aa1     cba     t210    t211
 v0.2.1  v1.0    v1.0.1  v1.1.3
@@ -383,7 +383,7 @@ EOF
 test_expect_success 'listing tags in column with column.*' '
 	test_config column.tag row &&
 	test_config column.ui dense &&
-	COLUMNS=40 git tag -l >actual &&
+	GIT_TEST_COLUMNS= COLUMNS=40 git tag -l >actual &&
 	cat >expected <<\EOF &&
 a1      aa1   cba     t210    t211
 v0.2.1  v1.0  v1.0.1  v1.1.3
@@ -397,7 +397,7 @@ test_expect_success 'listing tag with -n --column should fail' '
 
 test_expect_success 'listing tags -n in column with column.ui ignored' '
 	test_config column.ui "row dense" &&
-	COLUMNS=40 git tag -l -n >actual &&
+	GIT_TEST_COLUMNS= COLUMNS=40 git tag -l -n >actual &&
 	cat >expected <<\EOF &&
 a1              Foo
 aa1             Foo
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 0e7cf75435..1b116366a3 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -651,7 +651,7 @@ test_expect_success TTY 'git tag with auto-columns ' '
 	cat >expect <<-\EOF &&
 	initial  one      two      three    four     five
 	EOF
-	test_terminal env PAGER="cat >actual" COLUMNS=80 \
+	test_terminal env PAGER="cat >actual" GIT_TEST_COLUMNS= COLUMNS=80 \
 		git -c column.ui=auto tag --sort=authordate &&
 	test_cmp expect actual
 '
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index 2b72451ba3..669a3c7150 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -108,13 +108,13 @@ test_expect_success 'status --column' '
 #	dir2/modified  untracked
 #
 EOF
-	COLUMNS=50 git -c status.displayCommentPrefix=true status --column="column dense" >output &&
+	GIT_TEST_COLUMNS= COLUMNS=50 git -c status.displayCommentPrefix=true status --column="column dense" >output &&
 	test_cmp expect output
 '
 
 test_expect_success 'status --column status.displayCommentPrefix=false' '
 	strip_comments expect &&
-	COLUMNS=49 git -c status.displayCommentPrefix=false status --column="column dense" >output &&
+	GIT_TEST_COLUMNS= COLUMNS=49 git -c status.displayCommentPrefix=false status --column="column dense" >output &&
 	test_cmp expect output
 '
 
diff --git a/t/t9002-column.sh b/t/t9002-column.sh
index 89983527b6..50cf3e7b42 100755
--- a/t/t9002-column.sh
+++ b/t/t9002-column.sh
@@ -46,7 +46,7 @@ test_expect_success '80 columns' '
 	cat >expected <<\EOF &&
 one    two    three  four   five   six    seven  eight  nine   ten    eleven
 EOF
-	COLUMNS=80 git column --mode=column <lista >actual &&
+	GIT_TEST_COLUMNS= COLUMNS=80 git column --mode=column <lista >actual &&
 	test_cmp expected actual
 '
 
@@ -65,7 +65,7 @@ eleven
 EOF
 
 test_expect_success COLUMNS_CAN_BE_1 'COLUMNS = 1' '
-	COLUMNS=1 git column --mode=column <lista >actual &&
+	GIT_TEST_COLUMNS= COLUMNS=1 git column --mode=column <lista >actual &&
 	test_cmp expected actual
 '
 
@@ -74,9 +74,6 @@ test_expect_success 'width = 1' '
 	test_cmp expected actual
 '
 
-COLUMNS=20
-export COLUMNS
-
 test_expect_success '20 columns' '
 	cat >expected <<\EOF &&
 one    seven
@@ -86,7 +83,7 @@ four   ten
 five   eleven
 six
 EOF
-	git column --mode=column <lista >actual &&
+	GIT_TEST_COLUMNS= COLUMNS=20 git column --mode=column <lista >actual &&
 	test_cmp expected actual
 '
 
@@ -99,7 +96,7 @@ four   ten
 five   eleven
 six
 EOF
-	git column --mode=column,nodense < lista > actual &&
+	GIT_TEST_COLUMNS= COLUMNS=20 git column --mode=column,nodense < lista > actual &&
 	test_cmp expected actual
 '
 
@@ -110,7 +107,7 @@ two   six   ten
 three seven eleven
 four  eight
 EOF
-	git column --mode=column,dense < lista > actual &&
+	GIT_TEST_COLUMNS= COLUMNS=20 git column --mode=column,dense < lista > actual &&
 	test_cmp expected actual
 '
 
@@ -123,7 +120,7 @@ four    ten
 five    eleven
 six
 EOF
-	git column --mode=column --padding 2 <lista >actual &&
+	GIT_TEST_COLUMNS= COLUMNS=20 git column --mode=column --padding 2 <lista >actual &&
 	test_cmp expected actual
 '
 
@@ -136,7 +133,7 @@ test_expect_success '20 columns, indented' '
   five   eleven
   six
 EOF
-	git column --mode=column --indent="  " <lista >actual &&
+	GIT_TEST_COLUMNS= COLUMNS=20 git column --mode=column --indent="  " <lista >actual &&
 	test_cmp expected actual
 '
 
@@ -149,7 +146,7 @@ seven  eight
 nine   ten
 eleven
 EOF
-	git column --mode=row <lista >actual &&
+	GIT_TEST_COLUMNS= COLUMNS=20 git column --mode=row <lista >actual &&
 	test_cmp expected actual
 '
 
@@ -162,7 +159,7 @@ seven  eight
 nine   ten
 eleven
 EOF
-	git column --mode=row,nodense <lista >actual &&
+	GIT_TEST_COLUMNS= COLUMNS=20 git column --mode=row,nodense <lista >actual &&
 	test_cmp expected actual
 '
 
@@ -173,7 +170,7 @@ four  five   six
 seven eight  nine
 ten   eleven
 EOF
-	git column --mode=row,dense <lista >actual &&
+	GIT_TEST_COLUMNS= COLUMNS=20 git column --mode=row,dense <lista >actual &&
 	test_cmp expected actual
 '
 
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 9e26860544..82771643ba 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -406,10 +406,19 @@ LANG=C
 LC_ALL=C
 PAGER=cat
 TZ=UTC
-COLUMNS=80
-export LANG LC_ALL PAGER TZ COLUMNS
+export LANG LC_ALL PAGER TZ
 EDITOR=:
 
+# For repeatability we need to set term_columns()'s idea of
+# columns. We do this via GIT_TEST_COLUMNS and not COLUMNS because
+# e.g. versions of bash >= 5.0 have "shopt -s checkwinsize" on by
+# default. We could do "shopt -u checkwinsize >/dev/null 2>&1" here to
+# fix that particular issue, but this is not shell specific, and
+# future-proof the tests.
+GIT_TEST_COLUMNS=80
+COLUMNS=10
+export GIT_TEST_COLUMNS COLUMNS
+
 # 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
-- 
2.32.0.988.g1a6a4b2c5f

Re: [PATCH] test-lib.sh: use GIT_TEST_COLUMNS over COLUMNS

[Jeff King]

On Tue, Jul 27, 2021 at 01:57:09AM +0200, Ævar Arnfjörð Bjarmason wrote:

> In my c49a177beca (test-lib.sh: set COLUMNS=80 for --verbose
> repeatability, 2021-06-29) the test suite started breaking on recent
> versions of bash.
> 
> This is because it sets "shopt -s checkwinsize" starting with version
> 5.0, furthermore it started setting COLUMNS under "shopt -s
> checkwinsize" for non-interactive use around version 4.3.
> 
> A narrow fix for that issue would be to add this just above our
> setting of COLUMNS in test-lib.sh:
> 
> 	shopt -u checkwinsize >/dev/null 2>&1
> 
> But we'd then be at the mercy of the next shell or terminal that wants
> to be clever about COLUMNS.
> 
> Let's instead solve this more thoroughly. We'll now take
> GIT_TEST_COLUMNS over COLUMNS, and furthermore intentionally spoil the
> COLUMNS variable to break any tests that rely on it being set to a
> sane value.
> 
> If something breaks because we have a codepath that's not
> term_columns() checking COLUMNS we'd like to know about it, the narrow
> "shopt -u checkwinsize" fix won't give us that.

I guess people running with bash won't see the test breakage (because
bash will quietly "fix" the COLUMNS setting). But enough people run with
/bin/sh that we'll eventually notice.

> This approach does mean that any tests of ours that expected to test
> term_columns() behavior by setting COLUMNS will need to explicitly
> unset GIT_TEST_COLUMNS, or set it to the empty string. I'm doing the
> latter in all the tests changed here.

This is rather ugly, and I'm not in general a fan of adding more
test-only code into the bowels of Git itself. But it may be the least
bad solution.

The only alternative I could come up with is _not_ to set COLUMNS
everywhere, but instead insist on each individual test manually doing
"COLUMNS=80 git ...". Setting it as a one-shot seems to be reliable
enough. The downside is just that various tests will need to do so. We
already have quite a few that do, but I guess anything that uses the
progress meter is now subject to it.

-Peff

Re: [PATCH] test-lib.sh: use GIT_TEST_COLUMNS over COLUMNS

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> Let's instead solve this more thoroughly. We'll now take
>> GIT_TEST_COLUMNS over COLUMNS, and furthermore intentionally spoil the
>> COLUMNS variable to break any tests that rely on it being set to a
>> sane value.
>> 
>> If something breaks because we have a codepath that's not
>> term_columns() checking COLUMNS we'd like to know about it, the narrow
>> "shopt -u checkwinsize" fix won't give us that.
>
> I guess people running with bash won't see the test breakage (because
> bash will quietly "fix" the COLUMNS setting). But enough people run with
> /bin/sh that we'll eventually notice.
>
>> This approach does mean that any tests of ours that expected to test
>> term_columns() behavior by setting COLUMNS will need to explicitly
>> unset GIT_TEST_COLUMNS, or set it to the empty string. I'm doing the
>> latter in all the tests changed here.
>
> This is rather ugly, and I'm not in general a fan of adding more
> test-only code into the bowels of Git itself. But it may be the least
> bad solution.

Yeah, this really look unsatisfactory, especially with this repeated
pattern that is hard to read:

+	GIT_TEST_COLUMNS= COLUMNS=81 git branch --column=column >actual &&
+	GIT_TEST_COLUMNS= COLUMNS=80 git branch --column=column >actual &&
+	GIT_TEST_COLUMNS= COLUMNS=80 git branch >actual &&
+	GIT_TEST_COLUMNS= COLUMNS=80 git branch -v | cut -c -8 | sed "s/ *$//" >actual &&

Perhaps with something like

	test_with_columns () {
        	local columns=$1
		shift
		GIT_TEST_COLUMNS= COLUMNS=$columns "$@"
	}

we may be able to hide the ugly implementation detail like this:

	test_with_columns 81 git branch --column=column >actual

and may become a bit more palatable?  A good thing is that this can
be done as two-step process, with its first step being 

    s/^(\s*)COLUMNS=(\d+)/$1test_with_columns $2/;

plus addition of the helper to test-lib, perhaps like so:

	test_with_columns () {
        	local columns=$1
		shift
		COLUMNS=$columns "$@"
	}

and the whole GIT_TEST_COLUMNS stuff being the second step.

[PATCH v2 0/3] test-lib.sh: use GIT_TEST_COLUMNS over COLUMNS

[Ævar Arnfjörð Bjarmason]

A v2 which should address the comments on v1, the large search-replace
of existing COLUMN uses in the tests is now a separate step.

The below range-diff is rather confusing because the v1 1/1 is now
mostly attributed to this new split-out commit, as a result it looks
like most of the commit message is being deleted, but it's in fact
there mostly in 3/3. I just re-worded the last paragraph a bit.

Ævar Arnfjörð Bjarmason (3):
  test-lib-functions.sh: rename test_must_fail_acceptable()
  test-lib-functions.sh: add a test_with_columns function
  test-lib.sh: use GIT_TEST_COLUMNS over COLUMNS

 pager.c                       |  7 +++++++
 t/t3200-branch.sh             |  8 ++++----
 t/t4052-stat-output.sh        | 22 +++++++++++-----------
 t/t4205-log-pretty-formats.sh |  6 +++---
 t/t7004-tag.sh                |  6 +++---
 t/t7006-pager.sh              |  2 +-
 t/t7508-status.sh             |  4 ++--
 t/t9002-column.sh             | 23 ++++++++++-------------
 t/test-lib-functions.sh       | 21 ++++++++++++++++++---
 t/test-lib.sh                 | 13 +++++++++++--
 10 files changed, 70 insertions(+), 42 deletions(-)

Range-diff against v1:
-:  ----------- > 1:  739457b992f test-lib-functions.sh: rename test_must_fail_acceptable()
1:  f81f3911d52 ! 2:  36c57178c55 test-lib.sh: use GIT_TEST_COLUMNS over COLUMNS
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    test-lib.sh: use GIT_TEST_COLUMNS over COLUMNS
    +    test-lib-functions.sh: add a test_with_columns function
     
    -    In my c49a177beca (test-lib.sh: set COLUMNS=80 for --verbose
    -    repeatability, 2021-06-29) the test suite started breaking on recent
    -    versions of bash.
    +    Add a helper function to wrap patterns of "COLUMNS=N <command>" as
    +    "test_with_columns N <command>". This will be used and extended in the
    +    next commit.
     
    -    This is because it sets "shopt -s checkwinsize" starting with version
    -    5.0, furthermore it started setting COLUMNS under "shopt -s
    -    checkwinsize" for non-interactive use around version 4.3.
    -
    -    A narrow fix for that issue would be to add this just above our
    -    setting of COLUMNS in test-lib.sh:
    -
    -            shopt -u checkwinsize >/dev/null 2>&1
    -
    -    But we'd then be at the mercy of the next shell or terminal that wants
    -    to be clever about COLUMNS.
    -
    -    Let's instead solve this more thoroughly. We'll now take
    -    GIT_TEST_COLUMNS over COLUMNS, and furthermore intentionally spoil the
    -    COLUMNS variable to break any tests that rely on it being set to a
    -    sane value.
    -
    -    If something breaks because we have a codepath that's not
    -    term_columns() checking COLUMNS we'd like to know about it, the narrow
    -    "shopt -u checkwinsize" fix won't give us that.
    -
    -    The "shopt" fix won't future-poof us against other 3rd party software
    -    changes either. If that third-party software e.g. takes TIOCGWINSZ
    -    over columns on some platforms, our tests would be flaky and break
    -    there even without this change.
    -
    -    This approach does mean that any tests of ours that expected to test
    -    term_columns() behavior by setting COLUMNS will need to explicitly
    -    unset GIT_TEST_COLUMNS, or set it to the empty string. I'm doing the
    -    latter in all the tests changed here.
    -
    -    Reported-by: Fabian Stelzer <fabian.stelzer@campoint.net>
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    - ## pager.c ##
    -@@ pager.c: int term_columns(void)
    - 	term_columns_at_startup = 80;
    - 	term_columns_guessed = 1;
    - 
    -+	col_string = getenv("GIT_TEST_COLUMNS");
    -+	if (col_string && (n_cols = atoi(col_string)) > 0) {
    -+		term_columns_at_startup = n_cols;
    -+		term_columns_guessed = 0;
    -+		return term_columns_at_startup;
    -+	}
    -+
    - 	col_string = getenv("COLUMNS");
    - 	if (col_string && (n_cols = atoi(col_string)) > 0) {
    - 		term_columns_at_startup = n_cols;
    -
      ## t/t3200-branch.sh ##
     @@ t/t3200-branch.sh: test_expect_success 'git branch --list -v with --abbrev' '
      '
      
      test_expect_success 'git branch --column' '
     -	COLUMNS=81 git branch --column=column >actual &&
    -+	GIT_TEST_COLUMNS= COLUMNS=81 git branch --column=column >actual &&
    ++	test_with_columns 81 git branch --column=column >actual &&
      	cat >expect <<\EOF &&
        a/b/c   bam     foo     l     * main    n       o/p     r
        abc     bar     j/k     m/m     mb      o/o     q       topic
    @@ t/t3200-branch.sh: test_expect_success 'git branch --column with an extremely lo
      	test_when_finished "git branch -d $long" &&
      	git branch $long &&
     -	COLUMNS=80 git branch --column=column >actual &&
    -+	GIT_TEST_COLUMNS= COLUMNS=80 git branch --column=column >actual &&
    ++	test_with_columns 80 git branch --column=column >actual &&
      	cat >expect <<EOF &&
        a/b/c
        abc
    @@ t/t3200-branch.sh: EOF
      	git config column.ui column &&
      	git config column.branch "dense" &&
     -	COLUMNS=80 git branch >actual &&
    -+	GIT_TEST_COLUMNS= COLUMNS=80 git branch >actual &&
    ++	test_with_columns 80 git branch >actual &&
      	git config --unset column.branch &&
      	git config --unset column.ui &&
      	cat >expect <<\EOF &&
    @@ t/t3200-branch.sh: test_expect_success 'git branch --column -v should fail' '
      test_expect_success 'git branch -v with column.ui ignored' '
      	git config column.ui column &&
     -	COLUMNS=80 git branch -v | cut -c -8 | sed "s/ *$//" >actual &&
    -+	GIT_TEST_COLUMNS= COLUMNS=80 git branch -v | cut -c -8 | sed "s/ *$//" >actual &&
    ++	test_with_columns 80 git branch -v | cut -c -8 | sed "s/ *$//" >actual &&
      	git config --unset column.ui &&
      	cat >expect <<\EOF &&
        a/b/c
    @@ t/t4052-stat-output.sh: cat >expect72 <<'EOF'
      EOF
      test_expect_success "format-patch --cover-letter ignores COLUMNS (big change)" '
     -	COLUMNS=200 git format-patch -1 --stdout --cover-letter >output &&
    -+	GIT_TEST_COLUMNS= COLUMNS=200 git format-patch -1 --stdout --cover-letter >output &&
    ++	test_with_columns 200 git format-patch -1 --stdout --cover-letter >output &&
      	grep " | " output >actual &&
      	test_cmp expect72 actual
      '
    @@ t/t4052-stat-output.sh: EOF
      do
      	test_expect_success "$cmd $verb COLUMNS (big change)" '
     -		COLUMNS=200 git $cmd $args >output &&
    -+		GIT_TEST_COLUMNS= COLUMNS=200 git $cmd $args >output &&
    ++		test_with_columns 200 git $cmd $args >output &&
      		grep " | " output >actual &&
      		test_cmp "$expect" actual
      	'
    @@ t/t4052-stat-output.sh: do
      
      	test_expect_success "$cmd --graph $verb COLUMNS (big change)" '
     -		COLUMNS=200 git $cmd $args --graph >output &&
    -+		GIT_TEST_COLUMNS= COLUMNS=200 git $cmd $args --graph >output &&
    ++		test_with_columns 200 git $cmd $args --graph >output &&
      		grep " | " output >actual &&
      		test_cmp "$expect-graph" actual
      	'
    @@ t/t4052-stat-output.sh: EOF
      do
      	test_expect_success "$cmd $verb not enough COLUMNS (big change)" '
     -		COLUMNS=40 git $cmd $args >output &&
    -+		GIT_TEST_COLUMNS= COLUMNS=40 git $cmd $args >output &&
    ++		test_with_columns 40 git $cmd $args >output &&
      		grep " | " output >actual &&
      		test_cmp "$expect" actual
      	'
    @@ t/t4052-stat-output.sh: do
      
      	test_expect_success "$cmd --graph $verb not enough COLUMNS (big change)" '
     -		COLUMNS=40 git $cmd $args --graph >output &&
    -+		GIT_TEST_COLUMNS= COLUMNS=40 git $cmd $args --graph >output &&
    ++		test_with_columns 40 git $cmd $args --graph >output &&
      		grep " | " output >actual &&
      		test_cmp "$expect-graph" actual
      	'
    @@ t/t4052-stat-output.sh: EOF
      do
      	test_expect_success "$cmd $verb COLUMNS (long filename)" '
     -		COLUMNS=200 git $cmd $args >output &&
    -+		GIT_TEST_COLUMNS= COLUMNS=200 git $cmd $args >output &&
    ++		test_with_columns 200 git $cmd $args >output &&
      		grep " | " output >actual &&
      		test_cmp "$expect" actual
      	'
    @@ t/t4052-stat-output.sh: do
      
      	test_expect_success "$cmd --graph $verb COLUMNS (long filename)" '
     -		COLUMNS=200 git $cmd $args --graph >output &&
    -+		GIT_TEST_COLUMNS= COLUMNS=200 git $cmd $args --graph >output &&
    ++		test_with_columns 200 git $cmd $args --graph >output &&
      		grep " | " output >actual &&
      		test_cmp "$expect-graph" actual
      	'
    @@ t/t4052-stat-output.sh: while read verb expect cmd args
      	test_expect_success COLUMNS_CAN_BE_1 \
      		"$cmd $verb prefix greater than COLUMNS (big change)" '
     -		COLUMNS=1 git $cmd $args >output &&
    -+		GIT_TEST_COLUMNS= COLUMNS=1 git $cmd $args >output &&
    ++		test_with_columns 1 git $cmd $args >output &&
      		grep " | " output >actual &&
      		test_cmp "$expect" actual
      	'
    @@ t/t4052-stat-output.sh: do
      	test_expect_success COLUMNS_CAN_BE_1 \
      		"$cmd --graph $verb prefix greater than COLUMNS (big change)" '
     -		COLUMNS=1 git $cmd $args --graph >output &&
    -+		GIT_TEST_COLUMNS= COLUMNS=1 git $cmd $args --graph >output &&
    ++		test_with_columns 1 git $cmd $args --graph >output &&
      		grep " | " output >actual &&
      		test_cmp "$expect-graph" actual
      	'
    @@ t/t4052-stat-output.sh: cat >expect <<'EOF'
      test_expect_success 'merge --stat respects COLUMNS (big change)' '
      	git checkout -b branch HEAD^^ &&
     -	COLUMNS=100 git merge --stat --no-ff main^ >output &&
    -+	GIT_TEST_COLUMNS= COLUMNS=100 git merge --stat --no-ff main^ >output &&
    ++	test_with_columns 100 git merge --stat --no-ff main^ >output &&
      	grep " | " output >actual &&
      	test_cmp expect actual
      '
    @@ t/t4052-stat-output.sh: cat >expect <<'EOF'
      EOF
      test_expect_success 'merge --stat respects COLUMNS (long filename)' '
     -	COLUMNS=100 git merge --stat --no-ff main >output &&
    -+	GIT_TEST_COLUMNS= COLUMNS=100 git merge --stat --no-ff main >output &&
    ++	test_with_columns 100 git merge --stat --no-ff main >output &&
      	grep " | " output >actual &&
      	test_cmp expect actual
      '
    @@ t/t4205-log-pretty-formats.sh: test_expect_success 'left alignment formatting at
      
      test_expect_success 'left alignment formatting at the nth column' '
     -	COLUMNS=50 git log --pretty="tformat:%h %<|(-10)%s" >actual &&
    -+	GIT_TEST_COLUMNS= COLUMNS=50 git log --pretty="tformat:%h %<|(-10)%s" >actual &&
    ++	test_with_columns 50 git log --pretty="tformat:%h %<|(-10)%s" >actual &&
      	qz_to_tab_space <<-EOF >expected &&
      	$head1 message two                    Z
      	$head2 message one                    Z
    @@ t/t4205-log-pretty-formats.sh: test_expect_success 'right alignment formatting a
      
      test_expect_success 'right alignment formatting at the nth column' '
     -	COLUMNS=50 git log --pretty="tformat:%h %>|(-10)%s" >actual &&
    -+	GIT_TEST_COLUMNS= COLUMNS=50 git log --pretty="tformat:%h %>|(-10)%s" >actual &&
    ++	test_with_columns 50 git log --pretty="tformat:%h %>|(-10)%s" >actual &&
      	qz_to_tab_space <<-EOF >expected &&
      	$head1                      message two
      	$head2                      message one
    @@ t/t4205-log-pretty-formats.sh: test_expect_success 'center alignment formatting
      
      test_expect_success 'center alignment formatting at the nth column' '
     -	COLUMNS=70 git log --pretty="tformat:%h %><|(-30)%s" >actual &&
    -+	GIT_TEST_COLUMNS= COLUMNS=70 git log --pretty="tformat:%h %><|(-30)%s" >actual &&
    ++	test_with_columns 70 git log --pretty="tformat:%h %><|(-30)%s" >actual &&
      	qz_to_tab_space <<-EOF >expected &&
      	$head1           message two          Z
      	$head2           message one          Z
    @@ t/t7004-tag.sh: test_expect_success 'tag -l <pattern> -l <pattern> works, as our
      
      test_expect_success 'listing tags in column' '
     -	COLUMNS=41 git tag -l --column=row >actual &&
    -+	GIT_TEST_COLUMNS= COLUMNS=41 git tag -l --column=row >actual &&
    ++	test_with_columns 41 git tag -l --column=row >actual &&
      	cat >expected <<\EOF &&
      a1      aa1     cba     t210    t211
      v0.2.1  v1.0    v1.0.1  v1.1.3
    @@ t/t7004-tag.sh: EOF
      	test_config column.tag row &&
      	test_config column.ui dense &&
     -	COLUMNS=40 git tag -l >actual &&
    -+	GIT_TEST_COLUMNS= COLUMNS=40 git tag -l >actual &&
    ++	test_with_columns 40 git tag -l >actual &&
      	cat >expected <<\EOF &&
      a1      aa1   cba     t210    t211
      v0.2.1  v1.0  v1.0.1  v1.1.3
    @@ t/t7004-tag.sh: test_expect_success 'listing tag with -n --column should fail' '
      test_expect_success 'listing tags -n in column with column.ui ignored' '
      	test_config column.ui "row dense" &&
     -	COLUMNS=40 git tag -l -n >actual &&
    -+	GIT_TEST_COLUMNS= COLUMNS=40 git tag -l -n >actual &&
    ++	test_with_columns 40 git tag -l -n >actual &&
      	cat >expected <<\EOF &&
      a1              Foo
      aa1             Foo
    @@ t/t7006-pager.sh: test_expect_success TTY 'git tag with auto-columns ' '
      	initial  one      two      three    four     five
      	EOF
     -	test_terminal env PAGER="cat >actual" COLUMNS=80 \
    -+	test_terminal env PAGER="cat >actual" GIT_TEST_COLUMNS= COLUMNS=80 \
    ++	test_with_columns 80 test_terminal env PAGER="cat >actual" \
      		git -c column.ui=auto tag --sort=authordate &&
      	test_cmp expect actual
      '
    @@ t/t7508-status.sh: test_expect_success 'status --column' '
      #
      EOF
     -	COLUMNS=50 git -c status.displayCommentPrefix=true status --column="column dense" >output &&
    -+	GIT_TEST_COLUMNS= COLUMNS=50 git -c status.displayCommentPrefix=true status --column="column dense" >output &&
    ++	test_with_columns 50 git -c status.displayCommentPrefix=true status --column="column dense" >output &&
      	test_cmp expect output
      '
      
      test_expect_success 'status --column status.displayCommentPrefix=false' '
      	strip_comments expect &&
     -	COLUMNS=49 git -c status.displayCommentPrefix=false status --column="column dense" >output &&
    -+	GIT_TEST_COLUMNS= COLUMNS=49 git -c status.displayCommentPrefix=false status --column="column dense" >output &&
    ++	test_with_columns 49 git -c status.displayCommentPrefix=false status --column="column dense" >output &&
      	test_cmp expect output
      '
      
    @@ t/t9002-column.sh: test_expect_success '80 columns' '
      one    two    three  four   five   six    seven  eight  nine   ten    eleven
      EOF
     -	COLUMNS=80 git column --mode=column <lista >actual &&
    -+	GIT_TEST_COLUMNS= COLUMNS=80 git column --mode=column <lista >actual &&
    ++	test_with_columns 80 git column --mode=column <lista >actual &&
      	test_cmp expected actual
      '
      
    @@ t/t9002-column.sh: eleven
      
      test_expect_success COLUMNS_CAN_BE_1 'COLUMNS = 1' '
     -	COLUMNS=1 git column --mode=column <lista >actual &&
    -+	GIT_TEST_COLUMNS= COLUMNS=1 git column --mode=column <lista >actual &&
    ++	test_with_columns 1 git column --mode=column <lista >actual &&
      	test_cmp expected actual
      '
      
    @@ t/t9002-column.sh: four   ten
      six
      EOF
     -	git column --mode=column <lista >actual &&
    -+	GIT_TEST_COLUMNS= COLUMNS=20 git column --mode=column <lista >actual &&
    ++	test_with_columns 20 git column --mode=column <lista >actual &&
      	test_cmp expected actual
      '
      
    @@ t/t9002-column.sh: four   ten
      six
      EOF
     -	git column --mode=column,nodense < lista > actual &&
    -+	GIT_TEST_COLUMNS= COLUMNS=20 git column --mode=column,nodense < lista > actual &&
    ++	test_with_columns 20 git column --mode=column,nodense < lista > actual &&
      	test_cmp expected actual
      '
      
    @@ t/t9002-column.sh: two   six   ten
      four  eight
      EOF
     -	git column --mode=column,dense < lista > actual &&
    -+	GIT_TEST_COLUMNS= COLUMNS=20 git column --mode=column,dense < lista > actual &&
    ++	test_with_columns 20 git column --mode=column,dense < lista > actual &&
      	test_cmp expected actual
      '
      
    @@ t/t9002-column.sh: four    ten
      six
      EOF
     -	git column --mode=column --padding 2 <lista >actual &&
    -+	GIT_TEST_COLUMNS= COLUMNS=20 git column --mode=column --padding 2 <lista >actual &&
    ++	test_with_columns 20 git column --mode=column --padding 2 <lista >actual &&
      	test_cmp expected actual
      '
      
    @@ t/t9002-column.sh: test_expect_success '20 columns, indented' '
        six
      EOF
     -	git column --mode=column --indent="  " <lista >actual &&
    -+	GIT_TEST_COLUMNS= COLUMNS=20 git column --mode=column --indent="  " <lista >actual &&
    ++	test_with_columns 20 git column --mode=column --indent="  " <lista >actual &&
      	test_cmp expected actual
      '
      
    @@ t/t9002-column.sh: seven  eight
      eleven
      EOF
     -	git column --mode=row <lista >actual &&
    -+	GIT_TEST_COLUMNS= COLUMNS=20 git column --mode=row <lista >actual &&
    ++	test_with_columns 20 git column --mode=row <lista >actual &&
      	test_cmp expected actual
      '
      
    @@ t/t9002-column.sh: seven  eight
      eleven
      EOF
     -	git column --mode=row,nodense <lista >actual &&
    -+	GIT_TEST_COLUMNS= COLUMNS=20 git column --mode=row,nodense <lista >actual &&
    ++	test_with_columns 20 git column --mode=row,nodense <lista >actual &&
      	test_cmp expected actual
      '
      
    @@ t/t9002-column.sh: four  five   six
      ten   eleven
      EOF
     -	git column --mode=row,dense <lista >actual &&
    -+	GIT_TEST_COLUMNS= COLUMNS=20 git column --mode=row,dense <lista >actual &&
    ++	test_with_columns 20 git column --mode=row,dense <lista >actual &&
      	test_cmp expected actual
      '
      
     
    - ## t/test-lib.sh ##
    -@@ t/test-lib.sh: LANG=C
    - LC_ALL=C
    - PAGER=cat
    - TZ=UTC
    --COLUMNS=80
    --export LANG LC_ALL PAGER TZ COLUMNS
    -+export LANG LC_ALL PAGER TZ
    - EDITOR=:
    - 
    -+# For repeatability we need to set term_columns()'s idea of
    -+# columns. We do this via GIT_TEST_COLUMNS and not COLUMNS because
    -+# e.g. versions of bash >= 5.0 have "shopt -s checkwinsize" on by
    -+# default. We could do "shopt -u checkwinsize >/dev/null 2>&1" here to
    -+# fix that particular issue, but this is not shell specific, and
    -+# future-proof the tests.
    -+GIT_TEST_COLUMNS=80
    -+COLUMNS=10
    -+export GIT_TEST_COLUMNS COLUMNS
    + ## t/test-lib-functions.sh ##
    +@@ t/test-lib-functions.sh: test_region () {
    + test_readlink () {
    + 	perl -le 'print readlink($_) for @ARGV' "$@"
    + }
    ++
    ++# Test a with a given number of COLUMNS in the environment.
    ++test_with_columns () {
    ++	local columns=$1
    ++	shift
     +
    - # 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
    ++	COLUMNS=$columns "$@"
    ++}
-:  ----------- > 3:  6cbbb955e9a test-lib.sh: use GIT_TEST_COLUMNS over COLUMNS
-- 
2.32.0.1070.gec115ccd780

[PATCH v2 1/3] test-lib-functions.sh: rename test_must_fail_acceptable()

[Ævar Arnfjörð Bjarmason]

The test_must_fail_acceptable() is really a generic function that can
check if something is a real "git command", e.g. "git", "test-tool"
etc. Let's rename it in preparation for using it in another test
function.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib-functions.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index e28411bb75a..37da7d9a99a 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -895,7 +895,7 @@ list_contains () {
 # accepted by test_must_fail(). If the command is run with env, the env
 # and its corresponding variable settings will be stripped before we
 # test the command being run.
-test_must_fail_acceptable () {
+is_git_command_name () {
 	if test "$1" = "env"
 	then
 		shift
@@ -943,7 +943,7 @@ test_must_fail_acceptable () {
 #     (Don't use 'success', use 'test_might_fail' instead.)
 #
 # Do not use this to run anything but "git" and other specific testable
-# commands (see test_must_fail_acceptable()).  We are not in the
+# commands (see is_git_command_name()).  We are not in the
 # business of vetting system supplied commands -- in other words, this
 # is wrong:
 #
@@ -963,7 +963,7 @@ test_must_fail () {
 		_test_ok=
 		;;
 	esac
-	if ! test_must_fail_acceptable "$@"
+	if ! is_git_command_name "$@"
 	then
 		echo >&7 "test_must_fail: only 'git' is allowed: $*"
 		return 1
-- 
2.32.0.1070.gec115ccd780

[PATCH v2 2/3] test-lib-functions.sh: add a test_with_columns function

[Ævar Arnfjörð Bjarmason]

Add a helper function to wrap patterns of "COLUMNS=N <command>" as
"test_with_columns N <command>". This will be used and extended in the
next commit.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t3200-branch.sh             |  8 ++++----
 t/t4052-stat-output.sh        | 22 +++++++++++-----------
 t/t4205-log-pretty-formats.sh |  6 +++---
 t/t7004-tag.sh                |  6 +++---
 t/t7006-pager.sh              |  2 +-
 t/t7508-status.sh             |  4 ++--
 t/t9002-column.sh             | 23 ++++++++++-------------
 t/test-lib-functions.sh       |  8 ++++++++
 8 files changed, 42 insertions(+), 37 deletions(-)

diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index cc4b10236e2..e568156ca83 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -328,7 +328,7 @@ test_expect_success 'git branch --list -v with --abbrev' '
 '
 
 test_expect_success 'git branch --column' '
-	COLUMNS=81 git branch --column=column >actual &&
+	test_with_columns 81 git branch --column=column >actual &&
 	cat >expect <<\EOF &&
   a/b/c   bam     foo     l     * main    n       o/p     r
   abc     bar     j/k     m/m     mb      o/o     q       topic
@@ -341,7 +341,7 @@ test_expect_success 'git branch --column with an extremely long branch name' '
 	long=z$long/$long/$long/$long &&
 	test_when_finished "git branch -d $long" &&
 	git branch $long &&
-	COLUMNS=80 git branch --column=column >actual &&
+	test_with_columns 80 git branch --column=column >actual &&
 	cat >expect <<EOF &&
   a/b/c
   abc
@@ -367,7 +367,7 @@ EOF
 test_expect_success 'git branch with column.*' '
 	git config column.ui column &&
 	git config column.branch "dense" &&
-	COLUMNS=80 git branch >actual &&
+	test_with_columns 80 git branch >actual &&
 	git config --unset column.branch &&
 	git config --unset column.ui &&
 	cat >expect <<\EOF &&
@@ -383,7 +383,7 @@ test_expect_success 'git branch --column -v should fail' '
 
 test_expect_success 'git branch -v with column.ui ignored' '
 	git config column.ui column &&
-	COLUMNS=80 git branch -v | cut -c -8 | sed "s/ *$//" >actual &&
+	test_with_columns 80 git branch -v | cut -c -8 | sed "s/ *$//" >actual &&
 	git config --unset column.ui &&
 	cat >expect <<\EOF &&
   a/b/c
diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh
index 9eba436211f..3a91df50dd8 100755
--- a/t/t4052-stat-output.sh
+++ b/t/t4052-stat-output.sh
@@ -111,7 +111,7 @@ cat >expect72 <<'EOF'
  abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 EOF
 test_expect_success "format-patch --cover-letter ignores COLUMNS (big change)" '
-	COLUMNS=200 git format-patch -1 --stdout --cover-letter >output &&
+	test_with_columns 200 git format-patch -1 --stdout --cover-letter >output &&
 	grep " | " output >actual &&
 	test_cmp expect72 actual
 '
@@ -131,7 +131,7 @@ EOF
 while read verb expect cmd args
 do
 	test_expect_success "$cmd $verb COLUMNS (big change)" '
-		COLUMNS=200 git $cmd $args >output &&
+		test_with_columns 200 git $cmd $args >output &&
 		grep " | " output >actual &&
 		test_cmp "$expect" actual
 	'
@@ -139,7 +139,7 @@ do
 	case "$cmd" in diff|show) continue;; esac
 
 	test_expect_success "$cmd --graph $verb COLUMNS (big change)" '
-		COLUMNS=200 git $cmd $args --graph >output &&
+		test_with_columns 200 git $cmd $args --graph >output &&
 		grep " | " output >actual &&
 		test_cmp "$expect-graph" actual
 	'
@@ -159,7 +159,7 @@ EOF
 while read verb expect cmd args
 do
 	test_expect_success "$cmd $verb not enough COLUMNS (big change)" '
-		COLUMNS=40 git $cmd $args >output &&
+		test_with_columns 40 git $cmd $args >output &&
 		grep " | " output >actual &&
 		test_cmp "$expect" actual
 	'
@@ -167,7 +167,7 @@ do
 	case "$cmd" in diff|show) continue;; esac
 
 	test_expect_success "$cmd --graph $verb not enough COLUMNS (big change)" '
-		COLUMNS=40 git $cmd $args --graph >output &&
+		test_with_columns 40 git $cmd $args --graph >output &&
 		grep " | " output >actual &&
 		test_cmp "$expect-graph" actual
 	'
@@ -302,7 +302,7 @@ EOF
 while read verb expect cmd args
 do
 	test_expect_success "$cmd $verb COLUMNS (long filename)" '
-		COLUMNS=200 git $cmd $args >output &&
+		test_with_columns 200 git $cmd $args >output &&
 		grep " | " output >actual &&
 		test_cmp "$expect" actual
 	'
@@ -310,7 +310,7 @@ do
 	case "$cmd" in diff|show) continue;; esac
 
 	test_expect_success "$cmd --graph $verb COLUMNS (long filename)" '
-		COLUMNS=200 git $cmd $args --graph >output &&
+		test_with_columns 200 git $cmd $args --graph >output &&
 		grep " | " output >actual &&
 		test_cmp "$expect-graph" actual
 	'
@@ -331,7 +331,7 @@ while read verb expect cmd args
 do
 	test_expect_success COLUMNS_CAN_BE_1 \
 		"$cmd $verb prefix greater than COLUMNS (big change)" '
-		COLUMNS=1 git $cmd $args >output &&
+		test_with_columns 1 git $cmd $args >output &&
 		grep " | " output >actual &&
 		test_cmp "$expect" actual
 	'
@@ -340,7 +340,7 @@ do
 
 	test_expect_success COLUMNS_CAN_BE_1 \
 		"$cmd --graph $verb prefix greater than COLUMNS (big change)" '
-		COLUMNS=1 git $cmd $args --graph >output &&
+		test_with_columns 1 git $cmd $args --graph >output &&
 		grep " | " output >actual &&
 		test_cmp "$expect-graph" actual
 	'
@@ -356,7 +356,7 @@ cat >expect <<'EOF'
 EOF
 test_expect_success 'merge --stat respects COLUMNS (big change)' '
 	git checkout -b branch HEAD^^ &&
-	COLUMNS=100 git merge --stat --no-ff main^ >output &&
+	test_with_columns 100 git merge --stat --no-ff main^ >output &&
 	grep " | " output >actual &&
 	test_cmp expect actual
 '
@@ -365,7 +365,7 @@ cat >expect <<'EOF'
  aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 +++++++++++++++++++++++++++++++++++++++
 EOF
 test_expect_success 'merge --stat respects COLUMNS (long filename)' '
-	COLUMNS=100 git merge --stat --no-ff main >output &&
+	test_with_columns 100 git merge --stat --no-ff main >output &&
 	grep " | " output >actual &&
 	test_cmp expect actual
 '
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 5865daa8f8d..a035f749537 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -207,7 +207,7 @@ test_expect_success 'left alignment formatting at the nth column' '
 '
 
 test_expect_success 'left alignment formatting at the nth column' '
-	COLUMNS=50 git log --pretty="tformat:%h %<|(-10)%s" >actual &&
+	test_with_columns 50 git log --pretty="tformat:%h %<|(-10)%s" >actual &&
 	qz_to_tab_space <<-EOF >expected &&
 	$head1 message two                    Z
 	$head2 message one                    Z
@@ -350,7 +350,7 @@ test_expect_success 'right alignment formatting at the nth column' '
 '
 
 test_expect_success 'right alignment formatting at the nth column' '
-	COLUMNS=50 git log --pretty="tformat:%h %>|(-10)%s" >actual &&
+	test_with_columns 50 git log --pretty="tformat:%h %>|(-10)%s" >actual &&
 	qz_to_tab_space <<-EOF >expected &&
 	$head1                      message two
 	$head2                      message one
@@ -450,7 +450,7 @@ test_expect_success 'center alignment formatting at the nth column' '
 '
 
 test_expect_success 'center alignment formatting at the nth column' '
-	COLUMNS=70 git log --pretty="tformat:%h %><|(-30)%s" >actual &&
+	test_with_columns 70 git log --pretty="tformat:%h %><|(-30)%s" >actual &&
 	qz_to_tab_space <<-EOF >expected &&
 	$head1           message two          Z
 	$head2           message one          Z
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 2f72c5c6883..e893c43d705 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -372,7 +372,7 @@ test_expect_success 'tag -l <pattern> -l <pattern> works, as our buggy documenta
 '
 
 test_expect_success 'listing tags in column' '
-	COLUMNS=41 git tag -l --column=row >actual &&
+	test_with_columns 41 git tag -l --column=row >actual &&
 	cat >expected <<\EOF &&
 a1      aa1     cba     t210    t211
 v0.2.1  v1.0    v1.0.1  v1.1.3
@@ -383,7 +383,7 @@ EOF
 test_expect_success 'listing tags in column with column.*' '
 	test_config column.tag row &&
 	test_config column.ui dense &&
-	COLUMNS=40 git tag -l >actual &&
+	test_with_columns 40 git tag -l >actual &&
 	cat >expected <<\EOF &&
 a1      aa1   cba     t210    t211
 v0.2.1  v1.0  v1.0.1  v1.1.3
@@ -397,7 +397,7 @@ test_expect_success 'listing tag with -n --column should fail' '
 
 test_expect_success 'listing tags -n in column with column.ui ignored' '
 	test_config column.ui "row dense" &&
-	COLUMNS=40 git tag -l -n >actual &&
+	test_with_columns 40 git tag -l -n >actual &&
 	cat >expected <<\EOF &&
 a1              Foo
 aa1             Foo
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 0e7cf75435e..5189699debf 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -651,7 +651,7 @@ test_expect_success TTY 'git tag with auto-columns ' '
 	cat >expect <<-\EOF &&
 	initial  one      two      three    four     five
 	EOF
-	test_terminal env PAGER="cat >actual" COLUMNS=80 \
+	test_with_columns 80 test_terminal env PAGER="cat >actual" \
 		git -c column.ui=auto tag --sort=authordate &&
 	test_cmp expect actual
 '
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index 2b72451ba3e..808b8f50576 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -108,13 +108,13 @@ test_expect_success 'status --column' '
 #	dir2/modified  untracked
 #
 EOF
-	COLUMNS=50 git -c status.displayCommentPrefix=true status --column="column dense" >output &&
+	test_with_columns 50 git -c status.displayCommentPrefix=true status --column="column dense" >output &&
 	test_cmp expect output
 '
 
 test_expect_success 'status --column status.displayCommentPrefix=false' '
 	strip_comments expect &&
-	COLUMNS=49 git -c status.displayCommentPrefix=false status --column="column dense" >output &&
+	test_with_columns 49 git -c status.displayCommentPrefix=false status --column="column dense" >output &&
 	test_cmp expect output
 '
 
diff --git a/t/t9002-column.sh b/t/t9002-column.sh
index 89983527b62..9151d69bcf1 100755
--- a/t/t9002-column.sh
+++ b/t/t9002-column.sh
@@ -46,7 +46,7 @@ test_expect_success '80 columns' '
 	cat >expected <<\EOF &&
 one    two    three  four   five   six    seven  eight  nine   ten    eleven
 EOF
-	COLUMNS=80 git column --mode=column <lista >actual &&
+	test_with_columns 80 git column --mode=column <lista >actual &&
 	test_cmp expected actual
 '
 
@@ -65,7 +65,7 @@ eleven
 EOF
 
 test_expect_success COLUMNS_CAN_BE_1 'COLUMNS = 1' '
-	COLUMNS=1 git column --mode=column <lista >actual &&
+	test_with_columns 1 git column --mode=column <lista >actual &&
 	test_cmp expected actual
 '
 
@@ -74,9 +74,6 @@ test_expect_success 'width = 1' '
 	test_cmp expected actual
 '
 
-COLUMNS=20
-export COLUMNS
-
 test_expect_success '20 columns' '
 	cat >expected <<\EOF &&
 one    seven
@@ -86,7 +83,7 @@ four   ten
 five   eleven
 six
 EOF
-	git column --mode=column <lista >actual &&
+	test_with_columns 20 git column --mode=column <lista >actual &&
 	test_cmp expected actual
 '
 
@@ -99,7 +96,7 @@ four   ten
 five   eleven
 six
 EOF
-	git column --mode=column,nodense < lista > actual &&
+	test_with_columns 20 git column --mode=column,nodense < lista > actual &&
 	test_cmp expected actual
 '
 
@@ -110,7 +107,7 @@ two   six   ten
 three seven eleven
 four  eight
 EOF
-	git column --mode=column,dense < lista > actual &&
+	test_with_columns 20 git column --mode=column,dense < lista > actual &&
 	test_cmp expected actual
 '
 
@@ -123,7 +120,7 @@ four    ten
 five    eleven
 six
 EOF
-	git column --mode=column --padding 2 <lista >actual &&
+	test_with_columns 20 git column --mode=column --padding 2 <lista >actual &&
 	test_cmp expected actual
 '
 
@@ -136,7 +133,7 @@ test_expect_success '20 columns, indented' '
   five   eleven
   six
 EOF
-	git column --mode=column --indent="  " <lista >actual &&
+	test_with_columns 20 git column --mode=column --indent="  " <lista >actual &&
 	test_cmp expected actual
 '
 
@@ -149,7 +146,7 @@ seven  eight
 nine   ten
 eleven
 EOF
-	git column --mode=row <lista >actual &&
+	test_with_columns 20 git column --mode=row <lista >actual &&
 	test_cmp expected actual
 '
 
@@ -162,7 +159,7 @@ seven  eight
 nine   ten
 eleven
 EOF
-	git column --mode=row,nodense <lista >actual &&
+	test_with_columns 20 git column --mode=row,nodense <lista >actual &&
 	test_cmp expected actual
 '
 
@@ -173,7 +170,7 @@ four  five   six
 seven eight  nine
 ten   eleven
 EOF
-	git column --mode=row,dense <lista >actual &&
+	test_with_columns 20 git column --mode=row,dense <lista >actual &&
 	test_cmp expected actual
 '
 
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 37da7d9a99a..536339faaa2 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1718,3 +1718,11 @@ test_region () {
 test_readlink () {
 	perl -le 'print readlink($_) for @ARGV' "$@"
 }
+
+# Test a with a given number of COLUMNS in the environment.
+test_with_columns () {
+	local columns=$1
+	shift
+
+	COLUMNS=$columns "$@"
+}
-- 
2.32.0.1070.gec115ccd780

[PATCH v2 3/3] test-lib.sh: use GIT_TEST_COLUMNS over COLUMNS

[Ævar Arnfjörð Bjarmason]

In my c49a177beca (test-lib.sh: set COLUMNS=80 for --verbose
repeatability, 2021-06-29) the test suite started breaking on recent
versions of bash.

This is because it sets "shopt -s checkwinsize" starting with version
5.0, furthermore it started setting COLUMNS under "shopt -s
checkwinsize" for non-interactive use around version 4.3.

A narrow fix for that issue would be to add this just above our
setting of COLUMNS in test-lib.sh:

	shopt -u checkwinsize >/dev/null 2>&1

But we'd then be at the mercy of the next shell or terminal that wants
to be clever about COLUMNS.

Let's instead solve this more thoroughly. We'll now take
GIT_TEST_COLUMNS over COLUMNS, and furthermore intentionally spoil the
COLUMNS variable to break any tests that rely on it being set to a
sane value.

If something breaks because we have a codepath that's not
term_columns() checking COLUMNS we'd like to know about it, the narrow
"shopt -u checkwinsize" fix won't give us that.

The "shopt" fix won't future-poof us against other 3rd party software
changes either. If that third-party software e.g. takes TIOCGWINSZ
over columns on some platforms, our tests would be flaky and break
there even without this change.

This approach does mean that any tests of ours that expected to test
term_columns() behavior by setting COLUMNS will need to explicitly
unset GIT_TEST_COLUMNS, or set it to the empty string. Let's do that
in the new test_with_columns() helper, which we previously changed all
the tests that set COLUMNS to use.

Reported-by: Fabian Stelzer <fabian.stelzer@campoint.net>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 pager.c                 |  7 +++++++
 t/test-lib-functions.sh |  7 +++++++
 t/test-lib.sh           | 13 +++++++++++--
 3 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/pager.c b/pager.c
index 52f27a6765c..cfcc6dc04bd 100644
--- a/pager.c
+++ b/pager.c
@@ -165,6 +165,13 @@ int term_columns(void)
 	term_columns_at_startup = 80;
 	term_columns_guessed = 1;
 
+	col_string = getenv("GIT_TEST_COLUMNS");
+	if (col_string && (n_cols = atoi(col_string)) > 0) {
+		term_columns_at_startup = n_cols;
+		term_columns_guessed = 0;
+		return term_columns_at_startup;
+	}
+
 	col_string = getenv("COLUMNS");
 	if (col_string && (n_cols = atoi(col_string)) > 0) {
 		term_columns_at_startup = n_cols;
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 536339faaa2..959354e0c6e 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1724,5 +1724,12 @@ test_with_columns () {
 	local columns=$1
 	shift
 
+	if ! is_git_command_name "$@"
+	then
+		echo >&7 "test_with_columns: only 'git' is allowed: $*"
+		return 1
+	fi
+
+	GIT_TEST_COLUMNS= \
 	COLUMNS=$columns "$@"
 }
diff --git a/t/test-lib.sh b/t/test-lib.sh
index da13190970f..4bb231e5729 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -415,10 +415,19 @@ LANG=C
 LC_ALL=C
 PAGER=cat
 TZ=UTC
-COLUMNS=80
-export LANG LC_ALL PAGER TZ COLUMNS
+export LANG LC_ALL PAGER TZ
 EDITOR=:
 
+# For repeatability we need to set term_columns()'s idea of
+# columns. We do this via GIT_TEST_COLUMNS and not COLUMNS because
+# e.g. versions of bash >= 5.0 have "shopt -s checkwinsize" on by
+# default. We could do "shopt -u checkwinsize >/dev/null 2>&1" here to
+# fix that particular issue, but this is not shell specific, and
+# future-proof the tests.
+GIT_TEST_COLUMNS=80
+COLUMNS=10
+export GIT_TEST_COLUMNS COLUMNS
+
 # 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
-- 
2.32.0.1070.gec115ccd780

Re: [PATCH v2 2/3] test-lib-functions.sh: add a test_with_columns function

[SZEDER Gábor]

On Mon, Aug 02, 2021 at 03:46:27PM +0200, Ævar Arnfjörð Bjarmason wrote:
> Add a helper function to wrap patterns of "COLUMNS=N <command>" as
> "test_with_columns N <command>". This will be used and extended in the
> next commit.


> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 37da7d9a99a..536339faaa2 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -1718,3 +1718,11 @@ test_region () {
>  test_readlink () {
>  	perl -le 'print readlink($_) for @ARGV' "$@"
>  }
> +
> +# Test a with a given number of COLUMNS in the environment.
> +test_with_columns () {
> +	local columns=$1
> +	shift
> +
> +	COLUMNS=$columns "$@"
> +}

This function needs some redirections to separate the tested command's
standard error from the function's '-x' trace, see a5bf824f3b (t:
prevent '-x' tracing from interfering with test helpers' stderr,
2018-02-25).

Re: [PATCH v2 2/3] test-lib-functions.sh: add a test_with_columns function

[Eric Sunshine]

On Mon, Aug 2, 2021 at 1:14 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> On Mon, Aug 02, 2021 at 03:46:27PM +0200, Ævar Arnfjörð Bjarmason wrote:
> > +# Test a with a given number of COLUMNS in the environment.
> > +test_with_columns () {
> > +     local columns=$1
> > +     shift
> > +
> > +     COLUMNS=$columns "$@"
> > +}
>
> This function needs some redirections to separate the tested command's
> standard error from the function's '-x' trace, see a5bf824f3b (t:
> prevent '-x' tracing from interfering with test helpers' stderr,
> 2018-02-25).

This is also potentially problematic when a test is expected to fail,
in which case we avoid the:

    FOO=bar command

idiom and instead use:

    test_must_fail env FOO=bar command

[PATCH v3 0/3] test-lib.sh: use GIT_TEST_COLUMNS over COLUMNS

[Ævar Arnfjörð Bjarmason]

This v3 addresses SZEDER's note in <20210802171429.GC23408@szeder.dev>
that we were missing redirection. The "is_git_command_name" check has
also been moved one commit earlier, as it should.

Ævar Arnfjörð Bjarmason (3):
  test-lib-functions.sh: rename test_must_fail_acceptable()
  test-lib-functions.sh: add a test_with_columns function
  test-lib.sh: use GIT_TEST_COLUMNS over COLUMNS

 pager.c                       |  7 +++++++
 t/t3200-branch.sh             |  8 ++++----
 t/t4052-stat-output.sh        | 22 +++++++++++-----------
 t/t4205-log-pretty-formats.sh |  6 +++---
 t/t7004-tag.sh                |  6 +++---
 t/t7006-pager.sh              |  2 +-
 t/t7508-status.sh             |  4 ++--
 t/t9002-column.sh             | 23 ++++++++++-------------
 t/test-lib-functions.sh       | 21 ++++++++++++++++++---
 t/test-lib.sh                 | 13 +++++++++++--
 10 files changed, 70 insertions(+), 42 deletions(-)

Range-diff against v2:
1:  739457b992f = 1:  f45590a76d5 test-lib-functions.sh: rename test_must_fail_acceptable()
2:  36c57178c55 ! 2:  53e6e25ece6 test-lib-functions.sh: add a test_with_columns function
    @@ t/test-lib-functions.sh: test_region () {
     +	local columns=$1
     +	shift
     +
    -+	COLUMNS=$columns "$@"
    -+}
    ++	if ! is_git_command_name "$@"
    ++	then
    ++		echo >&7 "test_with_columns: only 'git' is allowed: $*"
    ++		return 1
    ++	fi
    ++
    ++	COLUMNS=$columns "$@" 2>&7
    ++} 7>&2 2>&4
3:  6cbbb955e9a ! 3:  74acba0f9ca test-lib.sh: use GIT_TEST_COLUMNS over COLUMNS
    @@ pager.c: int term_columns(void)
     
      ## t/test-lib-functions.sh ##
     @@ t/test-lib-functions.sh: test_with_columns () {
    - 	local columns=$1
    - 	shift
    + 		return 1
    + 	fi
      
    -+	if ! is_git_command_name "$@"
    -+	then
    -+		echo >&7 "test_with_columns: only 'git' is allowed: $*"
    -+		return 1
    -+	fi
    -+
     +	GIT_TEST_COLUMNS= \
    - 	COLUMNS=$columns "$@"
    - }
    + 	COLUMNS=$columns "$@" 2>&7
    + } 7>&2 2>&4
     
      ## t/test-lib.sh ##
     @@ t/test-lib.sh: LANG=C
-- 
2.33.0.rc0.597.gc569a812f0a

[PATCH v3 1/3] test-lib-functions.sh: rename test_must_fail_acceptable()

[Ævar Arnfjörð Bjarmason]

The test_must_fail_acceptable() is really a generic function that can
check if something is a real "git command", e.g. "git", "test-tool"
etc. Let's rename it in preparation for using it in another test
function.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib-functions.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index e28411bb75a..37da7d9a99a 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -895,7 +895,7 @@ list_contains () {
 # accepted by test_must_fail(). If the command is run with env, the env
 # and its corresponding variable settings will be stripped before we
 # test the command being run.
-test_must_fail_acceptable () {
+is_git_command_name () {
 	if test "$1" = "env"
 	then
 		shift
@@ -943,7 +943,7 @@ test_must_fail_acceptable () {
 #     (Don't use 'success', use 'test_might_fail' instead.)
 #
 # Do not use this to run anything but "git" and other specific testable
-# commands (see test_must_fail_acceptable()).  We are not in the
+# commands (see is_git_command_name()).  We are not in the
 # business of vetting system supplied commands -- in other words, this
 # is wrong:
 #
@@ -963,7 +963,7 @@ test_must_fail () {
 		_test_ok=
 		;;
 	esac
-	if ! test_must_fail_acceptable "$@"
+	if ! is_git_command_name "$@"
 	then
 		echo >&7 "test_must_fail: only 'git' is allowed: $*"
 		return 1
-- 
2.33.0.rc0.597.gc569a812f0a

[PATCH v3 2/3] test-lib-functions.sh: add a test_with_columns function

[Ævar Arnfjörð Bjarmason]

Add a helper function to wrap patterns of "COLUMNS=N <command>" as
"test_with_columns N <command>". This will be used and extended in the
next commit.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t3200-branch.sh             |  8 ++++----
 t/t4052-stat-output.sh        | 22 +++++++++++-----------
 t/t4205-log-pretty-formats.sh |  6 +++---
 t/t7004-tag.sh                |  6 +++---
 t/t7006-pager.sh              |  2 +-
 t/t7508-status.sh             |  4 ++--
 t/t9002-column.sh             | 23 ++++++++++-------------
 t/test-lib-functions.sh       | 14 ++++++++++++++
 8 files changed, 48 insertions(+), 37 deletions(-)

diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index cc4b10236e2..e568156ca83 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -328,7 +328,7 @@ test_expect_success 'git branch --list -v with --abbrev' '
 '
 
 test_expect_success 'git branch --column' '
-	COLUMNS=81 git branch --column=column >actual &&
+	test_with_columns 81 git branch --column=column >actual &&
 	cat >expect <<\EOF &&
   a/b/c   bam     foo     l     * main    n       o/p     r
   abc     bar     j/k     m/m     mb      o/o     q       topic
@@ -341,7 +341,7 @@ test_expect_success 'git branch --column with an extremely long branch name' '
 	long=z$long/$long/$long/$long &&
 	test_when_finished "git branch -d $long" &&
 	git branch $long &&
-	COLUMNS=80 git branch --column=column >actual &&
+	test_with_columns 80 git branch --column=column >actual &&
 	cat >expect <<EOF &&
   a/b/c
   abc
@@ -367,7 +367,7 @@ EOF
 test_expect_success 'git branch with column.*' '
 	git config column.ui column &&
 	git config column.branch "dense" &&
-	COLUMNS=80 git branch >actual &&
+	test_with_columns 80 git branch >actual &&
 	git config --unset column.branch &&
 	git config --unset column.ui &&
 	cat >expect <<\EOF &&
@@ -383,7 +383,7 @@ test_expect_success 'git branch --column -v should fail' '
 
 test_expect_success 'git branch -v with column.ui ignored' '
 	git config column.ui column &&
-	COLUMNS=80 git branch -v | cut -c -8 | sed "s/ *$//" >actual &&
+	test_with_columns 80 git branch -v | cut -c -8 | sed "s/ *$//" >actual &&
 	git config --unset column.ui &&
 	cat >expect <<\EOF &&
   a/b/c
diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh
index 9eba436211f..3a91df50dd8 100755
--- a/t/t4052-stat-output.sh
+++ b/t/t4052-stat-output.sh
@@ -111,7 +111,7 @@ cat >expect72 <<'EOF'
  abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 EOF
 test_expect_success "format-patch --cover-letter ignores COLUMNS (big change)" '
-	COLUMNS=200 git format-patch -1 --stdout --cover-letter >output &&
+	test_with_columns 200 git format-patch -1 --stdout --cover-letter >output &&
 	grep " | " output >actual &&
 	test_cmp expect72 actual
 '
@@ -131,7 +131,7 @@ EOF
 while read verb expect cmd args
 do
 	test_expect_success "$cmd $verb COLUMNS (big change)" '
-		COLUMNS=200 git $cmd $args >output &&
+		test_with_columns 200 git $cmd $args >output &&
 		grep " | " output >actual &&
 		test_cmp "$expect" actual
 	'
@@ -139,7 +139,7 @@ do
 	case "$cmd" in diff|show) continue;; esac
 
 	test_expect_success "$cmd --graph $verb COLUMNS (big change)" '
-		COLUMNS=200 git $cmd $args --graph >output &&
+		test_with_columns 200 git $cmd $args --graph >output &&
 		grep " | " output >actual &&
 		test_cmp "$expect-graph" actual
 	'
@@ -159,7 +159,7 @@ EOF
 while read verb expect cmd args
 do
 	test_expect_success "$cmd $verb not enough COLUMNS (big change)" '
-		COLUMNS=40 git $cmd $args >output &&
+		test_with_columns 40 git $cmd $args >output &&
 		grep " | " output >actual &&
 		test_cmp "$expect" actual
 	'
@@ -167,7 +167,7 @@ do
 	case "$cmd" in diff|show) continue;; esac
 
 	test_expect_success "$cmd --graph $verb not enough COLUMNS (big change)" '
-		COLUMNS=40 git $cmd $args --graph >output &&
+		test_with_columns 40 git $cmd $args --graph >output &&
 		grep " | " output >actual &&
 		test_cmp "$expect-graph" actual
 	'
@@ -302,7 +302,7 @@ EOF
 while read verb expect cmd args
 do
 	test_expect_success "$cmd $verb COLUMNS (long filename)" '
-		COLUMNS=200 git $cmd $args >output &&
+		test_with_columns 200 git $cmd $args >output &&
 		grep " | " output >actual &&
 		test_cmp "$expect" actual
 	'
@@ -310,7 +310,7 @@ do
 	case "$cmd" in diff|show) continue;; esac
 
 	test_expect_success "$cmd --graph $verb COLUMNS (long filename)" '
-		COLUMNS=200 git $cmd $args --graph >output &&
+		test_with_columns 200 git $cmd $args --graph >output &&
 		grep " | " output >actual &&
 		test_cmp "$expect-graph" actual
 	'
@@ -331,7 +331,7 @@ while read verb expect cmd args
 do
 	test_expect_success COLUMNS_CAN_BE_1 \
 		"$cmd $verb prefix greater than COLUMNS (big change)" '
-		COLUMNS=1 git $cmd $args >output &&
+		test_with_columns 1 git $cmd $args >output &&
 		grep " | " output >actual &&
 		test_cmp "$expect" actual
 	'
@@ -340,7 +340,7 @@ do
 
 	test_expect_success COLUMNS_CAN_BE_1 \
 		"$cmd --graph $verb prefix greater than COLUMNS (big change)" '
-		COLUMNS=1 git $cmd $args --graph >output &&
+		test_with_columns 1 git $cmd $args --graph >output &&
 		grep " | " output >actual &&
 		test_cmp "$expect-graph" actual
 	'
@@ -356,7 +356,7 @@ cat >expect <<'EOF'
 EOF
 test_expect_success 'merge --stat respects COLUMNS (big change)' '
 	git checkout -b branch HEAD^^ &&
-	COLUMNS=100 git merge --stat --no-ff main^ >output &&
+	test_with_columns 100 git merge --stat --no-ff main^ >output &&
 	grep " | " output >actual &&
 	test_cmp expect actual
 '
@@ -365,7 +365,7 @@ cat >expect <<'EOF'
  aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 +++++++++++++++++++++++++++++++++++++++
 EOF
 test_expect_success 'merge --stat respects COLUMNS (long filename)' '
-	COLUMNS=100 git merge --stat --no-ff main >output &&
+	test_with_columns 100 git merge --stat --no-ff main >output &&
 	grep " | " output >actual &&
 	test_cmp expect actual
 '
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 5865daa8f8d..a035f749537 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -207,7 +207,7 @@ test_expect_success 'left alignment formatting at the nth column' '
 '
 
 test_expect_success 'left alignment formatting at the nth column' '
-	COLUMNS=50 git log --pretty="tformat:%h %<|(-10)%s" >actual &&
+	test_with_columns 50 git log --pretty="tformat:%h %<|(-10)%s" >actual &&
 	qz_to_tab_space <<-EOF >expected &&
 	$head1 message two                    Z
 	$head2 message one                    Z
@@ -350,7 +350,7 @@ test_expect_success 'right alignment formatting at the nth column' '
 '
 
 test_expect_success 'right alignment formatting at the nth column' '
-	COLUMNS=50 git log --pretty="tformat:%h %>|(-10)%s" >actual &&
+	test_with_columns 50 git log --pretty="tformat:%h %>|(-10)%s" >actual &&
 	qz_to_tab_space <<-EOF >expected &&
 	$head1                      message two
 	$head2                      message one
@@ -450,7 +450,7 @@ test_expect_success 'center alignment formatting at the nth column' '
 '
 
 test_expect_success 'center alignment formatting at the nth column' '
-	COLUMNS=70 git log --pretty="tformat:%h %><|(-30)%s" >actual &&
+	test_with_columns 70 git log --pretty="tformat:%h %><|(-30)%s" >actual &&
 	qz_to_tab_space <<-EOF >expected &&
 	$head1           message two          Z
 	$head2           message one          Z
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 2f72c5c6883..e893c43d705 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -372,7 +372,7 @@ test_expect_success 'tag -l <pattern> -l <pattern> works, as our buggy documenta
 '
 
 test_expect_success 'listing tags in column' '
-	COLUMNS=41 git tag -l --column=row >actual &&
+	test_with_columns 41 git tag -l --column=row >actual &&
 	cat >expected <<\EOF &&
 a1      aa1     cba     t210    t211
 v0.2.1  v1.0    v1.0.1  v1.1.3
@@ -383,7 +383,7 @@ EOF
 test_expect_success 'listing tags in column with column.*' '
 	test_config column.tag row &&
 	test_config column.ui dense &&
-	COLUMNS=40 git tag -l >actual &&
+	test_with_columns 40 git tag -l >actual &&
 	cat >expected <<\EOF &&
 a1      aa1   cba     t210    t211
 v0.2.1  v1.0  v1.0.1  v1.1.3
@@ -397,7 +397,7 @@ test_expect_success 'listing tag with -n --column should fail' '
 
 test_expect_success 'listing tags -n in column with column.ui ignored' '
 	test_config column.ui "row dense" &&
-	COLUMNS=40 git tag -l -n >actual &&
+	test_with_columns 40 git tag -l -n >actual &&
 	cat >expected <<\EOF &&
 a1              Foo
 aa1             Foo
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 0e7cf75435e..5189699debf 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -651,7 +651,7 @@ test_expect_success TTY 'git tag with auto-columns ' '
 	cat >expect <<-\EOF &&
 	initial  one      two      three    four     five
 	EOF
-	test_terminal env PAGER="cat >actual" COLUMNS=80 \
+	test_with_columns 80 test_terminal env PAGER="cat >actual" \
 		git -c column.ui=auto tag --sort=authordate &&
 	test_cmp expect actual
 '
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index 2b72451ba3e..808b8f50576 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -108,13 +108,13 @@ test_expect_success 'status --column' '
 #	dir2/modified  untracked
 #
 EOF
-	COLUMNS=50 git -c status.displayCommentPrefix=true status --column="column dense" >output &&
+	test_with_columns 50 git -c status.displayCommentPrefix=true status --column="column dense" >output &&
 	test_cmp expect output
 '
 
 test_expect_success 'status --column status.displayCommentPrefix=false' '
 	strip_comments expect &&
-	COLUMNS=49 git -c status.displayCommentPrefix=false status --column="column dense" >output &&
+	test_with_columns 49 git -c status.displayCommentPrefix=false status --column="column dense" >output &&
 	test_cmp expect output
 '
 
diff --git a/t/t9002-column.sh b/t/t9002-column.sh
index 89983527b62..9151d69bcf1 100755
--- a/t/t9002-column.sh
+++ b/t/t9002-column.sh
@@ -46,7 +46,7 @@ test_expect_success '80 columns' '
 	cat >expected <<\EOF &&
 one    two    three  four   five   six    seven  eight  nine   ten    eleven
 EOF
-	COLUMNS=80 git column --mode=column <lista >actual &&
+	test_with_columns 80 git column --mode=column <lista >actual &&
 	test_cmp expected actual
 '
 
@@ -65,7 +65,7 @@ eleven
 EOF
 
 test_expect_success COLUMNS_CAN_BE_1 'COLUMNS = 1' '
-	COLUMNS=1 git column --mode=column <lista >actual &&
+	test_with_columns 1 git column --mode=column <lista >actual &&
 	test_cmp expected actual
 '
 
@@ -74,9 +74,6 @@ test_expect_success 'width = 1' '
 	test_cmp expected actual
 '
 
-COLUMNS=20
-export COLUMNS
-
 test_expect_success '20 columns' '
 	cat >expected <<\EOF &&
 one    seven
@@ -86,7 +83,7 @@ four   ten
 five   eleven
 six
 EOF
-	git column --mode=column <lista >actual &&
+	test_with_columns 20 git column --mode=column <lista >actual &&
 	test_cmp expected actual
 '
 
@@ -99,7 +96,7 @@ four   ten
 five   eleven
 six
 EOF
-	git column --mode=column,nodense < lista > actual &&
+	test_with_columns 20 git column --mode=column,nodense < lista > actual &&
 	test_cmp expected actual
 '
 
@@ -110,7 +107,7 @@ two   six   ten
 three seven eleven
 four  eight
 EOF
-	git column --mode=column,dense < lista > actual &&
+	test_with_columns 20 git column --mode=column,dense < lista > actual &&
 	test_cmp expected actual
 '
 
@@ -123,7 +120,7 @@ four    ten
 five    eleven
 six
 EOF
-	git column --mode=column --padding 2 <lista >actual &&
+	test_with_columns 20 git column --mode=column --padding 2 <lista >actual &&
 	test_cmp expected actual
 '
 
@@ -136,7 +133,7 @@ test_expect_success '20 columns, indented' '
   five   eleven
   six
 EOF
-	git column --mode=column --indent="  " <lista >actual &&
+	test_with_columns 20 git column --mode=column --indent="  " <lista >actual &&
 	test_cmp expected actual
 '
 
@@ -149,7 +146,7 @@ seven  eight
 nine   ten
 eleven
 EOF
-	git column --mode=row <lista >actual &&
+	test_with_columns 20 git column --mode=row <lista >actual &&
 	test_cmp expected actual
 '
 
@@ -162,7 +159,7 @@ seven  eight
 nine   ten
 eleven
 EOF
-	git column --mode=row,nodense <lista >actual &&
+	test_with_columns 20 git column --mode=row,nodense <lista >actual &&
 	test_cmp expected actual
 '
 
@@ -173,7 +170,7 @@ four  five   six
 seven eight  nine
 ten   eleven
 EOF
-	git column --mode=row,dense <lista >actual &&
+	test_with_columns 20 git column --mode=row,dense <lista >actual &&
 	test_cmp expected actual
 '
 
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 37da7d9a99a..adc853109e4 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1718,3 +1718,17 @@ test_region () {
 test_readlink () {
 	perl -le 'print readlink($_) for @ARGV' "$@"
 }
+
+# Test a with a given number of COLUMNS in the environment.
+test_with_columns () {
+	local columns=$1
+	shift
+
+	if ! is_git_command_name "$@"
+	then
+		echo >&7 "test_with_columns: only 'git' is allowed: $*"
+		return 1
+	fi
+
+	COLUMNS=$columns "$@" 2>&7
+} 7>&2 2>&4
-- 
2.33.0.rc0.597.gc569a812f0a

[PATCH v3 3/3] test-lib.sh: use GIT_TEST_COLUMNS over COLUMNS

[Ævar Arnfjörð Bjarmason]

In my c49a177beca (test-lib.sh: set COLUMNS=80 for --verbose
repeatability, 2021-06-29) the test suite started breaking on recent
versions of bash.

This is because it sets "shopt -s checkwinsize" starting with version
5.0, furthermore it started setting COLUMNS under "shopt -s
checkwinsize" for non-interactive use around version 4.3.

A narrow fix for that issue would be to add this just above our
setting of COLUMNS in test-lib.sh:

	shopt -u checkwinsize >/dev/null 2>&1

But we'd then be at the mercy of the next shell or terminal that wants
to be clever about COLUMNS.

Let's instead solve this more thoroughly. We'll now take
GIT_TEST_COLUMNS over COLUMNS, and furthermore intentionally spoil the
COLUMNS variable to break any tests that rely on it being set to a
sane value.

If something breaks because we have a codepath that's not
term_columns() checking COLUMNS we'd like to know about it, the narrow
"shopt -u checkwinsize" fix won't give us that.

The "shopt" fix won't future-poof us against other 3rd party software
changes either. If that third-party software e.g. takes TIOCGWINSZ
over columns on some platforms, our tests would be flaky and break
there even without this change.

This approach does mean that any tests of ours that expected to test
term_columns() behavior by setting COLUMNS will need to explicitly
unset GIT_TEST_COLUMNS, or set it to the empty string. Let's do that
in the new test_with_columns() helper, which we previously changed all
the tests that set COLUMNS to use.

Reported-by: Fabian Stelzer <fabian.stelzer@campoint.net>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 pager.c                 |  7 +++++++
 t/test-lib-functions.sh |  1 +
 t/test-lib.sh           | 13 +++++++++++--
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/pager.c b/pager.c
index 52f27a6765c..cfcc6dc04bd 100644
--- a/pager.c
+++ b/pager.c
@@ -165,6 +165,13 @@ int term_columns(void)
 	term_columns_at_startup = 80;
 	term_columns_guessed = 1;
 
+	col_string = getenv("GIT_TEST_COLUMNS");
+	if (col_string && (n_cols = atoi(col_string)) > 0) {
+		term_columns_at_startup = n_cols;
+		term_columns_guessed = 0;
+		return term_columns_at_startup;
+	}
+
 	col_string = getenv("COLUMNS");
 	if (col_string && (n_cols = atoi(col_string)) > 0) {
 		term_columns_at_startup = n_cols;
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index adc853109e4..0970c1293a8 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1730,5 +1730,6 @@ test_with_columns () {
 		return 1
 	fi
 
+	GIT_TEST_COLUMNS= \
 	COLUMNS=$columns "$@" 2>&7
 } 7>&2 2>&4
diff --git a/t/test-lib.sh b/t/test-lib.sh
index db61081d6b8..fc589226189 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -415,10 +415,19 @@ LANG=C
 LC_ALL=C
 PAGER=cat
 TZ=UTC
-COLUMNS=80
-export LANG LC_ALL PAGER TZ COLUMNS
+export LANG LC_ALL PAGER TZ
 EDITOR=:
 
+# For repeatability we need to set term_columns()'s idea of
+# columns. We do this via GIT_TEST_COLUMNS and not COLUMNS because
+# e.g. versions of bash >= 5.0 have "shopt -s checkwinsize" on by
+# default. We could do "shopt -u checkwinsize >/dev/null 2>&1" here to
+# fix that particular issue, but this is not shell specific, and
+# future-proof the tests.
+GIT_TEST_COLUMNS=80
+COLUMNS=10
+export GIT_TEST_COLUMNS COLUMNS
+
 # 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
-- 
2.33.0.rc0.597.gc569a812f0a