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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 24, 2024, 16:06 UTC
Message-ID: <xmqqa5mnty06.fsf@gitster.g>
In-Reply-To: <cf8f2256-d954-4a3e-bc2a-31b2de4c8e1d@web.de>

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.