git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Patrick Steinhardt <ps@pks.im>
Date
Dec 6, 2023, 14:39 UTC
Message-ID
<ZXCHj3hIpQb900WX@tanuki>
In-Reply-To
<389cd7b3-2350-4dc6-b282-e9d6e25fa68c@web.de>
On Wed, Dec 06, 2023 at 03:21:15PM +0100, René Scharfe wrote:
Show 41 quoted lines
> 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.

Show 5 quoted lines
> 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
Previous: René ScharfeNext: René Scharfe
Message 10 of 20 in “standardize incompatibility messages”
  1. 0/7 standardize incompatibility messagesRené Scharfe, Dec 6, 2023
  2. 3/7 revision: use die_for_incompatible_opt3() for --graph/--reverse/--walk-reflogsRené Scharfe, Dec 6, 2023
  3. Patrick SteinhardtDec 6, 2023
  4. René ScharfeDec 6, 2023
  5. Eric SunshineDec 6, 2023
  6. René ScharfeDec 6, 2023
  7. 4/7 revision, rev-parse: factorize incompatibility messages about --exclude-hiddenRené Scharfe, Dec 6, 2023
  8. Patrick SteinhardtDec 6, 2023
  9. René ScharfeDec 6, 2023
  10. Patrick SteinhardtDec 6, 2023
  11. René ScharfeDec 6, 2023
  12. Taylor BlauDec 6, 2023
  13. Patrick SteinhardtDec 7, 2023
  14. 1/7 push: use die_for_incompatible_opt4() for --delete/--tags/--all/--mirrorRené Scharfe, Dec 6, 2023
  15. 2/7 repack: use die_for_incompatible_opt3() for -A/-k/--cruftRené Scharfe, Dec 6, 2023
  16. Taylor BlauDec 6, 2023
  17. 5/7 clean: factorize incompatibility messageRené Scharfe, Dec 6, 2023
  18. 7/7 worktree: simplify incompatibility message for --orphan and commit-ishRené Scharfe, Dec 6, 2023
  19. 6/7 worktree: standardize incompatibility messagesRené Scharfe, Dec 6, 2023
  20. Patrick SteinhardtDec 6, 2023

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.