[PATCH 0/2] die_for_incompatible_opts(): unbounded number of options

[PATCH 0/2] die_for_incompatible_opts(): unbounded number of options

[Junio C Hamano]

We have die_for_incompatible_optN() (for 2 <= N <= 4) to check and
complain when two or more among N mutually incompatible options are
used.

What should a developer do if there are more than four options that
cannot be used at once?

Introduce die_for_incompatible_opts(), which can handle an arbitrary
number of mutually exclusive options.  This is done in two steps:

 - The API for existing functions takes N pairs (for 2 <= N <= 4) of
   'int set, const char *name' parameters that signal which options
   are set.  This parameter order is inconvenient for varargs, where
   a sentinel value marks the end of the argument list (and there is
   no clear sentinel value of type 'int').  The first patch rewrites
   all implementations and callers of die_for_incompatible_optN() to
   swap the parameter order to pairs of 'const char *name, int set'.

 - The second patch then introduces die_for_incompatible_opts(),
   which takes an arbitrary number of 'const char *name, int set'
   pairs terminated by a NULL sentinel.

We could do without the first step and use the 'const char *, int'
order only in die_for_incompatible_opts(), leaving the traditional
die_for_incompatible_opt[234]() functions using the
'int, const char *' order, but using a consistent ordering is
likely easier in the long run.

 1/2: die_for_incompatible_optN: swap the order of arguments
 2/2: die_for_incompatible_opts(): accept more than four options

 builtin/add.c          |  6 +++---
 builtin/clone.c        |  8 ++++----
 builtin/commit.c       | 24 ++++++++++++------------
 builtin/difftool.c     |  6 +++---
 builtin/gc.c           |  8 ++++----
 builtin/grep.c         |  6 +++---
 builtin/log.c          |  6 +++---
 builtin/merge-tree.c   |  8 ++++----
 builtin/pack-objects.c | 21 ++++++++++-----------
 builtin/push.c         |  8 ++++----
 builtin/repack.c       | 12 +++++++-----
 builtin/replay.c       | 18 +++++++++---------
 builtin/rev-list.c     |  6 +++---
 builtin/show-ref.c     |  7 ++++---
 parse-options.c        | 26 ++++++++++++++------------
 parse-options.h        | 40 +++++++++++++++++++++++-----------------
 revision.c             | 26 +++++++++++++-------------
 17 files changed, 123 insertions(+), 113 deletions(-)

-- 
2.55.0-862-g3c6f97f7b9

[PATCH 1/2] die_for_incompatible_optN: swap the order of arguments

[Junio C Hamano]

die_for_incompatible_opt<N>() takes N pairs of <int set, const char *name>
and complains if there is two or more pairs whose "set" part is non-zero.

