Release 1.6.0

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.

CHANGELOG.md

@@ -1,6 +1,35 @@
 opustags changelog
 ==================
 
+1.6.0 - 2021-01-01
+------------------
+
+- UTF-8 conversion errors are now fatal.
+- Introduce --raw for disabling encoding conversions.
+- Improve platform compatibility.
+
+This also happens to be opustags’s 8-year anniversary!
+
+1.5.1 - 2020-11-21
+------------------
+
+- Improve BSD support.
+
+1.5.0 - 2020-11-08
+------------------
+
+- Introduce --edit for interactive edition.
+
+1.4.0 - 2020-10-04
+------------------
+
+- Preserve permissions when overwriting files.
+- Support multiple files with --in-place.
+- Fix BSD support.
+
+Thanks to Reuben Thomas for contributing the pièce de résistance of this
+release!
+
 1.3.0 - 2019-02-02
 ------------------
 

CMakeLists.txt

@@ -2,20 +2,36 @@ cmake_minimum_required(VERSION 3.9)
 
 project(
 	opustags
-	VERSION 1.3.0
+	VERSION 1.6.0
 	LANGUAGES CXX
 )
 
-set(CMAKE_CXX_STANDARD 14)
+set(CMAKE_CXX_STANDARD 17)
 set(CMAKE_CXX_STANDARD_REQUIRED ON)
 
+# opustags is mainly developed with glibc, which introduces a few
+# incompatibilites with BSDs, like getline not being defined by default.
+# _GNU_SOURCE should trigger BSD’s libc GNU compatibility mode to fix that.
+add_definitions(-D_GNU_SOURCE)
+
 find_package(PkgConfig REQUIRED)
 pkg_check_modules(OGG REQUIRED ogg)
 add_compile_options(${OGG_CFLAGS})
 link_directories(${OGG_LIBRARY_DIRS})
 
+include(FindIconv)
+
+# We need endian.h on Linux, and sys/endian.h on BSD.
+include(CheckIncludeFileCXX)
+check_include_file_cxx(endian.h HAVE_ENDIAN_H)
+check_include_file_cxx(sys/endian.h HAVE_SYS_ENDIAN_H)
+
+include(CheckStructHasMember)
+check_struct_has_member("struct stat" st_mtim sys/stat.h HAVE_STAT_ST_MTIM LANGUAGE CXX)
+check_struct_has_member("struct stat" st_mtimespec sys/stat.h HAVE_STAT_ST_MTIMESPEC LANGUAGE CXX)
+
 configure_file(src/config.h.in config.h @ONLY)
-include_directories(BEFORE src "${CMAKE_BINARY_DIR}" ${OGG_INCLUDE_DIRS})
+include_directories(BEFORE src "${CMAKE_BINARY_DIR}" ${OGG_INCLUDE_DIRS} ${Iconv_INCLUDE_DIRS})
 
 add_library(
 	ot
@@ -25,11 +41,7 @@ add_library(
 	src/opus.cc
 	src/system.cc
 )
-target_link_libraries(ot PUBLIC ${OGG_LIBRARIES})
-
-if (APPLE)
-	target_link_libraries(ot PUBLIC iconv)
-endif()
+target_link_libraries(ot PUBLIC ${OGG_LIBRARIES} ${Iconv_LIBRARIES})
 
 add_executable(opustags src/opustags.cc)
 target_link_libraries(opustags ot)

CONTRIBUTING.md

@@ -42,7 +42,7 @@ questioned, and it was thus abandoned for a few years. Judging by the
 inquiries and contributions, albeit few, on GitHub, it looks like it remains
 relevant, so let's dust it off a bit.
 
-Today, opustags is written in C++14 and features a unit test suite in C++, and
+Today, opustags is written in C++ and features a unit test suite in C++, and
 an integration test suite in Perl. The code was refactored, organized into
 modules, and reviewed for safety.
 
@@ -60,7 +60,6 @@ More generally, here are a few features that could be added in the future:
 - Logicial stream listing and selection for multiplexed files.
 - Escaping control characters with --escape.
 - Dump binary packets with --binary.
-- Skip encoding conversion with --raw.
 - Edition of the vendor string.
 - Edition of the arbitrary binary block past the comments.
 - Support for OpusTags packets spanning multiple pages (> 64 kB).

README.md

@@ -21,7 +21,7 @@ Requirements
 ------------
 
 * a POSIX-compliant system,
-* a C++14 compiler,
+* a C++17 compiler,
 * CMake ≥ 3.9,
 * libogg 1.3.3.
 
@@ -48,17 +48,20 @@ Documentation
 
     Usage: opustags --help
            opustags [OPTIONS] FILE
+           opustags OPTIONS -i FILE...
            opustags OPTIONS FILE -o FILE
 
     Options:
       -h, --help                    print this help
       -o, --output FILE             specify the output file
-      -i, --in-place                overwrite the input file
+      -i, --in-place                overwrite the input files
       -y, --overwrite               overwrite the output file if it already exists
       -a, --add FIELD=VALUE         add a comment
       -d, --delete FIELD[=VALUE]    delete previously existing comments
       -D, --delete-all              delete all the previously existing comments
       -s, --set FIELD=VALUE         replace a comment
       -S, --set-all                 import comments from standard input
+      -e, --edit                    edit tags interactively in VISUAL/EDITOR
+      --raw                         disable encoding conversion
 
 See the man page, `opustags.1`, for extensive documentation.

opustags.1

@@ -10,6 +10,11 @@ opustags \- Ogg Opus tag editor
 .br
 .B opustags
 .I OPTIONS
+.B -i
+.R \fIFILE\fP...
+.br
+.B opustags
+.I OPTIONS
 .B -o
 .I OUTPUT INPUT
 .SH DESCRIPTION
@@ -24,7 +29,7 @@ You can use the options below to edit the tags before printing them.
 This could be useful to preview some changes before writing them.
 .PP
 In editing mode, you need to specify an output file with \fB--output\fP, or use \fB--in-place\fP to
-overwrite the input file. If the output is a regular file, the result is first written to a
+overwrite the input files. If the output is a regular file, the result is first written to a
 temporary file and then moved to its final location on success. On error, the temporary output file
 is deleted.
 .PP
@@ -92,7 +97,19 @@ Delete all the previously existing tags.
 Sets the tags from scratch.
 All the original tags are deleted and new ones are read from standard input.
 Each line must specify a \fIFIELD=VALUE\fP pair and be separated with line feeds.
-Blank lines are ignored.
+Blank lines and lines starting with \fI#\fP are ignored.
+.TP
+.B \-e, \-\-edit
+Edit tags interactively by spawning the program specified by the EDITOR
+environment variable. The allowed format is the same as \fB--set-all\fP.
+If TERM and VISUAL are set, VISUAL takes precedence over EDITOR.
+.TP
+.B \-\-raw
+OpusTags metadata should always be encoded in UTF-8, as per RFC 7845. However, some files may be
+corrupted or possibly even contain intentional binary data. In that case, --raw lets you edit that
+kind of binary data without ensuring the validity of the tags encoding. This option may also be
+useful when your system encoding is different from UTF-8 and you wish to preserve the full UTF-8
+character set even though your system cannot display it.
 .SH EXAMPLES
 .PP
 List all the tags in file foo.opus:
@@ -111,6 +128,10 @@ Remove the previously existing ARTIST tags and add the two X and Y ARTIST tags,
 tags without writing them to the Opus file:
 .PP
 	opustags in.opus --add ARTIST=X --add ARTIST=Y --delete ARTIST
+.PP
+Edit tags interactively in Vim:
+.PP
+	EDITOR=vim opustags --in-place --edit file.opus
 .SH CAVEATS
 .PP
 \fBopustags\fP currently has the following limitations:

src/cli.cc

@@ -4,19 +4,17 @@
  *
  * Provide all the features of the opustags executable from a C++ API. The main point of separating
  * this module from the main one is to allow easy testing.
- *
- * \todo Use a safer temporary file name for in-place editing, like tmpnam.
- * \todo Abort editing with --set-all if one comment is invalid?
  */
 
-#include <config.h>
 #include <opustags.h>
 
 #include <errno.h>
 #include <getopt.h>
 #include <limits.h>
+#include <stdlib.h>
 #include <string.h>
 #include <sys/stat.h>
+#include <unistd.h>
 
 using namespace std::literals::string_literals;
 
@@ -26,18 +24,21 @@ R"raw(
 
 Usage: opustags --help
        opustags [OPTIONS] FILE
+       opustags OPTIONS -i FILE...
        opustags OPTIONS FILE -o FILE
 
 Options:
   -h, --help                    print this help
   -o, --output FILE             specify the output file
-  -i, --in-place                overwrite the input file
+  -i, --in-place                overwrite the input files
   -y, --overwrite               overwrite the output file if it already exists
   -a, --add FIELD=VALUE         add a comment
   -d, --delete FIELD[=VALUE]    delete previously existing comments
   -D, --delete-all              delete all the previously existing comments
   -s, --set FIELD=VALUE         replace a comment
   -S, --set-all                 import comments from standard input
+  -e, --edit                    edit tags interactively in VISUAL/EDITOR
+  --raw                         disable encoding conversion
 
 See the man page for extensive documentation.
 )raw";
