patch, 8 partsInitial support for Python 3

> Although 2to3 will fix most issues in Python 2 code to make it run under
> Python 3, it does not handle the new strict separation between byte
> strings and unicode strings.  There is one instance in
> git_remote_helpers where we are caught by this.
> 
> Fix it by explicitly decoding the incoming byte string into a unicode
> string.  In this instance, use the locale under which the application is
> running.
> 
> Signed-off-by: John Keeping <john@keeping.me.uk>
> ---
>  git_remote_helpers/git/importer.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/git_remote_helpers/git/importer.py b/git_remote_helpers/git/importer.py
> index e28cc8f..6814003 100644
> --- a/git_remote_helpers/git/importer.py
> +++ b/git_remote_helpers/git/importer.py
> @@ -20,7 +20,7 @@ class GitImporter(object):
>          """Returns a dictionary with refs.
>          """
>          args = ["git", "--git-dir=" + gitdir, "for-each-ref", "refs/heads"]
> -        lines = check_output(args).strip().split('\n')
> +        lines = check_output(args).decode().strip().split('\n')
>          refs = {}
>          for line in lines:
>              value, name = line.split(' ')
> 

> On 01/12/2013 08:23 PM, John Keeping wrote:
>> Although 2to3 will fix most issues in Python 2 code to make it run under
>> Python 3, it does not handle the new strict separation between byte
>> strings and unicode strings.  There is one instance in
>> git_remote_helpers where we are caught by this.
>> 
>> Fix it by explicitly decoding the incoming byte string into a unicode
>> string.  In this instance, use the locale under which the application is
>> running.
>> 
>> Signed-off-by: John Keeping <john@keeping.me.uk>
>> ---
>>  git_remote_helpers/git/importer.py | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/git_remote_helpers/git/importer.py b/git_remote_helpers/git/importer.py
>> index e28cc8f..6814003 100644
>> --- a/git_remote_helpers/git/importer.py
>> +++ b/git_remote_helpers/git/importer.py
>> @@ -20,7 +20,7 @@ class GitImporter(object):
>>          """Returns a dictionary with refs.
>>          """
>>          args = ["git", "--git-dir=" + gitdir, "for-each-ref", "refs/heads"]
>> -        lines = check_output(args).strip().split('\n')
>> +        lines = check_output(args).decode().strip().split('\n')
>>          refs = {}
>>          for line in lines:
>>              value, name = line.split(' ')
>> 
> 
> Won't this change cause an exception if the branch names are not all
> valid strings in the current locale's encoding?  I don't see how this
> assumption is justified (e.g., see git-check-ref-format(1) for the rules
> governing reference names).

> On Sun, Jan 13, 2013 at 04:26:39AM +0100, Michael Haggerty wrote:
>> On 01/12/2013 08:23 PM, John Keeping wrote:
>>> Although 2to3 will fix most issues in Python 2 code to make it run under
>>> Python 3, it does not handle the new strict separation between byte
>>> strings and unicode strings.  There is one instance in
>>> git_remote_helpers where we are caught by this.
>>>
>>> Fix it by explicitly decoding the incoming byte string into a unicode
>>> string.  In this instance, use the locale under which the application is
>>> running.
>>>
>>> Signed-off-by: John Keeping <john@keeping.me.uk>
>>> ---
>>>  git_remote_helpers/git/importer.py | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/git_remote_helpers/git/importer.py b/git_remote_helpers/git/importer.py
>>> index e28cc8f..6814003 100644
>>> --- a/git_remote_helpers/git/importer.py
>>> +++ b/git_remote_helpers/git/importer.py
>>> @@ -20,7 +20,7 @@ class GitImporter(object):
>>>          """Returns a dictionary with refs.
>>>          """
>>>          args = ["git", "--git-dir=" + gitdir, "for-each-ref", "refs/heads"]
>>> -        lines = check_output(args).strip().split('\n')
>>> +        lines = check_output(args).decode().strip().split('\n')
>>>          refs = {}
>>>          for line in lines:
>>>              value, name = line.split(' ')
>>>
>>
>> Won't this change cause an exception if the branch names are not all
>> valid strings in the current locale's encoding?  I don't see how this
>> assumption is justified (e.g., see git-check-ref-format(1) for the rules
>> governing reference names).
> 
> Yes it will.  The problem is that for Python 3 we need to decode the
> byte string into a unicode string, which means we need to know what
> encoding it is.
> 
> I don't think we can just say "git-for-each-ref will print refs in
> UTF-8" since AFAIK git doesn't care what encoding the refs are in - I
> suspect that's determined by the filesystem which in the end probably
> maps to whatever bytes the shell fed git when the ref was created.
> 
> That's why I chose the current locale in this case.  I'm hoping someone
> here will correct me if we can do better, but I don't see any way of
> avoiding choosing some encoding here if we want to support Python 3
> (which I think we will, even if we don't right now).

> On 01/13/2013 05:17 PM, John Keeping wrote:
>> On Sun, Jan 13, 2013 at 04:26:39AM +0100, Michael Haggerty wrote:
>>> On 01/12/2013 08:23 PM, John Keeping wrote:
>>>> Although 2to3 will fix most issues in Python 2 code to make it run under
>>>> Python 3, it does not handle the new strict separation between byte
>>>> strings and unicode strings.  There is one instance in
>>>> git_remote_helpers where we are caught by this.
>>>>
>>>> Fix it by explicitly decoding the incoming byte string into a unicode
>>>> string.  In this instance, use the locale under which the application is
>>>> running.
>>>>
>>>> Signed-off-by: John Keeping <john@keeping.me.uk>
>>>> ---
>>>>  git_remote_helpers/git/importer.py | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/git_remote_helpers/git/importer.py b/git_remote_helpers/git/importer.py
>>>> index e28cc8f..6814003 100644
>>>> --- a/git_remote_helpers/git/importer.py
>>>> +++ b/git_remote_helpers/git/importer.py
>>>> @@ -20,7 +20,7 @@ class GitImporter(object):
>>>>          """Returns a dictionary with refs.
>>>>          """
>>>>          args = ["git", "--git-dir=" + gitdir, "for-each-ref", "refs/heads"]
>>>> -        lines = check_output(args).strip().split('\n')
>>>> +        lines = check_output(args).decode().strip().split('\n')
>>>>          refs = {}
>>>>          for line in lines:
>>>>              value, name = line.split(' ')
>>>>
>>>
>>> Won't this change cause an exception if the branch names are not all
>>> valid strings in the current locale's encoding?  I don't see how this
>>> assumption is justified (e.g., see git-check-ref-format(1) for the rules
>>> governing reference names).
>> 
>> Yes it will.  The problem is that for Python 3 we need to decode the
>> byte string into a unicode string, which means we need to know what
>> encoding it is.
>> 
>> I don't think we can just say "git-for-each-ref will print refs in
>> UTF-8" since AFAIK git doesn't care what encoding the refs are in - I
>> suspect that's determined by the filesystem which in the end probably
>> maps to whatever bytes the shell fed git when the ref was created.
>> 
>> That's why I chose the current locale in this case.  I'm hoping someone
>> here will correct me if we can do better, but I don't see any way of
>> avoiding choosing some encoding here if we want to support Python 3
>> (which I think we will, even if we don't right now).
> 
> I'm not just trying to be a nuisance here;

>                                            I'm struggling myself to
> understand how a program that cares about strings-vs-bytes (e.g., a
> Python3 script) should coexist with a program that doesn't (e.g., git
> [1]).  I think this will become a big issue if my Python version of the
> commit email script ever gets integrated and then made compatible with
> Python3.
> 
> You claim "for Python 3 we need to decode the byte string into a unicode
> string".  I understand that Python 3 strings are Unicode, but why/when
> is it necessary to decode data into a Unicode string as opposed to
> leaving it as a byte sequence?
> 
> In this particular case (from a cursory look over the code) it seems to
> me that (1) decoding to Unicode will sometimes fail for data that git
> considers valid and (2) there is no obvious reason that the data cannot
> be processed as byte sequences.

> Although 2to3 will fix most issues in Python 2 code to make it run under
> Python 3, it does not handle the new strict separation between byte
> strings and unicode strings.  There is one instance in
> git_remote_helpers where we are caught by this, which is when reading
> refs from "git for-each-ref".
>
> While we could fix this by explicitly handling refs as byte strings,
> this is merely punting the problem to users of the library since the
> same problem will be encountered as soon you want to display the ref
> name to a user.
>
> Instead of doing this, explicit decode the incoming byte string into a
> unicode string.

> John Keeping <john@keeping.me.uk> writes:
>> Although 2to3 will fix most issues in Python 2 code to make it run under
>> Python 3, it does not handle the new strict separation between byte
>> strings and unicode strings.  There is one instance in
>> git_remote_helpers where we are caught by this, which is when reading
>> refs from "git for-each-ref".
>>
>> While we could fix this by explicitly handling refs as byte strings,
>> this is merely punting the problem to users of the library since the
>> same problem will be encountered as soon you want to display the ref
>> name to a user.
>>
>> Instead of doing this, explicit decode the incoming byte string into a
>> unicode string.
> 
> That really feels wrong.  Displaying is a separate issue and it is
> the _right_ thing to punt the problem at the lower-level machinery
> level.

>> Following the lead of pygit2 (the Python bindings for
>> libgit2 - see [1] and [2]),...
> 
> I do not think other people getting it wrong is not an excuse to
> repeat the same mistake.
>
> Is it really so cumbersome to handle byte strings as byte strings in
> Python?

>> That really feels wrong.  Displaying is a separate issue and it is
>> the _right_ thing to punt the problem at the lower-level machinery
>> level.
>
> But the display will require decoding the ref name to a Unicode string,
> which depends on the encoding of the underlying ref name, so it feels
> like it should be decoded where it's read (see [1]).

> John Keeping <john@keeping.me.uk> writes:
>>> That really feels wrong.  Displaying is a separate issue and it is
>>> the _right_ thing to punt the problem at the lower-level machinery
>>> level.
>>
>> But the display will require decoding the ref name to a Unicode string,
>> which depends on the encoding of the underlying ref name, so it feels
>> like it should be decoded where it's read (see [1]).
> 
> If you botch the decoding in a way you cannot recover the original
> byte string, you cannot create a ref whose name is the original byte
> string, no?  Keeping the original byte string internally (this
> includes where you use it to create new refs or update existing
> refs), and attempting to convert it to Unicode when you choose to
> show that string as a part of a message to the user (and falling
> back to replacing some bytes to '?' if you cannot, but do so only in
> the message), you won't have that problem.

> diff --git a/git_remote_helpers/git/importer.py b/git_remote_helpers/git/importer.py
> @@ -18,13 +18,16 @@ class GitImporter(object):
>  
>      def get_refs(self, gitdir):
>          """Returns a dictionary with refs.
> +
> +        Note that the keys in the returned dictionary are byte strings as
> +        read from git.
>          """
>          args = ["git", "--git-dir=" + gitdir, "for-each-ref", "refs/heads"]
> -        lines = check_output(args).strip().split('\n')
> +        lines = check_output(args).strip().split('\n'.encode('utf-8'))
>          refs = {}
>          for line in lines:
> -            value, name = line.split(' ')
> -            name = name.strip('commit\t')
> +            value, name = line.split(' '.encode('utf-8'))
> +            name = name.strip('commit\t'.encode('utf-8'))
>              refs[name] = value
>          return refs

> john@keeping.me.uk wrote on Tue, 15 Jan 2013 22:40 +0000:
>> This is what keeping the refs as byte strings looks like.
> 
> As John knows, it is not possible to interpret text from a byte
> string without talking about the character encoding.
> 
> Git is (largely) a C program and uses the character set defined
> in the C standard, which is a subset of ASCII.  But git does
> "math" on strings, like this snippet that takes something from
> argv[] and prepends "refs/heads/":
> 
>     strcpy(refname, "refs/heads/");
>     strcpy(refname + strlen("refs/heads/"), ret->name);
> 
> The result doesn't talk about what character set it is using,
> but because it combines a prefix from ASCII with its input,
> git makes the assumption that the input is ASCII-compatible.
> 
> If you feed a UTF-16 string in argv, e.g.
> 
>     $ echo master | iconv -f ascii -t utf16 | xargs git branch
>     xargs: Warning: a NUL character occurred in the input.  It cannot be passed through in the argument list.  Did you mean to use the --null option?
>     fatal: Not a valid object name: ''.
> 
> you get an error about NUL, and not the branch you hoped for.
> Git assumes that the input character set contains roughly ASCII
> in byte positions 0..127.
> 
> That's one small reason why the useful character encodings put
> ASCII in the 0..127 range, including utf-8, big5 and shift-jis.
> ASCII is indeed special due to its legacy, and both C and Python
> recognize this.
> 
>> diff --git a/git_remote_helpers/git/importer.py b/git_remote_helpers/git/importer.py
>> @@ -18,13 +18,16 @@ class GitImporter(object):
>>  
>>      def get_refs(self, gitdir):
>>          """Returns a dictionary with refs.
>> +
>> +        Note that the keys in the returned dictionary are byte strings as
>> +        read from git.
>>          """
>>          args = ["git", "--git-dir=" + gitdir, "for-each-ref", "refs/heads"]
>> -        lines = check_output(args).strip().split('\n')
>> +        lines = check_output(args).strip().split('\n'.encode('utf-8'))
>>          refs = {}
>>          for line in lines:
>> -            value, name = line.split(' ')
>> -            name = name.strip('commit\t')
>> +            value, name = line.split(' '.encode('utf-8'))
>> +            name = name.strip('commit\t'.encode('utf-8'))
>>              refs[name] = value
>>          return refs
> 
> I'd suggest for this Python conundrum using byte-string literals, e.g.:
> 
>         lines = check_output(args).strip().split(b'\n')
> 	value, name = line.split(b' ')
> 	name = name.strip(b'commit\t')
> 
> Essentially identical to what you have, but avoids naming "utf-8" as
> the encoding.  It instead relies on Python's interpretation of
> ASCII characters in string context, which is exactly what C does.

> On Tue, Jan 15, 2013 at 07:03:16PM -0500, Pete Wyckoff wrote:
> > I'd suggest for this Python conundrum using byte-string literals, e.g.:
> > 
> >         lines = check_output(args).strip().split(b'\n')
> > 	value, name = line.split(b' ')
> > 	name = name.strip(b'commit\t')
> > 
> > Essentially identical to what you have, but avoids naming "utf-8" as
> > the encoding.  It instead relies on Python's interpretation of
> > ASCII characters in string context, which is exactly what C does.
> 
> The problem is that AFAICT the byte-string prefix is only available in
> Python 2.7 and later (compare [1] and [2]).  I think we need this more
> convoluted code if we want to keep supporting Python 2.6 (although
> perhaps 'ascii' would be a better choice than 'utf-8').
> 
> [1] http://docs.python.org/2.6/reference/lexical_analysis.html#literals
> [2] http://docs.python.org/2.7/reference/lexical_analysis.html#literals

> +py_version=$(shell $(PYTHON_PATH) -c \
> +	'import sys; print("%i.%i" % sys.version_info[:2])')
> +
>  all: $(pysetupfile)
> -	$(QUIET)$(PYTHON_PATH) $(pysetupfile) $(QUIETSETUP) build
> +	$(QUIET)test "$$(cat GIT-PYTHON_VERSION 2>/dev/null)" = "$(py_version)" || \
> +	flags=--force; \
> +	$(PYTHON_PATH) $(pysetupfile) $(QUIETSETUP) build $$flags
> +	$(QUIET)echo "$(py_version)" >GIT-PYTHON_VERSION

> john@keeping.me.uk wrote on Sat, 12 Jan 2013 19:23 +0000:
>> When different version of python are used to build via distutils, the
>> behaviour can change.  Detect changes in version and pass --force in
>> this case.
>[..]
>> diff --git a/git_remote_helpers/Makefile b/git_remote_helpers/Makefile
>[..]
>> +py_version=$(shell $(PYTHON_PATH) -c \
>> +	'import sys; print("%i.%i" % sys.version_info[:2])')
>> +
>>  all: $(pysetupfile)
>> -	$(QUIET)$(PYTHON_PATH) $(pysetupfile) $(QUIETSETUP) build
>> +	$(QUIET)test "$$(cat GIT-PYTHON_VERSION 2>/dev/null)" = "$(py_version)" || \
>> +	flags=--force; \
>> +	$(PYTHON_PATH) $(pysetupfile) $(QUIETSETUP) build $$flags
>> +	$(QUIET)echo "$(py_version)" >GIT-PYTHON_VERSION
> 
> Can you depend on ../GIT-PYTHON-VARS instead?  It comes from
> 96a4647 (Makefile: detect when PYTHON_PATH changes, 2012-12-18).
> It doesn't check version, just path, but hopefully that's good
> enough.  I'm imagining a rule that would do "clean" if
> ../GIT-PYTHON-VARS changed, then build without --force.

> On Sat, Jan 12, 2013 at 06:30:44PM -0500, Pete Wyckoff wrote:
> > john@keeping.me.uk wrote on Sat, 12 Jan 2013 19:23 +0000:
> >> When different version of python are used to build via distutils, the
> >> behaviour can change.  Detect changes in version and pass --force in
> >> this case.
> >[..]
> >> diff --git a/git_remote_helpers/Makefile b/git_remote_helpers/Makefile
> >[..]
> >> +py_version=$(shell $(PYTHON_PATH) -c \
> >> +	'import sys; print("%i.%i" % sys.version_info[:2])')
> >> +
> >>  all: $(pysetupfile)
> >> -	$(QUIET)$(PYTHON_PATH) $(pysetupfile) $(QUIETSETUP) build
> >> +	$(QUIET)test "$$(cat GIT-PYTHON_VERSION 2>/dev/null)" = "$(py_version)" || \
> >> +	flags=--force; \
> >> +	$(PYTHON_PATH) $(pysetupfile) $(QUIETSETUP) build $$flags
> >> +	$(QUIET)echo "$(py_version)" >GIT-PYTHON_VERSION
> > 
> > Can you depend on ../GIT-PYTHON-VARS instead?  It comes from
> > 96a4647 (Makefile: detect when PYTHON_PATH changes, 2012-12-18).
> > It doesn't check version, just path, but hopefully that's good
> > enough.  I'm imagining a rule that would do "clean" if
> > ../GIT-PYTHON-VARS changed, then build without --force.
> 
> I was trying to keep the git_remote_helpers directory self contained.  I
> can't see how to depend on ../GIT-PYTHON-VARS in a way that is as simple
> as this and keeps "make -C git_remote_helpers" working in a clean tree.
> 
> Am I missing something obvious here?

> john@keeping.me.uk wrote on Sun, 13 Jan 2013 16:26 +0000:
>> On Sat, Jan 12, 2013 at 06:30:44PM -0500, Pete Wyckoff wrote:
>> > john@keeping.me.uk wrote on Sat, 12 Jan 2013 19:23 +0000:
>> >> When different version of python are used to build via distutils, the
>> >> behaviour can change.  Detect changes in version and pass --force in
>> >> this case.
>> >[..]
>> >> diff --git a/git_remote_helpers/Makefile b/git_remote_helpers/Makefile
>> >[..]
>> >> +py_version=$(shell $(PYTHON_PATH) -c \
>> >> +	'import sys; print("%i.%i" % sys.version_info[:2])')
>> >> +
>> >>  all: $(pysetupfile)
>> >> -	$(QUIET)$(PYTHON_PATH) $(pysetupfile) $(QUIETSETUP) build
>> >> +	$(QUIET)test "$$(cat GIT-PYTHON_VERSION 2>/dev/null)" = "$(py_version)" || \
>> >> +	flags=--force; \
>> >> +	$(PYTHON_PATH) $(pysetupfile) $(QUIETSETUP) build $$flags
>> >> +	$(QUIET)echo "$(py_version)" >GIT-PYTHON_VERSION
>> > 
>> > Can you depend on ../GIT-PYTHON-VARS instead?  It comes from
>> > 96a4647 (Makefile: detect when PYTHON_PATH changes, 2012-12-18).
>> > It doesn't check version, just path, but hopefully that's good
>> > enough.  I'm imagining a rule that would do "clean" if
>> > ../GIT-PYTHON-VARS changed, then build without --force.
>> 
>> I was trying to keep the git_remote_helpers directory self contained.  I
>> can't see how to depend on ../GIT-PYTHON-VARS in a way that is as simple
>> as this and keeps "make -C git_remote_helpers" working in a clean tree.
>> 
>> Am I missing something obvious here?
> 
> Not if it wants to stay self-contained; you're right.
> 
> I'm not thrilled with how git_remote_helpers/Makefile always
> runs setup.py, and always generates PYLIBDIR, and now always
> invokes python a third time to see if its version changed.

> On Sun, Jan 13, 2013 at 12:14:02PM -0500, Pete Wyckoff wrote:
>> john@keeping.me.uk wrote on Sun, 13 Jan 2013 16:26 +0000:
>>> On Sat, Jan 12, 2013 at 06:30:44PM -0500, Pete Wyckoff wrote:
>>> > john@keeping.me.uk wrote on Sat, 12 Jan 2013 19:23 +0000:
>>> >> When different version of python are used to build via distutils, the
>>> >> behaviour can change.  Detect changes in version and pass --force in
>>> >> this case.
>>> >[..]
>>> >> diff --git a/git_remote_helpers/Makefile b/git_remote_helpers/Makefile
>>> >[..]
>>> >> +py_version=$(shell $(PYTHON_PATH) -c \
>>> >> +	'import sys; print("%i.%i" % sys.version_info[:2])')
>>> >> +
>>> >>  all: $(pysetupfile)
>>> >> -	$(QUIET)$(PYTHON_PATH) $(pysetupfile) $(QUIETSETUP) build
>>> >> +	$(QUIET)test "$$(cat GIT-PYTHON_VERSION 2>/dev/null)" = "$(py_version)" || \
>>> >> +	flags=--force; \
>>> >> +	$(PYTHON_PATH) $(pysetupfile) $(QUIETSETUP) build $$flags
>>> >> +	$(QUIET)echo "$(py_version)" >GIT-PYTHON_VERSION
>>> > 
>>> > Can you depend on ../GIT-PYTHON-VARS instead?  It comes from
>>> > 96a4647 (Makefile: detect when PYTHON_PATH changes, 2012-12-18).
>>> > It doesn't check version, just path, but hopefully that's good
>>> > enough.  I'm imagining a rule that would do "clean" if
>>> > ../GIT-PYTHON-VARS changed, then build without --force.
>>> 
>>> I was trying to keep the git_remote_helpers directory self contained.  I
>>> can't see how to depend on ../GIT-PYTHON-VARS in a way that is as simple
>>> as this and keeps "make -C git_remote_helpers" working in a clean tree.
>>> 
>>> Am I missing something obvious here?
>> 
>> Not if it wants to stay self-contained; you're right.
>> 
>> I'm not thrilled with how git_remote_helpers/Makefile always
>> runs setup.py, and always generates PYLIBDIR, and now always
>> invokes python a third time to see if its version changed.
> 
> I don't think PYLIBDIR will be calculated unless it's used ('=' not
> ':=' means its a deferred variable).
> 
> I wonder if the version check should move into setup.py - it would be
> just as easy to check the file there and massage sys.args, although
> possibly not as neat.

> For reference, putting the version check in setup.py looks like this:
> 
> -- >8 --
> 
> diff --git a/git_remote_helpers/setup.py b/git_remote_helpers/setup.py
> index 6de41de..2c21eb5 100644
> --- a/git_remote_helpers/setup.py
> +++ b/git_remote_helpers/setup.py
> @@ -3,6 +3,7 @@
>  """Distutils build/install script for the git_remote_helpers package."""
>  
>  from distutils.core import setup
> +import sys
>  
>  # If building under Python3 we need to run 2to3 on the code, do this by
>  # trying to import distutils' 2to3 builder, which is only available in
> @@ -13,6 +14,24 @@ except ImportError:
>      # 2.x
>      from distutils.command.build_py import build_py
>  
> +
> +current_version = '%d.%d' % sys.version_info[:2]
> +try:
> +    f = open('GIT-PYTHON_VERSION', 'r')
> +    latest_version = f.read().strip()
> +    f.close()
> +
> +    if latest_version != current_version:
> +        if not '--force' in sys.argv:
> +            sys.argv.insert(0, '--force')
> +except IOError:
> +    pass
> +
> +f = open('GIT-PYTHON_VERSION', 'w')
> +f.write(current_version)
> +f.close()
> +
> +
>  setup(
>      name = 'git_remote_helpers',
>      version = '0.1.0',
> 

> I started having a look to see how much work would be needed to make Git
> work with Python 3 and the answer is mostly not much.  The exception is
> git-p4.py which is hit hard by the distinction between byte strings and
> unicode strings, particularly because the Python output mode of p4
> targets Python 2.
> 
> I don't know if it's worthwhile to actually apply these but here they
> are in case anyone's interested.
> 
> Having said that, the changes are minimal and involve either wrapping
> parentheses around arguments to print or being a bit more explicit about
> how we expect byte strings to be decoded to unicode.
> 
> With these patches all tests pass with python3 except t98* (git-p4), but
> there are a couple of topics in-flight which will affect that
> (fc/remote-testgit-feature-done and er/replace-cvsimport).
> 
> John Keeping (8):
>   git_remote_helpers: Allow building with Python 3
>   git_remote_helpers: fix input when running under Python 3
>   git_remote_helpers: Force rebuild if python version changes
>   git_remote_helpers: Use 2to3 if building with Python 3
>   svn-fe: allow svnrdump_sim.py to run with Python 3
>   git-remote-testpy: hash bytes explicitly
>   git-remote-testpy: don't do unbuffered text I/O
>   git-remote-testpy: call print as a function
> 
>  contrib/svn-fe/svnrdump_sim.py     |  4 ++--
>  git-remote-testpy.py               | 40 +++++++++++++++++++-------------------
>  git_remote_helpers/.gitignore      |  1 +
>  git_remote_helpers/Makefile        | 10 ++++++++--
>  git_remote_helpers/git/importer.py |  2 +-
>  git_remote_helpers/setup.py        | 10 ++++++++++
>  6 files changed, 42 insertions(+), 25 deletions(-)

> john@keeping.me.uk wrote on Sat, 12 Jan 2013 19:23 +0000:
>> I started having a look to see how much work would be needed to make Git
>> work with Python 3 and the answer is mostly not much.  The exception is
>> git-p4.py which is hit hard by the distinction between byte strings and
>> unicode strings, particularly because the Python output mode of p4
>> targets Python 2.
>> 
>> I don't know if it's worthwhile to actually apply these but here they
>> are in case anyone's interested.
>> 
>> Having said that, the changes are minimal and involve either wrapping
>> parentheses around arguments to print or being a bit more explicit about
>> how we expect byte strings to be decoded to unicode.
>> 
>> With these patches all tests pass with python3 except t98* (git-p4), but
>> there are a couple of topics in-flight which will affect that
>> (fc/remote-testgit-feature-done and er/replace-cvsimport).
>> 
>> John Keeping (8):
>>   git_remote_helpers: Allow building with Python 3
>>   git_remote_helpers: fix input when running under Python 3
>>   git_remote_helpers: Force rebuild if python version changes
>>   git_remote_helpers: Use 2to3 if building with Python 3
>>   svn-fe: allow svnrdump_sim.py to run with Python 3
>>   git-remote-testpy: hash bytes explicitly
>>   git-remote-testpy: don't do unbuffered text I/O
>>   git-remote-testpy: call print as a function
>> 
>>  contrib/svn-fe/svnrdump_sim.py     |  4 ++--
>>  git-remote-testpy.py               | 40 +++++++++++++++++++-------------------
>>  git_remote_helpers/.gitignore      |  1 +
>>  git_remote_helpers/Makefile        | 10 ++++++++--
>>  git_remote_helpers/git/importer.py |  2 +-
>>  git_remote_helpers/setup.py        | 10 ++++++++++
>>  6 files changed, 42 insertions(+), 25 deletions(-)
> 
> These look good, in that there are relatively few changed needed.
> 
> Sebastian Morr tried a similar patch a year ago, in
> 
>     http://thread.gmane.org/gmane.comp.version-control.git/187545
> 
> He made changes beyond yours, in particular "print >>" lines,
> that you seem to handle with 2to3 during the build.  I'm not sure
> which approach is better in the long run.  He worked on the
> other .py in contrib/ too.

> Can you give me some hints about the byte/unicode string issues
> in git-p4.py?  There's really only one place that does:
> 
>     p4 = subprocess.Popen("p4 -G ...")
>     marshal.load(p4.stdout)
> 
> If that's the only issue, this might not be too paniful.

> On Sat, Jan 12, 2013 at 06:43:04PM -0500, Pete Wyckoff wrote:
>> Can you give me some hints about the byte/unicode string issues
>> in git-p4.py?  There's really only one place that does:
>> 
>>     p4 = subprocess.Popen("p4 -G ...")
>>     marshal.load(p4.stdout)
>> 
>> If that's the only issue, this might not be too paniful.
> 
> The problem is that what gets loaded there is a dictionary (encoded by
> p4) that maps byte strings to byte strings, so all of the accesses to
> that dictionary need to either:
> 
>    1) explicitly call encode() on a string constant
> or 2) use a byte string constant with a "b" prefix
> 
> Or we could re-write the dictionary once, which handles the keys... but
> some of the values are also used as strings and we can't handle that as
> a one-off conversion since in other places we really do want the byte
> string (think content of binary files).
> 
> Basically a thorough audit of all access to variables that come from p4
> would be needed, with explicit decode()s for authors, dates, etc.

