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

[Patrick Steinhardt <ps@pks.im>]

[-- 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 --]