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

fix(payload_api, git_diff): Accessing correct property in payload and Replacing --quiet with --no-patch in git diff #1934

Merged
merged 2 commits into from Feb 17, 2024

Conversation

codesculpture
Copy link
Contributor

1. Replacing github.context.payload.head with github.context.payload.pull_request?.head

github.context.payload.head is always undefined and github doc also there is no evidence this exist,
So while trying github.context.payload.pull_request?.head this returns the expected head object and can propogates to fork
and also fork is boolean not a string.
this is found during why the action reached this if block due to avoid git merge-base and seems it could be a accidental mistake

2. Replacing --quiet with --no-patch

As of now, seems every git commands are returning exitcode and if the exit code is none other than 0
we are assuming that operation is not completed successfully which is correct but except
git diff --quiet would return exit code 0 if there are no differences, but returns 1 if there is any differences.
as they mentioned here , --quiet would add
--exit-code.

Make the program exit with codes similar to diff(1). That is, it exits with 1 if there were differences and 0 means no differences.

As they mentioned here.
So seems it not expected, but we have similar flag to run git diff with no outputs, which is --no-patch

These all are found while try to avoid fetching more histories to save time from the eslint-changed-files action and this is a follow up of tj-actions/eslint-changed-files#1633

Copy link
Contributor

@tj-actions-bot tj-actions-bot left a comment

Choose a reason for hiding this comment

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

Thanks for implementing a fix, could you ensure that the test covers your changes if applicable.

@codesculpture
Copy link
Contributor Author

Ran test, some tests failed but all are paths related because i ran in windows, where paths were separated with double slashes \\ which is different from non-windows \ single slash, so is there anything i need to @jackton1 :)

since `payload.head` always been `undefined`
and after correcting property name it returns
`boolean` not a `string`
Because --quiet would make the git diff
exits with 1 when it has differences, still
its not expected. Exit code none other than 0
is considered error.
@jackton1
Copy link
Member

Hi @codesculpture, it looks good. Thanks

@jackton1
Copy link
Member

@all-contributors please add @codesculpture for code and bug

Copy link
Contributor

@jackton1

I've put up a pull request to add @codesculpture! 🎉

@jackton1 jackton1 merged commit e3cac49 into tj-actions:main Feb 17, 2024
66 checks passed
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.

None yet

3 participants