Skip to content

Conversation

apstasen
Copy link
Contributor

Fix lint by fetching enough commits (number of commits in pull request plus one) and using GITHUB_SHA that corresponds to sycl HEAD for pull_request_target instead of GITHUB_SHA^1 that corresponds to the first parent (previous state of sycl branch) of merge commit for pull_request used before.

@apstasen apstasen requested a review from a team as a code owner August 27, 2022 00:23
Copy link
Contributor

@aelovikov-intel aelovikov-intel left a comment

Choose a reason for hiding this comment

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

All owners are unavailable, so my LGTM is the most we can get.

The Lint task in this PR pre-commit shows the issue is fixed with this PR applied.

@againull can you please merge this in using your maintainer rights?

@againull againull merged commit 2a3271b into intel:sycl Aug 29, 2022
Comment on lines 29 to +36
steps:
- name: 'PR commits + 1'
run: echo "PR_FETCH_DEPTH=$(( ${{ github.event.pull_request.commits }} + 1 ))" >> "${GITHUB_ENV}"
- uses: actions/checkout@v2
with:
ref: ${{ github.event.pull_request.head.sha }}
persist-credentials: false
fetch-depth: 2
fetch-depth: ${{ env.PR_FETCH_DEPTH }}
Copy link
Contributor

Choose a reason for hiding this comment

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

This approach pulls a lot of unnecessary commits. We need to pull just 2 commits: head of tested branch and merge-base with sycl branch. Before your changes we used pull request merge commit, so using fetch-depth: 2 was reasonable way to pull these two required commits.
Now as we don't use merge commit, we should probably use fetch-depth: 1, but fetch only required refs.

We have pulldowns from the community, which add a lot of commits to the sycl branch.

Copy link
Contributor

Choose a reason for hiding this comment

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

Tagging @apstasen

Copy link
Contributor Author

@apstasen apstasen Sep 7, 2022

Choose a reason for hiding this comment

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

We only fetch commits that belong PR branch plus one parent commit from sycl branch (from the point when it was branched from sycl branch). If we use fetch depth 1, then git tree used in git clang format will not work properly since sycl HEAD and PR HEAD will not have common history. PR_FETCH_DEPTH is minimum possible number of commits we need.

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.

5 participants