Re: [StGit PATCH] add option to import series directly from a tar archive

From: Karl Hasselström <kha@treskal.com>
Date: Sep 8, 2008, 18:03 UTC
Message-ID: <20080908180317.GA6123@diana.vm.bytemark.co.uk>
In-Reply-To: <48C34EC7.9040102@gmail.com>

On 2008-09-06 22:47:19 -0500, Clark Williams wrote:

> Attached is my first cut at adding the ability to import a patch
> series by specifying the tarball.

Thanks!

> No Karl, I haven't developed a test for it (yet). I wanted to see
> what you guys thought first :)

I don't see a problem with it, and if you took the time to code it there is obviously at least one user (I have no idea how common patch series tarballs are). I do have some comments below, but nothing that would prevent you from writing a test or two right away. ;-)

> +           make_option('--tarfile',
> +                       help = 'import a series from a tar archive',
> +                       action = "store_true"),

As I hint below, you might want to autodetect tarballs with --series instead, since a tarball is just a tarred series directory.

> +        if n.startswith("../"):
> +            raise CmdException, "Relative path found in %s" % tar

I guess any occurrence of /../ in the middle of n should be caught as well? Or can't that happen?

By the way, is the separator always '/' in tarfile? Or should you use os.sep? (There is also os.pardir which you could use instead of '..', but that might be overdoing it a little ...)

> +        raise CmdException, "no series file found in %s" % tar

Perhaps "no 'series' file ...", to make it clear what the name should be?

Show 6 quoted lines

> +    # unpack into a tmp dir
> +    tmpdir = tempfile.mkdtemp('.stg')
> +    t.extractall(tmpdir)
> +
> +    # apply the series
> +    __import_series(os.path.join(tmpdir, seriesfile), options)

Hmm. It seems like such a waste to go via the file system here, when tarfile has such nice file extraction methods.

What you could do is something like this:

  1. Make two small classes with the same interface, one backed by a
     tarfile and one backed by a directory, that have two methods:
     get_series() and get_file(filename). Both methods return
     file-like objects (created by either open() or
     tarfile.extractfile()).

  2. Change __import_series() to use objects of this class rather than
     a directory directly -- starting with creating an instance of one
     or the other depending on tarfile.is_tarfile(). This will involve
     teaching __import_file to accept a file-like object instead of
     just a file name, but that's a one-liner.

  3. Drop the --tarfile flag, since you've just taught the --series
     flag to handle tarballs!

That said, if you don't feel like doing it the hard way, I won't insist. The way you coded it is in no way bad (in particular, you chose the right function to create a temp dir).

> +    # cleanup the tmpdir
> +    os.system('rm -rf %s' % tmpdir)

Aaah! My eyes! My _eyes_!!!!!

Seriously, though, you'd want to use something like shutil.rmtree here.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle