[Yum-devel] [PATCH] createrepo: buggy getFileList() - please review

[James Antill]

 No point CC'ing Paul at RH, he left RH 5-6 years ago :).

On Wed, 2012-11-28 at 07:03 -0500, Zdenek Pavlas wrote:
> Hi,
> 
> Tomas Mlcoch found that --skip-symlinks is broken,
> and I've merged his simple and obviously correct patch.
> 
> But the directory test is broken too, and the overriden
> getFileList() in SplitMetadataGenerator is buggy, too.
> So I've fixed these as well:
>
> commit 73400ac4a9717bcedb73902c7a510f3282e1a47b
> Author: Zdeněk Pavlas <zpavlas at redhat.com>
> Date:   Mon Nov 26 14:30:20 2012 +0100
> 
>     also fix os.path.isdir(), kill broken SplitMetadataGenerator.getFileList
> 
> diff --git a/createrepo/__init__.py b/createrepo/__init__.py
> index ef51b14..b3a3c06 100644
> --- a/createrepo/__init__.py
> +++ b/createrepo/__init__.py
> @@ -1263,24 +1262,6 @@ class SplitMetaDataGenerator(MetaDataGenerator):
>          (scheme, netloc, path, query, fragid) = urlparse.urlsplit(url)
>          return urlparse.urlunsplit((scheme, netloc, path, query, str(fragment)))
>  
> -    def getFileList(self, directory, ext):
> -
> -        extlen = len(ext)
> -
> -        def extension_visitor(arg, dirname, names):
> -            for fn in names:
> -                if os.path.isdir(fn):
> -                    continue
> -                elif fn[-extlen:].lower() == '%s' % (ext):
> -                    reldir = os.path.basename(dirname)
> -                    if reldir == os.path.basename(directory):
> -                        reldir = ""
> -                    arg.append(os.path.join(reldir, fn))
> -
> -        rpmlist = []
> -        os.path.walk(directory, extension_visitor, rpmlist)
> -        return rpmlist
> -
>      def doPkgMetadata(self):
>          """all the heavy lifting for the package metadata"""
>          if len(self.conf.directories) == 1:
> 
> While the first hunk is fine, I'm not 100% sure if the getFileList()
> in SplitMetadataGenerator wasn't modified on purpose.  It has been
> added in this commit:

 So ... split media has always been kind of a weird thing, in that
traditionally it was only ever used by our rel-eng and nobody cared
about it as long as it did whatever magic they wanted.
 
 Having a look at the point of the commit the difference seems to be:

original:

  83)                 elif string.lower(fn[-extlen:]) == '%s' % (ext):
  84)                      arg.append(os.path.join(directory,fn))


new:

 282)                 elif string.lower(fn[-extlen:]) == '%s' % (ext):
 283)                     reldir = os.path.basename(dirname)
 284)                     if reldir == os.path.basename(directory):
 285)                         reldir = ""
 286)                     arg.append(os.path.join(reldir,fn))

...which is the major difference now too, apart from the missing
skip_symlinks check.

> But, when .RPM file is found 0 or 1 subdirs below the repo root, it
> works exactly the same as the (current) base implementation. 

 So ... I've very rarely used --split, but this is roughly what it's
supposed to do. From man createrepo:

       --split
              Run  in  split  media mode. Rather than pass a single directory,
              take a set of directories corresponding to different volumes  in
              a media set.

...the idea (AIUI) is that you have, say:

CD1/foo.rpm
CD1/bar.rpm
CD2/baz.rpm
[...]

...then you run createrepo over the entire set with --split, and it
removes the dirs. so that the location is foo.rpm or bar.rpm (which will
be correct with reference to the CDn part being a "root").