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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 15, 2021, 03:02 UTC
Message-ID: <xmqqh7hzygng.fsf@gitster.g>
In-Reply-To: <pull.982.git.1623701952823.gitgitgadget@gmail.com>

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 11 quoted lines

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> Since https://github.com/gwsw/less/commit/bb0ee4e76c2, `less` prefers
> the `COLUMNS` variable over asking ncurses itself.
>
> This is typically not a problem because we ask `TIOCGWINSZ` in Git, to
> determine the correct value for `COLUMNS`.
>
> However, on Windows it _is_ a problem because Git for Windows' Bash (and
> `less.exe`) uses the MSYS2 runtime while `git.exe` does _not_, and
> therefore we never get the correct value in Git, ...

... meaning in git.exe, ioctl issued in term_columns() does not work, or TIOCGWINSZ is not even defined while building git.exe, so all it does is to default to 80 or use COLUMNS exported by somebody else?

These three lines need a bit of clarification.

> but `less.exe` has no
> problem obtaining it.
>
> Let's not override `COLUMNS` in that case.

If term_columns() sees existing COLUMNS, then the current behaviour is a no-op, so the problem is specifically what happens when there is no existing COLUMNS and we override it with the hardcoded default of 80? I guess this all makes sense, but "in that case" may want to get clarified.

Provided if my guesses above are all correct, the change makes sort-of sense to me, but I wonder why !defined(WIN32) is there in the first place. Doesn't any platform that lack TIOCGWINSZ have the same issue (not with "less" specifically, but "we export 80 that has no relation to reality in COLUMNS---we should leave it unset if we do not know")?

This is a tangent, but there are other callers of term_columns(). "git column" obviously wants to use it for determining display columns, "git diff" wants to measure how many columns to allocate for function name shown on the hunk header lines, and how wide the diffstat should be, and the progress bar adjusts to the terminal width, too.

Show 20 quoted lines

> diff --git a/pager.c b/pager.c
> index 3d37dd7adaa2..b84668eddca2 100644
> --- a/pager.c
> +++ b/pager.c
> @@ -111,11 +111,13 @@ void setup_pager(void)
>  	 * to get the terminal size. Let's grab it now, and then set $COLUMNS
>  	 * to communicate it to any sub-processes.
>  	 */
> +#if !defined(WIN32) || defined(TIOCGWINSZ)
>  	{
>  		char buf[64];
>  		xsnprintf(buf, sizeof(buf), "%d", term_columns());
>  		setenv("COLUMNS", buf, 0);
>  	}
> +#endif
>  
>  	setenv("GIT_PAGER_IN_USE", "true", 1);
>  
>
> base-commit: ebf3c04b262aa27fbb97f8a0156c2347fecafafb