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

On Wed, Aug 04 2021, 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.

On one hand, there is feedback to be addressed:

  https://public-inbox.org/git/20210802171429.GC23408@szeder.dev/

I read that a couple of days ago, but managed to forget about it. Sorry,
re-rolled with it addressed:
https://lore.kernel.org/git/cover-v3-0.3-00000000000-20210804T230335Z-avarab@gmail.com/ (local)

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/ (local) ...

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.