search

automerge / automerge-repo-rs

MIT License
39 stars 6 forks source link

Do not use the system's tmp dir #56

Closed teohhanhui closed 1 year ago

teohhanhui commented 1 year ago

(split out from #48)

/cc @issackelly

alexjg commented 1 year ago

I think it might be worth making this a configuration option. I think the reason for not using the system temp directory is that it might not be on the same filesystem as the data directory and thus atomic renames may not work (correct me if I'm wrong there). I think this is a good argument for not defaulting to the system temp directory. However, if - like me - you want to run this on your primary desktop and your system temp directory is on the same filesystem as the data directory then the temp directory has the really nice feature of cleaning itself up on every reboot and so if the sync server crashes and leaves data hanging around it will not persist beyond the next boot.

teohhanhui commented 1 year ago

Extrapolating from there, actually the user could already override the TMPDIR env var (or TMP / TEMP on Windows). So I think there's no need for a config.

I'm closing this.

issackelly commented 1 year ago

Alex, you're right about the need, and I don't really care that it's a config option or not. Unfortunately Han, I think I still need a config option. We can't assume an overwrite TMP for the whole application just for automerge-repo-rs. Some things do not need an atomic move out of TMP and can benefit from ramfs or something. I think that I could probably work around it but a confg variable that defaults to TMPDIR sounds like an "everbody wins!" option

alexjg commented 1 year ago

@issackelly I agree, although I don't care too much about defaulting to TMPDIR. I think the data directory itself is a good default (i.e. the least likely to result in bug reports for us 😅 ) but there should be the option to override it to whatever the user wants for people who understand the possible downsides.

teohhanhui commented 1 year ago

Not sure if this is okay?

alexjg commented 1 year ago

I think the only thing remaining is to add some brief documentation to the with_tmpdir methods explaining what they do (sadly this will have to be duplicated due to the need for duplicate implementations). The most important thing to explain is that the user needs to ensure that the tempdir is on the same filesystem as the data dir.

teohhanhui commented 1 year ago

Docs added.