[PATCH - stgit] Patch to allow import of compressed files

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

This patch allows StGit to directly import compressed (.gz and .bz2) files with
reasonable patch names.

I do a lot of work on modified kernel trees and usually the first two things imported
are a stable update patch followed immediately by an -rt patch, both of which are
compressed. With this patch I can just copy the files down directly from kernel.org
and import them, rather than having to keep uncompressed copies around.

Hey, I'm lazy... :)

Clark

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

iEYEARECAAYFAkhNeL4ACgkQqA4JVb61b9cu0ACdH/Z71xh4gaD5euF3BgYnIhiO
AkUAnipyN/dsTBQDyhc6uzFhxdxPeYvJ
=2G71
-----END PGP SIGNATURE-----

On 2008-06-09 13:38:55 -0500, Clark Williams wrote:

This patch allows StGit to directly import compressed (.gz and .bz2)
files with reasonable patch names.

I do a lot of work on modified kernel trees and usually the first
two things imported are a stable update patch followed immediately
by an -rt patch, both of which are compressed. With this patch I can
just copy the files down directly from kernel.org and import them,
rather than having to keep uncompressed copies around.

Hey, I'm lazy... :)

Lazy is good. Thanks for the patch!

+        if filename.endswith(".gz"):
+            import gzip
+            f = gzip.open(filename)
+            pname = filename.replace(".gz", "")
+        elif filename.endswith(".bz2"):
+            import bz2
+            f = bz2.BZ2File(filename, 'r')
+            pname = filename.replace(".bz2", "")

Some comments here:

  * By my reading of the docs, the second argument to BZ2File defaults
    to 'r' anyway, so you could omit it.

  * We try to use single quotes wherever possible (except when triple
    quoting). You're using a mix ...

  * .replace() will happily replace anywhere in the string. Please
    consider using stgit.util.strip_suffix() instead.

And last but not least, it'd be terrific if you'd let me bully you
into adding .gz and .bz2 test cases for t1800-import. :-)

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

On Tue, 10 Jun 2008, Karl Hasselström wrote:

On 2008-06-09 13:38:55 -0500, Clark Williams wrote:

quoted

This patch allows StGit to directly import compressed (.gz and .bz2)
files with reasonable patch names.

I do a lot of work on modified kernel trees and usually the first
two things imported are a stable update patch followed immediately
by an -rt patch, both of which are compressed. With this patch I can
just copy the files down directly from kernel.org and import them,
rather than having to keep uncompressed copies around.

Hey, I'm lazy... :)

Lazy is good. Thanks for the patch!

quoted

+        if filename.endswith(".gz"):
+            import gzip
+            f = gzip.open(filename)
+            pname = filename.replace(".gz", "")
+        elif filename.endswith(".bz2"):
+            import bz2
+            f = bz2.BZ2File(filename, 'r')
+            pname = filename.replace(".bz2", "")

Some comments here:

 * By my reading of the docs, the second argument to BZ2File defaults
   to 'r' anyway, so you could omit it.

Peanut gallery question: Why not just always try these methods and catch 
some format exception if they fail, proceeding to the next possible 
decompressor (proceeding on to no decompressor)?

That way if a file is called .GZ, it will still be handled properly; in 
fact, all files would still be handled properly.  And these formats leave 
notes in the first few bytes of the file as to if they should be tried, so 
it's not as if it would come at some performance cost.

-- Asheesh.

-- 
Say "twenty-three-skiddoo" to logout.

On Tue, Jun 10, 2008 at 8:38 AM, Asheesh Laroia [off-list ref] wrote:

Peanut gallery question: Why not just always try these methods and catch
some format exception if they fail, proceeding to the next possible
decompressor (proceeding on to no decompressor)?

That way if a file is called .GZ, it will still be handled properly; in
fact, all files would still be handled properly.  And these formats leave
notes in the first few bytes of the file as to if they should be tried, so
it's not as if it would come at some performance cost.

How about adding in '.tar' decompression as well, -after- the '.gz'
decompression? That way you can have .tar.gz's and still be fine.

-- 
Cheers,

Sverre Rabbelier

On 2008-06-10 10:07:25 +0200, Sverre Rabbelier wrote:

How about adding in '.tar' decompression as well, -after- the '.gz'
decompression? That way you can have .tar.gz's and still be fine.

A tar file would presumably contain more than one patch -- it'd be
more like a directory of patches than a single patch file.

I'm not saying it wouldn't be nice to support it, but it's not what
the original poster needed, and building it would be a bunch of extra
work.

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

On Tue, Jun 10, 2008 at 11:53 AM, Karl Hasselström [off-list ref] wrote:

On 2008-06-10 10:07:25 +0200, Sverre Rabbelier wrote:

quoted

