{"thread":{"id":"13593","subject":"encoding bug in git.el","startedAt":"2008-05-20T22:09:00Z","lastAt":"2008-06-03T15:54:02Z","messageCount":10,"participants":["Karl Hasselström","Clifford Caoile","Junio C Hamano","David Christensen"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"77352","messageId":"20080520220900.GA20570@diana.vm.bytemark.co.uk","threadId":"13593","inReplyTo":null,"subject":"encoding bug in git.el","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-05-20T22:09:00Z","receivedAt":"2008-05-20T22:09:00Z","isPatch":false,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"Recently, some commits started misrecording the \"ö\" in my name. (In\nemacs, for example, it looks like this in a utf8 buffer:\nHasselstr\\201\\366m.) I'm guessing there's an extra latin1->utf8\nconversion in there somewhere.\n\nIt turns out that the breakage occurs when I commit with the\ngit-status mode from git.el, and it was introduced by this commit:\n\n commit dbe48256b41c1e94d81f2458d7e84b1fdcb47026\n Author: Clifford Caoile \n\n git.el: Set process-environment instead of invoking env\n\nIt's in master, but not yet in maint. (In fact, it's the _only_ change\nto contrib/emacs that's in master but not in maint.)\n\n-- \nKarl Hasselström, kha@treskal.com\n www.treskal.com/kalle\n"},{"id":"77386","messageId":"1f748ec60805210708q34a26bebh915037713caa9a87@mail.gmail.com","threadId":"13593","inReplyTo":"87mymkbo9x.fsf@lysator.liu.se","subject":"Re: encoding bug in git.el","fromName":"Clifford Caoile","fromEmail":"piyo@users.sourceforge.net","sentAt":"2008-05-21T14:08:09Z","receivedAt":"2008-05-21T14:08:09Z","isPatch":false,"sender":{"key":"piyo@users.sourceforge.net","avatar":null},"body":"Hi:\n\nOn Wed, May 21, 2008 at 7:31 AM, David Kågedal wrote:\n> Karl Hasselström writes:\n>\n>> Recently, some commits started misrecording the \"ö\" in my name. (In\n>> emacs, for example, it looks like this in a utf8 buffer:\n>> Hasselstr\\201\\366m.) I'm guessing there's an extra latin1->utf8\n>> conversion in there somewhere.\n>\n> The \\201 looks more like Emacs' internal mule encoding, where\n> everything that isn't ASCII is prefixed with \\201 or something\n> similar.\n\nThanks for reporting this.\n\nI concur. This is not UTF-8 translation, but an emacs MULE encoding. I\nsuspect the U+F6 character is read in to the *git-commit* buffer in\nlatin-1 mode because git.el displays the Author line, then Emacs\nwrites that out as 0x81F6, because that is the emacs buffer code of\nU+F6.\n\nThis is because git.el, upon git-commit-tree, always redefines the\nenvironment variables like GIT_AUTHOR_NAME. However the difference is\nthat prior to commit dbe482, \"env\" handle the encoding while commit\ndbe482 lets emacs process-environment handle it. Unfortunately the\nstring is passed without the proper recoding in the latter case.\n\nHere is a proposed fix. I suggest that process-environment should be\ngiven these envvars already encoded as shown in this code sample:\n\n------------------ git.el ------------------\n[not a proper git-diff]\n@@ -216,6 +216,11 @@ and `git-diff-setup-hook'.\"\n \"Build a list of NAME=VALUE strings from a list of environment strings.\"\n (mapcar (lambda (entry) (concat (car entry) \"=\" (cdr entry))) env))\n\n+(defun git-get-env-strings-encoded (env encoding)\n+ \"Build a list of NAME=VALUE strings from a list of environment strings,\n+converting from mule-encoding to ENCODING (e.g. mule-utf-8, latin-1, etc).\"\n+ (mapcar (lambda (entry) (concat (car entry) \"=\"\n(encode-coding-string (cdr entry) encoding))) env))\n+\n (defun git-call-process-env (buffer env &rest args)\n \"Wrapper for call-process that sets environment strings.\"\n (let ((process-environment (append (git-get-env-strings env)\n@@ -265,7 +270,7 @@ and returns the process output as a string, or nil\nif the git failed.\"\n\n (defun git-run-command-region (buffer start end env &rest args)\n \"Run a git command with specified buffer region as input.\"\n- (unless (eq 0 (let ((process-environment (append (git-get-env-strings env)\n+ (unless (eq 0 (let ((process-environment (append\n(git-get-env-strings-encoded env coding-system-for-write)\n process-environment)))\n (git-run-process-region\n buffer start end \"git\" args)))\n\nThe buffer text is saved with the encoding coding-system-for-write,\nwhile the GIT_* envvars were not encoded, so when appending to\nprocess-environment variable, use the same encoding.\n\n(Reminder: the *git-commit* buffer's encoding is based on the git\nconfig i18n.commitencoding, which in turn sets\nbuffer-file-coding-system, which in turn sets coding-system-for-write)\n\nI tested this with U+F6 in the GIT_AUTHOR_NAME, git config user.name,\nand the commit text, and it seems to work better (I think it's fixed).\nPlease review it. Also, I am not sure if this fix needs to be\npropagated to the other areas where process-environment is redefined,\nso YMMV.\n\n(Lastly, while testing this for Japanese, I'm having some encoding\nproblem with meadow (Emacs on Windows), msysgit (git on Windows),\nset-language-mode Japanese, utf-8, and M-x git-commit-file but I don't\nthink its related to this exact problem. Hopefully.)\n\n>> It turns out that the breakage occurs when I commit with the\n>> git-status mode from git.el, and it was introduced by this commit:\n>>\n>> commit dbe48256b41c1e94d81f2458d7e84b1fdcb47026\n>> Author: Clifford Caoile \n>>\n>> git.el: Set process-environment instead of invoking env\n\n:-)\n\nThis must be the reason why process-environment wasn't used in all places.\n\n>> It's in master, but not yet in maint. (In fact, it's the _only_ change\n>> to contrib/emacs that's in master but not in maint.)\n\nPlease forgive my ignorance, but what does this mean?\n\nBest regards,\nClifford Caoile\n"},{"id":"77391","messageId":"20080521145434.GA31982@diana.vm.bytemark.co.uk","threadId":"13593","inReplyTo":"1f748ec60805210708q34a26bebh915037713caa9a87@mail.gmail.com","subject":"Re: encoding bug in git.el","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-05-21T14:54:34Z","receivedAt":"2008-05-21T14:54:34Z","isPatch":false,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2008-05-21 23:08:09 +0900, Clifford Caoile wrote:\n\n> > > It's in master, but not yet in maint. (In fact, it's the _only_\n> > > change to contrib/emacs that's in master but not in maint.)\n>\n> Please forgive my ignorance, but what does this mean?\n\nThat the change was committed to the \"master\" branch, and not the\n\"maint\" branch. So folks who run stable releases haven't seen the bug\nyet.\n\n-- \nKarl Hasselström, kha@treskal.com\n www.treskal.com/kalle\n"},{"id":"77425","messageId":"1f748ec60805211431o38cdab16j722178c2416c53f9@mail.gmail.com","threadId":"13593","inReplyTo":"20080521145434.GA31982@diana.vm.bytemark.co.uk","subject":"Re: encoding bug in git.el","fromName":"Clifford Caoile","fromEmail":"piyo@users.sourceforge.net","sentAt":"2008-05-21T21:31:03Z","receivedAt":"2008-05-21T21:31:03Z","isPatch":false,"sender":{"key":"piyo@users.sourceforge.net","avatar":null},"body":"On Wed, May 21, 2008 at 11:54 PM, Karl Hasselström wrote:\n> On 2008-05-21 23:08:09 +0900, Clifford Caoile wrote:\n>\n>> > > It's in master, but not yet in maint. (In fact, it's the _only_\n>> > > change to contrib/emacs that's in master but not in maint.)\n>>\n>> Please forgive my ignorance, but what does this mean?\n>\n> That the change was committed to the \"master\" branch, and not the\n> \"maint\" branch. So folks who run stable releases haven't seen the bug\n> yet.\n\nOk I understand.\n\nDid you test the proposed fix I sent? I would like to know your feedback.\n\nBest regards,\nClifford Caoile\n"},{"id":"77530","messageId":"20080523070936.GB31315@diana.vm.bytemark.co.uk","threadId":"13593","inReplyTo":"1f748ec60805211431o38cdab16j722178c2416c53f9@mail.gmail.com","subject":"Re: encoding bug in git.el","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-05-23T07:09:36Z","receivedAt":"2008-05-23T07:09:36Z","isPatch":false,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2008-05-22 06:31:03 +0900, Clifford Caoile wrote:\n\n> Did you test the proposed fix I sent? I would like to know your\n> feedback.\n\nNo, sorry, I haven't taken the time to do so yet. Will try to do so\nthis weekend.\n\n-- \nKarl Hasselström, kha@treskal.com\n www.treskal.com/kalle\n"},{"id":"77704","messageId":"20080525134200.GA31990@diana.vm.bytemark.co.uk","threadId":"13593","inReplyTo":"1f748ec60805210708q34a26bebh915037713caa9a87@mail.gmail.com","subject":"Re: encoding bug in git.el","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-05-25T13:42:00Z","receivedAt":"2008-05-25T13:42:00Z","isPatch":false,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2008-05-21 23:08:09 +0900, Clifford Caoile wrote:\n\n> Here is a proposed fix. I suggest that process-environment should be\n> given these envvars already encoded as shown in this code sample:\n>\n> ------------------ git.el ------------------\n> [not a proper git-diff]\n> @@ -216,6 +216,11 @@ and `git-diff-setup-hook'.\"\n> \"Build a list of NAME=VALUE strings from a list of environment strings.\"\n> (mapcar (lambda (entry) (concat (car entry) \"=\" (cdr entry))) env))\n>\n> +(defun git-get-env-strings-encoded (env encoding)\n> + \"Build a list of NAME=VALUE strings from a list of environment strings,\n> +converting from mule-encoding to ENCODING (e.g. mule-utf-8, latin-1, etc).\"\n> + (mapcar (lambda (entry) (concat (car entry) \"=\"\n> (encode-coding-string (cdr entry) encoding))) env))\n> +\n> (defun git-call-process-env (buffer env &rest args)\n> \"Wrapper for call-process that sets environment strings.\"\n> (let ((process-environment (append (git-get-env-strings env)\n> @@ -265,7 +270,7 @@ and returns the process output as a string, or nil\n> if the git failed.\"\n>\n> (defun git-run-command-region (buffer start end env &rest args)\n> \"Run a git command with specified buffer region as input.\"\n> - (unless (eq 0 (let ((process-environment (append (git-get-env-strings env)\n> + (unless (eq 0 (let ((process-environment (append\n> (git-get-env-strings-encoded env coding-system-for-write)\n> process-environment)))\n> (git-run-process-region\n> buffer start end \"git\" args)))\n>\n> The buffer text is saved with the encoding coding-system-for-write,\n> while the GIT_* envvars were not encoded, so when appending to\n> process-environment variable, use the same encoding.\n\nI don't claim to understand any of the design issues around this, but\nyour patch certainly fixes my problem (once I managed to apply it,\nwhich involved working around the lack of headers, non-matching\noffsets, and whitespace damage -- luckily it was just two hunks). So:\n\nTested-by: Karl Hasselström \n\nThanks for taking the time.\n\n-- \nKarl Hasselström, kha@treskal.com\n www.treskal.com/kalle\n"},{"id":"78133","messageId":"20080530122826.GA4937@diana.vm.bytemark.co.uk","threadId":"13593","inReplyTo":"20080525134200.GA31990@diana.vm.bytemark.co.uk","subject":"Re: encoding bug in git.el","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-05-30T12:28:26Z","receivedAt":"2008-05-30T12:28:26Z","isPatch":false,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2008-05-25 15:42:00 +0200, Karl Hasselström wrote:\n\n> On 2008-05-21 23:08:09 +0900, Clifford Caoile wrote:\n>\n> > Here is a proposed fix.\n>\n> I don't claim to understand any of the design issues around this,\n> but your patch certainly fixes my problem (once I managed to apply\n> it, which involved working around the lack of headers, non-matching\n> offsets, and whitespace damage -- luckily it was just two hunks).\n> So:\n>\n> Tested-by: Karl Hasselström \n>\n> Thanks for taking the time.\n\nHow are things going with this fix? Junio, I expect you're waiting for\na properly cleaned-up patch, possibly with acks from relevant people?\n\nI think it would be a mistake to release 1.5.6 with this bug still in\nit; if not this bugfix, then a revert of the offending commit.\n\n-- \nKarl Hasselström, kha@treskal.com\n www.treskal.com/kalle\n"},{"id":"78169","messageId":"7vod6nk05c.fsf@gitster.siamese.dyndns.org","threadId":"13593","inReplyTo":"20080530122826.GA4937@diana.vm.bytemark.co.uk","subject":"Re: encoding bug in git.el","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-05-30T20:27:43Z","receivedAt":"2008-05-30T20:27:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karl Hasselström writes:\n\n> On 2008-05-25 15:42:00 +0200, Karl Hasselström wrote:\n>\n>> On 2008-05-21 23:08:09 +0900, Clifford Caoile wrote:\n>>\n>> > Here is a proposed fix.\n>>\n>> I don't claim to understand any of the design issues around this,\n>> but your patch certainly fixes my problem (once I managed to apply\n>> it, which involved working around the lack of headers, non-matching\n>> offsets, and whitespace damage -- luckily it was just two hunks).\n>> So:\n>>\n>> Tested-by: Karl Hasselström \n>>\n>> Thanks for taking the time.\n>\n> How are things going with this fix? Junio, I expect you're waiting for\n> a properly cleaned-up patch, possibly with acks from relevant people?\n\nYou expected correctly.\n"},{"id":"78429","messageId":"20080602223907.9612.84564.stgit@yoghurt","threadId":"13593","inReplyTo":"7vod6nk05c.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] Revert \"git.el: Set process-environment instead of invoking env\"","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-06-02T22:41:44Z","receivedAt":"2008-06-02T22:41:44Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"This reverts commit dbe48256b41c1e94d81f2458d7e84b1fdcb47026, which\ncaused mis-encoding of non-ASCII author/committer names when the\ngit-status mode is used to create commits.\n\nSigned-off-by: Karl Hasselström \n\n---\n\nOn 2008-05-30 13:27:43 -0700, Junio C Hamano wrote:\n\n> Karl Hasselström writes:\n> \n> > How are things going with this fix? Junio, I expect you're waiting\n> > for a properly cleaned-up patch, possibly with acks from relevant\n> > people?\n> \n> You expected correctly.\n\nIn case no one who understands how, why, and whether the fix works\ncomes forward, here's a revert of the commit that introduced the\nproblem.\n\n contrib/emacs/git.el | 11 +++++++----\n 1 files changed, 7 insertions(+), 4 deletions(-)\n\n\ndiff --git a/contrib/emacs/git.el b/contrib/emacs/git.el\nindex 2557a76..4fa853f 100644\n--- a/contrib/emacs/git.el\n+++ b/contrib/emacs/git.el\n@@ -232,8 +232,10 @@ and returns the process output as a string, or nil if the git failed.\"\n \n (defun git-run-command-region (buffer start end env &rest args)\n \"Run a git command with specified buffer region as input.\"\n- (unless (eq 0 (let ((process-environment (append (git-get-env-strings env)\n- process-environment)))\n+ (unless (eq 0 (if env\n+ (git-run-process-region\n+ buffer start end \"env\"\n+ (append (git-get-env-strings env) (list \"git\") args))\n (git-run-process-region\n buffer start end \"git\" args)))\n (error \"Failed to run \\\"git %s\\\":\\n%s\" (mapconcat (lambda (x) x) args \" \") (buffer-string))))\n@@ -248,8 +250,9 @@ and returns the process output as a string, or nil if the git failed.\"\n (erase-buffer)\n (cd dir)\n (setq status\n- (let ((process-environment (append (git-get-env-strings env)\n- process-environment)))\n+ (if env\n+ (apply #'call-process \"env\" nil (list buffer t) nil\n+ (append (git-get-env-strings env) (list hook-name) args))\n (apply #'call-process hook-name nil (list buffer t) nil args))))\n (display-message-or-buffer buffer)\n (eq 0 status)))))\n"},{"id":"78518","messageId":"7011F840-D6E2-4BB0-9538-601DA4DF54C3@endpoint.com","threadId":"13593","inReplyTo":"20080602223907.9612.84564.stgit@yoghurt","subject":"Re: [PATCH] Revert \"git.el: Set process-environment instead of invoking env\"","fromName":"David Christensen","fromEmail":"david@endpoint.com","sentAt":"2008-06-03T15:54:02Z","receivedAt":"2008-06-03T15:54:02Z","isPatch":true,"sender":{"key":"david@endpoint.com","avatar":"https://gravatar.com/avatar/6089b35cc409d9d15ab439753a213d7528cd5e0a04f3917fa452d8dc45296612?d=mp&s=160"},"body":"> This reverts commit dbe48256b41c1e94d81f2458d7e84b1fdcb47026, which\n> caused mis-encoding of non-ASCII author/committer names when the\n> git-status mode is used to create commits.\n>\n> Signed-off-by: Karl Hasselström \n>\n> ---\n>\n> On 2008-05-30 13:27:43 -0700, Junio C Hamano wrote:\n>\n>> Karl Hasselström writes:\n>>\n>>> How are things going with this fix? Junio, I expect you're waiting\n>>> for a properly cleaned-up patch, possibly with acks from relevant\n>>> people?\n>>\n>> You expected correctly.\n>\n> In case no one who understands how, why, and whether the fix works\n> comes forward, here's a revert of the commit that introduced the\n> problem.\n\nThis likely is due to the process-coding-system selected by emacs; \nthe correct functioning of this command will rely on both the current \nbuffer's coding system and the coding system of the data returned by \nthe invocation of git-status. In order for this to function \nproperly, these should match. Both of these are variables which can \nbe customized local to the buffer as part of the routine, so this \ncould be fixed if we are able to determine at invocation time what \ncoding system the git-status command will return in (presumably some \nform of utf-8, but I believe this is configurable per repo).\n\nI'd be glad to take a more in-depth look at this, but I'm not up on \nthe code at this point.\n\nRegards,\n\nDavid\n--\nDavid Christensen\nEnd Point Corporation\ndavid@endpoint.com\n"}]}