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

git version 2.43.0.windows.1 introduced missaligned output of git commands #4764

Open
tophoo opened this issue Jan 14, 2024 · 24 comments
Open

Comments

@tophoo
Copy link

tophoo commented Jan 14, 2024

Setup

  • Which version of Git for Windows are you using? Is it 32-bit or 64-bit?
git version 2.43.0.windows.1
cpu: x86_64
built from commit: 4b968f3ea3b32a7bc50846bab49f3f381841d297
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?
Microsoft Windows [Version 10.0.22621.3007]

It also occurs on Windows 10, there seems to be no dependecy on the windows version.

  • What options did you set as part of the installation? Or did you choose the
    defaults?

The error did not occur when using the builtin git-update-for-windows and coming from an older version. The error only occurs after deinstalling the old git version and installing the new git version. I could reproduce the issue on a multiple machines.

Editor Option: VIM
Custom Editor Path:
Default Branch Option:
Path Option: Cmd
SSH Option: OpenSSH
Tortoise Option: false
CURL Option: OpenSSL
CRLF Option: CRLFAlways
Bash Terminal Option: MinTTY
Git Pull Behavior Option: Merge
Use Credential Manager: Enabled
Performance Tweaks FSCache: Enabled
Enable Symlinks: Enabled
Enable Pseudo Console Support: Disabled
Enable FSMonitor: Disabled
  • Any other interesting things about your environment that might be related
    to the issue you're seeing?

The error is not present, if the bash uses the vt100 terminal type instead of the default xterm. Though using vt100 is not really a solution to the problem.

Details

  • Which terminal/shell are you running Git from? e.g Bash/CMD/PowerShell/other

"C:\Program Files\Git\git-bash.exe"

The easiest way to reproduce it is:

git push -u origin HEAD
git push -u origin HEAD

After the bug has been triggered almost all output is broken, e.g. git pull, git status, etc.

  • What did you expect to occur after running these commands?
$ git push -u origin HEAD
Everything up-to-date
branch 'tmp' set up to track 'origin/tmp'.

$ git push -u origin HEAD
Everything up-to-date
branch 'tmp' set up to track 'origin/tmp'.
  • What actually happened instead?
$ git push -u origin HEAD
Everything up-to-date
branch 'tmp' set up to track 'origin/tmp'.

$ git push -u origin HEAD
Everything up-to-date
                     branch 'tmp' set up to track 'origin/tmp'.
@rimrul
Copy link
Member

rimrul commented Jan 14, 2024

Dupicate of #2739 and #3774

@tophoo
Copy link
Author

tophoo commented Jan 14, 2024

Yes, this issue looks like a duplicate of those tickets, but with 2.42 everything worked fine and I could now reproduce it on multiple machines.

@rimrul
Copy link
Member

rimrul commented Jan 14, 2024

With 2.42 and 2.41 that could be a result of #4571

@tophoo
Copy link
Author

tophoo commented Jan 14, 2024

I just re-installed git and checked using pseudo consoles and this also mitigates the problem.

@dscho
Copy link
Member

dscho commented Jan 16, 2024

Hmm, I don't seem to be able to reproduce:

me@work MINGW64 ~/64-bit-build-extra (main)
$ git push -u origin HEAD
Everything up-to-date
branch 'main' set up to track 'origin/main'.

me@work MINGW64 ~/64-bit-build-extra (main)
$ git version
git version 2.43.0.windows.1

me@work MINGW64 ~/64-bit-build-extra (main)
$ echo $MSYS
disable_pcon

@tophoo
Copy link
Author

tophoo commented Jan 19, 2024

Have you done a clean reinstall? I now use the option for pseudo consoles and as I said it mitigates the problem, but sometimes I still have the problem when doing git pull, but I have not been able (and also I did not try) to reproduce it in a minimal example. As it only affects git pull it is not a problem.

@dscho
Copy link
Member

dscho commented Jan 19, 2024

Have you done a clean reinstall?

No, but I know how the install works (since I implemented it), so I am actually rather fairly confident that I am running without ConPTY support.

@tophoo
Copy link
Author

tophoo commented Jan 19, 2024

Is there anything I can do to assist in finding the problem?

@dscho
Copy link
Member

dscho commented Jan 24, 2024

Since I cannot reproduce, my only suggestion is to try to pin-point the snapshot where the unwanted behavior started.

@tophoo
Copy link
Author

tophoo commented Feb 11, 2024

I finally had some time to test the snapshot versions and I could identify version

Mon, 28 Aug 2023 14:22:21 +0200
(commit 2f819d1670fff9a1818f63b6722e9959405378e3)

as the one that introduced the bug. With version

Mon, 21 Aug 2023 20:11:24 +0200
(commit a2e49ec355ae22e74288fa74f50821f3cc1a1551)

everything worked fine. I also tested the latest version

Fri, 9 Feb 2024 21:50:07 +0100
(commit 5cff971ba4c5c91f881d277bfeb6a88fa8c17745)

