Bugzilla – Full Text Bug Listing
| Summary: | Git log graph is broken on Node | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [ECD] Orion | Reporter: | Remy Suen <remy.suen> | ||||||
| Component: | Git | Assignee: | Remy Suen <remy.suen> | ||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||
| Severity: | major | ||||||||
| Priority: | P2 | CC: | steve_northover | ||||||
| Version: | 13.0 | ||||||||
| Target Milestone: | 15.0 | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Whiteboard: | |||||||||
| Bug Depends on: | 510074 | ||||||||
| Bug Blocks: | |||||||||
| Attachments: |
|
||||||||
Created attachment 266176 [details]
Image describing the difference in behaviour.
I spent some time looking at this and I got some code working. However, note the difference in behaviour in the provided attachment. I can make the Node implementation look the same as the Java implementation but I feel like the Java implementation is a little misleading in this case. Having the line extend further can incorrectly gives the user the feeling that there is another parent commit somewhere in the nether when there aren't actually anymore to be found.
Of course, since the usual button of 'More commits for "master"' isn't there, the user can likely deduce that there is nothing more to see here.
Thoughts anyone?
(In reply to Remy Suen from comment #1) > Having the line > extend further can incorrectly gives the user the feeling that there is > another parent commit somewhere in the nether when there aren't actually > anymore to be found. I did some more checking here. If the actual commit really doesn't have a parent commit (ergo, it is the initial commit of the repository), the graph does end so it's not like the Java implementation will "arbitrarily" extend the line further. > I spent some time looking at this and I got some code working.
Is this still a problem? Do you still have the code around and are you happy with it?
(In reply to Steve Northover from comment #3) > Is this still a problem? Yes, this is still a problem, Steve. > Do you still have the code around and are you > happy with it? The code was just a proof-of-concept thing. It did not consider merges and made all the commits into a straight line without any branching behaviour. The title says that this feature is broken on Node. If this is true and we do not have a fix, then please disable the feature for Node only (for now) so that we don't have broken code in the field. (In reply to Steve Northover from comment #5) > The title says that this feature is broken on Node. If this is true and we > do not have a fix, then please disable the feature for Node only (for now) > so that we don't have broken code in the field. I don't know how to disable features on the client. :( Hide the UI. (In reply to Steve Northover from comment #7) > Hide the UI. Sorry. I mean, how can the client tell if the server is a Java server or the Node server? I have delivered a naive implementation to master. https://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=004e3d690bb6ff856b662b47ff20f66f2f2ed6bf More work still needs to be done with regards to paging and merges. Essentially, going up to N commits is insufficient. You have to go up to at least N+1 because the parent commit is not necessarily guaranteed to be the following commit that also affects the interested file. So after you find N commits you still need to search in M other branches where M is the number of lingering branches that needs to have their parent commits resolved. I'm pretty sure there are some other cases I haven't thought of but here's some food for thought for now. 1. If it is a linear history, then you need to find at most one more commit that affects the file. O | O <- paging stopped | X <- go further | O <- okay 2. If the last commit is a merge, you'd have to dig through all its parents. O | O <- paging stopped |\ | \ X | <- go further | | | X <- go further | | O | <- one branch resolved | | | O <- other branch resolved, stop here 3. If the last commit is a merge, but only one commit is branch is relevant, only use that one commit. This is because the file's history in this case is essentially a linear history as the other branch does not affect the file. O | O <- paging stopped |\ | \ X | <- go further | | | O <- one branch resolved | | | / |/ O <- other branch resolved to here, but should just return the other commit 4. If an earlier commit was a merge commit, resolving the current branch is insufficient. O <- merge early in history |\ O \ <- other branch | | | O <- paging stopped | | X | <- go further | | | O <- okay, but the other branch needs to be resolved too | | O | <- now okay, the other branch has resolved to a relevant commit Pushed a pass over the code to master. https://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=2b1c10efad6b00d6b75df93df1f3aa81b4860a94 Use cases 2 and 3 from comment 9 is still unimplemented. Use case 2 should be working now too. https://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=b7a9b20626eddac11fb6be0d49587eb8d0a629dd Use case 3 should be working now too. https://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=6811b5a95ee231658a8446eab4e81a23dd96cc87 Histories with two unrelated branches needs to be fixed and the handling of newly added or deleted files is not perfect. Though for the latter case, I feel that it somewhat depends on whether we care about showing a complete history of deletions and readds or not. (In reply to Remy Suen from comment #12) > Histories with two unrelated branches needs to be fixed This has been pushed to master. https://github.com/eclipse/orion.client/commit/765bbd0947ff2f1fa2241b654b501d0265d61c30 > and the handling of > newly added or deleted files is not perfect. Though for the latter case, I > feel that it somewhat depends on whether we care about showing a complete > history of deletions and readds or not. Not going to bother with readds per the discussion on the orion-dev mailing list. If anyone feels otherwise, feel free to speak up. https://dev.eclipse.org/mhonarc/lists/orion-dev/msg04079.html Marking as FIXED. |
Created attachment 266172 [details] Image describing the error in question. orion.eclipse.org 1. Find a file under Git source control. 2. Select the file > Context menu > Open Related > Git Log 3. Click on the 'Toggles the graphical representation of the commit history' tool item. 4. You'll see a disjointed graph that's completely wrong. The problem is caused by the returned JSON object's 'Name' attribute within the 'Parents' array of a commit. --------------- Correct JSON from OrionHub { "commits": [ { "Name": "30b4fd4e9b76a26642230a8a55947782b8599f5f", "Parents": [ { "Name": "d9d47b4bfcb889cc80185f4ded259c2013a8cf82" } ] }, { "Name": "d9d47b4bfcb889cc80185f4ded259c2013a8cf82", "Parents": [ { "Name": "e7391a37b97c6164756fdbe5d50ed8ca1ad9aba2" } ] }, { "Name": "e7391a37b97c6164756fdbe5d50ed8ca1ad9aba2", "Parents": [ { "Name": "6afb58ec373e7a136748fb0b1e287c761c82f798" } ] } ] } --------------- Incorrect JSON from orion.eclipse.org { "commits": [ { "Name": "30b4fd4e9b76a26642230a8a55947782b8599f5f", "Parents": [ { "Name": "f50c8455bf24048b443a0fe4592e6b5f41901aef" } ] }, { "Name": "d9d47b4bfcb889cc80185f4ded259c2013a8cf82", "Parents": [ { "Name": "731fd4beaa46d00dca891d94bca3971f92841b77" } ] }, { "Name": "e7391a37b97c6164756fdbe5d50ed8ca1ad9aba2", "Parents": [ { "Name": "6afb58ec373e7a136748fb0b1e287c761c82f798" } ] } ] } --------------- Note that the JSON returned from OrionHub has an orderly list of parent and child commits. Each parent correctly points to the next commit in the JSON array. On the other hand, the parent in the JSON from orion.eclipse.org seems to be some other completely unknown commit. Of course, the parent returned in the orion.eclipse.org JSON is not actually a random commit, it is pointing at the actual parent commit of said commit. The Java implementation however points at the "parent" commit in the context of the history of the file. This is of course what you would expect and want. I am simply clarifying that the SHA-1 hash provided in the orion.eclipse.org isn't "random" or "wrong", it's just not what we're looking for here.