libmport: update local package on offline install when file is newer

[laffer1]

Summary

For offline / local-file installs (mport install <file.mport>), if the same package is already installed on the current OS release and the local file carries a newer version, mport now removes the installed copy and installs the file — an in-place update — instead of refusing with "already installed". The previous automatic flag is preserved.

• Without --force, a same-or-older local file still reports already installed (unchanged behavior).
• With --force, it reinstalls regardless of version (unchanged behavior).
• New helper lookup_current_os_installed() only matches an installed package whose os_release equals the running system's, so a package from a different OS release is not treated as a version bump.
• Deletes the actually-installed package meta (not the bundle stub) before the conflict/installed prechecks.
• mport.1 documents the new behavior.

Verification

• Builds clean under -Werror (cd libmport && make).
• Traced memory: already_installed is freed on the success path and in cleanup, NULL'd after each free (no leak / no double-free). mport_delete_primative does not free its pack arg, so freeing the array afterward is safe.
• Version direction confirmed against mport_version_cmp contract (-1 when first < second) and the existing check_if_older_installed usage.
• os_release gating is consistent with check_if_installed; --force cross-release installs are unaffected.
• Precommit cppcheck/splint: no new actionable findings (the one cppcheck warning at install_primative.c:251 is pre-existing; splint out-of-bounds notes are the same false-positive class this file already emits).

Not covered

No automated test exercises the new offline update-on-newer path — the ATF suite is registry-gated, so this path is verified by code tracing and a clean -Werror build rather than at runtime.

🤖 Generated with Claude Code

Summary by Sourcery

Allow offline installs from local package files to replace an already-installed package on the current OS release when the local file is newer, and document the behavior.

New Features:

• Support updating an already-installed package during offline/local-file installs when the provided package has a newer version on the current OS release.

Enhancements:

• Preserve the previous installation's automatic flag when performing an in-place update from a local package file.
• Restrict offline update checks to packages installed for the current OS release via a dedicated lookup helper.

Documentation:

• Document the new offline install behavior for updating installed packages with newer local package files in the mport(1) manual.

[sourcery-ai]

🧙 Sourcery has finished reviewing your pull request!

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[github-actions]

Claude finished @laffer1's task in 3m 40s —— View job

---

Claude finished reviewing this PR

