Skip to content

More caching of merge bases #1112

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

More caching of merge bases #1112

wants to merge 2 commits into from

Conversation

DanielRose
Copy link
Contributor

Merge bases are symmetric, i.e. the merge base of 'branchA' and 'branchB' is the same as 'branchB' and 'branchA'.
Consider a local and its remote (tracked) branch the same, with the state of the local branch preferred.

/// </summary>
public Commit FindMergeBase(Branch branch, Branch otherBranch)
{
var key = Tuple.Create(branch, otherBranch);
if (branch.IsSameBranch(otherBranch))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should ensure the tips are also pointing at the same place.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JakeGinnivan Actually here it might not be the case.

Consider a local and its remote (tracked) branch the same, with the state of the local branch preferred.

Normally for the merge base it will make no difference if the local or remote branch is not up to date. That is only the case if the local and/or the remote branch was rebased. With this change (actually the algorithm in IsSameBranch()), the state (and thus merge base) of the local branch is always chosen.

DanielRose and others added 2 commits January 25, 2017 11:25
Merge bases are symmetric, i.e. the merge base of 'branchA' and 'branchB' is the same as 'branchB' and 'branchA'.
Consider a local and its remote (tracked) branch the same, with the state of the local branch preferred.
@JakeGinnivan
Copy link
Contributor

After trying to rebase this I have a failing test and thought I would check to see if you can diagnose easier than I can. I am out of time for my mornings catch up session @DanielRose

@DanielRose
Copy link
Contributor Author

@JakeGinnivan I was (and am) pretty busy the last few weeks. I'll take a closer look at this as soon as I can.

@JakeGinnivan
Copy link
Contributor

Yeah no worries @DanielRose, understand that too well.

@JakeGinnivan
Copy link
Contributor

I know this is a while back now, should we close this or did you want to take another run at it?

@JakeGinnivan
Copy link
Contributor

@DanielRose

@JakeGinnivan
Copy link
Contributor

I am having a play with a completely new approach at #1243, I have high hopes its a better way forward than internal caching. Going to close this one down as it will likely have to be redone manually due to too much drift. Sorry about that. In fact I am now using the sha for the cache keys instead of branches so that may actually have the same effects as this PR anyways.

@DanielRose
Copy link
Contributor Author

@JakeGinnivan I was on a long vacation with limited Internet access. Once I find some free time I can take a look at your PR and see if I find anything :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants