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

Selective cperl core cherries 20210408 #156

Merged

Conversation

jkeenan
Copy link
Collaborator

@jkeenan jkeenan commented Apr 9, 2021

The purpose of this pull request is to pick some low-hanging fruit from a larger p.r. submitted in 2019 by @rurban.

3 of the commits are straightforward cherry-picks; 2 required slight corrections or adaptations.

I hope to work through the balance of the 44 commits and incorporate them into thematically grouped pull requests.

Thank you very much.
Jim Keenan

rurban and others added 5 commits April 9, 2021 18:55
Closes Coverity CID #165315

Signed-off-by: James E Keenan <[email protected]>
Said to be needed by Win32 cmd.exe

Adapted from timbunce@485d6e2, with one correction.
for all Currently a developer-only test checks

Signed-off-by: James E Keenan <[email protected]>
Derived from rurban/75133d3fd0eaf9a8a681d8856ca5fb2438efbfe2.
@jkeenan jkeenan requested a review from timbunce April 9, 2021 20:13
Copy link

@oodler577 oodler577 left a comment

Choose a reason for hiding this comment

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

Clearly okay, but all things considered; is there a benefit to adding the ./? Really just asking for my own information.

Copy link

@oodler577 oodler577 left a comment

Choose a reason for hiding this comment

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

I had to look up File::Spec, and indeed the intent is for this to be cross platform (and is consistent with the intent implied in the commit message).

Copy link

@oodler577 oodler577 left a comment

Choose a reason for hiding this comment

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

Is this just fixing an accidentally redundant check?

NYTProf.xs Show resolved Hide resolved
@jkeenan
Copy link
Collaborator Author

jkeenan commented Apr 9, 2021

Clearly okay, but all things considered; is there a benefit to adding the ./? Really just asking for my own information.

I tried it with a single ./. The test failed. Reini had it right.

@rurban
Copy link
Contributor

rurban commented Apr 10, 2021

Clearly okay, but all things considered; is there a benefit to adding the ./? Really just asking for my own information.

I tried it with a single ./. The test failed. Reini had it right.

Which './'? I normally added ./ to skip @INC searches, which are still security relevant in older perls. But I didn't see any ./ in these commits

Copy link
Owner

@timbunce timbunce left a comment

Choose a reason for hiding this comment

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

Thanks.

NYTProf.xs Show resolved Hide resolved
@jkeenan jkeenan merged commit 65df374 into timbunce:master Apr 10, 2021
@jkeenan jkeenan deleted the selective-cperl-core-cherries-20210408 branch April 10, 2021 16:32
@@ -9,7 +9,7 @@ eval "use autodie; 1"

print "autodie $autodie::VERSION $INC{'autodie.pm'}\n";

plan skip_all => "Currently a developer-only test" unless -d '.svn';
plan skip_all => "Currently a developer-only test" unless -d '../.git';
Copy link
Contributor

Choose a reason for hiding this comment

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

this doesn't seem right. wouldn't .git be in the current working directory? in which case we'd remove ../

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.

6 participants