Reset color settings after writing body

[runesoerensen]

The color settings are currently reset immediately after writing the header value in the log_warning and log_error functions, but before setting and writing the (non-bold), colored body value.

As a result, the color setting persist after the function call (e.g. after logging a warning, subsequent log_info output will be yellow). I might be missing something here -- but I doubt that's the intended behavior?

[edmorley]

I agree this is broken on main at the moment (see also #555), and that we need to adjust the resets here, however, I think we need a few more changes.

Something that came up during the buildpack_output work, is that because other tools can end up adding prefixes to each log line (for example, endosome adds the remote: prefix, and Pack CLI can add the [builder] prefixes for an untrusted builder, or the timestamp prefixes when using --timestamps), the log helpers need to wrap each line separately, rather than being able to rely on the ANSI codes from an earlier line still applying for the current line. See:

libcnb.rs/libherokubuildpack/src/buildpack_output/ansi_escape.rs

Lines 1 to 27 in 3fa2c6c

	/// Wraps each line in an ANSI escape sequence while preserving prior ANSI escape sequences.
	///
	/// ## Why does this exist?
	///
	/// When buildpack output is streamed to the user, each line is prefixed with `remote: ` by Git.
	/// Any colorization of text will apply to those prefixes which is not the desired behavior. This
	/// function colors lines of text while ensuring that styles are disabled at the end of each line.
	///
	/// ## Supports recursive colorization
	///
	/// Strings that are previously colorized will not be overridden by this function. For example,
	/// if a word is already colored yellow, that word will continue to be yellow.
	pub(crate) fn wrap_ansi_escape_each_line(ansi: &ANSI, body: impl AsRef<str>) -> String {
	let ansi_escape = ansi.to_str();
	body.as_ref()
	.split('\n')
	// If sub contents are colorized it will contain SUBCOLOR ... RESET. After the reset,
	// ensure we change back to the current color
	.map(|line| line.replace(RESET, &format!("{RESET}{ansi_escape}"))) // Handles nested color
	// Set the main color for each line and reset after so we don't colorize `remote:` by accident
	.map(|line| format!("{ansi_escape}{line}{RESET}"))
	// The above logic causes redundant colors and resets, clean them up
	.map(|line| line.replace(&format!("{ansi_escape}{ansi_escape}"), ansi_escape)) // Reduce useless color
	.map(|line| line.replace(&format!("{ansi_escape}{RESET}"), "")) // Empty lines or where the nested color is at the end of the line
	.collect::<Vec<String>>()
	.join("\n")
	}

The log_error() behaviour on main is currently equivalent to:

<colour>
[Error: Foo]
<reset><colour>Message

And with prefixes this ends up being:

[builder] <colour>
[builder] [Error: Foo]
[builder] <reset><colour>Message body

When really what we need is:

[builder] 
[builder] <colour>[Error: Foo]<reset>
[builder] <colour>Message body<reset>

ie: I think we need to change a few things:

1. Add the extra .reset() after writing the message body (as this PR does)
2. Leave the .reset() after writing the header (ie: skip the removal currently in this PR)
3. For the header, also change it to use two writeln! calls, rather than including the leading \n in a single call. The heading colour attributes would only then be specified on the second call.

Making those changes should fix #555.

(Those changes still won't fix the case where the message body is a multi-line string, but fixing the latter would be more involved and perhaps not worth doing given the log_* helpers are deprecated in favour of buildpack_output.)

[edmorley]

Removing skip-changelog since this (a) changes user-facing behaviour, (b) will fix a bug (see #555) - so should be mentioned in the changelog.