threads / patch / 61190

patch, 2 partsfactor out strbuf_expand_bad_format()

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

## tl;dr

7 messages between Mar 24, 2024 and Mar 24, 2024. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

René Scharfe· Mar 24, 2024, 07:55 UTC · lore

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(-)
Show changes to 4 files +27 −19

builtin/ls-files.c, builtin/ls-tree.c, strbuf.c, strbuf.h

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
René Scharfe· Mar 24, 2024, 08:04 UTC · re: René Scharfe · lore

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

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(-)
Show changes to builtin/cat-file.c +8 −9
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
Chris Torek· Mar 24, 2024, 09:00 UTC · re: René Scharfe · lore

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

Minor:
On Sun, Mar 24, 2024 at 1:00 AM René Scharfe <l.s.r@web.de> wrote:
Show 7 quoted lines
> @@ -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
René Scharfe· Mar 24, 2024, 11:17 UTC · re: Chris Torek · lore

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

Am 24.03.24 um 10:00 schrieb Chris Torek:
Show 12 quoted lines
> 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é
René Scharfe· Mar 24, 2024, 11:19 UTC · re: René Scharfe · lore

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

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(-)
Show changes to 4 files +27 −19

builtin/ls-files.c, builtin/ls-tree.c, strbuf.c, strbuf.h

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
Junio C Hamano· Mar 24, 2024, 16:06 UTC · re: René Scharfe · lore

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

René Scharfe <l.s.r@web.de> writes:
Show 10 quoted lines
> @@ -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.

Show 20 quoted lines
> +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.
René Scharfe· Mar 24, 2024, 11:21 UTC · re: René Scharfe · lore

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

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(-)
Show changes to builtin/cat-file.c +8 −9
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

← back to recent threads