[PATCH 0/7] standardize incompatibility messages

[PATCH 0/7] standardize incompatibility messages

[René Scharfe]

More of what a699367bb8 (i18n: factorize more 'incompatible options'
messages, 2022-01-31) did: Simplify checks for multiple mutually
exclusive options, reduce the number of strings to translate, improve UI
consistency a bit.

  push: use die_for_incompatible_opt4() for --delete/--tags/--all/--mirror
  repack: use die_for_incompatible_opt3() for -A/-k/--cruft
  revision: use die_for_incompatible_opt3() for --graph/--reverse/--walk-reflogs
  revision, rev-parse: factorize incompatibility messages about --exclude-hidden
  clean: factorize incompatibility message
  worktree: standardize incompatibility messages
  worktree: simplify incompatibility message for --orphan and commit-ish

 builtin/clean.c                    |  2 +-
 builtin/push.c                     | 12 ++++--------
 builtin/repack.c                   | 14 ++++----------
 builtin/rev-parse.c                |  9 ++++++---
 builtin/worktree.c                 | 21 +++++++++++----------
 revision.c                         | 27 +++++++++++++++------------
 t/t2400-worktree-add.sh            |  2 +-
 t/t6018-rev-list-glob.sh           |  6 ++----
 t/t6021-rev-list-exclude-hidden.sh |  4 ++--
 9 files changed, 46 insertions(+), 51 deletions(-)

--
2.43.0

[PATCH 1/7] push: use die_for_incompatible_opt4() for --delete/--tags/--all/--mirror

[René Scharfe]

The push option --delete is incompatible with --all, --mirror, and
--tags; --tags is incompatible with --all and --mirror; --all is
incompatible with --mirror.  This means they are all incompatible with
each other.  And --branches is an alias for --all.

Use the function for checking four mutually incompatible options,
die_for_incompatible_opt4(), to perform this check in one place and
without repetition.  This is shorter and clearer.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/push.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 2e708383c2..f77f424324 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -639,8 +639,10 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 		: &push_options_config);
 	set_push_cert_flags(&flags, push_cert);

-	if (deleterefs && (tags || (flags & (TRANSPORT_PUSH_ALL | TRANSPORT_PUSH_MIRROR))))
-		die(_("options '%s' and '%s' cannot be used together"), "--delete", "--all/--branches/--mirror/--tags");
+	die_for_incompatible_opt4(deleterefs, "--delete",
+				  tags, "--tags",
+				  flags & TRANSPORT_PUSH_ALL, "--all/--branches",
+				  flags & TRANSPORT_PUSH_MIRROR, "--mirror");
 	if (deleterefs && argc < 2)
 		die(_("--delete doesn't make sense without any refs"));

@@ -677,19 +679,13 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 		flags |= (TRANSPORT_PUSH_MIRROR|TRANSPORT_PUSH_FORCE);

 	if (flags & TRANSPORT_PUSH_ALL) {
-		if (tags)
-			die(_("options '%s' and '%s' cannot be used together"), "--all", "--tags");
 		if (argc >= 2)
 			die(_("--all can't be combined with refspecs"));
 	}
 	if (flags & TRANSPORT_PUSH_MIRROR) {
-		if (tags)
-			die(_("options '%s' and '%s' cannot be used together"), "--mirror", "--tags");
 		if (argc >= 2)
 			die(_("--mirror can't be combined with refspecs"));
 	}
-	if ((flags & TRANSPORT_PUSH_ALL) && (flags & TRANSPORT_PUSH_MIRROR))
-		die(_("options '%s' and '%s' cannot be used together"), "--all", "--mirror");

 	if (!is_empty_cas(&cas) && (flags & TRANSPORT_PUSH_FORCE_IF_INCLUDES))
 		cas.use_force_if_includes = 1;
--
2.43.0

[PATCH 2/7] repack: use die_for_incompatible_opt3() for -A/-k/--cruft

[René Scharfe]

The repack option --keep-unreachable is incompatible with -A, --cruft is
incompatible with -A and -k, and -k is short for --keep-unreachable.  So
they are all incompatible with each other.

Use the function for checking three mutually incompatible options,
die_for_incompatible_opt3(), to perform this check in one place and
without repetition.  This is shorter and clearer.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/repack.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index edaee4dbec..c54777bbe5 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -1203,19 +1203,13 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	if (delete_redundant && repository_format_precious_objects)
 		die(_("cannot delete packs in a precious-objects repo"));

-	if (keep_unreachable &&
-	    (unpack_unreachable || (pack_everything & LOOSEN_UNREACHABLE)))
-		die(_("options '%s' and '%s' cannot be used together"), "--keep-unreachable", "-A");
+	die_for_incompatible_opt3(unpack_unreachable || (pack_everything & LOOSEN_UNREACHABLE), "-A",
+				  keep_unreachable, "-k/--keep-unreachable",
+				  pack_everything & PACK_CRUFT, "--cruft");

