[v2,1/2] factor out strbuf_expand_bad_format()

Commit Message

René Scharfe March 24, 2024, 11:19 a.m. UTC

Extract a function for reporting placeholders that are not enclosed in a
parenthesis or are unknown.  This reduces the number of strings to
translate and improves consistency across commands.  Call it at the end
of the if/else chain, after exhausting all accepted possibilities.

Signed-off-by: René Scharfe <[e-mail]>
---
Changes since v1: s/ls-format/ls-files/

 builtin/ls-files.c | 10 +---------
 builtin/ls-tree.c  | 10 +---------
 strbuf.c           | 20 ++++++++++++++++++++
 strbuf.h           |  5 +++++
 4 files changed, 27 insertions(+), 18 deletions(-)

--
2.44.0

Comments

Junio C Hamano March 24, 2024, 4:06 p.m. UTC | #1

René Scharfe <[e-mail]> writes:

> @@ -274,12 +273,6 @@ static void show_ce_fmt(struct repository *repo, const struct cache_entry *ce,
>  			strbuf_addch(&sb, '%');
>  		else if ((len = strbuf_expand_literal(&sb, format)))
>  			format += len;
> -		else if (*format != '(')
> -			die(_("bad ls-files format: element '%s' "
> -			      "does not start with '('"), format);
> -		else if (!(end = strchr(format + 1, ')')))
> -			die(_("bad ls-files format: element '%s' "
> -			      "does not end in ')'"), format);

We used to do these two checks upfront, before the cascade of checks
that follow from here to the "else die()".

But we do not even take advantage of the fact that we already
checked these two in the following tests.  We do not take advantage
of the fact that we know where the placeholder ends by computing
"end" early, and all the checks in the "else if" cascade check that
the placeholder is enclosed inside a pair of parentheses again and
again.

So there is no point in optimizing to fail fast by having these two
tests early.

> +void strbuf_expand_bad_format(const char *format, const char *command)
> +{
> +	const char *end;
> +
> +	if (*format != '(')
> +		/* TRANSLATORS: The first %s is a command like "ls-tree". */
> +		die(_("bad %s format: element '%s' does not start with '('"),
> +		    command, format);
> +
> +	end = strchr(format + 1, ')');
> +	if (!end)
> +		/* TRANSLATORS: The first %s is a command like "ls-tree". */
> +		die(_("bad %s format: element '%s' does not end in ')'"),
> +		    command, format);
> +
> +	/* TRANSLATORS: %s is a command like "ls-tree". */
> +	die(_("bad %s format: %%%.*s"),
> +	    command, (int)(end - format + 1), format);
> +}
> +

Now these "pair of parentheses" checks are removed from the "else
if" cascade, and we check them only to give a different message that
tells _how_ the format was bad in expand_bad_format().

Looks good.

Patch

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 92f94e65bf..6eeb5cba78 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -266,7 +266,6 @@  static void show_ce_fmt(struct repository *repo, const struct cache_entry *ce,
 	struct strbuf sb = STRBUF_INIT;

 	while (strbuf_expand_step(&sb, &format)) {
-		const char *end;
 		size_t len;
 		struct stat st;

@@ -274,12 +273,6 @@  static void show_ce_fmt(struct repository *repo, const struct cache_entry *ce,
 			strbuf_addch(&sb, '%');
 		else if ((len = strbuf_expand_literal(&sb, format)))
 			format += len;
-		else if (*format != '(')
-			die(_("bad ls-files format: element '%s' "
-			      "does not start with '('"), format);
-		else if (!(end = strchr(format + 1, ')')))
-			die(_("bad ls-files format: element '%s' "
-			      "does not end in ')'"), format);
 		else if (skip_prefix(format, "(objectmode)", &format))
 			strbuf_addf(&sb, "%06o", ce->ce_mode);
 		else if (skip_prefix(format, "(objectname)", &format))
@@ -308,8 +301,7 @@  static void show_ce_fmt(struct repository *repo, const struct cache_entry *ce,
 		else if (skip_prefix(format, "(path)", &format))
 			write_name_to_buf(&sb, fullname);
 		else
-			die(_("bad ls-files format: %%%.*s"),
-			    (int)(end - format + 1), format);
+			strbuf_expand_bad_format(format, "ls-files");
 	}
 	strbuf_addch(&sb, line_terminator);
 	fwrite(sb.buf, sb.len, 1, stdout);
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index e4a891337c..bd803ace03 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -100,19 +100,12 @@  static int show_tree_fmt(const struct object_id *oid, struct strbuf *base,
 		return 0;

 	while (strbuf_expand_step(&sb, &format)) {
-		const char *end;
 		size_t len;

 		if (skip_prefix(format, "%", &format))
 			strbuf_addch(&sb, '%');
 		else if ((len = strbuf_expand_literal(&sb, format)))
 			format += len;
-		else if (*format != '(')
-			die(_("bad ls-tree format: element '%s' "
-			      "does not start with '('"), format);
-		else if (!(end = strchr(format + 1, ')')))
-			die(_("bad ls-tree format: element '%s' "
-			      "does not end in ')'"), format);
 		else if (skip_prefix(format, "(objectmode)", &format))
 			strbuf_addf(&sb, "%06o", mode);
 		else if (skip_prefix(format, "(objecttype)", &format))
@@ -135,8 +128,7 @@  static int show_tree_fmt(const struct object_id *oid, struct strbuf *base,
 			strbuf_setlen(base, baselen);
 			strbuf_release(&sbuf);
 		} else
-			die(_("bad ls-tree format: %%%.*s"),
-			    (int)(end - format + 1), format);
+			strbuf_expand_bad_format(format, "ls-tree");
 	}
 	strbuf_addch(&sb, options->null_termination ? '\0' : '\n');
 	fwrite(sb.buf, sb.len, 1, stdout);
diff --git a/strbuf.c b/strbuf.c
index 7827178d8e..449eb610f1 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -442,6 +442,26 @@  size_t strbuf_expand_literal(struct strbuf *sb, const char *placeholder)
 	return 0;
 }

+void strbuf_expand_bad_format(const char *format, const char *command)
+{
+	const char *end;
+
+	if (*format != '(')
+		/* TRANSLATORS: The first %s is a command like "ls-tree". */
+		die(_("bad %s format: element '%s' does not start with '('"),
+		    command, format);
+
+	end = strchr(format + 1, ')');
+	if (!end)
+		/* TRANSLATORS: The first %s is a command like "ls-tree". */
+		die(_("bad %s format: element '%s' does not end in ')'"),
+		    command, format);
+
+	/* TRANSLATORS: %s is a command like "ls-tree". */
+	die(_("bad %s format: %%%.*s"),
+	    command, (int)(end - format + 1), format);
+}
+
 void strbuf_addbuf_percentquote(struct strbuf *dst, const struct strbuf *src)
 {
 	size_t i, len = src->len;
diff --git a/strbuf.h b/strbuf.h
index e959caca87..c758de3729 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -337,6 +337,11 @@  size_t strbuf_expand_literal(struct strbuf *sb, const char *placeholder);
  */
 int strbuf_expand_step(struct strbuf *sb, const char **formatp);

+/**
+ * Used with `strbuf_expand_step` to report unknown placeholders.
+ */
+void strbuf_expand_bad_format(const char *format, const char *command);
+
 /**
  * Append the contents of one strbuf to another, quoting any
  * percent signs ("%") into double-percents ("%%") in the