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

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

[Johannes Schindelin via GitGitGadget]

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

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.

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

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

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

[Junio C Hamano]

"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

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

[Johannes Schindelin via GitGitGadget]

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

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

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

[Junio C Hamano]

"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
 

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

[Johannes Schindelin]

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

[PATCH v3] pager: avoid setting COLUMNS when we're guessing its value

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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

Co-authored-by: Junio C Hamano <gitster@pobox.com>
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 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 <Johannes.Schindelin@gmx.de>
      
       ## 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 <gitster@pobox.com>
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
       ## 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(-)

diff --git a/pager.c b/pager.c
index 3d37dd7adaa2..52f27a6765c8 100644
--- a/pager.c
+++ b/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
 

base-commit: ebf3c04b262aa27fbb97f8a0156c2347fecafafb
-- 
gitgitgadget

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

[Junio C Hamano]

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