cmerge: add methods to get all conflicting keys #4515

atmaxinger · 2022-10-06T19:00:58Z

Fixes #4489

Basics

Short descriptions of your changes are in the release notes
(added as entry in doc/news/_preparation_next_release.md which
contains _(my name)_)
Please always add something to the release notes.
Details of what you changed are in commit messages
(first line should have module: short statement syntax)
References to issues, e.g. close #X, are in the commit messages.
The buildservers are happy. If not, fix in this order:
- add a line in doc/news/_preparation_next_release.md
- reformat the code with scripts/dev/reformat-all
- make all unit tests pass
- fix all memleaks
The PR is rebased with current master.

Checklist

I added unit tests for my code
I fully described what my PR does in the documentation
(not in the PR description)
I fixed all affected documentation (see Documentation Guidelines)
I added code comments, logging, and assertions as appropriate (see Coding Guidelines)
I updated all meta data (e.g. README.md of plugins and METADATA.ini)
I mentioned every code not directly written by me in reuse syntax

Review

Documentation is introductory, concise, good to read and describes everything what the PR does
Examples are well chosen and understandable
Code is conforming to our Coding Guidelines
APIs are conforming to our Design Guidelines
Code is consistent to our Design Decisions

Labels

Add the "work in progress" label if you do not want the PR to be reviewed yet.
Add the "ready to merge" label if the basics are fulfilled and no further pushes are planned by you.

atmaxinger · 2022-10-06T19:05:03Z

Currently, keys will still be marked as conflicting, even if the merge strategy (OURS, THEIRS) was able to automatically resolve the conflict. This is to be in line with the current implementation of elektraMergeGetConflicts.

markus2330

Great to see this feature! 🚀

Please add more introduction material: examples how to use the (new) API, basic introduction of how to use the library etc. Please also improve the Doxygen docu, add examples etc. (Examples as full files, which get compiled and included via @snippet.

src/libs/merge/kdbmerge.c

markus2330 · 2022-10-15T08:50:20Z

src/libs/merge/kdbmerge.c

@@ -181,6 +189,147 @@ static int getTotalNonOverlaps (Key * informationKey)
 	       getNonOverlapOnlyBaseConflicts (informationKey);
 }

+static void addConflictingKeys (Key * informationKey, KeySet * conflictingKeys, const char * metaName)
+{
+	increaseStatisticalValue (informationKey, metaName);


Do we need these statistics? Can you please document what they are for (if they are needed)? Please create src/libs/merge/README.md with a basic introduction of what this library can do.

Yes, this is how the implementation of the merge algorithm work.

markus2330 · 2022-10-15T08:50:36Z

doc/news/_preparation_next_release.md


- <<TODO>>
+- Add methods to check which keys were causing a merge conflict _(Maximilian Irlinger @atmaxinger)_.


How is this method called? Please add links.

markus2330 · 2022-10-15T08:52:20Z

src/libs/merge/kdbmerge.c

+	KeySet * meta = keyMeta (informationKey);
+
+	Key * conflictsRoot = keyNew (META_ELEKTRA_MERGE_CONFLICT, KEY_END);
+	for (elektraCursor end, it = ksFindHierarchy (meta, conflictsRoot, &end); it < end; ++it)


As you are the first using ksFindHierarchy (and seemingly it fits perfectly for you here), can you please improve the docu of ksFindHierarchy? (At the moment it is not public. @kodebach is a strong supporter of getting it public.). Alternatively, please remove your uses of ksFindHierarchy and mark ksFindHierarchy as internal in Doxygen, as it actually should have been.

Oh I didn't know it was considered internal API. I think it's very handy so I'd be open to document it. Why would you keep it internal?

Mostly because it is poorly documented and tested, so if this fixed the function has very good potential to get public.

The only other problem might be overlap with other functions (is the API still minimal after adding it?). I didn't check this enough, though. @lawli3t did you look at ksFindHierarchy?

But even if it doesn't end up in elektra-core, we can still have it another lib.

So I've read again through the doxygen documentation for ksFindHierarchy and I find it quite understandable. What do you think is missing @markus2330 ?

Testing-wise, there are quite a few automated unit tests for this function too. I'm not sure what you mean by it is poorly tested.

I support @kodebach in that it should be made public. It contains quite a few optimizations that would be hard to reimplement outside of the core code of Elektra, especially now with copy-on-write.

markus2330 · 2022-10-15T08:55:10Z

src/libs/merge/kdbmerge.c

@@ -1,4 +1,3 @@
-
 #include "kdbmerge.h"
 #include "kdb.h"
 #include "kdbassert.h"


Please document what you use from #include "kdbprivate.h"

done, because of ksFindHierarchy

src/libs/merge/kdbmerge.c

markus2330 · 2022-10-15T09:01:28Z

src/libs/merge/kdbmerge.c

@@ -220,6 +369,35 @@ static char * strremove (char * string, const char * sub)
 	return string;
 }

