pkg: clarify argument parsing

Currently pkg parses all arguments and subcommands in a single
getopt_long() loop. This results in quite a bit of accidental
complexity, especially around the -r option for the bootstrap and add
subcommands.

This also results in the undocumented behavior of all options understood
by pkg(7) being accepted anywhere in the argument list regardless of
their position. This contradicts the behavior of pkg(8) and I find it
quite unexpected.

This patch splits the argument parsing done by pkg(7) into two phases
for global options and subcommand options. This gives behavior that
matches pkg(8), accepting the subset of pkg(8) options supported by
pkg(7) in the exact same positions that pkg(8) accepts them.

This is however a somewhat breaking change, as options are no longer
accepted by pkg(7) in places they are rejected by pkg(8). For example,
`pkg -f bootstrap` no longer works, only `pkg bootstrap -f`.

The original motivation for writing this patch was investigating support
for --rootdir in pkg(7). Since pkg(8) uses -r as the short form for
both --rootdir and --repository depending on the position, I decided
that pkg(7) needed to be brought inline with pkg(8)'s argument parsing
behavior to avoid confusion.

This patch also gets rid of args_add_message (unused since 40b9f92)

Relnotes:	yes
Reviewed by:	bapt
Sponsored by:	The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D49977

Author: ifreund

usr.sbin/pkg/pkg.7

@@ -22,7 +22,7 @@
 .\" OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
 .\" SUCH DAMAGE.
 .\"
-.Dd March 30, 2025
+.Dd April 29, 2025
 .Dt PKG 7
 .Os
 .Sh NAME
@@ -33,8 +33,9 @@
 .Op Fl d
 .Ar command ...
 .Nm
+.Op Fl d
 .Cm add
-.Op Fl dfy
+.Op Fl fy
 .Op Fl r Ar reponame
 .Ar pkg.pkg
 .Nm

usr.sbin/pkg/pkg.c

@@ -942,10 +942,6 @@ static const char args_bootstrap_message[] =
 "Too many arguments\n"
 "Usage: pkg [-4|-6] bootstrap [-f] [-y]\n";
 
