{"thread":{"id":"40349","subject":"[PATCH v7] git-p4: add config git-p4.pathEncoding","startedAt":"2015-09-14T17:10:39Z","lastAt":"2015-09-15T15:59:16Z","messageCount":8,"participants":["larsxschneider@gmail.com","Junio C Hamano","Luke Diamand","Lars Schneider"],"isPatch":true,"patchVersion":7,"patchTotal":null},"messages":[{"id":"269937","messageId":"1442250640-93838-1-git-send-email-larsxschneider@gmail.com","threadId":"40349","inReplyTo":null,"subject":"[PATCH v7] git-p4: add config git-p4.pathEncoding","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2015-09-14T17:10:39Z","receivedAt":"2015-09-14T17:10:39Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\ndiff to v6:\n* Always print the encoded path in verbose mode.\n\nv6 is already on next (a9e383). This patch must be applied on next.\nIs this the right way to handle this situation?\n\nThanks,\nLars\n\nLars Schneider (1):\n git-p4: improve path encoding verbose output\n\n git-p4.py | 19 +++++++++----------\n 1 file changed, 9 insertions(+), 10 deletions(-)\n\n--\n2.5.1\n"},{"id":"269938","messageId":"1442250640-93838-2-git-send-email-larsxschneider@gmail.com","threadId":"40349","inReplyTo":"1442250640-93838-1-git-send-email-larsxschneider@gmail.com","subject":"[PATCH v7] git-p4: improve path encoding verbose output","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2015-09-14T17:10:40Z","receivedAt":"2015-09-14T17:10:40Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\nIf a path with non-ASCII characters is detected then print always the\nencoding and the encoded string in verbose mode.\n\nSigned-off-by: Lars Schneider \n---\n git-p4.py | 19 +++++++++----------\n 1 file changed, 9 insertions(+), 10 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex d45cf2b..da25d3f 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -2220,16 +2220,15 @@ class P4Sync(Command, P4UserMap):\n text = regexp.sub(r'$\\1$', text)\n contents = [ text ]\n \n- if gitConfig(\"git-p4.pathEncoding\"):\n- relPath = relPath.decode(gitConfig(\"git-p4.pathEncoding\")).encode('utf8', 'replace')\n- elif self.verbose:\n- try:\n- relPath.decode('ascii')\n- except:\n- print (\n- \"Path with Non-ASCII characters detected and no path encoding defined. \"\n- \"Please check the encoding: %s\" % relPath\n- )\n+ try:\n+ relPath.decode('ascii')\n+ except:\n+ encoding = 'utf8'\n+ if gitConfig('git-p4.pathEncoding'):\n+ encoding = gitConfig('git-p4.pathEncoding')\n+ relPath = relPath.decode(encoding).encode('utf8', 'replace')\n+ if self.verbose:\n+ print 'Path with non-ASCII characters detected. Used %s to encode: %s ' % (encoding, relPath)\n \n self.gitStream.write(\"M %s inline %s\\n\" % (git_mode, relPath))\n \n-- \n2.5.1\n"},{"id":"269954","messageId":"xmqqfv2gx3gv.fsf@gitster.mtv.corp.google.com","threadId":"40349","inReplyTo":"1442250640-93838-1-git-send-email-larsxschneider@gmail.com","subject":"Re: [PATCH v7] git-p4: add config git-p4.pathEncoding","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-14T18:25:52Z","receivedAt":"2015-09-14T18:25:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"larsxschneider@gmail.com writes:\n\n> From: Lars Schneider \n>\n> diff to v6:\n> * Always print the encoded path in verbose mode.\n>\n> v6 is already on next (a9e383). This patch must be applied on next.\n> Is this the right way to handle this situation?\n\nYes, I thought that the reviewers found v6 was solid enough, and it\nis now in 'next'. It is best to do further polishing on the tip of\nthe topic that is in 'next', i.e. a9e38359 (git-p4: add config\ngit-p4.pathEncoding, 2015-09-03).\n\nLabeling the patch as v7 is misleading, but from the context (like\nyour cover letter description) it was clear that the patch is a\nfollow-up, so everything looks good from procedural point of view.\n\nThanks for making reviewer's life easier.\n"},{"id":"269955","messageId":"xmqqbnd4x2se.fsf@gitster.mtv.corp.google.com","threadId":"40349","inReplyTo":"1442250640-93838-2-git-send-email-larsxschneider@gmail.com","subject":"Re: [PATCH v7] git-p4: improve path encoding verbose output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-14T18:40:33Z","receivedAt":"2015-09-14T18:40:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"larsxschneider@gmail.com writes:\n\n> From: Lars Schneider \n>\n> If a path with non-ASCII characters is detected then print always the\n> encoding and the encoded string in verbose mode.\n\nEarlier if the user tells us that s/he knows what she is doing\nby setting the configuration, we just followed the instruction\nwithout complaining or notifying. The differences in this version\nare\n\n (1) if the path is in ASCII, the configuration is not even\n consulted, and we didn't do any path munging.\n\n (2) for a non-ASCII path, even if the user tells us that s/he knows\n what she is doing, we notify what we did under \"--verbose\"\n mode.\n\nI think (1) is a definite improvement, but it is not immediately\nobvious why (2) is an improvement. It is clearly a good thing to\nlet the user know when we munged the path without being told, but\nwhen the configuration is given, it can be argued both ways. It may\nbe a good thing to reassure that the configuration is kicking in, or\nit may be a needless noise to tell the user that we did what we were\ntold to do.\n\nIn any case, I suspectq that the call to decode-encode to munge\nrelPath is indented one level too deep in this patch. You would\nwant to use the configured value if exists and utf8 if there is no\nconfiguration, but in either case you would want to munge relPath\nwhen it does not decode as ASCII, no?\n\n> Signed-off-by: Lars Schneider \n> ---\n> git-p4.py | 19 +++++++++----------\n> 1 file changed, 9 insertions(+), 10 deletions(-)\n>\n> diff --git a/git-p4.py b/git-p4.py\n> index d45cf2b..da25d3f 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -2220,16 +2220,15 @@ class P4Sync(Command, P4UserMap):\n> text = regexp.sub(r'$\\1$', text)\n> contents = [ text ]\n> \n> - if gitConfig(\"git-p4.pathEncoding\"):\n> - relPath = relPath.decode(gitConfig(\"git-p4.pathEncoding\")).encode('utf8', 'replace')\n> - elif self.verbose:\n> - try:\n> - relPath.decode('ascii')\n> - except:\n> - print (\n> - \"Path with Non-ASCII characters detected and no path encoding defined. \"\n> - \"Please check the encoding: %s\" % relPath\n> - )\n> + try:\n> + relPath.decode('ascii')\n> + except:\n> + encoding = 'utf8'\n> + if gitConfig('git-p4.pathEncoding'):\n> + encoding = gitConfig('git-p4.pathEncoding')\n> + relPath = relPath.decode(encoding).encode('utf8', 'replace')\n> + if self.verbose:\n> + print 'Path with non-ASCII characters detected. Used %s to encode: %s ' % (encoding, relPath)\n> \n> self.gitStream.write(\"M %s inline %s\\n\" % (git_mode, relPath))\n"},{"id":"269987","messageId":"55F7C93D.1080309@diamand.org","threadId":"40349","inReplyTo":"1442250640-93838-2-git-send-email-larsxschneider@gmail.com","subject":"Re: [PATCH v7] git-p4: improve path encoding verbose output","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-09-15T07:31:09Z","receivedAt":"2015-09-15T07:31:09Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On 14/09/15 18:10, larsxschneider@gmail.com wrote:\n> From: Lars Schneider \n>\n> If a path with non-ASCII characters is detected then print always the\n\ns/print always/print/\n\n\n> encoding and the encoded string in verbose mode.\n>\n> Signed-off-by: Lars Schneider \n> ---\n> git-p4.py | 19 +++++++++----------\n> 1 file changed, 9 insertions(+), 10 deletions(-)\n>\n> diff --git a/git-p4.py b/git-p4.py\n> index d45cf2b..da25d3f 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -2220,16 +2220,15 @@ class P4Sync(Command, P4UserMap):\n> text = regexp.sub(r'$\\1$', text)\n> contents = [ text ]\n>\n> - if gitConfig(\"git-p4.pathEncoding\"):\n> - relPath = relPath.decode(gitConfig(\"git-p4.pathEncoding\")).encode('utf8', 'replace')\n> - elif self.verbose:\n> - try:\n> - relPath.decode('ascii')\n> - except:\n> - print (\n> - \"Path with Non-ASCII characters detected and no path encoding defined. \"\n> - \"Please check the encoding: %s\" % relPath\n> - )\n> + try:\n> + relPath.decode('ascii')\n> + except:\n> + encoding = 'utf8'\n> + if gitConfig('git-p4.pathEncoding'):\n> + encoding = gitConfig('git-p4.pathEncoding')\n\nIt would be better to query this once at startup. Otherwise we're \npotentially forking \"git config\" twice per file which on a large repo \ncould become significant. Make it an instance variable perhaps?\n\n> + relPath = relPath.decode(encoding).encode('utf8', 'replace')\n> + if self.verbose:\n> + print 'Path with non-ASCII characters detected. Used %s to encode: %s ' % (encoding, relPath)\n>\n> self.gitStream.write(\"M %s inline %s\\n\" % (git_mode, relPath))\n>\n>\n"},{"id":"269989","messageId":"55F7CA71.1020203@diamand.org","threadId":"40349","inReplyTo":"55F7C93D.1080309@diamand.org","subject":"Re: [PATCH v7] git-p4: improve path encoding verbose output","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-09-15T07:36:17Z","receivedAt":"2015-09-15T07:36:17Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On 15/09/15 08:31, Luke Diamand wrote:\n> On 14/09/15 18:10, larsxschneider@gmail.com wrote:\n>\n> It would be better to query this once at startup. Otherwise we're\n> potentially forking \"git config\" twice per file which on a large repo\n> could become significant. Make it an instance variable perhaps?\n\nThis is of course complete nonsense since gitConfig caches its results!\n"},{"id":"270037","messageId":"1E65031B-DE14-4B8A-9105-9EA0E6279573@gmail.com","threadId":"40349","inReplyTo":"xmqqbnd4x2se.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v7] git-p4: improve path encoding verbose output","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2015-09-15T15:55:40Z","receivedAt":"2015-09-15T15:55:40Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\nOn 14 Sep 2015, at 20:40, Junio C Hamano wrote:\n\n> larsxschneider@gmail.com writes:\n> \n>> From: Lars Schneider \n>> \n>> If a path with non-ASCII characters is detected then print always the\n>> encoding and the encoded string in verbose mode.\n> \n> Earlier if the user tells us that s/he knows what she is doing\n> by setting the configuration, we just followed the instruction\n> without complaining or notifying. The differences in this version\n> are\n> \n> (1) if the path is in ASCII, the configuration is not even\n> consulted, and we didn't do any path munging.\nCorrect!\n\n\n> (2) for a non-ASCII path, even if the user tells us that s/he knows\n> what she is doing, we notify what we did under \"--verbose\"\n> mode.\nCorrect!\n\n\n> I think (1) is a definite improvement, but it is not immediately\n> obvious why (2) is an improvement. It is clearly a good thing to\n> let the user know when we munged the path without being told, but\n> when the configuration is given, it can be argued both ways. It may\n> be a good thing to reassure that the configuration is kicking in, or\n> it may be a needless noise to tell the user that we did what we were\n> told to do.\nI get your point. However, changing file names in a repository is a pretty significant action and therefore I would prefer to explicitly tell the user about it. Some encodings differ only slightly I would like to have an easy way to look at all the changed paths to ensure I picked the right encoding (e.g. grep “Path with non-ASCII characters detected”). I also assume the user is OK with noise since s/he enabled “verbose” mode :-)\n\n\n> In any case, I suspectq that the call to decode-encode to munge\n> relPath is indented one level too deep in this patch. You would\n> want to use the configured value if exists and utf8 if there is no\n> configuration, but in either case you would want to munge relPath\n> when it does not decode as ASCII, no?\nGood catch! It works with the indented code too because UTF8 is the default encoding for relPath later on. However, with your suggestion the code is more explicit. I will change it in the next roll\n\nThanks!\n\n> \n>> Signed-off-by: Lars Schneider \n>> ---\n>> git-p4.py | 19 +++++++++----------\n>> 1 file changed, 9 insertions(+), 10 deletions(-)\n>> \n>> diff --git a/git-p4.py b/git-p4.py\n>> index d45cf2b..da25d3f 100755\n>> --- a/git-p4.py\n>> +++ b/git-p4.py\n>> @@ -2220,16 +2220,15 @@ class P4Sync(Command, P4UserMap):\n>> text = regexp.sub(r'$\\1$', text)\n>> contents = [ text ]\n>> \n>> - if gitConfig(\"git-p4.pathEncoding\"):\n>> - relPath = relPath.decode(gitConfig(\"git-p4.pathEncoding\")).encode('utf8', 'replace')\n>> - elif self.verbose:\n>> - try:\n>> - relPath.decode('ascii')\n>> - except:\n>> - print (\n>> - \"Path with Non-ASCII characters detected and no path encoding defined. \"\n>> - \"Please check the encoding: %s\" % relPath\n>> - )\n>> + try:\n>> + relPath.decode('ascii')\n>> + except:\n>> + encoding = 'utf8'\n>> + if gitConfig('git-p4.pathEncoding'):\n>> + encoding = gitConfig('git-p4.pathEncoding')\n>> + relPath = relPath.decode(encoding).encode('utf8', 'replace')\n>> + if self.verbose:\n>> + print 'Path with non-ASCII characters detected. Used %s to encode: %s ' % (encoding, relPath)\n>> \n>> self.gitStream.write(\"M %s inline %s\\n\" % (git_mode, relPath))\n"},{"id":"270044","messageId":"46B8F08C-F4A4-43D0-9798-D663F1C5F4BA@gmail.com","threadId":"40349","inReplyTo":"55F7C93D.1080309@diamand.org","subject":"Re: [PATCH v7] git-p4: improve path encoding verbose output","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2015-09-15T15:59:16Z","receivedAt":"2015-09-15T15:59:16Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\nOn 15 Sep 2015, at 09:31, Luke Diamand wrote:\n\n> On 14/09/15 18:10, larsxschneider@gmail.com wrote:\n>> From: Lars Schneider \n>> \n>> If a path with non-ASCII characters is detected then print always the\n> \n> s/print always/print/\nI will fix it.\n\n> \n> \n>> encoding and the encoded string in verbose mode.\n>> \n>> Signed-off-by: Lars Schneider \n>> ---\n>> git-p4.py | 19 +++++++++----------\n>> 1 file changed, 9 insertions(+), 10 deletions(-)\n>> \n>> diff --git a/git-p4.py b/git-p4.py\n>> index d45cf2b..da25d3f 100755\n>> --- a/git-p4.py\n>> +++ b/git-p4.py\n>> @@ -2220,16 +2220,15 @@ class P4Sync(Command, P4UserMap):\n>> text = regexp.sub(r'$\\1$', text)\n>> contents = [ text ]\n>> \n>> - if gitConfig(\"git-p4.pathEncoding\"):\n>> - relPath = relPath.decode(gitConfig(\"git-p4.pathEncoding\")).encode('utf8', 'replace')\n>> - elif self.verbose:\n>> - try:\n>> - relPath.decode('ascii')\n>> - except:\n>> - print (\n>> - \"Path with Non-ASCII characters detected and no path encoding defined. \"\n>> - \"Please check the encoding: %s\" % relPath\n>> - )\n>> + try:\n>> + relPath.decode('ascii')\n>> + except:\n>> + encoding = 'utf8'\n>> + if gitConfig('git-p4.pathEncoding'):\n>> + encoding = gitConfig('git-p4.pathEncoding')\n> \n> It would be better to query this once at startup. Otherwise we're potentially forking \"git config\" twice per file which on a large repo could become significant. Make it an instance variable perhaps?\nsolved in other email\n\n> \n>> + relPath = relPath.decode(encoding).encode('utf8', 'replace')\n>> + if self.verbose:\n>> + print 'Path with non-ASCII characters detected. Used %s to encode: %s ' % (encoding, relPath)\n>> \n>> self.gitStream.write(\"M %s inline %s\\n\" % (git_mode, relPath))\n\nThanks!"}]}