Re: [PATCH] Refactor: split off emptying a package's file info

[Guillem Jover <guillem@debian.org>]

Hi!

Some comments on the changes I made when applying it.

On Sat, 2009-08-29 at 19:57:24 -0400, David Benjamin wrote:
> Here's the first refactoring patch mentioned in "dpkg list-file performance".
> 
> ===
> Refactor: split off emptying a package's file info
> 
> Put it into a separate function for reuse by other routines and to
> simplify ensure_packagefiles_available.
> 
> Signed-off-by: David Benjamin <davidben@mit.edu>

Fixed the commit log. If you need to add out of band information in
the future, you can append it after the three dashes, which git
ignores.

> ---
>  src/filesdb.c |   38 +++++++++++++++++++++++++-------------
>  1 files changed, 25 insertions(+), 13 deletions(-)
> 
> diff --git a/src/filesdb.c b/src/filesdb.c
> index da8cde2..a437a40 100644
> --- a/src/filesdb.c
> +++ b/src/filesdb.c
> @@ -69,22 +69,15 @@ void note_must_reread_files_inpackage(struct pkginfo *pkg) {
>  static int saidread=0;
> - /* load the list of files in this package into memory, or update the
> -  * list if it is there but stale
> + /* erase the files saved in pkg
>    */
> -void ensure_packagefiles_available(struct pkginfo *pkg) {
> -  int fd;
> -  const char *filelistfile;
> -  struct fileinlist **lendp, *newent, *current;
> +static void _erase_pkg_file_data(struct pkginfo *pkg) {

No underscores at the beginning or end of identifiers. Formatted the
changed code and comment to the new coding style by placing the
function name to column 0 and the brace on the next line. Renamed the
function to pkg_files_blank.

> +  struct fileinlist *current;
>    struct filepackages *packageslump;
> -  int search, findlast, putat;
> -  struct stat stat_buf;
> -  char *loaded_list, *loaded_list_end, *thisline, *nextline, *ptr;
> +  int search, findlast;
> -  if (pkg->clientdata && pkg->clientdata->fileslistvalid) return;
> -  ensure_package_clientdata(pkg);
> -
> -  /* Throw away any the stale data, if there was any. */
> +  if (!pkg->clientdata)
> +    return; /* nothing to empty */

Added a blank line here.

>    for (current= pkg->clientdata->files;
>         current;
>         current= current->next) {
> @@ -121,6 +114,25 @@ void ensure_packagefiles_available(struct pkginfo *pkg) {
>       */
>    }
>    pkg->clientdata->files = NULL;
> +}
> +
> + /* load the list of files in this package into memory, or update the
> +  * list if it is there but stale
> +  */

Reformatted this comment as well.

> +void ensure_packagefiles_available(struct pkginfo *pkg) {

Same format change as the other function.

> +  int fd;
> +  const char *filelistfile;
> +  struct fileinlist **lendp, *newent;
> +  struct filepackages *packageslump;
> +  int putat;
> +  struct stat stat_buf;
> +  char *loaded_list, *loaded_list_end, *thisline, *nextline, *ptr;
> +
> +  if (pkg->clientdata && pkg->clientdata->fileslistvalid) return;

Placed return on the next line.

> +  ensure_package_clientdata(pkg);
> +
> +  /* Throw away any the stale data, if there was any. */
> +  _erase_pkg_file_data(pkg);

Added a blank line here.

>    /* Packages which aren't installed don't have a files list. */
>    if (pkg->status == stat_notinstalled) {

Pushed, thanks!

regards,
guillem