{"thread":{"id":"56221","subject":"[PATCH] test: fix for COLUMNS and bash 5","startedAt":"2021-08-05T19:48:33Z","lastAt":"2021-08-23T20:24:24Z","messageCount":9,"participants":["Felipe Contreras","Ævar Arnfjörð Bjarmason","SZEDER Gábor","Junio C Hamano","ZheNing Hu"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"432128","messageId":"20210805194825.1796765-1-felipe.contreras@gmail.com","threadId":"56221","inReplyTo":null,"subject":"[PATCH] test: fix for COLUMNS and bash 5","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-08-05T19:48:25Z","receivedAt":"2021-08-05T19:48:33Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Since c49a177bec (test-lib.sh: set COLUMNS=80 for --verbose\nrepeatability, 2021-06-29) multiple tests have been failing when using\nbash 5 because checkwinsize is enabled by default, therefore COLUMNS is\nreset using TIOCGWINSZ even for non-interactive shells.\n\nIt's debatable whether or not bash should even be doing that, but for\nnow we can avoid this undesirable behavior by disabling this option.\n\nReported-by: Fabian Stelzer \nSigned-off-by: Felipe Contreras \n---\n t/test-lib.sh | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex db61081d6b..a2b7dfecee 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -419,6 +419,12 @@ COLUMNS=80\n export LANG LC_ALL PAGER TZ COLUMNS\n EDITOR=:\n \n+# Since bash 5.0, checkwinsize is enabled by default which does update the\n+# COLUMNS variable every time a command completes, even for non-interactive\n+# shells.\n+# Disable that since we are aiming for reproducibility.\n+test -n \"$BASH_VERSION\" && shopt -u checkwinsize 2>/dev/null\n+\n # A call to \"unset\" with no arguments causes at least Solaris 10\n # /usr/xpg4/bin/sh and /bin/ksh to bail out. So keep the unsets\n # deriving from the command substitution clustered with the other\n-- \n2.32.0.40.gb9b36f9b52\n\n"},{"id":"432146","messageId":"87k0kzzc91.fsf@evledraar.gmail.com","threadId":"56221","inReplyTo":"20210805194825.1796765-1-felipe.contreras@gmail.com","subject":"Re: [PATCH] test: fix for COLUMNS and bash 5","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-05T23:35:42Z","receivedAt":"2021-08-05T23:46:08Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Aug 05 2021, Felipe Contreras wrote:\n\n> Since c49a177bec (test-lib.sh: set COLUMNS=80 for --verbose\n> repeatability, 2021-06-29) multiple tests have been failing when using\n> bash 5 because checkwinsize is enabled by default, therefore COLUMNS is\n> reset using TIOCGWINSZ even for non-interactive shells.\n>\n> It's debatable whether or not bash should even be doing that, but for\n> now we can avoid this undesirable behavior by disabling this option.\n>\n> Reported-by: Fabian Stelzer \n> Signed-off-by: Felipe Contreras \n\nI've got an alternative way of solving the same immeditate issue in[1],\nthere's discussion on that approach in the latest What's Cooking[2]. I\nbelive this approach would make at least SZEDER happier than with my\nseries :)\n\nAs noted in that discussion I'd prefer going for mine, but would also be\nfine with this if it's what Junio decides to pick up. We should have one\nor the other before 2.33 is out.\n\nMy preference for mine is in no small part that I'd like to not be\nresponsible for into-the-past test suite breakage the next time a\npopular shell decides to be clever about COLUMNS.\n\nBut this way we'll solve the immediate problem with bash, and I can say\nI told you so if that submarine breakage occurs with this approach :)\n\n1. https://lore.kernel.org/git/cover-v3-0.3-00000000000-20210804T230335Z-avarab@gmail.com/\n2. https://lore.kernel.org/git/cover-v3-0.3-00000000000-20210804T230335Z-avarab@gmail.com/\n\n> ---\n> t/test-lib.sh | 6 ++++++\n> 1 file changed, 6 insertions(+)\n>\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index db61081d6b..a2b7dfecee 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -419,6 +419,12 @@ COLUMNS=80\n> export LANG LC_ALL PAGER TZ COLUMNS\n> EDITOR=:\n> \n> +# Since bash 5.0, checkwinsize is enabled by default which does update the\n> +# COLUMNS variable every time a command completes, even for non-interactive\n> +# shells.\n\nI don't think this needs updating, but FWIW I think that around bash 4.X\n(I think 4.1, but that's from memory) is where it started doing this for\nnon-interactive shells, so we could have had breakage going back that\nfar.\n\nBut we're seeing this in practice now because with 5.0 this was turned\non by default.\n\nI.e. I think if you turn on checkwinsize it'll also break with the tests\non bash 4.1 (or whatever it was), but not on 3.x.\n\nNone of that's something I've tested, just from memory of skimming the\nbash commit logs for changes related to checkwinsize a some days ago\nwhen I came up with my fix for this.\n"},{"id":"432179","messageId":"20210806144911.GA1388237@szeder.dev","threadId":"56221","inReplyTo":"20210805194825.1796765-1-felipe.contreras@gmail.com","subject":"Re: [PATCH] test: fix for COLUMNS and bash 5","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2021-08-06T14:49:11Z","receivedAt":"2021-08-06T14:50:36Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Aug 05, 2021 at 02:48:25PM -0500, Felipe Contreras wrote:\n> Since c49a177bec (test-lib.sh: set COLUMNS=80 for --verbose\n> repeatability, 2021-06-29) multiple tests have been failing when using\n> bash 5 because checkwinsize is enabled by default, therefore COLUMNS is\n> reset using TIOCGWINSZ even for non-interactive shells.\n> \n> It's debatable whether or not bash should even be doing that, but for\n> now we can avoid this undesirable behavior by disabling this option.\n> \n> Reported-by: Fabian Stelzer \n> Signed-off-by: Felipe Contreras \n> ---\n> t/test-lib.sh | 6 ++++++\n> 1 file changed, 6 insertions(+)\n> \n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index db61081d6b..a2b7dfecee 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -419,6 +419,12 @@ COLUMNS=80\n\nCOLUMNS is set just before the start of the hunk context ...\n\n> export LANG LC_ALL PAGER TZ COLUMNS\n> EDITOR=:\n\n... so these two \"commands\" above are executed while COLUMNS is\nalready set but checkwinsize is not yet disabled. The reason I put\nquotes around that commands is that while exporting and setting\nvariables are indeed commands as defined in the POSIX Shell Command\nLanguage specs, Bash with checkwinsize enabled only \"checks the window\nsize after each extern (non-builtin) command\" (quoting 'man bash').\n\nSo even though it is safe to execute these variable setting and\nexporting commands after setting COLUMNS but disabling checkwinsize, I\nthink it would be prudent to disable checkwinsize before initializing\nCOLUMNS. (And perhaps adding \"non-builtin\" to the comment below.)\n\n> +# Since bash 5.0, checkwinsize is enabled by default which does update the\n> +# COLUMNS variable every time a command completes, even for non-interactive\n> +# shells.\n> +# Disable that since we are aiming for reproducibility.\n> +test -n \"$BASH_VERSION\" && shopt -u checkwinsize 2>/dev/null\n> +\n> # A call to \"unset\" with no arguments causes at least Solaris 10\n> # /usr/xpg4/bin/sh and /bin/ksh to bail out. So keep the unsets\n> # deriving from the command substitution clustered with the other\n> -- \n> 2.32.0.40.gb9b36f9b52\n> \n"},{"id":"432181","messageId":"610d6024abb9d_1594208ae@natae.notmuch","threadId":"56221","inReplyTo":"87k0kzzc91.fsf@evledraar.gmail.com","subject":"Re: [PATCH] test: fix for COLUMNS and bash 5","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-08-06T16:15:32Z","receivedAt":"2021-08-06T16:15:36Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n> On Thu, Aug 05 2021, Felipe Contreras wrote:\n> \n> > Since c49a177bec (test-lib.sh: set COLUMNS=80 for --verbose\n> > repeatability, 2021-06-29) multiple tests have been failing when using\n> > bash 5 because checkwinsize is enabled by default, therefore COLUMNS is\n> > reset using TIOCGWINSZ even for non-interactive shells.\n> >\n> > It's debatable whether or not bash should even be doing that, but for\n> > now we can avoid this undesirable behavior by disabling this option.\n> >\n> > Reported-by: Fabian Stelzer \n> > Signed-off-by: Felipe Contreras \n> \n> I've got an alternative way of solving the same immeditate issue in[1],\n> there's discussion on that approach in the latest What's Cooking[2].\n\nYes, but I'm not allowed to participate in that discussion.\n\n> My preference for mine is in no small part that I'd like to not be\n> responsible for into-the-past test suite breakage the next time a\n> popular shell decides to be clever about COLUMNS.\n> \n> But this way we'll solve the immediate problem with bash, and I can say\n> I told you so if that submarine breakage occurs with this approach :)\n\nI believe what bash is doing is a mistake. I don't think COLUMNS should\nbe updated within a non-interactive shell by default. I'll report that\nas a bug once I'm able to subsscribe to the bug-bash mailing list.\n\nSo I think it's the opposite: not only would similar fixes not be needed\nfor other shells, but it won't be needed for bash either.\n\n-- \nFelipe Contreras"},{"id":"432185","messageId":"20210806164433.8665-1-felipe.contreras@gmail.com","threadId":"56221","inReplyTo":"20210805194825.1796765-1-felipe.contreras@gmail.com","subject":"[PATCH v2] test: fix for COLUMNS and bash 5","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-08-06T16:44:33Z","receivedAt":"2021-08-06T16:44:47Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Since c49a177bec (test-lib.sh: set COLUMNS=80 for --verbose\nrepeatability, 2021-06-29) multiple tests have been failing when using\nbash 5 because checkwinsize is enabled by default, therefore COLUMNS is\nreset using TIOCGWINSZ even for non-interactive shells.\n\nIt's debatable whether or not bash should even be doing that, but for\nnow we can avoid this undesirable behavior by disabling this option.\n\nReported-by: Fabian Stelzer \nSigned-off-by: Felipe Contreras \n---\n\nSince v1 moved the code before setting COLUMNS as SZEDER Gábor suggested\nand mention checkwinsize could be set before bash 5 as Ævar Arnfjörð\nBjarmason mentioned.\n\nRange-diff against v1:\n1: 40273074de < -: ---------- test: fix for COLUMNS and bash 5\n-: ---------- > 1: 9f8c3ffa6a test: fix for COLUMNS and bash 5\n\n t/test-lib.sh | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex db61081d6b..6b1015a5af 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -409,6 +409,12 @@ then\n \tverbose=t\n fi\n \n+# In bash if checkwinsize is enabled the COLUMNS variable is updated every time\n+# an external command completes, even for non-interactive shells. Since bash 5.0\n+# this is enabled by default.\n+# Disable that since we are aiming for reproducibility.\n+test -n \"$BASH_VERSION\" && shopt -u checkwinsize 2>/dev/null\n+\n # For repeatability, reset the environment to known value.\n # TERM is sanitized below, after saving color control sequences.\n LANG=C\n-- \n2.32.0.40.gb9b36f9b52\n\n"},{"id":"432187","messageId":"xmqqmtpuik5m.fsf@gitster.g","threadId":"56221","inReplyTo":"20210806144911.GA1388237@szeder.dev","subject":"Re: [PATCH] test: fix for COLUMNS and bash 5","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-06T16:59:33Z","receivedAt":"2021-08-06T16:59:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n>> @@ -419,6 +419,12 @@ COLUMNS=80\n>\n> COLUMNS is set just before the start of the hunk context ...\n>\n>> export LANG LC_ALL PAGER TZ COLUMNS\n>> EDITOR=:\n>\n> ... so these two \"commands\" above are executed while COLUMNS is\n> already set but checkwinsize is not yet disabled. The reason I put\n> quotes around that commands is that while exporting and setting\n> variables are indeed commands as defined in the POSIX Shell Command\n> Language specs, Bash with checkwinsize enabled only \"checks the window\n> size after each extern (non-builtin) command\" (quoting 'man bash').\n>\n> So even though it is safe to execute these variable setting and\n> exporting commands after setting COLUMNS but disabling checkwinsize, I\n> think it would be prudent to disable checkwinsize before initializing\n> COLUMNS. (And perhaps adding \"non-builtin\" to the comment below.)\n\nOK. I tend to agree that a less invasive solution like this is\npreferred over adding new code to only help tests in the everyday\nbinary, especially this close to the final. Taking the above, I'd\nqueue this and hopefully I can merge it by -rc2 at the latest.\n\n---- >8 ------- >8 ------- >8 ------- >8 ------- >8 ------- >8 ----\nFrom: Felipe Contreras \nDate: Thu, 5 Aug 2021 14:48:25 -0500\nSubject: [PATCH] test: fix for COLUMNS and bash 5\n\nSince c49a177bec (test-lib.sh: set COLUMNS=80 for --verbose\nrepeatability, 2021-06-29) multiple tests have been failing when using\nbash 5 because checkwinsize is enabled by default, therefore COLUMNS is\nreset using TIOCGWINSZ even for non-interactive shells.\n\nIt's debatable whether or not bash should even be doing that, but for\nnow we can avoid this undesirable behavior by disabling this option.\n\nReported-by: Fabian Stelzer \nSigned-off-by: Felipe Contreras \n[jc: with SZEDER Gábor's suggestion to do this before setting COLUMNS]\nSigned-off-by: Junio C Hamano \n---\n t/test-lib.sh | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 4d4439a917..52701afaea 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -395,6 +395,12 @@ then\n \tverbose=t\n fi\n \n+# Since bash 5.0, checkwinsize is enabled by default which does\n+# update the COLUMNS variable every time a non-builtin command\n+# completes, even for non-interactive shells.\n+# Disable that since we are aiming for repeatability.\n+test -n \"$BASH_VERSION\" && shopt -u checkwinsize 2>/dev/null\n+\n # For repeatability, reset the environment to known value.\n # TERM is sanitized below, after saving color control sequences.\n LANG=C\n-- \n2.33.0-rc0-215-ge3efc65752\n\n"},{"id":"432217","messageId":"CAOLTT8S2_CucmhW8f6Ko8m5xUkfwSRjoqr_R_VyZdimGanMhoQ@mail.gmail.com","threadId":"56221","inReplyTo":"20210806164433.8665-1-felipe.contreras@gmail.com","subject":"Re: [PATCH v2] test: fix for COLUMNS and bash 5","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-08-07T01:37:08Z","receivedAt":"2021-08-07T01:36:23Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Felipe Contreras 于2021年8月7日周六 上午12:44写道：\n>\n> Since c49a177bec (test-lib.sh: set COLUMNS=80 for --verbose\n> repeatability, 2021-06-29) multiple tests have been failing when using\n> bash 5 because checkwinsize is enabled by default, therefore COLUMNS is\n> reset using TIOCGWINSZ even for non-interactive shells.\n>\n> It's debatable whether or not bash should even be doing that, but for\n> now we can avoid this undesirable behavior by disabling this option.\n>\n> Reported-by: Fabian Stelzer \n> Signed-off-by: Felipe Contreras \n> ---\n>\n> Since v1 moved the code before setting COLUMNS as SZEDER Gábor suggested\n> and mention checkwinsize could be set before bash 5 as Ævar Arnfjörð\n> Bjarmason mentioned.\n>\n> Range-diff against v1:\n> 1: 40273074de < -: ---------- test: fix for COLUMNS and bash 5\n> -: ---------- > 1: 9f8c3ffa6a test: fix for COLUMNS and bash 5\n>\n> t/test-lib.sh | 6 ++++++\n> 1 file changed, 6 insertions(+)\n>\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index db61081d6b..6b1015a5af 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -409,6 +409,12 @@ then\n> verbose=t\n> fi\n>\n> +# In bash if checkwinsize is enabled the COLUMNS variable is updated every time\n> +# an external command completes, even for non-interactive shells. Since bash 5.0\n> +# this is enabled by default.\n> +# Disable that since we are aiming for reproducibility.\n> +test -n \"$BASH_VERSION\" && shopt -u checkwinsize 2>/dev/null\n> +\n> # For repeatability, reset the environment to known value.\n> # TERM is sanitized below, after saving color control sequences.\n> LANG=C\n> --\n> 2.32.0.40.gb9b36f9b52\n>\n\nThis can work on Arch-Linux. LGTM.\n\nThanks.\n--\nZheNing Hu\n"},{"id":"433435","messageId":"87sfz0fh8o.fsf@evledraar.gmail.com","threadId":"56221","inReplyTo":"xmqqmtpuik5m.fsf@gitster.g","subject":"Re: [PATCH] test: fix for COLUMNS and bash 5","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-23T12:59:10Z","receivedAt":"2021-08-23T13:00:43Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Aug 06 2021, Junio C Hamano wrote:\n\n> SZEDER Gábor writes:\n>\n>>> @@ -419,6 +419,12 @@ COLUMNS=80\n>>\n>> COLUMNS is set just before the start of the hunk context ...\n>>\n>>> export LANG LC_ALL PAGER TZ COLUMNS\n>>> EDITOR=:\n>>\n>> ... so these two \"commands\" above are executed while COLUMNS is\n>> already set but checkwinsize is not yet disabled. The reason I put\n>> quotes around that commands is that while exporting and setting\n>> variables are indeed commands as defined in the POSIX Shell Command\n>> Language specs, Bash with checkwinsize enabled only \"checks the window\n>> size after each extern (non-builtin) command\" (quoting 'man bash').\n>>\n>> So even though it is safe to execute these variable setting and\n>> exporting commands after setting COLUMNS but disabling checkwinsize, I\n>> think it would be prudent to disable checkwinsize before initializing\n>> COLUMNS. (And perhaps adding \"non-builtin\" to the comment below.)\n>\n> OK. I tend to agree that a less invasive solution like this is\n> preferred over adding new code to only help tests in the everyday\n> binary, especially this close to the final. Taking the above, I'd\n> queue this and hopefully I can merge it by -rc2 at the latest.\n\nNow that we're post-release are you interested in a re-roll of\nhttps://lore.kernel.org/git/cover-v3-0.3-00000000000-20210804T230335Z-avarab@gmail.com/\n+ removal of the bash-specific checkwinsize added here, or would you\nlike to just keep Felipe's version which you integrated as 390b44eb2bc\n(test: fix for COLUMNS and bash 5, 2021-08-05).\n\nEither works for me, just poking to see whether I should drop this from\nmy local TODO list or not...\n"},{"id":"433489","messageId":"xmqqwnobq58y.fsf@gitster.g","threadId":"56221","inReplyTo":"87sfz0fh8o.fsf@evledraar.gmail.com","subject":"Re: [PATCH] test: fix for COLUMNS and bash 5","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-23T20:24:13Z","receivedAt":"2021-08-23T20:24:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Now that we're post-release are you interested in a re-roll of\n> https://lore.kernel.org/git/cover-v3-0.3-00000000000-20210804T230335Z-avarab@gmail.com/\n> + removal of the bash-specific checkwinsize added here, or would you\n\nLet's wait until we see a concrete breakage report that shows that\ncheckwinsize is insufficient. Even if such a second shell calls its\nfacility differently, as long as it works in a similar way and\nanother single liner like\n\n test -n \"$BASH_VERSION\" && shopt -u checkwinsize 2>/dev/null\n\nwe can keep piling such a single-liner next to each other, as the\nnumber of shells we need to cater to would be less than a handful\nanyway.\n\nThanks.\n"}]}