atom-community / sync-settings

Synchronize all your settings and packages across atom instances
https://atom.io/packages/sync-settings
MIT License
1.14k stars 105 forks source link

Project-specific settings cause diff view to show wrong value #738

Open savetheclocktower opened 2 years ago

savetheclocktower commented 2 years ago

Describe the bug

Project-specific settings are a feature provided by both the project-config and atomic-management packages. They do this not through monkeypatching or any similar trick, but via APIs that have been in Atom since this PR landed three years ago. (Other packages may use these APIs as well, but these are the ones I know of.) My point is that this isn’t just a problem caused by a rogue package; it could just as easily be triggered by a user with a clever init-file.

The effect of these APIs is that sync-settings can’t necessarily trust atom.config.get to return settings as they exist in the user’s config.cson file. Any such setting may be modulated through a project’s own specific config overrides — which sync-settings can safely ignore, since those settings come with their own synchronization mechanism.

It turns out that sync-settings largely does the right thing here by checking atom.config.settings directly when deciding if settings have changed — but it still calls atom.config.get when building the diff view if those settings have changed. So in the case where you change a setting in your config.cson but your local project window has overridden that setting, the diff view will retrieve that project’s value instead of the right one. This further means that the diff view can show different results based on which window you run “View Diff” from.

I haven’t gone so far as to try to backup from a view that’s doing the wrong thing — I don’t know if the wrong value is sync’d.

To Reproduce

Steps to reproduce the behavior:

  1. Install project-config.
  2. Create an .atom/config.json file that overrides a setting in your config.cson. I’ll use this one as an example — assume the setting is 7 in both your config.cson and the remote backup:

    {
    "zentabs": { "maximumOpenedTabs": 8 }
    }
  3. Reload the window for that project.
  4. Open your config.cson and change the value to something completely new, like 9.
  5. Run the “Sync Settings: View Diff” command.
  6. You’ll see zentabs.maximumOpenedTabs: 8 as the local value.

Expected behavior

You ought to see zentabs.maximumOpenedTabs: 9.

Versions

Additional context

As far as I can tell, the fix here is simply never to call atom.config.get directly, and instead to use a utility function that behaves like Lodash’s get method but retrieves keys directly from atom.config.settings.

savetheclocktower commented 2 years ago

I should point out that if you skip step 4 of the repro instructions, both “Check Backup” and “View Diff” behave as expected, because getDiffData doesn't report that the setting has changed at all.

And I just confirmed that the correct value is actually synchronized instead of the project-specific one. So instead of syncing the wrong thing, the diff view merely misrepresents what changes will actually be made if the user clicks on “Backup,” which is the better of the two outcomes but still not great.