Sorry for this late reply. I've been quite busy lately..
On Tue, Apr 2, 2013 at 4:53 AM, Junio C Hamano <[email protected]> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