@@ -52,62 +53,65 @@ static struct option getopt_options[] = {
 	{"set", required_argument, 0, 's'},
 	{"delete-all", no_argument, 0, 'D'},
 	{"set-all", no_argument, 0, 'S'},
+	{"edit", no_argument, 0, 'e'},
+	{"raw", no_argument, 0, 'r'},
 	{NULL, 0, 0, 0}
 };
 
-ot::status ot::parse_options(int argc, char** argv, ot::options& opt)
+ot::status ot::parse_options(int argc, char** argv, ot::options& opt, FILE* comments_input)
 {
 	static ot::encoding_converter to_utf8("", "UTF-8");
 	std::string utf8;
-	std::string::size_type equal;
+	const char* equal;
 	ot::status rc;
+	bool set_all = false;
 	opt = {};
 	if (argc == 1)
 		return {st::bad_arguments, "No arguments specified. Use -h for help."};
-	bool in_place = false;
 	int c;
 	optind = 0;
-	while ((c = getopt_long(argc, argv, ":ho:iyd:a:s:DS", getopt_options, NULL)) != -1) {
+	while ((c = getopt_long(argc, argv, ":ho:iyd:a:s:DSe", getopt_options, NULL)) != -1) {
 		switch (c) {
 		case 'h':
 			opt.print_help = true;
 			break;
 		case 'o':
-			if (!opt.path_out.empty())
+			if (opt.path_out)
 				return {st::bad_arguments, "Cannot specify --output more than once."};
 			opt.path_out = optarg;
-			if (opt.path_out.empty())
-				return {st::bad_arguments, "Output file path cannot be empty."};
 			break;
 		case 'i':
-			in_place = true;
+			opt.in_place = true;
+			opt.overwrite = true;
 			break;
 		case 'y':
 			opt.overwrite = true;
 			break;
 		case 'd':
-			rc = to_utf8(optarg, strlen(optarg), utf8);
-			if (rc != ot::st::ok)
-				return {st::bad_arguments, "Could not encode argument into UTF-8: " + rc.message};
-			opt.to_delete.emplace_back(std::move(utf8));
+			opt.to_delete.emplace_back(optarg);
 			break;
 		case 'a':
 		case 's':
-			rc = to_utf8(optarg, strlen(optarg), utf8);
-			if (rc != ot::st::ok)
-				return {st::bad_arguments, "Could not encode argument into UTF-8: " + rc.message};
-			if ((equal = utf8.find('=')) == std::string::npos)
+			equal = strchr(optarg, '=');
+			if (equal == nullptr)
 				return {st::bad_arguments, "Comment does not contain an equal sign: "s + optarg + "."};
 			if (c == 's')
-				opt.to_delete.emplace_back(utf8.substr(0, equal));
-			opt.to_add.emplace_back(std::move(utf8));
+				opt.to_delete.emplace_back(optarg, equal - optarg);
+			opt.to_add.emplace_back(optarg);
 			break;
 		case 'S':
-			opt.set_all = true;
+			opt.delete_all = true;
+			set_all = true;
 			break;
 		case 'D':
 			opt.delete_all = true;
 			break;
+		case 'e':
+			opt.edit_interactively = true;
+			break;
+		case 'r':
+			opt.raw = true;
+			break;
 		case ':':
 			return {st::bad_arguments,
 			        "Missing value for option '"s + argv[optind - 1] + "'."};
@@ -118,65 +122,102 @@ ot::status ot::parse_options(int argc, char** argv, ot::options& opt)
 	}
 	if (opt.print_help)
 		return st::ok;
-	if (optind != argc - 1)
-		return {st::bad_arguments, "Exactly one input file must be specified."};
-	opt.path_in = argv[optind];
-	if (opt.path_in.empty())
-		return {st::bad_arguments, "Input file path cannot be empty."};
-	if (in_place) {
-		if (!opt.path_out.empty())
-			return {st::bad_arguments, "Cannot combine --in-place and --output."};
-		if (opt.path_in == "-")
-			return {st::bad_arguments, "Cannot modify standard input in place."};
-		opt.path_out = opt.path_in;
-		opt.overwrite = true;
+
+	// All non-option arguments are input files.
+	bool stdin_as_input = false;
+	for (int i = optind; i < argc; i++) {
+		stdin_as_input = stdin_as_input || strcmp(argv[i], "-") == 0;
+		opt.paths_in.emplace_back(argv[i]);
+	}
+
+	// Convert arguments to UTF-8.
+	if (!opt.raw) {
+		for (std::list<std::string>* args : { &opt.to_add, &opt.to_delete }) {
+			for (std::string& arg : *args) {
+				rc = to_utf8(arg, utf8);
+				if (rc != ot::st::ok)
+					return {st::bad_arguments, "Could not encode argument into UTF-8: " + rc.message};
+				arg = std::move(utf8);
+			}
+		}
+	}
+
+	if (opt.in_place && opt.path_out)
+		return {st::bad_arguments, "Cannot combine --in-place and --output."};
+
+	if (opt.in_place && stdin_as_input)
+		return {st::bad_arguments, "Cannot modify standard input in place."};
+
+	if ((!opt.in_place || opt.edit_interactively) && opt.paths_in.size() != 1)
+		return {st::bad_arguments, "Exactly one input file must be specified."};
+
+	if (set_all && stdin_as_input)
+		return {st::bad_arguments, "Cannot use standard input as input file when --set-all is specified."};
+
+	if (opt.edit_interactively && (stdin_as_input || opt.path_out == "-"))
+		return {st::bad_arguments, "Cannot edit interactively when standard input or standard output are already used."};
+
+	if (opt.edit_interactively && !opt.path_out.has_value() && !opt.in_place)
+		return {st::bad_arguments, "Cannot edit interactively when no output is specified."};
+
+	if (opt.edit_interactively && (opt.delete_all || !opt.to_add.empty() || !opt.to_delete.empty()))
+		return {st::bad_arguments, "Cannot mix --edit with -adDsS."};
+
+	if (set_all) {
+		// Read comments from stdin and prepend them to opt.to_add.
+		std::list<std::string> comments;
+		auto rc = read_comments(comments_input, comments, opt.raw);
+		if (rc != st::ok)
+			return rc;
+		opt.to_add.splice(opt.to_add.begin(), std::move(comments));
 	}
-	if (opt.path_in == "-" && opt.set_all)
-		return {st::bad_arguments,
-		        "Cannot use standard input as input file when --set-all is specified."};
 	return st::ok;
 }
 
 /**
- * \todo Escape new lines.
+ * \todo Find a way to support new lines such that they can be read back by #read_comment without
+ *       ambiguity. We could add a raw mode and separate comments with a \0, or escape control
+ *       characters with a backslash, but we should also preserve compatibiltity with potential
+ *       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)
+ot::status ot::print_comments(const std::list<std::string>& comments, FILE* output, bool raw)
 {
-	static ot::encoding_converter from_utf8("UTF-8", "//TRANSLIT");
+	static ot::encoding_converter from_utf8("UTF-8", "");
 	std::string local;
-	bool info_lost = false;
-	bool bad_comments = false;
 	bool has_newline = false;
 	bool has_control = false;
-	for (const std::string& comment : comments) {
-		ot::status rc = from_utf8(comment, local);
-		if (rc == ot::st::information_lost) {
-			info_lost = true;
-		} else if (rc != ot::st::ok) {
-			bad_comments = true;
-			continue;
+	for (const std::string& utf8_comment : comments) {
+		const std::string* comment;
+		// Convert the comment from UTF-8 to the system encoding if relevant.
+		if (raw) {
+			comment = &utf8_comment;
+		} else {
+			ot::status rc = from_utf8(utf8_comment, local);
+			comment = &local;
+			if (rc != ot::st::ok) {
+				rc.message += " See --raw.";
+				return rc;
+			}
 		}
-		for (unsigned char c : comment) {
+
+		for (unsigned char c : *comment) {
 			if (c == '\n')
 				has_newline = true;
 			else if (c < 0x20)
 				has_control = true;
 		}
-		fwrite(local.data(), 1, local.size(), output);
-		putchar('\n');
+		fwrite(comment->data(), 1, comment->size(), output);
+		putc('\n', output);
 	}
-	if (info_lost)
-		fputs("warning: Some tags have been transliterated to your system encoding.\n", stderr);
-	if (bad_comments)
-		fputs("warning: Some tags are not properly encoded and have not been displayed.\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)
+ot::status ot::read_comments(FILE* input, std::list<std::string>& comments, bool raw)
 {
 	static ot::encoding_converter to_utf8("", "UTF-8");
 	comments.clear();
@@ -188,18 +229,24 @@ ot::status ot::read_comments(FILE* input, std::list<std::string>& comments)
 			--nread;
 		if (nread == 0)
 			continue;
+		if (line[0] == '#') // comment
+			continue;
 		if (memchr(line, '=', nread) == nullptr) {
 			ot::status rc = {ot::st::error, "Malformed tag: " + std::string(line, nread)};
 			free(line);
 			return rc;
 		}
-		std::string utf8;
-		ot::status rc = to_utf8(line, nread, utf8);
-		if (rc == ot::st::ok) {
-			comments.emplace_back(std::move(utf8));
+		if (raw) {
+			comments.emplace_back(line, nread);
 		} else {
-			free(line);
-			return {ot::st::badly_encoded, "UTF-8 conversion error: " + rc.message};
+			std::string utf8;
+			ot::status rc = to_utf8(std::string_view(line, nread), utf8);
+			if (rc == ot::st::ok) {
+				comments.emplace_back(std::move(utf8));
+			} else {
+				free(line);
+				return {ot::st::badly_encoded, "UTF-8 conversion error: " + rc.message};
+			}
 		}
 	}
 	free(line);
@@ -232,11 +279,7 @@ void ot::delete_comments(std::list<std::string>& comments, const std::string& se
 /** Apply the modifications requested by the user to the opustags packet. */
 static ot::status edit_tags(ot::opus_tags& tags, const ot::options& opt)
 {
-	if (opt.set_all) {
-		auto rc = ot::read_comments(stdin, tags.comments);
-		if (rc != ot::st::ok)
-			return rc;
-	} else if (opt.delete_all) {
+	if (opt.delete_all) {
 		tags.comments.clear();
 	} else for (const std::string& name : opt.to_delete) {
 		ot::delete_comments(tags.comments, name.c_str());
@@ -248,6 +291,68 @@ static ot::status edit_tags(ot::opus_tags& tags, const ot::options& opt)
 	return ot::st::ok;
 }
 
+/** Spawn VISUAL or EDITOR to edit the given tags. */
+static ot::status edit_tags_interactively(ot::opus_tags& tags, const std::optional<std::string>& base_path, bool raw)
+{
+	const char* editor = nullptr;
+	if (getenv("TERM") != nullptr)
+		editor = getenv("VISUAL");
+	if (editor == nullptr) // without a terminal, or if VISUAL is unset
+		editor = getenv("EDITOR");
+	if (editor == nullptr)
+		return {ot::st::error,
+		        "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);
+	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)};
+	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.
+	timespec before, after;
+	if ((rc = ot::get_file_timestamp(tags_path.c_str(), before)) != ot::st::ok)
+		return rc;
+	ot::status editor_rc = ot::run_editor(editor, tags_path);
+	if ((rc = ot::get_file_timestamp(tags_path.c_str(), after)) != ot::st::ok)
+		return rc; // probably because the file was deleted
+	bool modified = (before.tv_sec != after.tv_sec || before.tv_nsec != after.tv_nsec);
+	if (editor_rc != ot::st::ok) {
+		if (modified)
+			fprintf(stderr, "warning: Leaving %s on the disk.\n", tags_path.c_str());
+		else
+			remove(tags_path.c_str());
+		return editor_rc;
+	} else if (!modified) {
+		remove(tags_path.c_str());
+		fputs("Cancelling edition because the tags file was not modified.\n", stderr);
+		return ot::st::cancel;
+	}
+
+	// Applying the new tags.
+	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.get(), tags.comments, raw)) != ot::st::ok) {
+		fprintf(stderr, "warning: Leaving %s on the disk.\n", tags_path.c_str());
+		return rc;
+	}
+	tags_file.reset();
+
+	// Remove the temporary tags file only on success, because unlike the
+	// partial Ogg file that is irrecoverable, the edited tags file
+	// contains user data, so let’s leave users a chance to recover it.
+	remove(tags_path.c_str());
+
+	return ot::st::ok;
+}
+
 /**
  * Main loop of opustags. Read the packets from the reader, and forwards them to the writer.
  * Transform the OpusTags packet on the fly.
@@ -258,7 +363,6 @@ static ot::status process(ot::ogg_reader& reader, ot::ogg_writer* writer, const
 {
 	bool focused = false; /*< the stream on which we operate is defined */
 	int focused_serialno; /*< when focused, the serialno of the focused stream */
-	/** \todo Become stream-aware instead of counting the pages of all streams together. */
 	int absolute_page_no = -1; /*< page number in the physical stream, not logical */
 	for (;;) {
 		ot::status rc = reader.next_page();
@@ -275,6 +379,7 @@ static ot::status process(ot::ogg_reader& reader, ot::ogg_writer* writer, const
 			focused = true;
 			focused_serialno = serialno;
 		} else if (serialno != focused_serialno) {
+			/** \todo Support mixed streams. */
 			return {ot::st::error, "Muxed streams are not supported yet."};
 		}
 		if (absolute_page_no == 0) { // Identification header
@@ -294,12 +399,18 @@ static ot::status process(ot::ogg_reader& reader, ot::ogg_writer* writer, const
 			if ((rc = edit_tags(tags, opt)) != ot::st::ok)
 				return rc;
 			if (writer) {
+				if (opt.edit_interactively) {
+					fflush(writer->file); // flush before calling the subprocess
+					if ((rc = edit_tags_interactively(tags, writer->path, opt.raw)) != ot::st::ok)
+						return rc;
+				}
 				auto packet = ot::render_tags(tags);
 				rc = writer->write_header_packet(serialno, pageno, packet);
 				if (rc != ot::st::ok)
 					return rc;
 			} else {
-				ot::print_comments(tags.comments, stdout);
+				if ((rc = ot::print_comments(tags.comments, stdout, opt.raw)) != ot::st::ok)
+					return rc;
 				break;
 			}
 		} else {
@@ -312,23 +423,18 @@ static ot::status process(ot::ogg_reader& reader, ot::ogg_writer* writer, const
 	return ot::st::ok;
 }
 
-ot::status ot::run(const ot::options& opt)
+static ot::status run_single(const ot::options& opt, const std::string& path_in, const std::optional<std::string>& path_out)
 {
-	if (opt.print_help) {
-		fputs(help_message, stdout);
-		return st::ok;
-	}
-
 	ot::file input;
-	if (opt.path_in == "-")
+	if (path_in == "-")
 		input = stdin;
-	else if ((input = fopen(opt.path_in.c_str(), "r")) == nullptr)
+	else if ((input = fopen(path_in.c_str(), "re")) == nullptr)
 		return {ot::st::standard_error,
-		        "Could not open '" + opt.path_in + "' for reading: " + strerror(errno)};
+		        "Could not open '" + path_in + "' for reading: " + strerror(errno)};
 	ot::ogg_reader reader(input.get());
 
 	/* Read-only mode. */
-	if (opt.path_out.empty())
+	if (!path_out)
 		return process(reader, nullptr, opt);
 
 	/* Read-write mode.
@@ -339,11 +445,10 @@ ot::status ot::run(const ot::options& opt)
 	 *  - temporary_output.get() for regular files.
 	 *
 	 * We use a temporary output file for the following reasons:
-	 *  1. The partial .opus output may be seen by softwares like media players, or through
-	 *     inotify for the most attentive process.
+	 *  1. A partial .opus output would be seen by softwares like media players, but a .part
+	 *     (for partial) won’t.
 	 *  2. If the process crashes badly, or the power cuts off, we don't want to leave a partial
-	 *     file at the final location. The temporary file is still going to stay but will have an
-	 *     obvious name.
+	 *     file at the final location. The temporary file is going to remain though.
 	 *  3. If we're overwriting a regular file, we'd rather avoid wiping its content before we
 	 *     even started reading the input file. That way, the original file is always preserved
 	 *     on error or crash.
@@ -357,38 +462,58 @@ ot::status ot::run(const ot::options& opt)
 
 	ot::status rc = ot::st::ok;
 	struct stat output_info;
-	if (opt.path_out == "-") {
+	if (path_out == "-") {
 		output = stdout;
-	} else if (stat(opt.path_out.c_str(), &output_info) == 0) {
+	} else if (stat(path_out->c_str(), &output_info) == 0) {
 		/* The output file exists. */
 		if (!S_ISREG(output_info.st_mode)) {
 			/* Special files are opened for writing directly. */
-			if ((final_output = fopen(opt.path_out.c_str(), "w")) == nullptr)
+			if ((final_output = fopen(path_out->c_str(), "we")) == nullptr)
 				rc = {ot::st::standard_error,
-				      "Could not open '" + opt.path_out + "' for writing: " +
-				       strerror(errno)};
+				      "Could not open '" + path_out.value() + "' for writing: " +
+				      strerror(errno)};
 			output = final_output.get();
 		} else if (opt.overwrite) {
-			rc = temporary_output.open(opt.path_out.c_str());
+			rc = temporary_output.open(path_out->c_str());
 			output = temporary_output.get();
 		} else {
 			rc = {ot::st::error,
-			      "'" + opt.path_out + "' already exists. Use -y to overwrite."};
+			      "'" + path_out.value() + "' already exists. Use -y to overwrite."};
 		}
 	} else if (errno == ENOENT) {
-		rc = temporary_output.open(opt.path_out.c_str());
+		rc = temporary_output.open(path_out->c_str());
 		output = temporary_output.get();
 	} else {
 		rc = {ot::st::error,
-		      "Could not identify '" + opt.path_in + "': " + strerror(errno)};
+		      "Could not identify '" + path_in + "': " + strerror(errno)};
 	}
 	if (rc != ot::st::ok)
 		return rc;
 
 	ot::ogg_writer writer(output);
