On Sun, Sep 27, 2020 at 02:18:10PM -0700, Junio C Hamano wrote: > Jeff King <peff@xxxxxxxx> 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
