[Waiting Review] Improvements in Tfs branches support #480
Conversation
ghost
assigned 
|
What missing in this PR? When you plan merge it? |
|
Maybe support only simple renaming branch at start and baseless merge? |
|
I have worked on that yesterday after quite some long time without having time to do it :( What I need before merging this pull request is a good code review, doing some test on different scenarii (I have only tested with Did you have tested it successfully yourself? Does that solve cases where released git-tfs failed? |
|
Now I try to merge this branch with my to check it out. |
I'm not sure I want to do that. It's perhaps what I should have done in the beginning but things are too intricate and I'm not sure I will be able to merge them one by one without lot of works! :( |
|
@KindDragon I successfully manage the last point I want to be solved before trying to merge the PR. |
|
👍 👍 👍 |
|
@spraints @sc68cal @KindDragon |
| @@ -64,6 +65,9 @@ public int Run(string tfsUrl, string tfsRepositoryPath, string gitRepositoryPath | |||
|
|
|||
| VerifyTfsPathToClone(tfsRepositoryPath); | |||
|
|
|||
| if (withBranches && initBranch != null) | |||
| fetch.IgnoreBranches = false; | |||
|
|
|||
| if (retVal == 0) fetch.Run(withBranches); | |||
There was a problem hiding this comment.
I think branch --init --all should work almost identical as clone --with-branches.
Maybe we shouldn't call fetch if withBranches == true?
There was a problem hiding this comment.
hmm... Need some explanation... ;)
branch --init --all and clone --with-branches couldn't work as identical as because the branch command could only be used in an already initialized repository.
Except from that, with this PR, they do the same thing! \o/
you could do:
init+branch --init --allclone+branch --init --allclone --with-branches
and you will end up with the same repo!
Maybe we shouldn't call fetch if withBranches == true?
No, must keep the way it works. clone is init + fetch. If we remove fetch what is remaining? And I don't really understand why you want to condition fetch call...
if (withBranches && initBranch != null) fetch.IgnoreBranches = false;
That's just a security to prevent use of --with-branches and --ignore-branches that are 2 options incompatibles.
--ignore-branches is an option to keep the way git-tfs actually works for those where the TFS repository is a mess and can't support branches.
withBranches is an option to the fetch command to tell it to do special things to support branches when fetching. So it's OK...
To conclude, all is OK, like that! I have tested and approved!! ;)
There was a problem hiding this comment.
No, must keep the way it works. clone is init + fetch. If we remove fetch what is remaining? And I don't really understand why you want to condition fetch call...
Clone call after Fetch - InitBranch.Run() if withBranch = true. I thought it will fetch all branches?
There was a problem hiding this comment.
Oh! I understand! you're right, we can remove the call to the fetch because after we call initBranch.Run() now that the pull request manage branch initialization "on the fly".
But I'm not sure I will remove this call. Because if you've got a problem in the initBranch call, there is more chance that you've got your main branch fetched... I don't know....
I have updated the PR description with what this PR is doing... |
I have included your 2 commits in the PR. Is the problem solved by the remove of the 2 .ToLower()? Or does that still need to be investigated? |
|
I cannot reproduce bug anymore |
|
@pmiossec How to clone a single branch with all parent branches and branches that have been merged into this branch? |
|
With this PR, if that's the trunk branch that you clone, that's done If that's not the trunk that you want to clone, that's not supported for
|
|
I'm testing this build and it looks like the code is causing a loop. My command: git-tfs.exe clone --with-branches http://tfs:port/tfs/project $/project/rootBranch NewProjectName Output Branches to Initialize successively : This was looping for about a 1 1/2 days with both tests. Is there anything I can do to help debug this? |
|
@theyeag your log is a little light ;) but I think (and I hope) I know where your problem come from... I will update this pull request with a correction but the bad news is that you end in a loop because git-tfs was unable to initialized a branch. I think that it was unable to find the root changeset of the branch :( |
|
Ha, I was trying to get it down to the bare minimum. I hate diving through useless information so I try to save others where I can :). I have a guess to what may have caused this. Let me know if I can provide any logs, tests, or examples. I'm digging into the code but my C# is a little rusty. It may be a bit before I'm much use. |
|
@theyeag here is a new version. I hope it will solve the problem of the loop and better know why one of your branch couldn't be initialized...
Basless merge should be supported and deletion too (but perhaps not all cases possible)... |
| @@ -33,13 +33,24 @@ public LogEntry Apply(string lastCommit, IGitTreeModifier treeBuilder, ITfsWorks | |||
| var resolver = new PathResolver(Summary.Remote, initialTree); | |||
| var sieve = new ChangeSieve(_changeset, resolver); | |||
| workspace.Get(_changeset.ChangesetId, sieve.GetChangesToFetch()); | |||
| foreach (var change in sieve.GetChangesToApply()) | |||
| var changes = sieve.GetChangesToApply().ToList(); | |||
| if (!IsRenameChangeset(changes, workspace.Remote.TfsRepositoryPath)) | |||
There was a problem hiding this comment.
This seems like a really broad exclusion. I read it as "skip any changeset that has a deleted item outside of the directory I'm working in." I'm concerned that a changeset with changes like this:
- Edit $/Project/FileICareAbout.cs
- Delete $/OtherProject/FileIDoNotCareAbout.cs
will get skipped.
Also, I'd really prefer that logic like IsRenameChangeset lives in ChangeSieve so that it can be covered more easily by unit tests.
There was a problem hiding this comment.
....and then prevent all the delete when the changeset is a renamed changeset!
I will have a look at how to solve that in a better way....
|
@pmiossec - ❤️ This looks like a pretty awesome improvement! Other than the inline comments, can you
|
|
@pmiossec Clone |
|
@KindDragon I have cherry-picked your commit but if you end here, I think there is a case that is not well managed by the PR :( And I am interested by knowing what is this case. Delete branch? something else? |
|
I will try reproduce this issue again |
|
I plan to merge that very soon (one or two days) and continue building on top except if someone yell against it... It was waiting too long! |
[Waiting Review] Improvements in Tfs branches support
|
@pmiossec I think we should change Maybe made |
|
Perhaps you're right. I'll think about that. How to change that. Perhaps we will have to change the state of the internal parameter... |
|
Or maybe merge options
|
🙇 Thanks for your patience, I think this is a good time to have merged this. |
|
Unfortunately I have been having some trouble using this feature. I have a convoluted branch/merge heirarchy on the TFS server that I am attempting to pull from. Project A branches to a subfolder of Project B and H I want Project F and project H Unfortunately I don't seem to be able to clone F, with H as a branch without also having git-tfs attempt to clone A, B and C. The --ignore-branches switch does not seem to work with --with-branches. My goal: I want to clone a TFS repository and one of the branches that originated from this repository. There could be a couple of solutions to this problem:
Unless I'm missing something? |
|
Ok it turns out that I was missing something. I achieved my goal by doing the following:
|
| private string FindMergedRemoteAndFetch(int parentChangesetId, bool stopOnFailMergeCommit, out string omittedParentBranch) | ||
| { | ||
| var tfsRemotes = FindTfsRemoteOfChangeset(Tfs.GetChangeset(parentChangesetId)); | ||
| foreach (var tfsRemote in tfsRemotes.Where(r=>string.Compare(r.TfsRepositoryPath, this.TfsRepositoryPath, StringComparison.InvariantCultureIgnoreCase) != 0)) | ||
| return FindRemoteAndFetch(parentChangesetId, false, true, out omittedParentBranch); |
There was a problem hiding this comment.
The stopOnFailMergeCommit parameter is unused and false is passed to the stopOnFailMergeCommit parameter of the called method. Should it be fixed?
There was a problem hiding this comment.
That's always good to clean the mess 😉
Could you do a PR?
This pull request bring bing improvements in TFS branch support:
--ignore-branchesoption to ignore merge changeset and process it as a normal changeset (for those that can't manage branches due to a messy tfs repository).If I clone tfs/default (
git tfs clone https://tfs.codeplex.com:443/tfs/TFS16 $/vtccds/trunk) and it encounter a merge changeset, it create and fetch automatically the branch tfs/b1 to be able to create the merge commit.