-
Notifications
You must be signed in to change notification settings - Fork 920
Remote property accessor on branch branch should not throw an exception #823
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -181,7 +181,7 @@ private void GetUpstreamInformation(string canonicalName, out string remoteName, | |
| } | ||
| else if (canonicalName.LooksLikeRemoteTrackingBranch()) | ||
| { | ||
| remoteName = Proxy.git_branch_remote_name(repo.Handle, canonicalName); | ||
| remoteName = Proxy.git_branch_remote_name(repo.Handle, canonicalName, true); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you think of a test that could leverage this code path?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added a commit that tests this code path. Setting a tracked branch where the remote cannot be resolved (in this case the remote cannot be found) throws an exception. |
||
|
|
||
| Remote remote = repo.Network.Remotes.RemoteForName(remoteName); | ||
| mergeBranchName = remote.FetchSpecTransformToSource(canonicalName); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -203,15 +203,21 @@ public static ReferenceSafeHandle git_branch_move(ReferenceSafeHandle reference, | |
| } | ||
| } | ||
|
|
||
| public static string git_branch_remote_name(RepositorySafeHandle repo, string canonical_branch_name) | ||
| public static string git_branch_remote_name(RepositorySafeHandle repo, string canonical_branch_name, bool shouldThrowIfNotFound) | ||
| { | ||
| using (ThreadAffinity()) | ||
| using (var buf = new GitBuf()) | ||
| { | ||
| int res = NativeMethods.git_branch_remote_name(buf, repo, canonical_branch_name); | ||
| Ensure.Int32Result(res); | ||
|
|
||
| return LaxUtf8Marshaler.FromNative(buf.ptr) ?? string.Empty; | ||
| if (!shouldThrowIfNotFound && | ||
| (res == (int) GitErrorCode.NotFound || res == (int) GitErrorCode.Ambiguous)) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| Ensure.ZeroResult(res); | ||
| return LaxUtf8Marshaler.FromNative(buf.ptr); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jamill Can you please share a hint about this change? 😉
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we expect this to be null - it should always be a proper null terminated string? This is not a pattern that we follow consistently - for instance the next method does not do this coalescing.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like it's been bring in through 4e43723. But I do not remember why it's been added. Let's get rid of it provided all the tests pass. |
||
| } | ||
| } | ||
|
|
||
|
|
||
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'm not sure about "containing", but can't come with a better word...
@carlosmn @dahlbyk Thoughts?