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

Commit Message

Derrick Stolee June 3, 2022, 1:37 p.m. UTC

From: Derrick Stolee <[e-mail]>

It can be helpful to iterate through all the decorations on a commit
without necessarily writing them to a stream. Implement
for_each_decoration() and reimplement format_decorations_extended() to
use that iterator.

Signed-off-by: Derrick Stolee <[e-mail]>
---
 log-tree.c | 111 ++++++++++++++++++++++++++++++++++++-----------------
 log-tree.h |   4 ++
 2 files changed, 80 insertions(+), 35 deletions(-)

Comments

Junio C Hamano June 3, 2022, 5:39 p.m. UTC | #1

"Derrick Stolee via GitGitGadget" <[e-mail]> 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

Derrick Stolee June 3, 2022, 5:58 p.m. UTC | #2

On 6/3/2022 1:39 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <[e-mail]> 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().

This is more an issue with this particular caller since it needs
that current_pointed_by_HEAD() information. I had considered it
as something to include in the prototype of decoration_fn so it
could be incorporated into for_each_decoration(), but it ended up
being overly clunky for this one consumer. I'm open to other ideas,
though.

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

The goal of this method was to make it super simple to iterate
without doing any prep work: just load the commit and go.

>> +{
>> +	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;

The iterator returns quickly because the while loop notices a
NULL decoration. This use in format_decorations_extended() needs
that check because of the current_pointed_by_HEAD(decoration)
call based on the result being non-NULL.

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

You are right that this would work, but it's a bit messier. We
could go this route to avoid the duplicate get_name_decoration()
calls in format_decorations_extended().

Thanks,
-Stolee

Junio C Hamano June 3, 2022, 6:40 p.m. UTC | #3

Derrick Stolee <[e-mail]> writes:

>>> +	for_each_decoration(commit, append_decoration, &ctx);
>> 
>> The function for_each_decoration() that does not take decoration but
>> a commit felt iffy,...
>> ...
>> 	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.
>
> You are right that this would work, but it's a bit messier. We
> could go this route to avoid the duplicate get_name_decoration()
> calls in format_decorations_extended().

Having a convenience function that takes a commit and let you walk
over its decoration is OK, but calling it for_each_decoration()
bothers me somewhat (which is where my comment started from after
all).

Hopefully it will stay unambiguous, and it will stay to be
unnecessary to name it for_each_decoration_for_commit(), as I do not
think we'd add "decoration" to things that are not commits (or if we
added one, we probably will call it differently from "decoration").

So, OK.

Patch

diff --git a/log-tree.c b/log-tree.c
index d0ac0a6327a..b15a7c9db22 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -282,6 +282,54 @@  static void show_name(struct strbuf *sb, const struct name_decoration *decoratio
 		strbuf_addstr(sb, decoration->name);
 }
 
+struct format_decorations_context {
+	struct strbuf *sb;
+	int use_color;
+	const char *prefix;
+	const char *separator;
+	const char *suffix;
+	const char *color_commit;
+	const char *color_reset;
+	const struct name_decoration *current_and_HEAD;
+};
+
+static int append_decoration(const struct name_decoration *d,
+			     void *data)
+{
+	struct format_decorations_context *ctx = data;
+	/*
+	 * When both current and HEAD are there, only
+	 * show HEAD->current where HEAD would have
+	 * appeared, skipping the entry for current.
+	 */
+	if (d != ctx->current_and_HEAD) {
+		strbuf_addstr(ctx->sb, ctx->color_commit);
+		strbuf_addstr(ctx->sb, ctx->prefix);
+		strbuf_addstr(ctx->sb, ctx->color_reset);
+		strbuf_addstr(ctx->sb, decorate_get_color(ctx->use_color, d->type));
+		if (d->type == DECORATION_REF_TAG)
+			strbuf_addstr(ctx->sb, "tag: ");
+
+		show_name(ctx->sb, d);
+
+		if (ctx->current_and_HEAD &&
+		    d->type == DECORATION_REF_HEAD) {
+			strbuf_addstr(ctx->sb, " -> ");
+			strbuf_addstr(ctx->sb, ctx->color_reset);
+			strbuf_addstr(ctx->sb,
+				      decorate_get_color(
+						ctx->use_color,
+						ctx->current_and_HEAD->type));
+			show_name(ctx->sb, ctx->current_and_HEAD);
+		}
+		strbuf_addstr(ctx->sb, ctx->color_reset);
+
+		ctx->prefix = ctx->separator;
+	}
+
+	return 0;
+}
+
 /*
  * The caller makes sure there is no funny color before calling.
  * format_decorations_extended makes sure the same after return.
@@ -294,49 +342,42 @@  void format_decorations_extended(struct strbuf *sb,
 			const char *suffix)
 {
 	const struct name_decoration *decoration;
-	const struct name_decoration *current_and_HEAD;
-	const char *color_commit =
-		diff_get_color(use_color, DIFF_COMMIT);
-	const char *color_reset =
-		decorate_get_color(use_color, DECORATION_NONE);
+	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;
 
-	current_and_HEAD = current_pointed_by_HEAD(decoration);
-	while (decoration) {
-		/*
-		 * When both current and HEAD are there, only
-		 * show HEAD->current where HEAD would have
-		 * appeared, skipping the entry for current.
-		 */
-		if (decoration != current_and_HEAD) {
-			strbuf_addstr(sb, color_commit);
-			strbuf_addstr(sb, prefix);
-			strbuf_addstr(sb, color_reset);
-			strbuf_addstr(sb, decorate_get_color(use_color, decoration->type));
-			if (decoration->type == DECORATION_REF_TAG)
-				strbuf_addstr(sb, "tag: ");
-
-			show_name(sb, decoration);
-
-			if (current_and_HEAD &&
-			    decoration->type == DECORATION_REF_HEAD) {
-				strbuf_addstr(sb, " -> ");
-				strbuf_addstr(sb, color_reset);
-				strbuf_addstr(sb, decorate_get_color(use_color, current_and_HEAD->type));
-				show_name(sb, current_and_HEAD);
-			}
-			strbuf_addstr(sb, color_reset);
+	ctx.current_and_HEAD = current_pointed_by_HEAD(decoration);
 
-			prefix = separator;
-		}
+	for_each_decoration(commit, append_decoration, &ctx);
+
+	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;
 	}
-	strbuf_addstr(sb, color_commit);
-	strbuf_addstr(sb, suffix);
-	strbuf_addstr(sb, color_reset);
+
+	return 0;
 }
 
 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