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

From: Clark Williams <clark.williams@gmail.com>
Date: Sep 8, 2008, 18:11 UTC
Message-ID: <48C56AD9.6040007@gmail.com>
In-Reply-To: <20080908180317.GA6123@diana.vm.bytemark.co.uk>

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Karl Hasselström wrote:

Show 15 quoted lines

> 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. ;-)
>

Patch series tarballs are quite common from people who use quilt (e.g. many of the kernel -rt series developers). My biggest problem (now that I can directly import them) is to see if I can ease StGit's patch import rules a bit, since quilt accepts pretty much anything as long as there's a diff in there somewhere. I bomb out regularly importing the -rt series using StGit, because some people don't put complete email addresses in their patches.

As to the test, I'll get right on that...:)

Show 6 quoted lines

>> +           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.

Yeah I thought about that, as well as auto-detecting it in the file case. I'll look into that a bit more.

Show 7 quoted lines

> 
>> +        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?
>

Hence the "would you guys look at this?". Yeah, I need to detect sneaky stuff like that too.

> 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 ...)

I doubt there are many Windows-generated tarballs out there (except for the Cygwin case; I believe they use '/'), but I shouldn't be so Unix-centric. I'll work on cleaning it up.

I did consider adding Zipfile support as well, but didn't get a very good match-up between tar functionality and zip functionality. Maybe later...

Show 6 quoted lines

> 
>> +        raise CmdException, "no series file found in %s" % tar
> 
> Perhaps "no 'series' file ...", to make it clear what the name should
> be?
>

Yeah, that makes sense.

Show 30 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).

I did consider pulling directly from the tarball. I'll look into it.

Show 9 quoted lines

> 
>> +    # 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.
>

Man, I could not for the life of me remember which module had that in it. To be fair I wasn't up at work with my Python Essential Reference, which would have pointed me directly at it, but I would have thought I could have gotten there through the Python docs. Sigh...

You can dock my StGit pay for the visit to the eye doctor :)

Clark
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org

iEYEARECAAYFAkjFatgACgkQqA4JVb61b9fMRQCeLfK0zhPNEq3t5M4HW+vbRtaG VhgAn0rtszqVLbd1bz12MS0b/3r0OkT2 =gkf1 -----END PGP SIGNATURE-----