+	writer.path = path_out;
 	rc = process(reader, &writer, opt);
 	if (rc == ot::st::ok)
 		rc = temporary_output.commit();
 
 	return rc;
 }
+
+ot::status ot::run(const ot::options& opt)
+{
+	if (opt.print_help) {
+		fputs(help_message, stdout);
+		return st::ok;
+	}
+
+	ot::status global_rc = st::ok;
+	for (const auto& path_in : opt.paths_in) {
+		ot::status rc = run_single(opt, path_in, opt.in_place ? path_in : opt.path_out);
+		if (rc != st::ok) {
+			global_rc = st::error;
+			if (!rc.message.empty())
+				fprintf(stderr, "%s: error: %s\n", path_in.c_str(), rc.message.c_str());
+		}
+	}
+	return global_rc;
+}

src/config.h.in

@@ -1,2 +1,7 @@
 #cmakedefine PROJECT_NAME "@PROJECT_NAME@"
 #cmakedefine PROJECT_VERSION "@PROJECT_VERSION@"
+
+#cmakedefine HAVE_ENDIAN_H @HAVE_ENDIAN_H@
+#cmakedefine HAVE_SYS_ENDIAN_H @HAVE_SYS_ENDIAN_H@
+#cmakedefine HAVE_STAT_ST_MTIM @HAVE_STAT_ST_MTIM@
+#cmakedefine HAVE_STAT_ST_MTIMESPEC @HAVE_STAT_ST_MTIMESPEC@