• Gather context (diff, changed files, related helpers)
• Understand the request (code review of PR libmport: update local package on offline install when file is newer #173)
• Trace install_primative.c version/ownership logic against mport_delete_primative, mport_version_cmp, check_if_installed, and the existing mport_update_primative pattern
• Review mport.1 doc changes
• Post review feedback

Summary

The core logic is sound: lookup_current_os_installed() correctly gates on os_release (matching check_if_installed's semantics), mport_version_cmp(installed, new) < 0 correctly identifies "installed is older" (verified against version_cmp.c:52, which documents -1 when arg1 < arg2), and the already_installed/automatic memory handling is leak/double-free free on every path I traced (freed in the explicit early-return branches and unconditionally in cleanup:). The mport.1 wording is accurate.

Finding: delete-then-reinstall is not atomic, and now happens without --force

libmport/install_primative.c:228-244

When a newer local version is detected, the code unconditionally calls:

mport_delete_primative(mport, already_installed[0], 1)   // force=1

force=1 skips check_for_upwards_depends() (delete_primative.c:88-91), so the old package is removed from disk/DB before the new one is confirmed installable. Immediately after, several steps can still fail and goto cleanup without reinstalling anything:

• MPORT_PRECHECK_CONFLICTS at line 244-250
• age-verification / missing-dependency handling for deps at lines 274-300
• MPORT_PRECHECK_FILE_CONFLICTS/MPORT_PRECHECK_DEPENDS and mport_bundle_read_install_pkg at lines 381-391

Failure scenario: package A is installed and package B depends on it. Running a plain mport install A-newer.mport (offline, no --force) now deletes the installed A (bypassing the reverse-dependency check, since force=1 is hardcoded) and only afterward attempts the new install. If the new bundle fails any precondition (e.g. a file conflict, a missing dependency without --ignore-missing, or an asset-install error), the function returns an error with A now completely uninstalled and B broken — a strictly worse outcome than the pre-PR behavior of simply refusing with "already installed."

Bypassing the reverse-dependency check itself is arguably correct for an in-place update (the package name persists, so dependents shouldn't need to re-resolve) — but skipping it is only safe if the reinstall is guaranteed to complete. It isn't: unlike mport_update_primative (update_primative.c:36-104), which updates the existing row via mport_bundle_read_update_pkg and never deletes the package, this path does a full mport_delete_primative and hopes the subsequent install goes through cleanly. There's no rollback if it doesn't.

Options worth considering: run the CONFLICTS/DEPENDS/age preconditions against the new package before deleting the old one (reordering, since those checks don't require the old package to be gone first), or reuse the mport_update_primative-style in-place update instead of delete+reinstall for this offline case.

Minor

• libmport/install_primative.c:40-41 — the new /*@only@*///*@notnull@*/ splint annotations are only on the forward declaration, not the definition at line 149. Every other static helper in this file (e.g. find_file_with_prefix) is unannotated either way, so this is just an inconsistency within the new code itself, not a regression — low priority.

Fix this →

[sourcery-ai]

Hey - I've left some high level feedback:

• The new logic only ever looks at already_installed[0] from mport_pkgmeta_search_master, which assumes there is at most one installed package per name; if multiple entries are possible (e.g., legacy data), consider either constraining the query to a unique row or explicitly handling/reporting the multi-match case instead of silently ignoring it.
• By moving the --force-reinstall handling into lookup_current_os_installed and gating on matching os_release, the behavior for a locally installed package from a different OS release may have changed (e.g., --force no longer deleting/reinstalling that case); it would be good to double-check and, if needed, explicitly codify the intended cross-release --force behavior in this path.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The new logic only ever looks at `already_installed[0]` from `mport_pkgmeta_search_master`, which assumes there is at most one installed package per name; if multiple entries are possible (e.g., legacy data), consider either constraining the query to a unique row or explicitly handling/reporting the multi-match case instead of silently ignoring it.
- By moving the `--force`-reinstall handling into `lookup_current_os_installed` and gating on matching `os_release`, the behavior for a locally installed package from a different OS release may have changed (e.g., `--force` no longer deleting/reinstalling that case); it would be good to double-check and, if needed, explicitly codify the intended cross-release `--force` behavior in this path.

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[github-actions]

Claude finished @laffer1's task in 3m 36s —— View job

---

Claude finished re-reviewing this PR update

• Gather context (new commits since last review, diff vs origin/main)
• Check whether delete-before-confirm atomicity finding was addressed
• Check whether Sourcery's multi-row / cross-release --force comments were addressed
• Re-review lookup_current_os_installed() and surrounding logic
• Post final review feedback

Summary

Commit 02c537a addresses one of the two Sourcery comments but the primary finding from the earlier review (delete-before-confirm is not atomic) is still present in the code as pushed.

Sourcery comment 1 (multi-row assumption) — addressed

lookup_current_os_installed() (libmport/install_primative.c:159-195) now scans every row returned for pkg=%Q and selects the one whose os_release matches the running system, instead of blindly trusting already_installed[0]. This is real: mport_generate_master_schema() (libmport/db.c:502) adds a UNIQUE INDEX packages_pkg ON packages (pkg) for brand-new databases, but none of the mport_upgrade_master_schema_*to* steps (libmport/db.c:296-360) add that index retroactively — so a long-lived, upgraded master.db genuinely can carry more than one row per pkg name across different os_release values. Scanning is the right fix.

Sourcery comment 2 (cross-release --force behavior) — verified as unchanged, no regression

Confirmed by diffing against the pre-PR code (git show origin/main:libmport/install_primative.c): the old code gated the force-delete branch on mport_check_preconditions(..., MPORT_PRECHECK_INSTALLED), which calls check_if_installed() (libmport/check_preconditions.c:135-203). That function already treats a differing os_release as "not installed" (line 184: if (strcmp(os_release, system_os_release) != 0) break; → returns MPORT_OK), so the old force-delete path never fired for a cross-release match either. lookup_current_os_installed()'s gate on matching os_release preserves this exactly — no behavior change here. (As a side benefit, the new code deletes the actual DB-resident current_installed record rather than the old code's latent bug of calling mport_delete_primative on pkgs[0], the freshly-parsed bundle metadata.)

Not addressed: delete happens before the new package is confirmed installable

libmport/install_primative.c:245-271, still followed by preconditions/parse steps that can fail afterward:

already_installed = lookup_current_os_installed(mport, pkgs[0], &current_installed);
if (current_installed != NULL) {
    ...
    if (mport->force || version_cmp < 0) {
        ...
        if (mport_delete_primative(mport, current_installed, 1) != MPORT_OK) {  // force=1
            ret = mport_err_code();
            goto cleanup;
        }
    } else { ... }
    mport_pkgmeta_vec_free(already_installed);
}

if (mport_check_preconditions(mport, pkgs[0], MPORT_PRECHECK_CONFLICTS) != MPORT_OK) {  // line 273 — after delete
    ...
    goto cleanup;
}

and further down, precheck_flags (MPORT_PRECHECK_INSTALLED | DEPENDS | CONFLICTS [| FILE_CONFLICTS]) plus mport_bundle_read_install_pkg() at install_primative.c:410-420 — all still run after the delete.

mport_delete_primative(mport, current_installed, 1) hardcodes force=1, which unconditionally skips check_for_upwards_depends() (libmport/delete_primative.c:88-91).

Failure scenario (unchanged from the previous review): package A is installed and package B depends on it. Running mport install A-newer.mport offline, without --force, now deletes the installed A (bypassing the reverse-dependency check) before the new bundle's conflict/dependency/install steps run. If any of those subsequently fail (file conflict, missing dependency without --ignore-missing, an asset-install error in mport_bundle_read_install_pkg), the function returns an error with A now completely uninstalled and B broken — worse than the pre-PR "already installed" refusal.

This is the same gap flagged in the last review and neither of the two new commits touches it. It's still worth fixing before merge: either run the CONFLICTS/DEPENDS/age preconditions against the new package before deleting the old one, or restructure this path to do an in-place update (as mport_update_primative in update_primative.c does — it updates the row via mport_bundle_read_update_pkg and never deletes the package) instead of delete-then-reinstall.

Memory/ownership re-check (no new issues)

• mport_delete_primative does not free its pack argument (confirmed no free/mport_pkgmeta_free call on pack in delete_primative.c), so using current_installed for the log message before deleting it, then freeing the whole already_installed vector afterward, is safe — no use-after-free or double-free.
• All paths through the new block (force/newer-delete-success, already-installed-refusal, delete failure) reach cleanup: with already_installed either explicitly freed+NULL'd or still owned and freed once at install_primative.c:437-440. No leak or double-free found.

Minor (unchanged, low priority)

libmport/install_primative.c:40-41 — the /*@only@*///*@notnull@*/ splint annotations are still only on the forward declaration, not the definition at line 159. Cosmetic; not a regression since other static helpers in this file are unannotated either way.

Fix this →