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

[Jeff King <peff@peff.net>]

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

> Jeff King <peff@peff.net> writes:
> 
> > -		if (strcasecmp(iter.key.buf, log->trailer))
> > +		if (!string_list_has_string(&log->trailers, iter.key.buf))
> >  			continue;
> ...
> > +	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