src/opus.cc

@@ -18,7 +18,6 @@
  *
  * \todo Validate that the vendor string and comments are valid UTF-8.
  * \todo Validate that field names are ASCII: 0x20 through 0x7D, 0x3D ('=') excluded.
- * \todo Field names are case insensitive, respect that.
  *
  */
 
@@ -26,15 +25,20 @@
 
 #include <string.h>
 
+#ifdef HAVE_ENDIAN_H
+#  include <endian.h>
+#endif
+
+#ifdef HAVE_SYS_ENDIAN_H
+#  include <sys/endian.h>
+#endif
+
 #ifdef __APPLE__
 #include <libkern/OSByteOrder.h>
 #define htole32(x) OSSwapHostToLittleInt32(x)
 #define le32toh(x) OSSwapLittleToHostInt32(x)
 #endif
 
-/**
- * \todo See if the packet's data could be casted more nicely into a string.
- */
 ot::status ot::parse_tags(const ogg_packet& packet, opus_tags& tags)
 {
 	if (packet.bytes < 0)

src/opustags.cc

@@ -17,15 +17,11 @@
 int main(int argc, char** argv) {
 	setlocale(LC_ALL, "");
 	ot::options opt;
-	ot::status rc = ot::parse_options(argc, argv, opt);
+	ot::status rc = ot::parse_options(argc, argv, opt, stdin);
 	if (rc == ot::st::ok)
 		rc = ot::run(opt);
+	else if (!rc.message.empty())
+		fprintf(stderr, "error: %s\n", rc.message.c_str());
 
-	if (rc != ot::st::ok) {
-		if (!rc.message.empty())
-			fprintf(stderr, "error: %s\n", rc.message.c_str());
-		return EXIT_FAILURE;
-	} else {
-		return EXIT_SUCCESS;
-	}
+	return rc == ot::st::ok ? EXIT_SUCCESS : EXIT_FAILURE;
 }

src/opustags.h

@@ -24,14 +24,19 @@
 
 #pragma once
 
+#include <config.h>
+
 #include <iconv.h>
 #include <ogg/ogg.h>
 #include <stdio.h>
+#include <time.h>
 
 #include <functional>
 #include <list>
 #include <memory>
+#include <optional>
 #include <string>
+#include <string_view>
 #include <vector>
 
 namespace ot {
@@ -57,9 +62,10 @@ enum class st {
 	error,
 	standard_error, /**< Error raised by the C standard library. */
 	int_overflow,
+	cancel,
 	/* System */
 	badly_encoded,
-	information_lost,
+	child_process_failed,
 	/* Ogg */
 	bad_stream,
 	end_of_stream,
@@ -151,16 +157,31 @@ public:
 	~encoding_converter();
 	/**
 	 * Convert text using iconv. If the input sequence is invalid, return #st::badly_encoded and
-	 * abort the processing. If some character could not be converted perfectly, keep converting
-	 * the string and finally return #st::information_lost.
+	 * abort the processing, leaving out in an undefined state.
 	 */
-	status operator()(const std::string& in, std::string& out)
-		{ return (*this)(in.data(), in.size(), out); }
-	status operator()(const char* in, size_t n, std::string& out);
+	status operator()(std::string_view in, std::string& out);
 private:
 	iconv_t cd; /**< conversion descriptor */
 };
 
+/** Escape a string so that a POSIX shell interprets it as a single argument. */
+std::string shell_escape(std::string_view word);
+
+/**
+ * Execute the editor process specified in editor. Wait for the process to exit and
+ * return st::ok on success, or st::child_process_failed if it did not exit with 0.
+ *
+ * editor is passed unescaped to the shell, and may contain CLI options.
+ * path is the name of the file to edit, which will be passed as the last argument to editor.
+ */
+ot::status run_editor(std::string_view editor, std::string_view path);
+
+/**
+ * Return the specified path’s mtime, i.e. the last data modification
+ * timestamp.
+ */
+ot::status get_file_timestamp(const char* path, timespec& mtime);
+
 /** \} */
 
 /***********************************************************************************************//**
@@ -282,6 +303,10 @@ struct ogg_writer {
 	 * represented as a block of data and a length.
 	 */
 	FILE* file;
+	/**
+	 * Path to the output file.
+	 */
+	std::optional<std::string> path;
 };
 
 /**
@@ -374,18 +399,20 @@ struct options {
 	 */
 	bool print_help = false;
 	/**
-	 * Path to the input file. It cannot be empty. The special "-" string means stdin.
+	 * Paths to the input files. The special string "-" means stdin.
 	 *
-	 * This is the mandatory non-flagged parameter.
+	 * At least one input file must be given. If `--in-place` is used,
+	 * more than one may be given.
 	 */
-	std::string path_in;
+	std::vector<std::string> paths_in;
 	/**
-	 * Path to the optional file. The special "-" string means stdout. When empty, opustags runs
-	 * in read-only mode. For in-place editing, path_out is defined equal to path_in.
+	 * Optional path to output file. The special string "-" means stdout. For in-place
+	 * editing, the input file name is used. If no output file name is supplied, and
+	 * --in-place is not used, opustags runs in read-only mode.
 	 *
 	 * Options: --output, --in-place
 	 */
-	std::string path_out;
+	std::optional<std::string> path_out;
 	/**
 	 * By default, opustags won't overwrite the output file if it already exists. This can be
 	 * forced with --overwrite. It is also enabled by --in-place.
@@ -393,6 +420,21 @@ struct options {
 	 * Options: --overwrite, --in-place
 	 */
 	bool overwrite = false;
+	/**
+	 * Process files in-place.
+	 *
+	 * Options: --in-place
+	 */
+	bool in_place = false;
+	/**
+	 * Spawn EDITOR to edit tags interactively.
+	 *
+	 * stdin and stdout must be left free for the editor, so paths_in and
+	 * path_out can’t take `-`, and --set-all is not supported.
+	 *
+	 * Option: --edit
+	 */
+	bool edit_interactively = false;
 	/**
 	 * List of comments to delete. Each string is a selector according to the definition of
 	 * #delete_comments.
@@ -406,11 +448,11 @@ struct options {
 	 *
 	 * Option: --delete, --set
 	 */
-	std::vector<std::string> to_delete;
+	std::list<std::string> to_delete;
 	/**
 	 * Delete all the existing comments.
 	 *
-	 * Option: --delete-all
+	 * Option: --delete-all, --set-all
 	 */
 	bool delete_all = false;
 	/**
@@ -421,25 +463,22 @@ struct options {
 	 *
 	 * Options: --add, --set, --set-all
 	 */
-	std::vector<std::string> to_add;
+	std::list<std::string> to_add;
 	/**
-	 * Replace the previous comments by the ones supplied by the user.
-	 *
-	 * Read a list of comments from stdin and populate #to_add. Further comments may be added
-	 * with the --add option.
-	 *
-	 * Option: --set-all
+	 * Disable encoding conversions. OpusTags are specified to always be encoded as UTF-8, but
+	 * if for some reason a specific file contains binary tags that someone would like to
+	 * extract and set as-is, encoding conversion would get in the way.
 	 */
-	bool set_all = false;
+	bool raw = false;
 };
 
 /**
  * Parse the command-line arguments. Does not perform I/O related validations, but checks the
- * consistency of its arguments.
+ * consistency of its arguments. Comments are read if necessary from the given stream.
  *
  * On error, the state of the options structure is unspecified.
  */
