On Thu, Aug 05, 2021 at 01:06:55AM +0200, Ævar Arnfjörð Bjarmason wrote:
> > 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.
