Re: [PATCH] drop trailing newline from warning/error/die messages

From: Jeff King <peff@peff.net>
Date: Sep 6, 2024, 03:54 UTC
Message-ID: <20240906035401.GA4168449@coredump.intra.peff.net>
In-Reply-To: <ZtmOsjjcMZ4nEsQN@pks.im>

On Thu, Sep 05, 2024 at 12:57:54PM +0200, Patrick Steinhardt wrote:

Show 7 quoted lines

> > I just happened to notice one of these, so I grepped for more.
> 
> Do we maybe want to add a script that detects these via a Coccinelle
> script? As it turns out, Coccinelle has an embedded Python interpreter
> that allow us to extend it with arbitrary checks. So the following
> script would check for trailing newlines in `die()`, `die_errno()`,
> `error()` and `warning()`:

If you want to pursue this, I certainly have no objection, but personally I have burned enough time trying to make Coccinelle work that I don't want to fall down that particular rabbit hole again. ;)

I think long ago we tried to avoid using the embedded python, because not all builds had it (notably Debian's). But I think that may not be the case anymore.

If you did want to do automated quality checks of error messages, there are probably a few other common pitfalls we could detect (like ending with a full stop ".", and starting with a capital).

Show 5 quoted lines

>     unpack-trees.c:408: trailing newline in "the following paths have collided (e.g. case-sensitive paths\n" "on a case-insensitive filesystem) and only one from the same\n" "colliding group is in the working tree:\n"
> 
> The last three lines are a bit of a false positive, I think. All of
> these calls are `warning()` messages though, so we could potentially
> just drop those conversions.

Yeah, mine didn't find those because it looked for "warning(" on the same source code line as the trailing newline. Which is sort of sloppy, but also kind of works because any message long enough to require multiple lines probably meant the author knew what they were doing. :)

-Peff