How about adding in '.tar' decompression as well, -after- the '.gz'
decompression? That way you can have .tar.gz's and still be fine.

A tar file would presumably contain more than one patch -- it'd be
more like a directory of patches than a single patch file.

Why?
$ tar czvf mypatch.patch.tar.gz mypatch.patch

I'm not saying it wouldn't be nice to support it, but it's not what
the original poster needed, and building it would be a bunch of extra
work.

A bunch of extra work? http://docs.python.org/lib/module-tarfile.html
I say it'd be about as much work as the original patch ;).

-- 
Cheers,

Sverre Rabbelier

On 2008-06-10 11:57:05 +0200, Sverre Rabbelier wrote:

On Tue, Jun 10, 2008 at 11:53 AM, Karl Hasselström [off-list ref] wrote:

quoted

A tar file would presumably contain more than one patch -- it'd be
more like a directory of patches than a single patch file.

Why?
$ tar czvf mypatch.patch.tar.gz mypatch.patch

If there's just one patch in the tar file, why did you use a tar file
in the first place instead of just gzipping?

I'm pretty sure that anyone who really has use for the tar-file
capability would be using tar files with multiple patches in them.

quoted

I'm not saying it wouldn't be nice to support it, but it's not
what the original poster needed, and building it would be a bunch
of extra work.

A bunch of extra work?
http://docs.python.org/lib/module-tarfile.html I say it'd be about
as much work as the original patch ;).

I was refering to the fact that due to tar-files in the general case
containing more than one patch, you'd have to modify the parts of
imprt.py that deal with importing multiple patches at once, in
addition to the parts the current patch touches.

But you're probably right that the amount of additional work would not
be much more than what went into the current patch.

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

On Tue, Jun 10, 2008 at 12:28 PM, Karl Hasselström [off-list ref] wrote:

If there's just one patch in the tar file, why did you use a tar file
in the first place instead of just gzipping?

I guess mostly habbit :P. Whenever I zip something I create a gzipped
tarball because that's how I usually do it.

I'm pretty sure that anyone who really has use for the tar-file
capability would be using tar files with multiple patches in them.

Yeah, I guess that's true for most people indeed.

I was refering to the fact that due to tar-files in the general case
containing more than one patch, you'd have to modify the parts of
imprt.py that deal with importing multiple patches at once, in
addition to the parts the current patch touches.

Mhhh, yeah, but should be something like
for patch in patches
  applyPatch(patch)

But you're probably right that the amount of additional work would not
be much more than what went into the current patch.

To just support .tar, yeah, but let's see what the author has to say
about this ;).

-- 
Cheers,

Sverre Rabbelier

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

Karl Hasselström wrote:

quoted

+        if filename.endswith(".gz"):
+            import gzip
+            f = gzip.open(filename)
+            pname = filename.replace(".gz", "")
+        elif filename.endswith(".bz2"):
+            import bz2
+            f = bz2.BZ2File(filename, 'r')
+            pname = filename.replace(".bz2", "")

Some comments here:

  * By my reading of the docs, the second argument to BZ2File defaults
    to 'r' anyway, so you could omit it.

Done.

  * We try to use single quotes wherever possible (except when triple
    quoting). You're using a mix ...

I normally use single quotes too, but I've been doing a bunch of C programming
lately, so that's my excuse and I'm sticking with it. Replaced.

  * .replace() will happily replace anywhere in the string. Please
    consider using stgit.util.strip_suffix() instead.

Ah, didn't know about strip_suffix(). Done.

And last but not least, it'd be terrific if you'd let me bully you
into adding .gz and .bz2 test cases for t1800-import. :-)

I'll work on that. Can't do it right now, but I'll look at the test harness and see
what it'll take.

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

iEYEARECAAYFAkhOh34ACgkQqA4JVb61b9ea9gCgoV1MZbT2F62WEkduOfmkgdP3
BwIAnApT1o+VttF4VRHJj4DkPmi/HXfm
=Uwho
-----END PGP SIGNATURE-----

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

Karl Hasselström wrote:

Some comments here:

  * By my reading of the docs, the second argument to BZ2File defaults
    to 'r' anyway, so you could omit it.

  * We try to use single quotes wherever possible (except when triple
    quoting). You're using a mix ...

  * .replace() will happily replace anywhere in the string. Please
    consider using stgit.util.strip_suffix() instead.

And last but not least, it'd be terrific if you'd let me bully you
into adding .gz and .bz2 test cases for t1800-import. :-)

Sigh. Never reply before your second cup of coffee.

Updated patch attached.

Clark

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

iEYEARECAAYFAkhOh7EACgkQqA4JVb61b9c4BgCeJH0GUzQJCDdJ8gx5287KE/KO
uTwAoKD7V4JMqHnYmFCg01ij5aBbLDbq
=Yy/n
-----END PGP SIGNATURE-----

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