-status parse_options(int argc, char** argv, options& opt);
+status parse_options(int argc, char** argv, options& opt, FILE* comments);
 
 /**
  * Print all the comments, separated by line breaks. Since a comment may contain line breaks, this
@@ -447,16 +486,16 @@ status parse_options(int argc, char** argv, options& opt);
  *
  * The comments must be encoded in UTF-8, and are converted to the system locale when printed.
  *
- * The output generated is meant to be parseable by #ot::read_tags.
+ * The output generated is meant to be parseable by #ot::read_comments.
  */
-void print_comments(const std::list<std::string>& comments, FILE* output);
+status print_comments(const std::list<std::string>& comments, FILE* output, bool raw);
 
 /**
  * Parse the comments outputted by #ot::print_comments.
  *
  * The comments are converted from the system encoding to UTF-8, and returned as UTF-8.
  */
-status read_comments(FILE* input, std::list<std::string>& comments);
+status read_comments(FILE* input, std::list<std::string>& comments, bool raw);
 
 /**
  * Remove all comments matching the specified selector, which may either be a field name or a

src/system.cc

@@ -12,9 +12,14 @@
 #include <opustags.h>
 
 #include <errno.h>
+#include <stdlib.h>
 #include <string.h>
+#include <sys/stat.h>
+#include <sys/wait.h>
 #include <unistd.h>
 
+using namespace std::string_literals;
+
 ot::status ot::partial_file::open(const char* destination)
 {
 	abort();
@@ -33,11 +38,45 @@ ot::status ot::partial_file::open(const char* destination)
 	return st::ok;
 }
 
+static mode_t get_umask()
+{
+	// libc doesn’t seem to provide a way to get umask without changing it, so we need this workaround.
+	// https://www.gnu.org/software/libc/manual/html_node/Setting-Permissions.html
+	mode_t mask = umask(0);
+	umask(mask);
+	return mask;
+}
+
+/**
+ * Try reproducing the file permissions of file `source` onto file `dest`. If
+ * this fails for whatever reason, print a warning and leave the current
+ * permissions. When the source doesn’t exist, use the default file creation
+ * permissions according to umask.
+ */
+static void copy_permissions(const char* source, const char* dest)
+{
+	mode_t target_mode;
+	struct stat source_stat;
+	if (stat(source, &source_stat) == 0) {
+		// We could technically preserve a bit more than that but who
+		// would ever need S_ISUID and friends on an Opus file?
+		target_mode = source_stat.st_mode & 0777;
+	} else if (errno == ENOENT) {
+		target_mode = 0666 & ~get_umask();
+	} else {
+		fprintf(stderr, "warning: Could not read mode of %s: %s\n", source, strerror(errno));
+		return;
+	}
+	if (chmod(dest, target_mode) == -1)
+		fprintf(stderr, "warning: Could not set mode of %s: %s\n", dest, strerror(errno));
+}
+
 ot::status ot::partial_file::commit()
 {
 	if (file == nullptr)
 		return st::ok;
 	file.reset();
+	copy_permissions(final_name.c_str(), temporary_name.c_str());
 	if (rename(temporary_name.c_str(), final_name.c_str()) == -1)
 		return {st::standard_error,
 		        "Could not move the result file '" + temporary_name + "' to '" +
@@ -65,35 +104,87 @@ ot::encoding_converter::~encoding_converter()
 	iconv_close(cd);
 }
 
-ot::status ot::encoding_converter::operator()(const char* in, size_t n, std::string& out)
+ot::status ot::encoding_converter::operator()(std::string_view in, std::string& out)
 {
 	iconv(cd, nullptr, nullptr, nullptr, nullptr);
 	out.clear();
-	out.reserve(n);
-	char* in_cursor = const_cast<char*>(in);
-	size_t in_left = n;
+	out.reserve(in.size());
+	char* in_cursor = const_cast<char*>(in.data());
+	size_t in_left = in.size();
 	constexpr size_t chunk_size = 1024;
 	char chunk[chunk_size];
-	bool lost_information = false;
 	for (;;) {
 		char *out_cursor = chunk;
 		size_t out_left = chunk_size;
 		size_t rc = iconv(cd, &in_cursor, &in_left, &out_cursor, &out_left);
-		if (rc == (size_t) -1 && errno != E2BIG)
+
+		if (rc == (size_t) -1 && errno == E2BIG) {
+			// Loop normally.
+		} else if (rc == (size_t) -1) {
+			return {ot::st::badly_encoded, strerror(errno) + "."s};
+		} else if (rc != 0) {
 			return {ot::st::badly_encoded,
-			        "Could not convert string '" + std::string(in, n) + "': " +
-			        strerror(errno)};
-		if (rc != 0)
-			lost_information = true;
+				"Some characters could not be converted into the target encoding."};
+		}
+
 		out.append(chunk, out_cursor - chunk);
 		if (in_cursor == nullptr)
 			break;
 		else if (in_left == 0)
 			in_cursor = nullptr;
 	}
-	if (lost_information)
-		return {ot::st::information_lost,
-		        "Some characters could not be converted into the target encoding "
-		        "in string '" + std::string(in, n) + "'."};
 	return ot::st::ok;
 }