> On Sat, Jan 12, 2013 at 06:43:04PM -0500, Pete Wyckoff wrote:
> > Can you give me some hints about the byte/unicode string issues
> > in git-p4.py?  There's really only one place that does:
> > 
> >     p4 = subprocess.Popen("p4 -G ...")
> >     marshal.load(p4.stdout)
> > 
> > If that's the only issue, this might not be too paniful.
> 
> The problem is that what gets loaded there is a dictionary (encoded by
> p4) that maps byte strings to byte strings, so all of the accesses to
> that dictionary need to either:
> 
>    1) explicitly call encode() on a string constant
> or 2) use a byte string constant with a "b" prefix
> 
> Or we could re-write the dictionary once, which handles the keys... but
> some of the values are also used as strings and we can't handle that as
> a one-off conversion since in other places we really do want the byte
> string (think content of binary files).
> 
> Basically a thorough audit of all access to variables that come from p4
> would be needed, with explicit decode()s for authors, dates, etc.

> > I hesitated to take Sebastian's changes due to the huge number of
> > print() lines, but maybe a 2to3 approach would make that aspect
> > of python3 support not too onerous.
> 
> I think we'd want to change to print() eventually and having a single
> codebase for 2 and 3 would be nicer for development, but I think we need
> to be able to say "no one is using Python 2.5 or earlier" before we can
> do that and I'm not sure we're there yet.  From where we are at the
> moment I think 2to3 is a good answer, particularly where we're already
> using distutils to generate a release image.

