Git Push Not working for git:// protocol servers

[johnnyb]

• I was not able to find an open
  or closed issue
  matching what I'm seeing

There appears to be some bug when pushing to git:// protocol for repositories. I believe this is a carryover from msys-git, but no one has logged a bug in the new system. This URL ( http://pete.akeo.ie/2011/07/git-remote-repository.html ) refers to the bug, and said it was bug 457 in the old system. However, the page that it references is now gone now that you all have moved to github.

I searched this issue tracker for the issue but was unable to find it. Anyway, the report is below. I am working around the issue by pushing to a Mac laptop that can then push to the server.

Setup

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

$ git --version --build-options
git version 2.10.1.windows.1
sizeof-long: 4
machine: x86_64

• Which version of Windows are you running? Vista, 7, 8, 10? Is it 32-bit or 64-bit?
  Windows 10 64-bit

$ cmd.exe /c ver
Microsoft Windows [Version 10.0.14393]

• What options did you set as part of the installation? Or did you choose the
  defaults?
  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
> Path Option: CmdTools
> SSH Option: OpenSSH
> CRLF Option: CRLFAlways
> Bash Terminal Option: ConHost
> Performance Tweaks FSCache: Enabled
> 
> 

• Any other interesting things about your environment that might be related
  to the issue you're seeing?

  None - just did a fresh install to be sure

Details

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

  CMD I believe, but I'm used to UNIX-y environments so I'm a bit out of sorts.

• What commands did you run to trigger this issue? If you can provide a
  Minimal, Complete, and Verifiable example
  this will help us understand the issue.

git push --set-upstream origin master
This occurs when origin is set to the "git://" protocol.

• What did you expect to occur after running these commands?

  I expected it to push

• What actually happened instead?

  It gets stuck "Writing Objects". Depending on the particular push, it may get a different percentage of the way done.

• If the problem was occurring with a specific repository, can you provide the
  URL to that repository to help us with testing?

  It is an internal repository of a client of mine, so no.

[johnnyb]

I was searching google and I found that this bug is shown in Bug 89 of msysgit on github:

msysgit/msysgit#89 (comment)

The workaround, which is shown in the code but not in the thread itself, is to do:

git config --global sendpack.sideband false

Perhaps this would be a good global setting for the installer to set? Since this is essentially undocumented (or at least unpublicized), having a default-working configuration seems to be the best plan of action, unless it breaks something.

[dscho]

@johnnyb is this still the case with Git for Windows v2.21.0 that you cannot push via git:// unless you set sendpack.sideband?

I ask because we're about to release v2.22.0, and we dropped support for that config setting under the assumption that it is no longer necessary.

[johnnyb]

I will try to test it out this week and double-check.

[dscho]

v2.22.0 is already out. With the config setting in question removed.

Hopefully your tests come back saying that it's no longer needed. Because otherwise, we're a little late.

[RoboMWM]

This option is still required for pushing over git://

After trying to figure out why this option was working intermittently I discovered we had two different versions of git bash installed, with the option (and thus no hanging) only working in the older version.

[RoboMWM]

Mingw64 is what shows up in the prompt, and the Git directory is in the 64 bit program files directory, in case that matters (as I noticed some commits related to this).

Not sure where the discussion occurred that assumed this wasn't needed - my guess is that the same steps used to reproduce the bug would show it still exists.

[dscho]

Mingw64 is what shows up in the prompt, and the Git directory is in the 64 bit program files directory, in case that matters (as I noticed some commits related to this).

I do not understand. Could you explain this in more detail?

Not sure where the discussion occurred that assumed this wasn't needed - my guess is that the same steps used to reproduce the bug would show it still exists.

I think I relied on the test suite to catch this, but having had a quick look, we skip the git:// related tests on Windows because the way they are implemented require FIFOs which we cannot use (there is a FIFO emulation in MSYS2 that allows mkfifo to succeed, but git.exe cannot use them, being unaware of any MSYS2-emulated POSIX stuff).

Maybe you could re-introduce 1da059c via a PR and add a test case that does not require FIFOs?

[RoboMWM]

Re Mingw64, I saw a commit message about the issue not showing up in mingw-w64 gitgitgadget@4f9dd21

I'm not a C developer nor am I familiar with the internals of this project so I'm not sure where I'd start for making a PR :/

[dscho]

Re Mingw64, I saw a commit message about the issue not showing up in mingw-w64 gitgitgadget@4f9dd21

If you experience this issue with a 64-bit version of Git for Windows, then mingw-w64 is affected.

I'm not a C developer nor am I familiar with the internals of this project so I'm not sure where I'd start for making a PR

You will have to

• install Git for Windows' SDK
• sdk cd git
• cherry-pick --signoff the commit you already found
• make -j8 install to build it and install it into your SDK
• verify that it fixes your issue (by running commands in the Git SDK Bash)
• preferably provide as PR description a detailed account how to reproduce the issue with Git for Windows, starting from an empty repository.
• fork this repository if you have not done so yet,
• push the branch with the fix
• open the PR here.

It is not exactly as easy as buying an apple pie. On the positive side, it will get you what you want, as quickly as you can make it so.

[assarbad]

I will try to take care of this throughout this week. Let's see how quickly I can send a PR. Issue affects me as well, so reason enough to contribute.

@dscho how do you ensure that this will stick around? Will it also be sent to upstream Git or will it be local to Git for Windows?

Edit: regarding PRs, do you have any guidelines? Signed commits or so? Rebased and squashed to a single commit? Do I also have to test with the 32-bit SDK or can I rely on your CI setup?

[PhilipOakley]

re your edit:

regarding PRs, do you have any guidelines? Signed commits or so? Rebased and squashed to a single commit? Do I also have to test with the 32-bit SDK or can I rely on your CI setup?

Signed commits, small simple steps, usually start at current master, and keep the base constant, though watching for conflicts on next/pu.

See https://github.com/git-for-windows/git/blob/master/CONTRIBUTING.md and SubmittingPatches.

The patches do stick around and eventually get up-streamed, especially when battle tested.

[dscho]

how do you ensure that this will stick around?

Via that regression test you planned on implementing ;-)

Will it also be sent to upstream Git or will it be local to Git for Windows?

It was already sent to the Git mailing list: gitgitgadget#137 (due to the missing regression test, I was under the impression that it was no longer necessary in Git for Windows).

So as soon as you come up with that resurrection accompanied by a regression test, I will be working towards getting this into upstream Git, too.

As to PRs, there were a couple good ones recently that were merged. Maybe they illustrate the shape we like PRs to be in?

[assarbad]

Hehe, alright. Sounds like a plan.

One more question: are there any resources about the research so far into what goes wrong on Windows? The symptom appears to be pretty much a deadlock situation. As I understand it has to do with two "concurrently" issued requests on a single socket? Any references/pointers I can look into to get me started? I'd rather try to tackle (mid term) the underlying issue.

This sounds like a tricky problem that could use some loving care ;-)

Also, I'll have a look at previous PRs as well. Already read through large parts of the documentation linked before, but since there are plenty of cross-references elsewhere, it's a bit tedious.

[dscho]

are there any resources about the research so far into what goes wrong on Windows?

I don't think so. The closest is #304, but I don't think that this is actually addressing the culprit.

This sounds like a tricky problem that could use some loving care ;-)

I'm not so sure about that. It affects only the git push over git://. Which is not only unauthenticated, but also unsecured. To be quite honest, I would like to highly discourage working via git:// altogether. I'd much rather see an interested contributor working on a Git command that starts up an ad-hoc HTTPS server as an alternative to git daemon. In my mind, that would make a lot more sense than trying to fix pushing via git://, which should be deprecated and disallowed, as far as I am concerned. And yes, if you must, you can offer an HTTP (i.e. non-HTTPS) mode in that alternative. If you must.

😄

[dscho]

For the record, my advice still stands.

I will mark this as "up for grabs" to indicate that nobody is working on this, and users who want to see this problem resolved should feel highly encouraged to put in the work to resolve it.

[assarbad]

Hmm, I was just looking at this again and wanted to follow up on my original intent, but it seems this commit got introduced in the latest RC already (1da059c), @dscho? Does it mean nothing to do or will you still need some kind of test case?

Edit: my bad. I read the Github output wrong. The actual repo shows it's not merged.

[dscho]

