* progress test failure on fedora34 @ 2021-07-14 12:39 Fabian Stelzer 2021-07-14 15:35 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 26+ messages in thread From: Fabian Stelzer @ 2021-07-14 12:39 UTC (permalink / raw) To: git; +Cc: Ævar Arnfjörð Bjarmason 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 # ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: progress test failure on fedora34 2021-07-14 12:39 progress test failure on fedora34 Fabian Stelzer @ 2021-07-14 15:35 ` Ævar Arnfjörð Bjarmason 2021-07-14 16:35 ` Alex Henrie 0 siblings, 1 reply; 26+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-07-14 15:35 UTC (permalink / raw) To: Fabian Stelzer; +Cc: git 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... ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: progress test failure on fedora34 2021-07-14 15:35 ` Ævar Arnfjörð Bjarmason @ 2021-07-14 16:35 ` Alex Henrie 2021-07-18 8:05 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 26+ messages in thread From: Alex Henrie @ 2021-07-14 16:35 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: Fabian Stelzer, Git mailing list 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: progress test failure on fedora34 2021-07-14 16:35 ` Alex Henrie @ 2021-07-18 8:05 ` Ævar Arnfjörð Bjarmason 2021-07-19 9:00 ` Jeff King ` (2 more replies) 0 siblings, 3 replies; 26+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-07-18 8:05 UTC (permalink / raw) To: Alex Henrie; +Cc: Fabian Stelzer, Git mailing list 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, ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: progress test failure on fedora34 2021-07-18 8:05 ` Ævar Arnfjörð Bjarmason @ 2021-07-19 9:00 ` Jeff King 2021-07-19 17:18 ` Alex Henrie 2021-07-19 18:43 ` Felipe Contreras 2 siblings, 0 replies; 26+ messages in thread From: Jeff King @ 2021-07-19 9:00 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Alex Henrie, Fabian Stelzer, Git mailing list 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: progress test failure on fedora34 2021-07-18 8:05 ` Ævar Arnfjörð Bjarmason 2021-07-19 9:00 ` Jeff King @ 2021-07-19 17:18 ` Alex Henrie 2021-07-19 18:21 ` Alex Henrie 2021-07-19 18:43 ` Felipe Contreras 2 siblings, 1 reply; 26+ messages in thread From: Alex Henrie @ 2021-07-19 17:18 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: Fabian Stelzer, Git mailing list 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: progress test failure on fedora34 2021-07-19 17:18 ` Alex Henrie @ 2021-07-19 18:21 ` Alex Henrie 0 siblings, 0 replies; 26+ messages in thread From: Alex Henrie @ 2021-07-19 18:21 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: Fabian Stelzer, Git mailing list 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: progress test failure on fedora34 2021-07-18 8:05 ` Ævar Arnfjörð Bjarmason 2021-07-19 9:00 ` Jeff King 2021-07-19 17:18 ` Alex Henrie @ 2021-07-19 18:43 ` Felipe Contreras 2021-07-19 19:34 ` Felipe Contreras 2 siblings, 1 reply; 26+ messages in thread From: Felipe Contreras @ 2021-07-19 18:43 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason, Alex Henrie Cc: Fabian Stelzer, Git mailing list Æ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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: progress test failure on fedora34 2021-07-19 18:43 ` Felipe Contreras @ 2021-07-19 19:34 ` Felipe Contreras 2021-07-19 20:42 ` Alex Henrie 0 siblings, 1 reply; 26+ messages in thread From: Felipe Contreras @ 2021-07-19 19:34 UTC (permalink / raw) To: Felipe Contreras, Ævar Arnfjörð Bjarmason, Alex Henrie Cc: Fabian Stelzer, Git mailing list 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: progress test failure on fedora34 2021-07-19 19:34 ` Felipe Contreras @ 2021-07-19 20:42 ` Alex Henrie 2021-07-20 0:40 ` Felipe Contreras 2021-07-26 23:57 ` [PATCH] test-lib.sh: use GIT_TEST_COLUMNS over COLUMNS Ævar Arnfjörð Bjarmason 0 siblings, 2 replies; 26+ messages in thread From: Alex Henrie @ 2021-07-19 20:42 UTC (permalink / raw) To: Felipe Contreras Cc: Ævar Arnfjörð Bjarmason, Fabian Stelzer, Git mailing list 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: progress test failure on fedora34 2021-07-19 20:42 ` Alex Henrie @ 2021-07-20 0:40 ` Felipe Contreras 2021-07-21 0:45 ` ZheNing Hu 2021-07-26 23:57 ` [PATCH] test-lib.sh: use GIT_TEST_COLUMNS over COLUMNS Ævar Arnfjörð Bjarmason 1 sibling, 1 reply; 26+ messages in thread From: Felipe Contreras @ 2021-07-20 0:40 UTC (permalink / raw) To: Alex Henrie, Felipe Contreras Cc: Ævar Arnfjörð Bjarmason, Fabian Stelzer, Git mailing list 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: progress test failure on fedora34 2021-07-20 0:40 ` Felipe Contreras @ 2021-07-21 0:45 ` ZheNing Hu 2021-07-21 2:50 ` Felipe Contreras 0 siblings, 1 reply; 26+ messages in thread From: ZheNing Hu @ 2021-07-21 0:45 UTC (permalink / raw) To: Felipe Contreras Cc: Alex Henrie, Ævar Arnfjörð Bjarmason, Fabian Stelzer, Git mailing list 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: progress test failure on fedora34 2021-07-21 0:45 ` ZheNing Hu @ 2021-07-21 2:50 ` Felipe Contreras 0 siblings, 0 replies; 26+ messages in thread From: Felipe Contreras @ 2021-07-21 2:50 UTC (permalink / raw) To: ZheNing Hu, Felipe Contreras Cc: Alex Henrie, Ævar Arnfjörð Bjarmason, Fabian Stelzer, Git mailing list 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH] test-lib.sh: use GIT_TEST_COLUMNS over COLUMNS 2021-07-19 20:42 ` Alex Henrie 2021-07-20 0:40 ` Felipe Contreras @ 2021-07-26 23:57 ` Ævar Arnfjörð Bjarmason 2021-07-27 17:38 ` Jeff King 2021-08-02 13:46 ` [PATCH v2 0/3] " Ævar Arnfjörð Bjarmason 1 sibling, 2 replies; 26+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-07-26 23:57 UTC (permalink / raw) To: git Cc: Junio C Hamano, Alex Henrie, Felipe Contreras, Fabian Stelzer, Jeff King, Zbigniew Jędrzejewski-Szmek, Æ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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH] test-lib.sh: use GIT_TEST_COLUMNS over COLUMNS 2021-07-26 23:57 ` [PATCH] test-lib.sh: use GIT_TEST_COLUMNS over COLUMNS Ævar Arnfjörð Bjarmason @ 2021-07-27 17:38 ` Jeff King 2021-07-28 0:53 ` Junio C Hamano 2021-08-02 13:46 ` [PATCH v2 0/3] " Ævar Arnfjörð Bjarmason 1 sibling, 1 reply; 26+ messages in thread From: Jeff King @ 2021-07-27 17:38 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Junio C Hamano, Alex Henrie, Felipe Contreras, Fabian Stelzer, Zbigniew Jędrzejewski-Szmek 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] test-lib.sh: use GIT_TEST_COLUMNS over COLUMNS 2021-07-27 17:38 ` Jeff King @ 2021-07-28 0:53 ` Junio C Hamano 0 siblings, 0 replies; 26+ messages in thread From: Junio C Hamano @ 2021-07-28 0:53 UTC (permalink / raw) To: Jeff King Cc: Ævar Arnfjörð Bjarmason, git, Alex Henrie, Felipe Contreras, Fabian Stelzer, Zbigniew Jędrzejewski-Szmek 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 0/3] test-lib.sh: use GIT_TEST_COLUMNS over COLUMNS 2021-07-26 23:57 ` [PATCH] test-lib.sh: use GIT_TEST_COLUMNS over COLUMNS Ævar Arnfjörð Bjarmason 2021-07-27 17:38 ` Jeff King @ 2021-08-02 13:46 ` Ævar Arnfjörð Bjarmason 2021-08-02 13:46 ` [PATCH v2 1/3] test-lib-functions.sh: rename test_must_fail_acceptable() Ævar Arnfjörð Bjarmason ` (3 more replies) 1 sibling, 4 replies; 26+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-08-02 13:46 UTC (permalink / raw) To: git Cc: Junio C Hamano, Alex Henrie, Felipe Contreras, Fabian Stelzer, Jeff King, Zbigniew Jędrzejewski-Szmek, Æ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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 1/3] test-lib-functions.sh: rename test_must_fail_acceptable() 2021-08-02 13:46 ` [PATCH v2 0/3] " Ævar Arnfjörð Bjarmason @ 2021-08-02 13:46 ` Ævar Arnfjörð Bjarmason 2021-08-02 13:46 ` [PATCH v2 2/3] test-lib-functions.sh: add a test_with_columns function Ævar Arnfjörð Bjarmason ` (2 subsequent siblings) 3 siblings, 0 replies; 26+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-08-02 13:46 UTC (permalink / raw) To: git Cc: Junio C Hamano, Alex Henrie, Felipe Contreras, Fabian Stelzer, Jeff King, Zbigniew Jędrzejewski-Szmek, Æ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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2 2/3] test-lib-functions.sh: add a test_with_columns function 2021-08-02 13:46 ` [PATCH v2 0/3] " Ævar Arnfjörð Bjarmason 2021-08-02 13:46 ` [PATCH v2 1/3] test-lib-functions.sh: rename test_must_fail_acceptable() Ævar Arnfjörð Bjarmason @ 2021-08-02 13:46 ` Ævar Arnfjörð Bjarmason 2021-08-02 17:14 ` SZEDER Gábor 2021-08-02 13:46 ` [PATCH v2 3/3] test-lib.sh: use GIT_TEST_COLUMNS over COLUMNS Ævar Arnfjörð Bjarmason 2021-08-04 23:05 ` [PATCH v3 0/3] " Ævar Arnfjörð Bjarmason 3 siblings, 1 reply; 26+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-08-02 13:46 UTC (permalink / raw) To: git Cc: Junio C Hamano, Alex Henrie, Felipe Contreras, Fabian Stelzer, Jeff King, Zbigniew Jędrzejewski-Szmek, Æ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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v2 2/3] test-lib-functions.sh: add a test_with_columns function 2021-08-02 13:46 ` [PATCH v2 2/3] test-lib-functions.sh: add a test_with_columns function Ævar Arnfjörð Bjarmason @ 2021-08-02 17:14 ` SZEDER Gábor 2021-08-02 17:24 ` Eric Sunshine 0 siblings, 1 reply; 26+ messages in thread From: SZEDER Gábor @ 2021-08-02 17:14 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Junio C Hamano, Alex Henrie, Felipe Contreras, Fabian Stelzer, Jeff King, Zbigniew Jędrzejewski-Szmek 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). ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 2/3] test-lib-functions.sh: add a test_with_columns function 2021-08-02 17:14 ` SZEDER Gábor @ 2021-08-02 17:24 ` Eric Sunshine 0 siblings, 0 replies; 26+ messages in thread From: Eric Sunshine @ 2021-08-02 17:24 UTC (permalink / raw) To: SZEDER Gábor Cc: Ævar Arnfjörð Bjarmason, Git List, Junio C Hamano, Alex Henrie, Felipe Contreras, Fabian Stelzer, Jeff King, Zbigniew Jędrzejewski-Szmek 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 3/3] test-lib.sh: use GIT_TEST_COLUMNS over COLUMNS 2021-08-02 13:46 ` [PATCH v2 0/3] " Ævar Arnfjörð Bjarmason 2021-08-02 13:46 ` [PATCH v2 1/3] test-lib-functions.sh: rename test_must_fail_acceptable() Ævar Arnfjörð Bjarmason 2021-08-02 13:46 ` [PATCH v2 2/3] test-lib-functions.sh: add a test_with_columns function Ævar Arnfjörð Bjarmason @ 2021-08-02 13:46 ` Ævar Arnfjörð Bjarmason 2021-08-04 23:05 ` [PATCH v3 0/3] " Ævar Arnfjörð Bjarmason 3 siblings, 0 replies; 26+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-08-02 13:46 UTC (permalink / raw) To: git Cc: Junio C Hamano, Alex Henrie, Felipe Contreras, Fabian Stelzer, Jeff King, Zbigniew Jędrzejewski-Szmek, Æ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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v3 0/3] test-lib.sh: use GIT_TEST_COLUMNS over COLUMNS 2021-08-02 13:46 ` [PATCH v2 0/3] " Ævar Arnfjörð Bjarmason ` (2 preceding siblings ...) 2021-08-02 13:46 ` [PATCH v2 3/3] test-lib.sh: use GIT_TEST_COLUMNS over COLUMNS Ævar Arnfjörð Bjarmason @ 2021-08-04 23:05 ` Ævar Arnfjörð Bjarmason 2021-08-04 23:05 ` [PATCH v3 1/3] test-lib-functions.sh: rename test_must_fail_acceptable() Ævar Arnfjörð Bjarmason ` (2 more replies) 3 siblings, 3 replies; 26+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-08-04 23:05 UTC (permalink / raw) To: git Cc: Junio C Hamano, Alex Henrie, Felipe Contreras, Fabian Stelzer, Jeff King, Zbigniew Jędrzejewski-Szmek, SZEDER Gábor, Eric Sunshine, Æ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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v3 1/3] test-lib-functions.sh: rename test_must_fail_acceptable() 2021-08-04 23:05 ` [PATCH v3 0/3] " Ævar Arnfjörð Bjarmason @ 2021-08-04 23:05 ` Ævar Arnfjörð Bjarmason 2021-08-04 23:05 ` [PATCH v3 2/3] test-lib-functions.sh: add a test_with_columns function Ævar Arnfjörð Bjarmason 2021-08-04 23:05 ` [PATCH v3 3/3] test-lib.sh: use GIT_TEST_COLUMNS over COLUMNS Ævar Arnfjörð Bjarmason 2 siblings, 0 replies; 26+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-08-04 23:05 UTC (permalink / raw) To: git Cc: Junio C Hamano, Alex Henrie, Felipe Contreras, Fabian Stelzer, Jeff King, Zbigniew Jędrzejewski-Szmek, SZEDER Gábor, Eric Sunshine, Æ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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v3 2/3] test-lib-functions.sh: add a test_with_columns function 2021-08-04 23:05 ` [PATCH v3 0/3] " Ævar Arnfjörð Bjarmason 2021-08-04 23:05 ` [PATCH v3 1/3] test-lib-functions.sh: rename test_must_fail_acceptable() Ævar Arnfjörð Bjarmason @ 2021-08-04 23:05 ` Ævar Arnfjörð Bjarmason 2021-08-04 23:05 ` [PATCH v3 3/3] test-lib.sh: use GIT_TEST_COLUMNS over COLUMNS Ævar Arnfjörð Bjarmason 2 siblings, 0 replies; 26+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-08-04 23:05 UTC (permalink / raw) To: git Cc: Junio C Hamano, Alex Henrie, Felipe Contreras, Fabian Stelzer, Jeff King, Zbigniew Jędrzejewski-Szmek, SZEDER Gábor, Eric Sunshine, Æ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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v3 3/3] test-lib.sh: use GIT_TEST_COLUMNS over COLUMNS 2021-08-04 23:05 ` [PATCH v3 0/3] " Ævar Arnfjörð Bjarmason 2021-08-04 23:05 ` [PATCH v3 1/3] test-lib-functions.sh: rename test_must_fail_acceptable() Ævar Arnfjörð Bjarmason 2021-08-04 23:05 ` [PATCH v3 2/3] test-lib-functions.sh: add a test_with_columns function Ævar Arnfjörð Bjarmason @ 2021-08-04 23:05 ` Ævar Arnfjörð Bjarmason 2 siblings, 0 replies; 26+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-08-04 23:05 UTC (permalink / raw) To: git Cc: Junio C Hamano, Alex Henrie, Felipe Contreras, Fabian Stelzer, Jeff King, Zbigniew Jędrzejewski-Szmek, SZEDER Gábor, Eric Sunshine, Æ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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
end of thread, other threads:[~2021-08-04 23:05 UTC | newest] Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-14 12:39 progress test failure on fedora34 Fabian Stelzer 2021-07-14 15:35 ` Ævar Arnfjörð Bjarmason 2021-07-14 16:35 ` Alex Henrie 2021-07-18 8:05 ` Ævar Arnfjörð Bjarmason 2021-07-19 9:00 ` Jeff King 2021-07-19 17:18 ` Alex Henrie 2021-07-19 18:21 ` Alex Henrie 2021-07-19 18:43 ` Felipe Contreras 2021-07-19 19:34 ` Felipe Contreras 2021-07-19 20:42 ` Alex Henrie 2021-07-20 0:40 ` Felipe Contreras 2021-07-21 0:45 ` ZheNing Hu 2021-07-21 2:50 ` Felipe Contreras 2021-07-26 23:57 ` [PATCH] test-lib.sh: use GIT_TEST_COLUMNS over COLUMNS Ævar Arnfjörð Bjarmason 2021-07-27 17:38 ` Jeff King 2021-07-28 0:53 ` Junio C Hamano 2021-08-02 13:46 ` [PATCH v2 0/3] " Ævar Arnfjörð Bjarmason 2021-08-02 13:46 ` [PATCH v2 1/3] test-lib-functions.sh: rename test_must_fail_acceptable() Ævar Arnfjörð Bjarmason 2021-08-02 13:46 ` [PATCH v2 2/3] test-lib-functions.sh: add a test_with_columns function Ævar Arnfjörð Bjarmason 2021-08-02 17:14 ` SZEDER Gábor 2021-08-02 17:24 ` Eric Sunshine 2021-08-02 13:46 ` [PATCH v2 3/3] test-lib.sh: use GIT_TEST_COLUMNS over COLUMNS Ævar Arnfjörð Bjarmason 2021-08-04 23:05 ` [PATCH v3 0/3] " Ævar Arnfjörð Bjarmason 2021-08-04 23:05 ` [PATCH v3 1/3] test-lib-functions.sh: rename test_must_fail_acceptable() Ævar Arnfjörð Bjarmason 2021-08-04 23:05 ` [PATCH v3 2/3] test-lib-functions.sh: add a test_with_columns function Ævar Arnfjörð Bjarmason 2021-08-04 23:05 ` [PATCH v3 3/3] test-lib.sh: use GIT_TEST_COLUMNS over COLUMNS Ævar Arnfjörð Bjarmason
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.