Skip to content
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

Use Build.ancestors to find replacement commit for rebased build #566

Merged
merged 12 commits into from
May 24, 2022

Conversation

tmeasday
Copy link
Member

@tmeasday tmeasday commented May 12, 2022

Fixes #375

If one of our baseline builds has a commit that is not in the repository, we cannot figure out the changedFiles via git.

So, instead, we coordinate with the index to find an (nearest) ancestor of that baseline build that does have a commit, and use that commit instead for calculating the changedFiles.

Then we send the replacementCommit information up to the index, and the index will use that information to augment the list of onlyStoryFiles with the stories that have changed in between each replacement commit and the original.

This PR doesn't currently include any UI. @ghengeveld what are your thoughts there? At the least we should log some debug messages, maybe you can guide me there?

This PR depends on various unreleased PRs to Chromatic:

@tmeasday tmeasday requested a review from ghengeveld May 12, 2022 03:03
@tmeasday tmeasday marked this pull request as ready for review May 12, 2022 06:16
Copy link
Member Author

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

Add an info message at the end telling them this happened.

Copy link
Member

@ghengeveld ghengeveld left a comment

Choose a reason for hiding this comment

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

I was wondering whether you had considered this situation:

[A]-[B]- C     [main]
      \
       D -(E)  [feature]

Where B has been rebased and therefore doesn't exist in history anymore. How will we find A as ancestor of E if they're on a different branch? lastBuildOnBranch isn't going to help us.

bin-src/tasks/gitInfo.ts Show resolved Hide resolved
bin-src/git/getParentCommits.ts Outdated Show resolved Hide resolved
bin-src/git/getParentCommits.ts Show resolved Hide resolved
bin-src/git/getParentCommits.ts Outdated Show resolved Hide resolved
bin-src/git/getParentCommits.ts Outdated Show resolved Hide resolved
bin-src/git/getParentCommits.test.ts Show resolved Hide resolved
bin-src/git/getParentCommits.test.ts Show resolved Hide resolved
bin-src/git/getParentCommits.test.ts Outdated Show resolved Hide resolved
bin-src/git/getParentCommits.test.ts Show resolved Hide resolved
.storybook/preview.js Outdated Show resolved Hide resolved
@tmeasday
Copy link
Member Author

Where B has been rebased and therefore doesn't exist in history anymore. How will we find A as ancestor of E if they're on a different branch? lastBuildOnBranch isn't going to help us.

This sounds like a question about baseline calculations? I think in the scenario you outlined, where the "true" ancestor commit was rebased and is on a different branch, we will not find it, and instead we'll use the previous commit that has a build and is a git ancestor. In your picture:

[A]-[B]- C     [main]
      \
       D -(E)  [feature]

If B has been rebased and A has not, then A will be an ancestor of E in a straightforward way. The main problem in this scenario (and it is a problem, although I've never noticed it in practice) would be approvals from B would get lost on E.

PS none of this is an issue for TurboSnap by the way. TS is just going to copy what snapshots it can from the ancestor builds, whether they are the right choices or otherwise. Thanks to this PR it should even handle ancestor builds that don't have a commit too 👍

@tmeasday tmeasday changed the base branch from main to next May 24, 2022 03:09
@tmeasday tmeasday merged commit 28e43fc into next May 24, 2022
@tmeasday tmeasday deleted the tom/ch-1935-use-buildancestors-to-find-replacement branch May 24, 2022 03:09
@tmeasday tmeasday mentioned this pull request May 24, 2022
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.

Look at more than the current commit for squash merges
2 participants