> john@keeping.me.uk wrote on Sun, 13 Jan 2013 00:41 +0000:
>> On Sat, Jan 12, 2013 at 06:43:04PM -0500, Pete Wyckoff wrote:
>> > Can you give me some hints about the byte/unicode string issues
>> > in git-p4.py?  There's really only one place that does:
>> > 
>> >     p4 = subprocess.Popen("p4 -G ...")
>> >     marshal.load(p4.stdout)
>> > 
>> > If that's the only issue, this might not be too paniful.
>> 
>> The problem is that what gets loaded there is a dictionary (encoded by
>> p4) that maps byte strings to byte strings, so all of the accesses to
>> that dictionary need to either:
>> 
>>    1) explicitly call encode() on a string constant
>> or 2) use a byte string constant with a "b" prefix
>> 
>> Or we could re-write the dictionary once, which handles the keys... but
>> some of the values are also used as strings and we can't handle that as
>> a one-off conversion since in other places we really do want the byte
>> string (think content of binary files).
>> 
>> Basically a thorough audit of all access to variables that come from p4
>> would be needed, with explicit decode()s for authors, dates, etc.
> 
> Your auto-conversion snippet in the follow-up mail would work
> fine for most keys and values.  A few perforce docs and some
> playing around convince me that it is mostly utf-8, except for
> file data for particular types.
> 
> I'd still rather handle each command separately, and think about
> the conversions, to do it right in the long run.

