magic-folder .backup files are wrong #2909

New Issue

meejah · 2018-03-21T17:01:24Z

meejah commented

2018-03-21 17:01:24 +00:00

The behavior of producing .backup files is wrong (and I believe the spec is correct). Currently, whenever we decide to download a file, a .backup is produced if there was already a file there.

But, consider the case where Alice and Bob share a folder, and Alice makes a series of updates to a file foo: on the first update, Bob downloads it and writes it; on the second, Bob downloads but already has one so makes a foo.backup as well; on the third, Bob downloads but already has one and a foo.backup so produces foo.conflict.

The behavior of producing .backup files is wrong (and I believe the spec is correct). Currently, whenever we decide to download a file, a `.backup` is produced if there was already a file there. But, consider the case where Alice and Bob share a folder, and Alice makes a series of updates to a file `foo`: on the first update, Bob downloads it and writes it; on the second, Bob downloads but already has one so makes a `foo.backup` as well; on the third, Bob downloads but already has one *and* a `foo.backup` so produces `foo.conflict`.

meejah added the

code-frontend-magic-folder

normal

defect

1.12.1

labels 2018-03-21 17:01:24 +00:00

meejah added this to the undecided milestone 2018-03-21 17:01:24 +00:00

meejah commented

2018-03-21 21:55:54 +00:00

I'm fairly sure that (amongst other problems) also this conflict-case is wrong: https://github.com/tahoe-lafs/tahoe-lafs/blob/master/src/allmydata/frontends/magic_folder.py#L1279

We just blindly write .backup files all the time (i.e. anything that's downloaded) which is obviously wrong; but the conflicted cases don't seem to follow the spec either..

I'm fairly sure that (amongst other problems) also this conflict-case is wrong: <https://github.com/tahoe-lafs/tahoe-lafs/blob/master/src/allmydata/frontends/magic_folder.py#L1279> We just blindly write .backup files all the time (i.e. anything that's downloaded) which is obviously wrong; but the conflicted cases don't seem to follow the spec either..

exarkun commented

2019-03-20 16:33:45 +00:00

I think the link in meejah's previous comment was probably to https://github.com/tahoe-lafs/tahoe-lafs/blob/797932244da3036e0dd2463961a30cf2d319d99b/src/allmydata/frontends/magic_folder.py#L1279 (before new changes landed on master).

It looks like the logic has changed since this comment was made. I think meejah fixed the issue in affb80e39e33417abc2935c2da4e577173913f91 (or at least changed the code).

I think the link in meejah's previous comment was probably to <https://github.com/tahoe-lafs/tahoe-lafs/blob/797932244da3036e0dd2463961a30cf2d319d99b/src/allmydata/frontends/magic_folder.py#L1279> (before new changes landed on master). It looks like the logic has changed since this comment was made. I think meejah fixed the issue in affb80e39e33417abc2935c2da4e577173913f91 (or at least changed the code).

exarkun commented

2019-03-20 16:39:03 +00:00

It also looks like the .backup logic has changed. Previously, thanks to https://github.com/tahoe-lafs/tahoe-lafs/blob/797932244da3036e0dd2463961a30cf2d319d99b/src/allmydata/frontends/magic_folder.py#L958, any non-conflicted file being written would result in a backup file, including the case described in this ticket's description.

However, it looks like meejah fixed this behavior in 47b17876339443145e95081e9581327229a6f064 by removing the backup_path_u argument to fileutil.replace_file. This causes fileutil.replace_file to not create a backup file. Thus, backups are only created when metadata indicates the new version of the file is a deletion.

The merge for this was c9e00a988ae90ad5e63897607c70404974d370b2:

commit c9e00a988ae90ad5e63897607c70404974d370b2
Merge: 458889682 541493018
Author: meejah <meejah@meejah.ca>
Date:   Tue May 1 15:52:10 2018 -0600

    Merge pull request #475 from meejah/2909.backup-behavior.0
    
    #2909 fix .backup file behavior and (some of) the incorrect .conflict cases (#2911)

Therefore, closing as fixed.

It also looks like the `.backup` logic has changed. Previously, thanks to <https://github.com/tahoe-lafs/tahoe-lafs/blob/797932244da3036e0dd2463961a30cf2d319d99b/src/allmydata/frontends/magic_folder.py#L958>, any non-conflicted file being written would result in a backup file, including the case described in this ticket's description. However, it looks like meejah fixed this behavior in 47b17876339443145e95081e9581327229a6f064 by removing the `backup_path_u` argument to `fileutil.replace_file`. This causes `fileutil.replace_file` to not create a backup file. Thus, backups are only created when metadata indicates the new version of the file is a deletion. The merge for this was c9e00a988ae90ad5e63897607c70404974d370b2: ``` commit c9e00a988ae90ad5e63897607c70404974d370b2 Merge: 458889682 541493018 Author: meejah <meejah@meejah.ca> Date: Tue May 1 15:52:10 2018 -0600 Merge pull request #475 from meejah/2909.backup-behavior.0 #2909 fix .backup file behavior and (some of) the incorrect .conflict cases (#2911) ``` Therefore, closing as fixed.

exarkun added the

fixed

label 2019-03-20 16:39:03 +00:00

exarkun closed this issue

2019-03-20 16:39:03 +00:00

Sign in to join this conversation.