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

[Derrick Stolee <derrickstolee@github.com>]

On 6/3/2022 1:39 PM, Junio C Hamano wrote:
> "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().

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