+static Key * prependStringToKeyName (Key * key, const char * string, Key * informationKey)


Please have at least short docu also for internal stuff (e.g. why is /root special, preconditions, etc.)

Sure, but it will take some time. I didn't really write this function, just extracted it when I refactored another method.

Yeah, it is difficult to get used to other code. You are doing a great job here.

markus2330 · 2022-10-16T05:47:55Z

@atmaxinger Can you rescue some stuff from #3260?

Co-authored-by: Markus Raab <markus2330@users.noreply.github.com>

atmaxinger · 2023-02-03T19:48:01Z

jenkins build libelektra please

atmaxinger · 2023-02-06T08:42:09Z

jenkins build libelektra please

atmaxinger · 2023-02-06T16:41:44Z

jenkins build libelektra please

atmaxinger · 2023-02-06T18:26:10Z

@markus2330 could you please re-review this? I think I have addressed all of your points. Merging this PR would be a big deal for #3131

atmaxinger requested a review from markus2330 October 6, 2022 19:05

markus2330 suggested changes Oct 15, 2022

View reviewed changes

atmaxinger force-pushed the merge-get-conflicts branch from 356c4c3 to 5a0760e Compare October 18, 2022 19:51

markus2330 mentioned this pull request Oct 26, 2022

[decision] Change Tracking #4554

Merged

22 tasks

atmaxinger force-pushed the merge-get-conflicts branch from 9ad8bce to 2be5871 Compare January 27, 2023 19:44

atmaxinger requested a review from markus2330 January 27, 2023 22:09

atmaxinger mentioned this pull request Jan 27, 2023

3-way merge: elektraGetConflicts should return conflicting keys #4489

Closed

atmaxinger and others added 18 commits February 3, 2023 19:50

cmerge: add methods to get all conflicting keys

9981834

Restyled by clang-format

fb2ce73

merge: fix leaks in test_merge.c

dff3620

merge: fix leaks related to information key meta

91ad128

Restyled by clang-format

f233670

merge: start some documentation

c65b00a

Update src/libs/merge/kdbmerge.c

e72b3cb

Co-authored-by: Markus Raab <markus2330@users.noreply.github.com>

cmerge: fix compilation of Python bindings

3a8472a

cmerge: document why kdbprivate.h is included

10a08b2

Update src/libs/merge/kdbmerge.c

4aea225

Co-authored-by: Markus Raab <markus2330@users.noreply.github.com>

cmerge: remove outdated symbol

729aee9

Restyled by clang-format

cdaad59

Restyled by prettier-markdown

cb74cb3

cmerge: handle all cases in switch

9a42be8

cmerge: work around enum wrapping in python binding

20d4ed4

cmerge: remove "merge session" wording

ae968ca

cmerge: add example

e3365ff

Restyled by clang-format

adb8158

atmaxinger force-pushed the merge-get-conflicts branch from 9d81ed3 to adb8158 Compare February 3, 2023 18:51

markus2330 merged commit 3e43445 into ElektraInitiative:master Feb 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmerge: add methods to get all conflicting keys #4515

cmerge: add methods to get all conflicting keys #4515

atmaxinger commented Oct 6, 2022 •

edited

Loading

atmaxinger commented Oct 6, 2022

markus2330 left a comment

markus2330 Oct 15, 2022

atmaxinger Oct 15, 2022

markus2330 Oct 15, 2022

markus2330 Oct 15, 2022

atmaxinger Oct 15, 2022

markus2330 Oct 15, 2022

atmaxinger Jan 27, 2023

markus2330 Oct 15, 2022

atmaxinger Jan 27, 2023

markus2330 Oct 15, 2022

atmaxinger Oct 15, 2022

markus2330 Oct 15, 2022

markus2330 commented Oct 16, 2022

atmaxinger commented Feb 3, 2023

atmaxinger commented Feb 6, 2023

atmaxinger commented Feb 6, 2023

atmaxinger commented Feb 6, 2023


		- <<TODO>>
		- Add methods to check which keys were causing a merge conflict _(Maximilian Irlinger @atmaxinger)_.

cmerge: add methods to get all conflicting keys #4515

cmerge: add methods to get all conflicting keys #4515

Conversation

atmaxinger commented Oct 6, 2022 • edited Loading

Basics

Checklist

Review

Labels

atmaxinger commented Oct 6, 2022

markus2330 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markus2330 commented Oct 16, 2022

atmaxinger commented Feb 3, 2023

atmaxinger commented Feb 6, 2023

atmaxinger commented Feb 6, 2023

atmaxinger commented Feb 6, 2023

atmaxinger commented Oct 6, 2022 •

edited

Loading