Re: [PATCH 1/2] shortlog: introduce `--group-filter` to restrict output

From: Derrick Stolee <derrickstolee@github.com>
Date: Jun 8, 2023, 14:34 UTC
Message-ID: <3778f07f-6e6f-5a39-631d-1266d61b9715@github.com>
In-Reply-To: <5cec04b65d350ca8b482ca14260ef118341e4039.1686178917.git.me@ttaylorr.com>

On 6/7/2023 7:02 PM, Taylor Blau wrote:

Show 11 quoted lines

> This means that you could easily view the hashes of all commits you
> either wrote or co-authored with something like:
> 
>     $ git shortlog -n --group=author --group=trailer:Co-authored-by \
>         --group-filter="$(git config user.name)"
> 
> When filtering just by trailers, it is tempting to want to introduce a
> new grep mode for matching a given trailer, like `--author=<pattern>`
> for matching the author header. But this would not be suitable for the
> above, since we want commits which match either the author or the
> Co-authored-by trailer, not ones which match both.

One thing that is not immediately obvious from reading the patch, but becomes clearer in patch 2, is that your --group-filter is an exact string match. This differs from the --author filter in 'git log' and similar, which is actually a case-insensitive substring match.

Show 7 quoted lines

> +static int want_shortlog_group(struct shortlog *log, const char *group)
> +{
> +	if (!log->group_filter.nr)
> +		return 1;
> +	return string_list_has_string(&log->group_filter, group);
> +}
> +

This is the critical piece of code for this issue. Replacing it with

static int want_shortlog_group(struct shortlog *log, const char *group)
{
	struct string_list_item *item;
	if (!log->group_filter.nr)
		return 1;

	for_each_string_list_item(item, &log->group_filter) {
		if (strcasestr(group, item->string))
			return 1;
	}

	return 0;
}

Results in the case-insensitive substring search that I would expect from this parameter.

This would also solve the problem from Patch 2 where we want to search by email address. Using '-e --group-filter="my@email.com"' works, though it will catch users with 'tammy@email.com' emails, as well.

Something to consider at a high level before committing to this CLI.

Thanks, -Stolee