Re: [PATCH 1/4] log-tree: create for_each_decoration()

From: Junio C Hamano <gitster@pobox.com>
To: "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, johannes.schindelin@gmx.de, me@ttaylorr.com,
	Derrick Stolee <derrickstolee@github.com>
Subject: Re: [PATCH 1/4] log-tree: create for_each_decoration()
Date: Fri, 03 Jun 2022 10:39:52 -0700	[thread overview]
Message-ID: <xmqqa6athcif.fsf@gitster.g> (raw)
In-Reply-To: <4f9f34876413927d819313a70fcdefcad5b35689.1654263472.git.gitgitgadget@gmail.com> (Derrick Stolee via GitGitGadget's message of "Fri, 03 Jun 2022 13:37:49 +0000")

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

>  	const struct name_decoration *decoration;
> +	struct format_decorations_context ctx = {
> +		.sb = sb,
> +		.use_color = use_color,
> +		.prefix = prefix,
> +		.separator = separator,
> +		.suffix = suffix,
> +		.color_commit =	diff_get_color(use_color, DIFF_COMMIT),
> +		.color_reset = decorate_get_color(use_color, DECORATION_NONE),
> +	};
>  
>  	decoration = get_name_decoration(&commit->object);
>  	if (!decoration)
>  		return;
>  
> +	ctx.current_and_HEAD = current_pointed_by_HEAD(decoration);
>  
> +	for_each_decoration(commit, append_decoration, &ctx);

The function for_each_decoration() that does not take decoration but
a commit felt iffy, especially because we already have called
get_name_decoration() to obtain one for commit, and the API forces
us to do that again at the beginning of for_each_decoration().

> +	strbuf_addstr(sb, ctx.color_commit);
> +	strbuf_addstr(sb, ctx.suffix);
> +	strbuf_addstr(sb, ctx.color_reset);
> +}
> +
> +int for_each_decoration(const struct commit *c, decoration_fn fn, void *data)
> +{
> +	const struct name_decoration *decoration;
> +
> +	decoration = get_name_decoration(&c->object);
> +	while (decoration) {
> +		int res;
> +		if ((res = fn(decoration, data)))
> +			return res;
>  		decoration = decoration->next;
>  	}
> +
> +	return 0;
>  }

We'll know if this small waste is worth it when we see the new
caller(s), I guess, but even if they start from commit, allowing
them the same early return trick would require this piece of code:

	decoration = get_name_decoration(&commit->object);
	if (!decoration)
		return;

in them, and even if they do not need it, it would be trivial for
them to say

	for_each_decoration(get_name_decoration(&commit->object),
        		    do_whatever_they_need_to_do, &ctx);

so, we may want to revisit this after we finish reading the series
through.  Making the iterator take name_decoration does not look
too bad.

>  void show_decorations(struct rev_info *opt, struct commit *commit)
> diff --git a/log-tree.h b/log-tree.h
> index e7e4641cf83..ea07da2625b 100644
> --- a/log-tree.h
> +++ b/log-tree.h
> @@ -35,4 +35,8 @@ void fmt_output_commit(struct strbuf *, struct commit *, struct rev_info *);
>  void fmt_output_subject(struct strbuf *, const char *subject, struct rev_info *);
>  void fmt_output_email_subject(struct strbuf *, struct rev_info *);
>  
> +typedef int decoration_fn(const struct name_decoration *d,
> +			  void *data);
> +int for_each_decoration(const struct commit *c, decoration_fn fn, void *data);
> +
>  #endif