Fixed critical bug in merge parent changeset lookup #1195

Laibalion · 2018-05-25T07:48:03Z

Fixed critical bug in merge parent changeset lookup.
It seems like if you have multiple branches developed and merges of those intertwined, the lookup for the parent changeset for that merge was really, but really really, unstable.
Often we ended up with merges seemingly coming from the wrong branch because the lookup gave a wrong parent changeset.
Now, looking for the parent changeset is done by examining the actual mergesources of the merge itself.
When the merge changeset contains merges from more than 1 branch, this may still lead to missing merge history, but that's not solvable in git, as it just doesn't support such scenarios. If this scenario occurs, the branch with the highest mergesource version will be picked (in the old implementation this also failed)

improved usability by - exposing ignore-not-init-branches to commandline - added ignore-branches-regex to more granularly control which branches to initialize during fetching and cloning

Updated NEXT.md file with changes

Laibalion · 2018-05-25T13:39:16Z

Added small usability improvement to easily delete all tfs remotes

pmiossec · 2018-05-25T14:15:21Z

Could you create a pull request for the last commit because it don't seems linked to the bug solved....

Laibalion · 2018-05-25T14:30:54Z

I’ve removed the last commit and will create a separate pull request for it. My apologies.

…

________________________________ From: Philippe Miossec <notifications@github.com> Sent: Friday, May 25, 2018 4:15:24 PM To: git-tfs/git-tfs Cc: Geert Bleyen; Author Subject: Re: [git-tfs/git-tfs] Fixed critical bug in merge parent changeset lookup (#1195) Could you create a pull request for the last commit because it don't seems linked to the bug solved.... — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<https://nam01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fgit-tfs%2Fgit-tfs%2Fpull%2F1195%23issuecomment-392071577&data=02%7C01%7C%7C900335f09bf94c1db37308d5c249f58a%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636628545260098505&sdata=Aau53IrZmnEyDpanIYcGxkHJLhEmiXDX%2FjlPr8A1ihI%3D&reserved=0>, or mute the thread<https://nam01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAb8XAO5OIpZCiaQ6GzZfyohUHBb4kVDOks5t2BH8gaJpZM4UNhK9&data=02%7C01%7C%7C900335f09bf94c1db37308d5c249f58a%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636628545260098505&sdata=SVGxQmprpWhY82LoTjL%2FwL%2FrD9frV3CSpSTK2gitmiw%3D&reserved=0>.

Laibalion · 2018-05-28T20:33:56Z

@pmiossec , do you need more info on my changes to get them accepted?

I'm happy to elaborate on them.

pmiossec · 2018-05-28T21:56:53Z

Because I don't have a lot of time, I only review when the PR is nearly finished.

And I really need to find time to review it. Sorry for that!

This PR touch à piece of code which is very sensitive where a lot of people did a fix that solve it's problem but break for the others and we never found the perfect solution.

And I'm quite nervous since we lost our smoke tests that was using a repository on codeplex.

I hope you are improving the situation ;-)

I'll have a look...

pmiossec · 2018-05-28T21:57:59Z

I'm happy to elaborate on them.

Every information that could help understand is welcomed ;-)

Laibalion · 2018-05-29T04:59:07Z

Alright I'll try to explain what happen in the old and the fixed version.
Assume you have your trunk and few branches with some merges:

When processing changeset 11
According to my understanding the old code would incorrectly identify changeset 9 as the source changeset because it's source version is higher than Branch A's changeset 5. The call to VersionControlServerQueryMerges returns any merge up to a point, and next the code would try to take the highest source version of any of these merges.
This caused changeset 11 to have to correct changes, but git merge history will show it as coming from branch B instead of A. Further down the line, it caused further issues as it would not even have created the remote for branch A and it's history getting lost entirely.

In the fixed version it will actually inspect the merged changes in the changeset, and retrieve the highest sourceversion of a merge change in that changeset, correctly identifying changeset 5 as the parent.

I hope this make things a bit clearer :)

Laibalion · 2018-05-29T07:43:44Z

BTW, when is it planned to have a new release? For now I have to point me colleagues to my repository and let them build git-tfs themselves, and I know some of them would feel more comfortable when it's 'real' release :P

pmiossec · 2018-05-29T08:16:27Z

I hope this make things a bit clearer :)

That's clearer ;) I need to review now ;)

BTW, when is it planned to have a new release? For now I have to point me colleagues to my repository and let them build git-tfs themselves, and I know some of them would feel more comfortable when it's 'real' release :P

that could be done ;)
but they also could take the packages built by AppVeyor. There is a link on the repo main page...

Now, looking for the parent changeset is done by examining the actual mergesources of the merge itself. When the merge changeset contains merges from more than 1 branch, this may still lead to missing merge history, but that's not solvable in git, as it just doesn't support such scenarios. If this scenario occurs, the branch with the highest mergesource version will be picked (in the old implementation this also failed) See git-tfs#1195

