Re: [PATCH] test-lib.sh: use GIT_TEST_COLUMNS over COLUMNS

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 28, 2021, 00:53 UTC
Message-ID: <xmqqzgu7b6of.fsf@gitster.g>
In-Reply-To: <YQBEhj/Y6m0pqOth@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 21 quoted lines

>> Let's instead solve this more thoroughly. We'll now take
>> GIT_TEST_COLUMNS over COLUMNS, and furthermore intentionally spoil the
>> COLUMNS variable to break any tests that rely on it being set to a
>> sane value.
>> 
>> If something breaks because we have a codepath that's not
>> term_columns() checking COLUMNS we'd like to know about it, the narrow
>> "shopt -u checkwinsize" fix won't give us that.
>
> I guess people running with bash won't see the test breakage (because
> bash will quietly "fix" the COLUMNS setting). But enough people run with
> /bin/sh that we'll eventually notice.
>
>> This approach does mean that any tests of ours that expected to test
>> term_columns() behavior by setting COLUMNS will need to explicitly
>> unset GIT_TEST_COLUMNS, or set it to the empty string. I'm doing the
>> latter in all the tests changed here.
>
> This is rather ugly, and I'm not in general a fan of adding more
> test-only code into the bowels of Git itself. But it may be the least
> bad solution.

Yeah, this really look unsatisfactory, especially with this repeated pattern that is hard to read:

+ GIT_TEST_COLUMNS= COLUMNS=81 git branch --column=column >actual && + GIT_TEST_COLUMNS= COLUMNS=80 git branch --column=column >actual && + GIT_TEST_COLUMNS= COLUMNS=80 git branch >actual && + GIT_TEST_COLUMNS= COLUMNS=80 git branch -v | cut -c -8 | sed "s/ *$//" >actual &&

Perhaps with something like

	test_with_columns () {
        	local columns=$1
		shift
		GIT_TEST_COLUMNS= COLUMNS=$columns "$@"
	}

we may be able to hide the ugly implementation detail like this:

	test_with_columns 81 git branch --column=column >actual

and may become a bit more palatable? A good thing is that this can be done as two-step process, with its first step being

    s/^(\s*)COLUMNS=(\d+)/$1test_with_columns $2/;

plus addition of the helper to test-lib, perhaps like so:

	test_with_columns () {
        	local columns=$1
		shift
		COLUMNS=$columns "$@"
	}

and the whole GIT_TEST_COLUMNS stuff being the second step.