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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Dec 4, 2017, 14:31 UTC
Message-ID: <alpine.DEB.2.21.1.1712041521330.98586@virtualbox>
In-Reply-To: <20171202122046.6033-1-l.stelmach@samsung.com>

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