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

pidof: Support '-t' flag #318

Merged
merged 1 commit into from
Feb 18, 2025
Merged

pidof: Support '-t' flag #318

merged 1 commit into from
Feb 18, 2025

Conversation

dezgeg
Copy link
Contributor

@dezgeg dezgeg commented Feb 6, 2025

This flag makes pidof print thread ids of threads belonging to the matching processes.

@sylvestre
Copy link
Contributor

Could you please add a test to make sure we don't regress? Thanks

@dezgeg
Copy link
Contributor Author

dezgeg commented Feb 7, 2025

Could you please add a test to make sure we don't regress? Thanks

Added

Copy link
Contributor

@cakebaker cakebaker left a comment

Choose a reason for hiding this comment

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

Looks good.

I noticed a small difference to the original pidof, though: it lists the IDs in a descending order whereas we use an ascending order:

$ pidof konsole -t
26231 26230 26229 26228 1365 1364 1363 1362 1361
$ cargo run -q  pidof konsole -t
1361 1362 1363 1364 1365 26228 26229 26230 26231

Without the -t flag, both use a descending order.

This flag makes pidof print thread ids of threads belonging to the
matching processes.
@dezgeg
Copy link
Contributor Author

dezgeg commented Feb 17, 2025

Looks good.

I noticed a small difference to the original pidof, though: it lists the IDs in a descending order whereas we use an ascending order:

$ pidof konsole -t
26231 26230 26229 26228 1365 1364 1363 1362 1361
$ cargo run -q  pidof konsole -t
1361 1362 1363 1364 1365 26228 26229 26230 26231

Without the -t flag, both use a descending order.

Good catch, also found out that -s combined with -t wasn't working right. Both should work now.

Copy link
Collaborator

@Krysztal112233 Krysztal112233 left a comment

Choose a reason for hiding this comment

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

Great work :)

@Krysztal112233 Krysztal112233 merged commit 22c25b0 into uutils:main Feb 18, 2025
14 checks passed
Copy link

codecov bot commented Feb 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (ba00e9c) to head (967d288).
Report is 11 commits behind head on main.

Additional details and impacted files
@@     Coverage Diff     @@
##   main   #318   +/-   ##
===========================
===========================

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

4 participants