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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 27, 2020, 21:18 UTC
Message-ID: <xmqq8scuor59.fsf@gitster.c.googlers.com>
In-Reply-To: <20200927084015.GH2465761@coredump.intra.peff.net>

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?

I do realize that going to hashmap might be overkill, but once we have an easy-to-use wrapper around it, between one "table of strings" API and another "table of strings" API, I do not see a reason why we want to choose the string_list.

Thanks.