Skip to content

pidof: Support '-t' flag#318

Merged
Krysztal112233 merged 1 commit intouutils:mainfrom
dezgeg:pidof_t_flag
Feb 18, 2025
Merged

pidof: Support '-t' flag#318
Krysztal112233 merged 1 commit intouutils:mainfrom
dezgeg:pidof_t_flag

Conversation

@dezgeg
Copy link
Copy Markdown
Contributor

@dezgeg dezgeg commented Feb 6, 2025

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

@sylvestre
Copy link
Copy Markdown
Contributor

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

@dezgeg
Copy link
Copy Markdown
Contributor Author

dezgeg commented Feb 7, 2025

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

Added

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@codecov
Copy link
Copy Markdown

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