Re: [PATCHv6 2/6] pretty.c: teach format_commit_message() to reencode the output

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 27, 2010, 22:35 UTC
Message-ID: <7vvd4nb6wt.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1287689637-95301-3-git-send-email-patnotz@gmail.com>

"Pat Notz" <patnotz@gmail.com> writes:

Show 29 quoted lines

> @@ -1008,16 +1009,29 @@ void userformat_find_requirements(const char *fmt, struct userformat_want *w)
>  
>  void format_commit_message(const struct commit *commit,
>  			   const char *format, struct strbuf *sb,
> -			   const struct pretty_print_context *pretty_ctx)
> +			   const struct pretty_print_context *pretty_ctx,
> +			   const char *output_encoding)
>  {
>  	struct format_commit_context context;
> +	static char utf8[] = "UTF-8";
> +	char *enc;
> +
> +	enc = get_header(commit, "encoding");
> +	enc = enc ? enc : utf8;
>  
>  	memset(&context, 0, sizeof(context));
>  	context.commit = commit;
>  	context.pretty_ctx = pretty_ctx;
>  	context.wrap_start = sb->len;
> +	if (output_encoding && strcmp(enc, output_encoding))
> +		context.message = logmsg_reencode(commit, output_encoding);
> +	context.message = context.message ? context.message : commit->buffer;
> +
>  	strbuf_expand(sb, format, format_commit_item, &context);
>  	rewrap_message_tail(sb, &context, 0, 0, 0);
> +
> +	if (context.message != commit->buffer)
> +		free(context.message);
>  }

Three points.

 - Most of the callers give NULL to the output_encoding. Does it make
   sense to limit get_header(commit, "encoding") call only when the
   argument is given?

 - The conditional assignment to context.message with ?: is hard to read;
   perhaps it would be easier to read if you structure it like this:

	memset(&context, 0, sizeof(context));
        context.commit = ...;
        ...
	context.message = commit->buffer;
	if (output_encoding) {
        	enc = ...
                if (strcmp(enc, output_encoding))
                	context.message = ...
	}

 - Should output_encoding be a separate argument to this function?  If
   anybody is going to call this function with the same pretty_ctx with
   different output_encoding, your patch may make sense, but I suspect
   adding it as a new member to pretty_print_context structure may be much
   cleaner.  I would imagine that it would cut this patch down by 70% ;-)