associate.py

48: It shouldn't be hard. That calls takes a criteria, at very least we can scope it to include
most things besides the changelog and repodata stuffs until we can better identify the exact
fields that are required.

52: There's no need to get the flattened config, you can simply call get() on the config instance itself.

57: I'm guessing that's to reclaim memory. Might be a good idea to add a note saying that for
future developers who don't remember the pain we went through when it came to memory in this area.

162: There's no need to have copy_deps as a kwarg, it's always specified.

262: I might just be missing it right now, but how will the RPMs in that group get added? I
only see one call to identify_children_to_copy in the file.

Ah, ok, I see it now. We recurse through the entire associate call. It's still a big misleading
though because it's not clear in identify_children_to_copy that it's going to be caught.
Looking at just the logic of identify_children_to_copy, it looks like the understanding of
the model is broken. I'm not sure the best way to alleviate it; it might suffice to just
have the docstring indicate it's only *direct* children and requires the results to be broken
down.

267: The return value is confusing to interpret. Why are we returning a list of RPM names
and a separate list of RPM unit keys? It looks like they come from different parents, but
using the variable names rpm_names and rpm_unit_keys is confusing to read. Maybe if you
name them scoped to why the lists are there, like group_rpms v. errata_rpms it'd be easier
to follow.

218: Same thing as above, copy_rpms and copy_rpms_by_name doesn't need to be a kwarg.

304: We should rename _safe_copy to indicate it's only safe for group/category copying.
Any unit that has a file on disk shouldn't be passed in here for a number of reasons
(the path to that file is deleted in the copy, if we have two inventoried items pointing
to the same file orphaned clean up is gonna break, etc.)


Overall: It's a tricky problem being addressed once you add in the dependency flag. There's
some duplication going on in the sense that RPMs in a group and/or an errata will be copied
twice (more specifically checked to copy twice) in many cases (the "all" case being the most common),
but that may be preferrable to having to know at a single time everything to associate.

The code itself is somewhat difficult to read, but again, that might be due to the complex
nature of the problem. It looks like associate() handles all of the dependencies, but then
there's the recursive call in the middle in case categories are present, in which case
all of the checking for errata, groups, etc. happens again. So it ends up being slightly
weird in the sense that you're recursively calling not because the nature of the problem
lends itself cleanly to breaking down but because the associate method wasn't broken down
as cleanly as it maybe could have (all of the unit type logic is performed even though
the only type of unit being passed in is a group). It's still lightyears
ahead of the old implementation so I'm not saying it's a blocker, just giving my impressions.