threads / patch / 47331

patchgit-gui: Prevent double UTF-8 conversion

Subject: [PATCH] git-gui: Prevent double UTF-8 conversion

## tl;dr

11 messages between Nov 28, 2017 and Dec 14, 2017. Diffs are folded; open one to read it.

replies: 10people: 4as markdown or json

Łukasz Stelmach· Nov 28, 2017, 10:21 UTC · lore

Set encoding to utf-8 for file descriptors used to receive data from git commands.

With encoding on the file descriptor set to "binary" Tcl (8.6 in my case) does double conversion which breaks e.g. author name in amended commits.

For example "\305\201ukasz" (as written by git cat-file) becomes "\303\205\302\201ukasz".

Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
---
 git-gui/lib/commit.tcl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

So, this is the second attempt to draw some attention to this patch, since the first one has somewhat failed.

https://marc.info/?l=git&m=150331641702091&w=2
Show changes to git-gui/lib/commit.tcl +1 −1
diff --git a/git-gui/lib/commit.tcl b/git-gui/lib/commit.tcl
index 83620b7cb..bcb6499a0 100644
--- a/git-gui/lib/commit.tcl
+++ b/git-gui/lib/commit.tcl
@@ -26,7 +26,7 @@ You are currently in the middle of a merge that has not been fully completed.  Y
 	set parents [list]
 	if {[catch {
 			set fd [git_read cat-file commit $curHEAD]
-			fconfigure $fd -encoding binary -translation lf
+			fconfigure $fd -encoding utf-8 -translation lf
 			# By default commits are assumed to be in utf-8
 			set enc utf-8
 			while {[gets $fd line] > 0} {

-- 
2.11.0
Johannes Schindelin· Nov 28, 2017, 14:35 UTC · re: Łukasz Stelmach · lore

Re: [PATCH] git-gui: Prevent double UTF-8 conversion

Hi Łukasz,
On Tue, 28 Nov 2017, Łukasz Stelmach wrote:
> Set encoding to utf-8 for file descriptors used to receive data from git
> commands.

The patch only changes it for the `cat-file` command used to read the latest commit message for amending.

Maybe the commit message should be adjusted to reflect as much?
> With encoding on the file descriptor set to "binary" Tcl (8.6 in my
> case) does double conversion which breaks e.g. author name in amended
> commits.

Is the problem in question occurring when a commit message contains non-ASCII characters encoded in UTF-8 and you click the "Amend Last Commit" radio button on the right side above the "Commit Message" text box?

I tested this with Tcl/Tk 8.6.7-1 of Git for Windows' SDK, and with Tcl/Tk 8.6.0+9 of Ubuntu 16.04.3 LTS, in both cases the encoding of a commit message 'äöü' was handled correctly with "binary"...

Could you give more details about your system so it is easier for me to understand where the difference comes from?

Having said that, I also tested it *with* your patch, and it does not break anything.

> So, this is the second attempt to draw some attention to this patch,
> since the first one has somewhat failed.

Yeah, sadly Pat (who is still listed as the active Git GUI maintainer) has fallen very, very silent for over a year.

Pinging about the status of the patch was definitely the right thing to do.

Ciao, Johannes

Łukasz Stelmach· Dec 2, 2017, 10:33 UTC · re: Johannes Schindelin · lore

Re: [PATCH] git-gui: Prevent double UTF-8 conversion

On Tue, 28 Nov 2017 15:35:21 +0100, Johannes Schindelin wrote:
> On Tue, 28 Nov 2017, Łukasz Stelmach wrote:
Show 8 quoted lines
> > With encoding on the file descriptor set to "binary" Tcl (8.6 in my
> > case) does double conversion which breaks e.g. author name in amended
> > commits.
>
> Is the problem in question occurring when a commit message contains
> non-ASCII characters encoded in UTF-8 and you click the "Amend Last
> Commit" radio button on the right side above the "Commit Message" text
> box?

No, only with the author data. That means there is a difference in how Tcl handles input in with "read" (the message) and "gets" (headers including the author header).

Indeed! A few lines below the message is converted from utf-8 and author is not. So the right thing to do is to convert author too and not change the file descriptor settings.

I am going back to the drawing board. Thanks. 
-- 
Było mi bardzo miło.                                  --- Rurku. --- ...
>Łukasz<                                --- To dobrze, że mnie słuchasz.
Łukasz Stelmach· Dec 2, 2017, 12:20 UTC · re: Łukasz Stelmach · lore

[PATCH v2] git-gui: Prevent double UTF-8 conversion

Convert author's name from the UTF-8 (or any other) encoding in load_last_commit function the same way commit message is converted.

Amending commits in git-gui without such conversion breaks UTF-8 strings. For example, "\305\201ukasz" (as written by git cat-file) becomes "\303\205\302\201ukasz" in an amended commit.

Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
---
 git-gui/lib/commit.tcl | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
Show changes to git-gui/lib/commit.tcl +3 −3
diff --git a/git-gui/lib/commit.tcl b/git-gui/lib/commit.tcl
index 83620b7cb..f820c24bf 100644
--- a/git-gui/lib/commit.tcl
+++ b/git-gui/lib/commit.tcl
@@ -34,9 +34,7 @@ You are currently in the middle of a merge that has not been fully completed.  Y
 					lappend parents [string range $line 7 end]
 				} elseif {[string match {encoding *} $line]} {
 					set enc [string tolower [string range $line 9 end]]
-				} elseif {[regexp "author (.*)\\s<(.*)>\\s(\\d.*$)" $line all name email time]} {
-					set commit_author [list name $name email $email date $time]
-				}
+				} elseif {[regexp "author (.*)\\s<(.*)>\\s(\\d.*$)" $line all name email time]} { }
 			}
 			set msg [read $fd]
 			close $fd
@@ -44,7 +42,9 @@ You are currently in the middle of a merge that has not been fully completed.  Y
 			set enc [tcl_encoding $enc]
 			if {$enc ne {}} {
 				set msg [encoding convertfrom $enc $msg]
+				set name [encoding convertfrom $enc $name]
 			}
+			set commit_author [list name $name email $email date $time]
 			set msg [string trim $msg]
 		} err]} {
 		error_popup [strcat [mc "Error loading commit data for amend:"] "\n\n$err"]

-- 
2.11.0
Johannes Schindelin· Dec 4, 2017, 14:31 UTC · re: Łukasz Stelmach · lore

Re: [PATCH v2] git-gui: Prevent double UTF-8 conversion

Hi Łukasz,
On Sat, 2 Dec 2017, Łukasz Stelmach wrote:
Show 6 quoted lines
> Convert author's name from the UTF-8 (or any other) encoding in
> load_last_commit function the same way commit message is converted.
> 
> Amending commits in git-gui without such conversion breaks UTF-8
> strings. For example, "\305\201ukasz" (as written by git cat-file) becomes
> "\303\205\302\201ukasz" in an amended commit.

Okay, that makes the issue a lot clearer to me (the explicit mention of "author's name", that is).

Show 13 quoted lines
> diff --git a/git-gui/lib/commit.tcl b/git-gui/lib/commit.tcl
> index 83620b7cb..f820c24bf 100644
> --- a/git-gui/lib/commit.tcl
> +++ b/git-gui/lib/commit.tcl
> @@ -34,9 +34,7 @@ You are currently in the middle of a merge that has not been fully completed.  Y
>  					lappend parents [string range $line 7 end]
>  				} elseif {[string match {encoding *} $line]} {
>  					set enc [string tolower [string range $line 9 end]]
> -				} elseif {[regexp "author (.*)\\s<(.*)>\\s(\\d.*$)" $line all name email time]} {
> -					set commit_author [list name $name email $email date $time]
> -				}
> +				} elseif {[regexp "author (.*)\\s<(.*)>\\s(\\d.*$)" $line all name email time]} { }
>  			}

