pager: do not unnecessarily set COLUMNS on Windows

From: Johannes Schindelin <[email protected]>

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, but less.exe has no problem obtaining it.

Let's not override COLUMNS in that case.

This fixes https://github.com/git-for-windows/git/issues/3235

---
pager: do not unnecessarily set COLUMNS on Windows

A recent upgrade of the "less" package in Git for Windows causes problems. Here is a work-around.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-982%2Fdscho%2Ffix-duplicated-lines-when-moving-in-pager-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-982/dscho/fix-duplicated-lines-when-moving-in-pager-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/982

 pager.c | 2 ++
 1 file changed, 2 insertions(+)

index 3d37dd7adaa2..b84668eddca2 100644
@@ -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

gitgitgadget

"Johannes Schindelin via GitGitGadget" <[email protected]>

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

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.

From: Johannes Schindelin <[email protected]>

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, and then set that environment variable.

However, on Windows it _is_ a problem. The reason is that Git for Windows uses a version of less that relies on the MSYS2 runtime to interact with the pseudo terminal (typically inside a MinTTY window, which is also aware of the MSYS2 runtime). Both MinTTY and less.exe interact with that pseudo terminal via ioctl() calls (which the MSYS2 runtime emulates even if there is no such thing on Windows).

But git.exe itself is _not_ aware of the MSYS2 runtime, or for that matter of that pseudo terminal, and has no way to call ioctl() or TIOCGWINSZ.

Therefore, git.exe will fall back to hard-coding 80 columns, no matter what the actual terminal size is.

But less.exe is totally able to interact with the MSYS2 runtime and would not actually require Git's help (which actually makes things worse here). So let's not override COLUMNS on Windows.

Note: we do this _only_ on Windows, and _only_ if TIOCGWINSZ is not defined, to reduce any potential undesired fall-out from this patch.

This fixes https://github.com/git-for-windows/git/issues/3235

---
pager: do not unnecessarily set COLUMNS on Windows

A recent upgrade of the "less" package in Git for Windows causes problems. Here is a work-around.

Changes since v1:

* The commit message was reworded to clarify the underlying issue better.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-982%2Fdscho%2Ffix-duplicated-lines-when-moving-in-pager-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-982/dscho/fix-duplicated-lines-when-moving-in-pager-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/982

Range-diff vs v1:

1: 22d3a9a2c9a2 ! 1: 82099e53bbc9 pager: do not unnecessarily set COLUMNS on Windows
@@ Commit message
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.
+ determine the correct value for COLUMNS, and then set that environment
+ variable.

- 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, but less.exe has no
- problem obtaining it.
+ However, on Windows it _is_ a problem. The reason is that Git for
+ Windows uses a version of less that relies on the MSYS2 runtime to
+ interact with the pseudo terminal (typically inside a MinTTY window,
+ which is also aware of the MSYS2 runtime). Both MinTTY and less.exe
+ interact with that pseudo terminal via ioctl() calls (which the MSYS2
+ runtime emulates even if there is no such thing on Windows).

- Let's not override COLUMNS in that case.
+ But git.exe itself is _not_ aware of the MSYS2 runtime, or for that
+ matter of that pseudo terminal, and has no way to call ioctl() or
+ TIOCGWINSZ.
+
+ Therefore, git.exe will fall back to hard-coding 80 columns, no matter
+ what the actual terminal size is.
+
+ But less.exe is totally able to interact with the MSYS2 runtime and
+ would not actually require Git's help (which actually makes things
+ worse here). So let's not override COLUMNS on Windows.
+
+ Note: we do this _only_ on Windows, and _only_ if TIOCGWINSZ is not
+ defined, to reduce any potential undesired fall-out from this patch.

This fixes https://github.com/git-for-windows/git/issues/3235

 pager.c | 2 ++
 1 file changed, 2 insertions(+)

index 3d37dd7adaa2..b84668eddca2 100644
@@ -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

gitgitgadget

"Johannes Schindelin via GitGitGadget" <[email protected]>

Thanks for an updated log message to clarify the problem description.

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.

 pager.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

index 3d37dd7ada..52f27a6765 100644
@@ -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
 

Hi Junio,

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.

Ciao,
Dscho

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

From: Johannes Schindelin <[email protected]>

We query TIOCGWINSZ in Git to determine the correct value for COLUMNS, and then set that environment variable.

