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

From: Clark Williams <clark.williams@gmail.com>
Date: Jun 11, 2008, 17:28 UTC
Message-ID: <48500B28.1050505@gmail.com>
In-Reply-To: <20080611062753.GB15034@diana.vm.bytemark.co.uk>

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

Karl HasselstrÃ¶m wrote:

Show 27 quoted lines

> On 2008-06-10 08:54:58 -0500, Clark Williams wrote:
> 
>> --- 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 :)

Show 19 quoted lines

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