-	if (pack_everything & PACK_CRUFT) {
+	if (pack_everything & PACK_CRUFT)
 		pack_everything |= ALL_INTO_ONE;

-		if (unpack_unreachable || (pack_everything & LOOSEN_UNREACHABLE))
-			die(_("options '%s' and '%s' cannot be used together"), "--cruft", "-A");
-		if (keep_unreachable)
-			die(_("options '%s' and '%s' cannot be used together"), "--cruft", "-k");
-	}
-
 	if (write_bitmaps < 0) {
 		if (!write_midx &&
 		    (!(pack_everything & ALL_INTO_ONE) || !is_bare_repository()))
--
2.43.0

[PATCH 3/7] revision: use die_for_incompatible_opt3() for --graph/--reverse/--walk-reflogs

[René Scharfe]

The revision options --reverse is incompatibel with --walk-reflogs and
--graph is incompatible with both --reverse and --walk-reflogs.  So they
are all incompatible with each other.

Use the function for checking three mutually incompatible options,
die_for_incompatible_opt3(), to perform this check in one place and
without repetition.  This is shorter and clearer.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 revision.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/revision.c b/revision.c
index b2861474b1..1b7e1af6c6 100644
--- a/revision.c
+++ b/revision.c
@@ -3036,8 +3036,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 		revs->grep_filter.ignore_locale = 1;
 	compile_grep_patterns(&revs->grep_filter);

-	if (revs->reverse && revs->reflog_info)
-		die(_("options '%s' and '%s' cannot be used together"), "--reverse", "--walk-reflogs");
 	if (revs->reflog_info && revs->limited)
 		die("cannot combine --walk-reflogs with history-limiting options");
 	if (revs->rewrite_parents && revs->children.name)
@@ -3048,11 +3046,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 	/*
 	 * Limitations on the graph functionality
 	 */
-	if (revs->reverse && revs->graph)
-		die(_("options '%s' and '%s' cannot be used together"), "--reverse", "--graph");
+	die_for_incompatible_opt3(!!revs->graph, "--graph",
+				  !!revs->reverse, "--reverse",
+				  !!revs->reflog_info, "--walk-reflogs");

-	if (revs->reflog_info && revs->graph)
-		die(_("options '%s' and '%s' cannot be used together"), "--walk-reflogs", "--graph");
 	if (revs->no_walk && revs->graph)
 		die(_("options '%s' and '%s' cannot be used together"), "--no-walk", "--graph");
 	if (!revs->reflog_info && revs->grep_filter.use_reflog_filter)
--
2.43.0

[PATCH 4/7] revision, rev-parse: factorize incompatibility messages about --exclude-hidden

[René Scharfe]

Use the standard parameterized message for reporting incompatible
options to report options that are not accepted in combination with
--exclude-hidden.  This reduces the number of strings to translate and
makes the UI a bit more consistent.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/rev-parse.c                |  9 ++++++---
 revision.c                         | 18 ++++++++++++------
 t/t6018-rev-list-glob.sh           |  6 ++----
 t/t6021-rev-list-exclude-hidden.sh |  4 ++--
 4 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index fde8861ca4..917f122440 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -893,13 +893,15 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 			}
 			if (opt_with_value(arg, "--branches", &arg)) {
 				if (ref_excludes.hidden_refs_configured)
-					return error(_("--exclude-hidden cannot be used together with --branches"));
+					return error(_("options '%s' and '%s' cannot be used together"),
+						     "--exclude-hidden", "--branches");
 				handle_ref_opt(arg, "refs/heads/");
 				continue;
 			}
 			if (opt_with_value(arg, "--tags", &arg)) {
 				if (ref_excludes.hidden_refs_configured)
-					return error(_("--exclude-hidden cannot be used together with --tags"));
+					return error(_("options '%s' and '%s' cannot be used together"),
+						     "--exclude-hidden", "--tags");
 				handle_ref_opt(arg, "refs/tags/");
 				continue;
 			}
@@ -909,7 +911,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 			}
 			if (opt_with_value(arg, "--remotes", &arg)) {
 				if (ref_excludes.hidden_refs_configured)
-					return error(_("--exclude-hidden cannot be used together with --remotes"));
+					return error(_("options '%s' and '%s' cannot be used together"),
+						     "--exclude-hidden", "--remotes");
 				handle_ref_opt(arg, "refs/remotes/");
 				continue;
 			}
