deploy: Only hold local variant pointer, not in struct #3036
Conversation
LGTM, though this kind of changes do not seem controversial to me, so maybe I'm missing something subtle. I'm reading this patch in the context of minimizing data lifetime/scope in order to reduce the amount of action-at-a-distance side-effects (and thus subtle bugs). |
|
Yup, makes sense. Did something similar in #3006 (well, it's still structs, but it moves unpacking and conflicting options checking to |
|
Ah OK wow yep, the changes in #3006 look good! Do you want to try to factor that out, or I could, or we could merge a few things like this and then you rebase later? |
If you have cycles right now, feel free to do either of the last two (you factoring it out or merging stuff in and I'll rebase). Otherwise, I can probably pick this up next week as part of #3006 since it looks like there's some agreement on a path forward there. |
This is part of reducing the number of places that need to be touched when adding new API. There's no good reason to parse the options variant into a struct member; we can just keep it as a local variable.
Rather than having the caller do conditionals for `NULL`, do it inside the function. This simplifies the control flow and avoids duplication.
cgwalters
force-pushed
the
deploy-thinout
branch
from
7bda730 to
5baa9e8
Compare
|
OK. Hmm. Since this is a lot of code to touch let's try doing it incrementally. I pushed two smaller cleanup patches here. |
| @@ -809,6 +809,8 @@ rpmostree_origin_remove_packages (RpmOstreeOrigin *origin, | |||
| gboolean *out_changed, | |||
| GError **error) | |||
| { | |||
| if (!packages) | |||
| return TRUE; | |||
There was a problem hiding this comment.
This bit is missing for rpmostree_origin_add_packages for consistency. Though meh.. kinda leaning towards not even having it since the code already handles it fine. But obviously fine as is.
There was a problem hiding this comment.
True, will look at this in a followup!
deploy: Only hold local variant pointer, not in struct
This is part of reducing the number of places that need to
be touched when adding new API. There's no good reason to parse
the options variant into a struct member; we can just keep
it as a local variable.
origin: Make some package/module mutation functions no-ops for NULL
Rather than having the caller do conditionals for
NULL, do itinside the function. This simplifies the control flow and avoids
duplication.