{"thread":{"id":"60586","subject":"[PATCH 3/7] revision: use die_for_incompatible_opt3() for --graph/--reverse/--walk-reflogs","startedAt":"2023-12-06T11:52:19Z","lastAt":"2023-12-07T07:10:10Z","messageCount":20,"participants":["René Scharfe","Patrick Steinhardt","Eric Sunshine","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"485362","messageId":"20231206115215.94467-4-l.s.r@web.de","threadId":"60586","inReplyTo":"20231206115215.94467-1-l.s.r@web.de","subject":"[PATCH 3/7] revision: use die_for_incompatible_opt3() for --graph/--reverse/--walk-reflogs","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-12-06T11:51:57Z","receivedAt":"2023-12-06T11:52:19Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The revision options --reverse is incompatibel with --walk-reflogs and\n--graph is incompatible with both --reverse and --walk-reflogs.  So they\nare all incompatible with each other.\n\nUse the function for checking three mutually incompatible options,\ndie_for_incompatible_opt3(), to perform this check in one place and\nwithout repetition.  This is shorter and clearer.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n revision.c | 9 +++------\n 1 file changed, 3 insertions(+), 6 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex b2861474b1..1b7e1af6c6 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -3036,8 +3036,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\trevs->grep_filter.ignore_locale = 1;\n \tcompile_grep_patterns(&revs->grep_filter);\n\n-\tif (revs->reverse && revs->reflog_info)\n-\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--reverse\", \"--walk-reflogs\");\n \tif (revs->reflog_info && revs->limited)\n \t\tdie(\"cannot combine --walk-reflogs with history-limiting options\");\n \tif (revs->rewrite_parents && revs->children.name)\n@@ -3048,11 +3046,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t/*\n \t * Limitations on the graph functionality\n \t */\n-\tif (revs->reverse && revs->graph)\n-\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--reverse\", \"--graph\");\n+\tdie_for_incompatible_opt3(!!revs->graph, \"--graph\",\n+\t\t\t\t  !!revs->reverse, \"--reverse\",\n+\t\t\t\t  !!revs->reflog_info, \"--walk-reflogs\");\n\n-\tif (revs->reflog_info && revs->graph)\n-\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--walk-reflogs\", \"--graph\");\n \tif (revs->no_walk && revs->graph)\n \t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--no-walk\", \"--graph\");\n \tif (!revs->reflog_info && revs->grep_filter.use_reflog_filter)\n--\n2.43.0\n\n"},{"id":"485363","messageId":"20231206115215.94467-5-l.s.r@web.de","threadId":"60586","inReplyTo":"20231206115215.94467-1-l.s.r@web.de","subject":"[PATCH 4/7] revision, rev-parse: factorize incompatibility messages about --exclude-hidden","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-12-06T11:51:58Z","receivedAt":"2023-12-06T11:52:19Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Use the standard parameterized message for reporting incompatible\noptions to report options that are not accepted in combination with\n--exclude-hidden.  This reduces the number of strings to translate and\nmakes the UI a bit more consistent.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n builtin/rev-parse.c                |  9 ++++++---\n revision.c                         | 18 ++++++++++++------\n t/t6018-rev-list-glob.sh           |  6 ++----\n t/t6021-rev-list-exclude-hidden.sh |  4 ++--\n 4 files changed, 22 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex fde8861ca4..917f122440 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -893,13 +893,15 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t}\n \t\t\tif (opt_with_value(arg, \"--branches\", &arg)) {\n \t\t\t\tif (ref_excludes.hidden_refs_configured)\n-\t\t\t\t\treturn error(_(\"--exclude-hidden cannot be used together with --branches\"));\n+\t\t\t\t\treturn error(_(\"options '%s' and '%s' cannot be used together\"),\n+\t\t\t\t\t\t     \"--exclude-hidden\", \"--branches\");\n \t\t\t\thandle_ref_opt(arg, \"refs/heads/\");\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (opt_with_value(arg, \"--tags\", &arg)) {\n \t\t\t\tif (ref_excludes.hidden_refs_configured)\n-\t\t\t\t\treturn error(_(\"--exclude-hidden cannot be used together with --tags\"));\n+\t\t\t\t\treturn error(_(\"options '%s' and '%s' cannot be used together\"),\n+\t\t\t\t\t\t     \"--exclude-hidden\", \"--tags\");\n \t\t\t\thandle_ref_opt(arg, \"refs/tags/\");\n \t\t\t\tcontinue;\n \t\t\t}\n@@ -909,7 +911,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t}\n \t\t\tif (opt_with_value(arg, \"--remotes\", &arg)) {\n \t\t\t\tif (ref_excludes.hidden_refs_configured)\n-\t\t\t\t\treturn error(_(\"--exclude-hidden cannot be used together with --remotes\"));\n+\t\t\t\t\treturn error(_(\"options '%s' and '%s' cannot be used together\"),\n+\t\t\t\t\t\t     \"--exclude-hidden\", \"--remotes\");\n \t\t\t\thandle_ref_opt(arg, \"refs/remotes/\");\n \t\t\t\tcontinue;\n \t\t\t}\ndiff --git a/revision.c b/revision.c\nindex 1b7e1af6c6..25335a74ad 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2709,7 +2709,8 @@ static int handle_revision_pseudo_opt(struct rev_info *revs,\n \t\tclear_ref_exclusions(&revs->ref_excludes);\n \t} else if (!strcmp(arg, \"--branches\")) {\n \t\tif (revs->ref_excludes.hidden_refs_configured)\n-\t\t\treturn error(_(\"--exclude-hidden cannot be used together with --branches\"));\n+\t\t\treturn error(_(\"options '%s' and '%s' cannot be used together\"),\n+\t\t\t\t     \"--exclude-hidden\", \"--branches\");\n \t\thandle_refs(refs, revs, *flags, refs_for_each_branch_ref);\n \t\tclear_ref_exclusions(&revs->ref_excludes);\n \t} else if (!strcmp(arg, \"--bisect\")) {\n@@ -2720,12 +2721,14 @@ static int handle_revision_pseudo_opt(struct rev_info *revs,\n \t\trevs->bisect = 1;\n \t} else if (!strcmp(arg, \"--tags\")) {\n \t\tif (revs->ref_excludes.hidden_refs_configured)\n-\t\t\treturn error(_(\"--exclude-hidden cannot be used together with --tags\"));\n+\t\t\treturn error(_(\"options '%s' and '%s' cannot be used together\"),\n+\t\t\t\t     \"--exclude-hidden\", \"--tags\");\n \t\thandle_refs(refs, revs, *flags, refs_for_each_tag_ref);\n \t\tclear_ref_exclusions(&revs->ref_excludes);\n \t} else if (!strcmp(arg, \"--remotes\")) {\n \t\tif (revs->ref_excludes.hidden_refs_configured)\n-\t\t\treturn error(_(\"--exclude-hidden cannot be used together with --remotes\"));\n+\t\t\treturn error(_(\"options '%s' and '%s' cannot be used together\"),\n+\t\t\t\t     \"--exclude-hidden\", \"--remotes\");\n \t\thandle_refs(refs, revs, *flags, refs_for_each_remote_ref);\n \t\tclear_ref_exclusions(&revs->ref_excludes);\n \t} else if ((argcount = parse_long_opt(\"glob\", argv, &optarg))) {\n@@ -2743,21 +2746,24 @@ static int handle_revision_pseudo_opt(struct rev_info *revs,\n \t} else if (skip_prefix(arg, \"--branches=\", &optarg)) {\n \t\tstruct all_refs_cb cb;\n \t\tif (revs->ref_excludes.hidden_refs_configured)\n-\t\t\treturn error(_(\"--exclude-hidden cannot be used together with --branches\"));\n+\t\t\treturn error(_(\"options '%s' and '%s' cannot be used together\"),\n+\t\t\t\t     \"--exclude-hidden\", \"--branches\");\n \t\tinit_all_refs_cb(&cb, revs, *flags);\n \t\tfor_each_glob_ref_in(handle_one_ref, optarg, \"refs/heads/\", &cb);\n \t\tclear_ref_exclusions(&revs->ref_excludes);\n \t} else if (skip_prefix(arg, \"--tags=\", &optarg)) {\n \t\tstruct all_refs_cb cb;\n \t\tif (revs->ref_excludes.hidden_refs_configured)\n-\t\t\treturn error(_(\"--exclude-hidden cannot be used together with --tags\"));\n+\t\t\treturn error(_(\"options '%s' and '%s' cannot be used together\"),\n+\t\t\t\t     \"--exclude-hidden\", \"--tags\");\n \t\tinit_all_refs_cb(&cb, revs, *flags);\n \t\tfor_each_glob_ref_in(handle_one_ref, optarg, \"refs/tags/\", &cb);\n \t\tclear_ref_exclusions(&revs->ref_excludes);\n \t} else if (skip_prefix(arg, \"--remotes=\", &optarg)) {\n \t\tstruct all_refs_cb cb;\n \t\tif (revs->ref_excludes.hidden_refs_configured)\n-\t\t\treturn error(_(\"--exclude-hidden cannot be used together with --remotes\"));\n+\t\t\treturn error(_(\"options '%s' and '%s' cannot be used together\"),\n+\t\t\t\t     \"--exclude-hidden\", \"--remotes\");\n \t\tinit_all_refs_cb(&cb, revs, *flags);\n \t\tfor_each_glob_ref_in(handle_one_ref, optarg, \"refs/remotes/\", &cb);\n \t\tclear_ref_exclusions(&revs->ref_excludes);\ndiff --git a/t/t6018-rev-list-glob.sh b/t/t6018-rev-list-glob.sh\nindex 67d523d405..3b181f771c 100755\n--- a/t/t6018-rev-list-glob.sh\n+++ b/t/t6018-rev-list-glob.sh\n@@ -214,15 +214,13 @@ do\n \tfor pseudoopt in branches tags remotes\n \tdo\n \t\ttest_expect_success \"rev-parse --exclude-hidden=$section fails with --$pseudoopt\" '\n-\t\t\techo \"error: --exclude-hidden cannot be used together with --$pseudoopt\" >expected &&\n \t\t\ttest_must_fail git rev-parse --exclude-hidden=$section --$pseudoopt 2>err &&\n-\t\t\ttest_cmp expected err\n+\t\t\ttest_grep \"error: options .--exclude-hidden. and .--$pseudoopt. cannot be used together\" err\n \t\t'\n\n \t\ttest_expect_success \"rev-parse --exclude-hidden=$section fails with --$pseudoopt=pattern\" '\n-\t\t\techo \"error: --exclude-hidden cannot be used together with --$pseudoopt\" >expected &&\n \t\t\ttest_must_fail git rev-parse --exclude-hidden=$section --$pseudoopt=pattern 2>err &&\n-\t\t\ttest_cmp expected err\n+\t\t\ttest_grep \"error: options .--exclude-hidden. and .--$pseudoopt. cannot be used together\" err\n \t\t'\n \tdone\n done\ndiff --git a/t/t6021-rev-list-exclude-hidden.sh b/t/t6021-rev-list-exclude-hidden.sh\nindex cdf7aa9427..51df02105d 100755\n--- a/t/t6021-rev-list-exclude-hidden.sh\n+++ b/t/t6021-rev-list-exclude-hidden.sh\n@@ -151,12 +151,12 @@ do\n \tdo\n \t\ttest_expect_success \"$section: fails with --$pseudoopt\" '\n \t\t\ttest_must_fail git rev-list --exclude-hidden=$section --$pseudoopt 2>err &&\n-\t\t\ttest_grep \"error: --exclude-hidden cannot be used together with --$pseudoopt\" err\n+\t\t\ttest_grep \"error: options .--exclude-hidden. and .--$pseudoopt. cannot be used together\" err\n \t\t'\n\n \t\ttest_expect_success \"$section: fails with --$pseudoopt=pattern\" '\n \t\t\ttest_must_fail git rev-list --exclude-hidden=$section --$pseudoopt=pattern 2>err &&\n-\t\t\ttest_grep \"error: --exclude-hidden cannot be used together with --$pseudoopt\" err\n+\t\t\ttest_grep \"error: options .--exclude-hidden. and .--$pseudoopt. cannot be used together\" err\n \t\t'\n \tdone\n done\n--\n2.43.0\n\n"},{"id":"485366","messageId":"20231206115215.94467-2-l.s.r@web.de","threadId":"60586","inReplyTo":"20231206115215.94467-1-l.s.r@web.de","subject":"[PATCH 1/7] push: use die_for_incompatible_opt4() for --delete/--tags/--all/--mirror","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-12-06T11:51:55Z","receivedAt":"2023-12-06T11:52:19Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The push option --delete is incompatible with --all, --mirror, and\n--tags; --tags is incompatible with --all and --mirror; --all is\nincompatible with --mirror.  This means they are all incompatible with\neach other.  And --branches is an alias for --all.\n\nUse the function for checking four mutually incompatible options,\ndie_for_incompatible_opt4(), to perform this check in one place and\nwithout repetition.  This is shorter and clearer.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n builtin/push.c | 12 ++++--------\n 1 file changed, 4 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 2e708383c2..f77f424324 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -639,8 +639,10 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \t\t: &push_options_config);\n \tset_push_cert_flags(&flags, push_cert);\n\n-\tif (deleterefs && (tags || (flags & (TRANSPORT_PUSH_ALL | TRANSPORT_PUSH_MIRROR))))\n-\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--delete\", \"--all/--branches/--mirror/--tags\");\n+\tdie_for_incompatible_opt4(deleterefs, \"--delete\",\n+\t\t\t\t  tags, \"--tags\",\n+\t\t\t\t  flags & TRANSPORT_PUSH_ALL, \"--all/--branches\",\n+\t\t\t\t  flags & TRANSPORT_PUSH_MIRROR, \"--mirror\");\n \tif (deleterefs && argc < 2)\n \t\tdie(_(\"--delete doesn't make sense without any refs\"));\n\n@@ -677,19 +679,13 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \t\tflags |= (TRANSPORT_PUSH_MIRROR|TRANSPORT_PUSH_FORCE);\n\n \tif (flags & TRANSPORT_PUSH_ALL) {\n-\t\tif (tags)\n-\t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--all\", \"--tags\");\n \t\tif (argc >= 2)\n \t\t\tdie(_(\"--all can't be combined with refspecs\"));\n \t}\n \tif (flags & TRANSPORT_PUSH_MIRROR) {\n-\t\tif (tags)\n-\t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--mirror\", \"--tags\");\n \t\tif (argc >= 2)\n \t\t\tdie(_(\"--mirror can't be combined with refspecs\"));\n \t}\n-\tif ((flags & TRANSPORT_PUSH_ALL) && (flags & TRANSPORT_PUSH_MIRROR))\n-\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--all\", \"--mirror\");\n\n \tif (!is_empty_cas(&cas) && (flags & TRANSPORT_PUSH_FORCE_IF_INCLUDES))\n \t\tcas.use_force_if_includes = 1;\n--\n2.43.0\n\n"},{"id":"485367","messageId":"20231206115215.94467-1-l.s.r@web.de","threadId":"60586","inReplyTo":null,"subject":"[PATCH 0/7] standardize incompatibility messages","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-12-06T11:51:54Z","receivedAt":"2023-12-06T11:52:19Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"More of what a699367bb8 (i18n: factorize more 'incompatible options'\nmessages, 2022-01-31) did: Simplify checks for multiple mutually\nexclusive options, reduce the number of strings to translate, improve UI\nconsistency a bit.\n\n  push: use die_for_incompatible_opt4() for --delete/--tags/--all/--mirror\n  repack: use die_for_incompatible_opt3() for -A/-k/--cruft\n  revision: use die_for_incompatible_opt3() for --graph/--reverse/--walk-reflogs\n  revision, rev-parse: factorize incompatibility messages about --exclude-hidden\n  clean: factorize incompatibility message\n  worktree: standardize incompatibility messages\n  worktree: simplify incompatibility message for --orphan and commit-ish\n\n builtin/clean.c                    |  2 +-\n builtin/push.c                     | 12 ++++--------\n builtin/repack.c                   | 14 ++++----------\n builtin/rev-parse.c                |  9 ++++++---\n builtin/worktree.c                 | 21 +++++++++++----------\n revision.c                         | 27 +++++++++++++++------------\n t/t2400-worktree-add.sh            |  2 +-\n t/t6018-rev-list-glob.sh           |  6 ++----\n t/t6021-rev-list-exclude-hidden.sh |  4 ++--\n 9 files changed, 46 insertions(+), 51 deletions(-)\n\n--\n2.43.0\n\n"},{"id":"485368","messageId":"20231206115215.94467-3-l.s.r@web.de","threadId":"60586","inReplyTo":"20231206115215.94467-1-l.s.r@web.de","subject":"[PATCH 2/7] repack: use die_for_incompatible_opt3() for -A/-k/--cruft","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-12-06T11:51:56Z","receivedAt":"2023-12-06T11:52:19Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The repack option --keep-unreachable is incompatible with -A, --cruft is\nincompatible with -A and -k, and -k is short for --keep-unreachable.  So\nthey are all incompatible with each other.\n\nUse the function for checking three mutually incompatible options,\ndie_for_incompatible_opt3(), to perform this check in one place and\nwithout repetition.  This is shorter and clearer.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n builtin/repack.c | 14 ++++----------\n 1 file changed, 4 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex edaee4dbec..c54777bbe5 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -1203,19 +1203,13 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tif (delete_redundant && repository_format_precious_objects)\n \t\tdie(_(\"cannot delete packs in a precious-objects repo\"));\n\n-\tif (keep_unreachable &&\n-\t    (unpack_unreachable || (pack_everything & LOOSEN_UNREACHABLE)))\n-\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--keep-unreachable\", \"-A\");\n+\tdie_for_incompatible_opt3(unpack_unreachable || (pack_everything & LOOSEN_UNREACHABLE), \"-A\",\n+\t\t\t\t  keep_unreachable, \"-k/--keep-unreachable\",\n+\t\t\t\t  pack_everything & PACK_CRUFT, \"--cruft\");\n\n-\tif (pack_everything & PACK_CRUFT) {\n+\tif (pack_everything & PACK_CRUFT)\n \t\tpack_everything |= ALL_INTO_ONE;\n\n-\t\tif (unpack_unreachable || (pack_everything & LOOSEN_UNREACHABLE))\n-\t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--cruft\", \"-A\");\n-\t\tif (keep_unreachable)\n-\t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--cruft\", \"-k\");\n-\t}\n-\n \tif (write_bitmaps < 0) {\n \t\tif (!write_midx &&\n \t\t    (!(pack_everything & ALL_INTO_ONE) || !is_bare_repository()))\n--\n2.43.0\n\n"},{"id":"485369","messageId":"20231206115215.94467-6-l.s.r@web.de","threadId":"60586","inReplyTo":"20231206115215.94467-1-l.s.r@web.de","subject":"[PATCH 5/7] clean: factorize incompatibility message","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-12-06T11:51:59Z","receivedAt":"2023-12-06T11:52:19Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Use the standard parameterized message for reporting incompatible\noptions to inform users that they can't use -x and -X together.  This\nreduces the number of strings to translate and makes the UI slightly\nmore consistent.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n builtin/clean.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 49c224e626..d90766cad3 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -971,7 +971,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \tdir.flags |= DIR_SHOW_OTHER_DIRECTORIES;\n\n \tif (ignored && ignored_only)\n-\t\tdie(_(\"-x and -X cannot be used together\"));\n+\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"-x\", \"-X\");\n \tif (!ignored)\n \t\tsetup_standard_excludes(&dir);\n \tif (ignored_only)\n--\n2.43.0\n\n"},{"id":"485364","messageId":"20231206115215.94467-8-l.s.r@web.de","threadId":"60586","inReplyTo":"20231206115215.94467-1-l.s.r@web.de","subject":"[PATCH 7/7] worktree: simplify incompatibility message for --orphan and commit-ish","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-12-06T11:52:01Z","receivedAt":"2023-12-06T11:52:20Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Use a single translatable string to report that the worktree add option\n--orphan is incompatible with a commit-ish instead of having the\ncommit-ish in a separate translatable string.  This reduces the number\nof strings to translate and gives translators the full context.\n\nA similar message is used in builtin/describe.c, but with the plural of\ncommit-ish, and here we need the singular form.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n builtin/worktree.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 036ceaa981..4ac1621541 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -815,8 +815,8 @@ static int add(int ac, const char **av, const char *prefix)\n \t\tdie(_(\"options '%s' and '%s' cannot be used together\"),\n \t\t    \"--orphan\", \"--no-checkout\");\n \tif (opts.orphan && ac == 2)\n-\t\tdie(_(\"'%s' and '%s' cannot be used together\"), \"--orphan\",\n-\t\t    _(\"<commit-ish>\"));\n+\t\tdie(_(\"option '%s' and commit-ish cannot be used together\"),\n+\t\t    \"--orphan\");\n \tif (lock_reason && !keep_locked)\n \t\tdie(_(\"the option '%s' requires '%s'\"), \"--reason\", \"--lock\");\n \tif (lock_reason)\n--\n2.43.0\n\n"},{"id":"485365","messageId":"20231206115215.94467-7-l.s.r@web.de","threadId":"60586","inReplyTo":"20231206115215.94467-1-l.s.r@web.de","subject":"[PATCH 6/7] worktree: standardize incompatibility messages","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-12-06T11:52:00Z","receivedAt":"2023-12-06T11:52:20Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Use the standard parameterized message for reporting incompatible\noptions for worktree add.  This reduces the number of strings to\ntranslate and makes the UI slightly more consistent.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n builtin/worktree.c      | 17 +++++++++--------\n t/t2400-worktree-add.sh |  2 +-\n 2 files changed, 10 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 62b7e26f4b..036ceaa981 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -730,11 +730,11 @@ static int dwim_orphan(const struct add_opts *opts, int opt_track, int remote)\n \t}\n\n \tif (opt_track) {\n-\t\tdie(_(\"'%s' and '%s' cannot be used together\"), \"--orphan\",\n-\t\t    \"--track\");\n+\t\tdie(_(\"options '%s' and '%s' cannot be used together\"),\n+\t\t    \"--orphan\", \"--track\");\n \t} else if (!opts->checkout) {\n-\t\tdie(_(\"'%s' and '%s' cannot be used together\"), \"--orphan\",\n-\t\t    \"--no-checkout\");\n+\t\tdie(_(\"options '%s' and '%s' cannot be used together\"),\n+\t\t    \"--orphan\", \"--no-checkout\");\n \t}\n \treturn 1;\n }\n@@ -806,13 +806,14 @@ static int add(int ac, const char **av, const char *prefix)\n \tif (!!opts.detach + !!new_branch + !!new_branch_force > 1)\n \t\tdie(_(\"options '%s', '%s', and '%s' cannot be used together\"), \"-b\", \"-B\", \"--detach\");\n \tif (opts.detach && opts.orphan)\n-\t\tdie(_(\"options '%s', and '%s' cannot be used together\"),\n+\t\tdie(_(\"options '%s' and '%s' cannot be used together\"),\n \t\t    \"--orphan\", \"--detach\");\n \tif (opts.orphan && opt_track)\n-\t\tdie(_(\"'%s' and '%s' cannot be used together\"), \"--orphan\", \"--track\");\n+\t\tdie(_(\"options '%s' and '%s' cannot be used together\"),\n+\t\t    \"--orphan\", \"--track\");\n \tif (opts.orphan && !opts.checkout)\n-\t\tdie(_(\"'%s' and '%s' cannot be used together\"), \"--orphan\",\n-\t\t    \"--no-checkout\");\n+\t\tdie(_(\"options '%s' and '%s' cannot be used together\"),\n+\t\t    \"--orphan\", \"--no-checkout\");\n \tif (opts.orphan && ac == 2)\n \t\tdie(_(\"'%s' and '%s' cannot be used together\"), \"--orphan\",\n \t\t    _(\"<commit-ish>\"));\ndiff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh\nindex df4aff7825..245656b53a 100755\n--- a/t/t2400-worktree-add.sh\n+++ b/t/t2400-worktree-add.sh\n@@ -711,7 +711,7 @@ test_dwim_orphan () {\n \tlocal fetch_error_text=\"fatal: No local or remote refs exist despite at least one remote\" &&\n \tlocal orphan_hint=\"hint: If you meant to create a worktree containing a new orphan branch\" &&\n \tlocal invalid_ref_regex=\"^fatal: invalid reference: \" &&\n-\tlocal bad_combo_regex=\"^fatal: '[-a-z]*' and '[-a-z]*' cannot be used together\" &&\n+\tlocal bad_combo_regex=\"^fatal: options '[-a-z]*' and '[-a-z]*' cannot be used together\" &&\n\n \tlocal git_ns=\"repo\" &&\n \tlocal dashc_args=\"-C $git_ns\" &&\n--\n2.43.0\n\n"},{"id":"485378","messageId":"ZXByLkAtp_J0nGa0@tanuki","threadId":"60586","inReplyTo":"20231206115215.94467-1-l.s.r@web.de","subject":"Re: [PATCH 0/7] standardize incompatibility messages","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-06T13:07:58Z","receivedAt":"2023-12-06T13:08:03Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Dec 06, 2023 at 12:51:54PM +0100, René Scharfe wrote:\n> More of what a699367bb8 (i18n: factorize more 'incompatible options'\n> messages, 2022-01-31) did: Simplify checks for multiple mutually\n> exclusive options, reduce the number of strings to translate, improve UI\n> consistency a bit.\n\nThanks for working on this! The patch series looks mostly good to me,\nI've only got two questions.\n\nPatrick\n\n>   push: use die_for_incompatible_opt4() for --delete/--tags/--all/--mirror\n>   repack: use die_for_incompatible_opt3() for -A/-k/--cruft\n>   revision: use die_for_incompatible_opt3() for --graph/--reverse/--walk-reflogs\n>   revision, rev-parse: factorize incompatibility messages about --exclude-hidden\n>   clean: factorize incompatibility message\n>   worktree: standardize incompatibility messages\n>   worktree: simplify incompatibility message for --orphan and commit-ish\n> \n>  builtin/clean.c                    |  2 +-\n>  builtin/push.c                     | 12 ++++--------\n>  builtin/repack.c                   | 14 ++++----------\n>  builtin/rev-parse.c                |  9 ++++++---\n>  builtin/worktree.c                 | 21 +++++++++++----------\n>  revision.c                         | 27 +++++++++++++++------------\n>  t/t2400-worktree-add.sh            |  2 +-\n>  t/t6018-rev-list-glob.sh           |  6 ++----\n>  t/t6021-rev-list-exclude-hidden.sh |  4 ++--\n>  9 files changed, 46 insertions(+), 51 deletions(-)\n> \n> --\n> 2.43.0\n> \n> \n"},{"id":"485379","messageId":"ZXByNGuQTaOQ3sKW@tanuki","threadId":"60586","inReplyTo":"20231206115215.94467-4-l.s.r@web.de","subject":"Re: [PATCH 3/7] revision: use die_for_incompatible_opt3() for --graph/--reverse/--walk-reflogs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-06T13:08:04Z","receivedAt":"2023-12-06T13:08:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Dec 06, 2023 at 12:51:57PM +0100, René Scharfe wrote:\n> The revision options --reverse is incompatibel with --walk-reflogs and\n> --graph is incompatible with both --reverse and --walk-reflogs.  So they\n> are all incompatible with each other.\n> \n> Use the function for checking three mutually incompatible options,\n> die_for_incompatible_opt3(), to perform this check in one place and\n> without repetition.  This is shorter and clearer.\n> \n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n>  revision.c | 9 +++------\n>  1 file changed, 3 insertions(+), 6 deletions(-)\n> \n> diff --git a/revision.c b/revision.c\n> index b2861474b1..1b7e1af6c6 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -3036,8 +3036,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n>  \t\trevs->grep_filter.ignore_locale = 1;\n>  \tcompile_grep_patterns(&revs->grep_filter);\n> \n> -\tif (revs->reverse && revs->reflog_info)\n> -\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--reverse\", \"--walk-reflogs\");\n>  \tif (revs->reflog_info && revs->limited)\n>  \t\tdie(\"cannot combine --walk-reflogs with history-limiting options\");\n>  \tif (revs->rewrite_parents && revs->children.name)\n> @@ -3048,11 +3046,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n>  \t/*\n>  \t * Limitations on the graph functionality\n>  \t */\n> -\tif (revs->reverse && revs->graph)\n> -\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--reverse\", \"--graph\");\n> +\tdie_for_incompatible_opt3(!!revs->graph, \"--graph\",\n> +\t\t\t\t  !!revs->reverse, \"--reverse\",\n> +\t\t\t\t  !!revs->reflog_info, \"--walk-reflogs\");\n\nI've been wondering why we use `!!` here, as `die_for_incompatible_*()`\ndoesn't care for the actual value but only checks that it is non-zero.\nIs it because of the type mismatch, where these flags here use unsigned\nints whereas `die_for_incompatible_*()` expect ints?\n\nPatrick\n"},{"id":"485380","messageId":"ZXByOXWo6cIy71s2@tanuki","threadId":"60586","inReplyTo":"20231206115215.94467-5-l.s.r@web.de","subject":"Re: [PATCH 4/7] revision, rev-parse: factorize incompatibility messages about --exclude-hidden","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-06T13:08:09Z","receivedAt":"2023-12-06T13:08:12Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Dec 06, 2023 at 12:51:58PM +0100, René Scharfe wrote:\n> Use the standard parameterized message for reporting incompatible\n> options to report options that are not accepted in combination with\n> --exclude-hidden.  This reduces the number of strings to translate and\n> makes the UI a bit more consistent.\n> \n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n>  builtin/rev-parse.c                |  9 ++++++---\n>  revision.c                         | 18 ++++++++++++------\n>  t/t6018-rev-list-glob.sh           |  6 ++----\n>  t/t6021-rev-list-exclude-hidden.sh |  4 ++--\n>  4 files changed, 22 insertions(+), 15 deletions(-)\n> \n> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\n> index fde8861ca4..917f122440 100644\n> --- a/builtin/rev-parse.c\n> +++ b/builtin/rev-parse.c\n> @@ -893,13 +893,15 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n>  \t\t\t}\n>  \t\t\tif (opt_with_value(arg, \"--branches\", &arg)) {\n>  \t\t\t\tif (ref_excludes.hidden_refs_configured)\n> -\t\t\t\t\treturn error(_(\"--exclude-hidden cannot be used together with --branches\"));\n> +\t\t\t\t\treturn error(_(\"options '%s' and '%s' cannot be used together\"),\n> +\t\t\t\t\t\t     \"--exclude-hidden\", \"--branches\");\n\nThe repetitive nature of this patch and subsequent ones made me wonder\nwhether it would be useful to have a function similar to the\n`die_for_incompatible_*()` helper that knows to format this error\ncorrectly.\n\nPatrick\n"},{"id":"485382","messageId":"382fac73-1afc-4b89-ad0f-3320c7a64838@web.de","threadId":"60586","inReplyTo":"ZXByNGuQTaOQ3sKW@tanuki","subject":"Re: [PATCH 3/7] revision: use die_for_incompatible_opt3() for --graph/--reverse/--walk-reflogs","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-12-06T13:47:54Z","receivedAt":"2023-12-06T13:47:59Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 06.12.23 um 14:08 schrieb Patrick Steinhardt:\n> On Wed, Dec 06, 2023 at 12:51:57PM +0100, René Scharfe wrote:\n>> The revision options --reverse is incompatibel with --walk-reflogs and\n>> --graph is incompatible with both --reverse and --walk-reflogs.  So they\n>> are all incompatible with each other.\n>>\n>> Use the function for checking three mutually incompatible options,\n>> die_for_incompatible_opt3(), to perform this check in one place and\n>> without repetition.  This is shorter and clearer.\n>>\n>> Signed-off-by: René Scharfe <l.s.r@web.de>\n>> ---\n>>  revision.c | 9 +++------\n>>  1 file changed, 3 insertions(+), 6 deletions(-)\n>>\n>> diff --git a/revision.c b/revision.c\n>> index b2861474b1..1b7e1af6c6 100644\n>> --- a/revision.c\n>> +++ b/revision.c\n>> @@ -3036,8 +3036,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n>>  \t\trevs->grep_filter.ignore_locale = 1;\n>>  \tcompile_grep_patterns(&revs->grep_filter);\n>>\n>> -\tif (revs->reverse && revs->reflog_info)\n>> -\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--reverse\", \"--walk-reflogs\");\n>>  \tif (revs->reflog_info && revs->limited)\n>>  \t\tdie(\"cannot combine --walk-reflogs with history-limiting options\");\n>>  \tif (revs->rewrite_parents && revs->children.name)\n>> @@ -3048,11 +3046,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n>>  \t/*\n>>  \t * Limitations on the graph functionality\n>>  \t */\n>> -\tif (revs->reverse && revs->graph)\n>> -\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--reverse\", \"--graph\");\n>> +\tdie_for_incompatible_opt3(!!revs->graph, \"--graph\",\n>> +\t\t\t\t  !!revs->reverse, \"--reverse\",\n>> +\t\t\t\t  !!revs->reflog_info, \"--walk-reflogs\");\n>\n> I've been wondering why we use `!!` here, as `die_for_incompatible_*()`\n> doesn't care for the actual value but only checks that it is non-zero.\n> Is it because of the type mismatch, where these flags here use unsigned\n> ints whereas `die_for_incompatible_*()` expect ints?\n\n->graph and ->reflog_info are pointers and clang reports an int-conversion\nwarning without the double negation.  ->reverse is an unsigned:1 and so\ndoesn't need it, but I gave it one anyway for aesthetic reasons.\n\nRené\n"},{"id":"485383","messageId":"389cd7b3-2350-4dc6-b282-e9d6e25fa68c@web.de","threadId":"60586","inReplyTo":"ZXByOXWo6cIy71s2@tanuki","subject":"Re: [PATCH 4/7] revision, rev-parse: factorize incompatibility messages about --exclude-hidden","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-12-06T14:21:15Z","receivedAt":"2023-12-06T14:26:25Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 06.12.23 um 14:08 schrieb Patrick Steinhardt:\n> On Wed, Dec 06, 2023 at 12:51:58PM +0100, René Scharfe wrote:\n>> Use the standard parameterized message for reporting incompatible\n>> options to report options that are not accepted in combination with\n>> --exclude-hidden.  This reduces the number of strings to translate and\n>> makes the UI a bit more consistent.\n>>\n>> Signed-off-by: René Scharfe <l.s.r@web.de>\n>> ---\n>>  builtin/rev-parse.c                |  9 ++++++---\n>>  revision.c                         | 18 ++++++++++++------\n>>  t/t6018-rev-list-glob.sh           |  6 ++----\n>>  t/t6021-rev-list-exclude-hidden.sh |  4 ++--\n>>  4 files changed, 22 insertions(+), 15 deletions(-)\n>>\n>> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\n>> index fde8861ca4..917f122440 100644\n>> --- a/builtin/rev-parse.c\n>> +++ b/builtin/rev-parse.c\n>> @@ -893,13 +893,15 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n>>  \t\t\t}\n>>  \t\t\tif (opt_with_value(arg, \"--branches\", &arg)) {\n>>  \t\t\t\tif (ref_excludes.hidden_refs_configured)\n>> -\t\t\t\t\treturn error(_(\"--exclude-hidden cannot be used together with --branches\"));\n>> +\t\t\t\t\treturn error(_(\"options '%s' and '%s' cannot be used together\"),\n>> +\t\t\t\t\t\t     \"--exclude-hidden\", \"--branches\");\n>\n> The repetitive nature of this patch and subsequent ones made me wonder\n> whether it would be useful to have a function similar to the\n> `die_for_incompatible_*()` helper that knows to format this error\n> correctly.\n\nI wondered the same and experimented with a die_for_incompatible_opt2().\nIt would allow the compiler to detect typos.\n\nPassing in the conditions as parameters is a bit tedious and unlike its\nfor its higher-numbered siblings there is not much to win by doing that\ninstead of using an if statement or two nested ones.  We could pass in\n1 if we want to integrate that function into an if cascade like above,\nbut it would look a bit silly.  And here we'd need a non-fatal version\nanyway.\n\nPerhaps a build step that lists all new translatable strings would help?\nNah, that would require building each commit.\n\nA LLM-based tool to find translatable strings with the same meaning?\nDon't know how difficult that would be.\n\nSo I feel the same, but don't have a solution that would justify the\nchurn of replacing the duplicate strings with function calls. :-/\n\nRené\n"},{"id":"485384","messageId":"ZXCHj3hIpQb900WX@tanuki","threadId":"60586","inReplyTo":"389cd7b3-2350-4dc6-b282-e9d6e25fa68c@web.de","subject":"Re: [PATCH 4/7] revision, rev-parse: factorize incompatibility messages about --exclude-hidden","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-06T14:39:11Z","receivedAt":"2023-12-06T14:39:20Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Dec 06, 2023 at 03:21:15PM +0100, René Scharfe wrote:\n> Am 06.12.23 um 14:08 schrieb Patrick Steinhardt:\n> > On Wed, Dec 06, 2023 at 12:51:58PM +0100, René Scharfe wrote:\n> >> Use the standard parameterized message for reporting incompatible\n> >> options to report options that are not accepted in combination with\n> >> --exclude-hidden.  This reduces the number of strings to translate and\n> >> makes the UI a bit more consistent.\n> >>\n> >> Signed-off-by: René Scharfe <l.s.r@web.de>\n> >> ---\n> >>  builtin/rev-parse.c                |  9 ++++++---\n> >>  revision.c                         | 18 ++++++++++++------\n> >>  t/t6018-rev-list-glob.sh           |  6 ++----\n> >>  t/t6021-rev-list-exclude-hidden.sh |  4 ++--\n> >>  4 files changed, 22 insertions(+), 15 deletions(-)\n> >>\n> >> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\n> >> index fde8861ca4..917f122440 100644\n> >> --- a/builtin/rev-parse.c\n> >> +++ b/builtin/rev-parse.c\n> >> @@ -893,13 +893,15 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n> >>  \t\t\t}\n> >>  \t\t\tif (opt_with_value(arg, \"--branches\", &arg)) {\n> >>  \t\t\t\tif (ref_excludes.hidden_refs_configured)\n> >> -\t\t\t\t\treturn error(_(\"--exclude-hidden cannot be used together with --branches\"));\n> >> +\t\t\t\t\treturn error(_(\"options '%s' and '%s' cannot be used together\"),\n> >> +\t\t\t\t\t\t     \"--exclude-hidden\", \"--branches\");\n> >\n> > The repetitive nature of this patch and subsequent ones made me wonder\n> > whether it would be useful to have a function similar to the\n> > `die_for_incompatible_*()` helper that knows to format this error\n> > correctly.\n> \n> I wondered the same and experimented with a die_for_incompatible_opt2().\n> It would allow the compiler to detect typos.\n> \n> Passing in the conditions as parameters is a bit tedious and unlike its\n> for its higher-numbered siblings there is not much to win by doing that\n> instead of using an if statement or two nested ones.  We could pass in\n> 1 if we want to integrate that function into an if cascade like above,\n> but it would look a bit silly.  And here we'd need a non-fatal version\n> anyway.\n\nMaybe the easiest solution would be to have `error_incompatible_usage()`\nthat simply wraps `error()`. You'd pass in the incompatible params and\nit makes sure to format them accordingly. It could either accept two\nargs or even be a vararg function with sentinel `NULL`. It's not perfect\nof course, but would at least ensure that we can easily convert things\nover time without having to duplicate the exact message everywhere.\n\nBut ultimately I do not mind the current duplication all that much, the\npatch series looks good to me even without such a new helper.\n\n> Perhaps a build step that lists all new translatable strings would help?\n> Nah, that would require building each commit.\n> \n> A LLM-based tool to find translatable strings with the same meaning?\n> Don't know how difficult that would be.\n\nI don't think it's a problem to not convert everything in one go. The\ncurrent series is a good step in the right direction, and any additional\ninstances that were missed can be fixed in follow-ups.\n\nPatrick\n"},{"id":"485386","messageId":"4954cf77-63f6-4225-833f-3c28d642ed11@web.de","threadId":"60586","inReplyTo":"ZXCHj3hIpQb900WX@tanuki","subject":"Re: [PATCH 4/7] revision, rev-parse: factorize incompatibility messages about --exclude-hidden","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-12-06T17:07:29Z","receivedAt":"2023-12-06T17:07:34Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 06.12.23 um 15:39 schrieb Patrick Steinhardt:\n> On Wed, Dec 06, 2023 at 03:21:15PM +0100, René Scharfe wrote:\n>> Am 06.12.23 um 14:08 schrieb Patrick Steinhardt:\n>>> On Wed, Dec 06, 2023 at 12:51:58PM +0100, René Scharfe wrote:\n>>>> Use the standard parameterized message for reporting incompatible\n>>>> options to report options that are not accepted in combination with\n>>>> --exclude-hidden.  This reduces the number of strings to translate and\n>>>> makes the UI a bit more consistent.\n>>>>\n>>>> Signed-off-by: René Scharfe <l.s.r@web.de>\n>>>> ---\n>>>>  builtin/rev-parse.c                |  9 ++++++---\n>>>>  revision.c                         | 18 ++++++++++++------\n>>>>  t/t6018-rev-list-glob.sh           |  6 ++----\n>>>>  t/t6021-rev-list-exclude-hidden.sh |  4 ++--\n>>>>  4 files changed, 22 insertions(+), 15 deletions(-)\n>>>>\n>>>> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\n>>>> index fde8861ca4..917f122440 100644\n>>>> --- a/builtin/rev-parse.c\n>>>> +++ b/builtin/rev-parse.c\n>>>> @@ -893,13 +893,15 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n>>>>  \t\t\t}\n>>>>  \t\t\tif (opt_with_value(arg, \"--branches\", &arg)) {\n>>>>  \t\t\t\tif (ref_excludes.hidden_refs_configured)\n>>>> -\t\t\t\t\treturn error(_(\"--exclude-hidden cannot be used together with --branches\"));\n>>>> +\t\t\t\t\treturn error(_(\"options '%s' and '%s' cannot be used together\"),\n>>>> +\t\t\t\t\t\t     \"--exclude-hidden\", \"--branches\");\n>>>\n>>> The repetitive nature of this patch and subsequent ones made me wonder\n>>> whether it would be useful to have a function similar to the\n>>> `die_for_incompatible_*()` helper that knows to format this error\n>>> correctly.\n>>\n>> I wondered the same and experimented with a die_for_incompatible_opt2().\n>> It would allow the compiler to detect typos.\n>>\n>> Passing in the conditions as parameters is a bit tedious and unlike its\n>> for its higher-numbered siblings there is not much to win by doing that\n>> instead of using an if statement or two nested ones.  We could pass in\n>> 1 if we want to integrate that function into an if cascade like above,\n>> but it would look a bit silly.  And here we'd need a non-fatal version\n>> anyway.\n>\n> Maybe the easiest solution would be to have `error_incompatible_usage()`\n> that simply wraps `error()`.\n\nYes, but having two variants (die_ and error_) is unfortunate.\n\n> You'd pass in the incompatible params and\n> it makes sure to format them accordingly. It could either accept two\n> args or even be a vararg function with sentinel `NULL`.\n\nTempting, but passing the conditions separately is actually useful to\nimprove the shown message when there are more than two options.\n\n> It's not perfect\n> of course, but would at least ensure that we can easily convert things\n> over time without having to duplicate the exact message everywhere.\n\nMaybe the simplest option would be to use a macro, e.g.\n\n   #define INCOMPATIBLE_OPTIONS_MESSAGE \\\n           _(\"options '%s' and '%s' cannot be used together\")\n\nIt could be used with both error() and die(), and the compiler would\nstill ensure that two strings are passed along with it, but I don't know\nhow to encode that requirement in the macro name somehow to make it\nself-documenting.  Perhaps by getting the number two in there?\n\n> I don't think it's a problem to not convert everything in one go. The\n> current series is a good step in the right direction, and any additional\n> instances that were missed can be fixed in follow-ups.\n\nRight; whatever we do, we can (and should) do it step by step.\n\nRené\n"},{"id":"485387","messageId":"CAPig+cQBKk4PpCjT1RN+puS5vSo8_jZ3wONAgGhRvduw3jZT8A@mail.gmail.com","threadId":"60586","inReplyTo":"20231206115215.94467-4-l.s.r@web.de","subject":"Re: [PATCH 3/7] revision: use die_for_incompatible_opt3() for --graph/--reverse/--walk-reflogs","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-12-06T17:21:48Z","receivedAt":"2023-12-06T17:22:01Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Dec 6, 2023 at 6:53 AM René Scharfe <l.s.r@web.de> wrote:\n> The revision options --reverse is incompatibel with --walk-reflogs and\n\ns/incompatibel/incompatible/\n\n> --graph is incompatible with both --reverse and --walk-reflogs.  So they\n> are all incompatible with each other.\n>\n> Use the function for checking three mutually incompatible options,\n> die_for_incompatible_opt3(), to perform this check in one place and\n> without repetition.  This is shorter and clearer.\n>\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n"},{"id":"485388","messageId":"1a165940-56eb-4cc9-b570-e870c2ad97a2@web.de","threadId":"60586","inReplyTo":"CAPig+cQBKk4PpCjT1RN+puS5vSo8_jZ3wONAgGhRvduw3jZT8A@mail.gmail.com","subject":"Re: [PATCH 3/7] revision: use die_for_incompatible_opt3() for --graph/--reverse/--walk-reflogs","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-12-06T17:29:24Z","receivedAt":"2023-12-06T17:29:27Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 06.12.23 um 18:21 schrieb Eric Sunshine:\n> On Wed, Dec 6, 2023 at 6:53 AM René Scharfe <l.s.r@web.de> wrote:\n>> The revision options --reverse is incompatibel with --walk-reflogs and\n>\n> s/incompatibel/incompatible/\n\nAnd s/options/option/, *sigh*.\n\n>\n>> --graph is incompatible with both --reverse and --walk-reflogs.  So they\n>> are all incompatible with each other.\n>>\n>> Use the function for checking three mutually incompatible options,\n>> die_for_incompatible_opt3(), to perform this check in one place and\n>> without repetition.  This is shorter and clearer.\n>>\n>> Signed-off-by: René Scharfe <l.s.r@web.de>\n"},{"id":"485390","messageId":"ZXDJIXjKefNFhDdf@nand.local","threadId":"60586","inReplyTo":"20231206115215.94467-3-l.s.r@web.de","subject":"Re: [PATCH 2/7] repack: use die_for_incompatible_opt3() for -A/-k/--cruft","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-12-06T19:18:57Z","receivedAt":"2023-12-06T19:18:59Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Dec 06, 2023 at 12:51:56PM +0100, René Scharfe wrote:\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index edaee4dbec..c54777bbe5 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -1203,19 +1203,13 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>  \tif (delete_redundant && repository_format_precious_objects)\n>  \t\tdie(_(\"cannot delete packs in a precious-objects repo\"));\n>\n> -\tif (keep_unreachable &&\n> -\t    (unpack_unreachable || (pack_everything & LOOSEN_UNREACHABLE)))\n> -\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--keep-unreachable\", \"-A\");\n> +\tdie_for_incompatible_opt3(unpack_unreachable || (pack_everything & LOOSEN_UNREACHABLE), \"-A\",\n> +\t\t\t\t  keep_unreachable, \"-k/--keep-unreachable\",\n> +\t\t\t\t  pack_everything & PACK_CRUFT, \"--cruft\");\n\nOof, thanks for cleaning this one up.\n\nI had to read this hunk a couple of times to convince myself that it was\ndoing the right thing, but I believe it is.\n\n> -\tif (pack_everything & PACK_CRUFT) {\n> +\tif (pack_everything & PACK_CRUFT)\n>  \t\tpack_everything |= ALL_INTO_ONE;\n\nAnd adding the ALL_INTO_ONE bit here does not effect either of the below\ntwo checks, so moving them up is fine, too.\n\n> -\t\tif (unpack_unreachable || (pack_everything & LOOSEN_UNREACHABLE))\n> -\t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--cruft\", \"-A\");\n> -\t\tif (keep_unreachable)\n> -\t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--cruft\", \"-k\");\n> -\t}\n\nThanks,\nTaylor\n"},{"id":"485391","messageId":"ZXDKjdOiIdHipaKy@nand.local","threadId":"60586","inReplyTo":"4954cf77-63f6-4225-833f-3c28d642ed11@web.de","subject":"Re: [PATCH 4/7] revision, rev-parse: factorize incompatibility messages about --exclude-hidden","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-12-06T19:25:01Z","receivedAt":"2023-12-06T19:25:04Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Dec 06, 2023 at 06:07:29PM +0100, René Scharfe wrote:\n> > It's not perfect\n> > of course, but would at least ensure that we can easily convert things\n> > over time without having to duplicate the exact message everywhere.\n>\n> Maybe the simplest option would be to use a macro, e.g.\n>\n>    #define INCOMPATIBLE_OPTIONS_MESSAGE \\\n>            _(\"options '%s' and '%s' cannot be used together\")\n>\n> It could be used with both error() and die(), and the compiler would\n> still ensure that two strings are passed along with it, but I don't know\n> how to encode that requirement in the macro name somehow to make it\n> self-documenting.  Perhaps by getting the number two in there?\n\nI think that this is a great idea. It nicely solves Patrick's concern\nthat we have to duplicate this message ID everywhere, and equally solves\nyours by calling error() inline instead of having to pass down the\noption values.\n\nI think that including a number in the macro name would be helpful here.\n\n> > I don't think it's a problem to not convert everything in one go. The\n> > current series is a good step in the right direction, and any additional\n> > instances that were missed can be fixed in follow-ups.\n>\n> Right; whatever we do, we can (and should) do it step by step.\n\nI agree :-).\n\nThanks,\nTaylor\n"},{"id":"485415","messageId":"ZXFvzAb93otxIPjs@tanuki","threadId":"60586","inReplyTo":"ZXDKjdOiIdHipaKy@nand.local","subject":"Re: [PATCH 4/7] revision, rev-parse: factorize incompatibility messages about --exclude-hidden","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-07T07:10:04Z","receivedAt":"2023-12-07T07:10:10Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Dec 06, 2023 at 02:25:01PM -0500, Taylor Blau wrote:\n> On Wed, Dec 06, 2023 at 06:07:29PM +0100, René Scharfe wrote:\n> > > It's not perfect\n> > > of course, but would at least ensure that we can easily convert things\n> > > over time without having to duplicate the exact message everywhere.\n> >\n> > Maybe the simplest option would be to use a macro, e.g.\n> >\n> >    #define INCOMPATIBLE_OPTIONS_MESSAGE \\\n> >            _(\"options '%s' and '%s' cannot be used together\")\n> >\n> > It could be used with both error() and die(), and the compiler would\n> > still ensure that two strings are passed along with it, but I don't know\n> > how to encode that requirement in the macro name somehow to make it\n> > self-documenting.  Perhaps by getting the number two in there?\n> \n> I think that this is a great idea. It nicely solves Patrick's concern\n> that we have to duplicate this message ID everywhere, and equally solves\n> yours by calling error() inline instead of having to pass down the\n> option values.\n> \n> I think that including a number in the macro name would be helpful here.\n\nDoes our i18n tooling know how to extract such messages defined in\nmacros? I have to admit I don't really know how it works under the hood.\nBut if it does work then this looks like a good solution to me.\n\nPatrick\n"}]}