Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Duplicated lines when moving in pager #3235

Closed
t-b opened this issue May 24, 2021 · 39 comments · Fixed by #3272
Closed

Duplicated lines when moving in pager #3235

t-b opened this issue May 24, 2021 · 39 comments · Fixed by #3272
Milestone

Comments

@t-b
Copy link

t-b commented May 24, 2021

Setup

  • Which version of Git for Windows are you using? Is it 32-bit or 64-bit?

64bit

$ git --version --build-options

git version 2.32.0.rc0.windows.1    cpu: x86_64                                                                                                             built from commit: d837c6707a646a69003f0ed2a490911c30954894       sizeof-long: 4                                                                                                          sizeof-size_t: 8
shell-path: /bin/sh
feature: fsmonitor--daemon                                                                                              
  • Which version of Windows are you running? Vista, 7, 8, 10? Is it 32-bit or 64-bit?
$ cmd.exe /c ver

Microsoft Windows [Version 10.0.19042.985] 
  • What options did you set as part of the installation? Or did you choose the
    defaults?
# One of the following:
> type "C:\Program Files\Git\etc\install-options.txt"
> type "C:\Program Files (x86)\Git\etc\install-options.txt"
> type "%USERPROFILE%\AppData\Local\Programs\Git\etc\install-options.txt"
$ cat /etc/install-options.txt

Editor Option: VIM
Custom Editor Path:
Default Branch Option:
Path Option: Cmd
SSH Option: OpenSSH
Tortoise Option: false
CURL Option: OpenSSL
CRLF Option: CRLFCommitAsIs
Bash Terminal Option: ConHost
Git Pull Behavior Option: Rebase
Use Credential Manager: Disabled
Performance Tweaks FSCache: Enabled
Enable Symlinks: Disabled
Enable Pseudo Console Support: Disabled

Details

Problem shows in cmd and in Windows terminal. I haven't tried something else.

git log ecf7
# now move one down, one up with the arrow keys

and then the screen looks like

grafik

notice the duplicated line at the top. Somehow the pager thinks it has to break the line. But it is even not correctly broken. When I resize the cmd window it is redrawn correctly. This is especially annoying when the code itself is longer than the magical maximum line length.

This bug was not present with 2.31.x.

@dscho
Copy link
Member

dscho commented May 24, 2021

This bug was not present with 2.31.x.

Could you bisect through the snapshots?

@t-b
Copy link
Author

t-b commented May 25, 2021

@dscho Sure.

cc58341f46-20210525114532: present
de4a643419-20210517161711: present
67e748cc61-20210408225318: not present
0489a04b6d-20210404102631: not present
92b3d7c08c-20210305223722: not present

@dscho
Copy link
Member

dscho commented May 25, 2021

de4a643-20210517161711: present
67e748c-20210408225318: not present

