-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Karl Hasselström wrote:
> 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...:)
>> + 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.
>
>> + 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...
>
>> + 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.
>> + # 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.
>
>> + # 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-----
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
