Initial support for Python 3
cover Initial support for Python 3
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(-)
Signature · 1 line
1.8.1
PATCH 1/8 git_remote_helpers: Allow building with Python 3
Change inline Python to call "print" as a function not a statement.
This is harmless because Python 2 will see the parentheses as redundant grouping but they are necessary to run this code with Python 3.
--- git_remote_helpers/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
git_remote_helpers/Makefile +1 -1
@@ -23,7 +23,7 @@ endif PYLIBDIR=$(shell $(PYTHON_PATH) -c \ "import sys; \ - print 'lib/python%i.%i/site-packages' % sys.version_info[:2]") + print('lib/python%i.%i/site-packages' % sys.version_info[:2])") all: $(pysetupfile) $(QUIET)$(PYTHON_PATH) $(pysetupfile) $(QUIETSETUP) build
Signature · 1 line
1.8.1
[email protected] wrote on Sat, 12 Jan 2013 19:23 +0000:
Quoted 27 lines
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.pywhich 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: allowsvnrdump_sim.pyto 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
Quoted 7 lines
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.
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.
-- Pete
Quoted 38 lines · Pete Wyckoff
[email protected] wrote on Sat, 12 Jan 2013 19:23 +0000:
Quoted 27 lines
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.pywhich 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: allowsvnrdump_sim.pyto 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 functionQuoted 7 lines
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.
Quoted 8 lines
Sebastian Morr tried a similar patch a year ago, in
http://thread.gmane.org/gmane.comp.version-control.git/187545He 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.
In the long run I'd want to move away from "print >>" to use "print(file=..., ...)" but that's only available from Python 2.6 onwards (via a __future__ import) and I think we probably don't want to rule out Python 2.5 yet.
Without 2to3 the only way to do this for both Python 2 and 3 is as "file.write('...\n')".
Quoted 7 lines
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.
Quoted 3 lines
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
Quoted 16 lines · John Keeping
Quoted 7 lines · Pete Wyckoff
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
Quoted 7 lines
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.
Having thought about this a bit more, another possibility would be to apply this transformation once using something like this (completely untested, I haven't looked up the keys of interest):
-- >8 --
def _noop(s):
return s
def _decode(s):
return s.decode('utf-8')CONVERSION_MAP = {
'user': _decode,
'data': _decode
}d = marshal.load(p4.stdout)
retval = {}
for k, v in d.items():
key = k.decode('utf-8')
retval[key] = CONVERSION_MAP.get(key, _noop)(v)
return retval
-- 8< --
Obviously this isn't ideal but without p4 gaining a Python 3 output mode I suspect this would be the best we could do.
John
[email protected] wrote on Sun, 13 Jan 2013 00:41 +0000:
Quoted 16 lines · Pete Wyckoff
Quoted 7 lines
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
Quoted 7 lines
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.
Quoted 10 lines
Quoted 3 lines
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.
Thanks.
-- Pete
Quoted 25 lines · Pete Wyckoff
[email protected] wrote on Sun, 13 Jan 2013 00:41 +0000:
Quoted 16 lines · Pete Wyckoff
Quoted 7 lines
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" prefixQuoted 7 lines
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.
Quoted 7 lines
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 sent that on the assumption that the same key would have similar semantics wherever its used, but I don't use git-p4 or know much about perforce.
It would be interesting to know whether there is any likelihood of p4 gaining a Python 3 output mode (since the documentation currently say not to use "p4 -G" with Python 3). If it does then I would assume that it will make a sensible choice about unicode/bytes such that the existing git-p4 would Just Work with only a small change to the invocation of p4 to add the new argument.
Quoted 11 lines
Quoted 10 lines
Quoted 3 lines
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.
Quoted 3 lines
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.
Fair enough. As I don't use git-p4, it's not something I intend to tackle either (given the scale of the changes involved).
Given the minimal scope of the changes needed for everything else, I sent this series wondering whether it's sensible to move forward on the basis of "Python scripts except git-p4 work with Python 3. You must use Python 2 if you want to use git-p4".
John
PATCH 2/8 git_remote_helpers: fix input when running under Python 3
In reply to John KeepingAlthough 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.
--- git_remote_helpers/git/importer.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
git_remote_helpers/git/importer.py +1 -1
@@ -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(' ')
Signature · 1 line
1.8.1
Re: [PATCH 2/8] git_remote_helpers: fix input when running under Python 3
Quoted 20 lines · John Keeping
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_helperswhere 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 <[email protected]>--- git_remote_helpers/git/importer.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)git_remote_helpers/git/importer.py +0
-0
git_remote_helpers/git/importer.py class GitImporter(object): · line 26
Quoted 8 lines
git_remote_helpers/git/importer.py +1
-1@@ -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).
Michael
Signature · 3 lines
Michael Haggerty [email protected] http://softwareswirl.blogspot.com/
Re: [PATCH 2/8] git_remote_helpers: fix input when running under Python 3
Quoted 26 lines · Michael Haggerty
Quoted 25 lines · John Keeping
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_helperswhere 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 <[email protected]>--- git_remote_helpers/git/importer.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)git_remote_helpers/git/importer.py +1
-1@@ -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 = {}
Quoted 8 lines
git_remote_helpers/git/importer.py class GitImporter(object): · line 26
Quoted 3 lines
git_remote_helpers/git/importer.py +0
-0for 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).
John
Re: [PATCH 2/8] git_remote_helpers: fix input when running under Python 3
Quoted 45 lines · John Keeping
Quoted 26 lines · Michael Haggerty
Quoted 25 lines · John Keeping
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_helperswhere 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 <[email protected]>--- git_remote_helpers/git/importer.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)git_remote_helpers/git/importer.py +1
-1@@ -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 = {}Quoted 8 lines
git_remote_helpers/git/importer.py class GitImporter(object): · line 26
Quoted 3 lines
git_remote_helpers/git/importer.py +0
-0for 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.
Quoted 4 lines
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.
Michael
[1] And it doesn't just seem that "git doesn't care about Unicode *yet*". It seems more likely that "git will adamantly refuse to deal with Unicode". For example, Linus is quite clearly in favor of treating data as byte sequences in most situations:
https://plus.google.com/111049168280159033135/posts/f3fngVm174f
Signature · 3 lines
Michael Haggerty [email protected] http://softwareswirl.blogspot.com/
Re: [PATCH 2/8] git_remote_helpers: fix input when running under Python 3
Quoted 46 lines · Michael Haggerty
Quoted 45 lines · John Keeping
Quoted 26 lines · Michael Haggerty
Quoted 25 lines · John Keeping
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_helperswhere 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 <[email protected]>--- git_remote_helpers/git/importer.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)git_remote_helpers/git/importer.py +1
-1@@ -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 = {}Quoted 8 lines
git_remote_helpers/git/importer.py class GitImporter(object): · line 26
Quoted 3 lines
git_remote_helpers/git/importer.py +0
-0for 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.
Quoted 6 lines
Quoted 4 lines
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;
You're not being - I think this is a difficult issue and I don't know myself what the right answer is.
Quoted 12 lines
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?
Quoted 4 lines
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.
I've been thinking about this overnight and I think you're right that treating them as byte strings in Python is most correct. Having said that, when I'm programming in Python I would find it quite surprising that I had to treat ref strings specially - and as soon as I want to use one in a string context (e.g. printing it as part of a message to the user) I'm back to the same problem.
So I think we should try to solve the problem once rather than forcing everyone who wants to use the library to solve it individually. I just wish it was obvious what we should do!
John
PATCH v2 2/8 git_remote_helpers: fix input when running under 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, 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. Following the lead of pygit2 (the Python bindings for libgit2 - see [1] and [2]), use the filesystem encoding by default, providing a way for callers to override this if necessary.
[1] https://github.com/libgit2/pygit2/blob/e34911b63e5d2266f9f72a4e3f32e27b13190feb/src/pygit2/reference.c#L261
[2] https://github.com/libgit2/pygit2/blob/e34911b63e5d2266f9f72a4e3f32e27b13190feb/include/pygit2/utils.h#L55
---
I think this is in fact the best way to handle this, and I hope the above description clarified why I don't think we want to treat refs as byte strings in Python 3.
My only remaining question is whether it would be better to set the error mode when decoding to "replace" instead of "strict" (the default). "strict" will cause a UnicodeError if the string cannot be decoded whereas "replace" will use U+FFFD (the replacement character). [3]
I think it's better to use "strict" and let the user know that something has gone wrong rather than silently change the string, but I'd welcome other opinions.
[3] http://docs.python.org/2/library/codecs.html#codec-base-classes
git_remote_helpers/git/importer.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)
git_remote_helpers/git/importer.py +12 -2
@@ -1,5 +1,6 @@ import os import subprocess +import sys from git_remote_helpers.util import check_call, check_output
@@ -10,17 +11,26 @@ class GitImporter(object): This importer simply delegates to git fast-import. """ - def __init__(self, repo): + def __init__(self, repo, ref_encoding=None): """Creates a new importer for the specified repo. + + If ref_encoding is specified that refs are decoded using that + encoding. Otherwise the system filesystem encoding is used. """ self.repo = repo + self.ref_encoding = ref_encoding def get_refs(self, gitdir): """Returns a dictionary with refs. """ args = ["git", "--git-dir=" + gitdir, "for-each-ref", "refs/heads"] - lines = check_output(args).strip().split('\n') + encoding = self.ref_encoding + if encoding is None: + encoding = sys.getfilesystemencoding() + if encoding is None: + encoding = sys.getdefaultencoding() + lines = check_output(args).decode(encoding).strip().split('\n') refs = {} for line in lines: value, name = line.split(' ')
Signature · 1 line
1.8.1
Re: [RFC/PATCH 2/8 v2] git_remote_helpers: fix input when running under Python 3
Quoted 6 lines · John Keeping
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_helperswhere we are caught by this, which is when reading refs from "git for-each-ref".
Quoted 7 lines
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.
Quoted 2 lines
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?
Re: [RFC/PATCH 2/8 v2] git_remote_helpers: fix input when running under Python 3
Quoted 12 lines · Junio C Hamano
Quoted 11 lines · John Keeping
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_helperswhere 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.
Quoted 6 lines
Quoted 2 lines
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.
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]).
Quoted 8 lines
Quoted 2 lines
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?
As [1] says, there is a potential for bugs whenever people attempt to combine Unicode and byte strings. I think it also violates the principle of least surprise if a ref name (a string) doesn't behave like a normal string.
[1] http://docs.python.org/3.3/howto/unicode.html#tips-for-writing-unicode-aware-programs
John
Re: [RFC/PATCH 2/8 v2] git_remote_helpers: fix input when running under Python 3
Quoted 7 lines · John Keeping
Quoted 3 lines
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.
PATCH v3 2/8 git_remote_helpers: fix input when running under 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, which is when reading refs from "git for-each-ref".
Fix this by operating on the returned string as a byte string rather than a unicode string. As this method is currently only used internally by the class this does not affect code anywhere else.
Note that we cannot use byte strings in the source as the 'b' prefix is not supported before Python 2.7 so in order to maintain compatibility with the maximum range of Python versions we use an explicit call to encode().
---
Quoted 9 lines · Junio C Hamano
Quoted 7 lines · John Keeping
Quoted 3 lines
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]).
Quoted 8 lines
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.
Actually, this method is currently only used internally so I don't think my argument holds.
This is what keeping the refs as byte strings looks like.
git_remote_helpers/git/importer.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
git_remote_helpers/git/importer.py +6 -3
@@ -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
Signature · 1 line
1.8.1
Re: [RFC/PATCH 2/8 v3] git_remote_helpers: fix input when running under Python 3
[email protected] wrote on Tue, 15 Jan 2013 22:40 +0000:
Quoted 1 line
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.
Quoted 10 lines
git_remote_helpers/git/importer.py +3
-0@@ -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"]
git_remote_helpers/git/importer.py class GitImporter(object): · line 32
Quoted 10 lines
git_remote_helpers/git/importer.py +3
-3- 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.
-- Pete
Re: [RFC/PATCH 2/8 v3] git_remote_helpers: fix input when running under Python 3
Quoted 56 lines · Pete Wyckoff
[email protected] wrote on Tue, 15 Jan 2013 22:40 +0000:
Quoted 1 line
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.
Quoted 10 lines
git_remote_helpers/git/importer.py +3
-0@@ -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"]git_remote_helpers/git/importer.py class GitImporter(object): · line 32
Quoted 10 lines
git_remote_helpers/git/importer.py +3
-3- 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 refsI'd suggest for this Python conundrum using byte-string literals, e.g.:
Quoted 7 lines
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
John
Re: [RFC/PATCH 2/8 v3] git_remote_helpers: fix input when running under Python 3
[email protected] wrote on Wed, 16 Jan 2013 09:45 +0000:
Quoted 11 lines · Pete Wyckoff
Quoted 9 lines
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.
Quoted 7 lines
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
Drat. The b'' syntax seems to work on 2.6.8, in spite of the docs, but certainly isn't in 2.5.
I think you had hit on the best compromise with encoding, but maybe ascii is a little less presumptuous than utf-8, and more indicative of the encoding assumption.
-- Pete
PATCH 3/8 git_remote_helpers: Force rebuild if python version changes
In reply to John KeepingWhen different version of python are used to build via distutils, the behaviour can change. Detect changes in version and pass --force in this case.
--- git_remote_helpers/.gitignore | 1 + git_remote_helpers/Makefile | 8 +++++++- 2 files changed, 8 insertions(+), 1 deletion(-)
git_remote_helpers/.gitignore +1 -0
@@ -1,2 +1,3 @@ +/GIT-PYTHON_VERSION /build /dist
git_remote_helpers/Makefile +7 -1
@@ -25,8 +25,14 @@ PYLIBDIR=$(shell $(PYTHON_PATH) -c \ "import sys; \ print('lib/python%i.%i/site-packages' % sys.version_info[:2])") +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 install: $(pysetupfile) $(PYTHON_PATH) $(pysetupfile) install --prefix $(DESTDIR_SQ)$(prefix)
Signature · 1 line
1.8.1
Re: [PATCH 3/8] git_remote_helpers: Force rebuild if python version changes
[email protected] wrote on Sat, 12 Jan 2013 19:23 +0000:
Quoted 3 lines
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.
[..]
Quoted 1 line
git_remote_helpers/Makefile +0
-0
[..]
git_remote_helpers/Makefile PYLIBDIR=$(shell $(PYTHON_PATH) -c \ · line 35
Quoted 9 lines
git_remote_helpers/Makefile +7
-1+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.
-- Pete
Re: [PATCH 3/8] git_remote_helpers: Force rebuild if python version changes
Quoted 7 lines · Pete Wyckoff
[email protected] wrote on Sat, 12 Jan 2013 19:23 +0000:
Quoted 3 lines
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.
[..]
Quoted 1 line
git_remote_helpers/Makefile +0
-0[..]
Quoted 15 lines
git_remote_helpers/Makefile PYLIBDIR=$(shell $(PYTHON_PATH) -c \ · line 35
Quoted 9 lines
git_remote_helpers/Makefile +7
-1+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_VERSIONCan you depend on ../GIT-PYTHON-VARS instead? It comes from 96a4647 (
Makefile: detect whenPYTHON_PATHchanges, 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
Re: [PATCH 3/8] git_remote_helpers: Force rebuild if python version changes
[email protected] wrote on Sun, 13 Jan 2013 16:26 +0000:
Quoted 24 lines · Pete Wyckoff
Quoted 7 lines
[email protected] wrote on Sat, 12 Jan 2013 19:23 +0000:
Quoted 3 lines
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.
[..]
Quoted 1 line
git_remote_helpers/Makefile +0
-0[..]
Quoted 15 lines
git_remote_helpers/Makefile PYLIBDIR=$(shell $(PYTHON_PATH) -c \ · line 35
Quoted 9 lines
git_remote_helpers/Makefile +7
-1+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_VERSIONCan you depend on ../GIT-PYTHON-VARS instead? It comes from 96a4647 (
Makefile: detect whenPYTHON_PATHchanges, 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.
Quoted 5 lines
I was trying to keep the
git_remote_helpersdirectory 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 -Cgit_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.
-- Pete
Re: [PATCH 3/8] git_remote_helpers: Force rebuild if python version changes
Quoted 29 lines · Pete Wyckoff
[email protected] wrote on Sun, 13 Jan 2013 16:26 +0000:
Quoted 28 lines · Pete Wyckoff
Quoted 7 lines
[email protected] wrote on Sat, 12 Jan 2013 19:23 +0000:
Quoted 3 lines
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.
[..]
Quoted 1 line
git_remote_helpers/Makefile +0
-0[..]
Quoted 15 lines
git_remote_helpers/Makefile PYLIBDIR=$(shell $(PYTHON_PATH) -c \ · line 35
Quoted 9 lines
git_remote_helpers/Makefile +7
-1+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_VERSIONCan you depend on ../GIT-PYTHON-VARS instead? It comes from 96a4647 (
Makefile: detect whenPYTHON_PATHchanges, 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_helpersdirectory 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 -Cgit_remote_helpers" working in a clean tree.
Quoted 7 lines
Quoted 1 line
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.
John
Re: [PATCH 3/8] git_remote_helpers: Force rebuild if python version changes
Quoted 38 lines · John Keeping
Quoted 29 lines · Pete Wyckoff
[email protected] wrote on Sun, 13 Jan 2013 16:26 +0000:
Quoted 28 lines · Pete Wyckoff
Quoted 7 lines
[email protected] wrote on Sat, 12 Jan 2013 19:23 +0000:
Quoted 3 lines
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.
[..]
Quoted 1 line
git_remote_helpers/Makefile +0
-0[..]
Quoted 15 lines
git_remote_helpers/Makefile
Quoted 9 lines
git_remote_helpers/Makefile +7
-1+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_VERSIONCan you depend on ../GIT-PYTHON-VARS instead? It comes from 96a4647 (
Makefile: detect whenPYTHON_PATHchanges, 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_helpersdirectory 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 -Cgit_remote_helpers" working in a clean tree.Quoted 7 lines
Quoted 1 line
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.
Quoted 6 lines
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 --
git_remote_helpers/setup.py +19 -0
@@ -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',
Re: [PATCH 3/8] git_remote_helpers: Force rebuild if python version changes
[email protected] wrote on Tue, 15 Jan 2013 22:58 +0000:
Quoted 32 lines
For reference, putting the version check in
setup.pylooks like this:-- >8 --
git_remote_helpers/setup.py +12
-0@@ -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:
git_remote_helpers/setup.py except ImportError: · line 37
Quoted 10 lines
git_remote_helpers/setup.py +7
-0+ pass + +f = open('GIT-PYTHON_VERSION', 'w') +f.write(current_version) +f.close() + + setup( name = 'git_remote_helpers', version = '0.1.0',
That's about the same overhead as doing it in the Makefile, and a bit more obscure. I don't mind your initial version so much anymore. Thanks for thinking about it.
-- Pete
PATCH 4/8 git_remote_helpers: Use 2to3 if building with Python 3
In reply to John KeepingUsing the approach detailed on the Python wiki[1], run 2to3 on the code as part of the build if building with Python 3.
The code itself requires no changes to convert cleanly.
[1] http://wiki.python.org/moin/PortingPythonToPy3k
--- git_remote_helpers/setup.py | 10 ++++++++++ 1 file changed, 10 insertions(+)
git_remote_helpers/setup.py +10 -0
@@ -4,6 +4,15 @@ from distutils.core import setup +# 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 +# Python3. +try: + from distutils.command.build_py import build_py_2to3 as build_py +except ImportError: + # 2.x + from distutils.command.build_py import build_py + setup( name = 'git_remote_helpers', version = '0.1.0',
@@ -14,4 +23,5 @@ setup( url = 'http://www.git-scm.com/', package_dir = {'git_remote_helpers': ''}, packages = ['git_remote_helpers', 'git_remote_helpers.git'], + cmdclass = {'build_py': build_py}, )
Signature · 1 line
1.8.1
PATCH 5/8 svn-fe: allow svnrdump_sim.py to run with Python 3
In reply to John KeepingThe changes to allow this script to run with Python 3 are minimal and do not affect its functionality on the versions of Python 2 that are already supported (2.4 onwards).
--- contrib/svn-fe/svnrdump_sim.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
contrib/svn-fe/svnrdump_sim.py +2 -2
@@ -14,7 +14,7 @@ if sys.hexversion < 0x02040000: def getrevlimit(): var = 'SVNRMAX' - if os.environ.has_key(var): + if var in os.environ: return os.environ[var] return None
@@ -44,7 +44,7 @@ def writedump(url, lower, upper): if __name__ == "__main__": if not (len(sys.argv) in (3, 4, 5)): - print "usage: %s dump URL -rLOWER:UPPER" + print("usage: %s dump URL -rLOWER:UPPER") sys.exit(1) if not sys.argv[1] == 'dump': raise NotImplementedError('only "dump" is suppported.') url = sys.argv[2]
Signature · 1 line
1.8.1
PATCH 6/8 git-remote-testpy: hash bytes explicitly
In reply to John KeepingUnder 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.
--- git-remote-testpy.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
git-remote-testpy.py +4 -4
@@ -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(
Signature · 1 line
1.8.1
PATCH 7/8 git-remote-testpy: don't do unbuffered text I/O
In reply to John KeepingPython 3 forbids unbuffered I/O in text mode. Change the reading of stdin in git-remote-testpy so that we read the lines as bytes and then decode them a line at a time.
This allows us to keep the I/O unbuffered in order to avoid reintroducing the bug fixed by commit 7fb8e16 (git-remote-testgit: fix race when spawning fast-import).
--- git-remote-testpy.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
git-remote-testpy.py +3 -3
@@ -154,7 +154,7 @@ def do_import(repo, args): refs = [ref] while True: - line = sys.stdin.readline() + line = sys.stdin.readline().decode() if line == '\n': break if not line.startswith('import '):
@@ -217,7 +217,7 @@ def read_one_line(repo): line = sys.stdin.readline() - cmdline = line + cmdline = line.decode() if not cmdline: warn("Unexpected EOF")
@@ -269,7 +269,7 @@ def main(args): more = True - sys.stdin = os.fdopen(sys.stdin.fileno(), 'r', 0) + sys.stdin = os.fdopen(sys.stdin.fileno(), 'rb', 0) while (more): more = read_one_line(repo)
Signature · 1 line
1.8.1
PATCH 8/8 git-remote-testpy: call print as a function
In reply to John KeepingThis 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.
--- git-remote-testpy.py | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-)
git-remote-testpy.py +13 -13
@@ -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):
@@ -167,7 +167,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):
@@ -184,8 +184,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 = {
Signature · 1 line
1.8.1
v2 cover Initial Python 3 support
In reply to John KeepingThis series does enough so that everything except git-p4 runs under Python 3.
As discussed with Pete, it may not make sense to change git-p4 to support Python 3 until Perforce's Python output mode is changed. So does it make sense to merge this now and say "use Python 2 if you want git-p4"?
Changes since v1:
* rebased on master after fc/remote-testgit-feature-done was merged, leading to an extra change in patch 8 (git-remote-testpy: call print as a function)
* changed patch 2 (git_remote_helpers: fix input when running under Python 3) to treat ref names as byte strings
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 | 42 +++++++++++++++++++------------------- git_remote_helpers/.gitignore | 1 + git_remote_helpers/Makefile | 10 +++++++-- git_remote_helpers/git/importer.py | 9 +++++--- git_remote_helpers/setup.py | 10 +++++++++ 6 files changed, 48 insertions(+), 28 deletions(-)
Signature · 1 line
1.8.1.1.260.g99b33f4.dirty
PATCH v2 1/8 git_remote_helpers: allow building with Python 3
Change inline Python to call "print" as a function not a statement.
This is harmless because Python 2 will see the parentheses as redundant grouping but they are necessary to run this code with Python 3.
--- git_remote_helpers/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
git_remote_helpers/Makefile +1 -1
@@ -23,7 +23,7 @@ endif PYLIBDIR=$(shell $(PYTHON_PATH) -c \ "import sys; \ - print 'lib/python%i.%i/site-packages' % sys.version_info[:2]") + print('lib/python%i.%i/site-packages' % sys.version_info[:2])") all: $(pysetupfile) $(QUIET)$(PYTHON_PATH) $(pysetupfile) $(QUIETSETUP) build
Signature · 1 line
1.8.1.1.260.g99b33f4.dirty
PATCH v2 2/8 git_remote_helpers: fix input when running under Python 3
In reply to John KeepingAlthough 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".
Fix this by operating on the returned string as a byte string rather than a unicode string. As this method is currently only used internally by the class this does not affect code anywhere else.
Note that we cannot use byte strings in the source as the 'b' prefix is not supported before Python 2.7 so in order to maintain compatibility with the maximum range of Python versions we use an explicit call to encode().
--- git_remote_helpers/git/importer.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
git_remote_helpers/git/importer.py +6 -3
@@ -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('ascii')) refs = {} for line in lines: - value, name = line.split(' ') - name = name.strip('commit\t') + value, name = line.split(' '.encode('ascii')) + name = name.strip('commit\t'.encode('ascii')) refs[name] = value return refs
Signature · 1 line
1.8.1.1.260.g99b33f4.dirty
PATCH v2 3/8 git_remote_helpers: force rebuild if python version changes
In reply to John KeepingWhen different version of python are used to build via distutils, the behaviour can change. Detect changes in version and pass --force in this case.
--- git_remote_helpers/.gitignore | 1 + git_remote_helpers/Makefile | 8 +++++++- 2 files changed, 8 insertions(+), 1 deletion(-)
git_remote_helpers/.gitignore +1 -0
@@ -1,2 +1,3 @@ +/GIT-PYTHON_VERSION /build /dist
git_remote_helpers/Makefile +7 -1
@@ -25,8 +25,14 @@ PYLIBDIR=$(shell $(PYTHON_PATH) -c \ "import sys; \ print('lib/python%i.%i/site-packages' % sys.version_info[:2])") +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 install: $(pysetupfile) $(PYTHON_PATH) $(pysetupfile) install --prefix $(DESTDIR_SQ)$(prefix)
Signature · 1 line
1.8.1.1.260.g99b33f4.dirty
PATCH v2 4/8 git_remote_helpers: use 2to3 if building with Python 3
In reply to John KeepingUsing the approach detailed on the Python wiki[1], run 2to3 on the code as part of the build if building with Python 3.
The code itself requires no changes to convert cleanly.
[1] http://wiki.python.org/moin/PortingPythonToPy3k
--- git_remote_helpers/setup.py | 10 ++++++++++ 1 file changed, 10 insertions(+)
git_remote_helpers/setup.py +10 -0
@@ -4,6 +4,15 @@ from distutils.core import setup +# 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 +# Python3. +try: + from distutils.command.build_py import build_py_2to3 as build_py +except ImportError: + # 2.x + from distutils.command.build_py import build_py + setup( name = 'git_remote_helpers', version = '0.1.0',
@@ -14,4 +23,5 @@ setup( url = 'http://www.git-scm.com/', package_dir = {'git_remote_helpers': ''}, packages = ['git_remote_helpers', 'git_remote_helpers.git'], + cmdclass = {'build_py': build_py}, )
Signature · 1 line
1.8.1.1.260.g99b33f4.dirty
Re: [PATCH v2 4/8] git_remote_helpers: use 2to3 if building with Python 3
Quoted 1 line · John Keeping
This link seems dead.
--
Cheers,
Sverre Rabbelier
Re: [PATCH v2 4/8] git_remote_helpers: use 2to3 if building with Python 3
Quoted 4 lines · Sverre Rabbelier
Quoted 1 line · John Keeping
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
Sverre Rabbelier
Acked-byRe: [PATCH v2 4/8] git_remote_helpers: use 2to3 if building with Python 3
Assuming you tried this out on both 2.x and 3.x:
Quoted 16 lines · John Keeping
Quoted 4 lines · Sverre Rabbelier
Quoted 1 line · John Keeping
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-installationJohn
Signature · 3 lines
Cheers, Sverre Rabbelier
PATCH v2 5/8 svn-fe: allow svnrdump_sim.py to run with Python 3
In reply to John KeepingThe changes to allow this script to run with Python 3 are minimal and do not affect its functionality on the versions of Python 2 that are already supported (2.4 onwards).
--- contrib/svn-fe/svnrdump_sim.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
contrib/svn-fe/svnrdump_sim.py +2 -2
@@ -14,7 +14,7 @@ if sys.hexversion < 0x02040000: def getrevlimit(): var = 'SVNRMAX' - if os.environ.has_key(var): + if var in os.environ: return os.environ[var] return None
@@ -44,7 +44,7 @@ def writedump(url, lower, upper): if __name__ == "__main__": if not (len(sys.argv) in (3, 4, 5)): - print "usage: %s dump URL -rLOWER:UPPER" + print("usage: %s dump URL -rLOWER:UPPER") sys.exit(1) if not sys.argv[1] == 'dump': raise NotImplementedError('only "dump" is suppported.') url = sys.argv[2]
Signature · 1 line
1.8.1.1.260.g99b33f4.dirty
PATCH v2 6/8 git-remote-testpy: hash bytes explicitly
In reply to John KeepingUnder 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.
--- git-remote-testpy.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
git-remote-testpy.py +4 -4
@@ -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(
Signature · 1 line
1.8.1.1.260.g99b33f4.dirty
Re: [PATCH v2 6/8] git-remote-testpy: hash bytes explicitly
Quoted 8 lines · John Keeping
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 <[email protected]>---
Hmph. So what happens when the path is _not_ encoded in UTF-8?
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? 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?
Quoted 29 lines
git-remote-testpy.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)git-remote-testpy.py +4
-4@@ -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(
Re: [PATCH v2 6/8] git-remote-testpy: hash bytes explicitly
Quoted 6 lines · Junio C Hamano
Quoted 4 lines · John Keeping
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.
Quoted 6 lines
Quoted 4 lines
This moves the required Python version forward to 2.0.
Signed-off-by John Keeping <[email protected]>---
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.
Quoted 4 lines
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.
Quoted 6 lines
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?
You're right - I think we need to add ", errors='replace'" to the call to encode.
Quoted 29 lines
Quoted 29 lines
git-remote-testpy.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)git-remote-testpy.py +4
-4@@ -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(
Re: [PATCH v2 6/8] git-remote-testpy: hash bytes explicitly
Quoted 9 lines · John Keeping
Quoted 8 lines · Junio C Hamano
Quoted 6 lines · John Keeping
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.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
Signed-off-by John Keeping <[email protected]>---
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.
Now my brain's not working - we shouldn't get an error converting from a Unicode string to UTF-8, so I think this patch is OK as it is.
Quoted 9 lines
Quoted 4 lines
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.
Re: [PATCH v2 6/8] git-remote-testpy: hash bytes explicitly
In reply to John KeepingQuoted 2 lines · John Keeping
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?
Quoted 30 lines
Quoted 30 lines
Quoted 30 lines
git-remote-testpy.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)git-remote-testpy.py +4
-4@@ -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(
Re: [PATCH v2 6/8] git-remote-testpy: hash bytes explicitly
Quoted 7 lines · Junio C Hamano
Quoted 2 lines · John Keeping
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?
Quoted 29 lines
Quoted 29 lines
Quoted 29 lines
Quoted 29 lines
git-remote-testpy.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)git-remote-testpy.py +4
-4@@ -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(
Re: [PATCH v2 6/8] git-remote-testpy: hash bytes explicitly
Quoted 11 lines · John Keeping
Quoted 7 lines · Junio C Hamano
Quoted 2 lines · John Keeping
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?
If you go the .encode('hex') route, the log message needs to explain why the hashed values are now different from the old implementation and justify why it is safe to do so. I do not think I want to do that myself ;-).
Thanks.
Quoted 30 lines
Quoted 30 lines
Quoted 30 lines
Quoted 30 lines
Quoted 30 lines
git-remote-testpy.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)git-remote-testpy.py +4
-4@@ -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(
Re: [PATCH v2 6/8] git-remote-testpy: hash bytes explicitly
In reply to Junio C HamanoQuoted 6 lines · Junio C Hamano
Quoted 4 lines · John Keeping
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.
Quoted 6 lines
Quoted 4 lines
This moves the required Python version forward to 2.0.
Signed-off-by John Keeping <[email protected]>---
Hmph. So what happens when the path is _not_ encoded in UTF-8?
Oh, my brain was not working. Forget this part, and sorry for the noise. We are not decoding a bytestring to an array of unicode characters, but going the other way around here.
PATCH v2 7/8 git-remote-testpy: don't do unbuffered text I/O
In reply to John KeepingPython 3 forbids unbuffered I/O in text mode. Change the reading of stdin in git-remote-testpy so that we read the lines as bytes and then decode them a line at a time.
This allows us to keep the I/O unbuffered in order to avoid reintroducing the bug fixed by commit 7fb8e16 (git-remote-testgit: fix race when spawning fast-import).
--- git-remote-testpy.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
git-remote-testpy.py +3 -3
@@ -154,7 +154,7 @@ def do_import(repo, args): refs = [ref] while True: - line = sys.stdin.readline() + line = sys.stdin.readline().decode() if line == '\n': break if not line.startswith('import '):
@@ -225,7 +225,7 @@ def read_one_line(repo): line = sys.stdin.readline() - cmdline = line + cmdline = line.decode() if not cmdline: warn("Unexpected EOF")
@@ -277,7 +277,7 @@ def main(args): more = True - sys.stdin = os.fdopen(sys.stdin.fileno(), 'r', 0) + sys.stdin = os.fdopen(sys.stdin.fileno(), 'rb', 0) while (more): more = read_one_line(repo)
Signature · 1 line
1.8.1.1.260.g99b33f4.dirty
Re: [PATCH v2 7/8] git-remote-testpy: don't do unbuffered text I/O
git-remote-testpy.py def main(args): · line 280
Quoted 2 lines · John Keeping
git-remote-testpy.py +1
-1- sys.stdin = os.fdopen(sys.stdin.fileno(), 'r', 0) + sys.stdin = os.fdopen(sys.stdin.fileno(), 'rb', 0)
It is not immediately obvious why you would open stdin in rb mode, please add a comment.
--
Cheers,
Sverre Rabbelier
PATCH v2 8/8 git-remote-testpy: call print as a function
In reply to John KeepingThis 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.
--- git-remote-testpy.py | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-)
git-remote-testpy.py +14 -14
@@ -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 = {
Signature · 1 line
1.8.1.1.260.g99b33f4.dirty
Re: [PATCH v2 8/8] git-remote-testpy: call print as a function
Looks harmless enough.
Quoted 103 lines · John Keeping
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 <[email protected]>--- git-remote-testpy.py | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-)git-remote-testpy.py +14
-15@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 3 lines
Cheers, Sverre Rabbelier