[v4,15/28] format_trailer_info(): avoid double-printing the separator

Commit Message

Linus Arver Feb. 6, 2024, 5:12 a.m. UTC

From: Linus Arver <[e-mail]>

Do not hardcode the printing of ": " as the separator and space (which
can result in double-printing these characters); instead only
print the separator and space if we cannot find any recognized separator
somewhere in the key (yes, keys may have a trailing separator in it ---
we will eventually fix this design but not now). Do so by copying the
code out of print_tok_val(), and deleting the same function.

The test suite passes again with this change.

Signed-off-by: Linus Arver <[e-mail]>
---
 trailer.c | 25 +++++--------------------
 1 file changed, 5 insertions(+), 20 deletions(-)

Comments

Christian Couder Feb. 12, 2024, 11:38 p.m. UTC | #1

On Tue, Feb 6, 2024 at 6:12 AM Linus Arver via GitGitGadget
<[e-mail]> wrote:
>
> From: Linus Arver <[e-mail]>
>
> Do not hardcode the printing of ": " as the separator and space (which
> can result in double-printing these characters); instead only
> print the separator and space if we cannot find any recognized separator
> somewhere in the key (yes, keys may have a trailing separator in it ---
> we will eventually fix this design but not now). Do so by copying the
> code out of print_tok_val(), and deleting the same function.
>
> The test suite passes again with this change.

I think it should be clearer above that this fixes a bug that was
introduced earlier in the series.

Also I wonder why it was not possible to modify format_trailer_info()
like it is done in this patch before using it to replace
format_trailers().

Linus Arver Feb. 13, 2024, 5:21 p.m. UTC | #2

Christian Couder <[e-mail]> writes:

> On Tue, Feb 6, 2024 at 6:12 AM Linus Arver via GitGitGadget
> <[e-mail]> wrote:
>>
>> From: Linus Arver <[e-mail]>
>>
>> Do not hardcode the printing of ": " as the separator and space (which
>> can result in double-printing these characters); instead only
>> print the separator and space if we cannot find any recognized separator
>> somewhere in the key (yes, keys may have a trailing separator in it ---
>> we will eventually fix this design but not now). Do so by copying the
>> code out of print_tok_val(), and deleting the same function.
>>
>> The test suite passes again with this change.
>
> I think it should be clearer above that this fixes a bug that was
> introduced earlier in the series.

Ack, will add something like

    This double printing is from a bug introduced earlier when we
    started using format_trailer_info() everywhere.

to this patch's description, but also add explicit language in
"trailer: begin formatting unification" to say that the change is
introducing temporary bugs (and that this is why the tests break).

> Also I wonder why it was not possible to modify format_trailer_info()
> like it is done in this patch before using it to replace
> format_trailers().

The artificial organization apparent in this patch was deliberate, in
order to make it painfully obvious exactly what was being replaced and
how. See https://lore.kernel.org/git/xmqqjzno13ev.fsf@gitster.g/

Christian Couder Feb. 13, 2024, 5:25 p.m. UTC | #3

On Tue, Feb 13, 2024 at 6:21 PM Linus Arver <[e-mail]> wrote:
>
> Christian Couder <[e-mail]> writes:
>
> > On Tue, Feb 6, 2024 at 6:12 AM Linus Arver via GitGitGadget
> > <[e-mail]> wrote:

> > Also I wonder why it was not possible to modify format_trailer_info()
> > like it is done in this patch before using it to replace
> > format_trailers().
>
> The artificial organization apparent in this patch was deliberate, in
> order to make it painfully obvious exactly what was being replaced and
> how. See https://lore.kernel.org/git/xmqqjzno13ev.fsf@gitster.g/

As for the previous patch, I would have thought that it would be
better not to break the tests.

Linus Arver Feb. 13, 2024, 7:52 p.m. UTC | #4

Christian Couder <[e-mail]> writes:

> On Tue, Feb 13, 2024 at 6:21 PM Linus Arver <[e-mail]> wrote:
>>
>> Christian Couder <[e-mail]> writes:
>>
>> > On Tue, Feb 6, 2024 at 6:12 AM Linus Arver via GitGitGadget
>> > <[e-mail]> wrote:
>
>> > Also I wonder why it was not possible to modify format_trailer_info()
>> > like it is done in this patch before using it to replace
>> > format_trailers().
>>
>> The artificial organization apparent in this patch was deliberate, in
>> order to make it painfully obvious exactly what was being replaced and
>> how. See https://lore.kernel.org/git/xmqqjzno13ev.fsf@gitster.g/
>
> As for the previous patch, I would have thought that it would be
> better not to break the tests.

