RFC: handle "TF14098: Access Denied" exceptions #1176
Conversation
src/GitTfs/Core/GitTfsRemote.cs
Outdated
| { | ||
| // Ignore exceptions "TF14098: Access Denied: User ??? needs | ||
| // Read permission(s) for at least one item in changeset ???." | ||
| if (!ex.Message.Contains("TF14098")) |
There was a problem hiding this comment.
It would be nicer to check for the exact exception here: Microsoft.TeamFoundation.VersionControl.Client.ResourceAccessException. However, then I would need to include the Microsoft namespace here and I have the impression that would interfere the separation of concerns in the git-tfs project, no?
There was a problem hiding this comment.
However, then I would need to include the Microsoft namespace here and I have the impression that would interfere the separation of concerns in the git-tfs project, no?
Right, GitTfs/Core shouldn't be able to see anything in the Microsoft.TeamFoundation namespace, though it's mostly related to packaging/versioning rather than separation of concerns. If the class is consistent, it'd be ok to do something like ex.GetType().FullName == "Microsoft.TeamFoundation.VersionControl.Client.ResourceAccessException", though my preference is to stick with the check for TF14098 in the exception message.
I think this is a good change. I haven't had a chance to look through the code yet. I think that putting the workaround behind an option makes sense, and maybe also suggest using it if the error happens? |
src/GitTfs/Core/GitTfsRemote.cs
Outdated
| throw; | ||
| } | ||
|
|
||
| Trace.WriteLine("Quick fetch failed: " + ex.Message); |
There was a problem hiding this comment.
In this case, would there end up being any contents in the repo? If not, maybe it doesn't make sense to catch for quick-clone?
There was a problem hiding this comment.
I agree that no contents would be in the repo in case of quick-clone. However, it looks like as if QuickFetch is also called within a full fetch for the very first change set:
git-tfs/src/GitTfs/Commands/Fetch.cs
Line 192 in c89e8dd
Do you see an easy way to distinguish the two cases?
There was a problem hiding this comment.
Ah, right, I forgot about that. It'd still be good to make quick-clone fail, but not at the expense of breaking full clones or fetches. So, if it's not too cumbersome to separate the behavior of the two cases, please do.
src/GitTfs/Core/GitTfsRemote.cs
Outdated
| { | ||
| throw; | ||
| } | ||
| Trace.WriteLine("No commit found commit changeset " + rootChangesetId + ": " + ex.Message); |
There was a problem hiding this comment.
Should this return null, since sha1RootCommit will be null, and it looks like this method returns early if it's unable to set sha1RootCommit.
There was a problem hiding this comment.
Nice catch. Yeah, I will try that and update the PR!
|
@spraints I assume you would prefer it if this new behaviour would be tested somehow. Can you give me some pointers how I could approach that? For example an existing test case that is similar that I should study or maybe an existing test case that throws an exception like the one I am catching here? |
Yeah, tests would be great. I think an integration test would make the most sense. Here's an example of a test of https://github.com/git-tfs/git-tfs/blob/master/src/GitTfsTest/Integration/CloneTests.cs#L42-L61 You'll need to extend the TFS test double so that it can throw an error for a particular changeset instead of returning. Does that work as a starting point? |
|
I haven't forgotten about this. I found a few more issues and my Windows box is still working on the migration. I'll get back to it as soon as all my TFS content is in Git (hopefully in a week or two). |
|
@larsxschneider Any update on that? |
1d18a4b to
330a010
Compare
|
Uhh. Thanks for the reminder @pmiossec ! What do you think of the current implementation? Would that be acceptable already? I wanted to create a test case but I didn't know how to mock the access management. |
| } | ||
|
|
||
| public void QuickFetch(int changesetId) | ||
| public void QuickFetch(int changesetId, bool ignoreRestricted, bool printRestrictionHint) |
There was a problem hiding this comment.
Is this refactoring OK? If yes, then I could make it an independent commit before the --ignore-restricted-changeset commit.
330a010 to
87f3a41
Compare
If a TFS user has no read access to a changeset, then TFS responds with a "TF14098: Access Denied" error. This error aborts the fetch operation. Add the --ignore-restricted-changesets option which allows the user to ignore these respective changeset. If the option is set, then git tfs will import the access restricted changeset as empty commit and ignore the error.
87f3a41 to
d4391dc
Compare
I am working with a TFS repo that I don't have full access to. I am not interested in the parts that I don't have access to and I just want
git-tfsto ignore these parts. Therefore, I implemented this very basic workaround here. I wonder what you think about the approach? Do you see problems with it? Would that be something you would be willing to merge? Maybe I could mask the workaround behind an option--ignore-restricted-contentor something?See also #1033