| Message ID | 20231206115215.94467-5-l.s.r@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | standardize incompatibility messages | expand |
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
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é
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
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é
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
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
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
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(-) -- 2.43.0