Am 06.12.23 um 15:39 schrieb Patrick Steinhardt:

On Wed, Dec 06, 2023 at 03:21:15PM +0100, René Scharfe wrote:

quoted

Am 06.12.23 um 14:08 schrieb Patrick Steinhardt:

quoted

On Wed, Dec 06, 2023 at 12:51:58PM +0100, René Scharfe wrote:

quoted

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 <redacted>
---
 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é