Reopening, since it's not really fixed and a new minor bug was introduced: in line 6 there is condition that checks if $LOG_PATH is an existing directory. It always¹ is.

Authors of `mktemp` know well what they do and don't try to break their choices.

Do not hardcode the temporary directory. It's not always "/tmp". `mktemp` does it properly by detecting the right one from the environment. If you want to provide a string that allows name to be distinctive and give a hint that it's freecol's directory, use --suffix option:
mktemp --suffix=.freecol
and this will create directories in a form:
/temporaryDir/tmp.hHia91mAcU.freecol
/temporaryDir/tmp.0Aim6DsM91.freecol
...

If the condition from line 6 would not always fail, it would be allowing `install -md755 $LOG_PATH` to be executed. There are two bugs in that command:
1. Lack of quotes around variable expansion. Since the value is passed to command that changes permissions and can silently accept multiple paths, it creates a security threat. A low severity one, but still...
2. The command is widening permissions. The permissions should be ones that `mktemp` has set.

For unknown reason the current code is also truncating names space by providing only 3 X-es compared to default 10. This means that for 1% probability of collision only 70 instances are required and for 50%: 570. Not a big issue practially², but there is really no reason to decrease this value. Just stick to `mktemp`'s defaults.

There is really a good reason I've removed whole `[ -d ... ] install ... $LOGPATH` part. It wasn't for decreasing file size ;).

¹ The only case it isn't is when `mktemp` fails. But if it does, then
the command following the condition will fail too.
² Happily `mktemp` prevents users from decreasing names space so low
that it would become a real problem.

Thanks for the detailed write-up. I don't know why I didn't just copy your solution directly. Please review the new package.

It is a good thing that you didn't just copied it. After all I could have malicious intent and post something that would break things. Even without that: I'm not perfect too. I could make some mistake. It's better if more people check the code.

It's not about not copying my solution, but about overriding what `mktemp` authors have choosen.

I've checked up the package. Seems to work right. There are some minor things that could be changed, but I believe they're not worth another package upgrade.

Thanks for fixing.

Hi,

I've noticed 1 downside of using mktemp over the mkdir method :
with the mkdir method freecol used the same directory every time it was started (and the messages.log file was overwritten) .
Now it creates a new directory / file every time it's started.

I play freecol a lot, and tend to start it 10+ times between reboots , which leads to many directories for freecol in the tmp folder.
Suggestions how to avoid this ?

Because the directory was created in /tmp, I've assumed that it was meant to be a temporary directory. Hence `mktemp` is a proper solution. However I've never asked what is the purpose of creating the directory in the first place, blindly believing it just has to be there.

So maybe it's the right moment to ask the question: why is the directory needed?

I looked at the change log, and the mkdir statements were already present when it moved from AUR to community.

Freecol has extensive internal logging, but that starts AFTER the freecol.jar is activated.
messages.log captures the output from the 'java -Xmx256M -jar $FC_PATH/FreeCol.jar "$@" --freecol-data $FC_PATH/data \
+ &> ./messages.log' command.

If something goes wrong with starting freecol before it's internal logging kicks in, messages.log is the only place with info to check what happened.
It is useful info, where should a log file with runtime info be placed ?

Freecol stable stores it's own log file here :
~/.freecol

the latest git version of freecol follows XDG rules,
and defaults to ~/.cache/freecol for it's log file on new installs.

So it was inherited from AUR package. Does anyone knows if it serves any real purpose currently?

If something goes wrong when starting java, the output goes to stderr and this is the place everyone checks.

To be frank, we can probably do away with the temp dir altogether. What do you say? If there is a problem, just start it from a terminal and then you'll see the error. It's probably best anyway.

I think it's a good idea. If someone notices any problems (like their stderr being spammed with error messages during normal usage) this can be fixed later. However I would wait few days before pushing next update. Possibly someone else notices something we have missed. Updating a package over and over again in a short period makes no sense.

Also there is no need to start from terminal. In X stderr and stdout are redirected to ~/.xsessionerrors file and this is the place everyone check first for errors from graphical applications.

I pushed a new package, please have a look.