+
+std::string ot::shell_escape(std::string_view word)
+{
+	std::string escaped_word;
+	// Pre-allocate the result, assuming most of the time enclosing it in single quotes is enough.
+	escaped_word.reserve(2 + word.size());
+
+	escaped_word += '\'';
+	for (char c : word) {
+		if (c == '\'')
+			escaped_word += "'\\''";
+		else if (c == '!')
+			escaped_word += "'\\!'";
+		else
+			escaped_word += c;
+	}
+	escaped_word += '\'';
+
+	return escaped_word;
+}
+
+ot::status ot::run_editor(std::string_view editor, std::string_view path)
+{
+	std::string command = std::string(editor) + " " + shell_escape(path);
+	int status = system(command.c_str());
+
+	if (status == -1)
+		return {st::standard_error, "waitpid error: "s + strerror(errno)};
+	else if (!WIFEXITED(status))
+		return {st::child_process_failed,
+		        "Child process did not terminate normally: "s + strerror(errno)};
+	else if (WEXITSTATUS(status) != 0)
+		return {st::child_process_failed,
+		        "Child process exited with " + std::to_string(WEXITSTATUS(status))};
+
+	return st::ok;
+}
+
+ot::status ot::get_file_timestamp(const char* path, timespec& mtime)
+{
+	struct stat st;
+	if (stat(path, &st) == -1)
+		return {st::standard_error, path + ": stat error: "s + strerror(errno)};
+#if defined(HAVE_STAT_ST_MTIM)
+	mtime = st.st_mtim;
+#elif defined(HAVE_STAT_ST_MTIMESPEC)
+	mtime = st.st_mtimespec;
+#else
+	mtime.tv_sec = st.st_mtime;
+	mtime.tv_nsec = st.st_mtimensec;
+#endif
+	return st::ok;
+}

t/cli.cc

