Skip to content

Diff variable needs to be unquoted when passed to the application #167

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

Merged

Conversation

internalsystemerror
Copy link
Member

Q A
Documentation no
Bugfix yes
BC Break no
New Feature no
RFC no
QA no

Description

Sometimes when running the action on a pull request, it's unable to correctly perform the regex checks on the list of filenames and incorrectly returns no checks to be performed (see https://github.com/laminas/laminas-filter/actions/runs/3732216426/jobs/6332977468).

This appears to have been introduced by #117 and after various discussion over slack and with @Xerkus help, it is believed that this PR should fix that issue.

@internalsystemerror internalsystemerror added this to the 1.22.1 milestone Dec 19, 2022
@Xerkus Xerkus changed the base branch from 1.23.x to 1.22.x December 19, 2022 16:30
@internalsystemerror internalsystemerror merged commit 5e50256 into laminas:1.22.x Dec 19, 2022
@internalsystemerror internalsystemerror deleted the fix-diff-as-arguments branch December 19, 2022 16:31
@Xerkus Xerkus added the Bug Something isn't working label Dec 19, 2022
/action/main.js "${DIFF}"
# This variable is deliberately unquoted so that it can be properly processed on the other side.
# shellcheck disable=SC2086
/action/main.js ${DIFF}
Copy link
Member

Choose a reason for hiding this comment

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

That is sooooo odd; normal rule of thumb is to ALWAYS quote arguments in shell scripts. Would love to know why this occurs...

Copy link
Member Author

@internalsystemerror internalsystemerror Dec 20, 2022

Choose a reason for hiding this comment

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

Same. I think the list of files gets read as a single string, but I'm not certain. It should be easy enough to see what's happening around

/**
* Remove the first two arguments from this process.
* - first index contains the binary which is used to execute JS (nodejs)
* - second argument contains the filename which is being executed
* After the first two arguments, all remaining arguments should contain filenames from a `diff`. Can be empty in case
* the action is not run from a pull-request.
*/
/* eslint-disable-next-line no-magic-numbers */
const filesWithChanges: string[] = process.argv.slice(2).filter((value) => value !== '');
though.

Copy link
Member

Choose a reason for hiding this comment

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

Each file name from diff is expected to be a separate argument and it explicitly relies on shell word splitting behavior using IFS.
Double quoting prevented word splitting and the whole diff ended up passed as a single argument which then failed to match non-multiline regex like \.php$ as it would effectively be matching only the very last file

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

Successfully merging this pull request may close these issues.

3 participants