From: "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: Johannes Schindelin <johannes.schindelin@gmx.de>,
Johannes Schindelin <johannes.schindelin@gmx.de>
Subject: [PATCH v3] pager: avoid setting COLUMNS when we're guessing its value
Date: Mon, 21 Jun 2021 16:57:58 +0000 [thread overview]
Message-ID: <pull.982.v3.git.1624294679495.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.982.v2.git.1623847092299.gitgitgadget@gmail.com>
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
| 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
--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
prev parent reply other threads:[~2021-06-21 17:06 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-14 20:19 [PATCH] pager: do not unnecessarily set COLUMNS on Windows Johannes Schindelin via GitGitGadget
2021-06-15 3:02 ` Junio C Hamano
2021-06-16 12:38 ` [PATCH v2] " Johannes Schindelin via GitGitGadget
2021-06-17 2:20 ` Junio C Hamano
2021-06-17 11:43 ` Johannes Schindelin
2021-06-29 0:12 ` Junio C Hamano
2021-06-21 16:57 ` Johannes Schindelin via GitGitGadget [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=pull.982.v3.git.1624294679495.gitgitgadget@gmail.com \
--to=gitgitgadget@gmail.com \
--cc=git@vger.kernel.org \
--cc=johannes.schindelin@gmx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.