Ævar Arnfjörð Bjarmason [off-list ref] writes:
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.
...
...I'd be happy to remove the helper if Junio would take that version of
the patch; :)
FWIW, I didn't *request* it; the resulting test scripts that set and
unset both the standard COLUMNS and another test-only environment
variable looked typo-prone and hard to read, and that is why I
suggested to hide that behind a helper.
If we do not have to add a test-only enviroment variable at all, I
do not see the reason why we need a helper.
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.
Hmph. Wouldn't the same argument apply to the much simpler single
liner "shopt -u" solution? When writing new tests, there is nothing
to remember, and a new shell that needs a different trick to defeat
the auto-COLUMNS would be detected quickly by running the tests in a
terminal whose width is different from 80, no?
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.
Isn't that another downside of the approach you are advocating?
If we make Git rely on GIT_TEST_COLUMNS, we may honor it while
everybody else ignores it. If we only have to deal with COLUMNS
like everybody else does, Git and other tools that are used in our
tests will be affected the same way by overly-clever shells, no?
On Wed, Aug 04 2021, Junio C Hamano wrote:
Ævar Arnfjörð Bjarmason [off-list ref] writes:
[...]
quoted
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.
Hmph. Wouldn't the same argument apply to the much simpler single
liner "shopt -u" solution? When writing new tests, there is nothing
to remember, and a new shell that needs a different trick to defeat
the auto-COLUMNS would be detected quickly by running the tests in a
terminal whose width is different from 80, no?
It's different in the "bisecting" case I mentioned. I.e. "shopt -u" is a
shell-specific solution, the approach I'm suggesting bypasses any sort
of shell-specific solutions, both current and future ones.
quoted
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.
Isn't that another downside of the approach you are advocating?
If we make Git rely on GIT_TEST_COLUMNS, we may honor it while
everybody else ignores it. If we only have to deal with COLUMNS
like everybody else does, Git and other tools that are used in our
tests will be affected the same way by overly-clever shells, no?
I think it's an upside. We know how git handles the combination of
GIT_TEST_COLUMNS, COLUMNS and TIOCGWINSZ. We have no idea how any
third-party program decides to behave.
So by intentionally spoiling COLUMNS=* and only having our tests pay
attention to GIT_TEST_COLUMNS we pretty much guarantee that we won't
grow some dependency on a non-git program's handling of COLUMNS, which
e.g. could differ from platform to platform.
I don't know about any such case, and spoiling COLUMNS didn't reveal
any. I just thought it was worthwhile to once-and-for-all get us away
from any sort of shell or 3rd party code cleverness around COLUMNS, as
opposed to a very narrow bugfix for just the issue we've spotted with
bash recently.