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

From: Jeff King <peff@peff.net>
Date: Sep 28, 2020, 03:25 UTC
Message-ID: <20200928032515.GC3059492@coredump.intra.peff.net>
In-Reply-To: <xmqq8scuor59.fsf@gitster.c.googlers.com>

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

Show 18 quoted lines

> 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