>> > I hesitated to take Sebastian's changes due to the huge number of
>> > print() lines, but maybe a 2to3 approach would make that aspect
>> > of python3 support not too onerous.
>> 
>> I think we'd want to change to print() eventually and having a single
>> codebase for 2 and 3 would be nicer for development, but I think we need
>> to be able to say "no one is using Python 2.5 or earlier" before we can
>> do that and I'm not sure we're there yet.  From where we are at the
>> moment I think 2to3 is a good answer, particularly where we're already
>> using distutils to generate a release image.
> 
> Agreed.  The 2to3 diff is large but straightforward.  But these
> p4 -G interface errors require a lot of thought and work.  I'm
> not too eager to work on this yet.

> On Thu, Jan 17, 2013 at 09:15:08PM -0800, Sverre Rabbelier wrote:
>> On Thu, Jan 17, 2013 at 10:53 AM, John Keeping <john@keeping.me.uk> wrote:
>> > [1] http://wiki.python.org/moin/PortingPythonToPy3k
>>
>> This link seems dead.
>
> Looks like the Python wiki is down [1].
>
> I'll replace it with [2] since the content is similar and it should be
> easier to find a mirror of the Python documentation than of the wiki.
>
> [1] http://pyfound.blogspot.co.uk/2013/01/wikipythonorg-compromised.html
> [2] http://docs.python.org/3.3/howto/pyporting.html#during-installation
>
>
> John