If TIOCGWINSZ is not available, we fall back to the hard-coded value 80 _and still_ set the environment variable.

On Windows this is a problem. The reason is that Git for Windows uses a version of less that relies on the MSYS2 runtime to interact with the pseudo terminal (typically inside a MinTTY window, which is also aware of the MSYS2 runtime). Both MinTTY and less.exe interact with that pseudo terminal via ioctl() calls (which the MSYS2 runtime emulates even if there is no such thing on Windows). Since https://github.com/gwsw/less/commit/bb0ee4e76c2, less prefers the COLUMNS variable over asking ncurses itself.

But git.exe itself is _not_ aware of the MSYS2 runtime, or for that matter of that pseudo terminal, and has no way to call ioctl() or TIOCGWINSZ.

Therefore, git.exe will fall back to hard-coding 80 columns, no matter what the actual terminal size is.

But less.exe is totally able to interact with the MSYS2 runtime and would not actually require Git's help (which actually makes things worse here). So let's not override COLUMNS on Windows.

Let's just not set COLUMNS unless we managed to query the actual value from the terminal.

This fixes https://github.com/git-for-windows/git/issues/3235

---
pager: do not unnecessarily set COLUMNS on Windows

A recent upgrade of the "less" package in Git for Windows causes problems. Here is a work-around.

Changes since v2:

* We no longer avoid setting COLUMNS based on the Operating Systems and the presence of TIOCGWINSZ, but set a flag specifically when we successfully queried the terminal's dimensions, and otherwise skip the setenv(COLUMNS) call.

Changes since v1:

* The commit message was reworded to clarify the underlying issue better.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-982%2Fdscho%2Ffix-duplicated-lines-when-moving-in-pager-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-982/dscho/fix-duplicated-lines-when-moving-in-pager-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/982

Range-diff vs v2:

1: 82099e53bbc9 ! 1: fe2ee68de43e pager: do not unnecessarily set COLUMNS on Windows
@@ Metadata
Author: Johannes Schindelin <[email protected]>

## Commit message ##
- pager: do not unnecessarily set COLUMNS on Windows
+ pager: avoid setting COLUMNS when we're guessing its value

- Since https://github.com/gwsw/less/commit/bb0ee4e76c2, less prefers
- the COLUMNS variable over asking ncurses itself.
+ We query TIOCGWINSZ in Git to determine the correct value for
+ COLUMNS, and then set that environment variable.

- This is typically not a problem because we ask TIOCGWINSZ in Git, to
- determine the correct value for COLUMNS, and then set that environment
- variable.
+ If TIOCGWINSZ is not available, we fall back to the hard-coded value
+ 80 _and still_ set the environment variable.

- However, on Windows it _is_ a problem. The reason is that Git for
+ On Windows this is a problem. The reason is that Git for
Windows uses a version of less that relies on the MSYS2 runtime to interact with the pseudo terminal (typically inside a MinTTY window, which is also aware of the MSYS2 runtime). Both MinTTY and less.exe interact with that pseudo terminal via ioctl() calls (which the MSYS2 runtime emulates even if there is no such thing on Windows).
+ Since https://github.com/gwsw/less/commit/bb0ee4e76c2, less prefers
+ the COLUMNS variable over asking ncurses itself.

But git.exe itself is _not_ aware of the MSYS2 runtime, or for that matter of that pseudo terminal, and has no way to call ioctl() or
@@ Commit message
would not actually require Git's help (which actually makes things worse here). So let's not override COLUMNS on Windows.

- Note: we do this _only_ on Windows, and _only_ if TIOCGWINSZ is not
- defined, to reduce any potential undesired fall-out from this patch.
+ Let's just not set COLUMNS unless we managed to query the actual value
+ from the terminal.

This fixes https://github.com/git-for-windows/git/issues/3235

+ Co-authored-by: Junio C Hamano <[email protected]>

## pager.c ##
+@@
+ 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 */
@@ pager.c: 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);
+- setenv("COLUMNS", buf, 0);
++ if (!term_columns_guessed)
++ setenv("COLUMNS", buf, 0);
}
-+#endif

setenv("GIT_PAGER_IN_USE", "true", 1);
+@@ pager.c: 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

 pager.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

index 3d37dd7adaa2..52f27a6765c8 100644
@@ -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
 

base-commit: ebf3c04b262aa27fbb97f8a0156c2347fecafafb

gitgitgadget