Show "None" message in diagnose instead of invalid url #329
Conversation
GVFS/GVFS.Common/GVFSEnlistment.cs
Outdated
| @@ -11,9 +11,9 @@ namespace GVFS.Common | |||
| public partial class GVFSEnlistment : Enlistment | |||
| { | |||
| public const string BlobSizesCacheName = "blobSizes"; | |||
| public const string InvalidRepoUrl = "invalid://repoUrl"; | |||
There was a problem hiding this comment.
I don't think we should make this constant public. The point of it was that it should never leak out of this class.
Right now, I think you're fixing the symptom by checking for this constant before displaying it. But how did the invalid URL leak out of here in the first place? What other code paths might be affected? Rather than special case all uses, I'd like to understand the root cause better.
| @@ -53,7 +53,9 @@ protected override void Execute(GVFSEnlistment enlistment) | |||
| this.WriteMessage(GVFSPlatform.Instance.GitInstallation.GetInstalledGitBinPath()); | |||
| this.WriteMessage(string.Empty); | |||
| this.WriteMessage("Enlistment root: " + enlistment.EnlistmentRoot); | |||
| this.WriteMessage("Cache Server: " + CacheServerResolver.GetUrlFromConfig(enlistment)); | |||
| string cacheServer = CacheServerResolver.GetUrlFromConfig(enlistment); | |||
There was a problem hiding this comment.
Note that GetUrlFromConfig should have returned the origin URL if there was no cache server configured. So this is not just a case of "no cache server". Did we actually somehow create a bogus origin remote? Or is there some other bug that is causing that bad URL to leak out of this method?
If not cache server is present display "None"
jeschu1
force-pushed
the
cacheserverurl
branch
from
733d07e
to
0bf8b3f
Compare
jeschu1
changed the title
| @@ -90,7 +89,7 @@ public static GVFSEnlistment CreateWithoutRepoUrlFromDirectory(string directory, | |||
| return null; | |||
| } | |||
|
|
|||
| return new GVFSEnlistment(enlistmentRoot, InvalidRepoUrl, gitBinRoot, gvfsHooksRoot); | |||
| return new GVFSEnlistment(enlistmentRoot, string.Empty, gitBinRoot, gvfsHooksRoot); | |||
There was a problem hiding this comment.
@sanoursa we discussed sending null in here, however in the base if you send in null it will attempt to look up details for the repository here. We don't want that in this case. Previously there was a bug fix to diagnose explicitly saying don't fail if you can't get origin info.
We could look at other options like sending in a flag rather than the string.Empty. Let me know what you think.
There was a problem hiding this comment.
I think this is fine. Any place where we try to use this as a URL, we will still fail. And your fix in DiagnoseVerb now ensures that we show a good display string.
When a cache server is not set up we shouldn't be showing the invalid default url.
Instead let's show a helpful message to the user.
#289