[djbclark]

--simulate=diff aborted an assert-enabled cf-agent when the same package was recorded as installed twice without a version: DiffPkgOperations() passed the two empty versions to PkgVersionIsGreater(), which asserts both non-empty. ManifestPkgOperations() already guards that comparison; the diff branch now uses the same guard and keeps the same record (versioned over unversioned, otherwise the higher version, otherwise the first). Release builds compile the assert out and already picked the same record, so nothing changes for them. Two unit tests; the first aborts on the unguarded code (library built with asserts on):

$ tests/unit/simulate_mode_test      # without the fix
test_diff_repeated_unversioned_install: Starting test
simulate_mode_test: simulate_mode.c:676: PkgVersionIsGreater: Assertion `!NULL_OR_EMPTY(ver1) && !NULL_OR_EMPTY(ver2)' failed.
Aborted
$ tests/unit/simulate_mode_test      # with the fix
test_diff_repeated_unversioned_install: Test completed successfully.
test_diff_unversioned_then_versioned_install: Test completed successfully.
All 7 tests passed

Ubuntu 24.04 (--enable-debug): make -C tests/unit check → All 76 tests passed; tests/acceptance/29_simulate_mode/ as root → 4 of 4 Pass. Fork CI: djbclark#45

AI-assisted. I can't vouch for the C syntax beyond a basic level, but I did look at the logic and it seemed sound.

🤖 Generated with Claude Code

[cf-bottom]

Thanks for submitting a PR! Maybe @larsewi can review this?

[larsewi]

Thanks for the contribution 🚀 Could you also add tests for:

• Two unversioned, reported once
• An unversioned record never replaces an existing one
• A versioned record replaces a unversioned one
• First is higher version, keep first
• Last is higher version, keep last
• Same version, reported once

[larsewi]

Maybe you could break this up a bit so that it is easier to read for a human? E.g.:

Suggested change

	}
	static bool PkgRecordShouldBeReplaced(const PkgOperationRecord *prev_record,
	const char *new_ver)
	{
	if (prev_record == NULL)
	{
	/* Nothing recorded yet */
	return true;
	}
	if (NULL_OR_EMPTY(new_ver))
	{
	/* An unversioned record never replaces an existing one */
	return false;
	}
	if (NULL_OR_EMPTY(prev_record->pkg_ver))
	{
	/* A versioned record is more specific than an unversioned one */
	return true;
	}
	/* Both are versioned, keep the higher version */
	return PkgVersionIsGreater(new_ver, prev_record->pkg_ver);
	}

Then use this function as the if condition.

[larsewi]

Could you replace the strstr() function calls with StringContains from string_lib.h?

[djbclark]

Added unit tests for all requested cases in simulate_mode_test.c:

• Two unversioned, reported once (test_diff_repeated_unversioned_install)
• An unversioned record never replaces an existing one (test_diff_versioned_then_unversioned_install)
• A versioned record replaces an unversioned one (test_diff_unversioned_then_versioned_install)
• First is higher version, keep first (test_diff_install_higher_version_first_keeps_first)
• Last is higher version, keep last (test_diff_install_higher_version_last_keeps_last)
• Same version, reported once (test_diff_repeated_versioned_install)

[djbclark]

K will do thanks for the clear actionable suggestions

…

On Tue, Nov 3, 2026, 5:12 AM Lars Erik Wik ***@***.***> wrote: ***@***.**** requested changes on this pull request. Thanks for the contribution 🚀 Could you also add tests for: - Two unversioned, reported once - An unversioned record never replaces an existing one - A versioned record replaces a unversioned one - First is higher version, keep first - Last is higher version, keep last - Same version, reported once ------------------------------ In cf-agent/simulate_mode.c <#6424 (comment)>: > Maybe you could break this up a bit so that it is easier to read for a human? E.g.: ⬇️ Suggested change +} + +static bool PkgRecordShouldBeReplaced(const PkgOperationRecord *prev_record, + const char *new_ver) +{ + if (prev_record == NULL) + { + /* Nothing recorded yet */ + return true; + } + + if (NULL_OR_EMPTY(new_ver)) + { + /* An unversioned record never replaces an existing one */ + return false; + } + + if (NULL_OR_EMPTY(prev_record->pkg_ver)) + { + /* A versioned record is more specific than an unversioned one */ + return true; + } + + /* Both are versioned, keep the higher version */ + return PkgVersionIsGreater(new_ver, prev_record->pkg_ver); +} Then use this function as the if condition. ------------------------------ In tests/unit/simulate_mode_test.c <#6424 (comment)>: > @@ -147,6 +147,40 @@ static void test_manifest_install_cancels_removal(void) unlink(ToChangesChroot(CHROOT_PKGS_OPS_FILE)); } +/* Two recorded installations of the same package without a version must be + * reported once, not abort on comparing the empty versions. */ +static void test_diff_repeated_unversioned_install(void) +{ + write_pkgs_ops("i,foo,,\r\n" + "i,foo,,\r\n"); + + char output[4096]; + assert_true(call_with_captured_stdout(&DiffPkgOperations, output, sizeof(output))); + + const char *msg = "Package 'foo' would be installed"; + const char *first = strstr(output, msg); Could you replace the strstr() function calls with StringContains from string_lib.h? — Reply to this email directly, view it on GitHub <#6424?email_source=notifications&email_token=AABAGYAH4O25Z3BRPC5TVSD5XGVRLA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKNZVGAYTSOJVGQZKM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#pullrequestreview-5750199542>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABAGYA7N7PTBKIFWE7RNZD5XGVRLAVCNFSNUABDKJSXA33TNF2G64TZHMZTKNBTGU2DQO2JONZXKZJ3GU4TGOJTHE4TONBRUF3AE> . You are receiving this because you authored the thread.Message ID: ***@***.***>