[Rpm-maint] [PATCH 2/5] Determine when to perform Collection actions

[Panu Matilainen]

On Thu, 17 Jun 2010, Steve Lawrence wrote:
> +    p->collections = NULL;
> +    if (headerIsEntry(h, RPMTAG_COLLECTIONS)) {
> +	struct rpmtd_s colls;
> +	const char * collname;
> +	headerGet(h, RPMTAG_COLLECTIONS, &colls, HEADERGET_MINMEM);
> +	while ((collname = rpmtdNextString(&colls))) {
> +		argvAdd(&p->collections, collname);
> +	}
> +	argvSort(p->collections, NULL);
> +	rpmtdFreeData(&colls);
> +    }

This isn't an error by any means but you can just as well do

if (headerGet(h, RPMTAG_COLLECTIONS, &colls, HEADERGET_MINMEM)) {
     ... do stuff ...
}

without first testing for tag existence.

Also (just FYI) you can tell headerGet() to return data argv-style, so you 
dont need to perform a manual copy, basically

if (headerGet(h, RPMTAG_COLLECTIONS, &colls,
                                 (HEADERGET_ALLOC|HEADERGET_ARGV)) {
     p->collections = colls.data;
     argvSort(p->collections, NULL);
}

...but then you need to be careful not t use rpmtdFreeData() or argvFree() 
on the returned piece, but instead just free() it as headerGet() returns 
it all in a single allocation block. Also you can't modify p->collections 
afterwards if done that way which limits other possibilities so its 
probably best left as is.

ARGV_t isn't the most efficient way to store and handle data, these might 
call for hash tables but that's an internal implementation detail that can 
be changed later on if deemed necessary.

> --- a/lib/rpmte.h
> +++ b/lib/rpmte.h
> @@ -7,6 +7,7 @@
>  */
>
> #include <rpm/rpmtypes.h>
> +#include <rpm/argv.h>
>
> #ifdef __cplusplus
> extern "C" {
> @@ -107,6 +108,69 @@ rpm_color_t rpmteColor(rpmte te);
> rpm_color_t rpmteSetColor(rpmte te, rpm_color_t color);
>
> /** \ingroup rpmte
> + * Retrieve list of collections
> + * @param te		transaction element
> + * @return		list of collections
> + */
> +ARGV_const_t rpmteCollections(rpmte te);
> +
> +/** \ingroup rpmte
> + * Determine a transaction element is part of a collection
> + * @param te		transaction element
> + * @param collname	collection name
> + * @return		1 if collname is part of a collection, 0 if not
> + */
> +int rpmteHasCollection(rpmte te, const char * collname);
> +
> +/** \ingroup rpmte
> + * Retrieve list of collections that should be executed via post_add
> + * @param te		transaction element
> + * @return		list of collections
> + */
> +ARGV_const_t rpmteLastInCollectionAdd(rpmte te);
> +
> +/** \ingroup rpmte
> + * Retrieve list of collections that should be executed via post_any
> + * @param te		transaction element
> + * @return		list of collections
> + */
> +ARGV_const_t rpmteLastInCollectionAny(rpmte te);
> +
> +/** \ingroup rpmte
> + * Retrieve list of collections that should be executed via pre_remove
> + * @param te		transaction element
> + * @return		list of collections
> + */
> +ARGV_const_t rpmteFirstInCollectionRemove(rpmte te);
> +
> +/** \ingroup rpmte
> + * Add a collection to the list of last collections for the installation
> + * section of a transaction element
> + * @param te		transaction element
> + * @param collname		collection name
> + * @return		0 on success, non-zero on error
> + */
> +int rpmteAddToLastInCollectionAdd(rpmte te, const char * collname);
> +
> +/** \ingroup rpmte
> + * Add a collection to the list of last collections for the installation
> + * or removal section of a transaction element
> + * @param te		transaction element
> + * @param collname		collection name
> + * @return		0 on success, non-zero on error
> + */
> +int rpmteAddToLastInCollectionAny(rpmte te, const char * collname);
> +
> +/** \ingroup rpmte
> + * Add a collection to the list of first collections for the removal
> + * section of a transaction element
> + * @param te		transaction element
> + * @param collname		collection name
> + * @return		0 on success, non-zero on error
> + */
> +int rpmteAddToFirstInCollectionRemove(rpmte te, const char * collname);
> +
> +/** \ingroup rpmte
>  * Retrieve last instance installed to the database.
>  * @param te		transaction element
>  * @return		last install instance.

I think all of these should go to rpmte_internal.h instead of the public 
rpmte.h header: unless there's a real need to export something, 
keep it internal to permit changing API if/when needed without 
compatibility concerns. With the possible exception of 
rpmteHasCollection() and rpmteCollections() these aren't meaningfully 
usable outside rpmtsRun() internals anyway AFAICS.

 	- Panu -