-
Notifications
You must be signed in to change notification settings - Fork 23
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
Accept long options, and add exclusive as per linux version #8
Accept long options, and add exclusive as per linux version #8
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't built it yet, but looks good! Think we should add tests for the long form?
Also, the real reason I was keeping to short flags was in the hope we could get flock added to posix at some point. But I do appreciate human readable flags for scripting, and being compatible with existing scripts is absolutely worth giving up ideological purity.
@@ -165,7 +197,7 @@ int main(int argc, char *argv[]) { | |||
timer.it_value.tv_sec = (time_t) raw_timeval; | |||
timer.it_value.tv_usec = (suseconds_t) ((raw_timeval - timer.it_value.tv_sec) * 1000000); | |||
break; | |||
case '?': | |||
case 'h': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this do with unexpected flags?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing good I'm afraid. I've reinstated the accidentally removed ? case.
I think adding the long form options to tests (really duplicating the current tests, with different command lines) is the right call. I'll get that sorted today. As to the posix compatibility, I was initially trying to only to -e and -x, but the build system I was working with was trying to use long options as well, so I would up adding the whole lot, so ideological purity was first port of call, until it didn't work :) |
Good call on the tests - I duffed the -w optarg and the handling of -c. Pull request is update to include testing on the long form opts (excluding the --close and -o case, I couldn't think of a good way of testing them either...) |
LGTM, let grab it and make sure the tests pass. I should really throw together a Travis config for this, but it still wouldn't really guarantee cross platform support. |
@amaterasu- shucks, we didn't add the long opts and such to the documentation. I'm going to make sure it gets doc'd in #9. |
Not sure if you think this is valid for merge, but this pull request makes the utility command line compatible with the standard linux flock. Should be no functional change to operation.