it seems this commit got introduced in the latest RC already (1da059c

The problem here is that I use "merging-rebases", i.e. whenever I rebase Git for Windows' patches, I start by a fake merge (-s ours) of the previous patches.

So yes, the commit would be reachable, but by virtue of the latest merging-rebase it would be essentially backed out, anyway.

[assarbad]

Hey project members, just some observations I made while trying to get started now.

It should be stated more clearly on which branch one is supposed to base contributions. The CONTRIBUTING.md isn't explicit in that regard. One can only assume that master is the correct branch based on the description of what vs/master is and how to interact with it.

I am taking notes for myself right now and will see where they fit into the official project Wiki once I have delivered something.

---

Anyway, regarding the actual topic at hand (blocking inside git pack-objects), I think you would be well advised to talk to some of the folks at MS who are much better equipped to provide an answer than I am. As I understand you work (or worked?) for MS to drive this project forward? Anyway, looking at detect_msys_tty I can take a guess of what's going on here. The namespaces for both mailslots and (named) pipes are prone to blocking as I have learned many years ago. The only safe method I ever found to work with these was in a separate thread as I frequently ran into issues where operations such as the NtQueryObject call in detect_msys_tty blocked. For ntobjx I didn't go down that route yet, but a predecessor tool had also support for listing mailslots and pipes. If you rely on NtQueryObject for such a use-case, I think that's questionable (but then, so is the overall enterprise of pretending Windows is unixy enough for all of this ;)). Although by far not as knowledgeable as one Mark Russinovich of Sysinternals/Microsoft fame, I think that I know a thing or two about the NT native API.

For the record the two stack traces for the "client" and "server" processes respectively in the case of blocking:

Parent git process (git push):

0x0000000000000000
ntdll.dll!NtWaitForSingleObject+0x14
KERNELBASE.dll!WaitForSingleObjectEx+0x93
git.exe!waitpid+0x57
git.exe!finish_command+0x59
git.exe!send_pack+0xda5
git.exe!git_transport_push+0x139
git.exe!transport_push+0x53a
git.exe!push_with_options+0xe1
git.exe!cmd_push+0xaea
git.exe!handle_builtin+0x248
git.exe!cmd_main+0x1ab
git.exe!main+0x78
git.exe!wmain+0x33a
git.exe!__tmainCRTStartup+0x249
git.exe!mainCRTStartup+0x1b
KERNEL32.DLL!BaseThreadInitThunk+0x14
ntdll.dll!RtlUserThreadStart+0x21

Child git process (git pack-objects --all-progress-implied --revs --stdout --thin --delta-base-offset --progress):

0x0000000000000000
ntdll.dll!NtQueryObject+0x14
git.exe!detect_msys_tty+0x82
git.exe!winansi_init+0x1d1
git.exe!wmain+0x31f
git.exe!__tmainCRTStartup+0x249
git.exe!mainCRTStartup+0x1b
KERNEL32.DLL!BaseThreadInitThunk+0x14
ntdll.dll!RtlUserThreadStart+0x21

I remember having had this problem with pipes myself in the past and I'll try to consult with a friend who also had the very same problem in conjunction with some proprietary software in times of Windows XP. Several publicly available tools, e.g. from Microsoft but also others, dodge this bullet by employing a driver of their own. Not really practical for Git, I'll admit.

Thinking of XP, what's the lowest version of Windows on which Git for Windows is supposed to run? I'm asking because the GetFileInformationByHandleEx is supposed to do exactly what the NtQueryObject function does in this particular case. The older GetHandleInformation is related, but doesn't quite cover the use-case. Either way you'll get fewer "🙄" from Windows devs than when using the NT native API function (i.e. NtQueryObject).

All that said, the first move I want to make, though, is to fix the symptom (again), which the sideband configuration option allowed until recently.

[dscho]

It should be stated more clearly on which branch one is supposed to base contributions. The CONTRIBUTING.md isn't explicit in that regard. One can only assume that master is the correct branch based on the description of what vs/master is and how to interact with it.

That's a good observation. Indeed, our documentation can be improved.

The contributions should generally come in via PRs relative to master, not based on vs/master.

Having said that, I have fixed PRs that targeted inconvenient/incorrect branches in the past, as long as the contributors checked the "Allow maintainers to edit the PR" checkbox (or whatever the exact title of that checkbox is).

Thinking of XP, what's the lowest version of Windows on which Git for Windows is supposed to run?

This is covered by our FAQ (which is linked from https://gitforwindows.org as well as in each release notes that are shown by default when you install/upgrade Git for Windows):

Git for Windows version 2.10.0 was the last version supporting Windows XP and Server 2003.

While it does not state it specifically (please feel free to improve the FAQ in that respect), https://gitforwindows.org/requirements.html (which the installer will link to when it refuses to install Git for Windows on XP or earlier Windows versions) does say that Windows Vista or later is required.

I'm asking because the GetFileInformationByHandleEx is supposed to do exactly what the NtQueryObject function does in this particular case. The older GetHandleInformation is related, but doesn't quite cover the use-case. Either way you'll get fewer "🙄" from Windows devs than when using the NT native API function (i.e. NtQueryObject).

That sounds sensible, and if the GetFileInformationByHandleEx() function is not offered by the earliest Windows version we support (currently, Vista), then we can always use the DECLARE_PROC_ADDR()/INIT_PROC_ADDR() trick defined in compat/win32/lazyload.h to load functions dynamically, falling back to the ugly code using NT internals.

[dscho]

Let's just close this as stale.

[pkasting]

The release notes (https://github.com/git-for-windows/build-extra/blob/main/ReleaseNotes.md) still refer to this issue with an "until this is addressed..." sort of comment. Should they be updated?

[dscho]

The release notes (https://github.com/git-for-windows/build-extra/blob/main/ReleaseNotes.md) still refer to this issue with an "until this is addressed..." sort of comment. Should they be updated?

Good idea. Would you be so kind to open a PR?

[pkasting]

I can modify the release notes, but it seems kinda poor to say "X may hang, set your config this way". Seems like either the default config should be set that way to begin with, or we should explain what the tradeoff is that leads to that not being true, or this bug should still be open and available (and clear as to what the next steps are) and the notes should basically say "until someone looks into X".

[dscho]

I agree that it is not the most satisfying thing to say "X may hange, set your config this way" instead of fixing the underlying bug, but given the way the conversation with upstream Git went, I cannot fault anybody who no longer wants to work on this.

[pkasting]

Not implying you (or anyone else) is obligated to work on this. Just trying to figure out what the actual status is and how to word things. AFAICT it is:

• Root cause is early EOF due to mismatch between Windows and POSIX semantics
• Proposed patch on https://public-inbox.org/git/20180412210757.7792-1-kgybels@infogroep.be/ fixes the issue for Windows but introduces problems on Linux
• Agreement-in-principle on the right route forward on Linux; either the code would need to maintain the proposed impl for Windows under a conditional, or else implement a compatible ppoll for Windows
• No one is actively working on implementing the above; can work around with a setting change
• Reason for current setting default is _______, and the downside of that setting change is ______, which is why it's not the default

Is this accurate? Sorry for any misunderstandings, I was skimming, and I didn't bother to research the actual setting here.

If it is accurate, perhaps the right thing is to leave this particular bug open and unassigned, with a clear title and comment explaining the necessary next steps. The other possibility is that the relevant setting here gets toggled to the other state, but I assume that's undesirable.

[dscho]

Is this accurate?

As far as I can tell: yes!

• Reason for current setting default is _______, and the downside of that setting change is ______, which is why it's not the default

The reason for the current default to leave the side-band on is: For any non-git:// transfer, it works on Windows, and for git:// it works anywhere but Windows, and it adds the substantial benefit that remote error messages are transmitted and shown in a timely manner.

Turning off side-band works around the bug, at the hefty price that remote error messages are either shown late or not at all.

If it is accurate, perhaps the right thing is to leave this particular bug open and unassigned, with a clear title and comment explaining the necessary next steps.

I made it a practice to close tickets that nobody is working on. This was pretty much the only healthy option when there was an overwhelming number of open tickets and the wide-spread expectation that "someone" would work on them eventually, yet nobody ever did. This practice works well enough for me, better than the alternative at any rate.

If you want to work on this, it is easy enough to open a PR (potentially as draft, or with the title clearly marked as "[DO-NOT-MERGE-YET]") with the latest iteration of the patches that were sent to the Git mailing list. I am comfortable with long-running PRs that are kept open to work on the topic as time allows, in contrast to leaving tickets open that are forgotten except by people like me who try to look at the open tickets from time to time to see which ones have been addressed already and can be closed and are then overwhelmed.

[pkasting]

OK, fair. I do not want to work on it, but now I know enough that I think I can clarify the release notes.