[PATCH 1/2] factor out strbuf_expand_bad_format()

[PATCH 1/2] factor out strbuf_expand_bad_format()

[René Scharfe]

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 <l.s.r@web.de>
---
 builtin/ls-files.c | 10 +---------
 builtin/ls-tree.c  | 10 +---------
 strbuf.c           | 20 ++++++++++++++++++++
 strbuf.h           |  5 +++++
 4 files changed, 27 insertions(+), 18 deletions(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 92f94e65bf..89e7e726c0 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-format");
 	}
 	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
--
2.44.0

[PATCH 2/2] cat-file: use strbuf_expand_bad_format()

[René Scharfe]

Report unknown format elements and missing closing parentheses with
consistent and translated messages by calling strbuf_expand_bad_format()
at the very end of the combined if/else chain of expand_format() and
expand_atom().

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/cat-file.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index bbf851138e..fadf2da2f0 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -310,8 +310,8 @@ static int is_atom(const char *atom, const char *s, int slen)
 	return alen == slen && !memcmp(atom, s, alen);
 }

-static void expand_atom(struct strbuf *sb, const char *atom, int len,
-			struct expand_data *data)
+static int expand_atom(struct strbuf *sb, const char *atom, int len,
+		       struct expand_data *data)
 {
 	if (is_atom("objectname", atom, len)) {
 		if (!data->mark_query)
@@ -343,7 +343,8 @@ static void expand_atom(struct strbuf *sb, const char *atom, int len,
 			strbuf_addstr(sb,
 				      oid_to_hex(&data->delta_base_oid));
 	} else
-		die("unknown format element: %.*s", len, atom);
+		return 0;
+	return 1;
 }

 static void expand_format(struct strbuf *sb, const char *start,
@@ -354,12 +355,11 @@ static void expand_format(struct strbuf *sb, const char *start,

 		if (skip_prefix(start, "%", &start) || *start != '(')
 			strbuf_addch(sb, '%');
-		else if (!(end = strchr(start + 1, ')')))
-			die("format element '%s' does not end in ')'", start);
-		else {
-			expand_atom(sb, start + 1, end - start - 1, data);
+		else if ((end = strchr(start + 1, ')')) &&
+			 expand_atom(sb, start + 1, end - start - 1, data))
 			start = end + 1;
-		}
+		else
+			strbuf_expand_bad_format(start, "cat-file");
 	}
 }

--
2.44.0

Re: [PATCH 1/2] factor out strbuf_expand_bad_format()

[Chris Torek]

Minor:

On Sun, Mar 24, 2024 at 1:00 AM René Scharfe <l.s.r@web.de> wrote:
> @@ -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-format");

This last string constant is clearly supposed to be "ls-files".

Chris

Re: [PATCH 1/2] factor out strbuf_expand_bad_format()

[René Scharfe]

Am 24.03.24 um 10:00 schrieb Chris Torek:
> Minor:
>
> On Sun, Mar 24, 2024 at 1:00 AM René Scharfe <l.s.r@web.de> wrote:
>> @@ -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-format");
>
> This last string constant is clearly supposed to be "ls-files".

LOL.  Seems I can only hold one f-word in my mind at a time.

Thank you for catching this!

René

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

[René Scharfe]

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 <l.s.r@web.de>
---
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(-)

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
--
2.44.0

[PATCH v2 2/2] cat-file: use strbuf_expand_bad_format()

[René Scharfe]

Report unknown format elements and missing closing parentheses with
consistent and translated messages by calling strbuf_expand_bad_format()
at the very end of the combined if/else chain of expand_format() and
expand_atom().

Signed-off-by: René Scharfe <l.s.r@web.de>
---
Same as v1.

 builtin/cat-file.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index bbf851138e..fadf2da2f0 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -310,8 +310,8 @@ static int is_atom(const char *atom, const char *s, int slen)
 	return alen == slen && !memcmp(atom, s, alen);
 }

-static void expand_atom(struct strbuf *sb, const char *atom, int len,
-			struct expand_data *data)
+static int expand_atom(struct strbuf *sb, const char *atom, int len,
+		       struct expand_data *data)
 {
 	if (is_atom("objectname", atom, len)) {
 		if (!data->mark_query)
@@ -343,7 +343,8 @@ static void expand_atom(struct strbuf *sb, const char *atom, int len,
 			strbuf_addstr(sb,
 				      oid_to_hex(&data->delta_base_oid));
 	} else
-		die("unknown format element: %.*s", len, atom);
+		return 0;
+	return 1;
 }

 static void expand_format(struct strbuf *sb, const char *start,
@@ -354,12 +355,11 @@ static void expand_format(struct strbuf *sb, const char *start,

 		if (skip_prefix(start, "%", &start) || *start != '(')
 			strbuf_addch(sb, '%');
-		else if (!(end = strchr(start + 1, ')')))
-			die("format element '%s' does not end in ')'", start);
-		else {
-			expand_atom(sb, start + 1, end - start - 1, data);
+		else if ((end = strchr(start + 1, ')')) &&
+			 expand_atom(sb, start + 1, end - start - 1, data))
 			start = end + 1;
-		}
+		else
+			strbuf_expand_bad_format(start, "cat-file");
 	}
 }

--
2.44.0

Re: [PATCH v2 1/2] factor out strbuf_expand_bad_format()

[Junio C Hamano]

René Scharfe <l.s.r@web.de> 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.