From 65764a93a7ddac47064685aceac2a9d4672a989c Mon Sep 17 00:00:00 2001 From: WebFreak001 Date: Wed, 5 Oct 2022 21:51:36 +0200 Subject: [PATCH] Fix #568, don't output empty files without error Now when an error in formatting happens, it never outputs anything and doesn't override the file when working inplace. Additionally dfmt is no longer able to fail in the middle of a file, as now we first write everything to a buffer and only if everything was successful, that buffer is printed to stdout or written to the inplace file. This should also guard against segfaults with inplace file formatting erasing parts of the file, as well as the user thinking it was successful, even though dfmt didn't finish properly. --- .travis.sh | 2 +- makefile | 2 +- src/dfmt/main.d | 27 +++++++----- tests/expected_failures/issue0568.d | 1 + tests/test.d | 66 ++++++++++++++++++----------- tests/test.sh | 32 -------------- 6 files changed, 60 insertions(+), 70 deletions(-) create mode 100644 tests/expected_failures/issue0568.d delete mode 100755 tests/test.sh diff --git a/.travis.sh b/.travis.sh index cb48753..63d2cba 100755 --- a/.travis.sh +++ b/.travis.sh @@ -12,4 +12,4 @@ else make debug -j2 fi -cd tests && ./test.sh +cd tests && ./test.d diff --git a/makefile b/makefile index 1227910..d790c0c 100644 --- a/makefile +++ b/makefile @@ -31,7 +31,7 @@ gdc:githash $(GDC) $(SRC) $(GDC_FLAGS) -obin/dfmt test: debug - cd tests && ./test.sh + cd tests && ./test.d bin/dfmt-test: githash $(SRC) $(DC) $(DMD_TEST_FLAGS) $^ -of$@ diff --git a/src/dfmt/main.d b/src/dfmt/main.d index 316d8a2..ccf407b 100644 --- a/src/dfmt/main.d +++ b/src/dfmt/main.d @@ -38,13 +38,13 @@ version (NoMain) } else { - import std.array : front, popFront; - import std.stdio : stdout, stdin, stderr, writeln, File; import dfmt.config : Config; - import dfmt.formatter : format; - import std.path : buildPath, dirName, expandTilde; import dfmt.editorconfig : getConfigFor; + import dfmt.formatter : format; + import std.array : appender, front, popFront; import std.getopt : getopt, GetOptException; + import std.path : buildPath, dirName, expandTilde; + import std.stdio : File, stderr, stdin, stdout, writeln; int main(string[] args) { @@ -153,14 +153,13 @@ else args.popFront(); immutable bool readFromStdin = args.length == 0; - File output = stdout; version (Windows) { // On Windows, set stdout to binary mode (needed for correct EOL writing) // See Phobos' stdio.File.rawWrite { import std.stdio : _O_BINARY; - immutable fd = output.fileno; + immutable fd = stdout.fileno; _setmode(fd, _O_BINARY); version (CRuntime_DigitalMars) { @@ -220,7 +219,7 @@ else break; } immutable bool formatSuccess = format("stdin", buffer, - output.lockingTextWriter(), &config); + stdout.lockingTextWriter(), &config); return formatSuccess ? 0 : 1; } else @@ -262,10 +261,16 @@ else { buffer = new ubyte[](cast(size_t) f.size); f.rawRead(buffer); - if (inplace) - output = File(path, "wb"); - immutable bool formatSuccess = format(path, buffer, output.lockingTextWriter(), &config); - if (!formatSuccess) + auto output = appender!string; + immutable bool formatSuccess = format(path, buffer, output, &config); + if (formatSuccess) + { + if (inplace) + File(path, "wb").rawWrite(output.data); + else + stdout.rawWrite(output.data); + } + else retVal = 1; } } diff --git a/tests/expected_failures/issue0568.d b/tests/expected_failures/issue0568.d new file mode 100644 index 0000000..e7bfa7a --- /dev/null +++ b/tests/expected_failures/issue0568.d @@ -0,0 +1 @@ +### This file should not be erased just because it is not formatted correctly diff --git a/tests/test.d b/tests/test.d index 9310a2a..79f516b 100755 --- a/tests/test.d +++ b/tests/test.d @@ -29,39 +29,55 @@ int main() if (const result = spawnProcess(dfmtCommand, stdin, File(outFileName, "w")).wait) return result; - const outText = outFileName.readText; - const refText = refFileName.readText; - const outLines = outText.splitLines(Yes.keepTerminator); - const refLines = refText.splitLines(Yes.keepTerminator); - foreach (i; 0 .. min(refLines.length, outLines.length)) - if (outLines[i] != refLines[i]) - { - writeln("Found difference between ", outFileName, " and ", refFileName, " on line ", i + 1, ":"); - writefln("out: %(%s%)", [outLines[i]]); // Wrapping in array shows line endings. - writefln("ref: %(%s%)", [refLines[i]]); - return 1; - } - if (outLines.length < refLines.length) - { - writeln("Line ", outLines.length + 1, " in ", refFileName, " not found in ", outFileName, ":"); - writefln("%(%s%)", [refLines[outLines.length]]); - return 1; - } - if (outLines.length > refLines.length) - { - writeln("Line ", outLines.length + 1, " in ", outFileName, " not present in ", refFileName, ":"); - writefln("%(%s%)", [outLines[refLines.length]]); - return 1; - } + if (int ret = diff(refFileName, outFileName)) + return ret; } foreach (entry; dirEntries("expected_failures", "*.d", SpanMode.shallow)) - if (execute([dfmt, entry]).status == 0) + { + string copied = entry ~ ".out.d"; + copy(entry, copied); + scope (exit) + remove(copied); + if (execute([dfmt, copied, "--inplace"]).status == 0) { stderr.writeln("Expected failure on test ", entry, " but passed."); return 1; } + if (int ret = diff(entry, copied)) + return ret; + } + writeln("All tests succeeded."); return 0; } + +int diff(string refFileName, string outFileName) +{ + const outText = outFileName.readText; + const refText = refFileName.readText; + const outLines = outText.splitLines(Yes.keepTerminator); + const refLines = refText.splitLines(Yes.keepTerminator); + foreach (i; 0 .. min(refLines.length, outLines.length)) + if (outLines[i] != refLines[i]) + { + writeln("Found difference between ", outFileName, " and ", refFileName, " on line ", i + 1, ":"); + writefln("out: %(%s%)", [outLines[i]]); // Wrapping in array shows line endings. + writefln("ref: %(%s%)", [refLines[i]]); + return 1; + } + if (outLines.length < refLines.length) + { + writeln("Line ", outLines.length + 1, " in ", refFileName, " not found in ", outFileName, ":"); + writefln("%(%s%)", [refLines[outLines.length]]); + return 1; + } + if (outLines.length > refLines.length) + { + writeln("Line ", outLines.length + 1, " in ", outFileName, " not present in ", refFileName, ":"); + writefln("%(%s%)", [outLines[refLines.length]]); + return 1; + } + return 0; +} diff --git a/tests/test.sh b/tests/test.sh deleted file mode 100755 index 7b81277..0000000 --- a/tests/test.sh +++ /dev/null @@ -1,32 +0,0 @@ -#!/usr/bin/env bash -set -e - -for braceStyle in allman otbs knr -do - for source in *.d - do - test "$(basename $source '.d')" = 'test' && continue - - echo "${source}.ref" "${braceStyle}/${source}.out" - argsFile=$(basename "${source}" .d).args - if [ -e "${argsFile}" ]; then - args=$(cat "${argsFile}") - else - args= - fi - ../bin/dfmt --brace_style=${braceStyle} ${args} "${source}" > "${braceStyle}/${source}.out" - diff -u "${braceStyle}/${source}.ref" "${braceStyle}/${source}.out" - done -done - -set +e - -for source in expected_failures/*.d -do - if ../bin/dfmt "${source}" > /dev/null; then - echo "Expected failure on test ${source} but passed" - exit 1 - fi -done - -echo "This script is superseded by test.d."