Release 1.5.0

wordexp doesn’t work on OpenBSD, and escaping the path ourselves then
calling system() is actually easier than using wordexp.

CHANGELOG.md

@@ -1,6 +1,21 @@
 opustags changelog
 ==================
 
+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,27 @@ cmake_minimum_required(VERSION 3.9)
 
 project(
 	opustags
-	VERSION 1.3.0
+	VERSION 1.5.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)
+
 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 +32,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.
 

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,19 @@ 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
 
 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,12 @@ 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.
 .SH EXAMPLES
 .PP
 List all the tags in file foo.opus:
@@ -111,6 +121,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,9 +4,6 @@
  *
  * 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>
@@ -26,18 +23,20 @@ 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
 
 See the man page for extensive documentation.
 )raw";
@@ -52,35 +51,35 @@ 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'},
 	{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;
 	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;
@@ -103,11 +102,15 @@ ot::status ot::parse_options(int argc, char** argv, ot::options& opt)
 			opt.to_add.emplace_back(std::move(utf8));
 			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 ':':
 			return {st::bad_arguments,
 			        "Missing value for option '"s + argv[optind - 1] + "'."};
@@ -118,27 +121,52 @@ 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]);
+	}
+
+	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);
+		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)
 {
@@ -163,7 +191,7 @@ void ot::print_comments(const std::list<std::string>& comments, FILE* output)
 				has_control = true;
 		}
 		fwrite(local.data(), 1, local.size(), output);
-		putchar('\n');
+		putc('\n', output);
 	}
 	if (info_lost)
 		fputs("warning: Some tags have been transliterated to your system encoding.\n", stderr);
@@ -188,6 +216,8 @@ 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);
@@ -232,11 +262,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 +274,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)
+{
+	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.
+	std::string tags_path = base_path.value_or("tags") + ".XXXXXX.opustags";
+	int fd = mkstemps(const_cast<char*>(tags_path.data()), 9);
+	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);
+	if (fclose(tags_file) != 0)
+		return {ot::st::standard_error, tags_path + ": fclose error: "s + strerror(errno)};
+
+	// 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;
+	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, tags.comments)) != ot::st::ok) {
+		fprintf(stderr, "warning: Leaving %s on the disk.\n", tags_path.c_str());
+		return rc;
+	}
+	fclose(tags_file);
+
+	// 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 +346,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 +362,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,6 +382,11 @@ 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)) != ot::st::ok)
+						return rc;
+				}
 				auto packet = ot::render_tags(tags);
 				rc = writer->write_header_packet(serialno, pageno, packet);
 				if (rc != ot::st::ok)
@@ -312,23 +405,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 +427,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 +444,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/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.
  *
  */
 
@@ -32,9 +31,6 @@
 #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

@@ -27,11 +27,14 @@
 #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 +60,11 @@ 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,
@@ -161,6 +166,24 @@ 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 +305,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 +401,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 +422,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.
@@ -410,7 +454,7 @@ struct options {
 	/**
 	 * Delete all the existing comments.
 	 *
-	 * Option: --delete-all
+	 * Option: --delete-all, --set-all
 	 */
 	bool delete_all = false;
 	/**
@@ -421,25 +465,16 @@ struct options {
 	 *
 	 * Options: --add, --set, --set-all
 	 */
-	std::vector<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
-	 */
-	bool set_all = false;
+	std::list<std::string> to_add;
 };
 
 /**
  * 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,7 +482,7 @@ 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);
 

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 '" +
@@ -97,3 +136,49 @@ ot::status ot::encoding_converter::operator()(const char* in, size_t n, std::str
 		        "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)};
+	mtime = st.st_mtim; // more precise than st_mtime
+	return st::ok;
+}

t/cli.cc

@@ -39,14 +39,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 +55,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 +69,45 @@ 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[0] != "X" || opt.to_delete[1] != "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");
 }
 
 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 +117,28 @@ 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");
 }
 
 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 => 47;
 
 use Digest::MD5;
 use File::Basename;
+use File::Copy;
 use IPC::Open3;
 use List::MoreUtils qw(any);
 use Symbol 'gensym';
@@ -57,18 +58,20 @@ $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
 
 See the man page for extensive documentation.
 EOF
@@ -81,7 +84,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 +104,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
@@ -170,6 +180,7 @@ ARTIST=七面鳥
 
 A=A
 X=Y
+#IGNORE=COMMENTS
 END_IN
 OK=yes again
 ARTIST=七面鳥
@@ -201,6 +212,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 +247,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');

t/system.cc

@@ -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;
 }