pager: do not unnecessarily set COLUMNS on Windows
PATCH 1/1 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(+)
pager.c +2 -0
@@ -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
Signature · 1 line
gitgitgadget
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 11 lines · writes:
From: Johannes Schindelin <[email protected]>
Since https://github.com/gwsw/less/commit/bb0ee4e76c2,
lessprefers theCOLUMNSvariable over asking ncurses itself.This is typically not a problem because we ask
TIOCGWINSZin Git, to determine the correct value forCOLUMNS.However, on Windows it _is_ a problem because Git for Windows' Bash (and
less.exe) uses the MSYS2 runtime whilegit.exedoes _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.
Quoted 4 lines
but
less.exehas no
problem obtaining it.Let's not override
COLUMNSin 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.
Quoted 20 lines
pager.c +2
-0@@ -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 1/1 pager: do not unnecessarily set COLUMNS on Windows
In reply to Johannes Schindelin via GitGitGadgetFrom: 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(+)
pager.c +2 -0
@@ -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
Signature · 1 line
gitgitgadget
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 36 lines · writes:
From: Johannes Schindelin <[email protected]>
Since https://github.com/gwsw/less/commit/bb0ee4e76c2,
lessprefers theCOLUMNSvariable over asking ncurses itself.This is typically not a problem because we ask
TIOCGWINSZin Git, to determine the correct value forCOLUMNS, and then set that environment variable.However, on Windows it _is_ a problem. The reason is that Git for Windows uses a version of
lessthat 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 andless.exeinteract with that pseudo terminal viaioctl()calls (which the MSYS2 runtime emulates even if there is no such thing on Windows).But
git.exeitself is _not_ aware of the MSYS2 runtime, or for that matter of that pseudo terminal, and has no way to callioctl()orTIOCGWINSZ.Therefore,
git.exewill fall back to hard-coding 80 columns, no matter what the actual terminal size is.But
less.exeis 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 overrideCOLUMNSon Windows.Note: we do this _only_ on Windows, and _only_ if
TIOCGWINSZis 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 <[email protected]>---
pager: do not unnecessarily set COLUMNS on Windows
Quoted 7 lines
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(-)
pager.c +13 -3
@@ -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,
Quoted 61 lines · Junio C Hamano
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 36 lines · writes:
From: Johannes Schindelin <[email protected]>
Since https://github.com/gwsw/less/commit/bb0ee4e76c2,
lessprefers theCOLUMNSvariable over asking ncurses itself.This is typically not a problem because we ask
TIOCGWINSZin Git, to determine the correct value forCOLUMNS, and then set that environment variable.However, on Windows it _is_ a problem. The reason is that Git for Windows uses a version of
lessthat 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 andless.exeinteract with that pseudo terminal viaioctl()calls (which the MSYS2 runtime emulates even if there is no such thing on Windows).But
git.exeitself is _not_ aware of the MSYS2 runtime, or for that matter of that pseudo terminal, and has no way to callioctl()orTIOCGWINSZ.Therefore,
git.exewill fall back to hard-coding 80 columns, no matter what the actual terminal size is.But
less.exeis 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 overrideCOLUMNSon Windows.Note: we do this _only_ on Windows, and _only_ if
TIOCGWINSZis 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 <[email protected]>---
pager: do not unnecessarily set COLUMNS on WindowsQuoted 7 lines
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?
Quoted 10 lines
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 ofterm_columns(), to make a more intelligent decision in the future if they wanted to. The root of the issue I think is becauseterm_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
Quoted 53 lines
pager.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)pager.c +13
-3@@ -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 1/1 pager: avoid setting COLUMNS when we're guessing its value
In reply to Johannes Schindelin via GitGitGadgetFrom: 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(-)
pager.c +13 -3
@@ -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
Signature · 1 line
gitgitgadget
Quoted 24 lines · Johannes Schindelin
Quoted 11 lines · Junio C Hamano
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?
Quoted 10 lines
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 ofterm_columns(), to make a more intelligent decision in the future if they wanted to. The root of the issue I think is becauseterm_columns()does not give callers to tell if its returned value is merely a guess.
Quoted 3 lines
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.
Quoted 52 lines
Quoted 52 lines
pager.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)pager.c +13
-3@@ -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