diff --git a/revision.c b/revision.c
index 1b7e1af6c6..25335a74ad 100644
--- a/revision.c
+++ b/revision.c
@@ -2709,7 +2709,8 @@ static int handle_revision_pseudo_opt(struct rev_info *revs,
 		clear_ref_exclusions(&revs->ref_excludes);
 	} else if (!strcmp(arg, "--branches")) {
 		if (revs->ref_excludes.hidden_refs_configured)
-			return error(_("--exclude-hidden cannot be used together with --branches"));
+			return error(_("options '%s' and '%s' cannot be used together"),
+				     "--exclude-hidden", "--branches");
 		handle_refs(refs, revs, *flags, refs_for_each_branch_ref);
 		clear_ref_exclusions(&revs->ref_excludes);
 	} else if (!strcmp(arg, "--bisect")) {
@@ -2720,12 +2721,14 @@ static int handle_revision_pseudo_opt(struct rev_info *revs,
 		revs->bisect = 1;
 	} else if (!strcmp(arg, "--tags")) {
 		if (revs->ref_excludes.hidden_refs_configured)
-			return error(_("--exclude-hidden cannot be used together with --tags"));
+			return error(_("options '%s' and '%s' cannot be used together"),
+				     "--exclude-hidden", "--tags");
 		handle_refs(refs, revs, *flags, refs_for_each_tag_ref);
 		clear_ref_exclusions(&revs->ref_excludes);
 	} else if (!strcmp(arg, "--remotes")) {
 		if (revs->ref_excludes.hidden_refs_configured)
-			return error(_("--exclude-hidden cannot be used together with --remotes"));
+			return error(_("options '%s' and '%s' cannot be used together"),
+				     "--exclude-hidden", "--remotes");
 		handle_refs(refs, revs, *flags, refs_for_each_remote_ref);
 		clear_ref_exclusions(&revs->ref_excludes);
 	} else if ((argcount = parse_long_opt("glob", argv, &optarg))) {
@@ -2743,21 +2746,24 @@ static int handle_revision_pseudo_opt(struct rev_info *revs,
 	} else if (skip_prefix(arg, "--branches=", &optarg)) {
 		struct all_refs_cb cb;
 		if (revs->ref_excludes.hidden_refs_configured)
-			return error(_("--exclude-hidden cannot be used together with --branches"));
+			return error(_("options '%s' and '%s' cannot be used together"),
+				     "--exclude-hidden", "--branches");
 		init_all_refs_cb(&cb, revs, *flags);
 		for_each_glob_ref_in(handle_one_ref, optarg, "refs/heads/", &cb);
 		clear_ref_exclusions(&revs->ref_excludes);
 	} else if (skip_prefix(arg, "--tags=", &optarg)) {
 		struct all_refs_cb cb;
 		if (revs->ref_excludes.hidden_refs_configured)
-			return error(_("--exclude-hidden cannot be used together with --tags"));
+			return error(_("options '%s' and '%s' cannot be used together"),
+				     "--exclude-hidden", "--tags");
 		init_all_refs_cb(&cb, revs, *flags);
 		for_each_glob_ref_in(handle_one_ref, optarg, "refs/tags/", &cb);
 		clear_ref_exclusions(&revs->ref_excludes);
 	} else if (skip_prefix(arg, "--remotes=", &optarg)) {
 		struct all_refs_cb cb;
 		if (revs->ref_excludes.hidden_refs_configured)
-			return error(_("--exclude-hidden cannot be used together with --remotes"));
+			return error(_("options '%s' and '%s' cannot be used together"),
+				     "--exclude-hidden", "--remotes");
 		init_all_refs_cb(&cb, revs, *flags);
 		for_each_glob_ref_in(handle_one_ref, optarg, "refs/remotes/", &cb);
 		clear_ref_exclusions(&revs->ref_excludes);
diff --git a/t/t6018-rev-list-glob.sh b/t/t6018-rev-list-glob.sh
index 67d523d405..3b181f771c 100755
--- a/t/t6018-rev-list-glob.sh
+++ b/t/t6018-rev-list-glob.sh
@@ -214,15 +214,13 @@ do
 	for pseudoopt in branches tags remotes
 	do
 		test_expect_success "rev-parse --exclude-hidden=$section fails with --$pseudoopt" '
-			echo "error: --exclude-hidden cannot be used together with --$pseudoopt" >expected &&
 			test_must_fail git rev-parse --exclude-hidden=$section --$pseudoopt 2>err &&
-			test_cmp expected err
+			test_grep "error: options .--exclude-hidden. and .--$pseudoopt. cannot be used together" err
 		'

 		test_expect_success "rev-parse --exclude-hidden=$section fails with --$pseudoopt=pattern" '
-			echo "error: --exclude-hidden cannot be used together with --$pseudoopt" >expected &&
 			test_must_fail git rev-parse --exclude-hidden=$section --$pseudoopt=pattern 2>err &&
-			test_cmp expected err
+			test_grep "error: options .--exclude-hidden. and .--$pseudoopt. cannot be used together" err
 		'
 	done
 done
diff --git a/t/t6021-rev-list-exclude-hidden.sh b/t/t6021-rev-list-exclude-hidden.sh
index cdf7aa9427..51df02105d 100755
--- a/t/t6021-rev-list-exclude-hidden.sh
+++ b/t/t6021-rev-list-exclude-hidden.sh
@@ -151,12 +151,12 @@ do
 	do
 		test_expect_success "$section: fails with --$pseudoopt" '
 			test_must_fail git rev-list --exclude-hidden=$section --$pseudoopt 2>err &&
-			test_grep "error: --exclude-hidden cannot be used together with --$pseudoopt" err
+			test_grep "error: options .--exclude-hidden. and .--$pseudoopt. cannot be used together" err
 		'

 		test_expect_success "$section: fails with --$pseudoopt=pattern" '
 			test_must_fail git rev-list --exclude-hidden=$section --$pseudoopt=pattern 2>err &&
-			test_grep "error: --exclude-hidden cannot be used together with --$pseudoopt" err
+			test_grep "error: options .--exclude-hidden. and .--$pseudoopt. cannot be used together" err
 		'
 	done
 done
--
2.43.0

[PATCH 5/7] clean: factorize incompatibility message

[René Scharfe]

Use the standard parameterized message for reporting incompatible
options to inform users that they can't use -x and -X together.  This
reduces the number of strings to translate and makes the UI slightly
more consistent.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/clean.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/clean.c b/builtin/clean.c
index 49c224e626..d90766cad3 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -971,7 +971,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 	dir.flags |= DIR_SHOW_OTHER_DIRECTORIES;

 	if (ignored && ignored_only)
-		die(_("-x and -X cannot be used together"));
+		die(_("options '%s' and '%s' cannot be used together"), "-x", "-X");
 	if (!ignored)
 		setup_standard_excludes(&dir);
 	if (ignored_only)
--
2.43.0

[PATCH 6/7] worktree: standardize incompatibility messages

[René Scharfe]

Use the standard parameterized message for reporting incompatible
options for worktree add.  This reduces the number of strings to
translate and makes the UI slightly more consistent.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/worktree.c      | 17 +++++++++--------
 t/t2400-worktree-add.sh |  2 +-
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 62b7e26f4b..036ceaa981 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -730,11 +730,11 @@ static int dwim_orphan(const struct add_opts *opts, int opt_track, int remote)
 	}

 	if (opt_track) {
-		die(_("'%s' and '%s' cannot be used together"), "--orphan",
-		    "--track");
+		die(_("options '%s' and '%s' cannot be used together"),
+		    "--orphan", "--track");
 	} else if (!opts->checkout) {
-		die(_("'%s' and '%s' cannot be used together"), "--orphan",
-		    "--no-checkout");
+		die(_("options '%s' and '%s' cannot be used together"),
+		    "--orphan", "--no-checkout");
 	}
 	return 1;
 }
@@ -806,13 +806,14 @@ static int add(int ac, const char **av, const char *prefix)
 	if (!!opts.detach + !!new_branch + !!new_branch_force > 1)
 		die(_("options '%s', '%s', and '%s' cannot be used together"), "-b", "-B", "--detach");
 	if (opts.detach && opts.orphan)
-		die(_("options '%s', and '%s' cannot be used together"),
+		die(_("options '%s' and '%s' cannot be used together"),
 		    "--orphan", "--detach");
 	if (opts.orphan && opt_track)
-		die(_("'%s' and '%s' cannot be used together"), "--orphan", "--track");
+		die(_("options '%s' and '%s' cannot be used together"),
+		    "--orphan", "--track");
 	if (opts.orphan && !opts.checkout)
-		die(_("'%s' and '%s' cannot be used together"), "--orphan",
-		    "--no-checkout");
+		die(_("options '%s' and '%s' cannot be used together"),
+		    "--orphan", "--no-checkout");
 	if (opts.orphan && ac == 2)
 		die(_("'%s' and '%s' cannot be used together"), "--orphan",
 		    _("<commit-ish>"));
diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
index df4aff7825..245656b53a 100755
--- a/t/t2400-worktree-add.sh
+++ b/t/t2400-worktree-add.sh
@@ -711,7 +711,7 @@ test_dwim_orphan () {
 	local fetch_error_text="fatal: No local or remote refs exist despite at least one remote" &&
 	local orphan_hint="hint: If you meant to create a worktree containing a new orphan branch" &&
 	local invalid_ref_regex="^fatal: invalid reference: " &&
-	local bad_combo_regex="^fatal: '[-a-z]*' and '[-a-z]*' cannot be used together" &&
+	local bad_combo_regex="^fatal: options '[-a-z]*' and '[-a-z]*' cannot be used together" &&

 	local git_ns="repo" &&
 	local dashc_args="-C $git_ns" &&
--
2.43.0

[PATCH 7/7] worktree: simplify incompatibility message for --orphan and commit-ish

[René Scharfe]

Use a single translatable string to report that the worktree add option
--orphan is incompatible with a commit-ish instead of having the
commit-ish in a separate translatable string.  This reduces the number
of strings to translate and gives translators the full context.

A similar message is used in builtin/describe.c, but with the plural of
commit-ish, and here we need the singular form.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/worktree.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 036ceaa981..4ac1621541 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -815,8 +815,8 @@ static int add(int ac, const char **av, const char *prefix)
 		die(_("options '%s' and '%s' cannot be used together"),
 		    "--orphan", "--no-checkout");
 	if (opts.orphan && ac == 2)
-		die(_("'%s' and '%s' cannot be used together"), "--orphan",
-		    _("<commit-ish>"));
+		die(_("option '%s' and commit-ish cannot be used together"),
+		    "--orphan");
 	if (lock_reason && !keep_locked)
 		die(_("the option '%s' requires '%s'"), "--reason", "--lock");
 	if (lock_reason)
--
2.43.0

Re: [PATCH 0/7] standardize incompatibility messages

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1489 bytes --]

On Wed, Dec 06, 2023 at 12:51:54PM +0100, René Scharfe wrote:
> More of what a699367bb8 (i18n: factorize more 'incompatible options'
> messages, 2022-01-31) did: Simplify checks for multiple mutually
> exclusive options, reduce the number of strings to translate, improve UI
> consistency a bit.

Thanks for working on this! The patch series looks mostly good to me,
I've only got two questions.

Patrick

>   push: use die_for_incompatible_opt4() for --delete/--tags/--all/--mirror
>   repack: use die_for_incompatible_opt3() for -A/-k/--cruft
>   revision: use die_for_incompatible_opt3() for --graph/--reverse/--walk-reflogs
>   revision, rev-parse: factorize incompatibility messages about --exclude-hidden
>   clean: factorize incompatibility message
>   worktree: standardize incompatibility messages
>   worktree: simplify incompatibility message for --orphan and commit-ish
> 
>  builtin/clean.c                    |  2 +-
>  builtin/push.c                     | 12 ++++--------
>  builtin/repack.c                   | 14 ++++----------
>  builtin/rev-parse.c                |  9 ++++++---
>  builtin/worktree.c                 | 21 +++++++++++----------
>  revision.c                         | 27 +++++++++++++++------------
>  t/t2400-worktree-add.sh            |  2 +-
>  t/t6018-rev-list-glob.sh           |  6 ++----
>  t/t6021-rev-list-exclude-hidden.sh |  4 ++--
>  9 files changed, 46 insertions(+), 51 deletions(-)
> 
> --
> 2.43.0
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/7] revision: use die_for_incompatible_opt3() for --graph/--reverse/--walk-reflogs

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1957 bytes --]

