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

["SZEDER Gábor" <szeder.dev@gmail.com>]

On Wed, Aug 04, 2021 at 09:53:02PM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Aug 04 2021, Junio C Hamano wrote:
> 
> > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> >
> >>> * 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