search

fmang / opustags

Ogg Opus tags editor
BSD 3-Clause "New" or "Revised" License
81 stars 11 forks source link

Temporary file \todo already done? #28

Closed rrthomas closed 4 years ago

rrthomas commented 4 years ago

In cli.cc there is a comment:

  * \todo Use a safer temporary file name for in-place editing, like tmpnam.

But in system.cc, ot::partial_file::open already uses mkstemps to create the temporary file.

rrthomas commented 4 years ago

On the other hand, this comment in cli.cc:

     *  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.

Is no longer entirely valid: the file does not have an obvious name.

fmang commented 4 years ago

Feel free to open a pull request if you’d like to review the documentation. I’m not even sure removing the TODO is worth a commit to be honest.

The temporary file name is based on a pattern foo.XXXXXX.part, where .part is a common suffix for partial files. It’s clear enough in my opinion.

rrthomas commented 4 years ago

I'm sorry, I was under the impression that mkstemps created a file specifically under TMPDIR, hence why I thought it would be hard to find!

rrthomas commented 4 years ago

If it's not worth a commit, I could roll it into one of my PRs?

fmang commented 4 years ago

I reviewed the documentation and found that other TODOs were obsolete too. I cleaned everything in 6f7ac1f13bbbe30b173bc4c7ea2d52da2051bea7