On Wed, Dec 06, 2023 at 12:51:57PM +0100, René Scharfe wrote:
> The revision options --reverse is incompatibel with --walk-reflogs and
> --graph is incompatible with both --reverse and --walk-reflogs.  So they
> are all incompatible with each other.
> 
> Use the function for checking three mutually incompatible options,
> die_for_incompatible_opt3(), to perform this check in one place and
> without repetition.  This is shorter and clearer.
> 
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  revision.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/revision.c b/revision.c
> index b2861474b1..1b7e1af6c6 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -3036,8 +3036,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>  		revs->grep_filter.ignore_locale = 1;
>  	compile_grep_patterns(&revs->grep_filter);
> 
> -	if (revs->reverse && revs->reflog_info)
> -		die(_("options '%s' and '%s' cannot be used together"), "--reverse", "--walk-reflogs");
>  	if (revs->reflog_info && revs->limited)
>  		die("cannot combine --walk-reflogs with history-limiting options");
>  	if (revs->rewrite_parents && revs->children.name)
> @@ -3048,11 +3046,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>  	/*
>  	 * Limitations on the graph functionality
>  	 */
> -	if (revs->reverse && revs->graph)
> -		die(_("options '%s' and '%s' cannot be used together"), "--reverse", "--graph");
> +	die_for_incompatible_opt3(!!revs->graph, "--graph",
> +				  !!revs->reverse, "--reverse",
> +				  !!revs->reflog_info, "--walk-reflogs");

