-
Notifications
You must be signed in to change notification settings - Fork 451
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
Proper error message for 'git gui' on Mac #1388
Conversation
GVFS/GVFS.Hooks/Program.cs
Outdated
@@ -128,12 +129,15 @@ private static void ExitWithError(params string[] messages) | |||
|
|||
private static void CheckForLegalCommands(string[] args) | |||
{ | |||
string command = GetGitCommand(args); | |||
switch (command) | |||
if (!RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is the recommended pattern here. We try to rely on the GVFSPlatform.Instance
and a property therein.
Alternatively: why not let this be in all platforms? We would just delete this entire block and put up a notice in #1387 that we currently block when you use git gui
but would not block if you run git-gui
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/block/lock
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wilbaker thoughts on allowing windows users to call 'git gui', knowing that it will be blocking? They can still of course run 'git-gui' and it would simplify our code base.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the point of letting users run git gui
if nothing will work? Is that the case or will some things work other not? Are we going to document for users what works and what doesn't? Seems like a bad user experience to say you can run git gui
but we don't know what will work or what will be an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kewillford, to clarify since 'git gui' takes a GVFS lock everything run in that application works. However if you run 'git status' while 'git gui' is open, you will get a message saying it needs to wait for the lock.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is the recommended pattern here. We try to rely on the GVFSPlatform.Instance and a property therein.
👍
@wilbaker thoughts on allowing windows users to call 'git gui', knowing that it will be blocking?
We used to have that behavior on Windows and received enough feedback regarding the poor experience that we decided to block it and force users to call git-gui
. I'm not sure that we should go back to allowing users to get into that situation on Windows.
Is that the case or will some things work other not? Are we going to document for users what works and what doesn't?
Another option here (if we think the blocking behavior of git gui
is a bad enough experience that we should block users from getting into it) would be to keep the block but print a more accurate message on Mac (e.g. "git gui
is not supported in VFS for Git repos").
If we do allow git gui
on Mac I like the idea of printing a notice to users to let them know that an open git gui window might block other git commands from running.
cc: @jrbriggs for his thoughts as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spoke with @jrbriggs
For now we'll fix the error message to say "git gui is not supported in VFS for Git repos on Mac".
If we get user feedback that "git gui" is needed, we should consider making "/usr/local/git/libexec/git-core" accessible via path on mac.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More fun!
Please see comments as to why I stuck with RuntimeInformation.
Also this did require loading a tiny dll. If we hate that idea, perhaps we can change the existing error message to just say
"To access the 'git gui' in a GVFS repo, please invoke 'git-gui.exe' instead. Note you may have to access 'git-gui.exe' by it's full path on mac"
Thoughts @wilbaker?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see comments as to why I stuck with RuntimeInformation.
I couldn't find any comments on this. Rather than using RuntimeInformation
could you add a new method to GVFSHooksPlatform?
Something like this in GVFSHooksPlatform.Mac.cs:
public static string GetGitGuiBlockedMessage()
{
return "git gui is not supported in VFS for Git repos on Mac";
}
And the current message on Windows?
405aac1
to
4b0c049
Compare
resolves #700
#1387 has been created (not prioritized) to allow 'git-gui'