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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Sorry for this late reply. I've been quite busy lately..

On Tue, Apr 2, 2013 at 4:53 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>> -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?

I think I just happened to reuse the style of the old
format_decoration(). Will reuse the name "decoration" instead.

>>       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)?

legacy from pretty.c:format_decoration(). Will move it to the caller
format_commit_one.

>
>> +     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?

Yes, your eyeballs have really good quality ;)

>> @@ -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?

I think it's a sane convention, unless we want a some background color
going through show_decorations.

>> +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.

No problem.

> Wouldn't it be "format_decorations()", or does it handle only one?

All in one, apparently. Will rename.
--
Duy
--
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




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]