Re: What's cooking in git.git (Aug 2021, #02; Tue, 3)

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Aug 30, 2021, 21:03 UTC
Message-ID: <20210830210336.GJ23408@szeder.dev>
In-Reply-To: <87fsvon5xm.fsf@evledraar.gmail.com>

On Thu, Aug 05, 2021 at 01:06:55AM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 9 quoted lines

> > 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.

Show 12 quoted lines

> 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.

Show 7 quoted lines

> 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.