To implement a vararg die_for_incompatible_opts() to supersede them,
however, having an integer parameter as the first of the pair is a
bit inconvenient sentinel.  Swap the order of these pairs so that
the "const char *name" comes first and then "int set_or_unset" comes
next.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/add.c          |  6 +++---
 builtin/clone.c        |  8 ++++----
 builtin/commit.c       | 24 ++++++++++++------------
 builtin/difftool.c     |  6 +++---
 builtin/gc.c           |  8 ++++----
 builtin/grep.c         |  6 +++---
 builtin/log.c          |  6 +++---
 builtin/merge-tree.c   |  8 ++++----
 builtin/pack-objects.c | 21 ++++++++++-----------
 builtin/push.c         |  8 ++++----
 builtin/repack.c       | 12 +++++++-----
 builtin/replay.c       | 18 +++++++++---------
 builtin/rev-list.c     |  6 +++---
 builtin/show-ref.c     |  7 ++++---
 parse-options.c        |  8 ++++----
 parse-options.h        | 33 ++++++++++++++++-----------------
 revision.c             | 26 +++++++++++++-------------
 17 files changed, 106 insertions(+), 105 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index eab8f03cad..c9cb4e8265 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -511,9 +511,9 @@ int cmd_add(int argc,
 	else if (take_worktree_changes && ADDREMOVE_DEFAULT)
 		addremove = 0; /* "-u" was given but not "-A" */
 
-	die_for_incompatible_opt3(take_worktree_changes, "-u/--update",
-				  0 < addremove_explicit, "-A/--all",
-				  add_resolved, "--resolved");
+	die_for_incompatible_opt3("-u/--update", take_worktree_changes,
+				  "-A/--all", 0 < addremove_explicit,
+				  "--resolved", add_resolved);
 
 	if (!show_only && ignore_missing)
 		die(_("the option '%s' requires '%s'"), "--ignore-missing", "--dry-run");
diff --git a/builtin/clone.c b/builtin/clone.c
index 5b25cca510..7ebf6c31e2 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1361,10 +1361,10 @@ int cmd_clone(int argc,
 
 	transport_set_option(transport, TRANS_OPT_KEEP, "yes");
 
-	die_for_incompatible_opt2(!!option_rev, "--revision",
-				  !!option_branch, "--branch");
-	die_for_incompatible_opt2(!!option_rev, "--revision",
-				  option_mirror, "--mirror");
+	die_for_incompatible_opt2("--revision", !!option_rev,
+				  "--branch", !!option_branch);
+	die_for_incompatible_opt2("--revision", !!option_rev,
+				  "--mirror", option_mirror);
 
 	if (reject_shallow)
 		transport_set_option(transport, TRANS_OPT_REJECT_SHALLOW, "1");
diff --git a/builtin/commit.c b/builtin/commit.c
index 28f6174503..31c58491aa 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1338,14 +1338,14 @@ static int parse_and_validate_options(int argc, const char *argv[],
 	}
 	if (fixup_message && squash_message)
 		die(_("options '%s' and '%s' cannot be used together"), "--squash", "--fixup");
-	die_for_incompatible_opt4(!!use_message, "-C",
-				  !!edit_message, "-c",
-				  !!logfile, "-F",
-				  !!fixup_message, "--fixup");
-	die_for_incompatible_opt4(have_option_m, "-m",
-				  !!edit_message, "-c",
-				  !!use_message, "-C",
-				  !!logfile, "-F");
+	die_for_incompatible_opt4("-C", !!use_message,
+				  "-c", !!edit_message,
+				  "-F", !!logfile,
+				  "--fixup", !!fixup_message);
+	die_for_incompatible_opt4("-m", have_option_m,
+				  "-c", !!edit_message,
+				  "-C", !!use_message,
+				  "-F", !!logfile);
 	if (use_message || edit_message || logfile ||fixup_message || have_option_m)
 		FREE_AND_NULL(template_file);
 	if (edit_message)
@@ -1371,10 +1371,10 @@ static int parse_and_validate_options(int argc, const char *argv[],
 	if (patch_interactive)
 		interactive = 1;
 
-	die_for_incompatible_opt4(also, "-i/--include",
-				  only, "-o/--only",
-				  all, "-a/--all",
-				  interactive, "--interactive/-p/--patch");
+	die_for_incompatible_opt4("-i/--include", also,
+				  "-o/--only", only,
+				  "-a/--all", all,
+				  "--interactive/-p/--patch", interactive);
 	if (fixup_message) {
 		/*
 		 * We limit --fixup's suboptions to only alpha characters.
diff --git a/builtin/difftool.c b/builtin/difftool.c
index bc7b2ea443..ecdeaa3d2c 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -773,9 +773,9 @@ int cmd_difftool(int argc,
 	} else if (dir_diff)
 		die(_("options '%s' and '%s' cannot be used together"), "--dir-diff", "--no-index");
 
-	die_for_incompatible_opt3(use_gui_tool == 1, "--gui",
-				  !!difftool_cmd, "--tool",
-				  !!extcmd, "--extcmd");
+	die_for_incompatible_opt3("--gui", use_gui_tool == 1,
+				  "--tool", !!difftool_cmd,
+				  "--extcmd", !!extcmd);
 
 	/*
 	 * Explicitly specified GUI option is forwarded to git-mergetool--lib.sh;
diff --git a/builtin/gc.c b/builtin/gc.c
index de2f9e7fed..9aa69b6c6f 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1686,10 +1686,10 @@ static int maintenance_run(int argc, const char **argv, const char *prefix,
 			     builtin_maintenance_run_usage,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
 
-	die_for_incompatible_opt2(opts.auto_flag, "--auto",
-				  opts.schedule, "--schedule=");
-	die_for_incompatible_opt2(selected_tasks.nr, "--task=",
-				  opts.schedule, "--schedule=");
+	die_for_incompatible_opt2("--auto", opts.auto_flag,
+				  "--schedule=", opts.schedule);
+	die_for_incompatible_opt2("--task=", selected_tasks.nr,
+				  "--schedule=", opts.schedule);
 
 	gc_config(&cfg);
 	initialize_task_config(&opts, &selected_tasks);
diff --git a/builtin/grep.c b/builtin/grep.c
index d3d86abe01..76b163c0da 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1396,9 +1396,9 @@ int cmd_grep(int argc,
 	if (!show_in_pager && !opt.status_only)
 		setup_pager(the_repository);
 
-	die_for_incompatible_opt3(!use_index, "--no-index",
-				  untracked, "--untracked",
-				  cached, "--cached");
+	die_for_incompatible_opt3("--no-index", !use_index,
+				  "--untracked", untracked,
+				  "--cached", cached);
 
 	if (!use_index || untracked) {
 		int use_exclude = (opt_exclude < 0) ? use_index : !!opt_exclude;
diff --git a/builtin/log.c b/builtin/log.c
index 350b35c556..1acc154aae 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -2247,9 +2247,9 @@ int cmd_format_patch(int argc,
 	if (rev.show_notes)
 		load_display_notes(&rev.notes_opt);
 
-	die_for_incompatible_opt3(use_stdout, "--stdout",
-				  rev.diffopt.close_file, "--output",
-				  !!output_directory, "--output-directory");
+	die_for_incompatible_opt3("--stdout", use_stdout,
+				  "--output", rev.diffopt.close_file,
+				  "--output-directory", !!output_directory);
 
 	if (use_stdout && stdout_mboxrd)
 		rev.commit_format = CMIT_FMT_MBOXRD;
diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index 49f41e520f..efd9321fc1 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -600,10 +600,10 @@ int cmd_merge_tree(int argc,
 	if (quiet && o.show_messages == -1)
 		o.show_messages = 0;
 	o.merge_options.mergeability_only = quiet;
-	die_for_incompatible_opt2(quiet, "--quiet", o.show_messages, "--messages");
-	die_for_incompatible_opt2(quiet, "--quiet", o.name_only, "--name-only");
-	die_for_incompatible_opt2(quiet, "--quiet", o.use_stdin, "--stdin");
-	die_for_incompatible_opt2(quiet, "--quiet", !line_termination, "-z");
+	die_for_incompatible_opt2("--quiet", quiet, "--messages", o.show_messages);
+	die_for_incompatible_opt2("--quiet", quiet, "--name-only", o.name_only);
+	die_for_incompatible_opt2("--quiet", quiet, "--stdin", o.use_stdin);
+	die_for_incompatible_opt2("--quiet", quiet, "-z", !line_termination);
 
 	if (xopts.nr && o.mode == MODE_TRIVIAL)
 		die(_("--trivial-merge is incompatible with all other options"));
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 1d9dc31454..864a9ca701 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -5340,10 +5340,10 @@ int cmd_pack_objects(int argc,
 		strvec_push(&rp, "--unpacked");
 	}
 
-	die_for_incompatible_opt2(exclude_promisor_objects,
-				  "--exclude-promisor-objects",
-				  exclude_promisor_objects_best_effort,
-				  "--exclude-promisor-objects-best-effort");
+	die_for_incompatible_opt2("--exclude-promisor-objects",
+				  exclude_promisor_objects,
+				  "--exclude-promisor-objects-best-effort",
+				  exclude_promisor_objects_best_effort);
 	if (exclude_promisor_objects) {
 		fetch_if_missing = 0;
 
@@ -5385,14 +5385,13 @@ int cmd_pack_objects(int argc,
 	if (!pack_to_stdout && thin)
 		die(_("--thin cannot be used to build an indexable pack"));
 
-	die_for_incompatible_opt2(keep_unreachable, "--keep-unreachable",
-				  unpack_unreachable, "--unpack-unreachable");
+	die_for_incompatible_opt2("--keep-unreachable", keep_unreachable,
+				  "--unpack-unreachable", unpack_unreachable);
 	if (!rev_list_all || !rev_list_reflog || !rev_list_index)
 		unpack_unreachable_expiration = 0;
 
-	die_for_incompatible_opt2(stdin_packs, "--stdin-packs",
-				  filter_options.choice, "--filter");
-
+	die_for_incompatible_opt2("--stdin-packs", stdin_packs,
+				  "--filter", filter_options.choice);
 
 	if (stdin_packs && use_internal_rev_list)
 		die(_("cannot use internal rev list with --stdin-packs"));
@@ -5400,8 +5399,8 @@ int cmd_pack_objects(int argc,
 	if (cruft) {
 		if (use_internal_rev_list)
 			die(_("cannot use internal rev list with --cruft"));
-		die_for_incompatible_opt2(stdin_packs, "--stdin-packs",
-					  cruft, "--cruft");
+		die_for_incompatible_opt2("--stdin-packs", stdin_packs,
+					  "--cruft", cruft);
 	}
 
 	/*
diff --git a/builtin/push.c b/builtin/push.c
index 2377b5af55..f20b2a31fe 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -753,10 +753,10 @@ int cmd_push(int argc,
 
 	refspec_init_push(&rs, the_hash_algo);
 
-	die_for_incompatible_opt4(deleterefs, "--delete",
-				  tags, "--tags",
-				  flags & TRANSPORT_PUSH_ALL, "--all/--branches",
-				  flags & TRANSPORT_PUSH_MIRROR, "--mirror");
+	die_for_incompatible_opt4("--delete", deleterefs,
+				  "--tags", tags,
+				  "--all/--branches", flags & TRANSPORT_PUSH_ALL,
+				  "--mirror", flags & TRANSPORT_PUSH_MIRROR);
 	if (deleterefs && argc < 2)
 		die(_("--delete doesn't make sense without any refs"));
 
diff --git a/builtin/repack.c b/builtin/repack.c
index c4360382c1..bdac72a562 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -282,8 +282,8 @@ int cmd_repack(int argc,
 	po_args.depth = xstrdup_or_null(opt_depth);
 	po_args.threads = xstrdup_or_null(opt_threads);
 
-	die_for_incompatible_opt2(drop_filtered, "--drop-filtered",
-		!!filter_to, "--filter-to");
+	die_for_incompatible_opt2("--drop-filtered", drop_filtered,
+				  "--filter-to", !!filter_to);
 
 	if (dry_run && !drop_filtered)
 		die(_("--dry-run only takes effect with --drop-filtered"));
@@ -397,9 +397,11 @@ int cmd_repack(int argc,
 	if (delete_redundant && repo->repository_format_precious_objects)
 		die(_("cannot delete packs in a precious-objects repo"));
 
-	die_for_incompatible_opt3(unpack_unreachable || (pack_everything & LOOSEN_UNREACHABLE), "-A",
-				  keep_unreachable, "-k/--keep-unreachable",
-				  pack_everything & PACK_CRUFT, "--cruft");
+	die_for_incompatible_opt3("-A",
+				  unpack_unreachable ||
+				  (pack_everything & LOOSEN_UNREACHABLE),
+				  "-k/--keep-unreachable", keep_unreachable,
+				  "--cruft", pack_everything & PACK_CRUFT);
 
 	if (pack_everything & PACK_CRUFT)
 		pack_everything |= ALL_INTO_ONE;
diff --git a/builtin/replay.c b/builtin/replay.c
index 39e3a86f6c..b97d9de21f 100644
--- a/builtin/replay.c
+++ b/builtin/replay.c
@@ -123,15 +123,15 @@ int cmd_replay(int argc,
 		usage_with_options(replay_usage, replay_options);
 	}
 
-	die_for_incompatible_opt3(!!opts.onto, "--onto",
-				  !!opts.advance, "--advance",
-				  !!opts.revert, "--revert");
-	die_for_incompatible_opt2(!!opts.advance, "--advance",
-				  opts.contained, "--contained");
-	die_for_incompatible_opt2(!!opts.revert, "--revert",
-				  opts.contained, "--contained");
-	die_for_incompatible_opt2(!!opts.ref, "--ref",
-				  !!opts.contained, "--contained");
+	die_for_incompatible_opt3("--onto", !!opts.onto,
+				  "--advance", !!opts.advance,
+				  "--revert", !!opts.revert);
+	die_for_incompatible_opt2("--advance", !!opts.advance,
+				  "--contained", opts.contained);
+	die_for_incompatible_opt2("--revert", !!opts.revert,
+				  "--contained", opts.contained);
+	die_for_incompatible_opt2("--ref", !!opts.ref,
+				  "--contained", !!opts.contained);
 
 	/* Parse ref action mode from command line or config */
 	ref_mode = get_ref_action_mode(repo, ref_action);
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 02818b81c6..8e962b09da 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -755,9 +755,9 @@ int cmd_rev_list(int argc,
 		}
 	}
 
-	die_for_incompatible_opt2(revs.exclude_promisor_objects,
-				  "--exclude_promisor_objects",
-				  arg_missing_action, "--missing");
+	die_for_incompatible_opt2("--exclude_promisor_objects",
+				  revs.exclude_promisor_objects,
+				  "--missing", arg_missing_action);
 
 	if (arg_missing_action)
 		revs.do_not_die_on_missing_objects = 1;
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index d508441632..8f942ecbfc 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -337,9 +337,10 @@ struct repository *repo UNUSED)
 	argc = parse_options(argc, argv, prefix, show_ref_options,
 			     show_ref_usage, 0);
 
-	die_for_incompatible_opt3(exclude_existing_opts.enabled, "--exclude-existing",
-				  verify, "--verify",
-				  exists, "--exists");
+	die_for_incompatible_opt3("--exclude-existing",
+				  exclude_existing_opts.enabled,
+				  "--verify", verify,
+				  "--exists", exists);
 
 	if (exclude_existing_opts.enabled)
 		return cmd_show_ref__exclude_existing(&exclude_existing_opts);
diff --git a/parse-options.c b/parse-options.c
index 4519ead9dc..b56bc7e419 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1535,10 +1535,10 @@ void NORETURN usage_msg_optf(const char * const fmt,
 	usage_msg_opt(msg.buf, usagestr, options);
 }
 
-void die_for_incompatible_opt4(int opt1, const char *opt1_name,
-			       int opt2, const char *opt2_name,
-			       int opt3, const char *opt3_name,
-			       int opt4, const char *opt4_name)
+void die_for_incompatible_opt4(const char *opt1_name, int opt1,
+			       const char *opt2_name, int opt2,
+			       const char *opt3_name, int opt3,
+			       const char *opt4_name, int opt4)
 {
 	int count = 0;
 	const char *options[4];
diff --git a/parse-options.h b/parse-options.h
index d7f896a933..888949ab61 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -441,29 +441,28 @@ void NORETURN usage_msg_optf(const char *fmt,
 			     const char * const *usagestr,
 			     const struct option *options, ...);
 
-void die_for_incompatible_opt4(int opt1, const char *opt1_name,
-			       int opt2, const char *opt2_name,
-			       int opt3, const char *opt3_name,
-			       int opt4, const char *opt4_name);
+void die_for_incompatible_opt4(const char *opt1_name, int opt1,
+			       const char *opt2_name, int opt2,
+			       const char *opt3_name, int opt3,
+			       const char *opt4_name, int opt4);
 
 
-static inline void die_for_incompatible_opt3(int opt1, const char *opt1_name,
-					     int opt2, const char *opt2_name,
-					     int opt3, const char *opt3_name)
+static inline void die_for_incompatible_opt3(const char *opt1_name, int opt1,
+					     const char *opt2_name, int opt2,
+					     const char *opt3_name, int opt3)
 {
-	die_for_incompatible_opt4(opt1, opt1_name,
-				  opt2, opt2_name,
-				  opt3, opt3_name,
-				  0, "");
+	die_for_incompatible_opt4(opt1_name, opt1,
+				  opt2_name, opt2,
+				  opt3_name, opt3,
+				  "", 0);
 }
 
-static inline void die_for_incompatible_opt2(int opt1, const char *opt1_name,
-					     int opt2, const char *opt2_name)
+static inline void die_for_incompatible_opt2(const char *opt1_name, int opt1,
+					     const char *opt2_name, int opt2)
 {
-	die_for_incompatible_opt4(opt1, opt1_name,
-				  opt2, opt2_name,
-				  0, "",
-				  0, "");
+	die_for_incompatible_opt4(opt1_name, opt1,
+				  opt2_name, opt2,
+				  "", 0, "", 0);
 }
 
 /*
diff --git a/revision.c b/revision.c
index 50dc8b1991..b6108501fd 100644
--- a/revision.c
+++ b/revision.c
@@ -2352,27 +2352,27 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 
 	if ((argcount = parse_long_opt("max-count", argv, &optarg))) {
 		if (revs->max_count_type == 1)
-			die_for_incompatible_opt2(1, "--max-count", 1,
-						  "--max-count-oldest");
+			die_for_incompatible_opt2("--max-count", 1,
+						  "--max-count-oldest", 1);
 		revs->max_count = parse_count(optarg);
 		revs->no_walk = 0;
 		revs->max_count_type = 0;
 		return argcount;
 	} else if ((argcount = parse_long_opt("max-count-oldest", argv, &optarg))) {
 		if (revs->max_count_type == 0 && revs->max_count != -1)
-			die_for_incompatible_opt2(1, "--max-count", 1,
-						  "--max-count-oldest");
+			die_for_incompatible_opt2("--max-count", 1,
+						  "--max-count-oldest", 1);
 		if (revs->skip_count > 0)
-			die_for_incompatible_opt2(1, "--skip", 1,
-						  "--max-count-oldest");
+			die_for_incompatible_opt2("--skip", 1,
+						  "--max-count-oldest", 1);
 		revs->max_count = parse_count(optarg);
 		revs->no_walk = 0;
 		revs->max_count_type = 1;
 		revs->max_count_stage = 0;
 	} else if ((argcount = parse_long_opt("skip", argv, &optarg))) {
 		if (revs->max_count_type == 1)
-			die_for_incompatible_opt2(1, "--skip", 1,
-						  "--max-count-oldest");
+			die_for_incompatible_opt2("--skip", 1,
+						  "--max-count-oldest", 1);
 		revs->skip_count = parse_count(optarg);
 		return argcount;
 	} else if ((*arg == '-') && isdigit(arg[1])) {
@@ -3205,12 +3205,12 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 	/*
 	 * Limitations on the graph functionality
 	 */
-	die_for_incompatible_opt3(!!revs->graph, "--graph",
-				  !!revs->reverse, "--reverse",
-				  !!revs->reflog_info, "--walk-reflogs");
+	die_for_incompatible_opt3("--graph", !!revs->graph,
+				  "--reverse", !!revs->reverse,
+				  "--walk-reflogs", !!revs->reflog_info);
 
-	die_for_incompatible_opt2(!!revs->boundary, "--boundary",
-				  !!revs->maximal_only, "--maximal-only");
+	die_for_incompatible_opt2("--boundary", !!revs->boundary,
+				  "--maximal-only", !!revs->maximal_only);
 
 	if (revs->no_walk && revs->graph)
 		die(_("options '%s' and '%s' cannot be used together"), "--no-walk", "--graph");
-- 
2.55.0-862-g3c6f97f7b9

[PATCH 2/2] die_for_incompatible_opts(): accept more than four options

[Junio C Hamano]

Introduce die_for_incompatible_opts(), which takes an arbitrary
and unbounded number of <option-name, option-set> pairs and
complains when two or more of these options are set at the same time.

Reimplement die_for_incompatible_opt4() and others in terms of this
function.

To avoid allocation costs, the implementation reports only the first
four mutually incompatible options used.

This behavior is deliberate.  If a set of ten options were mutually
exclusive and a user specified seven of them at once, they would be
told that the first four cannot be used together.  If the user then
tries the remaining three, the same error for the remaining three
would be reported.  It is dubious that there is any practical
downside to not reporting all seven incompatible options at once,
especially given that there are other three mutually incompatible
options that the user will not be told about with this message
anyway.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 parse-options.c | 26 ++++++++++++++------------
 parse-options.h | 15 +++++++++++----
 2 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index b56bc7e419..163842837c 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1535,26 +1535,28 @@ void NORETURN usage_msg_optf(const char * const fmt,
 	usage_msg_opt(msg.buf, usagestr, options);
 }
 
-void die_for_incompatible_opt4(const char *opt1_name, int opt1,
-			       const char *opt2_name, int opt2,
-			       const char *opt3_name, int opt3,
-			       const char *opt4_name, int opt4)
+void die_for_incompatible_opts(const char *opt1_name, int opt1, ...)
 {
-	int count = 0;
+	unsigned count = 0;
 	const char *options[4];
+	va_list ap;
+
+	va_start(ap, opt1);
 
 	if (opt1)
 		options[count++] = opt1_name;
-	if (opt2)
-		options[count++] = opt2_name;
-	if (opt3)
-		options[count++] = opt3_name;
-	if (opt4)
-		options[count++] = opt4_name;
+	while (count < ARRAY_SIZE(options)) {
+		const char *name = va_arg(ap, const char *);
+		if (!name)
+			break;
+		if (va_arg(ap, int))
+			options[count++] = name;
+	}
+
 	switch (count) {
 	case 4:
 		die(_("options '%s', '%s', '%s', and '%s' cannot be used together"),
-		    opt1_name, opt2_name, opt3_name, opt4_name);
+		    options[0], options[1], options[2], options[3]);
 		break;
 	case 3:
 		die(_("options '%s', '%s', and '%s' cannot be used together"),
diff --git a/parse-options.h b/parse-options.h
index 888949ab61..79e4de9b32 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -441,11 +441,18 @@ void NORETURN usage_msg_optf(const char *fmt,
 			     const char * const *usagestr,
 			     const struct option *options, ...);
 
-void die_for_incompatible_opt4(const char *opt1_name, int opt1,
-			       const char *opt2_name, int opt2,
-			       const char *opt3_name, int opt3,
-			       const char *opt4_name, int opt4);
+void die_for_incompatible_opts(const char *opt1_name, int opt1, ...);
 
+static inline void die_for_incompatible_opt4(const char *opt1_name, int opt1,
+					     const char *opt2_name, int opt2,
+					     const char *opt3_name, int opt3,
+					     const char *opt4_name, int opt4)
+{
+	die_for_incompatible_opts(opt1_name, opt1,
+				  opt2_name, opt2,
+				  opt3_name, opt3,
+				  opt4_name, opt4, NULL);
+}
 
 static inline void die_for_incompatible_opt3(const char *opt1_name, int opt1,
 					     const char *opt2_name, int opt2,
-- 
2.55.0-862-g3c6f97f7b9

Re: [PATCH 2/2] die_for_incompatible_opts(): accept more than four options

[Elijah Newren]

On Wed, Aug 26, 2026 at 4:32 PM Junio C Hamano <gitster@pobox.com> wrote:
>
[...]
> +void die_for_incompatible_opts(const char *opt1_name, int opt1, ...)
>  {
> -       int count = 0;
> +       unsigned count = 0;
>         const char *options[4];
> +       va_list ap;
> +
> +       va_start(ap, opt1);
>
>         if (opt1)
>                 options[count++] = opt1_name;
> -       if (opt2)
> -               options[count++] = opt2_name;
> -       if (opt3)
> -               options[count++] = opt3_name;
> -       if (opt4)
> -               options[count++] = opt4_name;
> +       while (count < ARRAY_SIZE(options)) {
> +               const char *name = va_arg(ap, const char *);
> +               if (!name)
> +                       break;
> +               if (va_arg(ap, int))
> +                       options[count++] = name;
> +       }
> +
>         switch (count) {
>         case 4:
>                 die(_("options '%s', '%s', '%s', and '%s' cannot be used together"),
> -                   opt1_name, opt2_name, opt3_name, opt4_name);
> +                   options[0], options[1], options[2], options[3]);
>                 break;
>         case 3:
>                 die(_("options '%s', '%s', and '%s' cannot be used together"),
> diff --git a/parse-options.h b/parse-options.h

va_start() without a va_end()?

Re: [PATCH 2/2] die_for_incompatible_opts(): accept more than four options

[Jeff King]

On Wed, Aug 26, 2026 at 04:31:52PM -0700, Junio C Hamano wrote:

> To avoid allocation costs, the implementation reports only the first
> four mutually incompatible options used.
> 
> This behavior is deliberate.  If a set of ten options were mutually
> exclusive and a user specified seven of them at once, they would be
> told that the first four cannot be used together.  If the user then
> tries the remaining three, the same error for the remaining three
> would be reported.  It is dubious that there is any practical
> downside to not reporting all seven incompatible options at once,
> especially given that there are other three mutually incompatible
> options that the user will not be told about with this message
> anyway.

It took me a minute to understand why we would even want to have an
arbitrary-sized input if we are capping at 4 anyway. The answer is that
we are capping at 4 options _that the user actually specified_. But the
input can be the total set of conflicting options, which is greater. OK.

Really we could cap at 2 if we wanted to be technically correct, but it
might annoy the user to find each pair iteratively.

So that makes sense. Of course the follow-on question is whether any
callers actually want to pass more than 4 options. I don't see any
patches adding new calls.

> -void die_for_incompatible_opt4(const char *opt1_name, int opt1,
> -			       const char *opt2_name, int opt2,
> -			       const char *opt3_name, int opt3,
> -			       const char *opt4_name, int opt4)

One nice thing about foo4() without varargs is that the compiler will
tell you if you messed it up. The obvious downside being that you have
to count in order to avoid messing it up. ;)

But now we can forget the NULL terminator and cause a runtime problem.
So we probably want LAST_ARG_MUST_BE_NULL in the header file here:

> +void die_for_incompatible_opts(const char *opt1_name, int opt1, ...);

The rest of the patch looks OK, but just a few observations.

> +void die_for_incompatible_opts(const char *opt1_name, int opt1, ...)
>  {
> -	int count = 0;
> +	unsigned count = 0;
>  	const char *options[4];
> +	va_list ap;
> +
> +	va_start(ap, opt1);
>  
>  	if (opt1)
>  		options[count++] = opt1_name;
> -	if (opt2)
> -		options[count++] = opt2_name;
> -	if (opt3)
> -		options[count++] = opt3_name;
> -	if (opt4)
> -		options[count++] = opt4_name;
> +	while (count < ARRAY_SIZE(options)) {

Using ARRAY_SIZE() is nice, because we could in theory bump this 4
later. Though sadly here:

>  	switch (count) {
>  	case 4:
>  		die(_("options '%s', '%s', '%s', and '%s' cannot be used together"),
> -		    opt1_name, opt2_name, opt3_name, opt4_name);
> +		    options[0], options[1], options[2], options[3]);

we still hard-code various count values. It probably would be fine to
allocate a buffer for the message, though I guess that pushes
translators into lego-land.

> +static inline void die_for_incompatible_opt4(const char *opt1_name, int opt1,
> +					     const char *opt2_name, int opt2,
> +					     const char *opt3_name, int opt3,
> +					     const char *opt4_name, int opt4)
> +{
> +	die_for_incompatible_opts(opt1_name, opt1,
> +				  opt2_name, opt2,
> +				  opt3_name, opt3,
> +				  opt4_name, opt4, NULL);
> +}

OK, now we wrap the arbitrary-sized version. The "3" and "2" variants
could probably be cleaned up slightly by calling it, too, rather than
passing dummy 0/"" values.

-Peff

Re: [PATCH 2/2] die_for_incompatible_opts(): accept more than four options

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

>> diff --git a/parse-options.h b/parse-options.h
>
> va_start() without a va_end()?

Good eyes.  Thanks.

Re: [PATCH 2/2] die_for_incompatible_opts(): accept more than four options

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> It took me a minute to understand why we would even want to have an
> arbitrary-sized input if we are capping at 4 anyway. The answer is that
> we are capping at 4 options _that the user actually specified_. But the
> input can be the total set of conflicting options, which is greater. OK.
>
> Really we could cap at 2 if we wanted to be technically correct, but it
> might annoy the user to find each pair iteratively.
>
> So that makes sense. Of course the follow-on question is whether any
> callers actually want to pass more than 4 options. I don't see any
> patches adding new calls.

There isn't.  While I was writing [*], I wondered if the two calls
next to each other for opt3 and opt4 want to be combined to opt7.

  *  https://lore.kernel.org/git/xmqq1pbkefh0.fsf@gitster.g/



>> -void die_for_incompatible_opt4(const char *opt1_name, int opt1,
>> -			       const char *opt2_name, int opt2,
>> -			       const char *opt3_name, int opt3,
>> -			       const char *opt4_name, int opt4)
>
> One nice thing about foo4() without varargs is that the compiler will
> tell you if you messed it up. The obvious downside being that you have
> to count in order to avoid messing it up. ;)

Yes.  I like that and that is why the static inlines are kept to
cover the most common cases.

I think I can do without [1/2], by the way.

 - die_for_incompatible_optN() (2 <= N <= 4) will keep accepting N
   pairs of <int, const char *>

 - die_for_incompatible_opts() will take pairs of <int, const char *>,
   expects "int" to be 0 (not set), 1 (set), or EOF==-1 (sentinel).

 - static inline void die_for_incompatible_opt2() emulation layer
   will call die_for_incompatible_opts(!!opt1, opt1_name, !!opt2,
   opt2_name, EOF).  Similarly for opt3() and opt4() variants.

> Using ARRAY_SIZE() is nice, because we could in theory bump this 4
> later. Though sadly here:
>
>>  	switch (count) {
>>  	case 4:
>>  		die(_("options '%s', '%s', '%s', and '%s' cannot be used together"),
>> -		    opt1_name, opt2_name, opt3_name, opt4_name);
>> +		    options[0], options[1], options[2], options[3]);
>
> we still hard-code various count values. It probably would be fine to
> allocate a buffer for the message, though I guess that pushes
> translators into lego-land.

Very true.

We could switch to dynamic allocations immediately after we see
option[] filled, as we are committed to die() at that point and can
afford to waste cycles.  That way, for die_for_incompatible_opt10()
when the end-user uses 7 of them, we can fill option[4], switch to
dynamic allocation to collect all 7 of them and report.

The reason I chose not to is primarily because we cannot use the
existing message templates in that case, hurting i18n/l10n.

[PATCH v2] die_for_incompatible_opts(): unbounded number of options

[Junio C Hamano]

We have die_for_incompatible_optN() (for 2 <= N <= 4) to check and
complain when two or more among N mutually incompatible options are
used.

What should a developer do if there are more than four options that
cannot be used at once?

Introduce die_for_incompatible_opts(), which can handle an arbitrary
number of mutually exclusive options, and rewrite existing variants
using it.

The new function takes N pairs of <bool optN, const char *nameN>,
followed by EOF.  Note that even if the caller passes bool, it is
promoted to platform-natural int when calling this variadic
function.  Thus, the implementation uses va_arg(ap, int) to extract
the value, which allows it to distinguish between bool and EOF
serving as the sentinel.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 parse-options.c | 29 +++++++++++++++++------------
 parse-options.h | 38 ++++++++++++++++++++++++++------------
 2 files changed, 43 insertions(+), 24 deletions(-)

diff --git c/parse-options.c w/parse-options.c
index 4519ead9dc..0aad1e5373 100644
--- c/parse-options.c
+++ w/parse-options.c
@@ -1535,26 +1535,31 @@ void NORETURN usage_msg_optf(const char * const fmt,
 	usage_msg_opt(msg.buf, usagestr, options);
 }
 
-void die_for_incompatible_opt4(int opt1, const char *opt1_name,
-			       int opt2, const char *opt2_name,
-			       int opt3, const char *opt3_name,
-			       int opt4, const char *opt4_name)
+void die_for_incompatible_opts(bool opt1, const char *opt1_name, ...)
 {
-	int count = 0;
+	unsigned count = 0;
 	const char *options[4];
+	va_list ap;
 
 	if (opt1)
 		options[count++] = opt1_name;
-	if (opt2)
-		options[count++] = opt2_name;
-	if (opt3)
-		options[count++] = opt3_name;
-	if (opt4)
-		options[count++] = opt4_name;
+	va_start(ap, opt1_name);
+	while (count < ARRAY_SIZE(options)) {
+		int opt_set = va_arg(ap, int);
+		const char *opt_name;
+
+		if (opt_set == EOF)
+			break;
+		opt_name = va_arg(ap, const char *);
+		if (opt_set)
+			options[count++] = opt_name;
+	}
+	va_end(ap);
+
 	switch (count) {
 	case 4:
 		die(_("options '%s', '%s', '%s', and '%s' cannot be used together"),
-		    opt1_name, opt2_name, opt3_name, opt4_name);
+		    options[0], options[1], options[2], options[3]);
 		break;
 	case 3:
 		die(_("options '%s', '%s', and '%s' cannot be used together"),
diff --git c/parse-options.h w/parse-options.h
index d7f896a933..50bd715b86 100644
--- c/parse-options.h
+++ w/parse-options.h
@@ -441,29 +441,43 @@ void NORETURN usage_msg_optf(const char *fmt,
 			     const char * const *usagestr,
 			     const struct option *options, ...);
 
-void die_for_incompatible_opt4(int opt1, const char *opt1_name,
-			       int opt2, const char *opt2_name,
-			       int opt3, const char *opt3_name,
-			       int opt4, const char *opt4_name);
+/*
+ * Take N pairs of <bool optN, const char *opt_nameN> as parameters,
+ * followed by EOF.  The caller declares "The options opt_name1 through
+ * opt_nameN exist and the command line has options whose optN is set."
+ * and asks that an error be raised if two or more of these options are
+ * set at the same time.
+ */
+void die_for_incompatible_opts(bool opt1, const char *opt1_name, ...);
 
+static inline void die_for_incompatible_opt4(int opt1, const char *opt1_name,
+					     int opt2, const char *opt2_name,
+					     int opt3, const char *opt3_name,
+					     int opt4, const char *opt4_name)
+{
+	die_for_incompatible_opts(!!opt1, opt1_name,
+				  !!opt2, opt2_name,
+				  !!opt3, opt3_name,
+				  !!opt4, opt4_name,
+				  EOF);
+}
 
 static inline void die_for_incompatible_opt3(int opt1, const char *opt1_name,
 					     int opt2, const char *opt2_name,
 					     int opt3, const char *opt3_name)
 {
-	die_for_incompatible_opt4(opt1, opt1_name,
-				  opt2, opt2_name,
-				  opt3, opt3_name,
-				  0, "");
+	die_for_incompatible_opts(!!opt1, opt1_name,
+				  !!opt2, opt2_name,
+				  !!opt3, opt3_name,
+				  EOF);
 }
 
 static inline void die_for_incompatible_opt2(int opt1, const char *opt1_name,
 					     int opt2, const char *opt2_name)
 {
-	die_for_incompatible_opt4(opt1, opt1_name,
-				  opt2, opt2_name,
-				  0, "",
-				  0, "");
+	die_for_incompatible_opts(!!opt1, opt1_name,
+				  !!opt2, opt2_name,
+				  EOF);
 }
 
 /*

Re: [PATCH 2/2] die_for_incompatible_opts(): accept more than four options

[Jeff King]

On Thu, Aug 27, 2026 at 07:35:38AM -0700, Junio C Hamano wrote:

> > So that makes sense. Of course the follow-on question is whether any
> > callers actually want to pass more than 4 options. I don't see any
> > patches adding new calls.
> 
> There isn't.  While I was writing [*], I wondered if the two calls
> next to each other for opt3 and opt4 want to be combined to opt7.

OK. I wonder if we're approaching churn here, but I don't have a strong
feeling.

> I think I can do without [1/2], by the way.
> 
>  - die_for_incompatible_optN() (2 <= N <= 4) will keep accepting N
>    pairs of <int, const char *>
> 
>  - die_for_incompatible_opts() will take pairs of <int, const char *>,
>    expects "int" to be 0 (not set), 1 (set), or EOF==-1 (sentinel).
> 
>  - static inline void die_for_incompatible_opt2() emulation layer
>    will call die_for_incompatible_opts(!!opt1, opt1_name, !!opt2,
>    opt2_name, EOF).  Similarly for opt3() and opt4() variants.

Yeah, but then you can't get good compiler support, since I don't think
there is an integer equivalent to LAST_ARG_MUST_BE_NULL. So the varargs
interface feels less safe (and strictly worse since we are not actually
helping any case that has more than 4 items).

If we're not actually exposing the varargs version and expect people to
use the counted wrappers, then it's not as big a risk. But then I wonder
what the value of the patch is.

> We could switch to dynamic allocations immediately after we see
> option[] filled, as we are committed to die() at that point and can
> afford to waste cycles.  That way, for die_for_incompatible_opt10()
> when the end-user uses 7 of them, we can fill option[4], switch to
> dynamic allocation to collect all 7 of them and report.
> 
> The reason I chose not to is primarily because we cannot use the
> existing message templates in that case, hurting i18n/l10n.

Yeah, that makes sense.

-Peff

Re: [PATCH v2] die_for_incompatible_opts(): unbounded number of options

[Jeff King]

On Thu, Aug 27, 2026 at 10:28:32AM -0700, Junio C Hamano wrote:

> +void die_for_incompatible_opts(bool opt1, const char *opt1_name, ...)

I'm mildly negative on this, just because there's no compiler support
for making sure there is an EOF somewhere. Keeping patch 1 and using
LAST_ARG_MUST_BE_NULL would be preferable, IMHO.

-Peff