Re: [PATCHv5 4/8] pretty.c: teach format_commit_message() to reencode the output

[Pat Notz <patnotz@gmail.com>]

On Wed, Oct 13, 2010 at 3:59 PM, Junio C Hamano <gitster@pobox.com> wrote:
> "Pat Notz" <patnotz@gmail.com> writes:
>
>> diff --git a/pretty.c b/pretty.c
>> index a607fd6..e5ce7fb 100644
>> --- a/pretty.c
>> +++ b/pretty.c
>> @@ -1009,16 +1009,47 @@ 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;
>> +     char *buffer;
>> +     char *enc_buffer;
>> +     struct strbuf scratch_sb = STRBUF_INIT;
>> +     struct strbuf *sb_ptr;
>> +
>> +     enc = get_header(commit, "encoding");
>> +     enc = enc ? enc : utf8;
>> +     if(output_encoding && strcmp(enc,output_encoding)) {
>> +             sb_ptr = &scratch_sb;
>> +     } else {
>> +             sb_ptr = sb;
>> +     }
>
> Style...
>
>>       memset(&context, 0, sizeof(context));
>>       context.commit = commit;
>>       context.pretty_ctx = pretty_ctx;
>>       context.wrap_start = sb->len;
>> -     strbuf_expand(sb, format, format_commit_item, &context);
>> -     rewrap_message_tail(sb, &context, 0, 0, 0);
>> +     strbuf_expand(sb_ptr, format, format_commit_item, &context);
>> +     rewrap_message_tail(sb_ptr, &context, 0, 0, 0);
>> +
>> +     if(sb_ptr != sb) {
>> +             /* if re-encoding fails, take the content byte-for-byte */
>> +             buffer = strbuf_detach(sb_ptr, 0);
>> +             enc_buffer = reencode_string(buffer, output_encoding, enc);
>> +             enc_buffer = enc_buffer ? enc_buffer : buffer;
>> +
>> +             strbuf_addstr(sb,enc_buffer);
>> +
>> +             if(enc_buffer != buffer)
>> +                     free(enc_buffer);
>> +             free(buffer);
>> +     }
>> +
>> +     if(enc != utf8)
>> +             free(enc);
>>  }
>
> You are expanding and wrapping commit->buf before re-encoding, but I am
> not sure it is the right thing to do.  Wouldn't it become much simpler and
> more consistent if you re-encode first and then give the result to later
> expansion and wrapping process?  format_commit_one() would need to take
> "msg" not from c->commit->buffer but from a new field to hold reencoded
> result you will add in your patch to the structure, if you do so, of
> course.
>
> Besides, I am a bit lost as to what this patch has to do with the stated
> goal of the series, "Add commit message options for rebase --autosquash".

Thank you for the reviews; I'll digest your comments shortly. In
response to your final comment, this patch was in response to your
earlier suggestion[1] to teach format_commit_message() to deal with
potentially different encodings.  I went for your option #3 there but
perhaps it's overkill.  I still think that's the right thing to do
despite my implementation.  Otherwise, this routine is just adding
bits to the buffer without regard for consistent encoding.  My goal
was to introduce this separately from any new features.

Again, thanks for all your comments -- I'll try to improve the series.

[1] http://thread.gmane.org/gmane.comp.version-control.git/156883/focus=156891