Now, looking for the parent changeset is done by examining the actual mergesources of the merge itself. When the merge changeset contains merges from more than 1 branch, this may still lead to missing merge history, but that's not solvable in git, as it just doesn't support such scenarios. If this scenario occurs, the branch with the highest mergesource version will be picked (in the old implementation this also failed) See #1195

pmiossec · 2018-05-29T20:34:30Z

Thanks! Merged through #1198 ....

pmiossec · 2018-05-29T21:45:43Z

BTW, when is it planned to have a new release?

Wishes fulfilled 😄

Laibalion · 2018-05-30T06:44:44Z

Awesome, highly appreciated :-)

ericnewton76 · 2018-05-30T17:48:07Z

Hmmm I'm wondering if I was seeing the symptoms of this error. Thanks for fixing that, I'm going to try running it locally as well.

Laibalion · 2018-05-30T19:39:03Z

Yeah, i can almost not believe anyone else wouldn't have had the same problem. I noticed it only by coincidence that my history was coming from the wrong branch during some of my test runs.
Once i noticed it, it still cost a week of debugging to found the rootcause (my logfile of a testrun was about 400MB). Once that was found, fixing it was not so difficult.

Since we are planning a corporate migration i really needed to have it fixed 😁

jpsimard-nyx · 2018-09-21T13:43:30Z

Unfortunatly this fix seems to breaks the migration of our TFS repo. Also tried GitTfs-0.29.27-81196a92.pull_1214_merge with same result. 0.28 works wonderfully.
We receive way more warning: this changeset XXXXX is a merge changeset. But git-tfs is unable to determine the parent changeset. and in the log something that did not occur with 0.28 Looking for changeset XXXXX in git repository: CacheIsFull, stopped looking.
I've look in the source and found it strange that cacheIsFull flag is set at the end of FindCommitByChangesetId but tested and the beginning of the next one.

git-tfs/src/GitTfs/Core/GitRepository.cs

Lines 679 to 680 in 57a37c2

    
           if (remoteRef == null && commit == null) 
        
               cacheIsFull = true; // repository fully scanned

Anyway that comment seems to be right on spot

This PR touch à piece of code which is very sensitive where a lot of people did a fix that solve it's problem but break for the others and we never found the perfect solution.

About the tests I was wondering, since Azure DevOps is now free for opensource project if you could make you tests with the TFS in Azure DevOps repo.

And I'm quite nervous since we lost our smoke tests that was using a repository on codeplex.

Laibalion · 2018-09-21T13:59:41Z

Hi @LucianJp, that piece of code you highlight is not part of the changes I made.
Fact remains, the previous implementation was proven to be logically wrong (see my diagram earlier in this thread). Perhaps it worked 'wonderfully wrong' in the previous versions, as it would take merges from the wrong branch in some cases.

What broke in your migration? Are talking about a clean migration, or incremental between TFS<->git?
Without proper explanation of what issues you encounter and which commandline arguments you are using, we can't really know what's going on in your case.

geertbleyen added 6 commits May 18, 2018 15:29

fixed bug in external repository detection

f1d57d3

improved usability by - exposing ignore-not-init-branches to commandline - added ignore-branches-regex to more granularly control which branches to initialize during fetching and cloning

Updated NEXT.md file

406aa3b

Updated NEXT.md file with changes

fixed issue with null regex string

366056a

Added some more logging to changeset lookup logic

6be5c12

Merge branch 'master' of https://github.com/git-tfs/git-tfs

54651fc

fixed critical bug in merge parent changeset lookup

8327713

Laibalion force-pushed the master branch 2 times, most recently from 87fcd50 to 8327713 Compare May 25, 2018 14:29

Laibalion force-pushed the master branch from b171f62 to 8327713 Compare May 25, 2018 14:44

pmiossec mentioned this pull request May 29, 2018

Fix critical bug in merge parent changeset lookup #1198

Merged

pmiossec closed this May 29, 2018

Fixed critical bug in merge parent changeset lookup #1195

Fixed critical bug in merge parent changeset lookup #1195

Uh oh!

Conversation

Laibalion commented May 25, 2018

Uh oh!

Laibalion commented May 25, 2018

Uh oh!

pmiossec commented May 25, 2018

Uh oh!

Laibalion commented May 25, 2018 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Laibalion commented May 28, 2018

Uh oh!

pmiossec commented May 28, 2018

Uh oh!

pmiossec commented May 28, 2018

Uh oh!

Laibalion commented May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Laibalion commented May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pmiossec commented May 29, 2018

Uh oh!

pmiossec commented May 29, 2018

Uh oh!

pmiossec commented May 29, 2018

Uh oh!

Laibalion commented May 30, 2018

Uh oh!

ericnewton76 commented May 30, 2018

Uh oh!

Laibalion commented May 30, 2018

Uh oh!

jpsimard-nyx commented Sep 21, 2018

Uh oh!

Laibalion commented Sep 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Laibalion commented May 25, 2018 via email •

edited

Loading

Laibalion commented May 29, 2018 •

edited

Loading

Laibalion commented May 29, 2018 •

edited

Loading

Laibalion commented Sep 21, 2018 •

edited

Loading