[pacman-dev] [PATCH] Add Doxygen documentation for add.c of libalpm

[Andrew Gregory]

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 <richard at pougnet.ca>
> ---
>  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