I've been wondering why we use `!!` here, as `die_for_incompatible_*()`
doesn't care for the actual value but only checks that it is non-zero.
Is it because of the type mismatch, where these flags here use unsigned
ints whereas `die_for_incompatible_*()` expect ints?

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/7] revision, rev-parse: factorize incompatibility messages about --exclude-hidden

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1444 bytes --]

On Wed, Dec 06, 2023 at 12:51:58PM +0100, René Scharfe wrote:
> Use the standard parameterized message for reporting incompatible
> options to report options that are not accepted in combination with
> --exclude-hidden.  This reduces the number of strings to translate and
> makes the UI a bit more consistent.
> 
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  builtin/rev-parse.c                |  9 ++++++---
>  revision.c                         | 18 ++++++++++++------
>  t/t6018-rev-list-glob.sh           |  6 ++----
>  t/t6021-rev-list-exclude-hidden.sh |  4 ++--
>  4 files changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index fde8861ca4..917f122440 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -893,13 +893,15 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
>  			}
>  			if (opt_with_value(arg, "--branches", &arg)) {
>  				if (ref_excludes.hidden_refs_configured)
> -					return error(_("--exclude-hidden cannot be used together with --branches"));
> +					return error(_("options '%s' and '%s' cannot be used together"),
> +						     "--exclude-hidden", "--branches");

The repetitive nature of this patch and subsequent ones made me wonder
whether it would be useful to have a function similar to the
`die_for_incompatible_*()` helper that knows to format this error
correctly.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/7] revision: use die_for_incompatible_opt3() for --graph/--reverse/--walk-reflogs

[René Scharfe]

Am 06.12.23 um 14:08 schrieb Patrick Steinhardt:
> On Wed, Dec 06, 2023 at 12:51:57PM +0100, René Scharfe wrote:
>> The revision options --reverse is incompatibel with --walk-reflogs and
>> --graph is incompatible with both --reverse and --walk-reflogs.  So they
>> are all incompatible with each other.
>>
>> Use the function for checking three mutually incompatible options,
>> die_for_incompatible_opt3(), to perform this check in one place and
>> without repetition.  This is shorter and clearer.
>>
>> Signed-off-by: René Scharfe <l.s.r@web.de>
>> ---
>>  revision.c | 9 +++------
>>  1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/revision.c b/revision.c
>> index b2861474b1..1b7e1af6c6 100644
>> --- a/revision.c
>> +++ b/revision.c
>> @@ -3036,8 +3036,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>>  		revs->grep_filter.ignore_locale = 1;
>>  	compile_grep_patterns(&revs->grep_filter);
>>
>> -	if (revs->reverse && revs->reflog_info)
>> -		die(_("options '%s' and '%s' cannot be used together"), "--reverse", "--walk-reflogs");
>>  	if (revs->reflog_info && revs->limited)
>>  		die("cannot combine --walk-reflogs with history-limiting options");
>>  	if (revs->rewrite_parents && revs->children.name)
>> @@ -3048,11 +3046,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>>  	/*
>>  	 * Limitations on the graph functionality
>>  	 */
>> -	if (revs->reverse && revs->graph)
>> -		die(_("options '%s' and '%s' cannot be used together"), "--reverse", "--graph");
>> +	die_for_incompatible_opt3(!!revs->graph, "--graph",
>> +				  !!revs->reverse, "--reverse",
>> +				  !!revs->reflog_info, "--walk-reflogs");
>
> I've been wondering why we use `!!` here, as `die_for_incompatible_*()`
> doesn't care for the actual value but only checks that it is non-zero.
> Is it because of the type mismatch, where these flags here use unsigned
> ints whereas `die_for_incompatible_*()` expect ints?