This looks wrong, as the commit_author would now also be set if the header was not found (mind you, this would make for an incorrect Git commit, but the code explicitly tries to set commit_author only in the case that the author line was found.

But we cannot set commit_author here because the encoding is read as another header line (and in a valid commit object, the encoding line (if any) has to be *below* the author line).

So it *has* to be this way. Maybe mention this in the commit message, to avoid head-scratching?

However, I would still recommend to `set name ""` before the loop parsing the header, and...

Show 9 quoted lines
>  			set msg [read $fd]
>  			close $fd
> @@ -44,7 +42,9 @@ You are currently in the middle of a merge that has not been fully completed.  Y
>  			set enc [tcl_encoding $enc]
>  			if {$enc ne {}} {
>  				set msg [encoding convertfrom $enc $msg]
> +				set name [encoding convertfrom $enc $name]
>  			}
> +			set commit_author [list name $name email $email date $time]
Guarding this assignment in an `if {$name ne ""} { ... }`, just in case.
>  			set msg [string trim $msg]
>  		} err]} {
>  		error_popup [strcat [mc "Error loading commit data for amend:"] "\n\n$err"]

Thanks, Johannes

Łukasz Stelmach· Dec 5, 2017, 14:23 UTC · re: Łukasz Stelmach · lore

[PATCH v3] git-gui: Prevent double UTF-8 conversion

Convert author's name and e-mail address from the UTF-8 (or any other) encoding in load_last_commit function the same way commit message is converted.

Amending commits in git-gui without such conversion breaks UTF-8 strings. For example, "\305\201ukasz" (as written by git cat-file) becomes "\303\205\302\201ukasz" in an amended commit.

Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
---
 git-gui/lib/commit.tcl | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)
