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

Head vs base ahead error #7

Open
rhaynes opened this issue Jun 26, 2020 · 2 comments
Open

Head vs base ahead error #7

rhaynes opened this issue Jun 26, 2020 · 2 comments
Labels
bug Something isn't working

Comments

@rhaynes
Copy link

rhaynes commented Jun 26, 2020

https://github.com/jitterbit/get-changed-files/blob/master/src/main.ts#L79

The base branch that github reports can be anywhere between the initial commit on the base branch where head branched off to the latest commit on the base branch (or any commit in between). This seems to happen most frequently when new commits are pushed after a PR is already open. This is fine for repos that require a rebase or merge on the base branch before merging a PR, but otherwise the line above occasionally throws an error. If this line were changed to core.info instead of core.setFailed all changed files are reported correctly so it's probably unnecessary to consider this failed when that check doesn't pass.

@sean-krail
Copy link
Contributor

Hi @rhaynes, thanks for opening this issue.

The base branch that github reports can be anywhere between the initial commit on the base branch where head branched off to the latest commit on the base branch (or any commit in between). This seems to happen most frequently when new commits are pushed after a PR is already open.

This is true for the base commit's branch ref, but not for the base commit's SHA. The action should be using the base commit's SHA which doesn't change even if the base branch ref gets updated after the PR is already open. Here's where it gets the base commit's SHA:

case 'pull_request':
base = context.payload.pull_request?.base?.sha
head = context.payload.pull_request?.head?.sha
break
case 'push':
base = context.payload.before
head = context.payload.after
break

I may be wrong though. I'll try to find some time over the next couple weeks to reproduce/verify this.

@sean-krail sean-krail added the bug Something isn't working label Jul 1, 2020
@rhaynes
Copy link
Author

rhaynes commented Jul 7, 2020

Hi @sean-krail thanks for following up! I think I've figured out what's going on. You're correct the base sha doesn't change once the PR is open. The problem seems to be that the base sha is assigned when the PR is opened and is whatever sha is currently at for the base branch (as opposed to the commit sha when the branch was created off the base branch). In other words:

  1. Create two branches off the same base commit and make one commit to each branch
  2. PR and merge one of the two branches.
  3. PR the second branch. The script should fail b/c the base sha will be the sha after the first branch was merged, but the head commit on the second branch will be behind that merge commit.

If you rebase on master for the 2nd PR, it's not an issue, but if the merge process for your repo is to merge branches into master without rebasing it throws the error above in the case described.

In any case, the script correctly determines the diff between the base branch and head commit even though the head is behind the base commit so it seems like this error is unnecessary and the script would work as expected without it.

illabo added a commit to illabo/Flipper-iOS-App that referenced this issue Oct 30, 2020
as of jitterbit/get-changed-files#7 and others this is considered a bug by 'get-changed-files' author. On error there are still the correct list of files. In worst case we won't get any files and it isn't 'the end of the world' for us clearly.
yufei-cai added a commit to bosch-io/ditto that referenced this issue Dec 23, 2020
jufickel-b pushed a commit to bosch-io/ditto-clients that referenced this issue Jan 14, 2021
jaredly pushed a commit to jaredly/get-changed-files that referenced this issue Aug 4, 2021
…ted-git-info-2.8.9

chore(deps): bump hosted-git-info from 2.8.8 to 2.8.9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants