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

initial code to show how async 'git status' prompt can work #104

Closed
wants to merge 3 commits into from
Closed

initial code to show how async 'git status' prompt can work #104

wants to merge 3 commits into from

Conversation

jamesmanning
Copy link

This is intended more for discussion than being merged 'as-is' - if/when it were to be merged, the common bits of functionality in the sync and async code paths should be factored out (and maybe the sync case being the async code path with an added wait on the process to exit, but that's not really relevant right now).

The approach taken here intentionally doesn't use any of the other work (like caching status, filesystemwatcher, etc) - not that it couldn't be used with those, but more to keep it as simple as possible for now for a hopefully smaller diff and easier discussion about whether it's a good fit for posh-git.

The logic/data flow isn't much different than the existing synchronous prompt - it's controlled in this diff by a new bool in the GitPromptSettings (EnableAsyncFileStatus) for easier testing and comparison.

The key 'breakthrough' WRT earlier discussions in issues like #52 was finding that the RawUI interface didn't actually require cursor location manipulation in order to write to the screen buffer in a place other than wherever the cursor was already located. Instead, it supports writing a 2d rectangle to the buffer at a specified coordinate. This means we don't have to worry about the various bad states we might get into if we were manually moving the cursor and writing strings out to the buffer while the user was typing at their prompt (yay!)

PSHostRawUserInterface.SetBufferContents Method (Coordinates, BufferCell[,])

The main new/entry function to look at it is Write-GitStatusPromptAsync in the new file GitAsyncPrompt.ps1

in the case of EnableAsyncFileStatus -eq $true (and we're in a repo that hasn't been disabled), it does:

  • saves the current cursor position to know where it will write the multi-colored status string once it's done being calculated
  • writes out a 'placeholder' string to the host on its own line (not strictly necessary, just to give the user an idea taht there's an ongoing process happening, even if it's only going to take a few seconds)
  • run the same "git status" cmdline as usual, just doing so as a System.Diagnostics.Process
  • gathers stdout lines via OutputDataReceived event on the process
  • when Exited event happens on the process:
    • takes the stdout lines, converts to the same object already being stored in $global:GitStatus by the existing prompt code, and also writes the object to that global variable to maintain that behavior
    • calls Get-GitStatusBufferCells to convert that object into the set of buffer cells (multi-colored string, currently always 1 row tall just like the existing synchronous code)
    • calls Write-BufferCellsToPosition to write those buffer cells to the screen buffer (correct location was grabbed/stored earlier)

IME it runs 'fast enough' that even if it hit enter ~10 times, resulting in ~10 git status calls running, they all still complete very quickly (and they each write to their correct screen buffer location).

Does the idea/approach make sense for posh-git?

Thanks!

Related issues about perf of git status:

- while it works well for me in my testing, this is meant
  more as a basis of discussion for 1) whether it makes sense
  for this to be in posh-git and 2) if so, what it should
  look like.  This version intentionally doesn't refactor out
  common code with the synchronous path since the goal is
  validating/rejecting the idea instead of the particular impl
@dahlbyk
Copy link
Owner

dahlbyk commented Jun 20, 2013

Thanks, James! I think an ideal setup might be to combine a file watcher (to prevent unnecessary calls) with an opt-in async prompt for slow repositories (we could even automatically switch if status is detected to take longer than some threshold).

However, my first attempt to use this leaves me stuck at:

C:\Dev\OSS\posh-git [...]
>

Relevant settings:

> $GitPromptSettings | fl Enable*


EnableWindowTitle     : posh~git ~
EnableAsyncFileStatus : True
EnableFileStatus      : True
EnablePromptStatus    : True

Anything you'd just suggest I check before I dive in to debug?

@jamesmanning
Copy link
Author

Does the [...] not change to the git status information? Does it stay 'stuck' at [...] even for subsequent prompts?

Some things to try:

  • Since it's an exported function, you could try running Write-GitStatusPromptAsync
    • for instance, I could run: function prompt { "$pwd> " }
    • then if I run Write-GitStatusPromptAsync, I get the placeholder text on a line by itself:
      image
    • a few seconds later, it changes to the actual git status string:
      image
    • I couldn't figure out the 'right' way to add the dbg calls (specifically, which scope made the most sense for creating the stopwatch, and then the call tree underneath it having to accept the stopwatch parameter), so those might end up being valuable?
    • calling Write-GitStatusPromptAsync 'standalone' like this, combined with Set-PSDebug, was what I ended up doing during most of the testing/debugging, FWIW. I'd normally use ISE for such things, but SetBufferContents didn't appear to work the same way in that host, so I stuck with debugging in a console window.
  • in GitAsyncPrompt.ps1 line 3, the FileName for running git is just "git"
    • While I don't have git.cmd in my path and don't use it, I wasn't sure if making it "git.exe" specifically was a good/safe idea
    • However, since UseShellExecute = $false is set, if git.cmd is in the path and resolves ahead of git.exe, then maybe what's happening is it's failing to run git.cmd since that might require the shell being the one to execute it.
    • IOW, assuming you have a "git.exe" in your path, maybe replace "git" with "git.exe" for the FileName property of the ProcessStartInfo instance?
    • the existing call in Get-GitStatus (line 113 of GitUtils.ps1 - $status = git -c color.status=false status --short --branch 2>$null) shows it not specifying the extension for git, so if supporting git.cmd as what 'git' resolves to is needed, and it turns out UseShellExecute = $false is the culprit, then UseShellExecute = $true might be the right fix?

potentially useless aside

In my preferred prompt, the git status information is on the line above the 'real' prompt - since we don't know how wide the resulting status string is going to be, the 'async prompt' code captures the cursor position, writes the placeholder text and a newline. This means that when the placeholder is replaced with the 'real' status string later on, we're not overwriting part of their 'real' prompt line (where they may have already started typing something).

So, when the prompt initially renders for me, it has the placeholder on a line:

image

And then it gets replaced with the real status a few seconds later:

image

During testing of the prompt behavior, though, I included source'ing the posh-git profile.example.ps1 to see if it would Just Work. Since the current prompt function in there does a Write-Host for $pwd.ProviderPath instead of returning it back to the caller, the placeholder text isn't at the beginning of a line, but it still works correctly (at least in my testing) as the placeholder text is replaced with the status string:

So, when it first renders the prompt, we have the provder path first, then the placeholder text and newline, and the rest of the prompt (the "> " returned back from the prompt function)

image

And then a few seconds later, the placeholder gets overwritten with the actual status:

image

@dahlbyk
Copy link
Owner

dahlbyk commented Dec 31, 2016

Between #319 and #208 we should be in a better spot performance-wise. I'm going to go ahead an close this, but do keep an eye out for an upcoming release with those improvements and leave a note on #78 if you're still having issues.

I do appreciate the effort you put into this several years ago!

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 this pull request may close these issues.

2 participants