Could you diff the /etc/package-versions.txt files of these versions? I imagine it might be the most recent update to less. But then, it might be an msys2-runtime (although I don't think we accepted any changes in that component in the past few months).

@t-b
Copy link
Author

t-b commented May 25, 2021

$ diff -Nur 67e748c de4a643
--- 67e748c     2021-05-25 19:05:01.143102900 +0200
+++ de4a643     2021-05-25 19:06:40.413573800 +0200
@@ -2,33 +2,33 @@
 apr-util 1.6.1-1
 bash 4.4.023-1
 bzip2 1.0.8-2
-ca-certificates 20190110-1
+ca-certificates 20210119-1
 coreutils 8.32-1
 diffutils 3.7-1
 docx2txt 1.4-1
 dos2unix 7.4.2-1
 expat 2.3.0-1
-file 5.40-1
+file 5.40-2
 findutils 4.8.0-1
 gawk 5.0.0-1
 gcc-libs 10.2.0-1
 gettext 0.19.8.1-1
-git-extra 1.1.520.98cc8ec-1
+git-extra 1.1.523.d946137-1
 git-flow 1.12.3-1
-glib2 2.66.6-1
+glib2 2.68.1-1
 gmp 6.2.1-1
 gnupg 2.2.27-1
 grep 3.1-1
 gzip 1.10-1
 heimdal-libs 7.5.0-3
 icu 68.2-1
-less 563-2
+less 581-1
 libasprintf 0.19.8.1-1
 libassuan 2.5.5-1
 libbz2 1.0.8-2
 libcbor 0.8.0-1
-libcrypt 2.1-2
-libcurl 7.76.0-1
+libcrypt 2.1-3
+libcurl 7.76.1-1
 libedit 20210216_3.1-1
 libexpat 2.3.0-1
 libffi 3.3-1
@@ -55,60 +55,60 @@
 libpsl 0.21.1-2
 libreadline 8.1.0-1
 libsasl 2.1.27-1
-libserf 1.3.9-5
-libsqlite 3.35.4-1
+libserf 1.3.9-6
+libsqlite 3.35.5-1
 libssh2 1.9.0-1
 libtasn1 4.16.0-1
 libunistring 0.9.10-1
 libutil-linux 2.35.2-1
-libxml2 2.9.10-7
-libxslt 1.1.34-3
+libxml2 2.9.10-9
+libxslt 1.1.34-4
 mingw-w64-x86_64-antiword 0.37-2
 mingw-w64-x86_64-brotli 1.0.9-2
 mingw-w64-x86_64-bzip2 1.0.8-2
 mingw-w64-x86_64-ca-certificates 20200601-3
 mingw-w64-x86_64-c-ares 1.17.1-1
 mingw-w64-x86_64-connect 1.105-2
-mingw-w64-x86_64-curl 7.76.0-1
+mingw-w64-x86_64-curl 7.76.1-1
 mingw-w64-x86_64-expat 2.2.10-1
-mingw-w64-x86_64-gcc-libs 10.2.0-9
+mingw-w64-x86_64-gcc-libs 10.3.0-2
 mingw-w64-x86_64-gettext 0.19.8.1-10
-mingw-w64-x86_64-git 2.31.1.windows.1.28.g67e748cc61.20210408225318-1
+mingw-w64-x86_64-git 2.32.0.rc0.windows.1.4.gde4a643419.20210517161711-1
 mingw-w64-x86_64-git-credential-manager 1.20.0-1
-mingw-w64-x86_64-git-credential-manager-core 2.0.394.50751-1
-mingw-w64-x86_64-git-doc-html 2.31.1.windows.1.28.g67e748cc61.20210408225318-1
+mingw-w64-x86_64-git-credential-manager-core 2.0.435.9025-1
+mingw-w64-x86_64-git-doc-html 2.32.0.rc0.windows.1.4.gde4a643419.20210517161711-1
 mingw-w64-x86_64-git-lfs 2.13.3-1
-mingw-w64-x86_64-gmp 6.2.1-1
-mingw-w64-x86_64-gnutls 3.7.0-2
+mingw-w64-x86_64-gmp 6.2.1-2
+mingw-w64-x86_64-gnutls 3.7.1-1
 mingw-w64-x86_64-jansson 2.13.1-1
-mingw-w64-x86_64-jemalloc 5.2.1-1
-mingw-w64-x86_64-libffi 3.3-3
+mingw-w64-x86_64-jemalloc 5.2.1-2
+mingw-w64-x86_64-libffi 3.3-4
 mingw-w64-x86_64-libiconv 1.16-2
 mingw-w64-x86_64-libidn2 2.3.0-1
 mingw-w64-x86_64-libmetalink 0.1.3-3
-mingw-w64-x86_64-libssh2 1.9.0-2
+mingw-w64-x86_64-libssh2 1.9.0-3
 mingw-w64-x86_64-libsystre 1.0.1-4
-mingw-w64-x86_64-libtasn1 4.16.0-1
+mingw-w64-x86_64-libtasn1 4.16.0-2
 mingw-w64-x86_64-libtre-git r128.6fb7206-2
-mingw-w64-x86_64-libunistring 0.9.10-2
-mingw-w64-x86_64-libwinpthread-git 9.0.0.6154.e37b315b-1
+mingw-w64-x86_64-libunistring 0.9.10-3
+mingw-w64-x86_64-libwinpthread-git 9.0.0.6184.ab0fa5ad-1
 mingw-w64-x86_64-libzip 1.7.3-1
 mingw-w64-x86_64-mpc 1.2.1-1
 mingw-w64-x86_64-mpfr 4.1.0-3
-mingw-w64-x86_64-nettle 3.7-1
+mingw-w64-x86_64-nettle 3.7-2
 mingw-w64-x86_64-nghttp2 1.41.0-1
 mingw-w64-x86_64-odt2txt 0.5-2
 mingw-w64-x86_64-openssl 1.1.1.k-1
-mingw-w64-x86_64-pcre 8.44-2
+mingw-w64-x86_64-pcre 8.44-3
 mingw-w64-x86_64-pcre2 10.36-1
-mingw-w64-x86_64-tcl 8.6.11-2
-mingw-w64-x86_64-tk 8.6.11.1-1
+mingw-w64-x86_64-tcl 8.6.11-3
+mingw-w64-x86_64-tk 8.6.11.1-2
 mingw-w64-x86_64-wineditline 2.205-3
 mingw-w64-x86_64-wintoast 1.0.0.181.9b0663d-1
 mingw-w64-x86_64-xz 5.2.5-2
 mingw-w64-x86_64-zlib 1.2.11-9
-mingw-w64-x86_64-zstd 1.4.8-2
-mintty 1~3.4.7-1
+mingw-w64-x86_64-zstd 1.5.0-1
+mintty 1~3.5.0-1
 mpfr 4.1.0-1
 msys2-runtime 3.1.7-5
 nano 5.6.1-1
@@ -156,13 +156,13 @@
 tcl 8.6.10-1
 tig 2.5.3-1
 unzip 6.0-2
-vim 8.2.2689-1
+vim 8.2.2689-2
 which 2.21-2
 winpty 0.4.3-1
 zlib 1.2.11-1
-filesystem 2021.03-6
+filesystem 2021.04-4
 dash 0.5.11.3-1
-rebase 4.4.4-2
+rebase 4.4.4-3
 util-linux 2.35.2-1
 unzip 6.0-2
 mingw-w64-x86_64-xpdf-tools 4.00-1
(base)

67e748c.txt
de4a643.txt

Looks like less is to blame.

@dscho
Copy link
Member

dscho commented May 26, 2021

Looks like less is to blame.

Could you test that by specifically copying de4a643's less.exe into a 67e748c snapshot?

@t-b
Copy link
Author

t-b commented May 28, 2021

I tested using 67e748c-less into an installation of git version 2.32.0.rc2.windows.1 and the error is gone. This is the reverse test you were asking for but this should be as good or?

@dscho
Copy link
Member

dscho commented Jun 1, 2021

It shows that less is the culprit, yes. Next step: bisecting through less 563..581. You can do that by

  1. installing Git for Windows' SDK,
  2. sdk cd less,
  3. editing PKGBUILD
  4. build Git via sdk build
  5. test in-place via either copying less.exe directly, or via pacman -U <pkg>

Note: not all version numbers between those versions are available for download, see http://greenwoodsoftware.com/less/download.html. You could also see whether the two newer versions fix the issue.

@KalleOlaviNiemitalo
Copy link

KalleOlaviNiemitalo commented Jun 9, 2021

http://greenwoodsoftware.com/less/download.html does not list any versions between 563 and 581.2 numerically.

The changes between less 563 and 581 include "Patch for Windows virtual terminal", which sets variables related to line wrapping. I don't know whether that causes the problem, though.

The changes between less 581 and 581.2 don't include anything relevant.

@bmcdonnell-fb
Copy link

Following an enlarging resize of the Git Bash window, paging in less (at least) is pretty borked in since Git for Windows 2.32.0. Initially it doesn't populate to the bottom of the window, and wraps some lines prematurely. Then when scrolling it omits lines, and maybe duplicates some.

I've reverted to 2.31.1 because of it.

@dscho
Copy link
Member

dscho commented Jun 10, 2021

http://greenwoodsoftware.com/less/download.html does not list any versions between 563 and 581.2 numerically.

The changes between less 563 and 581 include "Patch for Windows virtual terminal", which sets variables related to line wrapping. I don't know whether that causes the problem, though.

The changes between less 581 and 581.2 don't include anything relevant.

Excellent analysis.

Did you get a chance to test those changes?

@dscho
Copy link
Member

dscho commented Jun 10, 2021

(And yes, I bet that SetConsoleMode(con_out, output_mode | ENABLE_VIRTUAL_TERMINAL_PROCESSING); is the culprit.)

@KalleOlaviNiemitalo
Copy link

Did you get a chance to test those changes?

No, I don't have a computer available for such use.

@dscho
Copy link
Member

dscho commented Jun 10, 2021

Okay. Just leaving this here for posterity: we should probably add a patch to MSYS2-packages/less for now, to guard the new handling behind the presence of the environment variable WT_SESSION (that indicates that we're inside a Windows Terminal).

@bmcdonnell
Copy link

Would #3235 (comment) (my comment above) be caused by this issue, or something else?

@KalleOlaviNiemitalo
Copy link

I thought perhaps less ought to explicitly configure the ENABLE_WRAP_AT_EOL_OUTPUT and DISABLE_NEWLINE_AUTO_RETURN output modes as well, because those auto_wrap and ignaw variables seem to describe how the terminal behaves in those respects. I don't entirely understand how they are used.

KalleOlaviNiemitalo referenced this issue in gwsw/less Jun 10, 2021
from Jason Shirk and Jason Hood.
@KalleOlaviNiemitalo
Copy link

guard the new handling behind the presence of the environment variable WT_SESSION (that indicates that we're inside a Windows Terminal)

@t-b wrote "Problem shows in cmd and in Windows terminal" and the screen shot looks like the "Eingabeaufforderung" window was created by conhost. I don't think checking WT_SESSION makes sense when the problem occurs with both Windows Terminal and conhost.

@t-b
Copy link
Author

t-b commented Jun 10, 2021

@KalleOlaviNiemitalo Yes the issue is present in "Windows terminal" and a plain cmd.

@KalleOlaviNiemitalo
Copy link

The SetConsoleMode calls in less 581 are not executed, and SetConsoleMode is not in the import table.

The issue does not occur if I run LESS=FRX less wide-file directly, but occurs if I run LESS=FRX git show HEAD:wide-file.

If I run !echo $COLUMNS from within git show in Git 2.32.0.windows.1, then it outputs 80, regardless of whether I leave $PAGER unset or point it to less 581 or less 563. If I run the same from within less, then it outputs a blank line.

COLUMNS=80 is however not in the PEB of the less.exe process. I guess MSYS keeps its environment variables somewhere else.

If I export COLUMNS in Git Bash, then less 581 with git version 2.32.0.windows.1 works OK.

It seems the problem is that something injects the incorrect value COLUMNS=80 and less 581 now trusts that while less 563 does not.

@KalleOlaviNiemitalo
Copy link

gwsw/less@bb0ee4e made the COLUMNS variable take precedence.

@KalleOlaviNiemitalo
Copy link

Workaround: git config --global core.pager "env -u COLUMNS less"

Which shows that the incorrect COLUMNS value is received from the parent process, rather than generated by some library that less uses.

@KalleOlaviNiemitalo
Copy link

Git sets COLUMNS in setup_pager:

git/pager.c

Lines 109 to 118 in 4c20499

/*
* After we redirect standard output, we won't be able to use an ioctl
* to get the terminal size. Let's grab it now, and then set $COLUMNS
* to communicate it to any sub-processes.
*/
{
char buf[64];
xsnprintf(buf, sizeof(buf), "%d", term_columns());
setenv("COLUMNS", buf, 0);
}

term_columns tries to read the window size from the COLUMNS environment variable or with TIOCGWINSZ, and defaults to 80 if neither works:

git/pager.c

Lines 160 to 171 in 4c20499

term_columns_at_startup = 80;
col_string = getenv("COLUMNS");
if (col_string && (n_cols = atoi(col_string)) > 0)
term_columns_at_startup = n_cols;
#ifdef TIOCGWINSZ
else {
struct winsize ws;
if (!ioctl(1, TIOCGWINSZ, &ws) && ws.ws_col)
term_columns_at_startup = ws.ws_col;
}
#endif

I have not verified with a debugger that this is what passes the incorrect COLUMNS=80 to less, but it looks very likely,

So, export COLUMNS helps because Bash keeps COLUMNS up to date and this code in Git reads the value and passes it to less.

I wonder why TIOCGWINSZ doesn't work, then.

@rosti-il
Copy link

I think I've found the same or a very similar bug when used Git Bash. This Git was installed with default installation options.

$ git --version --build-options
git version 2.32.0.windows.1
cpu: x86_64
built from commit: 4c204998d0e156d13d81abe1d1963051b1418fc0
sizeof-long: 4
sizeof-size_t: 8
shell-path: /bin/sh
feature: fsmonitor--daemon
$ cmd.exe /c ver

Microsoft Windows [Version 10.0.19043.1052]

How to reproduce:

  1. Clone https://github.com/rosti-il/git-forwin-diff-bug.git
  2. Make sure your mintty window is relatively small, so the next command will use scrolling
  3. Run git diff 1f47 2f53 and scroll down

One of the diffs that you normally should see is the following:

-                    error_popup [mc "Short SHA1 id %s is ambiguous" $id]
+                    error_popup [mc "Short $hashalgorithm id %s is ambiguous" $id]

But for some reason you see it distorted:

-                    error_popup [mc "Short SHA1 id %s is ambiguous" $id]
id]

image

image

As you can see in the attached screenshot the problematic line is splitted into two parts. When it's the last line of the scrolled output the first part is shown and if you continue to scroll down the second part is show instead. A similar bug will be triggered if you run it without colors, i.e. git diff 1f47 2f53 --no-color. A simple workaround is running git diff 1f47 2f53 | less instead.

P.S. this bug was found when I worked on the following PR in the mainstream Git repo:
gitk: Add a basic support of SHA256 repositories into Gitk #979

@KalleOlaviNiemitalo
Copy link

I wonder why TIOCGWINSZ doesn't work, then.

In git.exe version 2.32.0.windows.1 (amd64), setup_pager apparently starts at git+0x19a3f0, and term_columns is inlined:

  • At git+0x19a427, term_columns checks whether term_columns_at_startup (git+0x357000) was already set.

    git/pager.c

    Lines 157 to 158 in 4c20499

    if (term_columns_at_startup)
    return term_columns_at_startup;
  • At git+0x19a550, term_columns assigns term_columns_at_startup and calls getenv.

    git/pager.c

    Lines 160 to 162 in 4c20499

    term_columns_at_startup = 80;
    col_string = getenv("COLUMNS");
  • At git+0x19a56e, term_columns calls atoi, checks the result, and jumps to git+0x19a588 if not positive.

    git/pager.c

    Line 163 in 4c20499

    if (col_string && (n_cols = atoi(col_string)) > 0)
  • At git+0x19a57a, term_columns sets term_columns_at_startup and jumps to git+0x19a437.

    git/pager.c

    Line 164 in 4c20499

    term_columns_at_startup = n_cols;
  • At git+0x19a588, term_columns reads term_columns_at_startup and jumps to git+0x19a437.

    git/pager.c

    Line 173 in 4c20499

    return term_columns_at_startup;

That means the #ifdef TIOCGWINSZ section was not compiled in, i.e. term_columns always returns 80 unless the COLUMNS environment variable is set. That also explains why git show --stat squeezes its output into 80 columns even when the terminal is wider.

I think what this needs is:

  1. Make term_columns get the terminal width from the Windows console API. This change would correct the width in the pager and in git show --stat.

    There seems to be a TIOCGWINSZ implementation in msys2-runtime but I'm not sure whether git.exe is linked with that library.

  2. Change setup_pager to never add COLUMNS to the environment. Rather, the pager should query the console API, like less already does if COLUMNS is not defined. This change would prevent a stale COLUMNS value from being inherited by child processes of the pager if the terminal is resized while the pager is running.

@dscho
Copy link
Member

dscho commented Jun 14, 2021

@KalleOlaviNiemitalo excellent analysis!

1. There seems to be a [TIOCGWINSZ implementation in msys2-runtime](https://github.com/msys2/msys2-runtime/blob/12ee69ae288878ed4dfec45f3d5abf69954cc435/winsup/cygwin/fhandler_console.cc#L1235) but I'm not sure whether git.exe is linked with that library.

git.exe is a MINGW program, not an MSYS2 program, and therefore cannot be linked to the MSYS2 runtime.

I guess we will have to fix the MSYS2 runtime to set the correct COLUMNS, now that less relies on it.

@dscho
Copy link
Member

dscho commented Jun 14, 2021

Hmm. I cannot find any mention of COLUMNS in https://github.com/git-for-windows/msys2-runtime/. So I am not quite sure what we should do here.

A workaround in the meantime might be to call this in your shell:

export COLUMNS=$(tput cols)

@KalleOlaviNiemitalo
Copy link

Bash can set the COLUMNS variable by itself, and update it when the terminal is resized. It does not export COLUMNS to child processes by default but export COLUMNS fixes that, letting git show --stat and the pager know the correct number of columns. So, you don't really need =$(tput cols) there.

However, if I resize the terminal while the pager is running, then Bash does not update COLUMNS until I kill -WINCH $$ or resize the terminal at the Bash prompt. git config --global core.pager "env -u COLUMNS less" prevents less from receiving the stale value in that situation, and I don't think this configuration can cause any problems on Windows. It does not help git show --stat, though. Perhaps one could add kill -WINCH $$ to PROMPT_COMMAND.

According to be11f7a, setup_pager adds COLUMNS to the environment for the sake of Git child processes. It doesn't seem intended for the pager itself, but now unfortunately affects that too.

compat/winansi.c already contains a couple of GetConsoleScreenBufferInfo calls. I imagine term_columns could likewise call that. I'm not sure whether term_columns should use dwSize or srWindow from CONSOLE_SCREEN_BUFFER_INFO.

@dscho
Copy link
Member

dscho commented Jun 14, 2021

I just tested this, and it seems to work around the problem in an acceptable manner:

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

The rationale: if we are on Windows, we might very well not have a working ncurses, in which case we definitely want to stop trying to be clever about setting COLUMNS.

dscho added a commit to dscho/git that referenced this issue Jun 14, 2021
Since gwsw/less@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 git-for-windows#3235

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@dscho dscho linked a pull request Jun 14, 2021 that will close this issue
@dscho
Copy link
Member

dscho commented Jun 14, 2021

@t-b could I bother you to test the build artifacts of https://github.com/git-for-windows/git/actions/runs/936192709 once the run is done?

@t-b
Copy link
Author

t-b commented Jun 14, 2021

@dscho Sure. I can not reproduce the bug with that (git version 2.32.0.windows.1.8.g262eaa2f4f.20210614151600) version.

@rosti-il
Copy link

@t-b could I bother you to test the build artifacts of https://github.com/git-for-windows/git/actions/runs/936192709 once the run is done?

This fixes my use case as well. Thank you.

dscho added a commit to dscho/git that referenced this issue Jun 16, 2021
Since gwsw/less@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 git-for-windows#3235

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
gitster pushed a commit to git/git that referenced this issue Jun 17, 2021
Since gwsw/less@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 git-for-windows#3235

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
@TheWyo
Copy link

TheWyo commented Jun 17, 2021

@t-b could I bother you to test the build artifacts of https://github.com/git-for-windows/git/actions/runs/936192709 once the run is done?

Another +1 for this build fixing issues I've been seeing with diffing. Thanks for the fix!

@ralish
Copy link

ralish commented Jun 18, 2021

Chiming in to note that this issue extends beyond just git diff. I noticed earlier today that running git branch -vv in a certain repository would result in no output. Downgrading to Git v2.31.1 fixed the issue, and I eventually tracked it down to the version of less bundled with Git v2.32.

It turns out that the output of git branch -vv in this repository was 80 characters, and under less v581 that results in no output. This occurs under Command Prompt and PowerShell but not Git Bash, irrespective of whether any of the latter are running under Windows Terminal. Running git --no-pager branch -vv returns the expected output.

Further experimenting with the less source shows that the issue is seemingly fixed from v584 onwards (inc. using the latest v590 sources). This commit appears to be the fix: gwsw/less@0e822fc

@dscho
Copy link
Member

dscho commented Jun 18, 2021

Further experimenting with the less source shows that the issue is seemingly fixed from v584 onwards (inc. using the latest v590 sources). This commit appears to be the fix: gwsw/less@0e822fc

Excellent news! Unfortunately, according to less' home page, v590 is only released for beta testing.

There are two options I see going forward:

  1. Just wait.
  2. Backport gwsw/less@0e822fc into https://github.com/git-for-windows/MSYS2-packages/tree/HEAD/less

The second option is definitely more work, especially going forward: for now, we inherit the less package from MSYS2, but if we start building it ourselves, we will have to ship it forever (and take care of updating it). Of course, we could also instead patch it in MSYS2 and then be careful to watch the updates.

In any case, we would then have to monitor less. We might do that via monitoring https://github.com/gwsw/less/tags.atom, but that would not make it clear whether a version was released to the public or just to beta.

An alternative would be to have a separate repository to monitor the updates, where the website https://www.greenwoodsoftware.com/less/index.html could be scraped (and be stored). I do something like this in a private repository. Maybe it is time to bring this out of the shadows.

@ralish
Copy link

ralish commented Jun 18, 2021

@dscho Is there some reason we can't just downgrade to the previous stable release (v563) with the intention of updating to whatever becomes the stable release following the v590 beta? That seems like less work than backporting patches. I feel doing nothing is not a great option given the issues the shipped less release introduces.

@dscho
Copy link
Member

dscho commented Jun 18, 2021

An alternative would be to have a separate repository to monitor the updates, where the website https://www.greenwoodsoftware.com/less/index.html could be scraped (and be stored). I do something like this in a private repository. Maybe it is time to bring this out of the shadows.

This is now done here: https://github.com/git-for-windows/track-website-changes

@dscho
Copy link
Member

dscho commented Jun 18, 2021

@dscho Is there some reason we can't just downgrade to the previous stable release (v563) with the intention of updating to whatever becomes the stable release following the v590 beta?

Due to the way Git for Windows' SDK is set up, this would be non-trivial.

There is precendence, of course, see e.g. git-for-windows/git-sdk-64@2121197, git-for-windows/git-sdk-64@8f99f4d, and git-for-windows/git-sdk-64@a60459a.

But it has to be done via cloning git-sdk-64 and git-sdk-32 locally, downgrading via downloading the previous package version (pacman does not offer the functionality to install a specific version from a remote repository: pacman -S 'less<=563' results in error: target not found: less<=563), pacman -U <package>, then committing it with a helpful commit message, pushing it, and then (the most important part) not to forget to revert all of that once a good version comes out.

@ralish
Copy link

ralish commented Jun 18, 2021

Yuck. Well, there's at least a workaround for now in the form of using the less binaries from the Git v2.31.1 release, or alternatively, building a suitably bleeding edge version of less which has the referenced commit. Given a fixed release is in beta, hopefully a stable release isn't too far away, and it can be incorporated in a subsequent Git minor release.

@dscho
Copy link
Member

dscho commented Jun 18, 2021

and it can be incorporated in a subsequent Git minor release.

Even better: the snapshot following the upgrade will already contain the fix. Snapshots are just like full releases, modulo the funny-looking version (but everything else is just the same, down to the very same Azure Pipeline building snapshots and full releases).

dscho added a commit to dscho/git that referenced this issue Jun 21, 2021
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 gwsw/less@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 git-for-windows#3235

Co-authored-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
gitster added a commit to git/git that referenced this issue Jun 29, 2021
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 gwsw/less@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 git-for-windows#3235

Co-authored-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
@dscho dscho added this to the Next release milestone Jul 5, 2021
dscho added a commit to git-for-windows/build-extra that referenced this issue Jul 5, 2021
When scrolling in the pager (e.g. in the
output of `git log`), [lines were duplicated by
mistake](git-for-windows/git#3235). This
was fixed.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@Geoffrey-A
Copy link

Thank you very much to all involved. That was quite an annoying bug.

dscho added a commit that referenced this issue Aug 11, 2021
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 gwsw/less@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 #3235

Co-authored-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho added a commit that referenced this issue Aug 13, 2021
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 gwsw/less@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 #3235

Co-authored-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho added a commit that referenced this issue Aug 18, 2021
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 gwsw/less@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 #3235

Co-authored-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho added a commit that referenced this issue Aug 23, 2021
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 gwsw/less@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 #3235

Co-authored-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
git-for-windows-ci pushed a commit that referenced this issue Aug 24, 2021
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 gwsw/less@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 #3235

Co-authored-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dennisameling pushed a commit to dennisameling/git that referenced this issue Sep 15, 2021
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 gwsw/less@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 git-for-windows#3235

Co-authored-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants