From: Junio C Hamano <hidden> Date: 2021-08-04 18:07:00
Ævar Arnfjörð Bjarmason [off-list ref] writes:
quoted
* ab/test-columns (2021-08-02) 3 commits
- test-lib.sh: use GIT_TEST_COLUMNS over COLUMNS
- test-lib-functions.sh: add a test_with_columns function
- test-lib-functions.sh: rename test_must_fail_acceptable()
We're going to need this or another solution to the v2.33.0-rc0
regression in c49a177beca (test-lib.sh: set COLUMNS=80 for --verbose
repeatability, 2021-06-29) before the final v2.33.0.
Just a question. Is that true? Wouldn't a system that needs these
on top of c49a177beca already break the tests without c49a177beca?
IOW, is c49a177beca truly a "regression", or is it merely "a half
solution that solves for most but not all platforms"?
* ab/test-columns (2021-08-02) 3 commits
- test-lib.sh: use GIT_TEST_COLUMNS over COLUMNS
- test-lib-functions.sh: add a test_with_columns function
- test-lib-functions.sh: rename test_must_fail_acceptable()
We're going to need this or another solution to the v2.33.0-rc0
regression in c49a177beca (test-lib.sh: set COLUMNS=80 for --verbose
repeatability, 2021-06-29) before the final v2.33.0.
Just a question. Is that true? Wouldn't a system that needs these
on top of c49a177beca already break the tests without c49a177beca?
IOW, is c49a177beca truly a "regression", or is it merely "a half
solution that solves for most but not all platforms"?
Yes, because with c49a177beca your tests only break if you use the
--verbose option, i.e. if your stderr is connected to a terminal. I.e.:
./t0500-progress-display.sh --verbose
So in practice it mostly affects git developers who run with --verbose,
but probably nobody doing a build in the wild.
With c49a177beca they break on e.g.:
/bin/bash ./t0500-progress-display.sh
If your bash is recent enough, so "make test" if you're on a system with
a recent bash whose /bin/sh is /bin/bash.
This is because post-c49a177beca we don't "unset" COLUMNS anymore, which
bash takes as license to update it.
So we really do need that series in before the release to avoid that
common annoyance, a revert of c49a177beca is also possible, i.e. it
would still leave things broken under --verbose, but that breakage is
rare and existed before v2.33.
I think given the triviality of the fixes and that the cause is
well-understood it makes sense to go forward in this case.
From: SZEDER Gábor <hidden> Date: 2021-08-04 21:28:36
On Wed, Aug 04, 2021 at 09:53:02PM +0200, Ævar Arnfjörð Bjarmason wrote:
On Wed, Aug 04 2021, Junio C Hamano wrote:
quoted
Ævar Arnfjörð Bjarmason [off-list ref] writes:
quoted
quoted
* ab/test-columns (2021-08-02) 3 commits
- test-lib.sh: use GIT_TEST_COLUMNS over COLUMNS
- test-lib-functions.sh: add a test_with_columns function
- test-lib-functions.sh: rename test_must_fail_acceptable()
We're going to need this or another solution to the v2.33.0-rc0
regression in c49a177beca (test-lib.sh: set COLUMNS=80 for --verbose
repeatability, 2021-06-29) before the final v2.33.0.
Just a question. Is that true? Wouldn't a system that needs these
on top of c49a177beca already break the tests without c49a177beca?
IOW, is c49a177beca truly a "regression", or is it merely "a half
solution that solves for most but not all platforms"?
Yes, because with c49a177beca your tests only break if you use the
--verbose option, i.e. if your stderr is connected to a terminal. I.e.:
./t0500-progress-display.sh --verbose
So in practice it mostly affects git developers who run with --verbose,
but probably nobody doing a build in the wild.
With c49a177beca they break on e.g.:
/bin/bash ./t0500-progress-display.sh
If your bash is recent enough, so "make test" if you're on a system with
a recent bash whose /bin/sh is /bin/bash.
This is because post-c49a177beca we don't "unset" COLUMNS anymore, which
bash takes as license to update it.
So we really do need that series in before the release to avoid that
common annoyance, a revert of c49a177beca is also possible, i.e. it
would still leave things broken under --verbose, but that breakage is
rare and existed before v2.33.
I think given the triviality of the fixes and that the cause is
well-understood it makes sense to go forward in this case.
On one hand, there is feedback to be addressed:
https://public-inbox.org/git/20210802171429.GC23408@szeder.dev/
OTOH, setting the checkwinsize is the truly trivial, minimal, reliable
and uncontroversial fix for this issue, and IMO that should go into
the next release.
This fix in this patch series is not trivial: it introduces yet
another GIT_TEST variable and a helper function that developers will
have to remember to use in the future. Worse, this means that despite
aiming for future proofing I can't consider this approach future
proof, because it's easy to forget about such a rarely used test
helper function, and if anyone introduces yet another test setting
COLUMNS, then that will be prone to similar failures when run with
Bash.
I don't think that in this case we should aim for future proofing when
the cost is the additional cognitive load of yet another helper
function. I would instead prefer to go with the really trivial fix
for now and wait whether this issue pops up again with other shell or
terminal, hoping that this issue is a "one-hit-wonder" [1] and it
won't happen ever again.
[1] "one-hit-wonder" as in:
https://en.wikipedia.org/wiki/Bloom_filter#Cache_filtering
From: SZEDER Gábor <hidden> Date: 2021-08-04 21:36:32
On Wed, Aug 04, 2021 at 11:28:25PM +0200, SZEDER Gábor wrote:
On Wed, Aug 04, 2021 at 09:53:02PM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted
On Wed, Aug 04 2021, Junio C Hamano wrote:
quoted
Ævar Arnfjörð Bjarmason [off-list ref] writes:
quoted
quoted
* ab/test-columns (2021-08-02) 3 commits
- test-lib.sh: use GIT_TEST_COLUMNS over COLUMNS
- test-lib-functions.sh: add a test_with_columns function
- test-lib-functions.sh: rename test_must_fail_acceptable()
We're going to need this or another solution to the v2.33.0-rc0
regression in c49a177beca (test-lib.sh: set COLUMNS=80 for --verbose
repeatability, 2021-06-29) before the final v2.33.0.
Just a question. Is that true? Wouldn't a system that needs these
on top of c49a177beca already break the tests without c49a177beca?
IOW, is c49a177beca truly a "regression", or is it merely "a half
solution that solves for most but not all platforms"?
Yes, because with c49a177beca your tests only break if you use the
--verbose option, i.e. if your stderr is connected to a terminal. I.e.:
./t0500-progress-display.sh --verbose
So in practice it mostly affects git developers who run with --verbose,
but probably nobody doing a build in the wild.
With c49a177beca they break on e.g.:
/bin/bash ./t0500-progress-display.sh
If your bash is recent enough, so "make test" if you're on a system with
a recent bash whose /bin/sh is /bin/bash.
This is because post-c49a177beca we don't "unset" COLUMNS anymore, which
bash takes as license to update it.
So we really do need that series in before the release to avoid that
common annoyance, a revert of c49a177beca is also possible, i.e. it
would still leave things broken under --verbose, but that breakage is
rare and existed before v2.33.
I think given the triviality of the fixes and that the cause is
well-understood it makes sense to go forward in this case.
and uncontroversial fix for this issue, and IMO that should go into
the next release.
This fix in this patch series is not trivial: it introduces yet
another GIT_TEST variable and a helper function that developers will
have to remember to use in the future. Worse, this means that despite
aiming for future proofing I can't consider this approach future
proof, because it's easy to forget about such a rarely used test
helper function, and if anyone introduces yet another test setting
COLUMNS, then that will be prone to similar failures when run with
Bash.
I don't think that in this case we should aim for future proofing when
the cost is the additional cognitive load of yet another helper
function. I would instead prefer to go with the really trivial fix
for now and wait whether this issue pops up again with other shell or
terminal, hoping that this issue is a "one-hit-wonder" [1] and it
won't happen ever again.
[1] "one-hit-wonder" as in:
https://en.wikipedia.org/wiki/Bloom_filter#Cache_filtering
On Wed, Aug 04, 2021 at 09:53:02PM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted
On Wed, Aug 04 2021, Junio C Hamano wrote:
quoted
Ævar Arnfjörð Bjarmason [off-list ref] writes:
quoted
quoted
* ab/test-columns (2021-08-02) 3 commits
- test-lib.sh: use GIT_TEST_COLUMNS over COLUMNS
- test-lib-functions.sh: add a test_with_columns function
- test-lib-functions.sh: rename test_must_fail_acceptable()
We're going to need this or another solution to the v2.33.0-rc0
regression in c49a177beca (test-lib.sh: set COLUMNS=80 for --verbose
repeatability, 2021-06-29) before the final v2.33.0.
Just a question. Is that true? Wouldn't a system that needs these
on top of c49a177beca already break the tests without c49a177beca?
IOW, is c49a177beca truly a "regression", or is it merely "a half
solution that solves for most but not all platforms"?
Yes, because with c49a177beca your tests only break if you use the
--verbose option, i.e. if your stderr is connected to a terminal. I.e.:
./t0500-progress-display.sh --verbose
So in practice it mostly affects git developers who run with --verbose,
but probably nobody doing a build in the wild.
With c49a177beca they break on e.g.:
/bin/bash ./t0500-progress-display.sh
If your bash is recent enough, so "make test" if you're on a system with
a recent bash whose /bin/sh is /bin/bash.
This is because post-c49a177beca we don't "unset" COLUMNS anymore, which
bash takes as license to update it.
So we really do need that series in before the release to avoid that
common annoyance, a revert of c49a177beca is also possible, i.e. it
would still leave things broken under --verbose, but that breakage is
rare and existed before v2.33.
I think given the triviality of the fixes and that the cause is
well-understood it makes sense to go forward in this case.
OTOH, setting the checkwinsize is the truly trivial, minimal, reliable
and uncontroversial fix for this issue, and IMO that should go into
the next release.
Addressed below.
This fix in this patch series is not trivial: it introduces yet
another GIT_TEST variable and a helper function that developers will
have to remember to use in the future. Worse, this means that despite
aiming for future proofing I can't consider this approach future
proof, because it's easy to forget about such a rarely used test
helper function, and if anyone introduces yet another test setting
COLUMNS, then that will be prone to similar failures when run with
Bash.
How would it be forgotten? If you introduce tests like the ones changed
in 1/3 of the series and expect git to pay attention to COLUMNS you'll
find that they won't work, because if you set COLUMNS=123 we won't take
it over the GIT_TEST_COLUMNS=80 set in test-lib.sh.
So it seems like a fairly easy-to-discover thing. You'll grep for that
setting and find test_with_columns().
Most tests (such as t0500-progress-display.sh) won't need to concern
themselves with the new helper, it's only for those tests that want to
check how git itself behaves with a custom COLUMNS value, as opposed to
just wanting consistent repeatable results.
In any case, I didn't think a helper was needed in this case, it's
something Junio requested:
https://lore.kernel.org/git/xmqqzgu7b6of.fsf@gitster.g/ ...
I don't think that in this case we should aim for future proofing when
the cost is the additional cognitive load of yet another helper
function. I would instead prefer to go with the really trivial fix
for now and wait whether this issue pops up again with other shell or
terminal, hoping that this issue is a "one-hit-wonder" [1] and it
won't happen ever again.
...I'd be happy to remove the helper if Junio would take that version of
the patch; :)
But in the topic of the overall approach, I think it's worth
future-proofing here mainly because it's useful to be able to reliably
run "make test" on old commits for bisecting, which is a property we
mostly manage to uphold.
By narrowly targeting a fix at one specific shell's cleverness around
COLUMNS we'll leave open a window where we'll fail on other shells if
they introduce similar cleverness.
It hardly seems like a stretch that once bash starts doing that sort of
thing other shells might think to follow suit, and all have their own
non-standard way to turn it off.
You also didn't address the other rationale for it, namely that it's
also future-proofing us for submarine breakages in non-git programs
which'll understand the new COLUMNS=10, but not GIT_TEST_COLUMNS=80.
I.e. should our tests rely on their output, and those programs
themselves change how they treat e.g. COLUMNS v.s. TIOCGWINSZ any tests
relying on their output will change their behavior.
From: SZEDER Gábor <hidden> Date: 2021-08-30 21:03:42
On Thu, Aug 05, 2021 at 01:06:55AM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted
I don't think that in this case we should aim for future proofing when
the cost is the additional cognitive load of yet another helper
function. I would instead prefer to go with the really trivial fix
for now and wait whether this issue pops up again with other shell or
terminal, hoping that this issue is a "one-hit-wonder" [1] and it
won't happen ever again.
...I'd be happy to remove the helper if Junio would take that version of
the patch; :)
I have to agree with Junio here: setting 'GIT_TEST_COLUMNS=
COLUMNS=80' for each affected command like the first patch did was
just too ugly.
But in the topic of the overall approach, I think it's worth
future-proofing here mainly because it's useful to be able to reliably
run "make test" on old commits for bisecting, which is a property we
mostly manage to uphold.
By narrowly targeting a fix at one specific shell's cleverness around
COLUMNS we'll leave open a window where we'll fail on other shells if
they introduce similar cleverness.
It hardly seems like a stretch that once bash starts doing that sort of
thing other shells might think to follow suit, and all have their own
non-standard way to turn it off.
Bash has a lot of non-standard features that other shells decided to
avoid, even when they are really useful and unconroversial. To me it
does seems like a stretch that other shells will follow suit with
COLUMNS, when our first reaction to Bash's changed behavior was that
it's buggy.
You also didn't address the other rationale for it, namely that it's
also future-proofing us for submarine breakages in non-git programs
which'll understand the new COLUMNS=10, but not GIT_TEST_COLUMNS=80.
I.e. should our tests rely on their output, and those programs
themselves change how they treat e.g. COLUMNS v.s. TIOCGWINSZ any tests
relying on their output will change their behavior.
I occasionally scan through the test logs looking for any new
suspicious error messages from non-git commands and shell builtins, and
to find those error messages I had to collect the list of non-git
commands [1] executed in the test suite:
awk|basename|cat|chmod|cmp|comm|curl|cut|date|dd|diff|dirname|egrep|env|envsubst|expr|fgrep|find|getfacl|gettext|grep|gunzip|gzip|head|hostname|iconv|id|jgit|kill|ln|mkdir|mkfifo|mktemp|mv|perl|readlink|sed|setfacl|sleep|sort|tail|tar|touch|tr|uname|uniq|unzip|wc|xargs|xmllint|zipinfo|
Most of these are fairly low-level system commands, where supporting
COLUMNS in any way wouldn't make sense at all, or, worse, would be a
serious regression. Commands like 'curl', 'gunzip' or 'gzip' could
show a progress bar matching the width of the terminal, but our tests
should never rely on something like that, ever. Then there is 'jgit'
which could support COLUMNS for output intended to the user, just like
git does, but, again, our tests should never rely on that.
[1] It's an incomplete list, e.g. 'cp', 'ls', and 'rm' are
intentionally left out, because they are too often used in cases
when they can legitimately fail.