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

From: Wolfgang Müller <wolf@oriole.systems>
Date: Oct 17, 2024, 09:35 UTC
Message-ID: <20241017093528.18157-1-wolf@oriole.systems>
In-Reply-To: <20241011183445.229228-1-wolf@oriole.systems>

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().

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

Thanks-to: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Wolfgang Müller <wolf@oriole.systems>
---
Here's v4 with the trailers fixed and a small improvement to the test.
The commit including the additional test has been dropped.

Thanks!

 builtin/shortlog.c  | 12 ++++++++++++
 t/t4201-shortlog.sh |  4 ++++
 2 files changed, 16 insertions(+)

diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 3ed5c46078..c86b75d981 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -407,6 +407,18 @@ int cmd_shortlog(int argc,
 
 	struct parse_opt_ctx_t ctx;
 
+	/*
+	 * 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);
+
 	git_config(git_default_config, NULL);
 	shortlog_init(&log);
 	repo_init_revisions(the_repository, &rev, prefix);
diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
index c20c885724..5b5d3b637c 100755
--- a/t/t4201-shortlog.sh
+++ b/t/t4201-shortlog.sh
@@ -143,6 +143,10 @@ fuzz()
 	test_grep "too many arguments" out
 '
 
+test_expect_success 'shortlog --author from non-git directory does not segfault' '
+	nongit git shortlog --author=author </dev/null
+'
+
 test_expect_success 'shortlog should add newline when input line matches wraplen' '
 	cat >expect <<\EOF &&
 A U Thor (2):

Range-diff against v3:
1:  4813b458ac ! 1:  1a2959c0de builtin/shortlog: explicitly set hash algo when there is no repo
    @@ Commit message
         Fix this for now by explicitly setting the hash algorithm to SHA1. Also
         add a regression test for the segfault.
     
    -    Signed-off-by: Wolfgang Müller <wolf@oriole.systems>
         Thanks-to: Eric Sunshine <sunshine@sunshineco.com>
    +    Signed-off-by: Wolfgang Müller <wolf@oriole.systems>
     
      ## builtin/shortlog.c ##
     @@ builtin/shortlog.c: int cmd_shortlog(int argc,
    @@ t/t4201-shortlog.sh: fuzz()
      '
      
     +test_expect_success 'shortlog --author from non-git directory does not segfault' '
    -+	echo | nongit git shortlog --author=author
    ++	nongit git shortlog --author=author </dev/null
     +'
     +
      test_expect_success 'shortlog should add newline when input line matches wraplen' '
2:  9dfdc7510f < -:  ---------- shortlog: Test reading a log from a SHA256 repo in a non-git directory

-- 
2.47.0