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

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

[Łukasz Stelmach]

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(-)

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

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

[Stefan Beller]

+cc git gui maintainer

On Mon, Aug 21, 2017 at 4:53 AM, Łukasz Stelmach <l.stelmach@samsung.com> wrote:
> 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(-)
>
> 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
>

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

[Łukasz Stelmach]

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

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

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1457 bytes --]

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

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

[Łukasz Stelmach]

[-- Attachment #1: Type: text/plain, Size: 1067 bytes --]

On Tue, 28 Nov 2017 15:35:21 +0100, Johannes Schindelin wrote:
> On Tue, 28 Nov 2017, Łukasz Stelmach wrote:

> > 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.

[-- Attachment #2: Type: application/pgp-signature, Size: 617 bytes --]

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

[Łukasz Stelmach]

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(-)

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

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2457 bytes --]

Hi Łukasz,

On Sat, 2 Dec 2017, Łukasz Stelmach wrote:

> 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).

> 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...

>  			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

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

[Łukasz Stelmach]

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(-)

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

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 536 bytes --]

Hi Łukasz,

On Tue, 5 Dec 2017, Łukasz Stelmach 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>

This patch looks good to me.

Thanks,
Johannes

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

[Łukasz Stelmach]

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(-)

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

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

[Eric Sunshine]

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/

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

[Łukasz Stelmach]

[-- Attachment #1: Type: text/plain, Size: 1255 bytes --]

It was <2017-12-14 czw 10:42>, when Eric Sunshine wrote:
> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 472 bytes --]

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

[Eric Sunshine]

On Thu, Dec 14, 2017 at 5:43 AM, Łukasz Stelmach <l.stelmach@samsung.com> wrote:
> 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.