Repository navigation
Fix: empty array doesn't override an array of tables in toml_merge - #53
Closed
afonsojanu wants to merge 1 commit into
Closed
afonsojanu wants to merge 1 commit into
afonsojanu wants to merge 1 commit into
Conversation
is_array_of_tables() returns true for an empty array too, since a loop
over zero elements never finds one that fails the TOML_TABLE check.
That was fine on its own, but datum_merge()'s TOML_ARRAY case treats
"both sides are arrays of tables" as a signal to append src onto dst
rather than override it, and an empty array happens to satisfy that
check vacuously.
So merging a document that explicitly sets an existing array-of-tables
key to an empty array (arr = []) doesn't clear it as intended. Instead
the merge takes the append branch, appends zero elements from the
empty array, and leaves dst's original array of tables completely
untouched:
r1: [[arr]]
x = 1
r2: arr = []
toml_merge(r1, r2) -> arr still has one table, size stays 1,
when the override to [] should have taken effect
Excluding the empty case from is_array_of_tables() makes it fall
through to the plain override path instead, same as any other
non-array-of-tables src value.
Added two tests to test/merge/test1.c covering both directions: an
array of tables overridden by an empty array (the bug), and an empty
array overridden by an array of tables (already worked, kept as a
sanity check since the fix touches the same condition).
Owner
|
Thanks! This landed on main as ae0ce2f (squash-merged locally, so GitHub shows the PR as closed rather than merged). Authorship is preserved on the commit. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
`is_array_of_tables()` returns true for an empty array as well, since a loop over zero elements never runs into one that fails the `TOML_TABLE` check. On its own that's harmless, but `datum_merge()`'s `TOML_ARRAY` case uses "both sides are arrays of tables" as the signal to append src onto dst instead of overriding it, and an empty array satisfies that check vacuously.
So merging in a document that explicitly resets an existing array-of-tables key to an empty array doesn't clear it like it should. The merge takes the append branch, appends zero elements from the empty array, and leaves dst's original array of tables untouched:
Excluding the empty-array case from `is_array_of_tables()` makes that combination fall through to the plain override path instead, same as any other src value that isn't a genuine array of tables. I checked this doesn't change behavior for any of the other size/type combinations the previous `is_array_of_tables(*dst) && is_array_of_tables(src)` check (from #44) was meant to distinguish, since an empty array on either side that's genuinely being appended-to or replaced by a real array of tables gets the same end result either way.
Added two cases to `test/merge/test1.c`: one for the array-of-tables-overridden-by-empty-array direction (fails without the fix), and the reverse direction as a sanity check that it still works.
Ran `make clean test` locally; everything passes except `test-stdtest`, which needs the `toml-test` Go binary that wasn't available in my environment and is unrelated to this change.