Community
Participate
Working Groups
Steps: 1. Create a project. 2. Share it in a module, so the path to the project is like 'CVSROOT/TestModule/TestProject' 3. Create a branch IS: a branch contains a project tagged with the branch name SHOULD BE: a branch contains module tagged with the branch name and the project as a module child. Refreshing branches causes the TestModule to be added next to the project name, so the view is practically unusable - it offers informations without real value. + Branches + Branch1 + Project Branch1 + TestModule Branch1 + Project Important: if you delete and add the repo (it involves closing/disconnecting workspace projects) the branches will be rendered properly till the next branching is performed.
The first issue I have found is that the information about module tag is not stored anywhere. That's why the module does not appear until the refresh is finished. The separate issue is why the projects are displayed - situation may change if information about the module tag will be properly processed.
Tomasz, could you look into that and suggest where to start?
I would start from looking at RepositoriesView and its content provider - RemoteContentProvider. Any chances for a test case?
The steps are described in the beginning of the bug report. When you browse the repo, all information about modules and branches are retrieved and cached. Each project is identified by module/project path, and everything is correctly rendered. + Branches + Branch1 + TestModule Branch1 + Project Then branching happens. Only projects are branched, and projects in Eclipse have only project name, not module/project. So Eclipse caches project/branch, and when display happens, following structure is presented: + Branches + Branch2 + Project Branch2 The module has been skipped, because in the cache there are only projects without modules. If you refresh the repo, you will get something like: + Branches + Branch2 + Project Branch2 + TestModule Branch2 + Project Changes are merged. Cached local resources are not equal to the remote one, because local => project, and remote => module/project. So the cause is more or less known. The question is what to do next.
(In reply to comment #4) > The steps are described in the beginning of the bug report. I was referring to a JUnit test case, failing at the moment.
Created attachment 195490 [details] test which does not fail I tried to come up with a testcase but it worked properly - a module is a child of the branch. The difference may be related to the way in which resources are mapped. I am off tomorrow.
Changing the assignee to match the reality. Krzysztof, AFAIK it should be fixed in the nearest future.
Krzysztof is not going to work on it. I will take it.
(In reply to comment #8) > Krzysztof is not going to work on it. I will take it. Thanks :-)
I'll have a look at it.
I think that the problem is indeed in caching. Look at RepositoryRoot#addTags(String, CVSTag[]). It adds tags for given path to cache. It's javadoc says: "Accept the tags for any remote path that represents a folder. However, for the time being, the given version tags are added to the list of known tags for the remote ancestor of the resource that is a direct child of the remote root." The way I understand this sentence is that we store only direct children of remote root. That's why it works correctly for projects that are situated in the module top level. But when we add (from the comment 0) "TestModule/TestProject" this is not a direct child of the remote root. To be consistent with the javadoc we should add "TestModule" only. If we had project in a module like this: "TestModule/TestSubmodule/TestProject" we should also add "TestModule" only.
Created attachment 201405 [details] patch caching only the first segment This patch solves the problem. It adds the first segment of the path to the cache, not the full path. The path separators are "/" and "\".
(In reply to comment #12) > This patch solves the problem. It adds the first segment of the path to the > cache, not the full path. The path separators are "/" and "\". The doc on org.eclipse.team.internal.ccvs.ui.repo.RepositoryRoot.addTags(String, CVSTag[]) also says that "It is the responsibility of the caller to ensure that the given remote path is valid." and your patch changes that by allowing the caller to provide a path which is not precise. IMO we should either change the doc or fix the caller so it provides a proper value. Moreover, a test case would be nice to have. The one provided by Krzysztof in comment 6 could be a good start, but it's not very useful in its current shape. It passes even without a fix.
(In reply to comment #13) > The doc on org.eclipse.team.internal.ccvs.ui.repo.RepositoryRoot.addTags(String, > CVSTag[]) also says that "It is the responsibility of the caller to ensure that > the given remote path is valid." and your patch changes that by allowing the > caller to provide a path which is not precise. IMO we should either change the > doc or fix the caller so it provides a proper value. I have to say that I understand this comment differently. We want to add a tag for a resource of a given path, this *is* a correct path. The caller made sure that resource under this path really exists. However the rest of the javadoc says that for the time being we store only information about tags for top folder in the repository. So adding the full path is incorrect, because this is not the path of the top folder. As javadoc says that the solution of storing tags for the top folders is "for the time being" I believe it's better to leave callers passing full paths of the resources, because it seems that someday in future we will really store tags more detailed, for every resource. I suppose it's not a bad idea to rewrite javadoc to be more unambiguous.
Created attachment 202528 [details] Test that fails
(In reply to comment #15) > Created attachment 202528 [details] > Test that fails Quick comment: * There is no "folder1/b.txt" file in the test project at the time you're trying to delete it. * Do we actually need to make any changes to create a branch? * Do you really need moduleName and branchName to be fields? Limiting scope is good. * How many elements do you expect to be returned from RemoteContentProvider? Isn't it always 1? If so, I would skip the loop. * Since you're using a content provider to verify if the the branch is properly created I think we could consider moving the test to the "ui" package.
Created attachment 202665 [details] test > Quick comment: > * There is no "folder1/b.txt" file in the test project at the time you're trying > to delete it. corrected. > * Do we actually need to make any changes to create a branch? the branch is not created if there are no changes > * Do you really need moduleName and branchName to be fields? Limiting scope is > good. corrected > * How many elements do you expect to be returned from RemoteContentProvider? > Isn't it always 1? If so, I would skip the loop. What if there are more than one repository defined in the workspace? Then there would be more elements. > * Since you're using a content provider to verify if the the branch is properly > created I think we could consider moving the test to the "ui" package. done
(In reply to comment #17) > What if there are more than one repository defined in the workspace? Then there > would be more elements. As long as tests are run in isolation such case should not take place.
Created attachment 203490 [details] test So if we always have one repository in tests than we can skip the loop.
To be honest, I don't like that we're using only the first segment of the remote path as the cache key, but given the fact that this is how refreshing works I think it's the easiest way to fix it. The patch works and it's definitely heads in the right direction, I would change couple things: * update the comment on org.eclipse.team.internal.ccvs.ui.repo.RepositoryRoot.getCachePathFor(String) * the body could be simplified to return new Path(remotePath).segment(0); * add a similar test for tagging * name them with something like "testBranchSubmoduleChildren" * in the test, instead of calling #collectBranchChildren I would add a sequence of easy to read checks: // check if module is the only branch child RemoteContentProvider rcp = new RemoteContentProvider(); AllRootsElement are = new AllRootsElement(); Object[] repositoryRoots = rcp.getElements(are); assertEquals(1, repositoryRoots.length); Object[] categories = rcp.getChildren(repositoryRoots[0]); assertEquals(4, categories.length); assertTrue(categories[1] instanceof BranchCategory); Object[] tags = rcp.getChildren(categories[1]); assertEquals(1, tags.length); assertTrue(tags[0] instanceof CVSTagElement); assertEquals(branchName, ((CVSTagElement) tags[0]).getTag().getName()); Object[] modules = rcp.getChildren(tags[0]); assertEquals(1, modules.length); assertTrue(modules[0] instanceof RemoteResource); assertEquals(moduleName, ((RemoteResource) modules[0]).getName()); or even simpler: //... assertEquals(moduleName, ((RemoteResource) rcp.getChildren(tags[0])[0]).getName());
Created attachment 204535 [details] fix with test (In reply to comment #20) > * update the comment on > org.eclipse.team.internal.ccvs.ui.repo.RepositoryRoot.getCachePathFor(String) done > * the body could be simplified to return new Path(remotePath).segment(0); done > * add a similar test for tagging done > * name them with something like "testBranchSubmoduleChildren" done > * in the test, instead of calling #collectBranchChildren I would add a sequence > of easy to read checks: > [...] > rcp.getChildren(tags[0])[0]).getName()); I wanted to keep on the safe site so I didn't rely on the tree structure, but I suppose you're right that it's well defined. I agree with everything but one: you assumed that our branch will be the only branch in the tree. This is true if branches cache was empty at the begining, but I don't think it's cleaned before every test, so there might be some more branches in it. So I added iteration though branches and I'm looking for our branch by name.
(In reply to comment #21) > you assumed that our branch will be the only branch in the tree. This is true if > branches cache was empty at the begining, but I don't think it's cleaned before > every test, so there might be some more branches in it Good point. If it's trivial I would clean up the cache in a #setUp(). This way we will be able to focus on the case being tested and not on navigating thru other test leftovers. I'm not sure if this is just me, but after applying the latest patch I get 2 errors complaining about a local variable not used. Please check your Java>Compiler>Error/Warnings settings. There is a typo in both tests: "brancheExists" should be "branchExists". But you can ignore it once #setUp() is added. Moreover, in the second test the variable should be about tag not branch. "// refresh branches" is not the best choice in #testTagSubmoduleChildren(). These are probably copy-paste typos. You could also consider extracting those few lines with assertions into a method common for both test cases. Out of curiosity, why didn't you use Path(String) in RepositoryRoot.getCachePathFor(String)?
From our offline discussion I remember we were worried what would happen if you removed a branch/tag which had been used for more then one project under a module. Our first guess was that all projects for the given tag/branch will be removed from the cache even if we meant to remove only one of them. After looking at the code I can see that removing a tag can only happen when you're moving it (replacing) so the cache content should be fine. That finding and the fact we always display all folders under a selected module no matter if they were branched with the given branch name or not lead me to a conclusion that your patch is fine. I mean, once my comments from comment 22 are applied.
Created attachment 204949 [details] fix with tests
Created attachment 204950 [details] fix with tests
(In reply to comment #22) > Good point. If it's trivial I would clean up the cache in a #setUp(). This way > we will be able to focus on the case being tested and not on navigating thru > other test leftovers. This was not so trivial, but I've created new Test suite and it's done only for tests that test RepositoryView. > > I'm not sure if this is just me, but after applying the latest patch I get 2 > errors complaining about a local variable not used. Please check your > Java>Compiler>Error/Warnings settings. Sorry about that. My settings were fine, but there had to be a bug in one of the nightly builds. I've upgraded my Eclipse and it started to show unused variables errors again. > There is a typo in both tests: "brancheExists" should be "branchExists". But you > can ignore it once #setUp() is added. Moreover, in the second test the variable > should be about tag not branch. "// refresh branches" is not the best choice in > #testTagSubmoduleChildren(). These are probably copy-paste typos. You could also > consider extracting those few lines with assertions into a method common for > both test cases. This was not a typo. I was tagging the project by branch. From our offline discussion I know you wanted it to be tagged by version. I modified the test and we have it now, but it doesn't really check this error, because versions weren't cached anyway. But while writing it I discovered bug 360532. > Out of curiosity, why didn't you use Path(String) in > RepositoryRoot.getCachePathFor(String)? I found that a few years back it was used in this very same place. The change that caused this bug was made with only explanation in the comment to RepositoryRoot.getCachePathFor(String) (no bug for it). The explanation said that it's to "better support subdirectories", however from this bug we can see that our CVS support is not ready for it, so this bug is actually reverting a very old change.
Both tests fail when the test case is run as part of a larger suite (e.g. CVS UI Tests.launched). Actually, the test case is not added to the AllUITests.java so I had to do that first. The message is the same for both: junit.framework.AssertionFailedError: expected:<1> but was:<2> at junit.framework.Assert.fail(Assert.java:47) at junit.framework.Assert.failNotEquals(Assert.java:283) at junit.framework.Assert.assertEquals(Assert.java:64) at junit.framework.Assert.assertEquals(Assert.java:195) at junit.framework.Assert.assertEquals(Assert.java:201) at org.eclipse.team.tests.ccvs.ui.RepositoriesViewTests.getRepositoryRoot(RepositoriesViewTests.java:54) at org.eclipse.team.tests.ccvs.ui.RepositoriesViewTests.setUp(RepositoriesViewTests.java:43) [...] Other then that the patch looks good.
Created attachment 205121 [details] fix with tests There were two repositories in the workspace. Probably one of them was left by one of previous tests. I've added searching for our repository and tests pass.
I'm not able to apply your latest patch, please use the git repo.
Created attachment 205178 [details] fix with tests Corrected, it conflicted with fix for bug 360532.
The latest patch + some minor changes = 960dba32547fb75ebcb6cd0310340f23c6e61b62. The fix will be available in builds >=N20111014-2000. Krzysztof, could you please grab one of these and verify the fix?
While fixing it I found another bug: bug 360959. It is unrelated to this fix, but this fix has improved it a little.
Created attachment 205420 [details] verification result At first glance, everything looks fine. I have a module "testdocs" and three projects (TestProject with branch, TestProject2 with testbrunch and TestProject3 with testbranch). After branching, a proper module appears, and the tree structure is correct, but after branch refresh, all three projects are visible for all branches. If you try to checkout a project from a branch that does not exist, the project is empty.
(In reply to comment #33) > After branching, a proper module appears, and the tree structure is correct, > but after branch refresh, all three projects are visible for all branches. This happened also before this fix. This is how we work in this view: we get a directory that contains given tag and then we return its content no meter if it's tagged or not. For the example from comment 0 if we had also /TestModule/Project2 that is not branched we would see: + Branches + Branch1 + Project Branch1 + TestModule Branch1 + Project + Project2 and now we get: + Branches + Branch1 + TestModule Branch1 + Project + Project2 > If you try to checkout a project from a branch that does not exist, the project > is empty. This is expected, it always happens when you checkout a branch for a project that does not exist for this project.
Marking as verified, as the problematic scenario is solved and works for me.
(In reply to comment #35) > Marking as verified, as the problematic scenario is solved and works for me. The fix for the bug caused a regression, see bug 339990.
(In reply to comment #36) > The fix for the bug caused a regression, see bug 339990. It should read "bug 366016".
See also bug 360959.
