Fix broken SCM tree #8048
Fix broken SCM tree #8048
Conversation
Signed-off-by: Nigel Westbury <[email protected]>
|
@vince-fugnitto Could you help reviewing it? or find a reviewer? |
|
Applying "How to Test", I don't see any differences between this patch and the master branch commit. It seems to be the same. What exactly should not be the same ? Is there something specific in the tree ? |
Did you unstage the changes? The problem is caused because 'git status' returns the untracked files last. So if a folder contains both tracked and untracked files then the folder appears twice in the tree (except it doesn't because it will have the same id in both instances). Have a look at the screen shot in #8004. The problem occurs for both Theia's git package and vs code's builtin. |
|
I followed the See the tree under 'Staged Changes'. This tree is correct. Samething with / without this PR
|
|
@lmcbout Sorry, I meant the initial commit in the branch, not the initial commit in the repository. That would be resetting to daede30. I just checked out the branch then pressed the 'Amend' button three times (to amend the two merge commits plus the first commit in the branch). You just need to get both a modified file and an untracked file into the CHANGES (unstaged) section. These two files must be in the same folder and not at the root of the repository. Furthermore these two files must be separated (in the order returned by git status) by one or more other files that are in a different folder. |
| @@ -207,19 +211,19 @@ export class ScmTreeModel extends TreeModelImpl { | |||
| if (folderEnd - folderStart < nestingThreshold) { | |||
There was a problem hiding this comment.
To me, the name "nestingThreshold" seems to indicate the depth of the tree. I don't see how the difference between two different indices in the list of resources is related to that. Shouldn't this expression involve the level somehow?
| // Multiple files with first folder. | ||
| // See if more folder levels match and include those if so. | ||
| let thisLevel = level + 1; | ||
| while (thisLevel < firstFileParts.length - 1 && thisLevel < lastFileParts.length - 1 && firstFileParts[thisLevel] === lastFileParts[thisLevel]) { | ||
| thisLevel++; | ||
| } | ||
| const nodeRelativePath = firstFileParts.slice(level, thisLevel).join('/'); | ||
| result.push(this.toFileChangeFolderNode(resources, folderStart, folderEnd, thisLevel, nodeRelativePath, parent)); | ||
| result.push(this.toFileChangeFolderNode(sortedResources, folderStart, folderEnd, thisLevel, nodeRelativePath, parent)); | ||
| } | ||
| folderStart = folderEnd; | ||
| } |
There was a problem hiding this comment.
I think we can get rid of result.sort(...) if we keep separate result arrays for file nodes and folder nodes and concatenate them at the end. With your change, the list of resources is already sorted and as far as I can see, the sorting will be correct under the algorithm.
|
@westbury is this algorithm time-critical? I have an idea to make it O(n), just wondering if it would be worth giving it a try. |
|
Thanks for your reviews. @tsmaeder You are correct that the files and folders can be pushed to two lists and then appended, so O(n) rather than O(n log n). I would rather take the time to do a fuller performance investigation as we have a ticket against Mbed Studio complaining about the 1000 limit on the changes status. So I'll merge this fix for now. |
Fixes #8004
What it does
The code that generated the tree in the Source Control view assumed that files came back from the SCM sorted, or at least with files in the same folder kept together. Git mostly does, but not always. So this PR sorts the files before building the tree.
How to test
Review checklist
Reminder for reviewers