Re: [PATCH v5 5/5] pretty: user format ignores i18n.logOutputEncoding setting

[Junio C Hamano <gitster@pobox.com>]

Alexey Shumkin <Alex.Crezoff@gmail.com> writes:

Subject: Re: [PATCH v5 5/5] pretty: user format ignores i18n.logOutputEncoding setting

That is a statement of fact, and does not tell much to the reader.

I think you are saying that in the current implementation,
logoutputencoding is not honored in user format, that it is a bug
that needs to be fixed (as opposed to "that is by design, the
scripts that read from --format='' is responsible for doing the
conversion"), and that this patch fixes it.

So

	pretty: --format output should honor logOutputEncoding

or something?  At least it says what is the _desired_ outcome with
"should", hints that the status-quo is different from that desired
outcome and implies that this is a patch to fix it.

The "Subject" line is very important as that is the only thing we
see in many summarizing output format, e.g. shortlog, cover letter
and merge message.

> The following two commands are expected to give the same output to a terminal:
>
> 	$ git log --oneline --no-color
> 	$ git log --pretty=format:'%h %s'
>
> However, the former pays attention to i18n.logOutputEncoding
> configuration, while the latter does not when it format "%s".
> Log messages written in an encoding i18n.commitEncoding which differs
> from terminal encoding are shown corrupted with the latter even
> when i18n.logOutputEncoding and terminal encoding are the same.
>
> The same corruption is true for
> 	$ git diff --submodule=log
> and
> 	$ git rev-list --pretty=format:%s HEAD
> and
> 	$ git reset --hard
>
> Signed-off-by: Alexey Shumkin <Alex.Crezoff@gmail.com>
> ---
>  builtin/reset.c                  |  8 ++++++--
>  builtin/rev-list.c               |  1 +
>  builtin/shortlog.c               |  1 +
>  log-tree.c                       |  1 +
>  submodule.c                      |  3 +++
>  t/t4041-diff-submodule-option.sh | 10 +++++-----
>  t/t4205-log-pretty-formats.sh    | 34 +++++++++++++++++-----------------
>  t/t6006-rev-list-format.sh       |  8 ++++----
>  t/t7102-reset.sh                 |  2 +-
>  9 files changed, 39 insertions(+), 29 deletions(-)
>
> diff --git a/builtin/reset.c b/builtin/reset.c
> index 6032131..b23ed63 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -92,11 +92,15 @@ static int reset_index(const unsigned char *sha1, int reset_type, int quiet)
>  
>  static void print_new_head_line(struct commit *commit)
>  {
> -	const char *hex, *body;
> +	const char *hex, *body, *encoding;
>  
>  	hex = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);
>  	printf(_("HEAD is now at %s"), hex);
> -	body = strstr(commit->buffer, "\n\n");
> +	encoding = get_log_output_encoding();
> +	body = logmsg_reencode(commit, NULL, encoding);


> +	if (!body)
> +		body = commit->buffer;

Does this happen?  I thought body, without an error, can be the same
as commit->buffer.

> +	body = strstr(body, "\n\n");
>  	if (body) {
>  		const char *eol;
>  		size_t len;

Do we have a leak here?  body may point at a new piece of memory
logmsg_reencode() have allocated to hold the UTF-8 version of your
8859-5 message in commit->buffer.

It would be more like this, no?

diff --git a/builtin/reset.c b/builtin/reset.c
index 6032131..8d22ffe 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -92,11 +92,13 @@ static int reset_index(const unsigned char *sha1, int reset_type, int quiet)
 
 static void print_new_head_line(struct commit *commit)
 {
-	const char *hex, *body;
+	const char *hex, *body, *msg, *encoding; 
 
 	hex = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);
 	printf(_("HEAD is now at %s"), hex);
-	body = strstr(commit->buffer, "\n\n");
+
+	msg = logmsg_reencode(commit, NULL, get_log_output_encoding());
+	body = strstr(msg, "\n\n");
 	if (body) {
 		const char *eol;
 		size_t len;
@@ -107,6 +109,7 @@ static void print_new_head_line(struct commit *commit)
 	}
 	else
 		printf("\n");
+	logmsg_free(msg, commit);
 }
 
 static void update_index_from_diff(struct diff_queue_struct *q,