Asheesh Laroia wrote:

Peanut gallery question: Why not just always try these methods and catch
some format exception if they fail, proceeding to the next possible
decompressor (proceeding on to no decompressor)?

That way if a file is called .GZ, it will still be handled properly; in
fact, all files would still be handled properly.  And these formats
leave notes in the first few bytes of the file as to if they should be
tried, so it's not as if it would come at some performance cost.

Interesting thought. Do all the decompressors throw an error if the input format
isn't recognized?

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

iEYEARECAAYFAkhOiDMACgkQqA4JVb61b9fixACfZLtxY20tXyZA5oLHTBSm5JPj
ApQAnRsb6RkA2YKB5UXhXiEezMm1j1ZS
=Y7ZK
-----END PGP SIGNATURE-----

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

Sverre Rabbelier wrote:

On Tue, Jun 10, 2008 at 8:38 AM, Asheesh Laroia [off-list ref] wrote:

quoted

Peanut gallery question: Why not just always try these methods and catch
some format exception if they fail, proceeding to the next possible
decompressor (proceeding on to no decompressor)?

That way if a file is called .GZ, it will still be handled properly; in
fact, all files would still be handled properly.  And these formats leave
notes in the first few bytes of the file as to if they should be tried, so
it's not as if it would come at some performance cost.

How about adding in '.tar' decompression as well, -after- the '.gz'
decompression? That way you can have .tar.gz's and still be fine.

I thought about this, as well as the .zip format. The problem is in multiple files
and how to handle them. Do you look for a series and automagically add '--series'
semantics? What about ignore/replace?  If no series file, do you just try to apply
all of them? In what order? Look for '.patch' suffix and only use those? And what
about the old '.tgz' suffix? Lots of different ways to go here.

I don't think it would be a bad thing to add .tar and .zip handling, but I'd rather
bat it around a little on this list before doing it. That way we can find out how
people would like to use it.

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

iEYEARECAAYFAkhOiUcACgkQqA4JVb61b9cWawCeK+n8uo8XGGlURYG2ImuhUUNY
eZEAn31ZfjC7MgiP1VZ02uksrRJQiQVe
=Tu6s
-----END PGP SIGNATURE-----

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

Sverre Rabbelier wrote:

On Tue, Jun 10, 2008 at 12:28 PM, Karl Hasselström [off-list ref] wrote:

quoted

If there's just one patch in the tar file, why did you use a tar file
in the first place instead of just gzipping?

I guess mostly habbit :P. Whenever I zip something I create a gzipped
tarball because that's how I usually do it.

quoted

I'm pretty sure that anyone who really has use for the tar-file
capability would be using tar files with multiple patches in them.

Yeah, I guess that's true for most people indeed.

quoted

I was refering to the fact that due to tar-files in the general case
containing more than one patch, you'd have to modify the parts of
imprt.py that deal with importing multiple patches at once, in
addition to the parts the current patch touches.

Mhhh, yeah, but should be something like
for patch in patches
  applyPatch(patch)

But, since patches in general are ordered beasts, you can get into trouble if you
didn't name your patches such that they lexically sort in the proper order *and* the
tar library gives them to you in the right order.

I think if I were going to add this, I'd probably look for a series file and use that
 to apply in the proper order. Same for if we support .zip files. Seems like the only
other thing you could do is read in the list of files and sort them, then apply them.

quoted

But you're probably right that the amount of additional work would not
be much more than what went into the current patch.

To just support .tar, yeah, but let's see what the author has to say
about this ;).

Author. Mmmmmmm, much nicer than "Mad Python patch hacker". I like it!

Clark

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

iEUEARECAAYFAkhOim8ACgkQqA4JVb61b9eMBACWIoP2sEcIdH8+2R60NN26UB/b
PQCgozvd0Kkonjz7xJlMN5cJqpVzhkY=
=UOWy
-----END PGP SIGNATURE-----

On Tue, 10 Jun 2008, Clark Williams wrote:

Asheesh Laroia wrote:

quoted

Peanut gallery question: Why not just always try these methods and catch
some format exception if they fail, proceeding to the next possible
decompressor (proceeding on to no decompressor)?

That way if a file is called .GZ, it will still be handled properly; in
fact, all files would still be handled properly.  And these formats
leave notes in the first few bytes of the file as to if they should be
tried, so it's not as if it would come at some performance cost.

Interesting thought. Do all the decompressors throw an error if the input format
isn't recognized?

Yup!  (Try it!)

-- Asheesh.

-- 
You might have mail.

On 2008-06-10 08:54:58 -0500, Clark Williams wrote:

                                  ^
Here's why I keep blathering about tests! In Python, you don't have a
compiler to catch these for you ...

