Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable customization of temporary file prefixes and suffixes #447

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pgossman
Copy link

@pgossman pgossman commented Dec 4, 2020

Addresses #367.

This PR adds two flags, -tempfileprefix and -tempfilesuffix with defaults of ".unison" and ".unison.tmp".

I tested this change locally by interrupting a sync to leave an old temp file on disk, and unison found it successfully on the next run and was able to finish the sync. However, if a sync is interrupted, and then unison is run again with different args to tempfileprefix/suffix, then it does not pick up the old archive. It instead views the old archive as a new file to sync. I am unsure if this edge case should be handled, and if so, how to go about implementing a fix.

Would reading environment variables be preferred over new flags? I could see the case for it considering an envvar is used for the location of the .unison dir. I am unsure when one is preferred over the other.

@bcpierce00
Copy link
Owner

Doing it with environment variables has two advantages: (1) the settings can be different on different hosts, and (2) doesn't change the wire protocol.

I still wonder whether it might be best to just change the default names to something different, globally, and leave it at that.

@gdt
Copy link
Collaborator

gdt commented Dec 4, 2020

I generally dislike environment variables rather than explicit configuration, when explicit configuration is possible. For finding the dir with the config, it has to be an environment variable, and it is also usually possible to not set a variable and use the default.

I don't see it as a problem that running unison again with different settings after an interrupted run doesn't do what you might want.

Agreed about changing the defaults if there is a single plan, but so far I haven't seen a global analysis of all the situations and issues. (I also haven't seen the counter-notion, of "here's how to configure Dropbox to ignore these files"; it's not clear where the line of responsibility should be.) All that said, if there is a convention on each of Windows, Mac, POSIX that is already recognized and aligning to it isn't problematic, that sounds like a good first step.

@gdt
Copy link
Collaborator

gdt commented Dec 4, 2020

Doing it with environment variables has two advantages: (1) the settings can be different on different hosts, and (2) doesn't change the wire protocol.

I do not follow how environment variable vs command-line (or in a profile, or a unison-wide config file) bears on either making thing different on different hosts, or the wire protocol. I don't see any reason for temp file names to have to be consistent across hosts being synced.

Overall I feel like any unison-wide config that should happen for all profiles belongs in UNISONHOME/something, which is the subject of #372. Otherwise we are just moving things that should be into a config file into environment variables one at at time. However, this is awkward and error prone, as the default ssh config does not set environment variables.

@gdt
Copy link
Collaborator

gdt commented Dec 4, 2020

And, thanks for your contribution. This may well go through a bit of rototill, but that's part of how progress happens.

@tleedjarv
Copy link
Contributor

I don't know if this is really an issue that the current proposal does not allow different prefixes and suffixes on different hosts, but this is something I wanted to point out as well. I think this would only ever matter if one of the hosts is Windows and the other not, if even then.

However, having user preferences in the first place seems only really useful to cover for file system/OS special needs, which in turn almost implies that the preferences are different on client and server.

For this scenario, and if talking about built-in prefixes and suffixes, the simplest thing I can think of would be

