diff mbox series

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

Message ID pull.982.v2.git.1623847092299.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series [v2] pager: do not unnecessarily set COLUMNS on Windows | expand

Commit Message

Johannes Schindelin June 16, 2021, 12:38 p.m. UTC 
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`, 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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
    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(+)


base-commit: ebf3c04b262aa27fbb97f8a0156c2347fecafafb

Comments

Junio C Hamano June 17, 2021, 2:20 a.m. UTC | #1 
"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`, 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
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>     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.

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(-)

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
Johannes Schindelin June 17, 2021, 11:43 a.m. UTC | #2 
Hi Junio,

On Thu, 17 Jun 2021, Junio C Hamano wrote:

> "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`, 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
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >     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.
>
> 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.

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

>
>  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
>
>
Junio C Hamano June 29, 2021, 12:12 a.m. UTC | #3 
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
>>
>>
diff mbox series

Patch

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);