Re: [PATCH v2 02/12] pretty: share code between format_decoration and show_decorations

>> -void show_decorations(struct rev_info *opt, struct commit *commit)
>> +void format_decoration(struct strbuf *sb,
>> +                    const struct commit *commit,
>> +                    int use_color)
>>  {
>> -     const char *prefix;
>> -     struct name_decoration *decoration;
>> +     const char *prefix = " (";
>> +     struct name_decoration *d;
>
> This renaming of variable from decoration to d seems to make the
> patched result unnecessarily different from the original in
> show_decorations, making it harder to compare.  Intentional?

>>       const char *color_commit =
>> -             diff_get_color_opt(&opt->diffopt, DIFF_COMMIT);
>> +             diff_get_color(use_color, DIFF_COMMIT);
>>       const char *color_reset =
>> -             decorate_get_color_opt(&opt->diffopt, DECORATION_NONE);
>> +             decorate_get_color(use_color, DECORATION_NONE);
>> +
>> +     load_ref_decorations(DECORATE_SHORT_REFS);
>
> In cmd_log_init_finish(), we have loaded decorations with specified
> decoration_style already.  Why is this needed (and with a hardcoded
> style that may be different from what the user specified)?

>
>> +     d = lookup_decoration(&name_decoration, &commit->object);
>> +     if (!d)
>> +             return;
>> +     while (d) {
>> +             strbuf_addstr(sb, color_commit);
>> +             strbuf_addstr(sb, prefix);
>> +             strbuf_addstr(sb, decorate_get_color(use_color, d->type));
>> +             if (d->type == DECORATION_REF_TAG)
>> +                     strbuf_addstr(sb, "tag: ");
>> +             strbuf_addstr(sb, d->name);
>> +             strbuf_addstr(sb, color_reset);
>> +             prefix = ", ";
>> +             d = d->next;
>> +     }
>> +     if (prefix[0] == ',') {
>> +             strbuf_addstr(sb, color_commit);
>> +             strbuf_addch(sb, ')');
>> +             strbuf_addstr(sb, color_reset);
>> +     }
>
> Was this change to conditionally close ' (' mentioned in the log
> message?  It is in line with the version taken from pretty.c, and I
> think it may be an improvement, but I do not think the check is
> necessary in the context of this function.  You will never see
> prefix[0] != ',' after the loop, because "while (d)" above runs at
> least once; otherwise the "if (!d) return" would have returned from
> the function early, no?

>> @@ -625,8 +639,8 @@ void show_log(struct rev_info *opt)
>>                       printf(" (from %s)",
>>                              find_unique_abbrev(parent->object.sha1,
>>                                                 abbrev_commit));
>> +             fputs(diff_get_color_opt(&opt->diffopt, DIFF_RESET), stdout);
>>               show_decorations(opt, commit);
>> -             printf("%s", diff_get_color_opt(&opt->diffopt, DIFF_RESET));
>
> We used to show and then reset.  I can see the updated
> show_decorations() to format_decoration() callchain always reset at
> the end, so the loss of the final reset here is very sane, but is
> there a need to reset beforehand?  What is the calling convention
> for the updated show_decorations()?  The caller should make sure
> there is no funny colors in effect before calling, and the caller
> can rest assured that there is no funny colors when the function
> returns?

>> +void format_decoration(struct strbuf *sb,
>> +                    const struct commit *commit,
>> +                    int use_color);
>
> I think you can fit these on a single line, especially if you drop
> the unused variable names (they help when there are more than one
> parameter of the same type to document the order of the arguments,
> but that does not apply here).  That would help people who run
> "grep" on the header files without using CTAGS/ETAGS.