let tempFilePrefix = if Util.osType = `Win32 then "~$unison." else ".~unison."

(taking the prefix examples from #367)

@bcpierce00
Copy link
Owner

bcpierce00 commented Dec 6, 2020 via email

@pgossman
Copy link
Author

pgossman commented Dec 7, 2020

I also haven't seen the counter-notion, of "here's how to configure Dropbox to ignore these files"; it's not clear where the line of responsibility should be.

It seems the responsibility should be on Dropbox to allow filters on which files to exclude. Other backup services (Tarsnap, pCloud, Google Drive) support this. The only backup services I could find that don't support exclude patterns are Box and Dropbox, which will both ignore files starting with ".~" on Unix and "~$" on Windows.

As of now, the only use case for this PR is to avoid pickups by Dropbox. Perhaps it would be better to revisit this if users have other cases for customizing temp files, rather than introduce new config options that are not yet needed. I have updated this PR to a change in the default prefix per the suggestion from @tleedjarv.

Copy link
Contributor

@tleedjarv tleedjarv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't say anything about the prefixes but otherwise this looks good to me.

The previous ".unison." temp file prefix was used on Windows,
where hidden files begin with "~$". This change detects if the
OS is Windows, and will make temp files properly hidden with
a "~$" prefix if so.

Unix file prefixes were updated to begin with ".~", which extends
support for certain backup applications that sync files beginning
with ".", but not ".~".
@ChristianRiesen
Copy link

Just came across this again and wondered if this could be merged.
Ping @pgossman and @bcpierce00 perhaps to take another look?

@bcpierce00
Copy link
Owner

Here are the tradeoffs I see:

  • Changing the default names and doing nothing else has the (significant) advantage of being simple. But (1) if this change is motivated by Windows concerns, do we need to change anything on non-Windows systems, and (2) even on Windows we sure that this change won't break things for anybody?
  • Looking at an environment variable has three advantages over adding preferences: (1) it allows different choices on different hosts, and (2) it avoids changing the wire format (but maybe that's less of an issue with other changes in progress), and (3) it avoids adding more preferences to the (already large) list that people see when they ask what prefs are available. The downside is that these customizations are harder to find.

@ChristianRiesen
Copy link

ChristianRiesen commented Jul 27, 2021

The default change would work for me perfectly.

A potential issue is of course of someone configured externally some filter/ignore patttern and expects .unison. to be there instead of the two different ones now, depending on OS. If you are ok with the default change @bcpierce00 then that would solve my issue for certain.

If you envision changes down the road that make adding a parameter easier, you could still add a parameter once those changes are done.

@gdt
Copy link
Collaborator

gdt commented Jul 27, 2021

I am fuzzy now but I think this stalled partly because I'm not inclined to accommodate proprietary software that arguably should have a configurable ignore list on that side, and because it seems like there are perhaps OS-specific norms that we should follow. And further, perhaps, that the normal OS-specific norms aren't followed by dropbox.

rereading, it seems like this is basically a workaround for dropbox.

@acolomb
Copy link

acolomb commented Jul 27, 2021

I do see value in having the same (default) prefix on different OSes. Using different synchronization solutions in conjunction with Unison, and changing their ignore patterns based on which host we're on would make it harder.

Regarding the configuration method, I'd slightly prefer the option over environment vars. I wouldn't use it though, and in this case I think predictability is more important than accommodating other systems' shortcomings. (And tbh, Dropbox is really turning worse every month in so many aspects.)

@ChristianRiesen
Copy link

@gdt My initial use case comes up from Dropbox, yes. However now I'm also having another issue with this as there is a tool (I have no control over) that I can't exclude unison files from either. The .~ prefix would already solve that issue as well.

IMHO the endresult is that there is no actual standard in the "what file prefix/suffix to ignore" patterns and only having a single one (even if it matches a lot) will not match all use cases. My personal case would be solved with .~unison instead of .unison but a prefix option would make it a much more flexible approach, especially since there are no true standards in that realm.

@ChristianRiesen
Copy link

I am fuzzy now but I think this stalled partly because I'm not inclined to accommodate proprietary software that arguably should have a configurable ignore list on that side, and because it seems like there are perhaps OS-specific norms that we should follow. And further, perhaps, that the normal OS-specific norms aren't followed by dropbox.

rereading, it seems like this is basically a workaround for dropbox.

Just stumbled across this again. There is a tangible usecase to make it work with Dropbox, sure. It also supplies much more control over the temp files, which can be then configured to be ignored. An example I just came across again recently is an indexing engine that has a list of prefixes hardcoded. Another is a backup tool, again no way to tell it to just ignore these files as it takes it's own "logic" of what is and what isn't a temp file. Allowing this to be configureable would make unison a better tool, that helps in all those instances where other tools are failing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants