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?
> + # 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@xxxxxxxxxxx
www.treskal.com/kalle
--
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
