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

Merge: Create merge API for tooling #3202

Closed
dominicjaeger opened this issue Nov 12, 2019 · 9 comments
Closed

Merge: Create merge API for tooling #3202

dominicjaeger opened this issue Nov 12, 2019 · 9 comments

Comments

@dominicjaeger
Copy link
Contributor

We should write an API call that performs a three-way merge not on three sets but on three paths in Elektra. It should be able to have different behavior with data that is already in the paths.

Yes I agree, there is also the problem that the flags "that modify the result set" and the "strategy" flags can be combined in any way. So yes, elektraMergeResult(ours, theirs, base, char * strategy, result, char * behavior) might be the solution for tooling (and still providing a normal 3-way merge, called elektraMerge, for non-tools, i.e., that do not write the result to KDB).

This also means we do not only have a strategy, but also the behavior for what to do with the result (e.g. to wipe out, to overwrite, to fail on overwrite, to fail on non-empty).

As explained by @markus2330 in PR #3106.

@markus2330
Copy link
Contributor

Thank you for creating the issue! What is your proposed API? One KeySet with 4 parentKeys?

@dominicjaeger
Copy link
Contributor Author

I imagined three separate parameters, as you proposed elektraMergeResult(ours, theirs, base, char * strategy, result, char * behavior).
elektraMerge takes three KeySet *. Using three Key * for elektraMergeResult would look consistent to me. At the loss of some safety, would it make sense to use char* or can all the possible callers of elektraMergeResult create the required KeySets as easy as kdb with ckdb::KeySet?

@markus2330
Copy link
Contributor

markus2330 commented Nov 13, 2019

Then I do not understand the sentence "not on three sets but on three paths". The proposed API simply uses four sets instead of three.

elektraMergeResult would look consistent to me

I agree, it is also more consistent to me, and also more flexible. Only the amounts of parameters is a bit scary.

At the loss of some safety, would it make sense to use char*

Please write the whole proposed signature here, it is unclear where you want to use char *.

or can all the possible callers of elektraMergeResult create the required KeySets as easy as kdb with ckdb::KeySet?

I do not understand this question. In #3131 I listed all the callers.

@dominicjaeger
Copy link
Contributor Author

dominicjaeger commented Nov 13, 2019

I left out for example informationKey because it doesn't look relevant for the main idea.

int elektraMergeResult(Key * our, Key * their, Key * base, Key * result, int mergeStrategy, int behavior) {
    if (behavior == fail on non-empty) {
        return if something is below result;
    }
    KeySet * ourSet = get all keys below our;
    KeySet * theirSet = get all keys below their;
    KeySet * baseSet = get all keys below base;
    // Does not read or write anything from or to database, does not have behavior anymore
    KeySet * result = elektraMerge(ourSet, theirSet, baseSet, mergeStrategy);
    if (behaviour == delete everything in result path) {
        delete everything below result;
    }
    Write all keys from result to the database
    return 0;
}

But we could also do

// our, their, base and result are representations of paths in Elektra like "user/something/to/merge"
int elektraMergeResult(char * our, char * their, char * base, char * result, int mergeStrategy, int behavior) 

In #3131 I listed all the callers.

I was not aware that this list is exhaustive. In this case Key * might be better.

@markus2330
Copy link
Contributor

In the signature KeySet is missing. So it is only about Key vs. char?

@dominicjaeger
Copy link
Contributor Author

In the signature KeySet is missing.

I thought the whole discussion was about not using KeySet?

@markus2330
Copy link
Contributor

Then I do not understand how it should work for tools. The whole conflict handling in tools assumes that there is a long-living KDB handle, which was not able to do a kdbSet. Did you read the documentation about kdbSet? It seems like this also needs to be covered in the merging tutorial if it is not clear even to you. I added it in #280. Let us discuss this today.

@stale
Copy link

stale bot commented Nov 13, 2020

I mark this issue stale as it did not have any activity for one year. I'll close it in two weeks if no further activity occurs. If you want it to be alive again, ping the issue by writing a message here or create a new issue with the remainder of this issue.
Thank you for your contributions 💖

@stale stale bot added the stale label Nov 13, 2020
@stale
Copy link

stale bot commented Nov 27, 2020

I closed this issue now because it has been inactive for more than one year. If I closed it by mistake, please do not hesitate to reopen it or create a new issue with the remainder of this issue.
Thank you for your contributions 💖

@stale stale bot closed this as completed Nov 27, 2020
Improve 3-way merge automation moved this from To do to Done Nov 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

2 participants