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

Better names for conflicting files #1820

Closed
vxgmichel opened this issue Aug 30, 2021 · 2 comments
Closed

Better names for conflicting files #1820

vxgmichel opened this issue Aug 30, 2021 · 2 comments
Assignees

Comments

@vxgmichel
Copy link
Contributor

Quoting touilleMan:

Using remote_device_name correspond to a DeviceID which doesn't seems like a good idea given the user has no idea who this correspond to.
In such case it would be much better to do (by order of simplicity):

  1. conflict, conflict (2) etc.
  2. conflict 2000-01-01T12:12:12Z, conflict 2000-01-01T12:12:12Z (2) etc.
  3. conflict with John (using user label)

As a matter of fact I'm not really sure what 2) and 3) brings to the table given modification date and author are supposed to be easy to see with the history feature.
So I would vote for the much simpler solution 1) (and wait for a real user push before going more complex, especially considering nobody complained so far that conflict <uuid>@<uuid> is not that helpful 😆 )

There's also the problem of localization: the names should take into account the language configured in the global configuration.

My choice would go to either 1 or 3:

  • 1 because it's simple and usually a good a idea to wait for user feedback
  • 3 because the user label seems to be the most useful information to display
@vamonte vamonte self-assigned this Sep 6, 2021
@vamonte
Copy link
Contributor

vamonte commented Sep 7, 2021

@touilleMan @vxgmichel
I investigate this issue and before to write any code I checked the dedicated tests.
I tried to improve the test test_merge_folder_children to verify what happened when the filename built by the resolve conflict logic was already used. (I had one doubt because the function merge_folder_children didn't used get_conflict_filename to build the conflict filename.)

Thanks to the new test, I think, I find one new bug.
Below I will explain my logic. The code is avalaible on this PR (#1835).

   local_a = EntryID.new()
   remote_a = EntryID.new()
   common_child = EntryID.new()
   a1 = {"a":  local_a}
   a2 = {"a": remote_a}
   
   # The remote manifest and the local manifest have one same child named "a (conflicting with a@a)"
   # and one conflict on the child name "a"
   base = {"a (conflicting with a@a)":  common_child}
   a3 = {**base,  **a1}
   b3 = {**base,  **a2}

   result = merge_folder_children(base, a3, b3, "a@a")
   # I didn't know what was the logic to build the filename in this case 
   # so I used the same logic as the `get_conflict_filename` to my assert condition
   assert result == {
     "a":  remote_a, 
     "a (conflicting with a@a)": common_child, 
     "a (conflicting with a@a - 2)":  local_a
   }

This assert is always False, but two differents results happen randomly.

First case:

assert result == { 
  "a":  remote_a,  
  "a (conflicting with a@a)":  local_a
} 

In this case the common_child is removed and the local_a use its name.
We lose one file

Second case:

assert result == { 
  "a":  remote_a,  
  "a (conflicting with a@a)":  local_a,
  "a (conflicting with a@a) (conflicting with a@a)":  common_child
} 

In this case the logic detect the conflict on local_a, and rename it then it detect one conflict on common_child and rename it too.
One common file is renamed needlessly while a second file use its old name

The reason of the two differents results is in the merge_folder_children function.
Indeed in this function we do an union on two sets and the results is unordered. (l99)

  # All ids that might remain
   ids = set(local_reversed) | set(remote_reversed)

If the common_child variable is before the local_a variable in the ids list then we are in the first case else we are in the second case.

@vxgmichel
Copy link
Contributor Author

Done in #1857

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

No branches or pull requests

2 participants