->graph and ->reflog_info are pointers and clang reports an int-conversion
warning without the double negation.  ->reverse is an unsigned:1 and so
doesn't need it, but I gave it one anyway for aesthetic reasons.

René

Re: [PATCH 4/7] revision, rev-parse: factorize incompatibility messages about --exclude-hidden

[René Scharfe]

Am 06.12.23 um 14:08 schrieb Patrick Steinhardt:
> On Wed, Dec 06, 2023 at 12:51:58PM +0100, René Scharfe wrote:
>> Use the standard parameterized message for reporting incompatible
>> options to report options that are not accepted in combination with
>> --exclude-hidden.  This reduces the number of strings to translate and
>> makes the UI a bit more consistent.
>>
>> Signed-off-by: René Scharfe <l.s.r@web.de>
>> ---
>>  builtin/rev-parse.c                |  9 ++++++---
>>  revision.c                         | 18 ++++++++++++------
>>  t/t6018-rev-list-glob.sh           |  6 ++----
>>  t/t6021-rev-list-exclude-hidden.sh |  4 ++--
>>  4 files changed, 22 insertions(+), 15 deletions(-)
>>
>> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
>> index fde8861ca4..917f122440 100644
>> --- a/builtin/rev-parse.c
>> +++ b/builtin/rev-parse.c
>> @@ -893,13 +893,15 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
>>  			}
>>  			if (opt_with_value(arg, "--branches", &arg)) {
>>  				if (ref_excludes.hidden_refs_configured)
>> -					return error(_("--exclude-hidden cannot be used together with --branches"));
>> +					return error(_("options '%s' and '%s' cannot be used together"),
>> +						     "--exclude-hidden", "--branches");
>
> The repetitive nature of this patch and subsequent ones made me wonder
> whether it would be useful to have a function similar to the
> `die_for_incompatible_*()` helper that knows to format this error
> correctly.

I wondered the same and experimented with a die_for_incompatible_opt2().
It would allow the compiler to detect typos.

Passing in the conditions as parameters is a bit tedious and unlike its
for its higher-numbered siblings there is not much to win by doing that
instead of using an if statement or two nested ones.  We could pass in
1 if we want to integrate that function into an if cascade like above,
but it would look a bit silly.  And here we'd need a non-fatal version
anyway.

Perhaps a build step that lists all new translatable strings would help?
Nah, that would require building each commit.

A LLM-based tool to find translatable strings with the same meaning?
Don't know how difficult that would be.

So I feel the same, but don't have a solution that would justify the
churn of replacing the duplicate strings with function calls. :-/

René

Re: [PATCH 4/7] revision, rev-parse: factorize incompatibility messages about --exclude-hidden

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3187 bytes --]

On Wed, Dec 06, 2023 at 03:21:15PM +0100, René Scharfe wrote:
> Am 06.12.23 um 14:08 schrieb Patrick Steinhardt:
> > On Wed, Dec 06, 2023 at 12:51:58PM +0100, René Scharfe wrote:
> >> Use the standard parameterized message for reporting incompatible
> >> options to report options that are not accepted in combination with
> >> --exclude-hidden.  This reduces the number of strings to translate and
> >> makes the UI a bit more consistent.
> >>
> >> Signed-off-by: René Scharfe <l.s.r@web.de>
> >> ---
> >>  builtin/rev-parse.c                |  9 ++++++---
> >>  revision.c                         | 18 ++++++++++++------
> >>  t/t6018-rev-list-glob.sh           |  6 ++----
> >>  t/t6021-rev-list-exclude-hidden.sh |  4 ++--
> >>  4 files changed, 22 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> >> index fde8861ca4..917f122440 100644
> >> --- a/builtin/rev-parse.c
> >> +++ b/builtin/rev-parse.c
> >> @@ -893,13 +893,15 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> >>  			}
> >>  			if (opt_with_value(arg, "--branches", &arg)) {
> >>  				if (ref_excludes.hidden_refs_configured)
> >> -					return error(_("--exclude-hidden cannot be used together with --branches"));
> >> +					return error(_("options '%s' and '%s' cannot be used together"),
> >> +						     "--exclude-hidden", "--branches");
> >
> > The repetitive nature of this patch and subsequent ones made me wonder
> > whether it would be useful to have a function similar to the
> > `die_for_incompatible_*()` helper that knows to format this error
> > correctly.
> 
> I wondered the same and experimented with a die_for_incompatible_opt2().
> It would allow the compiler to detect typos.
> 
> Passing in the conditions as parameters is a bit tedious and unlike its
> for its higher-numbered siblings there is not much to win by doing that
> instead of using an if statement or two nested ones.  We could pass in
> 1 if we want to integrate that function into an if cascade like above,
> but it would look a bit silly.  And here we'd need a non-fatal version
> anyway.

Maybe the easiest solution would be to have `error_incompatible_usage()`
that simply wraps `error()`. You'd pass in the incompatible params and
it makes sure to format them accordingly. It could either accept two
args or even be a vararg function with sentinel `NULL`. It's not perfect
of course, but would at least ensure that we can easily convert things
over time without having to duplicate the exact message everywhere.

But ultimately I do not mind the current duplication all that much, the
patch series looks good to me even without such a new helper.

> Perhaps a build step that lists all new translatable strings would help?
> Nah, that would require building each commit.
> 
> A LLM-based tool to find translatable strings with the same meaning?
> Don't know how difficult that would be.

I don't think it's a problem to not convert everything in one go. The
current series is a good step in the right direction, and any additional
instances that were missed can be fixed in follow-ups.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/7] revision, rev-parse: factorize incompatibility messages about --exclude-hidden