@@ -12,7 +12,7 @@ void check_read_comments()
 	{
 		std::string txt = "TITLE=a b c\n\nARTIST=X\nArtist=Y\n"s;
 		ot::file input = fmemopen((char*) txt.data(), txt.size(), "r");
-		rc = ot::read_comments(input.get(), comments);
+		rc = ot::read_comments(input.get(), comments, false);
 		if (rc != ot::st::ok)
 			throw failure("could not read comments");
 		auto&& expected = {"TITLE=a b c", "ARTIST=X", "Artist=Y"};
@@ -22,14 +22,23 @@ void check_read_comments()
 	{
 		std::string txt = "CORRUPTED=\xFF\xFF\n"s;
 		ot::file input = fmemopen((char*) txt.data(), txt.size(), "r");
-		rc = ot::read_comments(input.get(), comments);
+		rc = ot::read_comments(input.get(), comments, false);
 		if (rc != ot::st::badly_encoded)
 			throw failure("did not get the expected error reading corrupted data");
 	}
+	{
+		std::string txt = "RAW=\xFF\xFF\n"s;
+		ot::file input = fmemopen((char*) txt.data(), txt.size(), "r");
+		rc = ot::read_comments(input.get(), comments, true);
+		if (rc != ot::st::ok)
+			throw failure("could not read comments");
+		if (comments.front() != "RAW=\xFF\xFF")
+			throw failure("parsed user comments did not match expectations");
+	}
 	{
 		std::string txt = "MALFORMED\n"s;
 		ot::file input = fmemopen((char*) txt.data(), txt.size(), "r");
-		rc = ot::read_comments(input.get(), comments);
+		rc = ot::read_comments(input.get(), comments, false);
 		if (rc != ot::st::error)
 			throw failure("did not get the expected error reading malformed comments");
 	}
@@ -39,14 +48,14 @@ void check_read_comments()
  * Wrap #ot::parse_options with a higher-level interface much more convenient for testing.
  * In practice, the argc/argv combo are enough though for the current state of opustags.
  */
-static ot::status parse_options(const std::vector<const char*>& args, ot::options& opt)
+static ot::status parse_options(const std::vector<const char*>& args, ot::options& opt, FILE *comments)
 {
 	int argc = args.size();
 	char* argv[argc];
-	for (size_t i = 0; i < argc; ++i)
+	for (int i = 0; i < argc; ++i)
 		argv[i] = strdup(args[i]);
-	ot::status rc = ot::parse_options(argc, argv, opt);
-	for (size_t i = 0; i < argc; ++i)
+	ot::status rc = ot::parse_options(argc, argv, opt, comments);
+	for (int i = 0; i < argc; ++i)
 		free(argv[i]);
 	return rc;
 }
@@ -55,7 +64,9 @@ void check_good_arguments()
 {
 	auto parse = [](std::vector<const char*> args) {
 		ot::options opt;
-		ot::status rc = parse_options(args, opt);
+		std::string txt = "N=1\n"s;
+		ot::file input = fmemopen((char*) txt.data(), txt.size(), "r");
+		ot::status rc = parse_options(args, opt, input.get());
 		if (rc.code != ot::st::ok)
 			throw failure("unexpected option parsing error");
 		return opt;
@@ -67,29 +78,49 @@ void check_good_arguments()
 		throw failure("did not catch --help");
 
 	opt = parse({"opustags", "x", "--output", "y", "-D", "-s", "X=Y Z", "-d", "a=b"});
-	if (opt.path_in != "x" || opt.path_out != "y" || !opt.delete_all || opt.overwrite ||
-	    opt.to_delete.size() != 2 || opt.to_delete[0] != "X" || opt.to_delete[1] != "a=b" ||
-	    opt.to_add.size() != 1 || opt.to_add[0] != "X=Y Z")
+	if (opt.paths_in.size() != 1 || opt.paths_in.front() != "x" || !opt.path_out ||
+	    opt.path_out != "y" || !opt.delete_all || opt.overwrite || opt.to_delete.size() != 2 ||
+	    opt.to_delete.front() != "X" || *std::next(opt.to_delete.begin()) != "a=b" ||
+	    opt.to_add != std::list<std::string>{"X=Y Z"})
 		throw failure("unexpected option parsing result for case #1");
 
 	opt = parse({"opustags", "-S", "x", "-S", "-a", "x=y z", "-i"});
-	if (opt.path_in != "x" || opt.path_out != "x" || !opt.set_all || !opt.overwrite ||
-	    opt.to_delete.size() != 0 || opt.to_add.size() != 1 || opt.to_add[0] != "x=y z")
+	if (opt.paths_in.size() != 1 || opt.paths_in.front() != "x" || opt.path_out ||
+	    !opt.overwrite || opt.to_delete.size() != 0 ||
+	    opt.to_add != std::list<std::string>{"N=1", "x=y z"})
 		throw failure("unexpected option parsing result for case #2");
+
+	opt = parse({"opustags", "-i", "x", "y", "z"});
+	if (opt.paths_in.size() != 3 || opt.paths_in[0] != "x" || opt.paths_in[1] != "y" ||
+	    opt.paths_in[2] != "z" || !opt.overwrite || !opt.in_place)
+		throw failure("unexpected option parsing result for case #3");
+
+	opt = parse({"opustags", "-ie", "x"});
+	if (opt.paths_in.size() != 1 || opt.paths_in[0] != "x" ||
+	    !opt.edit_interactively || !opt.overwrite || !opt.in_place)
+		throw failure("unexpected option parsing result for case #4");
+
+	opt = parse({"opustags", "-a", "X=\xFF", "--raw", "x"});
+	if (!opt.raw || opt.to_add.front() != "X=\xFF")
+		throw failure("--raw did not disable transcoding");
 }
 
 void check_bad_arguments()
 {
-	auto error_case = [](std::vector<const char*> args, const char* message, const std::string& name) {
+	auto error_code_case = [](std::vector<const char*> args, const char* message, ot::st error_code, const std::string& name) {
 		ot::options opt;
-		ot::status rc = parse_options(args, opt);
-		if (rc.code != ot::st::bad_arguments)
+		std::string txt = "N=1\nINVALID"s;
+		ot::file input = fmemopen((char*) txt.data(), txt.size(), "r");
+		ot::status rc = parse_options(args, opt, input.get());
+		if (rc.code != error_code)
 			throw failure("bad error code for case " + name);
 		if (rc.message != message)
 			throw failure("bad error message for case " + name + ", got: " + rc.message);
 	};
+	auto error_case = [&error_code_case](std::vector<const char*> args, const char* message, const std::string& name) {
+		 error_code_case(args, message, ot::st::bad_arguments, name);
+	};
 	error_case({"opustags"}, "No arguments specified. Use -h for help.", "no arguments");
-	error_case({"opustags", "--output", ""}, "Output file path cannot be empty.", "empty output path");
 	error_case({"opustags", "-a", "X"}, "Comment does not contain an equal sign: X.", "bad comment for -a");
 	error_case({"opustags", "--set", "X"}, "Comment does not contain an equal sign: X.", "bad comment for --set");
 	error_case({"opustags", "-a"}, "Missing value for option '-a'.", "short option with missing value");
@@ -99,13 +130,37 @@ void check_bad_arguments()
 	error_case({"opustags", "-x=y"}, "Unrecognized option '-x'.", "unrecognized short option with value");
 	error_case({"opustags", "--derp=y"}, "Unrecognized option '--derp=y'.", "unrecognized long option with value");
 	error_case({"opustags", "-aX=Y"}, "Exactly one input file must be specified.", "no input file");
-	error_case({"opustags", ""}, "Input file path cannot be empty.", "empty input file path");
 	error_case({"opustags", "-i", "-o", "/dev/null", "-"}, "Cannot combine --in-place and --output.", "in-place + output");
 	error_case({"opustags", "-S", "-"}, "Cannot use standard input as input file when --set-all is specified.",
 	                                    "set all and read opus from stdin");
 	error_case({"opustags", "-i", "-"}, "Cannot modify standard input in place.", "write stdin in-place");
 	error_case({"opustags", "-o", "x", "--output", "y", "z"},
 	           "Cannot specify --output more than once.", "double output");
+	error_code_case({"opustags", "-S", "x"}, "Malformed tag: INVALID", ot::st::error, "attempt to read invalid argument with -S");
+	error_case({"opustags", "-o", "", "--output", "y", "z"},
+	           "Cannot specify --output more than once.", "double output with first filename empty");
+	error_case({"opustags", "-e", "-i", "x", "y"},
+	           "Exactly one input file must be specified.", "editing interactively two files at once");
+	error_case({"opustags", "--edit", "-", "-o", "x"},
+	           "Cannot edit interactively when standard input or standard output are already used.",
+	           "editing interactively from stdandard intput");
+	error_case({"opustags", "--edit", "x", "-o", "-"},
+	           "Cannot edit interactively when standard input or standard output are already used.",
+	           "editing interactively to stdandard output");
+	error_case({"opustags", "--edit", "x"}, "Cannot edit interactively when no output is specified.", "editing without output");
+	error_case({"opustags", "--edit", "x", "-i", "-a", "X=Y"}, "Cannot mix --edit with -adDsS.", "mixing -e and -a");
+	error_case({"opustags", "--edit", "x", "-i", "-d", "X"}, "Cannot mix --edit with -adDsS.", "mixing -e and -d");
+	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.",
+	           "-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.",
+	           "-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.",
+	           "-s with binary data");
 }
 
 static void check_delete_comments()

t/opustags.t

@@ -4,10 +4,11 @@ use strict;
 use warnings;
 use utf8;
 
-use Test::More tests => 34;
+use Test::More tests => 50;
 
 use Digest::MD5;
 use File::Basename;
+use File::Copy;
 use IPC::Open3;
 use List::MoreUtils qw(any);
 use Symbol 'gensym';
@@ -57,18 +58,21 @@ $version
 
 Usage: opustags --help
        opustags [OPTIONS] FILE
+       opustags OPTIONS -i FILE...
        opustags OPTIONS FILE -o FILE
 
 Options:
   -h, --help                    print this help
   -o, --output FILE             specify the output file
-  -i, --in-place                overwrite the input file
+  -i, --in-place                overwrite the input files
   -y, --overwrite               overwrite the output file if it already exists
   -a, --add FIELD=VALUE         add a comment
   -d, --delete FIELD[=VALUE]    delete previously existing comments
   -D, --delete-all              delete all the previously existing comments
   -s, --set FIELD=VALUE         replace a comment
   -S, --set-all                 import comments from standard input
+  -e, --edit                    edit tags interactively in VISUAL/EDITOR
+  --raw                         disable encoding conversion
 
 See the man page for extensive documentation.
 EOF
@@ -81,7 +85,7 @@ error: Unrecognized option '--derp'.
 EOF
 
 is_deeply(opustags('../opustags'), ['', <<"EOF", 256], 'not an Ogg stream');
-error: Input is not a valid Ogg file.
+../opustags: error: Input is not a valid Ogg file.
 EOF
 
 ####################################################################################################
@@ -101,25 +105,32 @@ encoder=Lavc58.18.100 libopus
 EOF
 
 unlink('out.opus');
+my $previous_umask = umask(0022);
 is_deeply(opustags(qw(gobble.opus -o out.opus)), ['', '', 0], 'copy the file without changes');
 is(md5('out.opus'), '111a483596ac32352fbce4d14d16abd2', 'the copy is faithful');
+is((stat 'out.opus')[2] & 0777, 0644, 'apply umask on new files');
+umask($previous_umask);
 
 # empty out.opus
 { my $fh; open($fh, '>', 'out.opus') and close($fh) or die }
 is_deeply(opustags(qw(gobble.opus -o out.opus)), ['', <<'EOF', 256], 'refuse to override');
-error: 'out.opus' already exists. Use -y to overwrite.
+gobble.opus: error: 'out.opus' already exists. Use -y to overwrite.
 EOF
 is(md5('out.opus'), 'd41d8cd98f00b204e9800998ecf8427e', 'the output wasn\'t written');
 
 is_deeply(opustags(qw(gobble.opus -o /dev/null)), ['', '', 0], 'write to /dev/null');
 
+chmod(0604, 'out.opus');
 is_deeply(opustags(qw(gobble.opus -o out.opus --overwrite)), ['', '', 0], 'overwrite');
 is(md5('out.opus'), '111a483596ac32352fbce4d14d16abd2', 'successfully overwritten');
+is((stat 'out.opus')[2] & 0777, 0604, 'overwriting preserves output file\'s mode');
 
+chmod(0700, 'out.opus');
 is_deeply(opustags(qw(--in-place out.opus -a A=B --add=A=C --add), "TITLE=Foo Bar",
                    qw(--delete A --add TITLE=七面鳥 --set encoder=whatever -s 1=2 -s X=1 -a X=2 -s X=3)),
           ['', '', 0], 'complex tag editing');
 is(md5('out.opus'), '66780307a6081523dc9040f3c47b0448', 'check the footprint');
+is((stat 'out.opus')[2] & 0777, 0700, 'in-place editing preserves file mode');
 
 is_deeply(opustags('out.opus'), [<<'EOF', '', 0], 'check the tags written');
 A=B
@@ -151,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
 
@@ -170,6 +181,7 @@ ARTIST=七面鳥
 
 A=A
 X=Y
+#IGNORE=COMMENTS
 END_IN
 OK=yes again
 ARTIST=七面鳥
@@ -201,6 +213,34 @@ is_deeply(opustags('-', '-o', '-', {in => $data, mode => ':raw'}), [$data, '', 0
 
 unlink('out.opus');
 
+# Test --in-place
+unlink('out2.opus');
+copy('gobble.opus', 'out.opus');
+is_deeply(opustags(qw(out.opus --add BAR=baz -o out2.opus)), ['', '', 0], 'process multiple files with --in-place');
+is_deeply(opustags(qw(--in-place --add FOO=bar out.opus out2.opus)), ['', '', 0], 'process multiple files with --in-place');
+is(md5('out.opus'), '30ba30c4f236c09429473f36f8f861d2', 'the tags were added correctly (out.opus)');
+is(md5('out2.opus'), '0a4d20c287b2e46b26cb0eee353c2069', 'the tags were added correctly (out2.opus)');
+
+unlink('out.opus');
+unlink('out2.opus');
+
+####################################################################################################
+# Interactive edition
+
+$ENV{EDITOR} = 'sed -i -e y/aeiou/AEIOU/ `sleep 0.1`';
+is_deeply(opustags('gobble.opus', '-eo', "'screaming !'.opus"), ['', '', 0], 'edit a file with EDITOR');
+is(md5("'screaming !'.opus"), '56e85ccaa83a13c15576d75bbd6d835f', 'the tags were modified');
+
+$ENV{EDITOR} = 'true';
+is_deeply(opustags('-ie', "'screaming !'.opus"), ['', "Cancelling edition because the tags file was not modified.\n", 256], 'close -e without saving');
+is(md5("'screaming !'.opus"), '56e85ccaa83a13c15576d75bbd6d835f', 'the tags were not modified');
+
+$ENV{EDITOR} = 'false';
+is_deeply(opustags('-ie', "'screaming !'.opus"), ['', "'screaming !'.opus: error: Child process exited with 1\n", 256], 'editor exiting with an error');
+is(md5("'screaming !'.opus"), '56e85ccaa83a13c15576d75bbd6d835f', 'the tags were not modified');
+
+unlink("'screaming !'.opus");
+
 ####################################################################################################
 # Test muxed streams
 
@@ -208,7 +248,7 @@ system('ffmpeg -loglevel error -y -i gobble.opus -c copy -map 0:0 -map 0:0 -shor
 	or BAIL_OUT('could not create a muxed stream');
 
 is_deeply(opustags('muxed.ogg'), ['', <<'END_ERR', 256], 'muxed streams detection');
-error: Muxed streams are not supported yet.
+muxed.ogg: error: Muxed streams are not supported yet.
 END_ERR
 
 unlink('muxed.ogg');
@@ -216,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
@@ -234,14 +275,16 @@ 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
+END_OUT
+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
-TITLE=???
-ARTIST=\xe9\xe0\xe7
 I=\xf9\xce
 END_OUT
-warning: Some tags have been transliterated to your system encoding.
-END_ERR
 
 $ENV{LC_ALL} = '';
 
@@ -251,4 +294,20 @@ TITLE=七面鳥
 ARTIST=éàç
 I=ùÎ
 END_OUT
+
+unlink('out.opus');
 }
+
+####################################################################################################
+# Raw edition
+
+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 --raw), { mode => ':raw' }), [<<"END_OUT", '', 0], 'raw read');
+T=\xFF
+U=\xFE
+END_OUT
+
+unlink('out.opus');

t/system.cc

@@ -34,7 +34,7 @@ void check_converter()
 {
 	const char* ephemere_iso = "\xc9\x70\x68\xe9\x6d\xe8\x72\x65";
 	ot::encoding_converter to_utf8("ISO_8859-1", "UTF-8");
-	ot::encoding_converter from_utf8("UTF-8", "ISO_8859-1//TRANSLIT");
+	ot::encoding_converter from_utf8("UTF-8", "ISO_8859-1//IGNORE");
 	std::string out;
 
 	ot::status rc = to_utf8(ephemere_iso, out);
@@ -49,10 +49,19 @@ void check_converter()
 	is(rc, ot::st::badly_encoded, "conversion from bad UTF-8 fails");
 }
 
+void check_shell_esape()
+{
+	is(ot::shell_escape("foo"), "'foo'", "simple string");
+	is(ot::shell_escape("a'b"), "'a'\\''b'", "string with a simple quote");
+	is(ot::shell_escape("a!b"), "'a'\\!'b'", "string with a bang");
+	is(ot::shell_escape("a!b'c!d'e"), "'a'\\!'b'\\''c'\\!'d'\\''e'", "string with a bang");
+}
+
 int main(int argc, char **argv)
 {
-	plan(2);
+	plan(3);
 	run(check_partial_files, "test partial files");
 	run(check_converter, "test encoding converter");
+	run(check_shell_esape, "test shell escaping");
 	return 0;
 }