Make encoding errors fatal

With --raw there is a workaround. The tolerant approach was cool and nice until you want to edit something non-interactively and get the warning telling you you might have lost data after the file was written. Failing fast is most likely the better option here.
fmang · Dec 27, 2020 · 3e0b3fa · 3e0b3fa
1 parent 3e7b420
commit 3e0b3fa
Show file tree

Hide file tree

Showing 5 changed files with 35 additions and 34 deletions.
diff --git a/src/cli.cc b/src/cli.cc
@@ -181,7 +181,7 @@ ot::status ot::parse_options(int argc, char** argv, ot::options& opt, FILE* comm
  *       callers that don’t escape backslashes. Maybe add options to select a mode between simple,
  *       raw, and escaped.
  */
-void ot::print_comments(const std::list<std::string>& comments, FILE* output, bool raw)
+ot::status ot::print_comments(const std::list<std::string>& comments, FILE* output, bool raw)
 {
 	static ot::encoding_converter from_utf8("UTF-8", "");
 	std::string local;
@@ -197,8 +197,8 @@ void ot::print_comments(const std::list<std::string>& comments, FILE* output, bo
 			ot::status rc = from_utf8(utf8_comment, local);
 			comment = &local;
 			if (rc != ot::st::ok) {
-				bad_comments = true;
-				continue;
+				rc.message += " See --raw.";
+				return rc;
 			}
 		}
 
@@ -211,13 +211,11 @@ void ot::print_comments(const std::list<std::string>& comments, FILE* output, bo
 		fwrite(comment->data(), 1, comment->size(), output);
 		putc('\n', output);
 	}
-	if (bad_comments)
-		fputs("warning: Some tags could not be displayed because of incompatible encoding. See --raw.\n", stderr);
 	if (has_newline)
-		fputs("warning: Some tags contain newline characters. "
-		      "These are not supported by --set-all.\n", stderr);
+		fputs("warning: Some tags contain unsupported newline characters.\n", stderr);
 	if (has_control)
 		fputs("warning: Some tags contain control characters.\n", stderr);
+	return st::ok;
 }
 
 ot::status ot::read_comments(FILE* input, std::list<std::string>& comments, bool raw)
@@ -307,18 +305,18 @@ static ot::status edit_tags_interactively(ot::opus_tags& tags, const std::option
 		        "No editor specified in environment variable VISUAL or EDITOR."};
 
 	// Building the temporary tags file.
+	ot::status rc;
 	std::string tags_path = base_path.value_or("tags") + ".XXXXXX.opustags";
 	int fd = mkstemps(const_cast<char*>(tags_path.data()), 9);
-	FILE* tags_file;
+	ot::file tags_file;
 	if (fd == -1 || (tags_file = fdopen(fd, "w")) == nullptr)
 		return {ot::st::standard_error,
 		        "Could not open '" + tags_path + "': " + strerror(errno)};
-	ot::print_comments(tags.comments, tags_file, raw);
-	if (fclose(tags_file) != 0)
-		return {ot::st::standard_error, tags_path + ": fclose error: "s + strerror(errno)};
+	if ((rc = ot::print_comments(tags.comments, tags_file.get(), raw)) != ot::st::ok)
+		return rc;
+	tags_file.reset();
 
 	// Spawn the editor, and watch the modification timestamps.
-	ot::status rc;
 	timespec before, after;
 	if ((rc = ot::get_file_timestamp(tags_path.c_str(), before)) != ot::st::ok)
 		return rc;
@@ -342,11 +340,11 @@ static ot::status edit_tags_interactively(ot::opus_tags& tags, const std::option
 	tags_file = fopen(tags_path.c_str(), "re");
 	if (tags_file == nullptr)
 		return {ot::st::standard_error, "Error opening " + tags_path + ": " + strerror(errno)};
-	if ((rc = ot::read_comments(tags_file, tags.comments, raw)) != ot::st::ok) {
+	if ((rc = ot::read_comments(tags_file.get(), tags.comments, raw)) != ot::st::ok) {
 		fprintf(stderr, "warning: Leaving %s on the disk.\n", tags_path.c_str());
 		return rc;
 	}
-	fclose(tags_file);
+	tags_file.reset();
 
 	// Remove the temporary tags file only on success, because unlike the
 	// partial Ogg file that is irrecoverable, the edited tags file
@@ -412,7 +410,8 @@ static ot::status process(ot::ogg_reader& reader, ot::ogg_writer* writer, const
 				if (rc != ot::st::ok)
 					return rc;
 			} else {
-				ot::print_comments(tags.comments, stdout, opt.raw);
+				if ((rc = ot::print_comments(tags.comments, stdout, opt.raw)) != ot::st::ok)
+					return rc;
 				break;
 			}
 		} else {

diff --git a/src/opustags.h b/src/opustags.h
@@ -488,7 +488,7 @@ status parse_options(int argc, char** argv, options& opt, FILE* comments);
  *
  * The output generated is meant to be parseable by #ot::read_comments.
  */
-void print_comments(const std::list<std::string>& comments, FILE* output, bool raw);
+status print_comments(const std::list<std::string>& comments, FILE* output, bool raw);
 
 /**
  * Parse the comments outputted by #ot::print_comments.

diff --git a/src/system.cc b/src/system.cc
@@ -121,7 +121,7 @@ ot::status ot::encoding_converter::operator()(std::string_view in, std::string&
 		if (rc == (size_t) -1 && errno == E2BIG) {
 			// Loop normally.
 		} else if (rc == (size_t) -1) {
-			return {ot::st::badly_encoded, strerror(errno)};
+			return {ot::st::badly_encoded, strerror(errno) + "."s};
 		} else if (rc != 0) {
 			return {ot::st::badly_encoded,
 				"Some characters could not be converted into the target encoding."};

diff --git a/t/cli.cc b/t/cli.cc
@@ -153,13 +153,13 @@ void check_bad_arguments()
 	error_case({"opustags", "--edit", "x", "-i", "-D"}, "Cannot mix --edit with -adDsS.", "mixing -e and -D");
 	error_case({"opustags", "--edit", "x", "-i", "-S"}, "Cannot mix --edit with -adDsS.", "mixing -e and -S");
 	error_case({"opustags", "-d", "\xFF", "x"},
-	           "Could not encode argument into UTF-8: Invalid or incomplete multibyte or wide character",
+	           "Could not encode argument into UTF-8: Invalid or incomplete multibyte or wide character.",
 	           "-d with binary data");
 	error_case({"opustags", "-a", "X=\xFF", "x"},
-	           "Could not encode argument into UTF-8: Invalid or incomplete multibyte or wide character",
+	           "Could not encode argument into UTF-8: Invalid or incomplete multibyte or wide character.",
 	           "-a with binary data");
 	error_case({"opustags", "-s", "X=\xFF", "x"},
-	           "Could not encode argument into UTF-8: Invalid or incomplete multibyte or wide character",
+	           "Could not encode argument into UTF-8: Invalid or incomplete multibyte or wide character.",
 	           "-s with binary data");
 }
 

diff --git a/t/opustags.t b/t/opustags.t
@@ -162,7 +162,7 @@ is_deeply(opustags('out.opus', '-D', '-a', "X=foo\nbar\tquux"), [<<'END_OUT', <<
 X=foo
 bar	quux
 END_OUT
-warning: Some tags contain newline characters. These are not supported by --set-all.
+warning: Some tags contain unsupported newline characters.
 warning: Some tags contain control characters.
 END_ERR
 
@@ -256,15 +256,16 @@ unlink('muxed.ogg');
 ####################################################################################################
 # Locale
 
-my $locale = 'fr_FR.iso88591';
+my $locale = 'en_US.iso88591';
 my @all_locales = split(' ', `locale -a`);
 
 SKIP: {
-skip "locale $locale is not present", 4 unless (any { $_ eq $locale } @all_locales);
+skip "locale $locale is not present", 5 unless (any { $_ eq $locale } @all_locales);
 
 opustags(qw(gobble.opus -a TITLE=七面鳥 -a ARTIST=éàç -o out.opus -y));
 
 local $ENV{LC_ALL} = $locale;
+local $ENV{LANGUAGE} = '';
 
 is_deeply(opustags(qw(-S out.opus), {in => <<"END_IN", mode => ':raw'}), [<<"END_OUT", '', 0], 'set all in ISO-8859-1');
 T=\xef\xef\xf6
@@ -274,14 +275,17 @@ END_OUT
 
 is_deeply(opustags('-i', 'out.opus', "--add=I=\xf9\xce", {mode => ':raw'}), ['', '', 0], 'write tags in ISO-8859-1');
 
-is_deeply(opustags('out.opus', {mode => ':raw'}), [<<"END_OUT", <<'END_ERR', 0], 'read tags in ISO-8859-1');
+is_deeply(opustags('out.opus', {mode => ':raw'}), [<<"END_OUT", <<"END_ERR", 256], 'read tags in ISO-8859-1 with incompatible characters');
 encoder=Lavc58.18.100 libopus
-ARTIST=\xe9\xe0\xe7
-I=\xf9\xce
 END_OUT
-warning: Some tags could not be displayed because of incompatible encoding. See --raw.
+out.opus: error: Invalid or incomplete multibyte or wide character. See --raw.
 END_ERR
 
+is_deeply(opustags(qw(out.opus -d TITLE -d ARTIST), {mode => ':raw'}), [<<"END_OUT", '', 0], 'read tags in ISO-8859-1');
+encoder=Lavc58.18.100 libopus
+I=\xf9\xce
+END_OUT
+
 $ENV{LC_ALL} = '';
 
 is_deeply(opustags('out.opus'), [<<"END_OUT", '', 0], 'read tags in UTF-8');
@@ -290,22 +294,20 @@ TITLE=七面鳥
 ARTIST=éàç
 I=ùÎ
 END_OUT
-}
 
+unlink('out.opus');
+}
 
 ####################################################################################################
 # Raw edition
 
-is_deeply(opustags(qw(-S out.opus -i --raw -a), "U=\xFE", {in => <<"END_IN", mode => ':raw'}), ['', '', 0], 'raw set-all with binary data');
+is_deeply(opustags(qw(-S gobble.opus -o out.opus --raw -a), "U=\xFE", {in => <<"END_IN", mode => ':raw'}), ['', '', 0], 'raw set-all with binary data');
 T=\xFF
 END_IN
 
-is_deeply(opustags(qw(out.opus)), [<<"END_OUT", <<'END_ERR', 0], 'default read with binary data');
-END_OUT
-warning: Some tags could not be displayed because of incompatible encoding. See --raw.
-END_ERR
-
 is_deeply(opustags(qw(out.opus --raw), { mode => ':raw' }), [<<"END_OUT", '', 0], 'raw read');
 T=\xFF
 U=\xFE
 END_OUT
+
+unlink('out.opus');