I could just squash these patches together to avoid breaking tests (and
also avoid doing the flipping of expect_success to expect_fail and back
again). I don't mind at all which way we go, but now that we have these
patches broken out I wonder if it's better to just keep them that way.

Junio, do you mind if I squash the relevant changes together into just
one patch?  I'd like your input because you requested the current style
(modulo test breakages which was my error). Thanks.

Kristoffer Haugsbakk Feb. 13, 2024, 8:41 p.m. UTC | #5

On Tue, Feb 6, 2024, at 06:12, Linus Arver via GitGitGadget wrote:
> From: Linus Arver <[e-mail]>
>
> Do not hardcode the printing of ": " as the separator and space (which
> can result in double-printing these characters); instead only
> print the separator and space if we cannot find any recognized separator
> somewhere in the key (yes, keys may have a trailing separator in it ---
> we will eventually fix this design but not now). Do so by copying the
> code out of print_tok_val(), and deleting the same function.
>
> The test suite passes again with this change.
>
> Signed-off-by: Linus Arver <[e-mail]>

Nice find and a great commit message!

Linus Arver March 15, 2024, 5:31 a.m. UTC | #6

Linus Arver <[e-mail]> writes:

> Christian Couder <[e-mail]> writes:
>
>> On Tue, Feb 13, 2024 at 6:21 PM Linus Arver <[e-mail]> wrote:
>>>
>>> Christian Couder <[e-mail]> writes:
>>>
>>> > On Tue, Feb 6, 2024 at 6:12 AM Linus Arver via GitGitGadget
>>> > <[e-mail]> wrote:
>>
>>> > Also I wonder why it was not possible to modify format_trailer_info()
>>> > like it is done in this patch before using it to replace
>>> > format_trailers().
>>>
>>> The artificial organization apparent in this patch was deliberate, in
>>> order to make it painfully obvious exactly what was being replaced and
>>> how. See https://lore.kernel.org/git/xmqqjzno13ev.fsf@gitster.g/
>>
>> As for the previous patch, I would have thought that it would be
>> better not to break the tests.
>
> I could just squash these patches together to avoid breaking tests (and
> also avoid doing the flipping of expect_success to expect_fail and back
> again). I don't mind at all which way we go, but now that we have these
> patches broken out I wonder if it's better to just keep them that way.
>
> Junio, do you mind if I squash the relevant changes together into just
> one patch?  I'd like your input because you requested the current style
> (modulo test breakages which was my error). Thanks.

When I asked this question, I forgot that the number of test cases that
break are around ~50. This is a very large number. So I think it
would be cleaner to squash this and the previous patch down to avoid having
to flip test_expect_{success,failure} for 50+ individual test cases.

For the earlier patch

    [PATCH v4 10/28] format_trailer_info(): use trailer_item objects

there are only 8 failures so I think doing the *_{success,failure} flip
is reasonable.

Patch

diff --git a/trailer.c b/trailer.c
index c28b6c11cc5..5c42a19943a 100644
--- a/trailer.c
+++ b/trailer.c
@@ -144,24 +144,6 @@  static char last_non_space_char(const char *s)
 	return '\0';
 }
 
-static void print_tok_val(struct strbuf *out, const char *tok, const char *val)
-{
-	char c;
-
-	if (!tok) {
-		strbuf_addf(out, "%s\n", val);
-		return;
-	}
-
-	c = last_non_space_char(tok);
-	if (!c)
-		return;
-	if (strchr(separators, c))
-		strbuf_addf(out, "%s%s\n", tok, val);
-	else
-		strbuf_addf(out, "%s%c %s\n", tok, separators[0], val);
-}
-
 static struct trailer_item *trailer_from_arg(struct arg_item *arg_tok)
 {
 	struct trailer_item *new_item = xcalloc(1, sizeof(*new_item));
@@ -1104,8 +1086,11 @@  void format_trailer_info(const struct process_trailer_options *opts,
 				if (!opts->key_only && !opts->value_only) {
 					if (opts->key_value_separator)
 						strbuf_addbuf(out, opts->key_value_separator);
-					else
-						strbuf_addstr(out, ": ");
+					else {
+						char c = last_non_space_char(tok.buf);
+						if (c && !strchr(separators, c))
+							strbuf_addf(out, "%c ", separators[0]);
+					}
 				}
 				if (!opts->key_only)
 					strbuf_addbuf(out, &val);