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

find: Allow '/' to be used with -perm for GNU compatibility #1060

Closed
wants to merge 1 commit into from

Conversation

ricardobranco777
Copy link
Contributor

@ricardobranco777 ricardobranco777 commented Jan 14, 2024

Since 2005 GNU find(1) no longer allows '+' to be used with -perm, forcing the use of a / instead.

This PR adds support for this character as synonym for '+'.

Fixes https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=276326

@igalic
Copy link
Contributor

igalic commented Jan 14, 2024

I feel like this would require a documentation update

@ricardobranco777
Copy link
Contributor Author

I feel like this would require a documentation update

Fixed.

@bsdimp
Copy link
Member

bsdimp commented Jan 14, 2024

Can you rebase to a newer main and do a git push --force-with-lease so that the style checker also runs.

To my eyeballs, though, this looks good.

@ricardobranco777
Copy link
Contributor Author

--force-with-lease

Done.

Copy link
Contributor

@igalic igalic left a comment

Choose a reason for hiding this comment

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

one final request
looks fine otherwise

Copy link
Contributor

@igalic igalic Jan 14, 2024

Choose a reason for hiding this comment

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

this file will need a .Dd bump

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Thanks!

igalic

This comment was marked as duplicate.

@qemu-bsd-user
Copy link

Once the .Dd bump happens, I'll commit. It all looks good otherwise.
I also need to look at why my style checker didn't run with the forced push, but that's not the fault of the submitter...

@ricardobranco777
Copy link
Contributor Author

Once the .Dd bump happens, I'll commit. It all looks good otherwise. I also need to look at why my style checker didn't run with the forced push, but that's not the fault of the submitter...

Fixed.

@jlduran
Copy link
Member

jlduran commented Jan 15, 2024

I also need to look at why my style checker didn't run with the forced push

@bsdimp disguised as @qemu-bsd-user, maybe it is missing the synchronize activity type?
Ref: https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request, yes it is the only activity type not in the past tense.

@emaste
Copy link
Member

emaste commented Jan 15, 2024

I find preceded by a plus (“+”) or a slash (“/”) (for GNU compatibility), slightly awkward (although this entire -perm paragraph is already awkward).

I think / is not accepted by find on other BSDs or macOS, so we likely still prefer +? If the intent is to allow / for GNU compatibility but we feel that + is preferred I would probably leave the existing text intact, but add a sentence at the end along the lines of For compatibility with GNU find a slash ("/") is also accepted in place of a plus ("+").

In any case the code change LGTM (and thanks for improving GNU compatibility).

@bsdimp
Copy link
Member

bsdimp commented Jan 15, 2024

I find preceded by a plus (“+”) or a slash (“/”) (for GNU compatibility), slightly awkward (although this entire -perm paragraph is already awkward).

I think / is not accepted by find on other BSDs or macOS, so we likely still prefer +? If the intent is to allow / for GNU compatibility but we feel that + is preferred I would probably leave the existing text intact, but add a sentence at the end along the lines of For compatibility with GNU find a slash ("/") is also accepted in place of a plus ("+").

In any case the code change LGTM (and thanks for improving GNU compatibility).

Checking into things a bit more, POSIX.1 2001 (issue 6) and POSIX.1 2018 (issue 7) has the '+' sign listed, but not the '/' sign, so that's a gnu find extension. Gnu find evidentally deprecated + in favor of / in 2005. I've not checked to see if POSIX.1 202x has this or not. My not-completley-public standards archive is offline, so I can't check.

freebsd-git pushed a commit that referenced this pull request Jan 15, 2024
In 2005, Gnu find deprecated '+' as the leading character for the -perm
argument, instead preferring '/' with the same meaning. Implement that
behavior here, and document it in the man page.

Reviewed by: imp, emaste
Pull Request: #1060
@bsdimp
Copy link
Member

bsdimp commented Jan 15, 2024

Merged. I hope it's OK, but I added a blurb in the STANDARDS section about this extension.

@bsdimp bsdimp closed this Jan 15, 2024
@bsdimp bsdimp added the merged label Jan 15, 2024
bsdjhb pushed a commit to bsdjhb/cheribsd that referenced this pull request Mar 15, 2024
In 2005, Gnu find deprecated '+' as the leading character for the -perm
argument, instead preferring '/' with the same meaning. Implement that
behavior here, and document it in the man page.

Reviewed by: imp, emaste
Pull Request: freebsd/freebsd-src#1060
freebsd-git pushed a commit that referenced this pull request Apr 8, 2024
In 2005, Gnu find deprecated '+' as the leading character for the -perm
argument, instead preferring '/' with the same meaning. Implement that
behavior here, and document it in the man page.

Reviewed by: imp, emaste
Pull Request: #1060

(cherry picked from commit 2a121b9)
@ricardobranco777 ricardobranco777 deleted the find_perm branch April 8, 2024 19:41
freebsd-git pushed a commit that referenced this pull request Apr 8, 2024
In 2005, Gnu find deprecated '+' as the leading character for the -perm
argument, instead preferring '/' with the same meaning. Implement that
behavior here, and document it in the man page.

Reviewed by: imp, emaste
Pull Request: #1060

(cherry picked from commit 2a121b9)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants