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

Allow to use --wait with Code.exe #60192

Closed
rebornix opened this issue Oct 8, 2018 · 15 comments
Closed

Allow to use --wait with Code.exe #60192

rebornix opened this issue Oct 8, 2018 · 15 comments
Assignees
Labels
feature-request Request for new features or functionality verification-needed Verification of issue is requested verified Verification succeeded workbench-cli VS Code Command line issues
Milestone

Comments

@rebornix
Copy link
Member

rebornix commented Oct 8, 2018

  • VSCode Version: 1.29 Insiders
  • OS Version: Windows 10

Steps to Reproduce:

Good path:

  1. Set VSCode Insiders as the editor for Git SCM
  2. Try commands which involve commit message update, for example, git rebase or git cherry-pick which contains conflicts.
  3. Run the command in an external terminal, after resolving the conflicts, close all VSCode windows.
  4. Run the command with --continue flag.
  5. A new window is open and an editor is created for you to update the commit message

image

  1. Close the window, the command is finished properly

bad one:

  1. Run the command in the integrated terminal, so we are already in a VSCode window
  2. The commit message update editor is opened in the same window
  3. Close the editor, the command is still waiting for the file to be released.

image

Does this issue occur when all extensions are disabled?: Yes/No

@bpasero
Copy link
Member

bpasero commented Oct 9, 2018

@rebornix I cannot reproduce:

before

Can you setup the git handler to execute code out of sources to debug further?

@Tyriar any ideas?

@bpasero bpasero added the info-needed Issue requires more information from poster label Oct 9, 2018
@Tyriar
Copy link
Member

Tyriar commented Oct 9, 2018

@rebornix did you configure the git editor to use --wait?

@rebornix
Copy link
Member Author

rebornix commented Oct 9, 2018

I showed it to @Tyriar and my git config uses --wait. Will debug with Daniel about this.

@Tyriar
Copy link
Member

Tyriar commented Oct 10, 2018

I tried code-insiders.cmd --wait ./Documents/dev/Microsoft/vscode/package.json and it released just fine for me on Windows after closing the tab.

@pastelmind
Copy link
Contributor

Came here from #60294. I solved the problem by changing git config core.editor from

'C:\Users\Phil\AppData\Local\Programs\Microsoft VS Code\Code.exe' --wait

to

'C:\Users\Phil\AppData\Local\Programs\Microsoft VS Code\bin\code.cmd' --wait

or simply

code --wait

@joaomoreno
Copy link
Member

joaomoreno commented Oct 22, 2018

This needs to be fixed by the Git installer which seems to set up Code.exe as the editor, instead of code.cmd. cc @bpasero

Found this: git-for-windows/git#1875

@joaomoreno
Copy link
Member

joaomoreno commented Oct 22, 2018

Nope, it seems there's really an issue on our side, investigating:

image

@joaomoreno joaomoreno assigned joaomoreno and unassigned Tyriar Oct 22, 2018
@joaomoreno joaomoreno added bug Issue identified by VS Code Team member as probable bug and removed info-needed Issue requires more information from poster labels Oct 22, 2018
@rebornix
Copy link
Member Author

rebornix commented Oct 22, 2018

For the particular error @joaomoreno mentioned above, when we open a vscode instance first and then run git config -e, the code throws exceptions in https://github.com/Microsoft/vscode/blob/8a6778cecc020fa438446ade9a9030b399e988ff/src/vs/platform/launch/electron-main/launchService.ts#L234-L238 . Only the cli code creates waitMarkerFile so in this code path, args.wait is true but args.waitMarkerFile is undefined. As we didn't pass in the right waitMarkerFile, we lost the ability to quit this process and it waits until the window is being closed.

Creating a new waitMarkerFile if it doesn't exist in this case can fix it. But if we close all VSCode windows, and then run git config -e, this path of code is not being executed, a proper fix may go to electron-main/main or electron-main/app but it's beyond my knowledge.

