Re: [PATCH v2] pager: do not unnecessarily set COLUMNS on Windows

[Junio C Hamano <gitster@pobox.com>]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Thu, 17 Jun 2021, Junio C Hamano wrote:
>
>> I think treating this as "less" specific band-aid is OK, but I do
>> not think tying this to Windows is a good design choice.
>>
>> The guiding principle for this change is more like "if we do not
>> know and cannot learn the true value, internally assuming 80-columns
>> as a last resort fallback may be OK, but do not export it for
>> consumption for other people---they cannot tell if COLUMNS=80 they
>> see us export is because we actually measured the terminal width and
>> know it to be 80, or we just punted and used a fallback default", I
>> think, and there is nothing Windows-specific in there, no?
>>
>> In other words, if we use something like the attached as a "less
>> specific band-aid" for now (i.e. direct replacement of your patch to
>> fix the specific 'less' problem), and then later clean it up by
>> actually returning -1 (or -80) from term_columns() as "we do not
>> know" (or "we do not know---use the negation of this value as
>> default"), we can help not just this paticular caller you touched,
>> but all other callers of term_columns(), to make a more intelligent
>> decision in the future if they wanted to.  The root of the issue I
>> think is because term_columns() does not give callers to tell if its
>> returned value is merely a guess.
>
> That approach should also work. Do you want me to take custody of your
> patch and issue a v3? If yes, I will mark you as co-author because the
> patch is not really only mine any longer.

Surely.  The fewer conditionally compiled codepaths based on
platforms we have, the easier it becomes to reason about the code, I
would think.

>>  pager.c | 16 +++++++++++++---
>>  1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git c/pager.c w/pager.c
>> index 3d37dd7ada..52f27a6765 100644
>> --- c/pager.c
>> +++ w/pager.c
>> @@ -11,6 +11,10 @@
>>  static struct child_process pager_process = CHILD_PROCESS_INIT;
>>  static const char *pager_program;
>>
>> +/* Is the value coming back from term_columns() just a guess? */
>> +static int term_columns_guessed;
>> +
>> +
>>  static void close_pager_fds(void)
>>  {
>>  	/* signal EOF to pager */
>> @@ -114,7 +118,8 @@ void setup_pager(void)
>>  	{
>>  		char buf[64];
>>  		xsnprintf(buf, sizeof(buf), "%d", term_columns());
>> -		setenv("COLUMNS", buf, 0);
>> +		if (!term_columns_guessed)
>> +			setenv("COLUMNS", buf, 0);
>>  	}
>>
>>  	setenv("GIT_PAGER_IN_USE", "true", 1);
>> @@ -158,15 +163,20 @@ int term_columns(void)
>>  		return term_columns_at_startup;
>>
>>  	term_columns_at_startup = 80;
>> +	term_columns_guessed = 1;
>>
>>  	col_string = getenv("COLUMNS");
>> -	if (col_string && (n_cols = atoi(col_string)) > 0)
>> +	if (col_string && (n_cols = atoi(col_string)) > 0) {
>>  		term_columns_at_startup = n_cols;
>> +		term_columns_guessed = 0;
>> +	}
>>  #ifdef TIOCGWINSZ
>>  	else {
>>  		struct winsize ws;
>> -		if (!ioctl(1, TIOCGWINSZ, &ws) && ws.ws_col)
>> +		if (!ioctl(1, TIOCGWINSZ, &ws) && ws.ws_col) {
>>  			term_columns_at_startup = ws.ws_col;
>> +			term_columns_guessed = 0;
>> +		}
>>  	}
>>  #endif
>>
>>