Re: [PATCH v2 02/12] pretty: share code between format_decoration and show_decorations
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 1, 2013, 17:53 UTC
- Message-ID
- <7vd2uejfxr.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <1364636112-15065-3-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
Show 27 quoted lines
> This also adds color support to format_decoration()
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> log-tree.c | 60 +++++++++++++++++++++++++---------------
> log-tree.h | 3 ++
> pretty.c | 19 +------------
> t/t4207-log-decoration-colors.sh | 8 +++---
> 4 files changed, 45 insertions(+), 45 deletions(-)
>
> diff --git a/log-tree.c b/log-tree.c
> index 5dc45c4..7467a1d 100644
> --- a/log-tree.c
> +++ b/log-tree.c
> @@ -174,36 +174,50 @@ static void show_children(struct rev_info *opt, struct commit *commit, int abbre
> }
> }
>
> -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?
Show 8 quoted lines
> 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)?
Show 19 quoted lines
> + 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?
Show 40 quoted lines
> +}
> +
> +void show_decorations(struct rev_info *opt, struct commit *commit)
> +{
> + struct strbuf sb = STRBUF_INIT;
>
> if (opt->show_source && commit->util)
> printf("\t%s", (char *) commit->util);
> if (!opt->show_decorations)
> return;
> - decoration = lookup_decoration(&name_decoration, &commit->object);
> - if (!decoration)
> - return;
> - prefix = " (";
> - while (decoration) {
> - printf("%s", prefix);
> - fputs(decorate_get_color_opt(&opt->diffopt, decoration->type),
> - stdout);
> - if (decoration->type == DECORATION_REF_TAG)
> - fputs("tag: ", stdout);
> - printf("%s", decoration->name);
> - fputs(color_reset, stdout);
> - fputs(color_commit, stdout);
> - prefix = ", ";
> - decoration = decoration->next;
> - }
> - putchar(')');
> + format_decoration(&sb, commit, opt->diffopt.use_color);
> + fputs(sb.buf, stdout);
> + strbuf_release(&sb);
> }
>
> /*
> @@ -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?
Show 11 quoted lines
> diff --git a/log-tree.h b/log-tree.h > index 9140f48..e6a2da5 100644 > --- a/log-tree.h > +++ b/log-tree.h > @@ -13,6 +13,9 @@ int log_tree_diff_flush(struct rev_info *); > int log_tree_commit(struct rev_info *, struct commit *); > int log_tree_opt_parse(struct rev_info *, const char **, int); > void show_log(struct rev_info *opt); > +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.
Wouldn't it be "format_decorations()", or does it handle only one?