Re: [PATCH v2 1/3] config: fix segfault when parsing "core.abbrev" without repo

> The "core.abbrev" config allows the user to specify the minimum length
> when abbreviating object hashes. Next to the values "auto" and "no",
> this config also accepts a concrete length that needs to be bigger or
> equal to the minimum length and smaller or equal to the hash algorithm's
> hex length. While the former condition is trivial, the latter depends on
> the object format used by the current repository. It is thus a variable
> upper boundary that may either be 40 (SHA-1) or 64 (SHA-256).
>
> This has two major downsides. First, the user that specifies this config
> must be aware of the object hashes that its repository use. If they want
> to configure the value globally, then they cannot pick any value in the
> range `[41, 64]` if they have any repository that uses SHA-1. If they
> did, Git would error out when parsing the config.
>
> Second, and more importantly, parsing "core.abbrev" crashes when outside
> of a Git repository because we dereference `the_hash_algo` to figure out
> its hex length. Starting with c8aed5e8da (repository: stop setting SHA1
> as the default object hash, 2024-05-07) though, we stopped initializing
> `the_hash_algo` outside of Git repositories.
>
> Fix both of these issues by not making it an error anymore when the
> given length exceeds the hash length. Instead, leave the abbreviated
> length intact. `repo_find_unique_abbrev_r()` handles this just fine
> except for a performance penalty which we will fix in a subsequent
> commit.
>
> Reported-by: Kyle Lippincott <spectral@google.com>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  config.c         |  4 ++--
>  t/t4202-log.sh   | 12 ++++++++++++
>  t/t5601-clone.sh |  7 +++++++
>  3 files changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/config.c b/config.c
> index abce05b774..0416b0f2b6 100644
> --- a/config.c
> +++ b/config.c
> @@ -1460,10 +1460,10 @@ static int git_default_core_config(const char *var, const char *value,
>  		if (!strcasecmp(value, "auto"))
>  			default_abbrev = -1;
>  		else if (!git_parse_maybe_bool_text(value))
> -			default_abbrev = the_hash_algo->hexsz;
> +			default_abbrev = GIT_MAX_HEXSZ;

>  		else {
>  			int abbrev = git_config_int(var, value, ctx->kvi);
> -			if (abbrev < minimum_abbrev || abbrev > the_hash_algo->hexsz)
> +			if (abbrev < minimum_abbrev)
>  				return error(_("abbrev length out of range: %d"), abbrev);
>  			default_abbrev = abbrev;
>  		}
>

> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index 86c695eb0a..e97826458c 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -1237,6 +1237,18 @@ test_expect_success 'log.abbrevCommit configuration' '
>  	test_cmp expect.whatchanged.full actual
>  '
>
> +test_expect_success '--abbrev-commit with core.abbrev=false' '
> +	git log --no-abbrev >expect &&
> +	git -c core.abbrev=false log --abbrev-commit >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success '--abbrev-commit with core.abbrev=9000' '
> +	git log --no-abbrev >expect &&
> +	git -c core.abbrev=9000 log --abbrev-commit >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'show added path under "--follow -M"' '
>  	# This tests for a regression introduced in v1.7.2-rc0~103^2~2
>  	test_create_repo regression &&
> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
> index cc0b953f14..5d7ea147f1 100755
> --- a/t/t5601-clone.sh
> +++ b/t/t5601-clone.sh
> @@ -46,6 +46,13 @@ test_expect_success 'output from clone' '
>  	test $(grep Clon output | wc -l) = 1
>  '
>
> +test_expect_success 'output from clone with core.abbrev does not crash' '
> +	rm -fr dst &&
> +	echo "Cloning into ${SQ}dst${SQ}..." >expect &&
> +	git -c core.abbrev=12 clone -n "file://$(pwd)/src" dst >actual 2>&1 &&
> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'clone does not keep pack' '
>
>  	rm -fr dst &&
> --
> 2.45.2.457.g8d94cfb545.dirty