Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jan 16, 2017, 12:52 UTC
Message-ID: <alpine.DEB.2.20.1701161251100.3469@virtualbox>
In-Reply-To: <257b4175-9879-7814-5d8d-02050792574d@kdbg.org>

Hi Hannes,

On Mon, 16 Jan 2017, Johannes Sixt wrote:

Show 16 quoted lines

> Am 16.01.2017 um 02:51 schrieb Junio C Hamano:
> > * jk/vreport-sanitize (2017-01-11) 2 commits
> >  - vreport: sanitize ASCII control chars
> >  - Revert "vreportf: avoid intermediate buffer"
> >
> >  An error message with an ASCII control character like '\r' in it
> >  can alter the message to hide its early part, which is problematic
> >  when a remote side gives such an error message that the local side
> >  will relay with a "remote: " prefix.
> >
> >  Will merge to 'next'.
> 
> Please be not too hasty with advancing this topic to master. I could imagine
> that there is some unwanted fallout on systems where the end-of-line marker
> CRLF is common. Though, I haven't tested the topic myself, yet, nor do I
> expect regressions in *my* typical workflow.

Thank you for being so attentive.

This topic branch would indeed have caused problems. Worse: it would have caused problems that are not covered by our test suite, as Git for Windows' own utilities do not generate CR/LF line endings. So this regression would have bit our users. Nasty.

Something like this is necessary, at least:

-- snipsnap --
Subject: [PATCH] fixup! vreport: sanitize ASCII control chars

The original patch is incorrect, as it turns carriage returns into question marks.

However, carriage returns should be left alone when preceding a line feed, and simply turned into line feeds otherwise.

To make the end result more readable, the logic is inverted so that the common case (no substitution) is handled first.

While at it, let's lose the unnecessary curly braces.

We also add a regression test that verifies that carriage returns are handled properly. And as we expect CR/LF to be handled properly also on platforms other than Windows, this test case is not guarded by the MINGW prerequisite.

Spotted by Hannes Sixt.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t0000-basic.sh | 8 ++++++++
 usage.c          | 9 ++++++---
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 60811a3a7c..d494cb7c05 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -1063,4 +1063,12 @@ test_expect_success 'very long name in the index handled sanely' '
 	test $len = 4098
 '
 
+# This test verifies that the error reporting functions handle CR correctly.
+# --abbrev-ref is simply used out of convenience, as it reports an error including
+# a user-specified argument.
+test_expect_success 'error messages treat CR/LF correctly' '
+	test_must_fail git rev-parse --abbrev-ref "$(printf "CR/LF\\r\\n")" 2>err &&
+	grep "^fatal: .*CR/LF$" err
+'
+
 test_done
diff --git a/usage.c b/usage.c
index 50a6ccee44..71bc7c0329 100644
--- a/usage.c
+++ b/usage.c
@@ -15,10 +15,13 @@ void vreportf(const char *prefix, const char *err, va_list params)
 	char *p;
 
 	vsnprintf(msg, sizeof(msg), err, params);
-	for (p = msg; *p; p++) {
-		if (iscntrl(*p) && *p != '\t' && *p != '\n')
+	for (p = msg; *p; p++)
+		if (!iscntrl(*p) || *p == '\t' || *p == '\n')
+			continue;
+		else if (*p != '\r')
 			*p = '?';
-	}
+		else if (p[1] != '\n')
+			*p = '\n';
 	fprintf(fh, "%s%s\n", prefix, msg);
 }

-- 
2.11.0.windows.3