git: be61deae0aa2 - main - pkg: clarify argument parsing
- Reply: Cy Schubert : "Re: git: be61deae0aa2 - main - pkg: clarify argument parsing"
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Wed, 30 Apr 2025 01:01:02 UTC
The branch main has been updated by emaste:
URL: https://cgit.FreeBSD.org/src/commit/?id=be61deae0aa2923bb9908fc5b37b35712603e622
commit be61deae0aa2923bb9908fc5b37b35712603e622
Author: Isaac Freund <ifreund@freebsdfoundation.org>
AuthorDate: 2025-04-23 10:27:35 +0000
Commit: Ed Maste <emaste@FreeBSD.org>
CommitDate: 2025-04-30 01:00:17 +0000
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 40b9f924b1)
Relnotes: yes
Reviewed by: bapt
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D49977
---
usr.sbin/pkg/pkg.7 | 5 +-
usr.sbin/pkg/pkg.c | 174 ++++++++++++++++++++++++++---------------------------
2 files changed, 87 insertions(+), 92 deletions(-)
diff --git a/usr.sbin/pkg/pkg.7 b/usr.sbin/pkg/pkg.7
index 467f62d5747b..d2246f74a3fc 100644
--- a/usr.sbin/pkg/pkg.7
+++ b/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
diff --git a/usr.sbin/pkg/pkg.c b/usr.sbin/pkg/pkg.c
index 92fdbf0ebff8..b62eecfc6dce 100644
--- a/usr.sbin/pkg/pkg.c
+++ b/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);