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

3-way merge with OURS strategy #2

Open
markus2330 opened this issue Nov 19, 2019 · 9 comments
Open

3-way merge with OURS strategy #2

markus2330 opened this issue Nov 19, 2019 · 9 comments
Assignees

Comments

@markus2330
Copy link
Contributor

No description provided.

@FelixResch
Copy link
Collaborator

I added an implementation of the three-way merge based on Elektra Tools in the branch elektra-backend-threeway-merge.

I will add a few more tests and then create a PR.

@markus2330
Copy link
Contributor Author

Thank you, but please use the new merging API from @Chemin1: src/libs/merge

Please ask if you have any questions.

@markus2330
Copy link
Contributor Author

We got an answer from David Faure:

(IIRC) what KConfig does in such a case, at step 3, is to reload the file from
disk but without touching the entries marked as dirty (i.e. modified by the
configuration dialog), and then save the result.
I.e. this will overwrite entries modified by both, but preserve entries
modified externally and not modified by the application.
The comment at the top of KConfigIniBackend::parseConfig (and near the call to
that method) confirms this.
I can't pinpoint the actual logic in the code itself though (I don't know it
in full details).

@FelixResch did you spot such a code? Does the code do what it should do? Can you try if manually edited changes are really not lost?

@Chemin1 is this equivalent to 3-way merging or in which cases is 3-way merging better?

@FelixResch
Copy link
Collaborator

did you spot such a code? Does the code do what it should do? Can you try if manually edited changes are really not lost?

In KConfig.sync() the configuration is simply written to the backing storage.

In KConfig.reparseConfiguration() the changes are written to the backing storage by calling sync() and then the file is reread.

Hence the merging is actually done in reparseConfiguration() and not in sync(). The current implementation of the Elektra backend already reloads the changes during the call to sync(), but this can be removed.

@markus2330
Copy link
Contributor Author

My question was about the unmodified code with the INI backend (the kconfig repo before we did anything), what is this code doing? And please also try if it actually works (manually edit a file and then press the save button in the configure dialog).

@FelixResch
Copy link
Collaborator

My question was about the unmodified code with the INI backend (the kconfig repo before we did anything), what is this code doing?

This is the first part of my answer (We haven't changed the implementation of these methods)

And please also try if it actually works (manually edit a file and then press the save button in the configure dialog).

Yes, this works

@markus2330
Copy link
Contributor Author

Thank you for looking into it!

@Chemin1 can you please analyze if your algorithm will perform the same or better than this marking-dirty-configuration approach? The dirty approach sounds much simpler than a 3-way merge with our strategy.

@dominicjaeger
Copy link

is to reload the file from
disk but without touching the entries marked as dirty (i.e. modified by the
configuration dialog), and then save the result.

File was loaded once (base) and modified (our), then reloaded (their)

I.e. this will overwrite entries modified by both, but preserve entries
modified externally and not modified by the application

overwrite = use the marked version (our) and not the reloaded one (their)

@Chemin1 is this equivalent to 3-way merging or in which cases is 3-way merging better?

=> I think equivalent.

@Chemin1 can you please analyze if your algorithm will perform the same or better than this marking-dirty-configuration approach? The dirty approach sounds much simpler than a 3-way merge with our strategy.

As both seem to use three-way merging, the difference should be how good Elektra's and KConfig's Backends handle the properties of INI. I can analyze this, but I can't promise that I'll be able to finish it very quickly as I'll be participating in a tournament this weekend.

@markus2330
Copy link
Contributor Author

This is not needed, thank you for looking into it!

@FelixResch FelixResch moved this from To do to In progress in elektrify kconfig Nov 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
elektrify kconfig
  
In progress
Development

No branches or pull requests

3 participants