-static const char args_add_message[] =
-"Too many arguments\n"
-"Usage: pkg add [-f] [-y] {pkg.pkg}\n";
-
 static int
 pkg_query_yes_no(void)
 {
@@ -1072,137 +1068,138 @@ int
 main(int argc, char *argv[])
 {
 	char pkgpath[MAXPATHLEN];
+	char **original_argv;
 	const char *pkgarg, *repo_name;
 	bool activation_test, add_pkg, bootstrap_only, force, yes;
 	signed char ch;
 	const char *fetchOpts;
-	char *command;
 	struct repositories *repositories;
 
 	activation_test = false;
 	add_pkg = false;
 	bootstrap_only = false;
-	command = NULL;
 	fetchOpts = "";
 	force = false;
+	original_argv = argv;
 	pkgarg = NULL;
 	repo_name = NULL;
 	yes = false;
 
 	struct option longopts[] = {
 		{ "debug",		no_argument,		NULL,	'd' },
-		{ "force",		no_argument,		NULL,	'f' },
 		{ "only-ipv4",		no_argument,		NULL,	'4' },
 		{ "only-ipv6",		no_argument,		NULL,	'6' },
-		{ "yes",		no_argument,		NULL,	'y' },
 		{ NULL,			0,			NULL,	0   },
 	};
 
 	snprintf(pkgpath, MAXPATHLEN, "%s/sbin/pkg", getlocalbase());
 
-	while ((ch = getopt_long(argc, argv, "-:dfr::yN46", longopts, NULL)) != -1) {
+	while ((ch = getopt_long(argc, argv, "+:dN46", longopts, NULL)) != -1) {
 		switch (ch) {
 		case 'd':
 			debug++;
 			break;
-		case 'f':
-			force = true;
-			break;
 		case 'N':
 			activation_test = true;
 			break;
-		case 'y':
-			yes = true;
-			break;
 		case '4':
 			fetchOpts = "4";
 			break;
 		case '6':
 			fetchOpts = "6";
 			break;
-		case 'r':
-			/*
-			 * The repository can only be specified for an explicit
-			 * bootstrap request at this time, so that we don't
-			 * confuse the user if they're trying to use a verb that
-			 * has some other conflicting meaning but we need to
-			 * bootstrap.
-			 *
-			 * For that reason, we specify that -r has an optional
-			 * argument above and process the next index ourselves.
-			 * This is mostly significant because getopt(3) will
-			 * otherwise eat the next argument, which could be
-			 * something we need to try and make sense of.
-			 *
-			 * At worst this gets us false positives that we ignore
-			 * in other contexts, and we have to do a little fudging
-			 * in order to support separating -r from the reponame
-			 * with a space since it's not actually optional in
-			 * the bootstrap/add sense.
-			 */
-			if (add_pkg || bootstrap_only) {
-				if (optarg != NULL) {
-					repo_name = optarg;
-				} else if (optind < argc) {
-					repo_name = argv[optind];
-				}
+		default:
+			break;
+		}
+	}
+	if (debug > 1)
+		fetchDebug = 1;
 
-				if (repo_name == NULL || *repo_name == '\0') {
-					fprintf(stderr,
-					    "Must specify a repository with -r!\n");
-					exit(EXIT_FAILURE);
-				}
+	argc -= optind;
+	argv += optind;
+
+	if (argc >= 1) {
+		if (strcmp(argv[0], "bootstrap") == 0) {
+			bootstrap_only = true;
+		} else if (strcmp(argv[0], "add") == 0) {
+			add_pkg = true;
+		}
 
-				if (optarg == NULL) {
-					/* Advance past repo name. */
-					optreset = 1;
-					optind++;
+		optreset = 1;
+		optind = 1;
+		if (bootstrap_only || add_pkg) {
+			struct option sub_longopts[] = {
+				{ "force",	no_argument,	NULL,	'f' },
+				{ "yes",	no_argument,	NULL,	'y' },
+				{ NULL,		0,		NULL,	0   },
+			};
+			while ((ch = getopt_long(argc, argv, "+:fr:y",
+			    sub_longopts, NULL)) != -1) {
+				switch (ch) {
+				case 'f':
+					force = true;
+					break;
+				case 'r':
+					repo_name = optarg;
+					break;
+				case 'y':
+					yes = true;
+					break;
+				case ':':
+					fprintf(stderr, "Option -%c requires an argument\n", optopt);
+					exit(EXIT_FAILURE);
+					break;
+				default:
+					break;
 				}
 			}
-			break;
-		case 1:
-			// Non-option arguments, first one is the command
-			if (command == NULL) {
-				command = argv[optind-1];
-				if (strcmp(command, "add") == 0) {
-					add_pkg = true;
-				}
-				else if (strcmp(command, "bootstrap") == 0) {
-					bootstrap_only = true;
+		} else {
+			/*
+			 * Parse -y and --yes regardless of the pkg subcommand
+			 * specified. This is necessary to make, for example,
+			 * `pkg install -y foobar` work as expected when pkg is
+			 * not yet bootstrapped.
+			 */
+			struct option sub_longopts[] = {
+				{ "yes",	no_argument,	NULL,	'y' },
+				{ NULL,		0,		NULL,	0   },
+			};
+			while ((ch = getopt_long(argc, argv, "+y",
+			    sub_longopts, NULL)) != -1) {
+				switch (ch) {
+				case 'y':
+					yes = true;
+					break;
+				default:
+					break;
 				}
 			}
-			// bootstrap doesn't accept other arguments
-			else if (bootstrap_only) {
-				fprintf(stderr, args_bootstrap_message);
+
+		}
+		argc -= optind;
+		argv += optind;
+
+		if (bootstrap_only && argc > 0) {
+			fprintf(stderr, args_bootstrap_message);
+			exit(EXIT_FAILURE);
+		}
+
+		if (add_pkg) {
+			if (argc < 1) {
+				fprintf(stderr, "Path to pkg.pkg required\n");
 				exit(EXIT_FAILURE);
-			}
-			else if (add_pkg && pkgarg != NULL) {
+			} else if (argc == 1 && pkg_is_pkg_pkg(argv[0])) {
+				pkgarg = argv[0];
+			} else {
 				/*
-				 * Additional arguments also means it's not a
-				 * local bootstrap request.
+				 * If the target package is not pkg.pkg
+				 * or there is more than one target package,
+				 * this is not a local bootstrap request.
 				 */
 				add_pkg = false;
 			}
-			else if (add_pkg) {
-				/*
-				 * If it's not a request for pkg or pkg-devel,
-				 * then we must assume they were trying to
-				 * install some other local package and we
-				 * should try to bootstrap from the repo.
-				 */
-				if (!pkg_is_pkg_pkg(argv[optind-1])) {
-					add_pkg = false;
-				} else {
-					pkgarg = argv[optind-1];
-				}
-			}
-			break;
-		default:
-			break;
 		}
 	}
-	if (debug > 1)
-		fetchDebug = 1;
 
 	if ((bootstrap_only && force) || access(pkgpath, X_OK) == -1) {
 		struct repository *repo;
@@ -1218,10 +1215,7 @@ main(int argc, char *argv[])
 		config_init(repo_name);
 
 		if (add_pkg) {
-			if (pkgarg == NULL) {
-				fprintf(stderr, "Path to pkg.pkg required\n");
-				exit(EXIT_FAILURE);
-			}
+			assert(pkgarg != NULL);
 			if (access(pkgarg, R_OK) == -1) {
 				fprintf(stderr, "No such file: %s\n", pkgarg);
 				exit(EXIT_FAILURE);
@@ -1263,7 +1257,7 @@ main(int argc, char *argv[])
 		exit(EXIT_SUCCESS);
 	}
 
-	execv(pkgpath, argv);
+	execv(pkgpath, original_argv);
 
 	/* NOT REACHED */
 	return (EXIT_FAILURE);