> Under Python 3 'hasher.update(...)' must take a byte string and not a
> unicode string.  Explicitly encode the argument to this method as UTF-8
> so that this code works under Python 3.
>
> This moves the required Python version forward to 2.0.
>
> Signed-off-by: John Keeping <john@keeping.me.uk>
> ---

>  git-remote-testpy.py | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/git-remote-testpy.py b/git-remote-testpy.py
> index d94a66a..f8dc196 100644
> --- a/git-remote-testpy.py
> +++ b/git-remote-testpy.py
> @@ -31,9 +31,9 @@ from git_remote_helpers.git.exporter import GitExporter
>  from git_remote_helpers.git.importer import GitImporter
>  from git_remote_helpers.git.non_local import NonLocalGit
>  
> -if sys.hexversion < 0x01050200:
> -    # os.makedirs() is the limiter
> -    sys.stderr.write("git-remote-testgit: requires Python 1.5.2 or later.\n")
> +if sys.hexversion < 0x02000000:
> +    # string.encode() is the limiter
> +    sys.stderr.write("git-remote-testgit: requires Python 2.0 or later.\n")
>      sys.exit(1)
>  
>  def get_repo(alias, url):
> @@ -45,7 +45,7 @@ def get_repo(alias, url):
>      repo.get_head()
>  
>      hasher = _digest()
> -    hasher.update(repo.path)
> +    hasher.update(repo.path.encode('utf-8'))
>      repo.hash = hasher.hexdigest()
>  
>      repo.get_base_path = lambda base: os.path.join(

> John Keeping <john@keeping.me.uk> writes:
>
>> Under Python 3 'hasher.update(...)' must take a byte string and not a
>> unicode string.  Explicitly encode the argument to this method as UTF-8
>> so that this code works under Python 3.
>>
>> This moves the required Python version forward to 2.0.
>>
>> Signed-off-by: John Keeping <john@keeping.me.uk>
>> ---
>
> Hmph.  So what happens when the path is _not_ encoded in UTF-8?

> John Keeping <john@keeping.me.uk> writes:
> 
>> Under Python 3 'hasher.update(...)' must take a byte string and not a
>> unicode string.  Explicitly encode the argument to this method as UTF-8
>> so that this code works under Python 3.
>>
>> This moves the required Python version forward to 2.0.
>>
>> Signed-off-by: John Keeping <john@keeping.me.uk>
>> ---
> 
> Hmph.  So what happens when the path is _not_ encoded in UTF-8?

>                                    If that is not the case, and if
> this is used only to get a randomly-looking 40-byte hexadecimal
> string, then a lossy attempt to .encode('utf-8') and falling back to
> replace or ignore bytes in the original that couldn't be interpreted
> as part of a UTF-8 string would be OK, but doesn't .encode('utf-8')
> throw an exception if not told to 'ignore' or something?

> >  git-remote-testpy.py | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/git-remote-testpy.py b/git-remote-testpy.py
> > index d94a66a..f8dc196 100644
> > --- a/git-remote-testpy.py
> > +++ b/git-remote-testpy.py
> > @@ -31,9 +31,9 @@ from git_remote_helpers.git.exporter import GitExporter
> >  from git_remote_helpers.git.importer import GitImporter
> >  from git_remote_helpers.git.non_local import NonLocalGit
> >  
> > -if sys.hexversion < 0x01050200:
> > -    # os.makedirs() is the limiter
> > -    sys.stderr.write("git-remote-testgit: requires Python 1.5.2 or later.\n")
> > +if sys.hexversion < 0x02000000:
> > +    # string.encode() is the limiter
> > +    sys.stderr.write("git-remote-testgit: requires Python 2.0 or later.\n")
> >      sys.exit(1)
> >  
> >  def get_repo(alias, url):
> > @@ -45,7 +45,7 @@ def get_repo(alias, url):
> >      repo.get_head()
> >  
> >      hasher = _digest()
> > -    hasher.update(repo.path)
> > +    hasher.update(repo.path.encode('utf-8'))
> >      repo.hash = hasher.hexdigest()
> >  
> >      repo.get_base_path = lambda base: os.path.join(

> On Thu, Jan 17, 2013 at 12:36:33PM -0800, Junio C Hamano wrote:
>> John Keeping <john@keeping.me.uk> writes:
>> 
>>> Under Python 3 'hasher.update(...)' must take a byte string and not a
>>> unicode string.  Explicitly encode the argument to this method as UTF-8
>>> so that this code works under Python 3.
>>>
>>> This moves the required Python version forward to 2.0.
>>>
>>> Signed-off-by: John Keeping <john@keeping.me.uk>
>>> ---
>> 
>> Hmph.  So what happens when the path is _not_ encoded in UTF-8?
> 
> Do you mean encodable?  As you say below it will currently throw an
> exception.

> > Is the repo.hash (and local.hash that gets a copy of it) something
> > that needs to stay the same across multiple invocations of this
> > remote helper, and between the currently shipped Git and the version
> > of Git after applying this patch?
> 
> It's used to specify the path of the repository for importing or
> exporting, so it should stay consistent across invocations.  However,
> this is only an example remote helper so I don't think we should worry
> if it changes from one Git release to the next.

>
>> >  git-remote-testpy.py | 8 ++++----
>> >  1 file changed, 4 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/git-remote-testpy.py b/git-remote-testpy.py
>> > index d94a66a..f8dc196 100644
>> > --- a/git-remote-testpy.py
>> > +++ b/git-remote-testpy.py
>> > @@ -31,9 +31,9 @@ from git_remote_helpers.git.exporter import GitExporter
>> >  from git_remote_helpers.git.importer import GitImporter
>> >  from git_remote_helpers.git.non_local import NonLocalGit
>> >  
>> > -if sys.hexversion < 0x01050200:
>> > -    # os.makedirs() is the limiter
>> > -    sys.stderr.write("git-remote-testgit: requires Python 1.5.2 or later.\n")
>> > +if sys.hexversion < 0x02000000:
>> > +    # string.encode() is the limiter
>> > +    sys.stderr.write("git-remote-testgit: requires Python 2.0 or later.\n")
>> >      sys.exit(1)
>> >  
>> >  def get_repo(alias, url):
>> > @@ -45,7 +45,7 @@ def get_repo(alias, url):
>> >      repo.get_head()
>> >  
>> >      hasher = _digest()
>> > -    hasher.update(repo.path)
>> > +    hasher.update(repo.path.encode('utf-8'))
>> >      repo.hash = hasher.hexdigest()
>> >  
>> >      repo.get_base_path = lambda base: os.path.join(

> John Keeping <john@keeping.me.uk> writes:
> 
>> You're right - I think we need to add ", errors='replace'" to the call
>> to encode.
> 
> Of if it is used just as a opaque token, you can .encode('hex') or
> something to punt on the whole issue, no?

>>>>  git-remote-testpy.py | 8 ++++----
>>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/git-remote-testpy.py b/git-remote-testpy.py
>>>> index d94a66a..f8dc196 100644
>>>> --- a/git-remote-testpy.py
>>>> +++ b/git-remote-testpy.py
>>>> @@ -31,9 +31,9 @@ from git_remote_helpers.git.exporter import GitExporter
>>>>  from git_remote_helpers.git.importer import GitImporter
>>>>  from git_remote_helpers.git.non_local import NonLocalGit
>>>>  
>>>> -if sys.hexversion < 0x01050200:
>>>> -    # os.makedirs() is the limiter
>>>> -    sys.stderr.write("git-remote-testgit: requires Python 1.5.2 or later.\n")
>>>> +if sys.hexversion < 0x02000000:
>>>> +    # string.encode() is the limiter
>>>> +    sys.stderr.write("git-remote-testgit: requires Python 2.0 or later.\n")
>>>>      sys.exit(1)
>>>>  
>>>>  def get_repo(alias, url):
>>>> @@ -45,7 +45,7 @@ def get_repo(alias, url):
>>>>      repo.get_head()
>>>>  
>>>>      hasher = _digest()
>>>> -    hasher.update(repo.path)
>>>> +    hasher.update(repo.path.encode('utf-8'))
>>>>      repo.hash = hasher.hexdigest()
>>>>  
>>>>      repo.get_base_path = lambda base: os.path.join(

> On Thu, Jan 17, 2013 at 02:24:37PM -0800, Junio C Hamano wrote:
>> John Keeping <john@keeping.me.uk> writes:
>> 
>>> You're right - I think we need to add ", errors='replace'" to the call
>>> to encode.
>> 
>> Of if it is used just as a opaque token, you can .encode('hex') or
>> something to punt on the whole issue, no?
>
> Even better.  Are you happy to squash that in (assuming nothing else
> comes up) or shall I resend?

>
>>>>>  git-remote-testpy.py | 8 ++++----
>>>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/git-remote-testpy.py b/git-remote-testpy.py
>>>>> index d94a66a..f8dc196 100644
>>>>> --- a/git-remote-testpy.py
>>>>> +++ b/git-remote-testpy.py
>>>>> @@ -31,9 +31,9 @@ from git_remote_helpers.git.exporter import GitExporter
>>>>>  from git_remote_helpers.git.importer import GitImporter
>>>>>  from git_remote_helpers.git.non_local import NonLocalGit
>>>>>  
>>>>> -if sys.hexversion < 0x01050200:
>>>>> -    # os.makedirs() is the limiter
>>>>> -    sys.stderr.write("git-remote-testgit: requires Python 1.5.2 or later.\n")
>>>>> +if sys.hexversion < 0x02000000:
>>>>> +    # string.encode() is the limiter
>>>>> +    sys.stderr.write("git-remote-testgit: requires Python 2.0 or later.\n")
>>>>>      sys.exit(1)
>>>>>  
>>>>>  def get_repo(alias, url):
>>>>> @@ -45,7 +45,7 @@ def get_repo(alias, url):
>>>>>      repo.get_head()
>>>>>  
>>>>>      hasher = _digest()
>>>>> -    hasher.update(repo.path)
>>>>> +    hasher.update(repo.path.encode('utf-8'))
>>>>>      repo.hash = hasher.hexdigest()
>>>>>  
>>>>>      repo.get_base_path = lambda base: os.path.join(

> This is harmless in Python 2, which sees the parentheses as redundant
> grouping, but is required for Python 3.  Since this is the only change
> required to make this script just run under Python 3 without needing
> 2to3 it seems worthwhile.
>
> The case of an empty print must be handled specially because in that
> case Python 2 will interpret '()' as an empty tuple and print it as
> '()'; inserting an empty string fixes this.
>
> Signed-off-by: John Keeping <john@keeping.me.uk>
> ---
>  git-remote-testpy.py | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/git-remote-testpy.py b/git-remote-testpy.py
> index bc5e3cf..ccdb2dc 100644
> --- a/git-remote-testpy.py
> +++ b/git-remote-testpy.py
> @@ -87,9 +87,9 @@ def do_capabilities(repo, args):
>      """Prints the supported capabilities.
>      """
>
> -    print "import"
> -    print "export"
> -    print "refspec refs/heads/*:%s*" % repo.prefix
> +    print("import")
> +    print("export")
> +    print("refspec refs/heads/*:%s*" % repo.prefix)
>
>      dirname = repo.get_base_path(repo.gitdir)
>
> @@ -98,11 +98,11 @@ def do_capabilities(repo, args):
>
>      path = os.path.join(dirname, 'git.marks')
>
> -    print "*export-marks %s" % path
> +    print("*export-marks %s" % path)
>      if os.path.exists(path):
> -        print "*import-marks %s" % path
> +        print("*import-marks %s" % path)
>
> -    print # end capabilities
> +    print('') # end capabilities
>
>
>  def do_list(repo, args):
> @@ -115,16 +115,16 @@ def do_list(repo, args):
>
>      for ref in repo.revs:
>          debug("? refs/heads/%s", ref)
> -        print "? refs/heads/%s" % ref
> +        print("? refs/heads/%s" % ref)
>
>      if repo.head:
>          debug("@refs/heads/%s HEAD" % repo.head)
> -        print "@refs/heads/%s HEAD" % repo.head
> +        print("@refs/heads/%s HEAD" % repo.head)
>      else:
>          debug("@refs/heads/master HEAD")
> -        print "@refs/heads/master HEAD"
> +        print("@refs/heads/master HEAD")
>
> -    print # end list
> +    print('') # end list
>
>
>  def update_local_repo(repo):
> @@ -164,7 +164,7 @@ def do_import(repo, args):
>          ref = line[7:].strip()
>          refs.append(ref)
>
> -    print "feature done"
> +    print("feature done")
>
>      if os.environ.get("GIT_REMOTE_TESTGIT_FAILURE"):
>          die('Told to fail')
> @@ -172,7 +172,7 @@ def do_import(repo, args):
>      repo = update_local_repo(repo)
>      repo.exporter.export_repo(repo.gitdir, refs)
>
> -    print "done"
> +    print("done")
>
>
>  def do_export(repo, args):
> @@ -192,8 +192,8 @@ def do_export(repo, args):
>          repo.non_local.push(repo.gitdir)
>
>      for ref in changed:
> -        print "ok %s" % ref
> -    print
> +        print("ok %s" % ref)
> +    print('')
>
>
>  COMMANDS = {
> --
> 1.8.1.1.260.g99b33f4.dirty
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html