Show changes to git-gui/lib/commit.tcl +9 −3
diff --git a/git-gui/lib/commit.tcl b/git-gui/lib/commit.tcl
index 83620b7cb..75ea965da 100644
--- a/git-gui/lib/commit.tcl
+++ b/git-gui/lib/commit.tcl
@@ -25,6 +25,8 @@ You are currently in the middle of a merge that has not been fully completed.  Y
 	set msg {}
 	set parents [list]
 	if {[catch {
+			set name ""
+			set email ""
 			set fd [git_read cat-file commit $curHEAD]
 			fconfigure $fd -encoding binary -translation lf
 			# By default commits are assumed to be in utf-8
@@ -34,9 +36,7 @@ You are currently in the middle of a merge that has not been fully completed.  Y
 					lappend parents [string range $line 7 end]
 				} elseif {[string match {encoding *} $line]} {
 					set enc [string tolower [string range $line 9 end]]
-				} elseif {[regexp "author (.*)\\s<(.*)>\\s(\\d.*$)" $line all name email time]} {
-					set commit_author [list name $name email $email date $time]
-				}
+				} elseif {[regexp "author (.*)\\s<(.*)>\\s(\\d.*$)" $line all name email time]} { }
 			}
 			set msg [read $fd]
 			close $fd
@@ -44,7 +44,13 @@ You are currently in the middle of a merge that has not been fully completed.  Y
 			set enc [tcl_encoding $enc]
 			if {$enc ne {}} {
 				set msg [encoding convertfrom $enc $msg]
+				set name [encoding convertfrom $enc $name]
+				set email [encoding convertfrom $enc $email]
 			}
+			if {$name ne {} && $email ne {}} {
+				set commit_author [list name $name email $email date $time]
+			}
+
 			set msg [string trim $msg]
 		} err]} {
 		error_popup [strcat [mc "Error loading commit data for amend:"] "\n\n$err"]

-- 
2.11.0
Johannes Schindelin· Dec 7, 2017, 00:06 UTC · re: Łukasz Stelmach · lore

Re: [PATCH v3] git-gui: Prevent double UTF-8 conversion

Hi Łukasz,
On Tue, 5 Dec 2017, Łukasz Stelmach wrote:
Show 9 quoted lines
> Convert author's name and e-mail address from the UTF-8 (or any other)
> encoding in load_last_commit function the same way commit message is
> converted.
> 
> Amending commits in git-gui without such conversion breaks UTF-8
> strings. For example, "\305\201ukasz" (as written by git cat-file) becomes
> "\303\205\302\201ukasz" in an amended commit.
> 
> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
This patch looks good to me.

Thanks, Johannes

Łukasz Stelmach· Dec 14, 2017, 09:32 UTC · re: Łukasz Stelmach · lore

[PATCH v4] git-gui: Prevent double UTF-8 conversion

Convert author's name and e-mail address from the UTF-8 (or any other) encoding in load_last_commit function the same way commit message is converted.

Amending commits in git-gui without such conversion breaks UTF-8 strings. For example, "\305\201ukasz" (as written by git cat-file) becomes "\303\205\302\201ukasz" in an amended commit.

Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
Reviewed-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
Changes since v3:
- Added Reviewed-by footer. Thank you Johannes Schindelin, for review.
 git-gui/lib/commit.tcl | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)
Show changes to git-gui/lib/commit.tcl +9 −3
diff --git a/git-gui/lib/commit.tcl b/git-gui/lib/commit.tcl
index 83620b7cb..75ea965da 100644
--- a/git-gui/lib/commit.tcl
+++ b/git-gui/lib/commit.tcl
@@ -25,6 +25,8 @@ You are currently in the middle of a merge that has not been fully completed.  Y
 	set msg {}
 	set parents [list]
 	if {[catch {
+			set name ""
+			set email ""
 			set fd [git_read cat-file commit $curHEAD]
 			fconfigure $fd -encoding binary -translation lf
 			# By default commits are assumed to be in utf-8
@@ -34,9 +36,7 @@ You are currently in the middle of a merge that has not been fully completed.  Y
 					lappend parents [string range $line 7 end]
 				} elseif {[string match {encoding *} $line]} {
 					set enc [string tolower [string range $line 9 end]]
-				} elseif {[regexp "author (.*)\\s<(.*)>\\s(\\d.*$)" $line all name email time]} {
-					set commit_author [list name $name email $email date $time]
-				}
+				} elseif {[regexp "author (.*)\\s<(.*)>\\s(\\d.*$)" $line all name email time]} { }
 			}
 			set msg [read $fd]
 			close $fd
