duplicity-team team mailing list archive

[Mgorse <mgorse@xxxxxxxx>]

Thanks for the review.

Diff comments:

> 
> === modified file 'duplicity/__init__.py'
> --- duplicity/__init__.py	2018-10-10 20:25:04 +0000
> +++ duplicity/__init__.py	2018-11-29 19:12:48 +0000
> @@ -19,5 +19,9 @@
>  # along with duplicity; if not, write to the Free Software Foundation,
>  # Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
>  
> +import sys
>  import gettext
> -gettext.install(u'duplicity', unicode=True, names=[u'ngettext'])
> +if sys.version_info.major >= 3:
> +    gettext.install(u'duplicity', names=[u'ngettext'])
> +else:
> +    gettext.install(u'duplicity', names=[u'ngettext'])

I can't remember now why that is there; I might have added unicode=True for python 3 andthen removed it. I should probably just remove that version check.

> 
> === modified file 'duplicity/diffdir.py'
> --- duplicity/diffdir.py	2018-10-16 20:56:54 +0000
> +++ duplicity/diffdir.py	2018-11-29 19:12:48 +0000
> @@ -126,8 +135,11 @@
>          Callback activated when FileWithSignature read to end
>          """
>          ti.size = len(sig_string)
> -        ti.name = b"signature/" + b"/".join(index)
> -        sigTarFile.addfile(ti, cStringIO.StringIO(sig_string))
> +        if sys.version_info.major >= 3:

I'm not sure if what I've done is the right approach, but it appears that the name cannot be bytes in python 3, and I was also trying to be cautious and avoid relying on fsencode/fsdecode under python 2, per the comment in util.py.

> +            ti.name = u"signature/" + util.fsdecode(b"/".join(index))
> +        else:
> +            ti.name = b"signature/" + b"/".join(index)
> +        sigTarFile.addfile(ti, io.BytesIO(sig_string))
>  
>      if new_path.isreg() and sig_path and sig_path.isreg() and sig_path.difftype == u"signature":
>          delta_path.difftype = u"diff"
> @@ -481,7 +502,7 @@
>          Make tarblock out of tarinfo and file data
>          """
>          tarinfo.size = len(file_data)
> -        headers = tarinfo.tobuf(errors=u'replace')
> +        headers = tarinfo.tobuf(errors=u'replace', encoding=globals.fsencoding)

I'm not sure where you are proposing that util.fsencode() be used, since the encoding is done within tobuf(). I'm also not 100% sure that that change is needed. Without it, unicode characters were being replaced with ?'s when running the tests, because sys.getfilesystemencoding() was returning ANSI_X3.4-1968, because LANG and LC_CTYPE were unset in the test environment. I also have a change to have tox pass on the value of LC_CTYPE, so that getfilesystemencoding() returns the same value in the test environment as it returns on the system. There is a related comment at the end of globals.py, and it isn't clear to me if that work-around was added because of issues running the tests, or if end users are running into it as well (the latter seems plausible if someone is running a backup from a script that doesn't have LANG or LC_CTYPE set).

>          blocks, remainder = divmod(tarinfo.size, tarfile.BLOCKSIZE)  # @UnusedVariable
>          if remainder > 0:
>              filler_data = b"\0" * (tarfile.BLOCKSIZE - remainder)
> 
> === modified file 'duplicity/patchdir.py'
> --- duplicity/patchdir.py	2018-09-24 17:02:42 +0000
> +++ duplicity/patchdir.py	2018-11-29 19:12:48 +0000
> @@ -149,9 +154,11 @@
>  
>  def get_index_from_tarinfo(tarinfo):
>      u"""Return (index, difftype, multivol) pair from tarinfo object"""
> -    for prefix in [b"snapshot/", b"diff/", b"deleted/",
> -                   b"multivol_diff/", b"multivol_snapshot/"]:
> +    for prefix in [u"snapshot/", u"diff/", u"deleted/",
> +                   u"multivol_diff/", u"multivol_snapshot/"]:
>          tiname = util.get_tarinfo_name(tarinfo)
> +        if sys.version_info.major == 2 and isinstance(prefix, unicode):
> +            prefix = prefix.encode()

I was probably trying to have it be the same type as the tarinfo name, so it's related to the above comment / how we want to handle those.

>          if tiname.startswith(prefix):
>              name = tiname[len(prefix):]  # strip prefix
>              if prefix.startswith(u"multivol"):


-- 
https://code.launchpad.net/~mgorse/duplicity/0.8-series/+merge/359864
Your team duplicity-team is requested to review the proposed merge of lp:~mgorse/duplicity/0.8-series into lp:duplicity.