"Pat Notz" <patnotz@xxxxxxxxx> writes:
> @@ -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% ;-)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html