I just realized a problem with this that was already present in your
first version: if patch != None, so that you set pname = patch, you
overwrite pname since you strip the .gz/.bz2 suffixes _later_.

Other than that, it looks good. But you sounded tempted to go with the
idea of just trying the decompressors rather than go by the suffix? I
think that'd be an improvement.

As for testing, you'd simply make two copies of one of the subtests in
t1800, where you test .gz- and .bz2-compressed versions of the same
patch. Should take about five minutes to write.

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

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

Karl Hasselström wrote:

On 2008-06-10 08:54:58 -0500, Clark Williams wrote:

quoted

--- a/stgit/commands/imprt.py
+++ b/stgit/commands/imprt.py

@@ -178,8 +178,22 @@ def __create_patch(filename, message, author_name, author_email,
 def __import_file(filename, options, patch = None):
     """Import a patch from a file or standard input
     """
+    if patch:
+        pname = patch
+    else:
+        pname = filename
+
     if filename:
-        f = file(filename)
+        if filename.endswith('.gz'):
+            import gzip
+            f = gzip.open(filename)
+            pname = strip_suffix('.gz', filename)
+        elif filename.endswith('.bz2'):
+            import bz2
+            f = bz2.BZ2File(filename)
+            pname = strip_suffic('.bz2', filename)

                                  ^
Here's why I keep blathering about tests! In Python, you don't have a
compiler to catch these for you ...

Yup, caught that right after I sent the updated patch :)

quoted

+        else:
+            f = file(filename)
     else:
         f = sys.stdin
 

@@ -197,11 +211,6 @@ def __import_file(filename, options, patch = None):
     if filename:
         f.close()
 
-    if patch:
-        pname = patch
-    else:
-        pname = filename
-

I just realized a problem with this that was already present in your
first version: if patch != None, so that you set pname = patch, you
overwrite pname since you strip the .gz/.bz2 suffixes _later_.

Ah, I didn't realize that patchname overrides all (should have). I'll fix that next
go-round.

Other than that, it looks good. But you sounded tempted to go with the
idea of just trying the decompressors rather than go by the suffix? I
think that'd be an improvement.

Yeah, it's tempting. I'll play with it a bit and see what it would take. Seems it
would just take a try/except block with logic to make the patchname right (I still
would want to remove a .gz/.bz2 suffix from the patchname).

As for testing, you'd simply make two copies of one of the subtests in
t1800, where you test .gz- and .bz2-compressed versions of the same
patch. Should take about five minutes to write.

I'll make sure I have tests to go with the next patch.

Clark


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

iEYEARECAAYFAkhQCygACgkQqA4JVb61b9fckgCfbjep1oC3WT3hxVSo/8y6/FVM
O8AAn1GEGjvbwerEY0U0N1UlJC8Lv37R
=RHjB
-----END PGP SIGNATURE-----

On 2008-06-11 12:28:08 -0500, Clark Williams wrote:

Seems it would just take a try/except block

Yes. The only trick here is to catch the specific type of exception
you want, and nothing else. You can easily try it out at an
interactive Python prompt and just see what type of exception is
thrown.

with logic to make the patchname right (I still would want to remove
a .gz/.bz2 suffix from the patchname).

Indeed. (And now that's a bit more work, since you'll want to ignore
capitalization -- and you can't be sure the suffix is actually there.)

I'll make sure I have tests to go with the next patch.

Excellent!

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

Karl Hasselström [off-list ref] writes:

On 2008-06-09 13:38:55 -0500, Clark Williams wrote:

quoted

This patch allows StGit to directly import compressed (.gz and .bz2)
files with reasonable patch names.

I do a lot of work on modified kernel trees and usually the first
two things imported are a stable update patch followed immediately
by an -rt patch, both of which are compressed. With this patch I can
just copy the files down directly from kernel.org and import them,
rather than having to keep uncompressed copies around.

Hey, I'm lazy... :)

Lazy is good. Thanks for the patch!

quoted

+        if filename.endswith(".gz"):
+            import gzip
+            f = gzip.open(filename)
+            pname = filename.replace(".gz", "")
+        elif filename.endswith(".bz2"):
+            import bz2
+            f = bz2.BZ2File(filename, 'r')
+            pname = filename.replace(".bz2", "")

Some comments here:

  * By my reading of the docs, the second argument to BZ2File defaults
    to 'r' anyway, so you could omit it.

  * We try to use single quotes wherever possible (except when triple
    quoting). You're using a mix ...

  * .replace() will happily replace anywhere in the string. Please
    consider using stgit.util.strip_suffix() instead.

Or use os.path.splitext(filename) which will save you a couple of
endswith calls as well.

And last but not least, it'd be terrific if you'd let me bully you
into adding .gz and .bz2 test cases for t1800-import. :-)

-- 
David Kågedal [off-list ref]