[René Scharfe]

Am 06.12.23 um 15:39 schrieb Patrick Steinhardt:
> On Wed, Dec 06, 2023 at 03:21:15PM +0100, René Scharfe wrote:
>> Am 06.12.23 um 14:08 schrieb Patrick Steinhardt:
>>> On Wed, Dec 06, 2023 at 12:51:58PM +0100, René Scharfe wrote:
>>>> Use the standard parameterized message for reporting incompatible
>>>> options to report options that are not accepted in combination with
>>>> --exclude-hidden.  This reduces the number of strings to translate and
>>>> makes the UI a bit more consistent.
>>>>
>>>> Signed-off-by: René Scharfe <l.s.r@web.de>
>>>> ---
>>>>  builtin/rev-parse.c                |  9 ++++++---
>>>>  revision.c                         | 18 ++++++++++++------
>>>>  t/t6018-rev-list-glob.sh           |  6 ++----
>>>>  t/t6021-rev-list-exclude-hidden.sh |  4 ++--
>>>>  4 files changed, 22 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
>>>> index fde8861ca4..917f122440 100644
>>>> --- a/builtin/rev-parse.c
>>>> +++ b/builtin/rev-parse.c
>>>> @@ -893,13 +893,15 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
>>>>  			}
>>>>  			if (opt_with_value(arg, "--branches", &arg)) {
>>>>  				if (ref_excludes.hidden_refs_configured)
>>>> -					return error(_("--exclude-hidden cannot be used together with --branches"));
>>>> +					return error(_("options '%s' and '%s' cannot be used together"),
>>>> +						     "--exclude-hidden", "--branches");
>>>
>>> The repetitive nature of this patch and subsequent ones made me wonder
>>> whether it would be useful to have a function similar to the
>>> `die_for_incompatible_*()` helper that knows to format this error
>>> correctly.
>>
>> I wondered the same and experimented with a die_for_incompatible_opt2().
>> It would allow the compiler to detect typos.
>>
>> Passing in the conditions as parameters is a bit tedious and unlike its
>> for its higher-numbered siblings there is not much to win by doing that
>> instead of using an if statement or two nested ones.  We could pass in
>> 1 if we want to integrate that function into an if cascade like above,
>> but it would look a bit silly.  And here we'd need a non-fatal version
>> anyway.
>
> Maybe the easiest solution would be to have `error_incompatible_usage()`
> that simply wraps `error()`.

Yes, but having two variants (die_ and error_) is unfortunate.

> You'd pass in the incompatible params and
> it makes sure to format them accordingly. It could either accept two
> args or even be a vararg function with sentinel `NULL`.

Tempting, but passing the conditions separately is actually useful to
improve the shown message when there are more than two options.

> It's not perfect
> of course, but would at least ensure that we can easily convert things
> over time without having to duplicate the exact message everywhere.

Maybe the simplest option would be to use a macro, e.g.

   #define INCOMPATIBLE_OPTIONS_MESSAGE \
           _("options '%s' and '%s' cannot be used together")

It could be used with both error() and die(), and the compiler would
still ensure that two strings are passed along with it, but I don't know
how to encode that requirement in the macro name somehow to make it
self-documenting.  Perhaps by getting the number two in there?

> I don't think it's a problem to not convert everything in one go. The
> current series is a good step in the right direction, and any additional
> instances that were missed can be fixed in follow-ups.

Right; whatever we do, we can (and should) do it step by step.

René

Re: [PATCH 3/7] revision: use die_for_incompatible_opt3() for --graph/--reverse/--walk-reflogs

[Eric Sunshine]

On Wed, Dec 6, 2023 at 6:53 AM René Scharfe <l.s.r@web.de> wrote:
> The revision options --reverse is incompatibel with --walk-reflogs and

s/incompatibel/incompatible/

> --graph is incompatible with both --reverse and --walk-reflogs.  So they
> are all incompatible with each other.
>
> Use the function for checking three mutually incompatible options,
> die_for_incompatible_opt3(), to perform this check in one place and
> without repetition.  This is shorter and clearer.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>

Re: [PATCH 3/7] revision: use die_for_incompatible_opt3() for --graph/--reverse/--walk-reflogs

[René Scharfe]

Am 06.12.23 um 18:21 schrieb Eric Sunshine:
> On Wed, Dec 6, 2023 at 6:53 AM René Scharfe <l.s.r@web.de> wrote:
>> The revision options --reverse is incompatibel with --walk-reflogs and
>
> s/incompatibel/incompatible/

And s/options/option/, *sigh*.

>
>> --graph is incompatible with both --reverse and --walk-reflogs.  So they
>> are all incompatible with each other.
>>
>> Use the function for checking three mutually incompatible options,
>> die_for_incompatible_opt3(), to perform this check in one place and
>> without repetition.  This is shorter and clearer.
>>
>> Signed-off-by: René Scharfe <l.s.r@web.de>

Re: [PATCH 2/7] repack: use die_for_incompatible_opt3() for -A/-k/--cruft

[Taylor Blau]

On Wed, Dec 06, 2023 at 12:51:56PM +0100, René Scharfe wrote:
> diff --git a/builtin/repack.c b/builtin/repack.c
> index edaee4dbec..c54777bbe5 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -1203,19 +1203,13 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  	if (delete_redundant && repository_format_precious_objects)
>  		die(_("cannot delete packs in a precious-objects repo"));
>
> -	if (keep_unreachable &&
> -	    (unpack_unreachable || (pack_everything & LOOSEN_UNREACHABLE)))
> -		die(_("options '%s' and '%s' cannot be used together"), "--keep-unreachable", "-A");
> +	die_for_incompatible_opt3(unpack_unreachable || (pack_everything & LOOSEN_UNREACHABLE), "-A",
> +				  keep_unreachable, "-k/--keep-unreachable",
> +				  pack_everything & PACK_CRUFT, "--cruft");

Oof, thanks for cleaning this one up.

I had to read this hunk a couple of times to convince myself that it was
doing the right thing, but I believe it is.

> -	if (pack_everything & PACK_CRUFT) {
> +	if (pack_everything & PACK_CRUFT)
>  		pack_everything |= ALL_INTO_ONE;

And adding the ALL_INTO_ONE bit here does not effect either of the below
two checks, so moving them up is fine, too.

> -		if (unpack_unreachable || (pack_everything & LOOSEN_UNREACHABLE))
> -			die(_("options '%s' and '%s' cannot be used together"), "--cruft", "-A");
> -		if (keep_unreachable)
> -			die(_("options '%s' and '%s' cannot be used together"), "--cruft", "-k");
> -	}

Thanks,
Taylor

Re: [PATCH 4/7] revision, rev-parse: factorize incompatibility messages about --exclude-hidden

[Taylor Blau]

On Wed, Dec 06, 2023 at 06:07:29PM +0100, René Scharfe wrote:
> > It's not perfect
> > of course, but would at least ensure that we can easily convert things
> > over time without having to duplicate the exact message everywhere.
>
> Maybe the simplest option would be to use a macro, e.g.
>
>    #define INCOMPATIBLE_OPTIONS_MESSAGE \
>            _("options '%s' and '%s' cannot be used together")
>
> It could be used with both error() and die(), and the compiler would
> still ensure that two strings are passed along with it, but I don't know
> how to encode that requirement in the macro name somehow to make it
> self-documenting.  Perhaps by getting the number two in there?

I think that this is a great idea. It nicely solves Patrick's concern
that we have to duplicate this message ID everywhere, and equally solves
yours by calling error() inline instead of having to pass down the
option values.

I think that including a number in the macro name would be helpful here.

> > I don't think it's a problem to not convert everything in one go. The
> > current series is a good step in the right direction, and any additional
> > instances that were missed can be fixed in follow-ups.
>
> Right; whatever we do, we can (and should) do it step by step.

I agree :-).