@joaomoreno joaomoreno assigned bpasero and unassigned joaomoreno Oct 23, 2018
@joaomoreno joaomoreno removed this from the October 2018 milestone Oct 23, 2018
@joaomoreno
Copy link
Member

joaomoreno commented Oct 23, 2018

@bpasero The investigation from @rebornix is 100% correct. Why is this code in CLI and not in the main process too? IMO they should share it.

Also, can we find a better way to do this instead of using a fake --waitMarkerFilePath opt, which gets added after args get parsed?

@bpasero bpasero added this to the October 2018 milestone Oct 23, 2018
@vscodebot vscodebot bot removed the insiders label Oct 23, 2018
@bpasero bpasero added feature-request Request for new features or functionality workbench-cli VS Code Command line issues and removed bug Issue identified by VS Code Team member as probable bug labels Oct 23, 2018
bpasero added a commit that referenced this issue Oct 23, 2018
@bpasero bpasero changed the title File handler is not released after the editor is closed Allow to use --wait with Code.exe Oct 23, 2018
@bpasero bpasero removed this from the October 2018 milestone Oct 23, 2018
@bpasero bpasero removed their assignment Oct 23, 2018
@bpasero
Copy link
Member

bpasero commented Oct 23, 2018

I fixed the NPE. I will convert this to be a feature request. I never intended this to fully work with Code.exe, but if someone has energy, feel free to jump in.

One thing that always annoyed me when using Code.exe directly is the fact that once the Electron process terminates, you are never put back to the Windows prompt. Here the launching process has terminated but the prompt is not showing up again:

image

@joaomoreno
Copy link
Member

Thanks! Verified the NPE is fixed. We just have to hook up the right parts in the main process now.

Yeah, that is annoying, tho I think it's more of a cmd.exe issue. You can avoid it and simulate what git does by appending | rem to the command.

@joaomoreno
Copy link
Member

@bpasero Note that a lot of new users are hitting git-for-windows/git#1875 right now. You can easily get it by (1) installing VS Code and (2) installing Git and choose VS Code as the default editor. For that reason, we probably want to get this done.

It might be a simple matter of refactoring the code out of the CLI and be able to use it from the CLI and the main process, right?

@bpasero
Copy link
Member

bpasero commented Oct 26, 2018

@joaomoreno please review/test 4252e67 that addresses part of this issue for the case where Code is running and you start it with --wait. As discussed, the --waitMarkerFile will now also be set very early in the launching. It will still not be very useful for the case where Code.exe is the first process that starts, because that one will never terminate.

@joaomoreno joaomoreno assigned bpasero and unassigned joaomoreno Oct 26, 2018
@joaomoreno joaomoreno added this to the October 2018 milestone Oct 26, 2018
@joaomoreno
Copy link
Member

joaomoreno commented Oct 26, 2018

Verified all scenarios:

Code.exe bin\code.cmd
Code not yet running 👌 👌
Code already running 👌 👌

Build tested: https://az764295.vo.msecnd.net/insider/e34c00f63eaca1c438120d8fb37cb0c3d67e38e2/VSCode-win32-x64-1.29.0-insider.zip

It will still not be very useful for the case where Code.exe is the first process that starts, because that one will never terminate.

It is useful, since the user usually closes the tab/editor right after editing, which will shutdown Code. The only case which doesn't work is if the user opens a new file/tab before closing the original file. This is OK for now, let's just wait for that issue to pop up in the real world.

@bpasero Great job, thanks!

@bpasero
Copy link
Member

bpasero commented Oct 27, 2018

@joaomoreno thanks for verifying.

@bpasero bpasero added the verified Verification succeeded label Oct 27, 2018
@joaomoreno joaomoreno added the verification-needed Verification of issue is requested label Nov 1, 2018
@vscodebot vscodebot bot locked and limited conversation to collaborators Dec 11, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request Request for new features or functionality verification-needed Verification of issue is requested verified Verification succeeded workbench-cli VS Code Command line issues
Projects
None yet
Development

No branches or pull requests

5 participants