Re: [PATCH v3 1/2] builtin/shortlog: explicitly set hash algo when there is no repo

From: Taylor Blau <me@ttaylorr.com>
Date: Oct 16, 2024, 19:22 UTC
Message-ID: <ZxASXLz4lA+E9MuQ@nand.local>
In-Reply-To: <20241016182124.48148-2-wolf@oriole.systems>

On Wed, Oct 16, 2024 at 08:21:23PM +0200, Wolfgang Müller wrote:

Show 12 quoted lines

> Whilst git-shortlog(1) does not explicitly need any repository
> information when run without reference to one, it still parses some of
> its arguments with parse_revision_opt() which assumes that the hash
> algorithm is set. However, in c8aed5e8da (repository: stop setting SHA1
> as the default object hash, 2024-05-07) we stopped setting up a default
> hash algorithm and instead require commands to set it up explicitly.
>
> This was done for most other commands like in ab274909d4 (builtin/diff:
> explicitly set hash algo when there is no repo, 2024-05-07) but was
> missed for builtin/shortlog, making git-shortlog(1) segfault outside of
> a repository when given arguments like --author that trigger a call to
> parse_revision_opt().

Good analysis.

> Fix this for now by explicitly setting the hash algorithm to SHA1. Also
> add a regression test for the segfault.

Makes sense.

> Signed-off-by: Wolfgang Müller <wolf@oriole.systems>
> Thanks-to: Eric Sunshine <sunshine@sunshineco.com>

In the future, please ensure that your Signed-off-by is the last trailer in the section to indicate that you have certified everything above it (which in that case would include your Thanks-to here).

Show 8 quoted lines

> @@ -143,6 +143,10 @@ fuzz()
>  	test_grep "too many arguments" out
>  '
>
> +test_expect_success 'shortlog --author from non-git directory does not segfault' '
> +	echo | nongit git shortlog --author=author
> +'
> +

Instead of 'echo |', I wonder if it would be just as good to write this test as:

    nongit git shortlog --author=author </dev/null

Thanks, Taylor