Re: Possible regression in git-rev-list --header

From: Junio C Hamano <junkio@cox.net>
Date: Dec 30, 2006, 22:19 UTC
Message-ID: <7vlkkphvrb.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <7v7iw9jftv.fsf@assigned-by-dhcp.cox.net>

Junio C Hamano <junkio@cox.net> writes:

Show 19 quoted lines

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> On Sat, 30 Dec 2006, Marco Costalba wrote:
>>
>>> When 'commitencoding' variable is set in config file then git-rev-list
>>> called with --header option sends also the encoding information.
>>
>> As Jakub pointed out, qgit should not expect to know all headers. I am 
>> very sorry, since I said I looked at all parsers of the commit header in 
>> git, but that was _only_ git, and no porcelains.
>>
>> Please fix qgit, since I really consider this a bug.
>
> I have to agree with Johannes.  In principle Porcelains should
> be prepared to see and ignore unknown headers.
>
> However, this commit created by `commit-tree` certalinly can be
> improved.
> ...

Another thing. I think it would make sense to remove "encoding" header after pretty_print_commit successfully re-codes the buffer. An alternative is to rewrite "encoding" header to show which encoding the log now uses (and omit it if it is UTF-8).

The attached patch does the latter, but I think removing the header altogether in this case would make it easier to use overall. If you want to, you can change

	if (is_encoding_utf8(encoding))

in the patch with "if (1)" (the above macro is what was introduced in my previous patch).

Pro for having "encoding" rewritten as this patch does is that the output is still self-identifying. You can tell what encoding you are supposed to interpret the log messages in. Con against it is that the output is after re-coding as the user asked (either with the config or --encoding=<encoding> option), and the extra header becomes redundant information.

-- >8 --

 commit.c |   44 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 44 insertions(+), 0 deletions(-)

diff --git a/commit.c b/commit.c
index e13b9cb..bf82fe8 100644
--- a/commit.c
+++ b/commit.c
@@ -624,6 +624,48 @@ static char *get_header(const struct commit *commit, const char *key)
 	}
 }
 
+static char *replace_encoding_header(char *buf, char *encoding)
+{
+	char *encoding_header = strstr(buf, "\nencoding ");
+	char *end_of_encoding_header;
+	int encoding_header_pos;
+	int encoding_header_len;
+	int new_len;
+	int need_len;
+	int buflen = strlen(buf) + 1;
+
+	if (!encoding_header)
+		return buf; /* should not happen but be defensive */
+	encoding_header++;
+	end_of_encoding_header = strchr(encoding_header, '\n');
+	if (!end_of_encoding_header)
+		return buf; /* should not happen but be defensive */
+	end_of_encoding_header++;
+
+	encoding_header_len = end_of_encoding_header - encoding_header;
+	encoding_header_pos = encoding_header - buf;
+
+	if (is_encoding_utf8(encoding)) {
+		/* we have re-coded to UTF-8; drop the header */
+		memmove(encoding_header, end_of_encoding_header,
+			buflen - (encoding_header_pos + encoding_header_len));
+		return buf;
+	}
+	new_len = strlen(encoding);
+	need_len = new_len + strlen("encoding \n");
+	if (encoding_header_len < need_len) {
+		buf = xrealloc(buf, buflen + (need_len - encoding_header_len));
+		encoding_header = buf + encoding_header_pos;
+		end_of_encoding_header = encoding_header + encoding_header_len;
+	}
+	memmove(end_of_encoding_header + (need_len - encoding_header_len),
+		end_of_encoding_header,
+		buflen - (encoding_header_pos + encoding_header_len));
+	memcpy(encoding_header + 9, encoding, strlen(encoding));
+	encoding_header[9 + new_len] = '\n';
+	return buf;
+}
+
 static char *logmsg_reencode(const struct commit *commit)
 {
 	char *encoding;
@@ -642,6 +684,8 @@ static char *logmsg_reencode(const struct commit *commit)
 		return NULL;
 	}
 	out = reencode_string(commit->buffer, output_encoding, encoding);
+	out = replace_encoding_header(out, output_encoding);
+
 	free(encoding);
 	if (!out)
 		return NULL;