and it still had the same bug.
For the test I uninstalled git, removed the remaining folder C:\Program Files\Git and did a clean install of the 64bit version with just the default options.

@rimrul
Copy link
Member

rimrul commented Feb 11, 2024

That's probably the first snapshot with the fix for #4571, not the snapshot introducing the problem. That snapshot should be before 16 May 2023 16:44:00 +0200

@rimrul
Copy link
Member

rimrul commented Feb 11, 2024

So

Mon, 15 May 2023 12:50:37 +0200
(commit 69ed45537b45586c8747fa1083bd2373787b51d9)

probably shows the same issue.

@rimrul
Copy link
Member

rimrul commented Feb 11, 2024

Looking at the date of #2739 the actual first snapshot is probably in early June 2020 or earlier.

@dscho
Copy link
Member

dscho commented Feb 11, 2024

@tophoo excellent work!

Let's see:

$ git log --boundary --oneline --graph a2e49ec355ae22e74288fa74f50821f3cc1a1551...2f819d1670fff9a1818f63b6722e9959405378e3
* 2f819d1670ff (tag: v2.42.0.windows.2) fixup! http: support lazy-loading libcurl also on Windows
* b29582706bca fixup! http: support lazy-loading libcurl also on Windows
o a2e49ec355ae (tag: v2.42.0.windows.1) Merge 'readme' into HEAD

Nah, that cannot be the problem: that only touches the code involved with talking via HTTPS using libcurl. So let's see what packages changed? Luckily, the two snapshots you found correspond to official releases and we can compare https://github.com/git-for-windows/build-extra/blob/main/versions/package-versions-2.42.0.txt to https://github.com/git-for-windows/build-extra/blob/main/versions/package-versions-2.42.0.2.txt directly (otherwise we'd have needed to download the PortableGit snapshots and compare their respective etc\package-versions.txt):

diff --git a/versions/package-versions-2.42.0.txt b/versions/package-versions-2.42.0.2.txt
index 25e35a5ab..dc4bf5cd7 100644
--- a/versions/package-versions-2.42.0.txt
+++ b/versions/package-versions-2.42.0.2.txt
@@ -18,7 +18,7 @@ gmp 6.3.0-1
 gmp 6.3.0-1
 gnupg 2.2.41-1
 grep 1~3.0-6
-gzip 1.12-2
+gzip 1.13-1
 heimdal-libs 7.8.0-4
 less 643-1
 libasprintf 0.22-1
@@ -76,11 +76,11 @@ mingw-w64-x86_64-curl-winssl 8.2.1-1
 mingw-w64-x86_64-expat 2.5.0-1
 mingw-w64-x86_64-gcc-libs 13.2.0-2
 mingw-w64-x86_64-gettext 0.21.1-2
-mingw-w64-x86_64-git 2.42.0.1.a2e49ec355-1
+mingw-w64-x86_64-git 2.42.0.2.2f819d1670-1
 mingw-w64-x86_64-git-credential-manager 2.3.2-1
-mingw-w64-x86_64-git-doc-html 2.42.0.1.a2e49ec355-1
+mingw-w64-x86_64-git-doc-html 2.42.0.2.2f819d1670-1
 mingw-w64-x86_64-git-extra 1.1.636.2db97b993-1
-mingw-w64-x86_64-git-lfs 3.3.0-3
+mingw-w64-x86_64-git-lfs 3.4.0-1
 mingw-w64-x86_64-gmp 6.3.0-1
 mingw-w64-x86_64-gnutls 3.8.0-1
 mingw-w64-x86_64-libffi 3.4.4-1
@@ -111,8 +111,8 @@ mingw-w64-x86_64-xz 5.4.4-1
 mingw-w64-x86_64-zlib 1.3-1
 mingw-w64-x86_64-zstd 1.5.5-1
 mintty 1~3.6.4-1
-mpfr 4.2.0.p12-1
-msys2-runtime 3.4.7-2
+mpfr 4.2.1-1
+msys2-runtime 3.4.7-3
 nano 7.2-1
 ncurses 6.4-1
 nettle 3.9.1-1

Ah, that msys2-runtime version bump could be it. So let's see what changed there (found by looking through https://github.com/git-for-windows/MSYS2-packages/commits/HEAD/msys2-runtime/PKGBUILD): git-for-windows/MSYS2-packages@bfade9b#diff-18f8821497e7250c23a1cdeea1dcb80911a8d3589da6bc196b129b76f6dc61e7.

Hmm. But that does not make any sense. What that did was to restore some fast behavior that side-stepped contacting a (potentially unreachable) Active Directory domain controller. Nothing to do with output.

And I don't see anything else in the release notes of v2.42.0(2)...

So maybe it does have something to do with those Git commits after all? The reported output does have something to do with HTTPS transport, after all. But no, https://github.com/git-for-windows/git/compare/a2e49ec355ae22e74288fa74f50821f3cc1a1551..2f819d1670fff9a1818f63b6722e9959405378e3 shows only a change relevant to output in the code that would show an error if the libcurl library is not found (which is not the case here).

