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

[Junio C Hamano <gitster@pobox.com>]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

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

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

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

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