flock: add more tests and a new option '--fd' - #4553

Maybe a little bit improve the PR's title ;-) The PR is definitely not only about tests.

Forgot to adapt the man page and usage info, I'll do it tomorrow morning.

Review notes (by AI)

Nice work on the --fd option and test coverage. I traced every flock invocation form through the new argument parsing and found a few issues — the most important one is a regression in the filename + -c path.

Critical: missing -c argument validation in filename branch

The old code validated that exactly one argument follows -c:

if (argc != optind + 3)
    errx(EX_USAGE, _("%s requires exactly one command argument"), argv[optind + 1]);

The new else if (argc > optind + 1 && !use_fd) branch calls build_shell_cmd_argv(sh_c_argv, argv[optind + 2]) when IS_COMMAND_OPT(argv[optind + 1]) is true, but never checks that argv[optind + 2] exists. Two regressions:

• flock file -c (no command after -c): passes NULL to build_shell_cmd_argv → execvp("sh", ["sh", "-c", NULL]) → undefined behavior / crash
• flock file -c cmd1 cmd2: silently ignores cmd2 instead of erroring as before

Note that the cmd_opt_given path (for --fd ... -c cmd) correctly validates with argc != optind + 1 — the same validation is just missing from the filename path.

opterr = 0 suppresses all unknown-option errors

Setting opterr = 0 suppresses getopt error messages for all unrecognized options, not just -c/--command. With the old code, flock --bogus file cmd printed flock: unrecognized option '--bogus' then "Try --help". With the new code the user only gets "Try --help" with no indication of which option was bad.

Consider keeping opterr = 1 and checking IS_COMMAND_OPT in the default: handler before falling through to errtryhelp — getopt will have already printed its diagnostic for genuinely bad options.

Minor issues

• errx(EXIT_FAILURE, "missing file descriptor, file or directory") — missing _() i18n wrapper and uses EXIT_FAILURE (1) instead of EX_USAGE (64) as the old code did
• Man page: *--fd*:: should be *--fd* _number_:: for consistency with *--length* _number_:: etc.
• Double blank line after the if (fd < 0) block
• Indentation inside the default: case body uses 5 tabs where surrounding code uses 3

— assisted by Claude Code

I agree with all points except for the opterr = 1. Given that the following syntax is now permitted flock --fd 10 --command 'echo hey' getopt will successfully parse all options until it hits --command, where it prints the error message first before evaluating our fallthrough logic in default:. Therefore, inaccurately describing a valid usage as erroneous.

I tested one workaround where we handle the error message output ourselves with something like this:

default:
			/* When getopt(3) detects an unknown option it increments
			 * optind by 1, conversely, it does not increment optind
			 * when it encounters a non-option argument. This holds
			 * true only because our @shortopts starts with a '+'.
			 */
			if (IS_COMMAND_OPT(argv[optind-1])) {
				cmd_opt_given = true;
				break;
			}
			printf(_("%s: unrecognized option '%s'\n"),
				program_invocation_short_name, argv[optind - 1]);
			errtryhelp(EX_USAGE);

It might look ugly but TBH we have little room left given that the initial usage design requires the --command option to be towards the end of the command and after the filename/directory :(

Another idea was to add --command to longopts and let getopt permutate argv so we handle the arguments after parsing depending on what we have seen (--fd) and how many arguments are left. But that turns out to be bad since we can do things like flock --fd 10 bash -c ... where we'd evaluate the actual wrapped command's option, in this example bash's '-c' arg.