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

Add -o/--owner to filter files by owner and/or group #581

Merged
merged 8 commits into from
May 18, 2020

Conversation

alexmaco
Copy link
Contributor

@alexmaco alexmaco commented May 1, 2020

Closes #307

This implements a single constraint.
Implementation is unix-specific for now. It can easily be extended to cover redox, but I think it needs some testing first: the users crate makes no mention of redox support, and there is also redox_users

Possible questions:

  • should this PR also add support for multiple --owner arguments (like with time_constraints)
  • help text refinements

Edit: it seems redox uses the unix target_family as of rust-lang/rust#60139, so no special handling is needed.

src/app.rs Outdated
.long_help(
"Filter files by their user and/or group. \
Format: [(user|uid)][:(group|gid)]. Either side is optional. \
Precede either side with a ! to exclude files instead.",
Copy link
Owner

@sharkdp sharkdp May 12, 2020

Choose a reason for hiding this comment

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

Minor: …either side with a '!' to exclude…

Specifying the full format is great, but a few examples would make things even easier to understand, I think. Maybe:

Examples:
  --owner john
  --owner :students
  --owner '!john:students'

@sharkdp
Copy link
Owner

sharkdp commented May 12, 2020

Thank you very much for this contribution! The code looks great.

should this PR also add support for multiple --owner arguments (like with time_constraints)

It would be great, but we can also add this at a later stage. Multiple --owner arguments would act in an OR-like fashion, right? Similar to --extension. Maybe we should think about this in a bit more detail. Combining this with negation could lead to surprising results(?).

A few questions / comments:

  • Did you do some research into other crates that would work in an OS-independent way? I am not familiar with how file ownership works on Windows, but they certainly have the concept of owners (https://docs.microsoft.com/en-us/windows/win32/secauthz/finding-the-owner-of-a-file-object-in-c--)
  • I guess it would be hard to write actual integration tests for this?
  • For some reason, the AppVeyor tests did not run for this PR and I can not manually trigger a build. Travis CI tests also didn't run for the last commit. Could you maybe rebase this PR to see if that triggers a build?
  • Could you please add an entry to the "unreleased" section in the CHANGELOG.md under "Features"? The format for entries is:
    - Description what has been changed, see #123 (@user)
    
    where #123 links to the bug ticket and/or this PR and user is your username.
  • It would be great if you could also update the man page. Otherwise, I can also sync it with the --help text before the next release.

@alexmaco
Copy link
Contributor Author

Thank you very much for this contribution! The code looks great.
You're welcome!

should this PR also add support for multiple --owner arguments (like with time_constraints)

It would be great, but we can also add this at a later stage. Multiple --owner arguments would act in an OR-like fashion, right? Similar to --extension. Maybe we should think about this in a bit more detail. Combining this with negation could lead to surprising results(?).

Yes, it should work by OR-ing, and negations should work by AND-ing, which i think would make sense when seen in examples, but may not be intuitive to everyone.
I'm waiting for any clear ideas on this topic.

* Did you do some research into other crates that would work in an OS-independent way? I am not familiar with how file ownership works on Windows, but they certainly have the concept of owners (https://docs.microsoft.com/en-us/windows/win32/secauthz/finding-the-owner-of-a-file-object-in-c--)

Unfortunately no, I didn't dig very far, since I can only properly try it out on linux systems. Finding a cross-platform way to do this would be interesting however. I imagine that it could also account for more interesting setups, such as LDAP/ActiveDirectory/whatever-the-equivalent-on-OSX is. But I'm not familiar enough with windows to propose solutions that look idiomatic on the platform.

* I guess it would be hard to write actual integration tests for this?

Integration tests would certainly be interesting. I guess setting up a docker image with known users/groups and IDs would be sufficient. It looks like travis can speak docker.

* For some reason, the AppVeyor tests did not run for this PR and I can not manually trigger a build. Travis CI tests also didn't run for the last commit. Could you maybe rebase this PR to see if that triggers a build?

Sure

* Could you please add an entry to the "unreleased" section in the `CHANGELOG.md` under "Features"? 
...
* It would be great if you could also update the man page. 

Ok, I did not realize more docs existed 😄. I will update them.

Copy link
Owner

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

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

Thank you very much for the updates!

@sharkdp
Copy link
Owner

sharkdp commented May 18, 2020

Integration tests would certainly be interesting. I guess setting up a docker image with known users/groups and IDs would be sufficient. It looks like travis can speak docker.

That sounds rather involved. Let's try without an integration test for now.

@sharkdp sharkdp merged commit a9dc45e into sharkdp:master May 18, 2020
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.

Finding by owner username/group/uid/gid
2 participants