upgrader: Generate "computed" origin

[cgwalters]

This is is part of reducing internal complexity - in particular
our "representations of state". The code here is basically
printing a warning if a requested package is already in the base
tree, and avoid passing that package down to the core.
To accomplish this original code here was doing:

origin -> (computed state) -> treespec

Now that we dropped treespec in a recent PR, it's going:

origin -> (computed state) -> treefile

Instead of having anonymous (computed state) we can
just make that a new (filtered) origin file. IOW
we now do:

origin -> origin -> treefile

This is easier to understand and debug. Note
how duplication around checking "requires local
assembly" between the upgrader and origin code
just drops out!

[cgwalters]

Pushed:

rust/origin: Don't mutate input origin, do translate override

Ahhhh that was so confusing and tricky to debug. But, hooray again for the test suite!

[lucab]

we can just make that a new (filtered) origin file. [...] This is easier to understand and debug.

Direction and code looks fine to me, however I disagree with this claim. This now has two members (origin and computed_origin) in the same scope with the same type and very subtle/minimal differences. They can be freely mixed up by human mistake, and compiler can't help catching that. Overall IMHO this increases cognitive load and adds more foggy corners to debugging.

In this context, I would naively have expected computed_origin to be the only thing laying around, with the pristine origin possibly reconstructed on the fly when eventually calling into libostree (or, if that's not possible, to have the keyfile further encapsulated within RpmOstreeOrigin).
Have you already considered and discarded that approach for some reason?

[lucab]

I'd be happier if we can keep only a single RpmOstreeOrigin * in this scope (i.e. the computed one), see longer comment in the PR.

[cgwalters]

This now has two members (origin and computed_origin) in the same scope with the same type and very subtle/minimal differences. They can be freely mixed up by human mistake, and compiler can't help catching that. Overall IMHO this increases cognitive load and adds more foggy corners to debugging.

We replaced 3 previous variables with one though. Hmm. I can rename the other one to original_origin or something too?

In this context, I would naively have expected computed_origin to be the only thing laying around,

I was alluding to the reason we can't do that in the commit message here:

The code here is basically printing a warning if a requested package is already in the base tree, and avoid passing that package down to the core.

The core problem is around the case where someone does e.g. rpm-ostree install --allow-inactive docker on FCOS. That command is saying "I want docker even though it's currently in the base. If at some point in the future it drops out of the base, start layering it".

What this logic in the upgrader is doing is emitting the warning around having the package in the base, and also drops out the request to install it (because at time t0 it is in the base). But that request needs to stay in the origin file so that we remember to start layering at time t1 later if needed.

[lucab]

Yes, I fully agree with the direction of this (both for the reduction of variables, and for the explicit computed state). The concern is mainly around having two subtly-diverging entities available in the same scope.
If there is no way around that then we can merge this and reap the benefits already. But from a "humans are fallible" point of view, the one above is IMHO a sharp edge and I wanted to check whether different arrangements are feasible.

[cgwalters]

I don't see a way to get down to just one thing while preserving the current semantics. But very clearly some comments here were warranted 😄 I added a big one at the top, and also renamed the first variable to original_origin.

[jlebon]

LGTM! Approach makes a lot of sense to me. The ability to use e.g. rpmostree_origin_has_packages on the computed origin is nice.