On Thu, Sep 05, 2024 at 04:51:49AM -0400, Jeff King wrote:
> Our error reporting routines append a trailing newline, and the strings
> we pass to them should not include them (otherwise we get an extra blank
> line after the message).
>
> These cases were all found by looking at the results of:
>
> git grep -P '[^_](error|error_errno|warning|die|die_errno)\(.*\\n"[,)]' '*.c'
>
> Note that we _do_ sometimes include a newline in the middle of such
> messages, to create multiline output (hence our grep matching "," or ")"
> after we see the newline, so we know we're at the end of the string).
>
> It's possible that one or more of these cases could intentionally be
> including a blank line at the end, but having looked at them all
> manually, I think these are all just mistakes.
>
> Signed-off-by: Jeff King <peff@xxxxxxxx>
> ---
> 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()`:
@r@
expression fmt;
position p;
@@
(
die(_(fmt), ...)@p
|
die(fmt, ...)@p
|
die_errno(_(fmt), ...)@p
|
die_errno(fmt, ...)@p
|
error(_(fmt), ...)@p
|
error(fmt, ...)@p
|
warning(_(fmt), ...)@p
|
warning(fmt, ...)@p
)
@script:python@
fmt << r.fmt;
p << r.p;
@@
if fmt.endswith("\\n\""):
print("{}:{}:{}".format(p[0].file, p[0].line, fmt))
It doesn't auto-generate the patch for us. But at least we'd notice new
instances of these via CI. Output on top of 2e7b89e038 (The twelfth
batch, 2024-09-03):
builtin/bisect.c:586: trailing newline in "revision walk setup failed\n"
builtin/bisect.c:1111: trailing newline in "revision walk setup failed\n"
builtin/push.c:639: trailing newline in "No configured push destination.\n" "Either specify the URL from the command-line or configure a remote repository using\n" "\n" " git remote add <name> <url>\n" "\n" "and then push using the remote name\n" "\n" " git push <name>\n"
builtin/rebase.c:1410: trailing newline in "It seems that there is already a %s directory, and\n" "I wonder if you are in the middle of another rebase. " "If that is the\n" "case, please try\n\t%s\n" "If that is not the case, please\n\t%s\n" "and run me again. I am stopping in case you still " "have something\n" "valuable there.\n"
compat/precompose_utf8.c:158: trailing newline in "iconv_open(%s,%s) failed, but needed:\n" " precomposed unicode is not supported.\n" " If you want to use decomposed unicode, run\n" " \"git config core.precomposeunicode false\"\n"
sequencer.c:3824: trailing newline in "execution failed: %s\n%s" "You can fix the problem, and then run\n" "\n" " git rebase --continue\n" "\n"
sequencer.c:3834: trailing newline in "execution succeeded: %s\nbut " "left changes to the index and/or the working tree.\n" "Commit or stash your changes, and then run\n" "\n" " git rebase --continue\n" "\n"
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.
Patrick