@tophoo could you try more things with the Aug 21 snapshot? You do not need to use the installer, just extract a Portable Git somewhere, and call git-bash.exe in its top-level directory. Verify that it shows the correct behavior. Then extract PortableGit-2.42.0.2 (i.e. the Aug 28 snapshot) and copy its mingw64\bin\git.exe over the one in the extracted PortableGit-2.42.0, then see whether the output regresses again? To make sure that the test is thorough, please close the Git Bash before replacing the file. Do the same with usr\bin\msys-2.0.dll. Hopefully you can identify one of those files as the culprit, because that would make it easier to dig further.

@dscho
Copy link
Member

dscho commented Feb 11, 2024

@rimrul I guess it is time to bite the bullet and just turn on ConPTY support by default (and remove the experimental option in favor of a new checkbox on the -- admittedly already long -- components page)? Do you have an opinion on that?

@rimrul
Copy link
Member

rimrul commented Feb 11, 2024

@rimrul I guess it is time to bite the bullet and just turn on ConPTY support by default (and remove the experimental option in favor of a new checkbox on the -- admittedly already long -- components page)? Do you have an opinion on that?

It might make sense to flip the default, but I have no objective criteria to decide if it's time for that. When we do flip the default we should probably stop calling it experimental. It could make sense as a component, but keeping it as an option would work, too. Did we ever define any criteria for what's an option on a custom wizard page and what's a component? Is it just "if it's experimental or doesn't quite work as a checkbox it's an option"?

@dscho
Copy link
Member

dscho commented Feb 11, 2024

When we do flip the default we should probably stop calling it experimental.

@rimrul right.

TBH I would still call it buggy. Unfortunately, its bugginess has crept into the disable_pcon part of the code so much as to make enable_pcon the "less buggy" mode.

Did we ever define any criteria for what's an option on a custom wizard page and what's a component?

Not really. My kind of unwritten rule was that Components are things that need less explanation and/or are less likely to be of interest to users. That's my kind of thinking here, too: if we enable ConPTY support by default, we do it because the alternative would mean more buggy behavior for users and therefore they should be less interested in overriding the default defined by Git for Windows.

@tophoo
Copy link
Author

tophoo commented Feb 11, 2024

The first new data point: I cannot reproduce the bug with the portable version, neither the version from 28.08.2023 nor the latest version from 09.02.2024 show the bug.
Then I reinstalled the 64bit installer version from 21.08.2023. I replaced mingw64/bin/git.exe with the portable version from 28.08.203 and nothing changed. Then I copied usr/bin/msys-2.0.dll and again nothing changed.

@tophoo
Copy link
Author

tophoo commented Feb 11, 2024

Now I have installed 28.08.2023 and then replaced:

  • mingw64 with the portable mingw64 folder from 21.08.2023 -> bug is still present
  • usr with the portable usr folder from 21.08.2023 -> bug is still present
  • etc with the portable etc folder from 21.08.2023 -> the bug disappeared 🤔

So I diffed the portable versions and they only differ in

  • etc/gitconfig
  • etc/package-versions.txt

@tophoo
Copy link
Author

tophoo commented Feb 11, 2024

If I diff the installed 64bit folder of etc with the portable version from 28.08.2023, there is a diff in etc/:
etc/git-bash.config
with the content:

MSYS=disable_pcon

The file does not exist in the portable version.

@rimrul
Copy link
Member

rimrul commented Feb 11, 2024

If I diff the installed 64bit folder of etc with the portable version from 28.08.2023, there is a diff in etc/: etc/git-bash.config with the content:

MSYS=disable_pcon

The file does not exist in the portable version.

So we're back to "this works in 2.41.0/2.42.0/snapshots between 2023-05-16 and 2023-08-21 due to #4571".

@dscho
Copy link
Member

dscho commented Feb 11, 2024

If I diff the installed 64bit folder of etc with the portable version from 28.08.2023, there is a diff in etc/: etc/git-bash.config with the content:

MSYS=disable_pcon

The file does not exist in the portable version.

Can you try to replace the contents with MSYS=enable_pcon in the Portable Git and see whether that changes the behavior? That would indeed prove @rimrul's observation that the ConPTY support is the root cause here.

@tophoo
Copy link
Author

tophoo commented Feb 11, 2024

Yes, @rimrul is right: If I add add a etc/git-bash.config with the content:

MSYS=disable_pcon

I get the same bug also with the version from 21.08.2023. So I just need to bisect the snapshot versions between 2023-05-16 and 2023-08-21 and add this patch manually?

@dscho
Copy link
Member

dscho commented Feb 11, 2024

I just need to bisect the snapshot versions between 2023-05-16 and 2023-08-21 and add this patch manually?

No, @rimrul already correctly identified the PR that changed the behavior: #4571.

The more I think about it, the more certain I am that we need to flip the default to enable_pcon. It's just too bad because that means that Windows 7/8 support is already broken, by that buggy disable_pcon mode.

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

No branches or pull requests

3 participants