[pacman-dev] [PATCH] pacsort: refactor version comparison for correctness

[Andrew Gregory]

On 11/05/13 at 11:03pm, Dave Reisner wrote:
> This refactors the existing vercmp machinery in order to:
> 
> 1) Correct a deficiency in files comparison. This was too lazily written
> and could produce incorrect results when a version contained an epoch.
> 2) Improve tarball detection. Calling fnmatch a second time for the
> uncompressed tarball case is not a noticeable performance hit, and is
> more correct.
> 3) Fix a bug in tarball detection. The glob pattern would match files
> with only a pkgver, e.g. foopkg-1.2.3-x86_64.pkg.tar.xz.
> 4) Be generally cleaner code, easier to follow.
> 
> The test suite gets extended to validate my claims for the new behavior.
> 
> Fixes FS#37631.
> 
> Signed-off-by: Dave Reisner <dreisner at archlinux.org>
> ---
> This supersedes an earlier patch I sent against pacsort which was a bit
> more directed and less useful.
> 
>  src/util/pacsort.c       | 164 ++++++++++++++++++++++++++++++++++-------------
>  test/util/pacsorttest.sh |  31 ++++++++-
>  2 files changed, 150 insertions(+), 45 deletions(-)
> 
> diff --git a/src/util/pacsort.c b/src/util/pacsort.c
> index 687e558..554352c 100644
> --- a/src/util/pacsort.c
> +++ b/src/util/pacsort.c

<snip>

> +	const char *pkgver_end;
> +	const char *slash;
> +
> +	/* Find the basename */
> +	slash = strrchr(path, '/');
> +	meta->pkgname = slash ? slash + 1 : path;
> +	pkgver_end = strrchr(meta->pkgname, '-');
> +
> +	/* Read backwards through pkgrel */
> +	for(meta->pkgver = pkgver_end - 1;
> +			meta->pkgver > meta->pkgname && *meta->pkgver != '-';
> +			--meta->pkgver)
> +		;
> +	/* Read backwards through pkgver */
> +	for(--meta->pkgver;
> +			meta->pkgver > meta->pkgname && *meta->pkgver != '-';
> +			--meta->pkgver)
> +		;

Nitpick.  Personally, I would have an easier time reading this if we
used a temporary variable for meta->pkgver so that the for loop could
fit all on one line the way it is in _alpm_splitname.

> +	++meta->pkgver;
> +
> +	meta->pkgname_len = meta->pkgver - meta->pkgname - 1;
> +	meta->pkgver_len = pkgver_end - meta->pkgver;
> +}

<snip>

> +static int vercmp_files(const char *file1, const char *file2)
> +{
> +	struct pkgmeta_t pkg1, pkg2;
> +
> +	/* In order for 2 files to be considered equal, they must have the same
> +	 * basename. For example, the two follow strings are considered equal:
> +	 *   /var/cache/pacman/pkg/pacman-4.1.1-1-x86_64.pkg.tar.gz
> +	 *   /share/packages/pacman-4.1.1-1-x86_64.pkg.tar.gz
> +	 */
> +
> +	extract_pkgmeta(file1, &pkg1);
> +	extract_pkgmeta(file2, &pkg2);

We could do this extraction before the sort instead of for every
comparison so that we sort a *pkg_meta_t[] instead of *char[].  Then
we could also verify that our input consists entirely of paths,
because mixed paths and version numbers with --files will give
nonsensical results that change based on the order of the input.

> +
> +	return pkgmeta_cmp(&pkg1, &pkg2);
> +}
> +
> +static int vercmp(const void *v1, const void *v2)
> +{
> +	const char *s1 = *(const char **)v1, *s2 = *(const char **)v2;
> +	int cmp;
> +
> +	if(opts.filemode && is_pkgtar(s1) && is_pkgtar(s1)) {

You check s1 twice.

> +		cmp = vercmp_files(s1, s2);
> +	} else {
> +		cmp = vercmp_versions(s1, s2);
>  	}
>  
> -	return r;
> +	return opts.order * cmp;
>  }
>  
>  static char escape_char(const char *string)