Release 1.5.0

[fmang]

--edit is quite a big feature, and there is still a pending fix for BSD (#33). Now sounds like a good time to release 1.5.0.

@omar-polo --edit spawns a subprocess, which is something that tends to differ across platforms. I think opustags is still POSIX-compatible, but I’d feel more confident if someone tested it. If you have the time to do that, I’d be grateful! Feel free to give your feedback on the feature too.

[omar-polo]

Well, turns out that OpenBSD doesn't have wordexp(3) in its standard library, so I can't compile the master branch. There may be a reason for this, because (from a quick look) other BSDs provide it (and it seems to be required by POSIX). (I have discovered the existance of wordexp today, so I don't know the background on this choice.)

I started scratching a possible fallback (I'm basically shell-escaping the path by hand and exec'ing /bin/sh -c "$EDITOR $path" -- the outcome should be the same). I'm not confident yet so I haven't made a PR for it -- not to mention that it's probably very un-idiomatic C++ code and I don't personally like #ifndefs too much.

With that, it compiles and runs fine: I am able to change the tags with my preferred editor. It's a useful feature, especially because it's not unusual to have song or album titles with "funny" characters that the shell may try to interpret, and editing with a real editor is handy.

Just one more thing: while trying to replace wordexp I've discovered that if ot::run_editor calls exit before exec'ing the editor, it corrupts the file (but mpv can still play it).

; sha256 "/tmp/Wish You Well.opus"
SHA256 (/tmp/Wish You Well.opus) = 7ff1cf5b149661a5f60ad5fbe134561b46e0f34762409b533ec532196cb89aec
; ./opustags -i -e "/tmp/Wish You Well.opus" # with exit(1) right after the fork, in the subprocess
; echo $?
0
; ./opustags -i -e /tmp/Wish\ You\ Well.opus
/tmp/Wish You Well.opus: error: Comment header did not start with OpusTags
; sha256 "/tmp/Wish You Well.opus"
SHA256 (/tmp/Wish You Well.opus) = ac196720a4b30fd60161cfb4a4035098bc7a4d9d7c0d788cc2dcfc6644214977

and this is due (I suppose) the fact that exit(3) will flush the buffers, and this is happening twice (once in the subprocess and once in the main process). If I explicitly use _exit the file doesn't get corrupted. Now, the chance that the subprocess calls exit before execvp are quite small (if not impossible), but I thought to mention this anyway.

Cheers!

[fmang]

@omar-polo The corruption bug you reported is serious. edit_tags_interactively in cli.cc didn’t return the right error code to its caller, so the process continued editing the file even though the child process exited with an error.

Took me some time to understand the double-flush issue, but you’re absolutely correct. I added an explicit flush.

Concerning wordexp, maybe OpenBSD rejects it because it lets you execute arbitrary commands, but EDITOR is meant to be called anyway. Your escaping looks good, and that’s how Git does it (https://github.com/git/git/blob/master/quote.c), so I expect it to be right.

If we need to call the shell, maybe it will be easier to call system instead of fork/execvp. I’ll try something.

Thanks a lot!

[omar-polo]

Your reasoning on using system instead of fork/execvp seems fine. Usually one uses fork+exec+waitpid to avoid the troubles running commands containing user-provided data, but in this we need to execute user-provided data. It's still needed to escape the file path though.

If it's easier for you I can extend the proof of code I was writing so it escapes also ! and call system, otherwise I'll be happy to test what you'll come up with 👍 :)

[fmang]

@omar-polo You look like you prefer C over C++, and for that kind of string operations I think I’d rather use C++’s automatic allocation. You already did most of the work anyway, so I’ll take inspiration from your POC and let you know when it’s ready. Thanks for the offer!

[fmang]

Should be good now!

@rrthomas If you have any other suggestions concerning --edit or anything else before the release, don’t hesitate.

I’ll be leaving this PR open for a week in case I’m forgetting something.

[omar-polo]

Yeah, I have some background in C but not in C++. Anyway, thanks for fixing this so it works here!

Cheers!

[rrthomas]

Looks good. I just checked to see if there's a wordexp implementation in gnulib, but there isn't.