On 6/3/2022 1:39 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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
