Re: [PATCH v2 8/8] shortlog: allow multiple groups to be specified

On Sun, Sep 27, 2020 at 02:18:10PM -0700, Junio C Hamano wrote:

Jeff King [off-list ref] writes:

quoted

-		if (strcasecmp(iter.key.buf, log->trailer))
+		if (!string_list_has_string(&log->trailers, iter.key.buf))
 			continue;

...

quoted

+	if (!log.groups)
+		log.groups = SHORTLOG_GROUP_AUTHOR;
+	string_list_sort(&log.trailers);

I initially reacted with "oh, why sort?" to this line, before
realizing that the list is used as a look-up table, which, if I
recall correctly, you said you want to see us move off of in the
longer term.

As we already have the string-set in this series, I am wondering
why we are not using it.  It's not like the code at some point
needs to iterate over log.trailers in some stable order, right?

Right, it's purely a lookup, and the new strset or any hashmap would
work.  However, the trailers list is part of the public shortlog.h[1]
interface, so we'd have to move strset to be a public thing, too. I
think that may be where we want to go eventually, but I thought I'd
trial it as something strictly internal to shortlog.c for now.

I'm also not _too_ bothered by this particular use of string-list as a
lookup table. It's one of the "easy" cases: we build up the list in one
phase, and then do lookups in the other, so sorting in between is no big
deal (though I do think a strset is even easier, as you do not have to
remember to sort between the phases).

-Peff