{"thread":{"id":"55930","subject":"[PATCH] pager: do not unnecessarily set COLUMNS on Windows","startedAt":"2021-06-14T20:20:23Z","lastAt":"2021-06-29T00:12:43Z","messageCount":7,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"427382","messageId":"pull.982.git.1623701952823.gitgitgadget@gmail.com","threadId":"55930","inReplyTo":null,"subject":"[PATCH] pager: do not unnecessarily set COLUMNS on Windows","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-06-14T20:19:12Z","receivedAt":"2021-06-14T20:20:23Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nSince https://github.com/gwsw/less/commit/bb0ee4e76c2, `less` prefers\nthe `COLUMNS` variable over asking ncurses itself.\n\nThis is typically not a problem because we ask `TIOCGWINSZ` in Git, to\ndetermine the correct value for `COLUMNS`.\n\nHowever, on Windows it _is_ a problem because Git for Windows' Bash (and\n`less.exe`) uses the MSYS2 runtime while `git.exe` does _not_, and\ntherefore we never get the correct value in Git, but `less.exe` has no\nproblem obtaining it.\n\nLet's not override `COLUMNS` in that case.\n\nThis fixes https://github.com/git-for-windows/git/issues/3235\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n    pager: do not unnecessarily set COLUMNS on Windows\n    \n    A recent upgrade of the \"less\" package in Git for Windows causes\n    problems. Here is a work-around.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-982%2Fdscho%2Ffix-duplicated-lines-when-moving-in-pager-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-982/dscho/fix-duplicated-lines-when-moving-in-pager-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/982\n\n pager.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/pager.c b/pager.c\nindex 3d37dd7adaa2..b84668eddca2 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -111,11 +111,13 @@ void setup_pager(void)\n \t * to get the terminal size. Let's grab it now, and then set $COLUMNS\n \t * to communicate it to any sub-processes.\n \t */\n+#if !defined(WIN32) || defined(TIOCGWINSZ)\n \t{\n \t\tchar buf[64];\n \t\txsnprintf(buf, sizeof(buf), \"%d\", term_columns());\n \t\tsetenv(\"COLUMNS\", buf, 0);\n \t}\n+#endif\n \n \tsetenv(\"GIT_PAGER_IN_USE\", \"true\", 1);\n \n\nbase-commit: ebf3c04b262aa27fbb97f8a0156c2347fecafafb\n-- \ngitgitgadget\n"},{"id":"427400","messageId":"xmqqh7hzygng.fsf@gitster.g","threadId":"55930","inReplyTo":"pull.982.git.1623701952823.gitgitgadget@gmail.com","subject":"Re: [PATCH] pager: do not unnecessarily set COLUMNS on Windows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-15T03:02:11Z","receivedAt":"2021-06-15T03:02:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" <gitgitgadget@gmail.com>\nwrites:\n\n> From: Johannes Schindelin <johannes.schindelin@gmx.de>\n>\n> Since https://github.com/gwsw/less/commit/bb0ee4e76c2, `less` prefers\n> the `COLUMNS` variable over asking ncurses itself.\n>\n> This is typically not a problem because we ask `TIOCGWINSZ` in Git, to\n> determine the correct value for `COLUMNS`.\n>\n> However, on Windows it _is_ a problem because Git for Windows' Bash (and\n> `less.exe`) uses the MSYS2 runtime while `git.exe` does _not_, and\n> therefore we never get the correct value in Git, ...\n\n... meaning in git.exe, ioctl issued in term_columns() does not\nwork, or TIOCGWINSZ is not even defined while building git.exe,\nso all it does is to default to 80 or use COLUMNS exported by\nsomebody else?\n\nThese three lines need a bit of clarification.\n\n> but `less.exe` has no\n> problem obtaining it.\n>\n> Let's not override `COLUMNS` in that case.\n\nIf term_columns() sees existing COLUMNS, then the current behaviour\nis a no-op, so the problem is specifically what happens when there\nis no existing COLUMNS and we override it with the hardcoded default\nof 80?  I guess this all makes sense, but \"in that case\" may want to\nget clarified.\n\nProvided if my guesses above are all correct, the change makes\nsort-of sense to me, but I wonder why !defined(WIN32) is there in\nthe first place.  Doesn't any platform that lack TIOCGWINSZ have the\nsame issue (not with \"less\" specifically, but \"we export 80 that has\nno relation to reality in COLUMNS---we should leave it unset if we\ndo not know\")?\n\nThis is a tangent, but there are other callers of term_columns().\n\"git column\" obviously wants to use it for determining display\ncolumns, \"git diff\" wants to measure how many columns to allocate\nfor function name shown on the hunk header lines, and how wide the\ndiffstat should be, and the progress bar adjusts to the terminal\nwidth, too.\n\n> diff --git a/pager.c b/pager.c\n> index 3d37dd7adaa2..b84668eddca2 100644\n> --- a/pager.c\n> +++ b/pager.c\n> @@ -111,11 +111,13 @@ void setup_pager(void)\n>  \t * to get the terminal size. Let's grab it now, and then set $COLUMNS\n>  \t * to communicate it to any sub-processes.\n>  \t */\n> +#if !defined(WIN32) || defined(TIOCGWINSZ)\n>  \t{\n>  \t\tchar buf[64];\n>  \t\txsnprintf(buf, sizeof(buf), \"%d\", term_columns());\n>  \t\tsetenv(\"COLUMNS\", buf, 0);\n>  \t}\n> +#endif\n>  \n>  \tsetenv(\"GIT_PAGER_IN_USE\", \"true\", 1);\n>  \n>\n> base-commit: ebf3c04b262aa27fbb97f8a0156c2347fecafafb\n"},{"id":"427608","messageId":"pull.982.v2.git.1623847092299.gitgitgadget@gmail.com","threadId":"55930","inReplyTo":"pull.982.git.1623701952823.gitgitgadget@gmail.com","subject":"[PATCH v2] pager: do not unnecessarily set COLUMNS on Windows","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-06-16T12:38:11Z","receivedAt":"2021-06-16T12:38:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nSince https://github.com/gwsw/less/commit/bb0ee4e76c2, `less` prefers\nthe `COLUMNS` variable over asking ncurses itself.\n\nThis is typically not a problem because we ask `TIOCGWINSZ` in Git, to\ndetermine the correct value for `COLUMNS`, and then set that environment\nvariable.\n\nHowever, on Windows it _is_ a problem. The reason is that Git for\nWindows uses a version of `less` that relies on the MSYS2 runtime to\ninteract with the pseudo terminal (typically inside a MinTTY window,\nwhich is also aware of the MSYS2 runtime). Both MinTTY and `less.exe`\ninteract with that pseudo terminal via `ioctl()` calls (which the MSYS2\nruntime emulates even if there is no such thing on Windows).\n\nBut `git.exe` itself is _not_ aware of the MSYS2 runtime, or for that\nmatter of that pseudo terminal, and has no way to call `ioctl()` or\n`TIOCGWINSZ`.\n\nTherefore, `git.exe` will fall back to hard-coding 80 columns, no matter\nwhat the actual terminal size is.\n\nBut `less.exe` is totally able to interact with the MSYS2 runtime and\nwould not actually require Git's help (which actually makes things\nworse here). So let's not override `COLUMNS` on Windows.\n\nNote: we do this _only_ on Windows, and _only_ if `TIOCGWINSZ` is not\ndefined, to reduce any potential undesired fall-out from this patch.\n\nThis fixes https://github.com/git-for-windows/git/issues/3235\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n    pager: do not unnecessarily set COLUMNS on Windows\n    \n    A recent upgrade of the \"less\" package in Git for Windows causes\n    problems. Here is a work-around.\n    \n    Changes since v1:\n    \n     * The commit message was reworded to clarify the underlying issue\n       better.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-982%2Fdscho%2Ffix-duplicated-lines-when-moving-in-pager-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-982/dscho/fix-duplicated-lines-when-moving-in-pager-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/982\n\nRange-diff vs v1:\n\n 1:  22d3a9a2c9a2 ! 1:  82099e53bbc9 pager: do not unnecessarily set COLUMNS on Windows\n     @@ Commit message\n          the `COLUMNS` variable over asking ncurses itself.\n      \n          This is typically not a problem because we ask `TIOCGWINSZ` in Git, to\n     -    determine the correct value for `COLUMNS`.\n     +    determine the correct value for `COLUMNS`, and then set that environment\n     +    variable.\n      \n     -    However, on Windows it _is_ a problem because Git for Windows' Bash (and\n     -    `less.exe`) uses the MSYS2 runtime while `git.exe` does _not_, and\n     -    therefore we never get the correct value in Git, but `less.exe` has no\n     -    problem obtaining it.\n     +    However, on Windows it _is_ a problem. The reason is that Git for\n     +    Windows uses a version of `less` that relies on the MSYS2 runtime to\n     +    interact with the pseudo terminal (typically inside a MinTTY window,\n     +    which is also aware of the MSYS2 runtime). Both MinTTY and `less.exe`\n     +    interact with that pseudo terminal via `ioctl()` calls (which the MSYS2\n     +    runtime emulates even if there is no such thing on Windows).\n      \n     -    Let's not override `COLUMNS` in that case.\n     +    But `git.exe` itself is _not_ aware of the MSYS2 runtime, or for that\n     +    matter of that pseudo terminal, and has no way to call `ioctl()` or\n     +    `TIOCGWINSZ`.\n     +\n     +    Therefore, `git.exe` will fall back to hard-coding 80 columns, no matter\n     +    what the actual terminal size is.\n     +\n     +    But `less.exe` is totally able to interact with the MSYS2 runtime and\n     +    would not actually require Git's help (which actually makes things\n     +    worse here). So let's not override `COLUMNS` on Windows.\n     +\n     +    Note: we do this _only_ on Windows, and _only_ if `TIOCGWINSZ` is not\n     +    defined, to reduce any potential undesired fall-out from this patch.\n      \n          This fixes https://github.com/git-for-windows/git/issues/3235\n      \n\n\n pager.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/pager.c b/pager.c\nindex 3d37dd7adaa2..b84668eddca2 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -111,11 +111,13 @@ void setup_pager(void)\n \t * to get the terminal size. Let's grab it now, and then set $COLUMNS\n \t * to communicate it to any sub-processes.\n \t */\n+#if !defined(WIN32) || defined(TIOCGWINSZ)\n \t{\n \t\tchar buf[64];\n \t\txsnprintf(buf, sizeof(buf), \"%d\", term_columns());\n \t\tsetenv(\"COLUMNS\", buf, 0);\n \t}\n+#endif\n \n \tsetenv(\"GIT_PAGER_IN_USE\", \"true\", 1);\n \n\nbase-commit: ebf3c04b262aa27fbb97f8a0156c2347fecafafb\n-- \ngitgitgadget\n"},{"id":"427652","messageId":"xmqqv96dmduh.fsf@gitster.g","threadId":"55930","inReplyTo":"pull.982.v2.git.1623847092299.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] pager: do not unnecessarily set COLUMNS on Windows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-17T02:20:22Z","receivedAt":"2021-06-17T02:20:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" <gitgitgadget@gmail.com>\nwrites:\n\n> From: Johannes Schindelin <johannes.schindelin@gmx.de>\n>\n> Since https://github.com/gwsw/less/commit/bb0ee4e76c2, `less` prefers\n> the `COLUMNS` variable over asking ncurses itself.\n>\n> This is typically not a problem because we ask `TIOCGWINSZ` in Git, to\n> determine the correct value for `COLUMNS`, and then set that environment\n> variable.\n>\n> However, on Windows it _is_ a problem. The reason is that Git for\n> Windows uses a version of `less` that relies on the MSYS2 runtime to\n> interact with the pseudo terminal (typically inside a MinTTY window,\n> which is also aware of the MSYS2 runtime). Both MinTTY and `less.exe`\n> interact with that pseudo terminal via `ioctl()` calls (which the MSYS2\n> runtime emulates even if there is no such thing on Windows).\n>\n> But `git.exe` itself is _not_ aware of the MSYS2 runtime, or for that\n> matter of that pseudo terminal, and has no way to call `ioctl()` or\n> `TIOCGWINSZ`.\n>\n> Therefore, `git.exe` will fall back to hard-coding 80 columns, no matter\n> what the actual terminal size is.\n>\n> But `less.exe` is totally able to interact with the MSYS2 runtime and\n> would not actually require Git's help (which actually makes things\n> worse here). So let's not override `COLUMNS` on Windows.\n>\n> Note: we do this _only_ on Windows, and _only_ if `TIOCGWINSZ` is not\n> defined, to reduce any potential undesired fall-out from this patch.\n>\n> This fixes https://github.com/git-for-windows/git/issues/3235\n>\n> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> ---\n>     pager: do not unnecessarily set COLUMNS on Windows\n>     \n>     A recent upgrade of the \"less\" package in Git for Windows causes\n>     problems. Here is a work-around.\n>     \n>     Changes since v1:\n>     \n>      * The commit message was reworded to clarify the underlying issue\n>        better.\n\nThanks for an updated log message to clarify the problem\ndescription.\n\nI think treating this as \"less\" specific band-aid is OK, but I do\nnot think tying this to Windows is a good design choice.\n\nThe guiding principle for this change is more like \"if we do not\nknow and cannot learn the true value, internally assuming 80-columns\nas a last resort fallback may be OK, but do not export it for\nconsumption for other people---they cannot tell if COLUMNS=80 they\nsee us export is because we actually measured the terminal width and\nknow it to be 80, or we just punted and used a fallback default\", I\nthink, and there is nothing Windows-specific in there, no?\n\nIn other words, if we use something like the attached as a \"less\nspecific band-aid\" for now (i.e. direct replacement of your patch to\nfix the specific 'less' problem), and then later clean it up by\nactually returning -1 (or -80) from term_columns() as \"we do not\nknow\" (or \"we do not know---use the negation of this value as\ndefault\"), we can help not just this paticular caller you touched,\nbut all other callers of term_columns(), to make a more intelligent\ndecision in the future if they wanted to.  The root of the issue I\nthink is because term_columns() does not give callers to tell if its\nreturned value is merely a guess.\n\n pager.c | 16 +++++++++++++---\n 1 file changed, 13 insertions(+), 3 deletions(-)\n\ndiff --git c/pager.c w/pager.c\nindex 3d37dd7ada..52f27a6765 100644\n--- c/pager.c\n+++ w/pager.c\n@@ -11,6 +11,10 @@\n static struct child_process pager_process = CHILD_PROCESS_INIT;\n static const char *pager_program;\n \n+/* Is the value coming back from term_columns() just a guess? */\n+static int term_columns_guessed;\n+\n+\n static void close_pager_fds(void)\n {\n \t/* signal EOF to pager */\n@@ -114,7 +118,8 @@ void setup_pager(void)\n \t{\n \t\tchar buf[64];\n \t\txsnprintf(buf, sizeof(buf), \"%d\", term_columns());\n-\t\tsetenv(\"COLUMNS\", buf, 0);\n+\t\tif (!term_columns_guessed)\n+\t\t\tsetenv(\"COLUMNS\", buf, 0);\n \t}\n \n \tsetenv(\"GIT_PAGER_IN_USE\", \"true\", 1);\n@@ -158,15 +163,20 @@ int term_columns(void)\n \t\treturn term_columns_at_startup;\n \n \tterm_columns_at_startup = 80;\n+\tterm_columns_guessed = 1;\n \n \tcol_string = getenv(\"COLUMNS\");\n-\tif (col_string && (n_cols = atoi(col_string)) > 0)\n+\tif (col_string && (n_cols = atoi(col_string)) > 0) {\n \t\tterm_columns_at_startup = n_cols;\n+\t\tterm_columns_guessed = 0;\n+\t}\n #ifdef TIOCGWINSZ\n \telse {\n \t\tstruct winsize ws;\n-\t\tif (!ioctl(1, TIOCGWINSZ, &ws) && ws.ws_col)\n+\t\tif (!ioctl(1, TIOCGWINSZ, &ws) && ws.ws_col) {\n \t\t\tterm_columns_at_startup = ws.ws_col;\n+\t\t\tterm_columns_guessed = 0;\n+\t\t}\n \t}\n #endif\n \n"},{"id":"427743","messageId":"nycvar.QRO.7.76.6.2106171342270.57@tvgsbejvaqbjf.bet","threadId":"55930","inReplyTo":"xmqqv96dmduh.fsf@gitster.g","subject":"Re: [PATCH v2] pager: do not unnecessarily set COLUMNS on Windows","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-06-17T11:43:32Z","receivedAt":"2021-06-17T11:43:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 17 Jun 2021, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" <gitgitgadget@gmail.com>\n> writes:\n>\n> > From: Johannes Schindelin <johannes.schindelin@gmx.de>\n> >\n> > Since https://github.com/gwsw/less/commit/bb0ee4e76c2, `less` prefers\n> > the `COLUMNS` variable over asking ncurses itself.\n> >\n> > This is typically not a problem because we ask `TIOCGWINSZ` in Git, to\n> > determine the correct value for `COLUMNS`, and then set that environment\n> > variable.\n> >\n> > However, on Windows it _is_ a problem. The reason is that Git for\n> > Windows uses a version of `less` that relies on the MSYS2 runtime to\n> > interact with the pseudo terminal (typically inside a MinTTY window,\n> > which is also aware of the MSYS2 runtime). Both MinTTY and `less.exe`\n> > interact with that pseudo terminal via `ioctl()` calls (which the MSYS2\n> > runtime emulates even if there is no such thing on Windows).\n> >\n> > But `git.exe` itself is _not_ aware of the MSYS2 runtime, or for that\n> > matter of that pseudo terminal, and has no way to call `ioctl()` or\n> > `TIOCGWINSZ`.\n> >\n> > Therefore, `git.exe` will fall back to hard-coding 80 columns, no matter\n> > what the actual terminal size is.\n> >\n> > But `less.exe` is totally able to interact with the MSYS2 runtime and\n> > would not actually require Git's help (which actually makes things\n> > worse here). So let's not override `COLUMNS` on Windows.\n> >\n> > Note: we do this _only_ on Windows, and _only_ if `TIOCGWINSZ` is not\n> > defined, to reduce any potential undesired fall-out from this patch.\n> >\n> > This fixes https://github.com/git-for-windows/git/issues/3235\n> >\n> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> > ---\n> >     pager: do not unnecessarily set COLUMNS on Windows\n> >\n> >     A recent upgrade of the \"less\" package in Git for Windows causes\n> >     problems. Here is a work-around.\n> >\n> >     Changes since v1:\n> >\n> >      * The commit message was reworded to clarify the underlying issue\n> >        better.\n>\n> Thanks for an updated log message to clarify the problem\n> description.\n>\n> I think treating this as \"less\" specific band-aid is OK, but I do\n> not think tying this to Windows is a good design choice.\n>\n> The guiding principle for this change is more like \"if we do not\n> know and cannot learn the true value, internally assuming 80-columns\n> as a last resort fallback may be OK, but do not export it for\n> consumption for other people---they cannot tell if COLUMNS=80 they\n> see us export is because we actually measured the terminal width and\n> know it to be 80, or we just punted and used a fallback default\", I\n> think, and there is nothing Windows-specific in there, no?\n>\n> In other words, if we use something like the attached as a \"less\n> specific band-aid\" for now (i.e. direct replacement of your patch to\n> fix the specific 'less' problem), and then later clean it up by\n> actually returning -1 (or -80) from term_columns() as \"we do not\n> know\" (or \"we do not know---use the negation of this value as\n> default\"), we can help not just this paticular caller you touched,\n> but all other callers of term_columns(), to make a more intelligent\n> decision in the future if they wanted to.  The root of the issue I\n> think is because term_columns() does not give callers to tell if its\n> returned value is merely a guess.\n\nThat approach should also work. Do you want me to take custody of your\npatch and issue a v3? If yes, I will mark you as co-author because the\npatch is not really only mine any longer.\n\nCiao,\nDscho\n\n>\n>  pager.c | 16 +++++++++++++---\n>  1 file changed, 13 insertions(+), 3 deletions(-)\n>\n> diff --git c/pager.c w/pager.c\n> index 3d37dd7ada..52f27a6765 100644\n> --- c/pager.c\n> +++ w/pager.c\n> @@ -11,6 +11,10 @@\n>  static struct child_process pager_process = CHILD_PROCESS_INIT;\n>  static const char *pager_program;\n>\n> +/* Is the value coming back from term_columns() just a guess? */\n> +static int term_columns_guessed;\n> +\n> +\n>  static void close_pager_fds(void)\n>  {\n>  \t/* signal EOF to pager */\n> @@ -114,7 +118,8 @@ void setup_pager(void)\n>  \t{\n>  \t\tchar buf[64];\n>  \t\txsnprintf(buf, sizeof(buf), \"%d\", term_columns());\n> -\t\tsetenv(\"COLUMNS\", buf, 0);\n> +\t\tif (!term_columns_guessed)\n> +\t\t\tsetenv(\"COLUMNS\", buf, 0);\n>  \t}\n>\n>  \tsetenv(\"GIT_PAGER_IN_USE\", \"true\", 1);\n> @@ -158,15 +163,20 @@ int term_columns(void)\n>  \t\treturn term_columns_at_startup;\n>\n>  \tterm_columns_at_startup = 80;\n> +\tterm_columns_guessed = 1;\n>\n>  \tcol_string = getenv(\"COLUMNS\");\n> -\tif (col_string && (n_cols = atoi(col_string)) > 0)\n> +\tif (col_string && (n_cols = atoi(col_string)) > 0) {\n>  \t\tterm_columns_at_startup = n_cols;\n> +\t\tterm_columns_guessed = 0;\n> +\t}\n>  #ifdef TIOCGWINSZ\n>  \telse {\n>  \t\tstruct winsize ws;\n> -\t\tif (!ioctl(1, TIOCGWINSZ, &ws) && ws.ws_col)\n> +\t\tif (!ioctl(1, TIOCGWINSZ, &ws) && ws.ws_col) {\n>  \t\t\tterm_columns_at_startup = ws.ws_col;\n> +\t\t\tterm_columns_guessed = 0;\n> +\t\t}\n>  \t}\n>  #endif\n>\n>\n"},{"id":"428076","messageId":"pull.982.v3.git.1624294679495.gitgitgadget@gmail.com","threadId":"55930","inReplyTo":"pull.982.v2.git.1623847092299.gitgitgadget@gmail.com","subject":"[PATCH v3] pager: avoid setting COLUMNS when we're guessing its value","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-06-21T16:57:58Z","receivedAt":"2021-06-21T17:06:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nWe query `TIOCGWINSZ` in Git to determine the correct value for\n`COLUMNS`, and then set that environment variable.\n\nIf `TIOCGWINSZ` is not available, we fall back to the hard-coded value\n80 _and still_ set the environment variable.\n\nOn Windows this is a problem. The reason is that Git for\nWindows uses a version of `less` that relies on the MSYS2 runtime to\ninteract with the pseudo terminal (typically inside a MinTTY window,\nwhich is also aware of the MSYS2 runtime). Both MinTTY and `less.exe`\ninteract with that pseudo terminal via `ioctl()` calls (which the MSYS2\nruntime emulates even if there is no such thing on Windows).\nSince https://github.com/gwsw/less/commit/bb0ee4e76c2, `less` prefers\nthe `COLUMNS` variable over asking ncurses itself.\n\nBut `git.exe` itself is _not_ aware of the MSYS2 runtime, or for that\nmatter of that pseudo terminal, and has no way to call `ioctl()` or\n`TIOCGWINSZ`.\n\nTherefore, `git.exe` will fall back to hard-coding 80 columns, no matter\nwhat the actual terminal size is.\n\nBut `less.exe` is totally able to interact with the MSYS2 runtime and\nwould not actually require Git's help (which actually makes things\nworse here). So let's not override `COLUMNS` on Windows.\n\nLet's just not set `COLUMNS` unless we managed to query the actual value\nfrom the terminal.\n\nThis fixes https://github.com/git-for-windows/git/issues/3235\n\nCo-authored-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n    pager: do not unnecessarily set COLUMNS on Windows\n    \n    A recent upgrade of the \"less\" package in Git for Windows causes\n    problems. Here is a work-around.\n    \n    Changes since v2:\n    \n     * We no longer avoid setting COLUMNS based on the Operating Systems and\n       the presence of TIOCGWINSZ, but set a flag specifically when we\n       successfully queried the terminal's dimensions, and otherwise skip\n       the setenv(COLUMNS) call.\n    \n    Changes since v1:\n    \n     * The commit message was reworded to clarify the underlying issue\n       better.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-982%2Fdscho%2Ffix-duplicated-lines-when-moving-in-pager-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-982/dscho/fix-duplicated-lines-when-moving-in-pager-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/982\n\nRange-diff vs v2:\n\n 1:  82099e53bbc9 ! 1:  fe2ee68de43e pager: do not unnecessarily set COLUMNS on Windows\n     @@ Metadata\n      Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>\n      \n       ## Commit message ##\n     -    pager: do not unnecessarily set COLUMNS on Windows\n     +    pager: avoid setting COLUMNS when we're guessing its value\n      \n     -    Since https://github.com/gwsw/less/commit/bb0ee4e76c2, `less` prefers\n     -    the `COLUMNS` variable over asking ncurses itself.\n     +    We query `TIOCGWINSZ` in Git to determine the correct value for\n     +    `COLUMNS`, and then set that environment variable.\n      \n     -    This is typically not a problem because we ask `TIOCGWINSZ` in Git, to\n     -    determine the correct value for `COLUMNS`, and then set that environment\n     -    variable.\n     +    If `TIOCGWINSZ` is not available, we fall back to the hard-coded value\n     +    80 _and still_ set the environment variable.\n      \n     -    However, on Windows it _is_ a problem. The reason is that Git for\n     +    On Windows this is a problem. The reason is that Git for\n          Windows uses a version of `less` that relies on the MSYS2 runtime to\n          interact with the pseudo terminal (typically inside a MinTTY window,\n          which is also aware of the MSYS2 runtime). Both MinTTY and `less.exe`\n          interact with that pseudo terminal via `ioctl()` calls (which the MSYS2\n          runtime emulates even if there is no such thing on Windows).\n     +    Since https://github.com/gwsw/less/commit/bb0ee4e76c2, `less` prefers\n     +    the `COLUMNS` variable over asking ncurses itself.\n      \n          But `git.exe` itself is _not_ aware of the MSYS2 runtime, or for that\n          matter of that pseudo terminal, and has no way to call `ioctl()` or\n     @@ Commit message\n          would not actually require Git's help (which actually makes things\n          worse here). So let's not override `COLUMNS` on Windows.\n      \n     -    Note: we do this _only_ on Windows, and _only_ if `TIOCGWINSZ` is not\n     -    defined, to reduce any potential undesired fall-out from this patch.\n     +    Let's just not set `COLUMNS` unless we managed to query the actual value\n     +    from the terminal.\n      \n          This fixes https://github.com/git-for-windows/git/issues/3235\n      \n     +    Co-authored-by: Junio C Hamano <gitster@pobox.com>\n          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n      \n       ## pager.c ##\n     +@@\n     + static struct child_process pager_process = CHILD_PROCESS_INIT;\n     + static const char *pager_program;\n     + \n     ++/* Is the value coming back from term_columns() just a guess? */\n     ++static int term_columns_guessed;\n     ++\n     ++\n     + static void close_pager_fds(void)\n     + {\n     + \t/* signal EOF to pager */\n      @@ pager.c: void setup_pager(void)\n     - \t * to get the terminal size. Let's grab it now, and then set $COLUMNS\n     - \t * to communicate it to any sub-processes.\n     - \t */\n     -+#if !defined(WIN32) || defined(TIOCGWINSZ)\n       \t{\n       \t\tchar buf[64];\n       \t\txsnprintf(buf, sizeof(buf), \"%d\", term_columns());\n     - \t\tsetenv(\"COLUMNS\", buf, 0);\n     +-\t\tsetenv(\"COLUMNS\", buf, 0);\n     ++\t\tif (!term_columns_guessed)\n     ++\t\t\tsetenv(\"COLUMNS\", buf, 0);\n       \t}\n     -+#endif\n       \n       \tsetenv(\"GIT_PAGER_IN_USE\", \"true\", 1);\n     +@@ pager.c: int term_columns(void)\n     + \t\treturn term_columns_at_startup;\n     + \n     + \tterm_columns_at_startup = 80;\n     ++\tterm_columns_guessed = 1;\n     + \n     + \tcol_string = getenv(\"COLUMNS\");\n     +-\tif (col_string && (n_cols = atoi(col_string)) > 0)\n     ++\tif (col_string && (n_cols = atoi(col_string)) > 0) {\n     + \t\tterm_columns_at_startup = n_cols;\n     ++\t\tterm_columns_guessed = 0;\n     ++\t}\n     + #ifdef TIOCGWINSZ\n     + \telse {\n     + \t\tstruct winsize ws;\n     +-\t\tif (!ioctl(1, TIOCGWINSZ, &ws) && ws.ws_col)\n     ++\t\tif (!ioctl(1, TIOCGWINSZ, &ws) && ws.ws_col) {\n     + \t\t\tterm_columns_at_startup = ws.ws_col;\n     ++\t\t\tterm_columns_guessed = 0;\n     ++\t\t}\n     + \t}\n     + #endif\n       \n\n\n pager.c | 16 +++++++++++++---\n 1 file changed, 13 insertions(+), 3 deletions(-)\n\ndiff --git a/pager.c b/pager.c\nindex 3d37dd7adaa2..52f27a6765c8 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -11,6 +11,10 @@\n static struct child_process pager_process = CHILD_PROCESS_INIT;\n static const char *pager_program;\n \n+/* Is the value coming back from term_columns() just a guess? */\n+static int term_columns_guessed;\n+\n+\n static void close_pager_fds(void)\n {\n \t/* signal EOF to pager */\n@@ -114,7 +118,8 @@ void setup_pager(void)\n \t{\n \t\tchar buf[64];\n \t\txsnprintf(buf, sizeof(buf), \"%d\", term_columns());\n-\t\tsetenv(\"COLUMNS\", buf, 0);\n+\t\tif (!term_columns_guessed)\n+\t\t\tsetenv(\"COLUMNS\", buf, 0);\n \t}\n \n \tsetenv(\"GIT_PAGER_IN_USE\", \"true\", 1);\n@@ -158,15 +163,20 @@ int term_columns(void)\n \t\treturn term_columns_at_startup;\n \n \tterm_columns_at_startup = 80;\n+\tterm_columns_guessed = 1;\n \n \tcol_string = getenv(\"COLUMNS\");\n-\tif (col_string && (n_cols = atoi(col_string)) > 0)\n+\tif (col_string && (n_cols = atoi(col_string)) > 0) {\n \t\tterm_columns_at_startup = n_cols;\n+\t\tterm_columns_guessed = 0;\n+\t}\n #ifdef TIOCGWINSZ\n \telse {\n \t\tstruct winsize ws;\n-\t\tif (!ioctl(1, TIOCGWINSZ, &ws) && ws.ws_col)\n+\t\tif (!ioctl(1, TIOCGWINSZ, &ws) && ws.ws_col) {\n \t\t\tterm_columns_at_startup = ws.ws_col;\n+\t\t\tterm_columns_guessed = 0;\n+\t\t}\n \t}\n #endif\n \n\nbase-commit: ebf3c04b262aa27fbb97f8a0156c2347fecafafb\n-- \ngitgitgadget\n"},{"id":"428651","messageId":"xmqq4kdh5y16.fsf@gitster.g","threadId":"55930","inReplyTo":"nycvar.QRO.7.76.6.2106171342270.57@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2] pager: do not unnecessarily set COLUMNS on Windows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-29T00:12:37Z","receivedAt":"2021-06-29T00:12:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> On Thu, 17 Jun 2021, Junio C Hamano wrote:\n>\n>> I think treating this as \"less\" specific band-aid is OK, but I do\n>> not think tying this to Windows is a good design choice.\n>>\n>> The guiding principle for this change is more like \"if we do not\n>> know and cannot learn the true value, internally assuming 80-columns\n>> as a last resort fallback may be OK, but do not export it for\n>> consumption for other people---they cannot tell if COLUMNS=80 they\n>> see us export is because we actually measured the terminal width and\n>> know it to be 80, or we just punted and used a fallback default\", I\n>> think, and there is nothing Windows-specific in there, no?\n>>\n>> In other words, if we use something like the attached as a \"less\n>> specific band-aid\" for now (i.e. direct replacement of your patch to\n>> fix the specific 'less' problem), and then later clean it up by\n>> actually returning -1 (or -80) from term_columns() as \"we do not\n>> know\" (or \"we do not know---use the negation of this value as\n>> default\"), we can help not just this paticular caller you touched,\n>> but all other callers of term_columns(), to make a more intelligent\n>> decision in the future if they wanted to.  The root of the issue I\n>> think is because term_columns() does not give callers to tell if its\n>> returned value is merely a guess.\n>\n> That approach should also work. Do you want me to take custody of your\n> patch and issue a v3? If yes, I will mark you as co-author because the\n> patch is not really only mine any longer.\n\nSurely.  The fewer conditionally compiled codepaths based on\nplatforms we have, the easier it becomes to reason about the code, I\nwould think.\n\n>>  pager.c | 16 +++++++++++++---\n>>  1 file changed, 13 insertions(+), 3 deletions(-)\n>>\n>> diff --git c/pager.c w/pager.c\n>> index 3d37dd7ada..52f27a6765 100644\n>> --- c/pager.c\n>> +++ w/pager.c\n>> @@ -11,6 +11,10 @@\n>>  static struct child_process pager_process = CHILD_PROCESS_INIT;\n>>  static const char *pager_program;\n>>\n>> +/* Is the value coming back from term_columns() just a guess? */\n>> +static int term_columns_guessed;\n>> +\n>> +\n>>  static void close_pager_fds(void)\n>>  {\n>>  \t/* signal EOF to pager */\n>> @@ -114,7 +118,8 @@ void setup_pager(void)\n>>  \t{\n>>  \t\tchar buf[64];\n>>  \t\txsnprintf(buf, sizeof(buf), \"%d\", term_columns());\n>> -\t\tsetenv(\"COLUMNS\", buf, 0);\n>> +\t\tif (!term_columns_guessed)\n>> +\t\t\tsetenv(\"COLUMNS\", buf, 0);\n>>  \t}\n>>\n>>  \tsetenv(\"GIT_PAGER_IN_USE\", \"true\", 1);\n>> @@ -158,15 +163,20 @@ int term_columns(void)\n>>  \t\treturn term_columns_at_startup;\n>>\n>>  \tterm_columns_at_startup = 80;\n>> +\tterm_columns_guessed = 1;\n>>\n>>  \tcol_string = getenv(\"COLUMNS\");\n>> -\tif (col_string && (n_cols = atoi(col_string)) > 0)\n>> +\tif (col_string && (n_cols = atoi(col_string)) > 0) {\n>>  \t\tterm_columns_at_startup = n_cols;\n>> +\t\tterm_columns_guessed = 0;\n>> +\t}\n>>  #ifdef TIOCGWINSZ\n>>  \telse {\n>>  \t\tstruct winsize ws;\n>> -\t\tif (!ioctl(1, TIOCGWINSZ, &ws) && ws.ws_col)\n>> +\t\tif (!ioctl(1, TIOCGWINSZ, &ws) && ws.ws_col) {\n>>  \t\t\tterm_columns_at_startup = ws.ws_col;\n>> +\t\t\tterm_columns_guessed = 0;\n>> +\t\t}\n>>  \t}\n>>  #endif\n>>\n>>\n"}]}