@@ -44,7 +44,13 @@ You are currently in the middle of a merge that has not been fully completed.  Y
 			set enc [tcl_encoding $enc]
 			if {$enc ne {}} {
 				set msg [encoding convertfrom $enc $msg]
+				set name [encoding convertfrom $enc $name]
+				set email [encoding convertfrom $enc $email]
 			}
+			if {$name ne {} && $email ne {}} {
+				set commit_author [list name $name email $email date $time]
+			}
+
 			set msg [string trim $msg]
 		} err]} {
 		error_popup [strcat [mc "Error loading commit data for amend:"] "\n\n$err"]

-- 
2.11.0
Eric Sunshine· Dec 14, 2017, 09:42 UTC · re: Łukasz Stelmach · lore

Re: [PATCH v4] git-gui: Prevent double UTF-8 conversion

On Thu, Dec 14, 2017 at 4:32 AM, Łukasz Stelmach <l.stelmach@samsung.com> wrote:
Show 14 quoted lines
> Convert author's name and e-mail address from the UTF-8 (or any other)
> encoding in load_last_commit function the same way commit message is
> converted.
>
> Amending commits in git-gui without such conversion breaks UTF-8
> strings. For example, "\305\201ukasz" (as written by git cat-file) becomes
> "\303\205\302\201ukasz" in an amended commit.
>
> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
> Reviewed-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> ---
> Changes since v3:
>
> - Added Reviewed-by footer. Thank you Johannes Schindelin, for review.

No need to re-send. If you consult Junio's latest "What's cooking"[1], you'll find that your patch has already graduated from his 'pu' branch to 'next' and is slated to graduate to 'master' (at some point).

[1]: https://public-inbox.org/git/xmqqzi6mutcc.fsf@gitster.mtv.corp.google.com/
Łukasz Stelmach· Dec 14, 2017, 10:43 UTC · re: Eric Sunshine · lore

Re: [PATCH v4] git-gui: Prevent double UTF-8 conversion

It was <2017-12-14 czw 10:42>, when Eric Sunshine wrote:
Show 21 quoted lines
> On Thu, Dec 14, 2017 at 4:32 AM, Łukasz Stelmach <l.stelmach@samsung.com> wrote:
>> Convert author's name and e-mail address from the UTF-8 (or any other)
>> encoding in load_last_commit function the same way commit message is
>> converted.
>>
>> Amending commits in git-gui without such conversion breaks UTF-8
>> strings. For example, "\305\201ukasz" (as written by git cat-file) becomes
>> "\303\205\302\201ukasz" in an amended commit.
>>
>> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
>> Reviewed-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
>> ---
>> Changes since v3:
>>
>> - Added Reviewed-by footer. Thank you Johannes Schindelin, for review.
>
> No need to re-send. If you consult Junio's latest "What's cooking"[1],
> you'll find that your patch has already graduated from his 'pu' branch
> to 'next' and is slated to graduate to 'master' (at some point).
>
> [1]: https://public-inbox.org/git/xmqqzi6mutcc.fsf@gitster.mtv.corp.google.com/

I am sorry. I didn't get any notifiaction by mail and I haven't studied Documentation/SubmittingPatches throughly enough.

-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics
Eric Sunshine· Dec 14, 2017, 18:18 UTC · re: Łukasz Stelmach · lore

Re: [PATCH v4] git-gui: Prevent double UTF-8 conversion

On Thu, Dec 14, 2017 at 5:43 AM, Łukasz Stelmach <l.stelmach@samsung.com> wrote:
Show 9 quoted lines
> It was <2017-12-14 czw 10:42>, when Eric Sunshine wrote:
>> No need to re-send. If you consult Junio's latest "What's cooking"[1],
>> you'll find that your patch has already graduated from his 'pu' branch
>> to 'next' and is slated to graduate to 'master' (at some point).
>>
>> [1]: https://public-inbox.org/git/xmqqzi6mutcc.fsf@gitster.mtv.corp.google.com/
>
> I am sorry. I didn't get any notifiaction by mail and I haven't studied
> Documentation/SubmittingPatches throughly enough.

No need for an apology. My response was just a simple informational message to help you (and perhaps other newcomers reading this thread) get up to speed.

← back to recent threads