Change Details

the poop emoji a.k.a. U+1F4A9 will apparently break phabricator when submitting a diff as seen with D7472. the diff actually submitted was this {F77492} and copied here for easy reading (except the <U+###> are really unicode chars): ``` diff --git a/jenkins/phabricator-post.py b/jenkins/phabricator-post.py index 284ecc0..b5d822c 100755 --- a/jenkins/phabricator-post.py +++ b/jenkins/phabricator-post.py @@ -1,4 +1,6 @@ #!/usr/bin/env python +# -*- coding: utf-8 -*- + import json import os import sys @@ -43,9 +45,9 @@ if __name__ == '__main__': print "D%s failed tests on diff id %s" % (differential_id, diff_id) if int(num_failures) == 0: - message = "Jenkins is unable to run tests with this patch. See %s to see why." % console_output_url + message = u"<U+26D4>️ Jenkins is unable to run tests with this patch. See %s to see why. <U+1F4A9>" % console_output_url else: - message = "This patch causes %s tests to fail! See %s to see which tests are failing." % (num_failures, test_report_url) + message = u"<U+26D4>️ This patch causes %s tests to fail! See the failing build here: %s <U+1F4A9>" % (num_failures, test_report_url) #action = "reject" action = "none" @@ -53,7 +55,7 @@ if __name__ == '__main__': elif result == "SUCCESS": print "D%s passed all tests with diff id %s" % (differential_id, diff_id) - message = "This patch does not break any tests. +1. See the passing build here: %s" % test_report_url + message = u"<U+2705> This patch does not break any tests. +1. See the passing build here: %s" % test_report_url action = "none" # This comment should not email everyone. silent = True ```

Currently, we store diffs as UTF-8. This is a lovely idea, but diffs are not UTF-8, and they also aren't UTF-8 with only BMP characters, which is what we actually are able to store. This causes various problems which we'd be better off dealing with at a higher level than we do. Particularly, this means `arc patch` can never work for some set of diffs, which is probably not a reasonable behavior. We should store diffs as binary and mangle encodings at display time (hopefully with some level of caching so this doesn't destroy performance). The major issue with this is that it implies an enormous migration for all existing installs.

the poop emoji a.k.a.Currently, we store diffs as UTF-8. This is a lovely idea, but diffs are not UTF-8, and they also aren't UTF-8 with only BMP characters, which is what we actually are able to store. This causes various problems which we'd be better off dealing with at a higher level than we do. Particularly, this means `arc patch` can never work for some set of diffs, U+1F4A9 will apparently break phabricator when submitting a diff as seen with D7472which is probably not a reasonable behavior. the diff actually submitted was this {F77492} and copied here for easy reading (except the <U+###> are really unicode chars):We should store diffs as binary and mangle encodings at display time (hopefully with some level of caching so this doesn't destroy performance). ``` diff --git a/jenkins/phabricator-post.py b/jenkins/phabricator-post.py index 284ecc0..b5d822c 100755 --- a/jenkins/phabricator-post.py +++ b/jenkins/phabricator-post.py @@ -1,4 +1,6 @@ #!/usr/bin/env python +# -*- coding: utf-8 -*- + import json import os import sys @@ -43,9 +45,9 @@ if __name__ == '__main__': print "D%s failed tests on diff id %s" % (differential_id, diff_id) if int(num_failures) == 0: - message = "Jenkins is unable to run tests with this patch. See %s to see why." % console_output_url + message = u"<U+26D4>️ Jenkins is unable to run tests with this patch. See %s to see why. <U+1F4A9>" % console_output_url else: - message = "This patch causes %s tests to fail! See %s to see which tests are failing." % (num_failures, test_report_url) + message = u"<U+26D4>️ This patch causes %s tests to fail! See the failing build here: %s <U+1F4A9>" % (num_failures, test_report_url) #action = "reject" action = "none" @@ -53,7 +55,7 @@ if __name__ == '__main__': elif result == "SUCCESS": print "D%s passed all tests with diff id %s" % (differential_id, diff_id) - message = "This patch does not break any tests. +1. See the passing build here: %s" % test_report_url + message = u"<U+2705> This patch does not break any tests. +1. See the passing build here: %s" % test_report_url action = "none" # This comment should not email everyone. silent = True ```The major issue with this is that it implies an enormous migration for all existing installs.