Thanks,
Taylor

Re: [PATCH 4/7] revision, rev-parse: factorize incompatibility messages about --exclude-hidden

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1328 bytes --]

On Wed, Dec 06, 2023 at 02:25:01PM -0500, Taylor Blau wrote:
> On Wed, Dec 06, 2023 at 06:07:29PM +0100, René Scharfe wrote:
> > > It's not perfect
> > > of course, but would at least ensure that we can easily convert things
> > > over time without having to duplicate the exact message everywhere.
> >
> > Maybe the simplest option would be to use a macro, e.g.
> >
> >    #define INCOMPATIBLE_OPTIONS_MESSAGE \
> >            _("options '%s' and '%s' cannot be used together")
> >
> > It could be used with both error() and die(), and the compiler would
> > still ensure that two strings are passed along with it, but I don't know
> > how to encode that requirement in the macro name somehow to make it
> > self-documenting.  Perhaps by getting the number two in there?
> 
> I think that this is a great idea. It nicely solves Patrick's concern
> that we have to duplicate this message ID everywhere, and equally solves
> yours by calling error() inline instead of having to pass down the
> option values.
> 
> I think that including a number in the macro name would be helpful here.

Does our i18n tooling know how to extract such messages defined in
macros? I have to admit I don't really know how it works under the hood.
But if it does work then this looks like a good solution to me.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]