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

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

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

> 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.

> 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