diff BUGFIX user-ordered replace operation #2439
Conversation
src/diff.c
Outdated
| /** | ||
| * @brief In user-ordered lists, certain operations on sibling nodes can result in logically identical changes. | ||
| * However, applying the first change may cause the second one to fail. | ||
| * To prevent this, the affected node is unlinked. |
There was a problem hiding this comment.
Seems the affected node is freed, not just unlinked.
src/diff.c
Outdated
| size_t bufflen1 = 0, buffused1 = 0; | ||
| size_t bufflen2 = 0, buffused2 = 0; | ||
|
|
||
| /* Itereate throught previous nodes and look for logically identical changes */ |
There was a problem hiding this comment.
Typo and please keep the exact same comment formatting, starts with a lowercase letter.
src/diff.c
Outdated
| size_t bufflen2 = 0, buffused2 = 0; | ||
|
|
||
| /* Itereate throught previous nodes and look for logically identical changes */ | ||
| while (diff_iter->prev->next != NULL) { |
There was a problem hiding this comment.
!= NULL is redundant and we do not write it.
src/diff.c
Outdated
| metas[1] = lyd_find_meta(diff_iter->meta, mod, "orig-key"); | ||
|
|
||
| /* If keys don't match, skip - not a candidate for cyclic change */ | ||
| if (strcmp(lyd_get_meta_value(metas[0]), lyd_get_meta_value(metas[1]))) { |
There was a problem hiding this comment.
Are the metadata guaranteed to be found? Otherwise you get a SEGFAULT.
src/diff.c
Outdated
| if ((ret = lyd_unlink_tree(*diff))) { | ||
| return ret; | ||
| } | ||
|
|
||
| lyd_free_all(*diff); |
There was a problem hiding this comment.
Can be implemented by simply calling lyd_free_tree(*diff).
src/diff.c
Outdated
| free(buff1); | ||
| buff1 = NULL; | ||
| bufflen1 = 0; | ||
| buffused1 = 0; | ||
| free(buff2); | ||
| buff2 = NULL; | ||
| bufflen2 = 0; | ||
| buffused2 = 0; |
There was a problem hiding this comment.
Seems like a good candidate for a cleanup label where everything is freed. In between iterations it should be enough to zero buffused.
src/diff.c
Outdated
| ret = lyd_diff_merge_replace(&diff_node, cur_op, src_diff); | ||
|
|
||
| /* diff_node was unlinked and do not need any further changes */ | ||
| if (!diff_node) { |
There was a problem hiding this comment.
Up for a discussion, redundant nodes are detected in lyd_diff_is_redundant() at the function end, perhaps that is where the check should be.
2 participants
No description provided.