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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 11, 2024, 17:54 UTC
Message-ID: <xmqqh6dzwek0.fsf@gitster.g>
In-Reply-To: <7ded51bbce1b23cf4110e3bf0abb7579efd4d344.1718095090.git.ps@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 9 quoted lines

> Fix both of these issues by not making it an error anymore when the
> given length exceeds the hash length. Instead, if we have a repository,
> then we truncate the length to the maximum length of `the_hash_algo`.
> Otherwise, we simply leave the abbreviated length intact and store it
> as-is. This is equivalent to the logic in `parse_opt_abbrev_cb()` and is
> handled just fine by `repo_find_unique_abbrev_r()`. In practice, we
> should never even end up using `default_abbrev` without a repository
> anyway given that abbreviating object IDs to unique prefixes requires us
> to have access to an object database.

Makes sense.

Show 11 quoted lines

> diff --git a/config.c b/config.c
> index abce05b774..ab2844d9e1 100644
> --- a/config.c
> +++ b/config.c
> @@ -1460,11 +1460,14 @@ 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 = startup_info->have_repository ?
> +				the_hash_algo->hexsz : GIT_MAX_HEXSZ;

We will need to have some code that further adjusts overly long default_abbrev when we really have to abbreviate (at which time, hopefully we are already aware of the real hash algorithm used in the repository, and that may be SHA-1) anyway.

So do we even need the conditional here? Can't we just set it to GIT_MAX_HEXSZ here unconditionally?

Show 13 quoted lines

> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index 86c695eb0a..99c063e4cd 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -1237,6 +1237,12 @@ test_expect_success 'log.abbrevCommit configuration' '
>  	test_cmp expect.whatchanged.full actual
>  '
>  
> +test_expect_success 'log.abbrevCommit with --abbrev=9000' '
> +	git log --no-abbrev >expect &&
> +	git log --abbrev-commit --abbrev=9000 >actual &&
> +	test_cmp expect actual
> +'

Interesting. We didn't have coverage for "we want to see full object names" case.

Show 6 quoted lines

> +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
> +'

OK.