Re: [RFC PATCH v2] builtin/shortlog: explicitly set hash algo when there is no repo

From: Patrick Steinhardt <ps@pks.im>
Date: Oct 16, 2024, 05:32 UTC
Message-ID: <Zw9P90_QtiAAH3yz@pks.im>
In-Reply-To: <20241015114826.715158-1-wolf@oriole.systems>

On Tue, Oct 15, 2024 at 01:48:26PM +0200, Wolfgang Müller wrote:

Show 12 quoted lines

> +	/*
> +	 * NEEDSWORK: Later on we'll call parse_revision_opt which relies on
> +	 * the hash algorithm being set but since we are operating outside of a
> +	 * Git repository we cannot determine one. This is only needed because
> +	 * parse_revision_opt expects hexsz for --abbrev which is irrelevant
> +	 * for shortlog outside of a git repository. For now explicitly set
> +	 * SHA1, but ideally the parsing machinery would be split between
> +	 * git/nongit so that we do not have to do this.
> +	 */
> +	if (nongit && !the_hash_algo)
> +		repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
> +

Makes sense. As you say, git-shortlog(1) should ideally recognize the hash function used by the input. The next-best thing would be to provide a command line option to switch it. But punting on that should be fine for now.

Show 16 quoted lines

>  	const struct option options[] = {
>  		OPT_BIT('c', "committer", &log.groups,
>  			N_("group by committer rather than author"),
> diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
> index c20c885724..ed39c67ba1 100755
> --- a/t/t4201-shortlog.sh
> +++ b/t/t4201-shortlog.sh
> @@ -143,6 +143,11 @@ fuzz()
>  	test_grep "too many arguments" out
>  '
>  
> +test_expect_success 'shortlog --author from non-git directory does not segfault' '
> +	git log --no-expand-tabs HEAD >log &&
> +	env GIT_DIR=non-existing git shortlog --author=author <log 2>out
> +'
> +

I'd like to see another testcase added that exercises behaviour when git-shortlog(1) is passed SHA256 output outside of a repo, as I'm curious how it'll behave in that scenario.

Thanks!

Patrick