Thanks for the patch. What did you wrap the comments to? We normally wrap the doxygen to about 80 columns just like the code. I have concerns about some of the specific documentation explained below. On 08/14/13 at 03:10pm, Richard Pougnet wrote:

Signed-off-by: Richard Pougnet --- lib/libalpm/add.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 49 insertions(+), 3 deletions(-)

diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c index 45c57b0..73112fc 100644 --- a/lib/libalpm/add.c +++ b/lib/libalpm/add.c @@ -47,7 +47,16 @@ #include "remove.h" #include "handle.h"

-/** Add a package to the transaction. */ +/** Adds a package to transaction add list + * Finds the target package and compares + * to the local version (if it exists). + * If version is newer then cache, or downgrade + * is desired, the package is then added to the list + * and log file is updated. + * @param handle the context handle + * @param pkg the package to be added + * @return 0 on success, 1 on error + */ int SYMEXPORT alpm_add_pkg(alpm_handle_t *handle, alpm_pkg_t *pkg) { const char *pkgname, *pkgver;

This already has full doxygen documentation in alpm.h. Unfortunately, we have several functions like this with real documentation in alpm.h and a stub at the function definition.

@@ -104,6 +113,18 @@ int SYMEXPORT alpm_add_pkg(alpm_handle_t *handle, alpm_pkg_t *pkg) return 0; }

+/** Extracts a package. + * Loads the package archive, reads it + * and then extracts. + * If non critical error occured, logs and + * continues. Else, extract fails. + * @param handle the context handle + * @param archive the package archive + * @param entry the entry to be extracted + * @param filename the filename of the archive + * @param origname the name of the package + * @return 0 on success, 1 on error + */ static int perform_extraction(alpm_handle_t *handle, struct archive *archive, struct archive_entry *entry, const char *filename, const char *origname) {

This neither loads nor extracts a *package*. It extracts a single file from an already loaded package. Filename and origname aren't the name of the archive or package; they're the path to the file being extracted.

@@ -129,6 +150,12 @@ static int perform_extraction(alpm_handle_t *handle, struct archive *archive, } return 0; }

There should be a blank line here.

+/** Attempts to rename an entry + * @param handle the context handler + * @param src the original name + * @param dest the final name + * @return 0 on success, 1 on error +*/

static int try_rename(alpm_handle_t *handle, const char *src, const char *dest) { @@ -141,7 +168,14 @@ static int try_rename(alpm_handle_t *handle, const char *src, const char *dest) } return 0; } - +/** Extracts a single file + * @param handle the context handler + * @param archive the package archive + * @param entry the archive entry + * @param newpkg the new package + * @param oldpkg the old package + * @return 0 on success, 1 on error +*/ static int extract_single_file(alpm_handle_t *handle, struct archive *archive, struct archive_entry *entry, alpm_pkg_t *newpkg, alpm_pkg_t *oldpkg) {

The overwhelming bulk of this function is about the proper handling of backup files. The description should acknowledge that fact.

@@ -450,7 +484,15 @@ needbackup_cleanup: free(entryname_orig); return errors; } -

Why did you remove the blank lines?

+/** Commits a package to the database + * Creates a database entry for a newly installed + * package. + * @param handle the context handler + * @param newpkg the package to be entered + * @param pkg_current the current package number + * @param pkg_count total number of packages to be committed + * @return 0 on success, -1 on error +*/ static int commit_single_pkg(alpm_handle_t *handle, alpm_pkg_t *newpkg, size_t pkg_current, size_t pkg_count) {

This doesn't just create the database entry, it's responsible for the actual installation of the package as well.

@@ -667,6 +709,10 @@ cleanup: return ret; }

+/** Upgrades a package + * @param handle the context handler + * @return 0 on success, -1 on error +*/ int _alpm_upgrade_packages(alpm_handle_t *handle) { size_t pkg_count, pkg_current;

This doesn't just "upgrade a package". This function is responsible for the entire actual installation process of all of the packages being installed.

-- 1.8.3.4