Re: [PATCH v2 01/15] rebase -i: handle NULL value for rebase.missingCommitsCheck

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Thomas

On 10/11/2026 07:36, Thomas Bachem via GitGitGadget wrote:
> From: Thomas Bachem <mail@thomasbachem.com>
> 
> A rebase.missingCommitsCheck without a value, as in
> 
>    [rebase]
>    missingCommitsCheck
> 
> or "git -c rebase.missingCommitsCheck", makes "git rebase -i"
> segfault as it writes the todo list: get_missing_commit_check_level()
> passes the NULL value to strcasecmp().
> 
> The setting takes "ignore", "warn" or "error". An unrecognized value,
> perhaps one meant for a newer version of Git, gets a warning and is
> treated as "ignore". Treat a missing value the same way.

Sounds good

> diff --git a/rebase-interactive.c b/rebase-interactive.c
> index 809f76a87b..9eb70b09b3 100644
> --- a/rebase-interactive.c
> +++ b/rebase-interactive.c
> @@ -30,8 +30,14 @@ static enum missing_commit_check_level get_missing_commit_check_level(void)
>   {
>   	const char *value;
>   
> -	if (repo_config_get_value(the_repository, "rebase.missingcommitscheck", &value) ||
> -			!strcasecmp("ignore", value))
> +	if (repo_config_get_value(the_repository, "rebase.missingcommitscheck", &value))
> +		return MISSING_COMMIT_CHECK_IGNORE;
> +	if (!value) {
> +		warning(_("missing value for option "
> +			  "rebase.missingCommitsCheck. Ignoring."));
> +		return MISSING_COMMIT_CHECK_IGNORE;
> +	}

If "value" is NULL  we print a warning and continue as if the user had 
said "ignore" - good

> +	if (!strcasecmp("ignore", value))
>   		return MISSING_COMMIT_CHECK_IGNORE;
>   	if (!strcasecmp("warn", value))
>   		return MISSING_COMMIT_CHECK_WARN;
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index b3ee96f560..5b9fc67c19 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1562,6 +1562,18 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = error' '
>   	test B = $(git cat-file commit HEAD^ | sed -ne \$p)
>   '
>   
> +test_expect_success 'rebase -i warns about and ignores rebase.missingCommitsCheck without a value' '
> +	rebase_setup_and_clean missing-commit &&
> +	(
> +		set_fake_editor &&
> +		FAKE_LINES="1 2 3 4" \
> +			git -c rebase.missingCommitsCheck rebase -i --root 2>actual

Because this uses "--root", it is hard to tell if it is dropping any 
commits or not. Using something like

	FAKE_LINES=1 \
		git -c rebase.missingCommitCheck rebase -i HEAD~3

makes it easier to see we're dropping a commit by looking at FAKE_LINES 
and the upstream commit passed to rebase.


> +	) &&
> +	test D = $(git cat-file commit HEAD | sed -ne \$p) &&

This is quite common in our older tests, but makes it very hard to 
diagnose test failures because it does not print anything if there is an 
error. These days we can use

	test_commit_message HEAD -m D

> +	test_grep "missing value for option rebase.missingCommitsCheck" actual &&
> +	test_grep ! "commits may have been dropped" actual

We check for the error message caused by a missing config value and also 
check we don't complain about the missing commits - good.

Thanks

Phillip

> +'
> +
>   test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = ignore' '
>   	test_config rebase.missingCommitsCheck ignore &&
>   	rebase_setup_and_clean missing-commit &&