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

Added long options to squash command. #151

Merged
merged 2 commits into from
Oct 31, 2015
Merged

Conversation

gsenesac
Copy link
Contributor

First pass at Issue #147. Let me know if you have any feedback and I'll update the PR.

@Dr-Emann
Copy link
Contributor

Every long option program I've ever used has separated multi-word options with a dash, not an underscore.

@nemequ
Copy link
Member

nemequ commented Oct 27, 2015

Thanks for the patch!

@Dr-Emann is right, definitely use dashes not underscores. Also, -f is "force" not "file", and it doesn't take an argument.

Travis spotted some issues; the squash_options argument to both parg_reorder and parg_getopt_long should be the raw array, not a pointer to an array. In other words, you need to remove the '&' from the arguments.

parg_getopt_long works a bit different than parg_getopt, the code as-is won't work. I think the easiest way to fix it would be to pass the address of an int to the last parameter of parg_getopt_long instead of NULL (let's call it long_index), then if opt == 0 replace opt with squash_options[long_index].val. So you would end up with something like

  while ((opt = parg_getopt_long (&ps, optend, argv, "c:ko:123456789LPfdhb:V", squash_options, &long_index)) != -1) {
    if (opt == 0)
      opt = squash_options[long_index].val;

    switch (opt) {
      ...

Finally, when declaring the squash_options array, please use NULL for the pointer field instead of 0. IIRC NULL can technically be defined as something other than 0 (though I doubt any platform does it, and it would break lots of software).

@gsenesac
Copy link
Contributor Author

Fair enough on the dashes vs underscores, I've never realized until now that I too cannot recall anyone using underscores in long options. Yeah that & was a bad copy-paste error from a getopt example. Updated the 0's to NULLs where the arguments are pointers in the option array. Lastly, I updated "file" to be "force" and removed the argument requirement.

However, I'm not following on why parg_getopt_long won't work the way it's coded here (although the Travis CI build failure would indicate otherwise I did actually test this before I sent the PR). On my machine the current code (as originally submitted) builds on my machine (OS X) and parses the long options without a problem. Although I agree one could use the long index parameter, right now parg_getopt_long is just returning the same character for the long option as its corresponding short option so the while loop should work just the same. Just for kicks I tested this by printing out the value of opt:

➜  build git:(long_opts) ✗ ./utils/squash --help
opt = h
Usage: ./utils/squash [OPTION]... INPUT [OUTPUT]
Compress and decompress files.

Options:
        -k, --keep              Keep input file when finished.
        -o, --option key=value  Pass the option to the encoder/decoder.
        -1 .. -9                Pass the compression level to the encoder.
                                Equivalent to -o level=N
        -c, --codec codec       Use the specified codec.  By default squash will
                                attempt to guess it based on the extension.
        -L, --list-codecs       List available codecs and exit
        -P, --list-plugins      List available plugins and exit
        -f, --force              Overwrite the output file if it exists.
        -d, --decompress        Decompress
        -V, --version           Print version number and exit
        -h, --help              Print this help screen and exit.
➜  build git:(long_opts) ✗ ./utils/squash --version
opt = V
squash version 0.8.0 (library version 0.8.0)

@nemequ nemequ merged commit 2319c99 into quixdb:master Oct 31, 2015
@nemequ
Copy link
Member

nemequ commented Oct 31, 2015

However, I'm not following on why parg_getopt_long won't work the way it's coded here

Ah, you're right, sorry. I was misreading the parg documentation; it's only an issue if the flag field in struct parg_option is non-null.

(although the Travis CI build failure would indicate otherwise I did actually test this before I sent the PR).

I have Travis set up to build with -Werror, but that isn't the default for Squash since it can easily cause build errors when people (try to) use a compiler or compiler version we haven't tested with.

Anyways, thanks for the patches, they're greatly appreciated.

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.

None yet

3 participants