Use correct repoUrl for credential#327
Conversation
| try | ||
| { | ||
| GVFSEnlistment enlistment = GVFSEnlistment.CreateFromDirectory( | ||
| this.Enlistment = GVFSEnlistment.CreateFromDirectory( |
There was a problem hiding this comment.
Alternatively I could grab the repoUrl off of enlistment and use it in the TryGetCredentials below. I'm not sure how much of a no no resetting this.Enlistment is, but in this case it is being updating to be correct.
There was a problem hiding this comment.
See my other comment. Given that you had to add a new set accessor, I would go with your other idea for the fix.
GVFS/GVFS/RepairJobs/RepairJob.cs
Outdated
| protected ITracer Tracer { get; } | ||
| protected TextWriter Output { get; } | ||
| protected GVFSEnlistment Enlistment { get; } | ||
| protected GVFSEnlistment Enlistment { get; set; } |
There was a problem hiding this comment.
Yea I definitely think we should keep the property readonly. In general, readonly state is much easier to reason about than mutable state.
| try | ||
| { | ||
| GVFSEnlistment enlistment = GVFSEnlistment.CreateFromDirectory( | ||
| this.Enlistment = GVFSEnlistment.CreateFromDirectory( |
There was a problem hiding this comment.
See my other comment. Given that you had to add a new set accessor, I would go with your other idea for the fix.
This change allows us to reinitialize the GitProces with valid repository information information and pass it correctly to the credential call.
| { | ||
| this.Enlistment.UnmountGVFS(); | ||
|
|
||
| this.Enlistment.Repair(); |
There was a problem hiding this comment.
Do you know if the repair without the --confirm would have caught this as well? I was thinking that for each of our tests in this class would should run the repair without --confirm before running the confirm version to catch other possible code paths.
There was a problem hiding this comment.
@kewillford , yes without --confirm we still get the error. It's up to us if we want to do another run without the flag. My only concern would be the length of time to run the tests, but if it's a test worth doing we should do it!
There was a problem hiding this comment.
I was thinking we could do it in the test cases we already have not adding new test cases.
Run repair without --confirm then run with it in the same test case. Hopefully this wouldn't add to much time.
kewillford
left a comment
kewillford
left a comment
There was a problem hiding this comment.
Only question is if we can get better coverage by running the repair without --confirm in each test case.
sanoursa
left a comment
sanoursa
left a comment
There was a problem hiding this comment.
Latest changes look good!
4 participants
This change allows us to reinitialize the GitProcess with valid repository information and pass it correctly to the credential call.
The bug was caused by the refactor here.
A new unit test was added that exposed the failure.
#289